aminghadersohi commented on code in PR #44146:
URL: https://github.com/apache/superset/pull/44146#discussion_r4058345717
##########
superset/core/mcp/core_mcp_injection.py:
##########
@@ -210,7 +210,10 @@ def decorator(func: F) -> F:
if protect:
from superset.mcp_service.auth import mcp_auth_hook
- wrapped_func = mcp_auth_hook(func)
+ # Pass the registered (extension-prefixed) name so call-time
+ # checks keyed on tool identity cannot be confused by an
+ # extension tool that shares a base name with a host tool.
+ wrapped_func = mcp_auth_hook(func, tool_name=tool_name)
Review Comment:
The mechanism is described correctly, but this is the intended fail-closed
behavior rather than a defect, so I'm not changing the code here.
Extension tools *should* refuse under `MCP_DATASET_ROLE_ALLOWLIST`.
`SCOPED_TOOLS` is a deny-by-default allowlist of paths whose dataset lineage
Superset can attribute to a registered dataset UUID. A third-party extension
tool is not one of those: Superset cannot vouch that extension-owned code
resolves its arguments to a registered dataset, and it may reach data through
SQL, cached results, or an external source. Refusing is the same call the doc
makes for SQL Lab, chart data, and previews.
Both suggested remedies would reopen exactly the collision the
`tool_name=tool_name` line exists to close. Passing the bare name for extension
context (or stripping the prefix in `dataset_scope.py`) would let
`extensions.pub.ext.query_dataset` — arbitrary extension code — inherit the
built-in `query_dataset`'s allowlisted status purely by naming its function the
same thing. That turns the allowlist into something any installed extension can
opt itself into.
This is already stated and pinned:
- `docs/admin_docs/configuration/mcp-server.mdx`: "Every other
**authenticated** tool refuses in this mode, including SQL Lab, chart/dashboard
previews and data, cached query results, external semantic sources, mutations,
and extension tools."
-
`tests/unit_tests/mcp_service/test_dataset_scope.py::test_unscopable_operations_refuse`
covers `extensions.acme.demo.query_dataset` specifically, with the docstring
"An extension tool that reuses a scoped tool's base name registers under its
prefixed name and must not inherit the built-in tool's handling."
Refusals here are readable, not silent: unsupported tools stay listed and
return `UNSUPPORTED_TOOL_ERROR` when called, so the assistant gets an
explanation it can pass on. An extension that needs to serve dataset reads
under scoped mode is a deliberate design question for a follow-up, not a
prefix-normalization fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
