rusackas commented on code in PR #42935:
URL: https://github.com/apache/superset/pull/42935#discussion_r3744938346
##########
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:
Good catch, fixed. Both `sanitize_user_input` and `sanitize_sql_expression`
now recheck emptiness right after `_remove_dangerous_unicode`, so an
all-zero-width input raises (or returns `None` with `allow_empty=True`) instead
of slipping through as `""`.
##########
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:
The resolver gets the full access token, including `claims["iss"]`, so it
can derive an issuer-scoped identity, but there's no way to verify a black-box
callable actually does that at config-check time. This check only requires one
be configured (documented in the docstring above), which is the most we can
enforce for an arbitrary operator-supplied function.
--
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]
