Copilot commented on code in PR #40646:
URL: https://github.com/apache/superset/pull/40646#discussion_r3337911672
##########
superset/mcp_service/auth.py:
##########
@@ -421,29 +421,28 @@ def get_user_from_request() -> User:
if hasattr(g, "user") and g.user:
return g.user
- # No auth source available — raise with diagnostic details
+ # No auth source available. Keep the client-facing message generic so it
+ # does not disclose server configuration; the detailed diagnostics are
+ # logged server-side only.
auth_enabled = current_app.config.get("MCP_AUTH_ENABLED", False)
jwt_configured = bool(
current_app.config.get("MCP_JWKS_URI")
or current_app.config.get("MCP_JWT_PUBLIC_KEY")
or current_app.config.get("MCP_JWT_SECRET")
)
- details = [
- f"No JWT access token in MCP request context "
- f"(MCP_AUTH_ENABLED={auth_enabled}, "
- f"JWT keys configured={jwt_configured})",
- "No API key in Authorization header",
- "MCP_DEV_USERNAME is not configured",
- "g.user was not set by external middleware",
- ]
- configured_prefixes = current_app.config.get("FAB_API_KEY_PREFIXES",
["sst_"])
- prefix_example = configured_prefixes[0] if configured_prefixes else "sst_"
- raise ValueError(
- "No authenticated user found. Tried:\n"
- + "\n".join(f" - {d}" for d in details)
- + f"\n\nEither pass a valid API key (Bearer {prefix_example}...), "
- "JWT token, or configure MCP_DEV_USERNAME for development."
+ dev_username_configured = bool(current_app.config.get("MCP_DEV_USERNAME"))
+ logger.warning(
+ "MCP authentication failed: no valid credentials provided "
+ "(no JWT access token, no API key, no g.user from middleware)"
)
Review Comment:
`get_user_from_request()` logs a WARNING for the expected "no auth source
configured" case. This is inconsistent with the surrounding design (e.g.
tools/list wants to fail open with no log) and will spam logs in dev/internal
deployments or when MCP_AUTH_ENABLED is False. Consider logging at DEBUG unless
MCP_AUTH_ENABLED is enabled (or moving this log to the caller).
##########
superset/mcp_service/utils/error_sanitization.py:
##########
@@ -63,8 +66,24 @@ def _get_generic_error_message(error_str: str) -> str | None:
return None
-def _sanitize_validation_error(error: Exception) -> str:
- """SECURITY FIX: Sanitize validation errors to prevent disclosure."""
+def _sanitize_validation_error(error: Exception, log_original: bool = True) ->
str:
+ """SECURITY FIX: Sanitize validation errors to prevent disclosure.
+
+ Args:
+ error: The original exception to sanitize for client-facing output.
+ log_original: When True (default), log the original (unsanitized)
+ error server-side at INFO level before returning the sanitized
+ version. This preserves full diagnostics for operators while the
+ client only ever receives the sanitized message. Set to False to
+ suppress the server-side log (e.g. when the caller already logged).
+ """
+ if log_original:
+ logger.info(
+ "Sanitizing validation error (%s): %s",
+ type(error).__name__,
+ error,
+ )
+
Review Comment:
Logging the *unsanitized* validation error at INFO by default can
re-introduce sensitive data exposure in shared application logs (these
exceptions can include SQL fragments, schema/table names, or user-supplied
values). If this logging is needed for diagnostics, consider either (a) logging
the sanitized message at INFO and the original at DEBUG, or (b) keeping
original-only logging opt-in (default `log_original=False`) or behind a config
flag.
--
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]
