codeant-ai-for-open-source[bot] commented on code in PR #40864:
URL: https://github.com/apache/superset/pull/40864#discussion_r3375130937
##########
superset/mcp_service/jwt_verifier.py:
##########
@@ -273,7 +273,13 @@ async def load_access_token(self, token: str) ->
AccessToken | None: # noqa: C9
)
return None
- # All validations passed
+ # All validations passed -- record a success entry so that
+ # successful access leaves an audit trail, not just failures.
+ logger.info(
+ "JWT authentication succeeded for client '%s' (scopes=%s)",
+ client_id,
+ sorted(scopes),
+ )
Review Comment:
**Suggestion:** The success-path audit log sorts `scopes` before logging,
which can throw `TypeError` for non-orderable scope entries and make
authentication fail even though validation already passed. Logging should not
change auth outcome; log `scopes` as-is (or normalize to strings defensively)
to avoid converting a successful token into a `"Token validation failed"` path.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP JWT auth can reject valid tokens unexpectedly.
- ❌ MCP tools become unavailable for affected JWT clients.
- ⚠️ Authentication errors masked as generic validation failures.
- ⚠️ Operators see confusing failures from benign scope values.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In MCP server startup, `_create_auth_provider()` at
`superset/mcp_service/server.py:13-44` calls
`create_default_mcp_auth_factory()` from
`superset/mcp_service/mcp_config.py:320-332`, which returns a
`DetailedJWTVerifier`
instance used as the MCP auth provider when `MCP_AUTH_ENABLED` is true.
2. At runtime or in a unit test, obtain a `DetailedJWTVerifier` instance
(for example the
`hs256_verifier` fixture in
`tests/unit_tests/mcp_service/test_jwt_verifier.py:46-55`) and
monkeypatch its `_extract_scopes` method to return a scopes list with mixed,
non-orderable
types, e.g. `["read", 1]`.
3. Use `_make_token()` from
`tests/unit_tests/mcp_service/test_jwt_verifier.py:37-43` to
build a syntactically valid JWT whose header and payload will pass all
checks in
`DetailedJWTVerifier.load_access_token()` (defined at
`superset/mcp_service/jwt_verifier.py:142`), and patch
`hs256_verifier.jwt.decode` to
return a `claims` dict with valid `sub`, `iss`, `aud`, and non-expired `exp`
so that the
code reaches the success path after step 7 (scope checks).
4. Call `await hs256_verifier.load_access_token(token)`: inside
`load_access_token`, after
scopes are obtained and required scopes checked, the success log at
`superset/mcp_service/jwt_verifier.py:177-183` executes `sorted(scopes)`.
Python raises
`TypeError: '<' not supported between instances of 'int' and 'str'` for a
list like
`["read", 1]`, which is caught by the enclosing `except (ValueError,
JoseError, KeyError,
AttributeError, TypeError)` block at the end of `load_access_token`, causing
`_jwt_failure_reason` to be set to `"Token validation failed"` and `None` to
be returned
instead of an `AccessToken`. When this verifier is used via
`DetailedBearerAuthBackend.authenticate()` in the same file (lines 61-85),
that `None`
result and failure reason lead to an `AuthenticationError` and a 401
response, so a token
that otherwise passed all validations is rejected solely because of the
logging
`sorted(scopes)` call.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8c4b76e197a7487d87f6c2872ca99b8d&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=8c4b76e197a7487d87f6c2872ca99b8d&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:** 278:282
**Comment:**
*Logic Error: The success-path audit log sorts `scopes` before logging,
which can throw `TypeError` for non-orderable scope entries and make
authentication fail even though validation already passed. Logging should not
change auth outcome; log `scopes` as-is (or normalize to strings defensively)
to avoid converting a successful token into a `"Token validation failed"` path.
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%2F40864&comment_hash=6633221ba03c6337efdcd83d0d24b76aeddf443cf223117b9e4fe0e81a4e3ab5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40864&comment_hash=6633221ba03c6337efdcd83d0d24b76aeddf443cf223117b9e4fe0e81a4e3ab5&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]
