rusackas commented on code in PR #40653:
URL: https://github.com/apache/superset/pull/40653#discussion_r3382301500
##########
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:
Good catch — addressed. The factory-coercion invariant is now pinned with a
dedicated test (`test_algorithm_invariant_is_pinned_after_construction` asserts
`verifier.algorithm is not None`), and I added `test_algorithm_null_rejected`,
which feeds a header with `"alg": null` (JSON null → Python `None`) and
confirms it is rejected via the mismatch guard ("Algorithm mismatch") since the
verifier always pins a concrete algorithm.
##########
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:
Agreed — pinned. Added
`test_algorithm_invariant_is_pinned_after_construction` asserting
`verifier.algorithm is not None` (and truthy) after construction, so the
load-bearing invariant that the `alg: null` / HS256-RS256-confusion defenses
depend on is now explicit and regression-guarded.
##########
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:
Thanks — agreed the fail-closed-for-unmapped-methods design is the right
one. Your point here is an operational/deployment-docs note (monitor the
WARNING log emitted when a scoped token hits an unmapped method permission)
rather than a code change, so I am leaving the code as-is; I will fold a note
about monitoring that warning into the MCP auth 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]
