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

[ju...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

justinpark pushed a commit to branch 6.2
in repository https://gitbox.apache.org/repos/asf/superset.git

commit 210da7f0ae6d431131467ee9bd53359b5cd57cf4
Author: Evan Rusackas <ev...@preset.io>
AuthorDate: Tue Jun 2 08:31:05 2026 -0700

    fix(mcp): generic auth errors, required token expiry, and safer auth logging (#40646)
    
    Co-authored-by: Claude Code <no...@anthropic.com>
    (cherry picked from commit ca8855dc0370a0c7264859b2b093cb7bbfd39e20)
---
 superset/mcp_service/utils/error_sanitization.py   | 44 +++++++++++++-
 .../mcp_service/utils/test_error_sanitization.py   | 70 ++++++++++++++++++++++
 2 files changed, 112 insertions(+), 2 deletions(-)

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