You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2026/09/17 10:51:06 UTC
(superset) 01/01: fix(mcp): return a specific error per failure class on tool calls
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch mcp-specific-denial-errors
in repository https://gitbox.apache.org/repos/asf/superset.git
commit ff9f301f31d7cfe00ad3b4e190e0ee053be1511a
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Thu Sep 17 10:50:52 2026 +0000
fix(mcp): return a specific error per failure class on tool calls
FastMCP re-raises everything a tool body throws as
`ToolError(f"Error calling tool {name!r}: {e}") from e` before any
middleware error hook runs. GlobalErrorHandlerMiddleware matched its
first branch, `isinstance(error, ToolError)`, and re-raised — so its
entire per-type branch chain was unreachable for tool failures and the
caller got whatever `str(exc)` happened to say.
An RBAC denial, a query over a dropped table, and an internal defect
were therefore indistinguishable to an MCP client, and raw driver output
(connection strings) and server paths bypassed sanitization entirely.
Argument errors were unaffected: FastMCP raises those outside the tool
body, so they were the only class still reaching the branch chain.
Classify on the unwrapped cause, and add a datasource branch so a failed
query says so instead of implying a bad tool name or arguments.
Co-Authored-By: Claude Opus 5 <no...@anthropic.com>
---
superset/mcp_service/middleware.py | 174 ++++++++++--
.../mcp_service/test_error_classification.py | 310 +++++++++++++++++++++
2 files changed, 466 insertions(+), 18 deletions(-)
diff --git a/superset/mcp_service/middleware.py b/superset/mcp_service/middleware.py
index 439de294b43..3d903b6864c 100644
--- a/superset/mcp_service/middleware.py
+++ b/superset/mcp_service/middleware.py
@@ -30,7 +30,7 @@ from fastmcp.server.middleware.middleware import CallNext
from fastmcp.tools.tool import Tool, ToolResult
from flask import g
from pydantic import ValidationError
-from sqlalchemy.exc import OperationalError, TimeoutError
+from sqlalchemy.exc import OperationalError, SQLAlchemyError, TimeoutError
from starlette.exceptions import HTTPException
from superset.commands.exceptions import (
@@ -38,7 +38,19 @@ from superset.commands.exceptions import (
ForbiddenError,
ObjectNotFoundError,
)
-from superset.exceptions import SupersetException, SupersetSecurityException
+from superset.errors import SupersetErrorType
+from superset.exceptions import (
+ ColumnNotFoundException,
+ DatabaseNotFound,
+ QueryObjectValidationError,
+ SupersetErrorsException,
+ SupersetException,
+ SupersetGenericDBErrorException,
+ SupersetParseError,
+ SupersetSecurityException,
+ SupersetTimeoutException,
+ SupersetVizException,
+)
from superset.extensions import event_logger, stats_logger_manager
from superset.mcp_service.auth import (
_get_app_context_manager,
@@ -165,6 +177,103 @@ def _invoke_error_hook(error: Exception, hook_context: dict[str, Any]) -> None:
logger.warning("MCP_ERROR_HOOK raised an exception: %s", hook_error)
+def _unwrap_tool_error(error: Exception) -> Exception:
+ """Return the original exception behind FastMCP's ``ToolError`` wrapper.
+
+ FastMCP catches every non-``FastMCPError`` raised inside a tool body and
+ re-raises it as ``ToolError(f"Error calling tool {name!r}: {e}") from e``
+ *before* any middleware error hook runs (see ``FastMCP._call_tool``). By
+ the time :class:`GlobalErrorHandlerMiddleware` sees a tool failure, the
+ concrete type — ``MCPPermissionDeniedError``, ``SupersetException``,
+ ``OperationalError`` — is no longer the exception itself, only its
+ ``__cause__``. Classifying the wrapper instead of the cause collapses
+ every distinct failure into one undifferentiated message.
+
+ A ``ToolError`` raised deliberately by tool code is re-raised by FastMCP
+ untouched and therefore carries no ``__cause__``; it is already formatted
+ for MCP and is returned as-is.
+ """
+ if isinstance(error, ToolError) and error.__cause__ is not None:
+ cause = error.__cause__
+ if isinstance(cause, Exception):
+ return cause
+ return error
+
+
+# Exception classes that mean "the query behind this tool failed", not "the
+# caller used the tool wrong". The tool name and arguments were valid; the
+# datasource, table, column, or connection it reads is broken or gone.
+_DATASOURCE_ERROR_EXCEPTIONS = (
+ ColumnNotFoundException,
+ DatabaseNotFound,
+ QueryObjectValidationError,
+ SupersetGenericDBErrorException,
+ SupersetParseError,
+ SupersetTimeoutException,
+ SupersetVizException,
+ SQLAlchemyError,
+)
+
+# ``SupersetError.error_type`` values in the DB-engine, viz, and SQL Lab
+# families. Superset raises a bare ``SupersetErrorException`` for many of
+# these, so the exception class alone is not enough to classify them.
+_DATASOURCE_ERROR_TYPES = frozenset(
+ {
+ SupersetErrorType.COLUMN_DOES_NOT_EXIST_ERROR,
+ SupersetErrorType.CONNECTION_DATABASE_TIMEOUT,
+ SupersetErrorType.CONNECTION_HOST_DOWN_ERROR,
+ SupersetErrorType.CONNECTION_PORT_CLOSED_ERROR,
+ SupersetErrorType.CONNECTION_UNKNOWN_DATABASE_ERROR,
+ SupersetErrorType.DATABASE_NOT_FOUND_ERROR,
+ SupersetErrorType.FAILED_FETCHING_DATASOURCE_INFO_ERROR,
+ SupersetErrorType.GENERIC_DB_ENGINE_ERROR,
+ SupersetErrorType.INVALID_SQL_ERROR,
+ SupersetErrorType.OBJECT_DOES_NOT_EXIST_ERROR,
+ SupersetErrorType.RESULTS_BACKEND_ERROR,
+ SupersetErrorType.SCHEMA_DOES_NOT_EXIST_ERROR,
+ SupersetErrorType.SQLLAB_TIMEOUT_ERROR,
+ SupersetErrorType.SYNTAX_ERROR,
+ SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR,
+ SupersetErrorType.TABLE_NOT_FOUND_ERROR,
+ SupersetErrorType.UNKNOWN_DATASOURCE_TYPE_ERROR,
+ SupersetErrorType.VIZ_GET_DF_ERROR,
+ }
+)
+
+
+def _datasource_error_reason(error: Exception) -> str | None:
+ """Return the enumerated reason for a datasource failure, if any.
+
+ Only ``SupersetErrorType`` members are returned — they are a closed,
+ non-sensitive vocabulary. Raw driver output (which can carry SQL, table
+ contents, or connection strings) is never surfaced from here.
+ """
+ errors = getattr(error, "errors", None)
+ if isinstance(error, SupersetErrorsException) and errors:
+ # SupersetErrorsException carries a list of SupersetError.
+ error_type = getattr(errors[0], "error_type", None)
+ elif getattr(error, "error", None) is not None:
+ # SupersetErrorException carries a single SupersetError.
+ error_type = getattr(error.error, "error_type", None)
+ else:
+ # Plain SupersetException exposes error_type directly.
+ error_type = getattr(error, "error_type", None)
+ if error_type not in _DATASOURCE_ERROR_TYPES:
+ return None
+ return str(getattr(error_type, "value", error_type))
+
+
+def _is_datasource_error(error: Exception) -> bool:
+ """Classify a failure as coming from the datasource behind the tool.
+
+ ``SupersetSecurityException`` subclasses ``SupersetErrorException``, so
+ callers must check for permission failures *before* calling this.
+ """
+ return isinstance(
+ error, _DATASOURCE_ERROR_EXCEPTIONS
+ ) or _datasource_error_reason(error) is not None
+
+
# Errors caused by the LLM/user — expected in normal MCP operation.
# Agents send bad params, try tools they lack access to, request nonexistent
# resources. These are 400-class errors and should be logged at WARNING.
@@ -625,15 +734,11 @@ class LoggingMiddleware(Middleware):
)
return result
except Exception as exc:
- # GlobalErrorHandlerMiddleware (inner) wraps tool exceptions in
- # ToolError with the original attached as __cause__; unwrap it so
- # error_type and the user/system classification reflect the real
- # failure rather than the ToolError wrapper.
- original = (
- exc.__cause__
- if isinstance(exc, ToolError) and exc.__cause__ is not None
- else exc
- )
+ # Tool exceptions arrive wrapped in ToolError with the original
+ # attached as __cause__; unwrap it so error_type and the
+ # user/system classification reflect the real failure rather than
+ # the ToolError wrapper.
+ original = _unwrap_tool_error(exc)
error_type = type(original).__name__
raised_is_user_error = _is_user_error(original)
success = False
@@ -954,12 +1059,23 @@ class GlobalErrorHandlerMiddleware(Middleware):
async def _handle_error( # noqa: C901
self,
- error: Exception,
+ wrapped_error: Exception,
context: MiddlewareContext,
tool_name: str,
duration_ms: int,
) -> None:
- """Handle different types of errors with appropriate responses"""
+ """Handle different types of errors with appropriate responses.
+
+ ``wrapped_error`` is what reached the middleware; ``error`` is the
+ real failure. FastMCP re-raises everything a tool body throws as
+ ``ToolError(...) from e`` before any middleware runs, so classifying
+ the exception as received would funnel an RBAC denial, a dead table,
+ and an internal bug into the same message. Every decision below —
+ log severity, error-tracker capture, and the client-facing text — is
+ therefore made on the unwrapped cause. See :func:`_unwrap_tool_error`.
+ """
+ error = _unwrap_tool_error(wrapped_error)
+
# Extract user context for logging
user_id = None
try:
@@ -1027,7 +1143,8 @@ class GlobalErrorHandlerMiddleware(Middleware):
# Handle specific error types with appropriate responses
if isinstance(error, ToolError):
- # Tool errors are already formatted for MCP
+ # A ToolError that survived _unwrap_tool_error was raised
+ # deliberately by tool code and is already formatted for MCP.
raise error
elif isinstance(error, ValidationError):
# Pydantic validation errors
@@ -1056,12 +1173,19 @@ class GlobalErrorHandlerMiddleware(Middleware):
f"Service error in {tool_name}: {_sanitize_error_for_logging(error)}"
) from error
elif isinstance(error, MCPPermissionDeniedError):
- # MCP RBAC permission denied — convert to structured ToolError.
- # Must come before the generic PermissionError branch because
- # MCPPermissionDeniedError inherits from PermissionError.
+ # MCP RBAC permission denied. Rendered from the exception's own
+ # structured fields ("Permission denied: <permission> on
+ # <resource>") rather than through _sanitize_error_for_logging,
+ # which flattens every PermissionError to "Access denied" and
+ # would throw away the two facts that make the denial
+ # actionable. Must come before the generic PermissionError
+ # branch because MCPPermissionDeniedError inherits from it.
raise ToolError(str(error)) from error
elif isinstance(error, PermissionError):
- # Permission/authorization errors
+ # Authorization failures that are not raised by the MCP RBAC
+ # decorator still get the "Permission denied" shape, so callers
+ # can tell a denial from a malformed call no matter which layer
+ # refused them.
raise ToolError(
f"Permission denied for {tool_name}: "
f"You don't have access to this resource."
@@ -1083,6 +1207,20 @@ class GlobalErrorHandlerMiddleware(Middleware):
f"Permission denied for {tool_name}: "
f"{_sanitize_error_for_logging(error)}"
) from error
+ elif _is_datasource_error(error):
+ # The query behind the tool failed. The tool name and arguments
+ # were fine, so the caller must NOT be told to re-check the tool
+ # schema — that advice fits an argument error and sends them
+ # down the wrong path for a dropped table or a dead connection.
+ # Only the enumerated SupersetErrorType is echoed; raw driver
+ # output could carry SQL or connection details.
+ reason = _datasource_error_reason(error) or type(error).__name__
+ raise ToolError(
+ f"Datasource error in {tool_name}: the query against the "
+ f"underlying datasource failed ({reason}). The tool name and "
+ f"arguments were valid — the datasource, table, or column it "
+ f"reads may be missing, renamed, or unreachable."
+ ) from error
elif isinstance(error, SupersetException):
# Other Superset errors — .status determines severity (already
# classified by _is_user_error above for log level)
diff --git a/tests/unit_tests/mcp_service/test_error_classification.py b/tests/unit_tests/mcp_service/test_error_classification.py
new file mode 100644
index 00000000000..49d92be891d
--- /dev/null
+++ b/tests/unit_tests/mcp_service/test_error_classification.py
@@ -0,0 +1,310 @@
+# 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.
+
+"""Per-failure-class error classification for MCP tool calls.
+
+Three unrelated failures used to reach MCP clients as one undifferentiated
+message, because FastMCP re-raises everything a tool body throws as
+``ToolError(f"Error calling tool {name!r}: {e}") from e`` *before* any
+middleware error hook runs. ``GlobalErrorHandlerMiddleware`` then matched its
+first branch, ``isinstance(error, ToolError)``, and re-raised — so its entire
+per-type branch chain was unreachable for tool failures, and the client got
+whatever ``str(exc)`` happened to say.
+
+The rest of the suite exercises that chain by injecting *raw* exceptions into
+``on_message``, which is why the gap went unnoticed: no test drove a tool
+exception through FastMCP's wrapping first. These tests do, end to end.
+"""
+
+from typing import Any
+
+import pytest
+from fastmcp import FastMCP
+from fastmcp.client import Client
+from fastmcp.exceptions import ToolError
+
+from superset.commands.exceptions import ForbiddenError
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import (
+ SupersetErrorException,
+ SupersetException,
+ SupersetSecurityException,
+)
+from superset.mcp_service.auth import MCPPermissionDeniedError
+from superset.mcp_service.middleware import (
+ _datasource_error_reason,
+ _is_datasource_error,
+ _unwrap_tool_error,
+ GlobalErrorHandlerMiddleware,
+ ToolResultCompatibilityMiddleware,
+)
+
+# A connection string and an internal path, planted in the exception messages
+# below to prove neither reaches the caller.
+LEAKED_DSN = "postgresql://admin:hunter2@db.internal/prod"
+LEAKED_PATH = "/srv/app/superset/mcp_service/chart/tool/get_chart_data.py"
+
+
+def _dead_table_error() -> SupersetErrorException:
+ """The upstream failure a chart over a dropped table produces."""
+ return SupersetErrorException(
+ SupersetError(
+ message=f'relation "orders" does not exist ({LEAKED_DSN})',
+ error_type=SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+
+def _build_server() -> FastMCP:
+ """A FastMCP server with one tool per failure class.
+
+ ``LoggingMiddleware`` is deliberately omitted: it needs a Flask app
+ context, and it does not participate in classifying the response.
+ """
+ mcp: FastMCP = FastMCP("error-classification")
+
+ @mcp.tool
+ def rbac_denied(unused: int = 1) -> str:
+ """RBAC denial raised by the @tool permission decorator."""
+ raise MCPPermissionDeniedError(
+ permission_name="can_get",
+ view_name="User",
+ tool_name="rbac_denied",
+ )
+
+ @mcp.tool
+ def forbidden(unused: int = 1) -> str:
+ """Authorization refused below the MCP layer."""
+ raise ForbiddenError()
+
+ @mcp.tool
+ def dead_table(unused: int = 1) -> str:
+ """Well-formed call whose datasource is gone."""
+ raise _dead_table_error()
+
+ @mcp.tool
+ def needs_id(id: int) -> str: # noqa: A002
+ """Schema mismatch: callers guess ``identifier`` for ``id``."""
+ return "ok"
+
+ @mcp.tool
+ def internal_bug(unused: int = 1) -> str:
+ """A genuine server-side defect."""
+ raise RuntimeError(f"unexpected failure in {LEAKED_PATH}")
+
+ mcp.add_middleware(ToolResultCompatibilityMiddleware(structured_output_enabled=False))
+ mcp.add_middleware(GlobalErrorHandlerMiddleware())
+ return mcp
+
+
+async def _call(tool: str, arguments: dict[str, Any] | None = None) -> str:
+ """Return the client-facing text for a failing tool call."""
+ async with Client(_build_server()) as client:
+ result = await client.call_tool(tool, arguments or {}, raise_on_error=False)
+ assert result.is_error, f"{tool} was expected to fail"
+ return result.content[0].text
+
+
+class TestFastMCPWrappingPrecondition:
+ """Guard the assumption the classification fix is built on."""
+
+ @pytest.mark.asyncio
+ async def test_tool_exceptions_arrive_wrapped_in_tool_error(self) -> None:
+ """FastMCP wraps tool exceptions before middleware sees them.
+
+ If a FastMCP upgrade stops doing this, ``_unwrap_tool_error`` becomes
+ a no-op and the branch chain keeps working — but this test failing is
+ the signal that the comments explaining *why* it exists are stale.
+ """
+ seen: list[Exception] = []
+
+ mcp: FastMCP = FastMCP("precondition")
+
+ @mcp.tool
+ def boom(unused: int = 1) -> str:
+ raise PermissionError("denied")
+
+ class Capture(ToolResultCompatibilityMiddleware):
+ async def on_call_tool(self, context: Any, call_next: Any) -> Any:
+ try:
+ return await call_next(context)
+ except Exception as exc: # noqa: BLE001
+ seen.append(exc)
+ raise
+
+ mcp.add_middleware(Capture(structured_output_enabled=False))
+
+ async with Client(mcp) as client:
+ await client.call_tool("boom", {}, raise_on_error=False)
+
+ assert len(seen) == 1
+ assert isinstance(seen[0], ToolError)
+ assert isinstance(seen[0].__cause__, PermissionError)
+
+
+class TestUnwrapToolError:
+ """``_unwrap_tool_error`` must recover the cause without eating
+ deliberately raised ToolErrors."""
+
+ def test_returns_cause_of_wrapped_error(self) -> None:
+ cause = MCPPermissionDeniedError(permission_name="can_read", view_name="Chart")
+ wrapped = ToolError("Error calling tool 'list_charts': denied")
+ wrapped.__cause__ = cause
+
+ assert _unwrap_tool_error(wrapped) is cause
+
+ def test_passes_through_deliberate_tool_error(self) -> None:
+ """A ToolError raised by tool code has no cause and is already
+ formatted for MCP, so it must survive untouched."""
+ deliberate = ToolError("'search_tools' cannot be called via the proxy")
+
+ assert _unwrap_tool_error(deliberate) is deliberate
+
+ def test_passes_through_unwrapped_exception(self) -> None:
+ error = ValueError("page must be positive")
+
+ assert _unwrap_tool_error(error) is error
+
+
+class TestDatasourceErrorClassification:
+ """Only genuine query/datasource failures take the datasource branch."""
+
+ def test_classifies_missing_table(self) -> None:
+ assert _is_datasource_error(_dead_table_error()) is True
+
+ def test_reason_is_the_enumerated_error_type(self) -> None:
+ assert (
+ _datasource_error_reason(_dead_table_error())
+ == SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR.value
+ )
+
+ def test_security_exception_is_not_a_datasource_error(self) -> None:
+ """SupersetSecurityException subclasses SupersetErrorException, so the
+ permission branch must win — a denied table is not a broken table."""
+ error = SupersetSecurityException(
+ SupersetError(
+ message="denied",
+ error_type=SupersetErrorType.TABLE_SECURITY_ACCESS_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ assert _is_datasource_error(error) is False
+
+ def test_unrelated_superset_exception_is_not_a_datasource_error(self) -> None:
+ assert _is_datasource_error(SupersetException("something else")) is False
+
+ def test_reason_is_none_for_unlisted_error_type(self) -> None:
+ error = SupersetErrorException(
+ SupersetError(
+ message="nope",
+ error_type=SupersetErrorType.GENERIC_COMMAND_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ assert _datasource_error_reason(error) is None
+
+
+class TestPerClassClientFacingErrors:
+ """Each failure class reaches the caller as its own honest message."""
+
+ @pytest.mark.asyncio
+ async def test_rbac_denial_uses_permission_denied_shape(self) -> None:
+ """An RBAC denial names the permission and the resource, and never
+ tells the caller to go re-read the tool schema."""
+ message = await _call("rbac_denied")
+
+ assert "Permission denied: can_get on User" in message
+ assert "Validation error" not in message
+ assert "inputSchema" not in message
+ assert "Internal error" not in message
+
+ @pytest.mark.asyncio
+ async def test_non_rbac_authorization_failure_also_says_permission_denied(
+ self,
+ ) -> None:
+ """Denials raised below the MCP RBAC decorator get the same shape, so
+ a caller can recognise a denial regardless of which layer refused."""
+ message = await _call("forbidden")
+
+ assert "Permission denied" in message
+
+ @pytest.mark.asyncio
+ async def test_schema_mismatch_keeps_validation_guidance(self) -> None:
+ """Wrong argument names still get the advice that actually fits:
+ the offending fields, so the caller can match the inputSchema."""
+ message = await _call("needs_id", {"identifier": 5})
+
+ assert "Validation error in needs_id" in message
+ assert "id" in message
+ assert "identifier" in message
+ assert "Permission denied" not in message
+
+ @pytest.mark.asyncio
+ async def test_datasource_failure_blames_the_query_not_the_caller(self) -> None:
+ """A dropped table says the datasource failed and states the call was
+ valid — the previous message implied a bad tool name or arguments."""
+ message = await _call("dead_table")
+
+ assert "Datasource error in dead_table" in message
+ assert SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR.value in message
+ assert "arguments were valid" in message
+ assert "Permission denied" not in message
+ assert "Validation error" not in message
+
+ @pytest.mark.asyncio
+ async def test_internal_error_is_opaque_and_carries_an_error_id(self) -> None:
+ message = await _call("internal_bug")
+
+ assert "Internal error in internal_bug" in message
+ assert "Error ID:" in message
+
+ @pytest.mark.asyncio
+ async def test_the_four_classes_do_not_share_a_message(self) -> None:
+ """The regression in one assertion: conflating these is the bug."""
+ messages = [
+ await _call("rbac_denied"),
+ await _call("needs_id", {"identifier": 5}),
+ await _call("dead_table"),
+ await _call("internal_bug"),
+ ]
+
+ assert len(set(messages)) == len(messages)
+
+
+class TestNoNewDisclosure:
+ """Classification must not widen what a failure discloses."""
+
+ @pytest.mark.asyncio
+ async def test_datasource_error_does_not_echo_raw_driver_output(self) -> None:
+ """Only the enumerated SupersetErrorType is surfaced; the driver
+ message (which carried a DSN) is not."""
+ message = await _call("dead_table")
+
+ assert LEAKED_DSN not in message
+ assert "hunter2" not in message
+ assert "db.internal" not in message
+ assert "orders" not in message
+
+ @pytest.mark.asyncio
+ async def test_internal_error_does_not_echo_server_paths(self) -> None:
+ message = await _call("internal_bug")
+
+ assert LEAKED_PATH not in message
+ assert "mcp_service" not in message