From ca8855dc0370a0c7264859b2b093cb7bbfd39e20 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Tue, 2 Jun 2026 08:31:05 -0700 Subject: [PATCH] fix(mcp): generic auth errors, required token expiry, and safer auth logging (#40646) Co-authored-by: Claude Code --- superset/mcp_service/auth.py | 56 +++++++++------ superset/mcp_service/jwt_verifier.py | 61 +++++++++++++--- superset/mcp_service/middleware.py | 3 +- .../mcp_service/utils/error_sanitization.py | 44 +++++++++++- .../mcp_service/test_auth_api_key.py | 6 +- .../mcp_service/test_auth_user_resolution.py | 30 +++++++- .../mcp_service/test_jwt_verifier.py | 66 +++++++++++++++-- .../unit_tests/mcp_service/test_middleware.py | 6 +- .../utils/test_error_sanitization.py | 70 +++++++++++++++++++ 9 files changed, 296 insertions(+), 46 deletions(-) create mode 100644 tests/unit_tests/mcp_service/utils/test_error_sanitization.py diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index fa09102132d..1b851e46da1 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -66,6 +66,17 @@ CLASS_PERMISSION_ATTR = "_class_permission_name" METHOD_PERMISSION_ATTR = "_method_permission_name" +class MCPNoAuthSourceError(ValueError): + """Raised when no authentication source is available for a request. + + Subclasses ``ValueError`` so existing ``except ValueError`` handlers and + tests keep working, while callers that need to distinguish "no auth source + configured at all" (fail open in dev/internal deployments) from a genuine + credential failure (fail closed) can ``isinstance``-check instead of + matching a fragile message string. + """ + + class MCPPermissionDeniedError(Exception): """Raised when user lacks required RBAC permission for an MCP tool.""" @@ -421,28 +432,29 @@ 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.debug( + "MCP authentication failed: no valid credentials provided " + "(no JWT access token, no API key, no g.user from middleware)" + ) + logger.debug( + "MCP auth diagnostics: MCP_AUTH_ENABLED=%s, JWT keys configured=%s, " + "MCP_DEV_USERNAME configured=%s", + auth_enabled, + jwt_configured, + dev_username_configured, + ) + raise MCPNoAuthSourceError( + "Authentication required. No valid credentials provided." ) @@ -500,13 +512,13 @@ def check_chart_data_access(chart: Any) -> "DatasetValidationResult": def _log_user_resolution_failure(exc: ValueError) -> None: """Log a user-resolution ValueError at the appropriate level. - "No authenticated user found" is expected in unauthenticated/dev - deployments (no JWT, no API key, no MCP_DEV_USERNAME configured) and - during tools/list scanning — log at DEBUG to avoid ERROR noise. - All other ValueErrors (e.g. dev username not in DB) are genuine - credential failures and are logged at ERROR. + ``MCPNoAuthSourceError`` (no JWT, no API key, no MCP_DEV_USERNAME + configured) is expected in unauthenticated/dev deployments and during + tools/list scanning — log at DEBUG to avoid ERROR noise. All other + ValueErrors (e.g. dev username not in DB) are genuine credential failures + and are logged at ERROR. """ - if "No authenticated user found" in str(exc): + if isinstance(exc, MCPNoAuthSourceError): logger.debug("MCP: no auth source configured, unauthenticated request") else: logger.error("MCP user resolution failed, denying request: %s", exc) diff --git a/superset/mcp_service/jwt_verifier.py b/superset/mcp_service/jwt_verifier.py index beb8b584a53..a5f11224336 100644 --- a/superset/mcp_service/jwt_verifier.py +++ b/superset/mcp_service/jwt_verifier.py @@ -50,10 +50,14 @@ from starlette.middleware.base import BaseHTTPMiddleware from starlette.requests import HTTPConnection, Request from starlette.responses import HTMLResponse, JSONResponse, Response +from superset.mcp_service.utils.error_sanitization import ( + sanitize_for_log as _sanitize_for_log, +) from superset.utils import json logger = logging.getLogger(__name__) + # Thread-safe storage for the specific JWT failure reason. # Set by DetailedJWTVerifier.load_access_token() on failure, # read by DetailedBearerAuthBackend.authenticate() to raise @@ -305,8 +309,27 @@ def _auth_error_handler(conn: HTTPConnection, exc: AuthenticationError) -> Respo if _prefers_browser_html(conn): return HTMLResponse(status_code=200, content=_MCP_BROWSER_HELLO_HTML) - # Log detailed reason server-side only - logger.warning("JWT authentication failed: %s", exc) + # Log detailed reason server-side only, with request context for + # auditing/troubleshooting. Guard each lookup since the connection + # object may be partially populated. + client_host = "unknown" + request_path = "unknown" + user_agent = "unknown" + try: + if getattr(conn, "client", None): + client_host = _sanitize_for_log(conn.client.host) + request_path = _sanitize_for_log(conn.scope.get("path", "unknown")) + user_agent = _sanitize_for_log(conn.headers.get("user-agent", "unknown")) + except (AttributeError, KeyError, TypeError): + logger.debug("Could not extract full request context for auth failure") + + logger.warning( + "JWT authentication failed: %s (source_ip=%s, path=%s, user_agent=%s)", + exc, + client_host, + request_path, + user_agent, + ) return JSONResponse( status_code=401, @@ -416,7 +439,7 @@ class DetailedJWTVerifier(MCPJWTVerifier): _jwt_failure_reason.set(reason) logger.debug( "Algorithm mismatch: token uses '%s', expected '%s'", - token_alg, + _sanitize_for_log(token_alg), self.algorithm, ) return None @@ -455,12 +478,23 @@ class DetailedJWTVerifier(MCPJWTVerifier): or "unknown" ) - # Step 4: Check expiration + # Step 4: Check expiration. An ``exp`` claim is required — tokens + # without one would never expire and are rejected. exp = claims.get("exp") - if exp and exp < time.time(): + if exp is None: + reason = "Token missing expiration" + _jwt_failure_reason.set(reason) + logger.debug( + "Token missing required exp claim for client '%s'", + _sanitize_for_log(client_id), + ) + return None + if exp < time.time(): reason = "Token expired" _jwt_failure_reason.set(reason) - logger.debug("Token expired for client '%s'", client_id) + logger.debug( + "Token expired for client '%s'", _sanitize_for_log(client_id) + ) return None # Step 5: Validate issuer @@ -476,7 +510,7 @@ class DetailedJWTVerifier(MCPJWTVerifier): _jwt_failure_reason.set(reason) logger.debug( "Issuer mismatch: token has '%s', expected '%s'", - iss, + _sanitize_for_log(iss), self.issuer, ) return None @@ -503,7 +537,7 @@ class DetailedJWTVerifier(MCPJWTVerifier): _jwt_failure_reason.set(reason) logger.debug( "Audience mismatch: token has '%s', expected '%s'", - aud, + _sanitize_for_log(aud), self.audience, ) return None @@ -524,12 +558,19 @@ class DetailedJWTVerifier(MCPJWTVerifier): ) return None - # All validations passed + # All validations passed. Log the successful authentication with + # safe metadata only — never the token contents or any secret. + logger.info( + "JWT authentication succeeded: client_id='%s', scopes=%s, " + "auth_method='bearer_jwt'", + _sanitize_for_log(client_id), + _sanitize_for_log(sorted(scopes)), + ) return AccessToken( token=token, client_id=str(client_id), scopes=scopes, - expires_at=int(exp) if exp else None, + expires_at=int(exp), claims=dict(claims), ) diff --git a/superset/mcp_service/middleware.py b/superset/mcp_service/middleware.py index 685022c3a9f..0ca30ace71c 100644 --- a/superset/mcp_service/middleware.py +++ b/superset/mcp_service/middleware.py @@ -43,6 +43,7 @@ from superset.mcp_service.auth import ( _get_app_context_manager, get_user_from_request, is_tool_visible_to_current_user, + MCPNoAuthSourceError, MCPPermissionDeniedError, ) from superset.mcp_service.constants import ( @@ -511,7 +512,7 @@ class RBACToolVisibilityMiddleware(Middleware): try: user = get_user_from_request() except ValueError as exc: - if "No authenticated user found" in str(exc): + if isinstance(exc, MCPNoAuthSourceError): # No auth source configured at all → fail open. # No log: this is expected in dev/internal deployments. return tools diff --git a/superset/mcp_service/utils/error_sanitization.py b/superset/mcp_service/utils/error_sanitization.py index 332a3aa7111..8923713a53b 100644 --- a/superset/mcp_service/utils/error_sanitization.py +++ b/superset/mcp_service/utils/error_sanitization.py @@ -23,7 +23,29 @@ disclosure (e.g., SQL fragments, schema names, table names) while preserving actionable error messages for LLM callers. """ +import logging import re +from typing import Any + +logger = logging.getLogger(__name__) + + +def sanitize_for_log(value: Any) -> str: + """Escape control characters in attacker-controlled values before logging. + + Claim values (alg, iss, aud, client_id, ...) and error strings are + attacker-controlled and may contain newlines or other control characters + that could be used to forge or split log lines. Escaping ``\\n``/``\\r``/``\\t`` + keeps each logged value confined to a single, unambiguous log entry. + Backslash is escaped first to avoid double-escaping the replacements. + """ + return ( + str(value) + .replace("\\", "\\\\") + .replace("\n", "\\n") + .replace("\r", "\\r") + .replace("\t", "\\t") + ) def _redact_sql_select(error_str: str, error_str_upper: str) -> str: @@ -63,8 +85,26 @@ 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: + # Sanitize control characters before logging to prevent log-line injection. + safe_error = sanitize_for_log(error) + logger.info( + "Sanitizing validation error (%s): %s", + type(error).__name__, + safe_error, + ) + error_str = str(error) # Pydantic tagged-union errors prefix the message with a long diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py index 6a0bcab6719..98dc381b3cc 100644 --- a/tests/unit_tests/mcp_service/test_auth_api_key.py +++ b/tests/unit_tests/mcp_service/test_auth_api_key.py @@ -113,7 +113,7 @@ def test_api_key_disabled_skips_auth(app) -> None: # Without API key auth or MCP_DEV_USERNAME, should raise ValueError # about no authenticated user (not about invalid API key) - with pytest.raises(ValueError, match="No authenticated user found"): + with pytest.raises(ValueError, match="Authentication required"): get_user_from_request() # SecurityManager API key methods should never be called @@ -137,7 +137,7 @@ def test_no_request_context_skips_api_key_auth(app) -> None: # Explicitly mock has_request_context to False because the test # framework's app fixture may implicitly provide a request context. with patch("superset.mcp_service.auth.has_request_context", return_value=False): - with pytest.raises(ValueError, match="No authenticated user found"): + with pytest.raises(ValueError, match="Authentication required"): get_user_from_request() mock_sm.extract_api_key_from_request.assert_not_called() @@ -172,7 +172,7 @@ def test_fab_without_extract_method_skips_gracefully(app) -> None: app.appbuilder = MagicMock() app.appbuilder.sm = mock_sm - with pytest.raises(ValueError, match="No authenticated user found"): + with pytest.raises(ValueError, match="Authentication required"): get_user_from_request() diff --git a/tests/unit_tests/mcp_service/test_auth_user_resolution.py b/tests/unit_tests/mcp_service/test_auth_user_resolution.py index 294e316bccb..0cd4a9cf181 100644 --- a/tests/unit_tests/mcp_service/test_auth_user_resolution.py +++ b/tests/unit_tests/mcp_service/test_auth_user_resolution.py @@ -229,10 +229,38 @@ def test_raises_when_no_auth_source(app) -> None: app.config.pop("MCP_DEV_USERNAME", None) g.pop("user", None) with patch("fastmcp.server.dependencies.get_access_token", return_value=None): - with pytest.raises(ValueError, match="No authenticated user found"): + with pytest.raises(ValueError, match="Authentication required"): get_user_from_request() +def test_no_auth_source_error_message_has_no_config_details(app) -> None: + """Client-facing auth error must be generic — no server config disclosed. + + Diagnostics (MCP_AUTH_ENABLED, JWT key presence, MCP_DEV_USERNAME, + API key prefixes) must go to server-side logs, never the exception + message returned toward the client. + """ + with app.app_context(): + app.config.pop("MCP_DEV_USERNAME", None) + g.pop("user", None) + with patch("fastmcp.server.dependencies.get_access_token", return_value=None): + with pytest.raises(ValueError, match="Authentication required") as exc_info: + get_user_from_request() + + message = str(exc_info.value) + assert message == "Authentication required. No valid credentials provided." + # No configuration diagnostics should leak into the client-facing message + for leak in ( + "MCP_AUTH_ENABLED", + "MCP_DEV_USERNAME", + "JWT keys", + "API key", + "sst_", + "Bearer", + ): + assert leak not in message + + def test_dev_username_not_found_raises(app) -> None: """MCP_DEV_USERNAME configured but user not in DB raises ValueError.""" with app.app_context(): diff --git a/tests/unit_tests/mcp_service/test_jwt_verifier.py b/tests/unit_tests/mcp_service/test_jwt_verifier.py index e01362d277c..2d06541d185 100644 --- a/tests/unit_tests/mcp_service/test_jwt_verifier.py +++ b/tests/unit_tests/mcp_service/test_jwt_verifier.py @@ -271,8 +271,8 @@ async def test_valid_token(hs256_verifier): @pytest.mark.asyncio -async def test_valid_token_no_expiration(hs256_verifier): - """Valid token without expiration should still succeed.""" +async def test_token_without_expiration_rejected(hs256_verifier): + """Token without an exp claim must be rejected (exp is required).""" token = _make_token( {"alg": "HS256", "typ": "JWT"}, { @@ -290,9 +290,11 @@ async def test_valid_token_no_expiration(hs256_verifier): with patch.object(hs256_verifier.jwt, "decode", return_value=claims): result = await hs256_verifier.load_access_token(token) - assert result is not None - assert result.client_id == "user1" - assert result.expires_at is None + assert result is None + reason = _jwt_failure_reason.get() + assert reason == "Token missing expiration" + # Claim values must not leak into the contextvar reason + assert "user1" not in reason @pytest.mark.asyncio @@ -724,3 +726,57 @@ async def test_catch_all_exception_sets_generic_reason(hs256_verifier): reason = _jwt_failure_reason.get() assert reason == "Token validation failed" assert "unexpected type" not in reason + + +@pytest.mark.asyncio +async def test_successful_auth_logged_with_safe_metadata(hs256_verifier, caplog): + """Successful auth emits an INFO log with safe metadata, no token/secret.""" + future_exp = int(time.time()) + 3600 + token = _make_token( + {"alg": "HS256", "typ": "JWT"}, + { + "sub": "user1", + "iss": "test-issuer", + "aud": "test-audience", + "exp": future_exp, + "scope": "read write", + }, + ) + claims = { + "sub": "user1", + "iss": "test-issuer", + "aud": "test-audience", + "exp": future_exp, + "scope": "read write", + } + + with caplog.at_level(logging.INFO, logger="superset.mcp_service.jwt_verifier"): + with patch.object(hs256_verifier.jwt, "decode", return_value=claims): + result = await hs256_verifier.load_access_token(token) + + assert result is not None + + info_messages = [r.message for r in caplog.records if r.levelno == logging.INFO] + success_logs = [m for m in info_messages if "authentication succeeded" in m] + assert success_logs, "Expected an INFO log on successful authentication" + msg = success_logs[0] + assert "user1" in msg + assert "bearer_jwt" in msg + # The raw token string and HS256 secret must never be logged + assert token not in msg + assert "test-secret-key-for-hs256-tokens" not in msg + + +def test_sanitize_for_log_escapes_newlines(): + """_sanitize_for_log escapes newline/carriage-return/tab to prevent + log-line injection from attacker-controlled claim values.""" + from superset.mcp_service.jwt_verifier import _sanitize_for_log + + injected = "RS256\nFAKE LOG LINE: admin authenticated" + sanitized = _sanitize_for_log(injected) + + assert "\n" not in sanitized + assert "\\n" in sanitized + assert _sanitize_for_log("a\rb\tc") == "a\\rb\\tc" + # Backslashes are escaped first so escapes are unambiguous + assert _sanitize_for_log("a\\nb") == "a\\\\nb" diff --git a/tests/unit_tests/mcp_service/test_middleware.py b/tests/unit_tests/mcp_service/test_middleware.py index b9625a97bb5..39907416295 100644 --- a/tests/unit_tests/mcp_service/test_middleware.py +++ b/tests/unit_tests/mcp_service/test_middleware.py @@ -34,7 +34,7 @@ from superset.commands.exceptions import ( ) from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import SupersetException, SupersetSecurityException -from superset.mcp_service.auth import MCPPermissionDeniedError +from superset.mcp_service.auth import MCPNoAuthSourceError, MCPPermissionDeniedError from superset.mcp_service.mcp_config import MCP_RESPONSE_SIZE_CONFIG from superset.mcp_service.middleware import ( _is_user_error, @@ -1237,7 +1237,9 @@ class TestRBACToolVisibilityMiddleware: ), patch( "superset.mcp_service.middleware.get_user_from_request", - side_effect=ValueError("No authenticated user found"), + side_effect=MCPNoAuthSourceError( + "Authentication required. No valid credentials provided." + ), ), ): result = await middleware.on_list_tools(MagicMock(), call_next) diff --git a/tests/unit_tests/mcp_service/utils/test_error_sanitization.py b/tests/unit_tests/mcp_service/utils/test_error_sanitization.py new file mode 100644 index 00000000000..d0ee3bd4fec --- /dev/null +++ b/tests/unit_tests/mcp_service/utils/test_error_sanitization.py @@ -0,0 +1,70 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for MCP validation-error sanitization.""" + +import logging + +from superset.mcp_service.utils.error_sanitization import _sanitize_validation_error + + +def test_sanitize_redacts_table_name(): + error = ValueError("Invalid reference to table users in query") + # Pure content check — suppress the server-side INFO log of the original. + result = _sanitize_validation_error(error, log_original=False) + assert "users" not in result + assert "[REDACTED]" in result + + +def test_sanitize_logs_original_by_default(caplog): + """The original (unsanitized) error is logged server-side before sanitizing.""" + error = ValueError("Invalid reference to table secret_revenue in query") + + with caplog.at_level( + logging.INFO, logger="superset.mcp_service.utils.error_sanitization" + ): + result = _sanitize_validation_error(error) + + # The client-facing result is sanitized (no raw table name) + assert "secret_revenue" not in result + + # The server-side INFO log retains the original, unsanitized detail + info_messages = [r.message for r in caplog.records if r.levelno == logging.INFO] + assert any("secret_revenue" in m for m in info_messages), ( + "Original error should be logged server-side before sanitizing" + ) + + +def test_sanitize_can_suppress_original_log(caplog): + """log_original=False suppresses the server-side log of the original error.""" + error = ValueError("Invalid reference to table secret_revenue in query") + + with caplog.at_level( + logging.INFO, logger="superset.mcp_service.utils.error_sanitization" + ): + _sanitize_validation_error(error, log_original=False) + + info_messages = [r.message for r in caplog.records if r.levelno == logging.INFO] + assert not any("secret_revenue" in m for m in info_messages) + + +def test_sanitize_does_not_change_client_output_with_logging(): + """log_original must not affect the sanitized client-facing output.""" + error = ValueError("Validation failed due to a timeout") + assert _sanitize_validation_error( + error, log_original=True + ) == _sanitize_validation_error(error, log_original=False)