From 343f5397188cd70bf16ed24b9c66b11764573840 Mon Sep 17 00:00:00 2001 From: go165 <196723798+go165@users.noreply.github.com> Date: Mon, 15 Jun 2026 02:30:37 +0800 Subject: [PATCH] fix(server): hide unexpected tool exception details --- src/mcp/server/mcpserver/tools/base.py | 11 ++++++++- tests/client/test_list_roots_callback.py | 2 +- tests/client/test_sampling_callback.py | 2 +- tests/interaction/_requirements.py | 4 ++-- tests/interaction/mcpserver/test_tools.py | 13 +++++----- tests/server/mcpserver/test_server.py | 11 +++++---- tests/server/mcpserver/test_tool_manager.py | 24 +++++++++++++++---- .../test_url_elicitation_error_throw.py | 5 ++-- 8 files changed, 51 insertions(+), 21 deletions(-) diff --git a/src/mcp/server/mcpserver/tools/base.py b/src/mcp/server/mcpserver/tools/base.py index 754313eb8a..9b878b996a 100644 --- a/src/mcp/server/mcpserver/tools/base.py +++ b/src/mcp/server/mcpserver/tools/base.py @@ -4,16 +4,20 @@ from functools import cached_property from typing import TYPE_CHECKING, Any +import pydantic_core from pydantic import BaseModel, Field from mcp.server.mcpserver.exceptions import ToolError from mcp.server.mcpserver.utilities.context_injection import find_context_parameter from mcp.server.mcpserver.utilities.func_metadata import FuncMetadata, func_metadata +from mcp.server.mcpserver.utilities.logging import get_logger from mcp.shared._callable_inspection import is_async_callable from mcp.shared.exceptions import UrlElicitationRequiredError from mcp.shared.tool_name_validation import validate_and_warn_tool_name from mcp.types import Icon, ToolAnnotations +logger = get_logger(__name__) + if TYPE_CHECKING: from mcp.server.context import LifespanContextT, RequestT from mcp.server.mcpserver.context import Context @@ -115,5 +119,10 @@ async def run( # Re-raise UrlElicitationRequiredError so it can be properly handled # as an MCP error response with code -32042 raise - except Exception as e: + except ToolError: + raise + except pydantic_core.ValidationError as e: raise ToolError(f"Error executing tool {self.name}: {e}") from e + except Exception as e: + logger.exception("Unexpected error executing tool %s", self.name) + raise ToolError(f"Error executing tool {self.name}: unexpected internal error") from e diff --git a/tests/client/test_list_roots_callback.py b/tests/client/test_list_roots_callback.py index be4b9a97b9..c391d0af24 100644 --- a/tests/client/test_list_roots_callback.py +++ b/tests/client/test_list_roots_callback.py @@ -44,4 +44,4 @@ async def test_list_roots(context: Context, message: str): result = await client.call_tool("test_list_roots", {"message": "test message"}) assert result.is_error is True assert isinstance(result.content[0], TextContent) - assert result.content[0].text == "Error executing tool test_list_roots: List roots not supported" + assert result.content[0].text == "Error executing tool test_list_roots: unexpected internal error" diff --git a/tests/client/test_sampling_callback.py b/tests/client/test_sampling_callback.py index 6efcac0a52..b926bacb38 100644 --- a/tests/client/test_sampling_callback.py +++ b/tests/client/test_sampling_callback.py @@ -54,7 +54,7 @@ async def test_sampling_tool(message: str, ctx: Context) -> bool: result = await client.call_tool("test_sampling", {"message": "Test message for sampling"}) assert result.is_error is True assert isinstance(result.content[0], TextContent) - assert result.content[0].text == "Error executing tool test_sampling: Sampling not supported" + assert result.content[0].text == "Error executing tool test_sampling: unexpected internal error" @pytest.mark.anyio diff --git a/tests/interaction/_requirements.py b/tests/interaction/_requirements.py index caed8905d0..77d843c6ab 100644 --- a/tests/interaction/_requirements.py +++ b/tests/interaction/_requirements.py @@ -677,8 +677,8 @@ def __post_init__(self) -> None: "mcpserver:tool:handler-throws": Requirement( source="sdk", behavior=( - "An exception raised by a tool function (ToolError or otherwise) is caught and returned as a " - "tool result with isError true and the failure text in content; it does not become a JSON-RPC error." + "An exception raised by a tool function is caught and returned as a tool result with isError true; " + "explicit ToolError messages are returned to the client, while unexpected exception details are hidden." ), ), "mcpserver:tool:input-validation": Requirement( diff --git a/tests/interaction/mcpserver/test_tools.py b/tests/interaction/mcpserver/test_tools.py index 05135c1286..93adc7f5c3 100644 --- a/tests/interaction/mcpserver/test_tools.py +++ b/tests/interaction/mcpserver/test_tools.py @@ -81,19 +81,22 @@ async def test_call_tool_function_exception_becomes_error_result(connect: Connec The function's `-> str` annotation gives the tool a derived output schema, but the error result is built before any schema validation runs, so no validation failure is layered on - top of the original exception. + top of the original exception. Unexpected exception details are logged server-side and + hidden from the client-facing result. """ mcp = MCPServer("errors") @mcp.tool() def explode() -> str: - raise ValueError("boom") + raise ValueError("secret token leaked") async with connect(mcp) as client: result = await client.call_tool("explode", {}) assert result == snapshot( - CallToolResult(content=[TextContent(text="Error executing tool explode: boom")], is_error=True) + CallToolResult( + content=[TextContent(text="Error executing tool explode: unexpected internal error")], is_error=True + ) ) @@ -109,9 +112,7 @@ def flux() -> str: async with connect(mcp) as client: result = await client.call_tool("flux", {}) - assert result == snapshot( - CallToolResult(content=[TextContent(text="Error executing tool flux: flux capacitor offline")], is_error=True) - ) + assert result == snapshot(CallToolResult(content=[TextContent(text="flux capacitor offline")], is_error=True)) @requirement("mcpserver:tool:unknown-name") diff --git a/tests/server/mcpserver/test_server.py b/tests/server/mcpserver/test_server.py index 21352b5f2f..712454fb39 100644 --- a/tests/server/mcpserver/test_server.py +++ b/tests/server/mcpserver/test_server.py @@ -265,7 +265,8 @@ async def test_tool_exception_handling(self): assert len(result.content) == 1 content = result.content[0] assert isinstance(content, TextContent) - assert "Test error" in content.text + assert content.text == "Error executing tool error_tool_fn: unexpected internal error" + assert "Test error" not in content.text assert result.is_error is True async def test_tool_error_handling(self): @@ -276,11 +277,12 @@ async def test_tool_error_handling(self): assert len(result.content) == 1 content = result.content[0] assert isinstance(content, TextContent) - assert "Test error" in content.text + assert content.text == "Error executing tool error_tool_fn: unexpected internal error" + assert "Test error" not in content.text assert result.is_error is True async def test_tool_error_details(self): - """Test that exception details are properly formatted in the response""" + """Test that unexpected exception details are hidden in the response.""" mcp = MCPServer() mcp.add_tool(error_tool_fn) async with Client(mcp) as client: @@ -288,7 +290,8 @@ async def test_tool_error_details(self): content = result.content[0] assert isinstance(content, TextContent) assert isinstance(content.text, str) - assert "Test error" in content.text + assert content.text == "Error executing tool error_tool_fn: unexpected internal error" + assert "Test error" not in content.text assert result.is_error is True async def test_tool_return_value_conversion(self): diff --git a/tests/server/mcpserver/test_tool_manager.py b/tests/server/mcpserver/test_tool_manager.py index e4dfd4ff9b..faf50abd64 100644 --- a/tests/server/mcpserver/test_tool_manager.py +++ b/tests/server/mcpserver/test_tool_manager.py @@ -383,18 +383,34 @@ async def async_tool(x: int, ctx: Context) -> str: assert result == "42" @pytest.mark.anyio - async def test_context_error_handling(self): - """Test error handling when context injection fails.""" + async def test_unexpected_error_hides_internal_details(self): + """Test error handling does not expose unexpected exception details.""" def tool_with_context(x: int, ctx: Context) -> str: - raise ValueError("Test error") + raise ValueError("secret token leaked") manager = ToolManager() manager.add_tool(tool_with_context) - with pytest.raises(ToolError, match="Error executing tool tool_with_context"): + with pytest.raises(ToolError) as exc_info: await manager.call_tool("tool_with_context", {"x": 42}, context=Context()) + assert str(exc_info.value) == "Error executing tool tool_with_context: unexpected internal error" + assert "secret token leaked" not in str(exc_info.value) + + @pytest.mark.anyio + async def test_tool_error_preserves_message(self): + """Test explicit ToolError messages remain visible to clients.""" + + def tool_with_expected_error() -> str: + raise ToolError("safe client-facing error") + + manager = ToolManager() + manager.add_tool(tool_with_expected_error) + + with pytest.raises(ToolError, match="safe client-facing error"): + await manager.call_tool("tool_with_expected_error", {}, context=Context()) + class TestToolAnnotations: def test_tool_annotations(self): diff --git a/tests/server/mcpserver/test_url_elicitation_error_throw.py b/tests/server/mcpserver/test_url_elicitation_error_throw.py index 1f45fd60f0..cfbba76a81 100644 --- a/tests/server/mcpserver/test_url_elicitation_error_throw.py +++ b/tests/server/mcpserver/test_url_elicitation_error_throw.py @@ -92,7 +92,7 @@ async def multi_auth(ctx: Context) -> str: @pytest.mark.anyio async def test_normal_exceptions_still_return_error_result(): - """Test that normal exceptions still return CallToolResult with is_error=True.""" + """Test that normal exceptions still return CallToolResult without exposing details.""" mcp = MCPServer(name="NormalErrorServer") @mcp.tool(description="A tool that raises a normal exception") @@ -105,4 +105,5 @@ async def failing_tool(ctx: Context) -> str: assert result.is_error is True assert len(result.content) == 1 assert isinstance(result.content[0], types.TextContent) - assert "Something went wrong" in result.content[0].text + assert result.content[0].text == "Error executing tool failing_tool: unexpected internal error" + assert "Something went wrong" not in result.content[0].text