rusackas commented on code in PR #40646:
URL: https://github.com/apache/superset/pull/40646#discussion_r3338280165
##########
superset/mcp_service/middleware.py:
##########
@@ -511,7 +511,7 @@ async def on_list_tools(
try:
user = get_user_from_request()
except ValueError as exc:
- if "No authenticated user found" in str(exc):
+ if "Authentication required" in str(exc):
Review Comment:
The PR addresses this: the substring check is on the PR-controlled message
string 'Authentication required.' (not on attacker-supplied content). The 'No
authenticated user found' check was replaced with this static string, so
there's no path where attacker-supplied data can match the substring and be
misclassified as 'no auth configured'.
##########
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:
The no-auth-configured path already logs at DEBUG (see the logger.debug
calls in get_user_from_request and _log_user_resolution_failure). WARNING is
only emitted for genuine credential failures (other ValueError values). The log
level is correct in the PR.
##########
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:
The PR adds a _sanitize_for_log() function (see jwt_verifier.py)
specifically to escape control characters in attacker-controlled claim values
before logging. The log injection concern is addressed.
##########
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:
The no-auth path logs at DEBUG in this PR (see the logger.debug calls
replacing the old error details). WARNING is reserved for genuine failures. The
concern is addressed.
##########
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:
The _sanitize_for_log() helper (added in jwt_verifier.py) strips control
characters from attacker-controlled claim values. Validation error messages
logged to INFO come from the JWT library and don't contain user-supplied SQL or
schema names — they contain claim values which are sanitized before logging.
Agreed that defaulting to sanitized-only logging at INFO is safer; the PR
applies sanitization consistently.
--
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]
