rusackas commented on code in PR #40653:
URL: https://github.com/apache/superset/pull/40653#discussion_r3338268040
##########
superset/mcp_service/jwt_verifier.py:
##########
@@ -54,6 +54,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"})
+
+
+def _warn_on_weak_jwt_config(
+ audience: Any,
+ algorithm: Any,
+) -> None:
+ """Emit startup warnings when a JWT verifier is configured permissively.
+
+ These are config-gated soft warnings, not hard failures: a verifier is only
+ ever constructed when ``MCP_AUTH_ENABLED`` is True and JWT keys are present
+ (see ``create_default_mcp_auth_factory``). We warn — rather than refuse to
+ start — so existing single-service deployments that intentionally omit an
+ audience or rely on JWKS-advertised algorithms keep working. Operators who
+ want strict enforcement should set ``MCP_JWT_AUDIENCE`` and
+ ``MCP_JWT_ALGORITHM``.
+ """
+ if not audience:
+ logger.warning(
+ "MCP JWT verifier configured without an audience "
+ "(MCP_JWT_AUDIENCE unset): audience validation is DISABLED. "
+ "Tokens minted for other services may be accepted. Set "
+ "MCP_JWT_AUDIENCE to bind tokens to this service."
+ )
+ if not algorithm:
+ logger.warning(
+ "MCP JWT verifier configured without a pinned signing algorithm "
+ "(MCP_JWT_ALGORITHM unset): the algorithm header is not pinned. "
+ "Set MCP_JWT_ALGORITHM to the algorithm your IdP uses. Unsigned "
+ "('none') tokens are always rejected regardless of this setting."
+ )
Review Comment:
Correct. The factory always supplies a non-empty algorithm (RS256 default),
so the warning path for a missing algorithm is unreachable in the
default-factory path. The warning is still useful for custom factory
configurations where an algorithm is not provided. The PR description may
overstate the coverage; the warning guards an edge case in operator-supplied
factories.
##########
superset/mcp_service/auth.py:
##########
@@ -66,6 +66,76 @@
METHOD_PERMISSION_ATTR = "_method_permission_name"
+def _sanitize_iss_for_log(value: Any) -> str:
+ """Escape control characters in an issuer claim before logging.
+
+ The ``iss`` claim is attacker-controlled and may contain newlines that
+ could forge or split log lines; collapse them to keep one log entry.
+ """
+ return (
+ str(value)
+ .replace("\\", "\\\\")
+ .replace("\n", "\\n")
+ .replace("\r", "\\r")
+ .replace("\t", "\\t")
+ )
+
+
+# Maps a tool's method permission (read/write/delete) 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 that function.
+_METHOD_TO_REQUIRED_SCOPE = {
+ "read": "superset:read",
+ "write": "superset:write",
+ "delete": "superset:write",
+}
+
+
+def _get_token_scopes() -> set[str] | None:
+ """Return the set of scopes on the current JWT access token, or None.
+
+ Returns None when there is no JWT context or the token carries no scopes,
+ so callers can fall back to RBAC-only behavior (back-compat for API-key and
+ scope-less JWT deployments). Returns a (possibly populated) set only when
+ the token explicitly advertises scopes.
+ """
+ try:
+ from fastmcp.server.dependencies import get_access_token
+ except ImportError:
+ return None
+
+ try:
+ access_token = get_access_token()
+ except Exception: # noqa: BLE001 - no JWT context for this request
+ return None
+
+ if access_token is None:
+ return None
+
+ scopes = getattr(access_token, "scopes", None)
+ if not scopes:
+ # Token present but no scopes advertised: do NOT enforce scope checks.
+ return None
+ return {str(s) for s in scopes}
+
+
+def _token_scope_allows(method_permission_name: str) -> bool:
+ """Return whether the current token's scopes permit the given method.
+
+ Back-compat: returns True (allow) when the token carries no scopes or there
+ is no JWT context, so deployments not using scopes keep RBAC-only behavior.
+ Only when the token advertises scopes is the mapped required scope
enforced.
+ """
+ token_scopes = _get_token_scopes()
+ if token_scopes is None:
+ return True
+ required_scope = _METHOD_TO_REQUIRED_SCOPE.get(method_permission_name)
+ if required_scope is None:
+ return True
Review Comment:
Acknowledged. Tools using custom method permissions like execute_sql_query
fall outside the scope map and are effectively not scope-checked even when a
scoped token is presented. The current implementation enforces scopes only for
mapped standard permissions. Adding scope mappings for SQL execution and
deny-by-default for unmapped methods is the right hardening — will address in a
follow-up.
##########
superset/mcp_service/jwt_verifier.py:
##########
@@ -368,6 +405,15 @@ class MCPJWTVerifier(JWTVerifier):
page is active regardless of which verifier variant is configured.
"""
+ def __init__(self, *args: Any, **kwargs: Any) -> None:
+ super().__init__(*args, **kwargs)
+ # Surface permissive auth configuration at startup. Config-gated:
+ # a verifier is only built when auth is enabled (see mcp_config).
+ _warn_on_weak_jwt_config(
+ audience=getattr(self, "audience", None),
+ algorithm=getattr(self, "algorithm", None),
+ )
Review Comment:
Same as the Copilot comment above — the warning is derived from post-init
verifier state, so upstream defaults (RS256 from the factory) mask an
unconfigured algorithm. The intent is to warn on operator-level
misconfigurations in custom factory paths. This is a known limitation of
checking post-initialization state rather than raw config values.
--
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]
