fix(mcp): enforce audience, algorithm, issuer binding, and token scopes (strict mode)#40653
fix(mcp): enforce audience, algorithm, issuer binding, and token scopes (strict mode)#40653rusackas wants to merge 8 commits into
Conversation
|
Bito Automatic Review Skipped - Branch Excluded |
dosubot
Bot
added
authentication
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40653 +/- ##
==========================================
- Coverage 64.19% 64.11% -0.09%
==========================================
Files 2666 2667 +1
Lines 143991 144188 +197
Branches 33108 33146 +38
==========================================
+ Hits 92428 92439 +11
- Misses 49950 50136 +186
Partials 1613 1613
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Hardens Superset’s MCP service authentication/authorization pipeline by tightening JWT verification behavior and adding scope-aware tool authorization, while preserving backward compatibility via config-gated “strict mode” behavior and warnings.
Changes:
- Reject unsigned JWTs (
alg=none) during token loading and add startup warnings for weak JWT verifier configuration. - Add scope-aware authorization to MCP tool RBAC checks (token scopes ∩ DB RBAC), enforced only when scopes are advertised.
- Warn on potentially ambiguous user resolution when multiple JWT issuers are configured without an issuer-aware user resolver; add unit tests covering these behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
superset/mcp_service/jwt_verifier.py |
Adds forbidden-algorithm rejection and weak-config warning helper invoked during verifier initialization. |
superset/mcp_service/auth.py |
Adds token-scope extraction + scope/RBAC intersection enforcement and multi-issuer resolver warning. |
tests/unit_tests/mcp_service/test_jwt_verifier.py |
Adds tests for alg=none rejection and weak-config warning behavior. |
tests/unit_tests/mcp_service/test_auth_rbac.py |
Adds tests for scope-aware authorization outcomes and RBAC-only fallback behavior. |
tests/unit_tests/mcp_service/test_auth_user_resolution.py |
Adds tests validating the multi-issuer warning behavior and suppression with a custom resolver. |
|
The warning about 'algorithm not pinned' not triggering in normal configuration appears to be a design choice in the implementation. Since |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
rusackas
force-pushed
the
fix/mcp-strict-auth
branch
from
c4b02ed to
747bfd2
Compare
rusackas
force-pushed
the
fix/mcp-strict-auth
branch
from
747bfd2 to
967f32c
Compare
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #2e13f4
Actionable Suggestions - 1
-
tests/unit_tests/mcp_service/test_auth_rbac.py - 1
- Test helper misplaced in test file · Line 351-366
Review Details
-
Files reviewed - 5 · Commit Range:
20d42f4..967f32c- superset/mcp_service/auth.py
- superset/mcp_service/jwt_verifier.py
- tests/unit_tests/mcp_service/test_auth_rbac.py
- tests/unit_tests/mcp_service/test_auth_user_resolution.py
- tests/unit_tests/mcp_service/test_jwt_verifier.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
AI Code Review powered by
rusackas
force-pushed
the
fix/mcp-strict-auth
branch
from
967f32c to
bac78d7
Compare
There was a problem hiding this comment.
Code Review Agent Run #5970fa
Actionable Suggestions - 1
-
superset/mcp_service/auth.py - 1
- CWE-755: Duplicate Sanitization · Line 80-92
Additional Suggestions - 2
-
tests/unit_tests/mcp_service/test_auth_rbac.py - 2
-
Scope enforcement untested for visibility path · Line 366-480All 7 new scope-aware tests call `check_tool_permission` directly, bypassing `is_tool_visible_to_current_user`. However, the actual tool-list pipeline calls `is_tool_visible_to_current_user` (auth.py line 337), which internally delegates to `check_tool_permission`. If the `is_tool_visible_to_current_user` call path diverges — e.g., a future change adds logic between line 333–337 — scope enforcement on tool visibility will not be caught by these tests.
Code suggestion
--- a/tests/unit_tests/mcp_service/test_auth_rbac.py +++ b/tests/unit_tests/mcp_service/test_auth_rbac.py @@ -478,3 +478,43 @@ def test_scope_execute_sql_query_requires_write_scope(app_context) -> None: with _patch_token_scopes(["superset:read"]): assert check_tool_permission(func) is False with _patch_token_scopes(["superset:write"]): assert check_tool_permission(func) is True + + +def test_visibility_denied_when_token_lacks_required_scope(app_context) -> None: + """is_tool_visible_to_current_user hides tool when token lacks required scope.""" + g.user = MagicMock(username="editor") + func = _make_tool_func(class_perm="Chart", method_perm="write") + tool = _make_mock_tool(fn=func) + + mock_sm = MagicMock() + mock_sm.can_access = MagicMock(return_value=True) + with ( + patch("superset.security_manager", mock_sm), + _patch_token_scopes(["superset:read"]), + ): + result = is_tool_visible_to_current_user(tool) + + assert result is False + + +def test_visibility_allows_when_token_has_required_scope(app_context) -> None: + """is_tool_visible_to_current_user shows tool when token has required scope.""" + g.user = MagicMock(username="editor") + func = _make_tool_func(class_perm="Chart", method_perm="write") + tool = _make_mock_tool(fn=func) + + mock_sm = MagicMock() + mock_sm.can_access = MagicMock(return_value=True) + with ( + patch("superset.security_manager", mock_sm), + _patch_token_scopes(["superset:read", "superset:write"]), + ): + result = is_tool_visible_to_current_user(tool) + + assert result is True + + +def test_visibility_falls_back_to_rbac_when_no_jwt_context_for_visibility(app_context) -> None: + """is_tool_visible_to_current_user shows tool when no JWT context (RBAC-only).""" + g.user = MagicMock(username="editor") + func = _make_tool_func(class_perm="Chart", method_perm="write") + tool = _make_mock_tool(fn=func) + + mock_sm = MagicMock() + mock_sm.can_access = MagicMock(return_value=True) + with ( + patch("superset.security_manager", mock_sm), + _patch_token_scopes(None), + ): + result = is_tool_visible_to_current_user(tool) + + assert result is True
-
Missing scope test for delete method permission · Line 366-480The `_METHOD_TO_REQUIRED_SCOPE` map at auth.py line 109 maps `delete` → `superset:write`, but the 7 new scope tests never use the `delete` method permission. Without a dedicated test, adding/changing the `delete` entry could silently break without detection.
Code suggestion
--- a/tests/unit_tests/mcp_service/test_auth_rbac.py +++ b/tests/unit_tests/mcp_service/test_auth_rbac.py @@ -478,3 +478,18 @@ def test_scope_execute_sql_query_requires_write_scope(app_context) -> None: with _patch_token_scopes(["superset:read"]): assert check_tool_permission(func) is False with _patch_token_scopes(["superset:write"]): assert check_tool_permission(func) is True + + +def test_scope_delete_requires_write_scope(app_context) -> None: + """Delete method permission requires superset:write scope.""" + g.user = MagicMock(username="editor") + func = _make_tool_func(class_perm="Chart", method_perm="delete") + + mock_sm = MagicMock() + mock_sm.can_access = MagicMock(return_value=True) + with patch("superset.security_manager", mock_sm): + with _patch_token_scopes(["superset:read"]): + assert check_tool_permission(func) is False + with _patch_token_scopes(["superset:write"]): + assert check_tool_permission(func) is True
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/mcp_service/auth.py - 1
- CWE-253: Miscoded Check for Multiple · Line 431-431
Review Details
-
Files reviewed - 5 · Commit Range:
42bec5e..b9a2004- superset/mcp_service/auth.py
- superset/mcp_service/jwt_verifier.py
- tests/unit_tests/mcp_service/test_auth_rbac.py
- tests/unit_tests/mcp_service/test_auth_user_resolution.py
- tests/unit_tests/mcp_service/test_jwt_verifier.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
AI Code Review powered by
Code Review Agent Run #e38378Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…es (strict mode)
Adds config-gated, fail-closed-only-when-configured hardening to the MCP
JWT auth path, with back-compat fallbacks so unconfigured/single-service
deployments keep working:
- Audience: warn at verifier init when no audience is configured while auth
is enabled (validation stays unchanged when an audience IS set).
- Algorithm: always reject unsigned ("none") tokens regardless of pinning;
warn at init when no algorithm is pinned.
- Issuer binding: keep the existing username/email lookup for single-issuer
(the verifier already pins the issuer); warn for multi-issuer configs that
lack an issuer-aware MCP_USER_RESOLVER.
- Token scopes: enforce the intersection of token scopes and DB RBAC, but
ONLY when the token actually advertises scopes — scope-less tokens, API
keys and dev-mode fall back to RBAC-only behavior.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Harden the JWKS key-retrieval step in DetailedJWTVerifier so any failure to fetch verification keys from a remote identity provider results in a clean authentication failure (token rejected) rather than an unhandled 500. The step previously caught only ValueError, relying on the upstream verifier to normalize all transport failures. We now also catch raw httpx errors, asyncio timeouts, and OS-level connection errors so a fetch failure always fails CLOSED and can never be treated as a skipped or successful signature check. Adds parametrized unit tests covering connect/read timeouts, connection refused, non-200 responses, and OS errors, asserting the token is rejected with a generic, non-leaking failure reason. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… attribute
The factory (create_default_mcp_auth_factory) always supplies algorithm='RS256'
when MCP_JWT_ALGORITHM is unset, so self.algorithm is always truthy by the time
_warn_on_weak_jwt_config checks it — the "algorithm not pinned" warning never
fired. Fix by reading current_app.config.get('MCP_JWT_ALGORITHM') directly,
which returns None when the operator didn't explicitly configure an algorithm.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t in MCPJWTVerifier When MCPJWTVerifier is instantiated outside a Flask application context (e.g. in unit tests), current_app.config.get raises RuntimeError. Guard the config lookup with try/except and fall back to the explicit algorithm kwarg so the weak-config warning fires correctly in both app-context and test-context paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…SQL exec Address review feedback: scope enforcement previously failed open for any method permission absent from _METHOD_TO_REQUIRED_SCOPE, so a scoped token could reach tools using custom permissions (notably execute_sql_query) with no scope check. Map execute_sql_query to the write scope and deny-by-default any unmapped method permission when a scoped token is presented, so adding a tool with a new custom permission can no longer silently bypass scope checks. Also move the _patch_token_scopes test helper into the contiguous helper block so a future test inserted before its definition cannot NameError. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the duplicate _sanitize_iss_for_log helper in auth.py, which was byte-for-byte identical to sanitize_for_log in superset/mcp_service/utils/error_sanitization.py (already used by jwt_verifier.py). Import and reuse the shared utility to avoid divergence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…patch target After rebasing onto master, the merged check_tool_permission exceeded the C901 cyclomatic complexity limit; extract the scope-denial logging into a helper. Also correct the security_manager patch target in the scope tests (superset.mcp_service.auth.security_manager, matching the rest of the file). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rusackas
force-pushed
the
fix/mcp-strict-auth
branch
from
0f1786a to
32546c6
Compare
There was a problem hiding this comment.
Code Review Agent Run #912c9e
Actionable Suggestions - 2
-
superset/mcp_service/auth.py - 2
- CWE-117: Log Injection in _log_scope_denial · Line 216-216
- CWE-117: Log Injection in _log_scope_denial (debug path) · Line 225-225
Review Details
-
Files reviewed - 5 · Commit Range:
4699243..32546c6- superset/mcp_service/auth.py
- superset/mcp_service/jwt_verifier.py
- tests/unit_tests/mcp_service/test_auth_rbac.py
- tests/unit_tests/mcp_service/test_auth_user_resolution.py
- tests/unit_tests/mcp_service/test_jwt_verifier.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
AI Code Review powered by
…tion Apply _sanitize_for_log to g.user.username in both the warning and debug branches of _log_scope_denial, consistent with how token_iss is already sanitized in this file (CWE-117). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #7b0f95Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
SUMMARY
Hardens the MCP service's JWT authentication path with four strict-mode enforcements. Each is config-gated and only fails closed when the relevant configuration is set; otherwise it warns and preserves existing behavior, so single-service / unconfigured deployments are not broken. Stacked on
fix/mcp-auth-error-and-logging(same files).Audience enforcement — When
MCP_JWT_AUDIENCEIS configured, audience validation is unchanged. When it is NOT configured, the verifier logs a clear WARNING at init that audience validation is disabled. We chose warn over fail-closed because failing init would break valid single-service deployments that intentionally omit an audience.Algorithm enforcement — Unsigned (
none) tokens are now always rejected inload_access_token, regardless of whether an algorithm is pinned (case-insensitive). Additionally, a WARNING is logged at init when no algorithm is pinned. We did not hard-fail on unpinned algorithm because fastmcp'sJWTVerifieralways coerces an algorithm default, and JWKS-based deployments legitimately rely on advertised algorithms.Issuer-bound user lookup — For single-issuer deployments (the common case) the issuer is already pinned by the verifier, so the existing username/email lookup key is unambiguous and is left unchanged (changing it would break those deployments). For multi-issuer configs (
MCP_JWT_ISSUERis a list) without an issuer-awareMCP_USER_RESOLVER, a WARNING is logged recommending a compound (iss+sub) resolver. This is the least-breaking correct option.Scope-aware tool authorization —
check_tool_permission()now enforces the intersection of token scopes and DB RBAC: the tool method (read/write/delete) maps to a required scope and access is denied if the token lacks it. Critically, this is enforced ONLY when the token actually carries scopes. Scope-less JWTs, API keys, and dev-mode fall back to the current RBAC-only behavior unchanged.Each enforcement fails closed only when the relevant config is set, with explicit back-compat fallbacks documented inline.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend auth hardening, no UI.
TESTING INSTRUCTIONS
Unit tests added/extended (all pass; full
tests/unit_tests/mcp_service/suite green — 2145 passed):test_jwt_verifier.py:nonealgorithm rejected (pinned and unpinned), audience-missing warns, algorithm-unpinned warns, no warning when fully configured.test_auth_rbac.py: scope intersection denies when token lacks the required scope (read & write), allows when scope present, falls back to RBAC when token has no scopes or no JWT context.test_auth_user_resolution.py: multi-issuer warns without a custom resolver; single-issuer and custom-resolver paths do not warn.Run:
pytest tests/unit_tests/mcp_service/test_jwt_verifier.py tests/unit_tests/mcp_service/test_auth_rbac.py tests/unit_tests/mcp_service/test_auth_user_resolution.pyADDITIONAL INFORMATION
🤖 Generated with Claude Code