Abdulrehman-PIAIC80387 opened a new pull request, #40412:
URL: https://github.com/apache/superset/pull/40412
### SUMMARY
Fixes #39395. Implements the three structural safeguards @mistercrunch
outlined in the issue body, plus a single allowlist for the one
intentionally-public tool.
The race-condition fix in #39385 only protects tools that actually go
through `mcp_auth_hook`. This PR makes that invariant **enforceable** at
startup rather than convention-based.
### Changes
**1. `superset/mcp_service/auth.py`** — stamp a marker on the wrapper at the
end of `mcp_auth_hook`:
```python
new_wrapper._mcp_auth_protected = True
```
**2. `superset/core/mcp/core_mcp_injection.py`** — fail-fast in
`create_tool_decorator`: the prior path silently returned the unwrapped
function on error, which is exactly one of the bypass paths the issue calls out.
```python
except Exception as e:
logger.error("Failed to register MCP tool %s: %s", name or
func.__name__, e)
raise # was: return func
```
**3. `superset/mcp_service/app.py`** — `assert_all_tools_protected(mcp)`
invoked at the end of `init_fastmcp_server()`. Iterates
`mcp.local_provider._components`, filters to `tool:` entries (FastMCP 3.x keys
all components in one dict), and raises a `RuntimeError` for any tool whose
`.fn` lacks the marker.
The original spec used `mcp._tool_manager._tools` — that attribute doesn't
exist on FastMCP 3.x (the version pinned in `pyproject.toml`). The current
iteration path is the closest stable equivalent. `local_provider.remove_tool()`
is already used elsewhere in the codebase (`MCP_DISABLED_TOOLS` flow), so
reaching into `local_provider` matches existing practice.
**4. `ALLOWED_UNPROTECTED`** in `app.py` — contains a single entry,
`generate_bug_report`, which is the only tool in the codebase deliberately
decorated with `@tool(protect=False)` (so users can collect diagnostics even
when auth itself is broken).
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/mcp_service/test_auth_safeguards.py -v
```
Five new tests cover:
- `test_mcp_auth_hook_stamps_protection_marker` — wrapper carries the marker
- `test_assert_all_tools_protected_passes_when_every_tool_is_marked` — happy
path
- `test_assert_all_tools_protected_raises_on_unprotected_tool` — bypass
paths blow up
- `test_assert_all_tools_protected_respects_allowlist` — allowlist works as
the only legitimate escape hatch
- `test_create_tool_decorator_fails_fast_on_registration_error` —
silent-fallback path is gone
Existing `test_mcp_tool_registration.py` (13 tests) continues to pass — the
new assertion runs as part of `init_fastmcp_server()` against the real tool
registry and validates that every shipped tool except `generate_bug_report` is
properly wrapped.
```
5 passed (new safeguards tests)
13 passed (existing regression check)
```
### ADDITIONAL INFORMATION
- [x] Has associated issue: #39395
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
