aminghadersohi commented on code in PR #40653:
URL: https://github.com/apache/superset/pull/40653#discussion_r3381095372
##########
superset/mcp_service/jwt_verifier.py:
##########
@@ -354,6 +391,33 @@ class MCPJWTVerifier(JWTVerifier):
page is active regardless of which verifier variant is configured.
"""
+ def __init__(self, *args: Any, **kwargs: Any) -> None:
+ # Capture the explicit algorithm kwarg before super().__init__() can
+ # coerce it (the factory defaults it to "RS256" when MCP_JWT_ALGORITHM
+ # is unset, so self.algorithm is always truthy post-construction).
+ explicit_algorithm = kwargs.get("algorithm")
+ super().__init__(*args, **kwargs)
+ # Surface permissive auth configuration at startup. Config-gated:
+ # a verifier is only built when auth is enabled (see mcp_config).
+ # Prefer the raw MCP_JWT_ALGORITHM config value over the constructor
+ # kwarg because the factory always supplies a non-None algorithm
+ # default; falling back to the kwarg lets unit tests that construct
+ # verifiers directly (without an app context) also get the warning
+ # when no algorithm is pinned.
+ from flask import current_app
+
+ try:
+ config_algorithm = current_app.config.get("MCP_JWT_ALGORITHM")
+ except RuntimeError:
+ # No Flask application context (e.g. unit tests constructing the
+ # verifier directly). Fall back to the explicit constructor arg.
+ config_algorithm = None
+
+ _warn_on_weak_jwt_config(
Review Comment:
The comment documents the key invariant: "the factory defaults it to 'RS256'
when MCP_JWT_ALGORITHM is unset, so self.algorithm is always truthy
post-construction." This invariant is load-bearing for the `alg: null` and
HS256/RS256-confusion defenses (line 514 only fires when `self.algorithm` is
truthy). Worth pinning with a test assertion: `assert verifier.algorithm is not
None` in the fixture setup or in a dedicated test.
##########
superset/mcp_service/jwt_verifier.py:
##########
@@ -57,6 +59,41 @@
logger = logging.getLogger(__name__)
+# Algorithms that are never acceptable for bearer-token verification.
+# "none" (unsigned tokens) must never be honored — accepting it would let any
+# caller forge claims. Comparison is case-insensitive to catch "None"/"NONE".
+_FORBIDDEN_ALGORITHMS = frozenset({"none"})
Review Comment:
`_FORBIDDEN_ALGORITHMS` catches string `"none"` case-insensitively — good.
Note that a JWT header with `"alg": null` (JSON null → Python `None`) bypasses
this check (`isinstance(None, str)` is False) and also bypasses the mismatch
check at line 514 if `self.algorithm` is ever falsy. In production the factory
always coerces `self.algorithm` to a non-null default (documented in the
`__init__` comment), so this is safe in practice. A test for `alg: null` and an
assertion `assert verifier.algorithm is not None` after construction would make
the invariant explicit.
##########
superset/mcp_service/auth.py:
##########
@@ -90,6 +93,81 @@ class MCPNoAuthSourceError(ValueError):
"""
+# Maps a tool's method permission to the OAuth-style token scope it requires.
+# Used by ``check_tool_permission`` for scope-aware authorization: enforcement
+# is the INTERSECTION of token scopes and DB RBAC. Only applied when the token
+# actually carries scopes — see ``_token_scope_allows``.
+#
+# SECURITY: this map must cover EVERY method permission used by an MCP tool.
+# A scoped token presented for a method that is NOT in this map is denied
+# (fail closed) rather than allowed, so adding a tool with a new custom
+# permission cannot silently bypass scope enforcement. ``execute_sql_query``
+# is a privileged, write-class operation and therefore requires the write
+# scope. When introducing a new method permission, add it here.
+_METHOD_TO_REQUIRED_SCOPE = {
Review Comment:
`_METHOD_TO_REQUIRED_SCOPE`: the fail-closed behavior for unmapped methods
(line 155-167) is the right design. One operational note: when a new tool
introduces a custom method permission (e.g. `execute_privileged_export`), a
scoped-token deployment will silently deny it until the map is updated. The
SECURITY comment at the top already calls this out — just flagging that the
WARNING log at line 162-166 is the operator's only signal, so log monitoring
for that message is worth including in deployment docs.
--
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]
