codeant-ai-for-open-source[bot] commented on code in PR #40653:
URL: https://github.com/apache/superset/pull/40653#discussion_r3338004249
##########
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:
**Suggestion:** Weak-config warnings are derived from post-initialization
verifier attributes instead of raw constructor inputs, so unset algorithm
configuration can be masked by upstream/default coercion (for example
defaulting to `RS256`). This causes the "algorithm not pinned" warning to be
skipped in real default-factory paths even when `MCP_JWT_ALGORITHM` is not
configured. Evaluate warning conditions from the original kwargs/config before
defaults are applied. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Operators miss warning about unset MCP_JWT_ALGORITHM configuration.
- ⚠️ Weak algorithm pinning may go unnoticed in deployments.
- ⚠️ Troubleshooting misconfigurations harder without explicit startup
warnings.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure MCP auth via `create_default_mcp_auth_factory()` in
`superset/mcp_service/mcp_config.py:7-52` with `MCP_AUTH_ENABLED=True`, a
JWKS URI or
public key set, and **no** `MCP_JWT_ALGORITHM` entry in `app.config`.
2. In this configuration, `create_default_mcp_auth_factory()` builds
`common_kwargs` at
`mcp_config.py:23-37`, setting `common_kwargs["algorithm"] =
app.config.get("MCP_JWT_ALGORITHM", "RS256")` at line 37, so the `algorithm`
keyword
passed to the verifier is `"RS256"` even though `MCP_JWT_ALGORITHM` is unset
in the app
config.
3. The factory then instantiates `MCPJWTVerifier(**common_kwargs)` at
`mcp_config.py:47-50`, which calls `MCPJWTVerifier.__init__()` at
`superset/mcp_service/jwt_verifier.py:408-415`; inside `__init__`,
`_warn_on_weak_jwt_config(audience=getattr(self, "audience", None),
algorithm=getattr(self, "algorithm", None))` is invoked.
4. Because `self.algorithm` was set from the default `"RS256"`,
`_warn_on_weak_jwt_config()` at `jwt_verifier.py:65-92` sees a truthy
`algorithm` and
skips the `if not algorithm:` branch, so the warning message about
"MCP_JWT_ALGORITHM
unset" is never logged even though the configuration did not explicitly pin
an algorithm,
causing the missing-algorithm warning to be silently skipped in this default
factory path.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=91270393055941ec8ec51bcf9ba6836f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=91270393055941ec8ec51bcf9ba6836f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/jwt_verifier.py
**Line:** 408:415
**Comment:**
*Logic Error: Weak-config warnings are derived from post-initialization
verifier attributes instead of raw constructor inputs, so unset algorithm
configuration can be masked by upstream/default coercion (for example
defaulting to `RS256`). This causes the "algorithm not pinned" warning to be
skipped in real default-factory paths even when `MCP_JWT_ALGORITHM` is not
configured. Evaluate warning conditions from the original kwargs/config before
defaults are applied.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40653&comment_hash=b44b71c5c63025f05a46175420287fa0a5577f4d30a8e67396bf2c1d5542027e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40653&comment_hash=b44b71c5c63025f05a46175420287fa0a5577f4d30a8e67396bf2c1d5542027e&reaction=dislike'>👎</a>
##########
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:
**Suggestion:** Scope enforcement currently fails open for any RBAC method
name not present in the scope map. Existing tools use custom method permissions
like `execute_sql_query`, so tokens that do carry scopes can still access those
tools without any scope check as long as RBAC passes. Add an explicit scope
mapping for custom method permissions (at minimum SQL execution) or
deny-by-default when scoped tokens are present and no mapping exists. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Scoped tokens can execute SQL tools without scope restriction.
- ⚠️ Token scopes no longer reflect intended least-privilege semantics.
- ⚠️ Automated clients may overperform privileged SQL operations
unexpectedly.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable MCP auth and SQL Lab tools so the default auth wrapper is used;
tool wrappers
around FastMCP tools call `check_tool_permission()` in
`superset/mcp_service/auth.py:161-259`, and SQL tools are defined with
custom method
permissions (see step 2).
2. Note that the SQL execution tools `execute_sql` and `get_chart_sql` are
decorated with
`method_permission_name="execute_sql_query"` in
`superset/mcp_service/sql_lab/tool/execute_sql.py:19-23` and
`superset/mcp_service/chart/tool/get_chart_sql.py:8-12`, and the Superset
MCP instructions
document this separate permission in `superset/mcp_service/app.py:5-12`.
3. Issue a JWT access token whose `scopes` claim is populated (so
`_get_token_scopes()`
returns a non-None set) and call the `execute_sql` MCP tool; the call path
enters
`check_tool_permission()` at `superset/mcp_service/auth.py:161-218`, which
computes
`method_permission_name="execute_sql_query"` and verifies RBAC via
`security_manager.can_access("can_execute_sql_query", "SQLLab")`.
4. During that same check, `_token_scope_allows()` at
`superset/mcp_service/auth.py:123-136` is called with
`method_permission_name="execute_sql_query"`; because
`_METHOD_TO_REQUIRED_SCOPE` at
`auth.py:88-92` only maps `"read"`, `"write"`, and `"delete"`,
`required_scope =
_METHOD_TO_REQUIRED_SCOPE.get(method_permission_name)` at line 133 returns
`None`, causing
the `if required_scope is None: return True` branch at lines 134-135 to
bypass scope
enforcement entirely, so even scoped tokens (with limited scopes) can
execute these SQL
tools as long as RBAC passes.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3e6ec08f166d4d799a57989348235189&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3e6ec08f166d4d799a57989348235189&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/auth.py
**Line:** 133:135
**Comment:**
*Security: Scope enforcement currently fails open for any RBAC method
name not present in the scope map. Existing tools use custom method permissions
like `execute_sql_query`, so tokens that do carry scopes can still access those
tools without any scope check as long as RBAC passes. Add an explicit scope
mapping for custom method permissions (at minimum SQL execution) or
deny-by-default when scoped tokens are present and no mapping exists.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40653&comment_hash=4f3bf857252c1fec977b41798456e86aa269ae1cc0ed9712bd785b7035590f8c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40653&comment_hash=4f3bf857252c1fec977b41798456e86aa269ae1cc0ed9712bd785b7035590f8c&reaction=dislike'>👎</a>
--
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]
