fix(mcp): add safeguards to ensure all MCP tools are wrapped with mcp_auth_hook

[Abdulrehman-PIAIC80387]

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:

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.

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

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

• Has associated issue: fix(mcp): add safeguards to ensure all MCP tools are wrapped with mcp_auth_hook #39395
• Required feature flags:
• Changes UI
• Includes DB Migration
• Introduces new feature or API
• Removes existing feature or API

[bito-code-review]

Code Review Agent Run #54e736

Actionable Suggestions - 0

Review Details

• Files reviewed - 4 · Commit Range: a701608..a701608

  • superset/core/mcp/core_mcp_injection.py
  • superset/mcp_service/app.py
  • superset/mcp_service/auth.py
  • tests/unit_tests/mcp_service/test_auth_safeguards.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codecov]

Codecov Report

❌ Patch coverage is 21.42857% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.19%. Comparing base (fe484f6) to head (a701608).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/mcp_service/app.py	16.66%	10 Missing ⚠️
superset/core/mcp/core_mcp_injection.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #40412      +/-   ##
==========================================
- Coverage   64.20%   64.19%   -0.01%     
==========================================
  Files        2592     2592              
  Lines      139232   139245      +13     
  Branches    32327    32331       +4     
==========================================
+ Hits        89390    89393       +3     
- Misses      48307    48317      +10     
  Partials     1535     1535              

Flag	Coverage Δ
hive	39.24% <21.42%> (-0.01%)	⬇️
mysql	58.75% <21.42%> (-0.01%)	⬇️
postgres	58.83% <21.42%> (-0.01%)	⬇️
presto	40.92% <21.42%> (-0.01%)	⬇️
python	60.39% <21.42%> (-0.01%)	⬇️
sqlite	58.47% <21.42%> (-0.01%)	⬇️
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.