fix(mcp): generic auth errors, required token expiry, and safer auth logging (#40646)

Co-authored-by: Claude Code <noreply@anthropic.com>

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)

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),
             )
 

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

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

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()
 
 

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():

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"

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)

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)