codeant-ai-for-open-source[bot] commented on code in PR #42935:
URL: https://github.com/apache/superset/pull/42935#discussion_r3744381514
##########
superset/mcp_service/utils/sanitization.py:
##########
@@ -385,6 +385,11 @@ def sanitize_user_input(
f"Maximum allowed length is {max_length} characters."
)
+ # Remove dangerous Unicode characters BEFORE any check so zero-widths
+ # smuggled inside a denylisted keyword can't slip past the pattern
+ # checks below (mirrors sanitize_sql_expression's ordering).
+ value = _remove_dangerous_unicode(value)
Review Comment:
**Suggestion:** Canonicalization can turn a truthy input such as a
zero-width character or control character into an empty string, but the
function does not repeat the non-empty check afterward. With
`allow_empty=False`, this returns an empty value despite the documented
non-empty contract; with `allow_empty=True`, it returns `""` instead of the
documented `None`. Recheck emptiness after `_remove_dangerous_unicode` and
apply the requested `allow_empty` behavior. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP SQL identifier validators accept empty strings.
- ⚠️ Optional dashboard fields return empty strings unexpectedly.
- ⚠️ Sanitizer callers receive values violating documented contracts.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b1496b99224b4da5a4c539f2e9cd3988&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b1496b99224b4da5a4c539f2e9cd3988&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/utils/sanitization.py
**Line:** 391:391
**Comment:**
*Logic Error: Canonicalization can turn a truthy input such as a
zero-width character or control character into an empty string, but the
function does not repeat the non-empty check afterward. With
`allow_empty=False`, this returns an empty value despite the documented
non-empty contract; with `allow_empty=True`, it returns `""` instead of the
documented `None`. Recheck emptiness after `_remove_dangerous_unicode` and
apply the requested `allow_empty` behavior.
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%2F42935&comment_hash=f672cdd256d8f142108cf3db74061ef786f4322b677ba66e2f4e221637003a00&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42935&comment_hash=f672cdd256d8f142108cf3db74061ef786f4322b677ba66e2f4e221637003a00&reaction=dislike'>👎</a>
##########
superset/mcp_service/mcp_config.py:
##########
@@ -568,6 +570,41 @@ def _is_mcp_guest_auth_enabled(app: Flask) -> bool:
return True
+def validate_multi_issuer_user_resolver(app: Flask) -> None:
+ """Reject a multi-issuer JWT trust config that has no issuer-aware
resolver.
+
+ ``default_user_resolver`` maps token claims to Superset users by
+ username/email without binding the token's ``iss`` claim. When more than
+ one issuer is trusted (``MCP_JWT_ISSUER`` configured as a list/tuple/set),
+ that lookup is not issuer-scoped: distinct issuers minting the same
+ username or email claim would resolve to the identical Superset user.
+ Single-issuer deployments are unaffected — the issuer is already pinned
+ by the verifier, so the username space is unambiguous.
+
+ Operators trusting more than one issuer must supply an issuer-aware
+ ``MCP_USER_RESOLVER`` (e.g. one that derives a compound iss+sub identity)
+ before the service will consider that configuration usable.
+ """
+ configured_issuer = app.config.get("MCP_JWT_ISSUER")
+ if (
+ isinstance(configured_issuer, (list, tuple, set))
+ and len(configured_issuer) > 1
+ and not app.config.get("MCP_USER_RESOLVER")
Review Comment:
**Suggestion:** The guard treats any configured `MCP_USER_RESOLVER` as
issuer-aware, but the resolver contract only returns a username and this value
is still looked up through the same username/email-only path. A resolver that
simply returns `sub`, `email`, or `alice` bypasses the protection while
preserving the issuer-collision vulnerability. Define and enforce an
issuer-aware resolver contract, or validate that the resolver derives an
issuer-scoped identity before allowing multiple issuers. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Different issuers can collapse onto one Superset account.
- ❌ Cross-issuer identity collisions can apply wrong permissions.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0e5bf508f97c42f18fe73df512fc1cd7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0e5bf508f97c42f18fe73df512fc1cd7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/mcp_config.py
**Line:** 592:592
**Comment:**
*Security: The guard treats any configured `MCP_USER_RESOLVER` as
issuer-aware, but the resolver contract only returns a username and this value
is still looked up through the same username/email-only path. A resolver that
simply returns `sub`, `email`, or `alice` bypasses the protection while
preserving the issuer-collision vulnerability. Define and enforce an
issuer-aware resolver contract, or validate that the resolver derives an
issuer-scoped identity before allowing multiple issuers.
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%2F42935&comment_hash=99e7a30827ab2bb2f438e965233e93d8878cef46f4ed3ca5cffb9d86d064179b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42935&comment_hash=99e7a30827ab2bb2f438e965233e93d8878cef46f4ed3ca5cffb9d86d064179b&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]
