You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/06/02 00:19:00 UTC
(superset) branch fix/mcp-auth-error-and-logging created (now d873794d676)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a change to branch fix/mcp-auth-error-and-logging
in repository https://gitbox.apache.org/repos/asf/superset.git
at d873794d676 fix(mcp): generic auth errors, required token expiry, and safer auth logging
This branch includes the following new commits:
new d873794d676 fix(mcp): generic auth errors, required token expiry, and safer auth logging
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/01: fix(mcp): generic auth errors, required token expiry, and safer auth logging
Posted by ru...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch fix/mcp-auth-error-and-logging
in repository https://gitbox.apache.org/repos/asf/superset.git
commit d873794d676b09db78b85cbef2cd30c7f9c877ce
Author: Claude Code <no...@anthropic.com>
AuthorDate: Mon Jun 1 17:18:41 2026 -0700
fix(mcp): generic auth errors, required token expiry, and safer auth logging
Hardens MCP authentication error handling and logging:
- get_user_from_request() now returns a generic client-facing auth error
("Authentication required. No valid credentials provided.") and moves the
configuration diagnostics to server-side warning/debug logs.
- DetailedJWTVerifier logs successful authentication at INFO with safe
metadata only (client_id, scopes, auth method) — never token contents.
- JWT tokens without an exp claim are now rejected instead of accepted.
- The auth-failure handler now includes request context (source IP, path,
user-agent) in its server-side warning log, guarding for missing fields.
- _sanitize_validation_error gains a log_original flag (default True) that
logs the original error server-side before returning the sanitized message.
- Attacker-controlled claim values logged at debug are passed through a
_sanitize_for_log helper that escapes control characters.
Also updates middleware and tests that depended on the previous error string.
Co-Authored-By: Claude Opus 4.8 <no...@anthropic.com>
---
superset/mcp_service/auth.py | 35 +++++------
superset/mcp_service/jwt_verifier.py | 73 +++++++++++++++++++---
superset/mcp_service/middleware.py | 2 +-
superset/mcp_service/utils/error_sanitization.py | 23 ++++++-
tests/unit_tests/mcp_service/test_auth_api_key.py | 6 +-
.../mcp_service/test_auth_user_resolution.py | 30 ++++++++-
tests/unit_tests/mcp_service/test_jwt_verifier.py | 66 +++++++++++++++++--
tests/unit_tests/mcp_service/test_middleware.py | 4 +-
.../mcp_service/utils/test_error_sanitization.py | 69 ++++++++++++++++++++
9 files changed, 268 insertions(+), 40 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index 69d8e625531..7e9ee32a6e6 100644
--- a/superset/mcp_service/auth.py
+++ b/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)"
)
+ 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 ValueError("Authentication required. No valid credentials provided.")
def has_dataset_access(dataset: "SqlaTable") -> bool:
@@ -500,13 +499,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
+ "Authentication required" 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.
"""
- if "No authenticated user found" in str(exc):
+ if "Authentication required" in str(exc):
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 23b517f3889..781495ca1fa 100644
--- a/superset/mcp_service/jwt_verifier.py
+++ b/superset/mcp_service/jwt_verifier.py
@@ -54,6 +54,24 @@ from superset.utils import json
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, ...) 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.
+ """
+ return (
+ str(value)
+ .replace("\\", "\\\\")
+ .replace("\n", "\\n")
+ .replace("\r", "\\r")
+ .replace("\t", "\\t")
+ )
+
+
# 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 +323,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 +453,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 +492,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 +524,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 +551,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,7 +572,14 @@ 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),
diff --git a/superset/mcp_service/middleware.py b/superset/mcp_service/middleware.py
index 685022c3a9f..ee72789c52a 100644
--- a/superset/mcp_service/middleware.py
+++ b/superset/mcp_service/middleware.py
@@ -511,7 +511,7 @@ class RBACToolVisibilityMiddleware(Middleware):
try:
user = get_user_from_request()
except ValueError as exc:
- if "No authenticated user found" in str(exc):
+ if "Authentication required" in str(exc):
# 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..b56a8832898 100644
--- a/superset/mcp_service/utils/error_sanitization.py
+++ b/superset/mcp_service/utils/error_sanitization.py
@@ -23,8 +23,11 @@ disclosure (e.g., SQL fragments, schema names, table names) while preserving
actionable error messages for LLM callers.
"""
+import logging
import re
+logger = logging.getLogger(__name__)
+
def _redact_sql_select(error_str: str, error_str_upper: str) -> str:
"""Redact SELECT...FROM clause content to prevent data disclosure."""
@@ -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,
+ )
+
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 34669e51d1e..8f2ac57c058 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 0eb934696bc..e6f7aa1fd8a 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..fef2f467117 100644
--- a/tests/unit_tests/mcp_service/test_middleware.py
+++ b/tests/unit_tests/mcp_service/test_middleware.py
@@ -1237,7 +1237,9 @@ class TestRBACToolVisibilityMiddleware:
),
patch(
"superset.mcp_service.middleware.get_user_from_request",
- side_effect=ValueError("No authenticated user found"),
+ side_effect=ValueError(
+ "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..f120b2e6f61
--- /dev/null
+++ b/tests/unit_tests/mcp_service/utils/test_error_sanitization.py
@@ -0,0 +1,69 @@
+# 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")
+ result = _sanitize_validation_error(error)
+ 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)