From 7cf339c81049ef2c4bd035302febb0060c231f16 Mon Sep 17 00:00:00 2001 From: Varun Sharma Date: Mon, 2 Mar 2026 16:45:56 +0530 Subject: [PATCH 1/5] fix: prevent tool exceptions from leaking internal details to client Tool.run() and _handle_call_tool() now return a generic error message for unexpected exceptions instead of str(e), which could expose sensitive internal details like connection strings, file paths, or stack traces to MCP clients. - ToolError is still passed through unchanged (intentional user-facing errors) - UrlElicitationRequiredError and MCPError are re-raised at the server level - All other exceptions log the full traceback server-side and return a generic 'An unexpected error occurred' message to the client Fixes #698 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/mcp/server/mcpserver/server.py | 10 ++++++++-- src/mcp/server/mcpserver/tools/base.py | 8 +++++++- tests/client/test_list_roots_callback.py | 2 +- tests/client/test_sampling_callback.py | 2 +- tests/server/mcpserver/test_server.py | 11 +++++++---- tests/server/mcpserver/test_tool_manager.py | 2 +- .../mcpserver/test_url_elicitation_error_throw.py | 4 +++- 7 files changed, 28 insertions(+), 11 deletions(-) diff --git a/src/mcp/server/mcpserver/server.py b/src/mcp/server/mcpserver/server.py index 9c7105a7b..cc2084098 100644 --- a/src/mcp/server/mcpserver/server.py +++ b/src/mcp/server/mcpserver/server.py @@ -33,7 +33,7 @@ from mcp.server.lowlevel.helper_types import ReadResourceContents from mcp.server.lowlevel.server import LifespanResultT, Server, request_ctx from mcp.server.lowlevel.server import lifespan as default_lifespan -from mcp.server.mcpserver.exceptions import ResourceError +from mcp.server.mcpserver.exceptions import ResourceError, ToolError from mcp.server.mcpserver.prompts import Prompt, PromptManager from mcp.server.mcpserver.resources import FunctionResource, Resource, ResourceManager from mcp.server.mcpserver.tools import Tool, ToolManager @@ -304,8 +304,14 @@ async def _handle_call_tool( result = await self.call_tool(params.name, params.arguments or {}) except MCPError: raise - except Exception as e: + except ToolError as e: return CallToolResult(content=[TextContent(type="text", text=str(e))], is_error=True) + except Exception: + logger.exception(f"Error calling tool {params.name}") + return CallToolResult( + content=[TextContent(type="text", text=f"An unexpected error occurred calling tool {params.name}")], + is_error=True, + ) if isinstance(result, CallToolResult): return result if isinstance(result, tuple) and len(result) == 2: diff --git a/src/mcp/server/mcpserver/tools/base.py b/src/mcp/server/mcpserver/tools/base.py index f6bfadbc4..495268d29 100644 --- a/src/mcp/server/mcpserver/tools/base.py +++ b/src/mcp/server/mcpserver/tools/base.py @@ -2,6 +2,7 @@ import functools import inspect +import logging from collections.abc import Callable from functools import cached_property from typing import TYPE_CHECKING, Any @@ -15,6 +16,8 @@ from mcp.shared.tool_name_validation import validate_and_warn_tool_name from mcp.types import Icon, ToolAnnotations +logger = logging.getLogger(__name__) + if TYPE_CHECKING: from mcp.server.context import LifespanContextT, RequestT from mcp.server.mcpserver.server import Context @@ -112,8 +115,11 @@ async def run( # Re-raise UrlElicitationRequiredError so it can be properly handled # as an MCP error response with code -32042 raise + except ToolError: + raise except Exception as e: - raise ToolError(f"Error executing tool {self.name}: {e}") from e + logger.exception(f"Error executing tool {self.name}") + raise ToolError(f"An unexpected error occurred executing tool {self.name}") from e def _is_async_callable(obj: Any) -> bool: diff --git a/tests/client/test_list_roots_callback.py b/tests/client/test_list_roots_callback.py index 6a2f49f39..532beba14 100644 --- a/tests/client/test_list_roots_callback.py +++ b/tests/client/test_list_roots_callback.py @@ -45,4 +45,4 @@ async def test_list_roots(context: Context[None], 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 == "An unexpected error occurred executing tool test_list_roots" diff --git a/tests/client/test_sampling_callback.py b/tests/client/test_sampling_callback.py index 3357bc921..c932ecaa5 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): 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 == "An unexpected error occurred executing tool test_sampling" @pytest.mark.anyio diff --git a/tests/server/mcpserver/test_server.py b/tests/server/mcpserver/test_server.py index cfbe6587b..d82a7a59d 100644 --- a/tests/server/mcpserver/test_server.py +++ b/tests/server/mcpserver/test_server.py @@ -258,7 +258,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 "unexpected error" in content.text.lower() + assert "Test error" not in content.text assert result.is_error is True async def test_tool_error_handling(self): @@ -269,11 +270,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 "unexpected error" in content.text.lower() + 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 exception details are NOT exposed to the client""" mcp = MCPServer() mcp.add_tool(error_tool_fn) async with Client(mcp) as client: @@ -281,7 +283,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 "error_tool_fn" in content.text + 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 550bba50a..d661d64c1 100644 --- a/tests/server/mcpserver/test_tool_manager.py +++ b/tests/server/mcpserver/test_tool_manager.py @@ -410,7 +410,7 @@ def tool_with_context(x: int, ctx: Context[ServerSessionT, None]) -> str: mcp = MCPServer() ctx = mcp.get_context() - with pytest.raises(ToolError, match="Error executing tool tool_with_context"): + with pytest.raises(ToolError, match="unexpected error occurred executing tool tool_with_context"): await manager.call_tool("tool_with_context", {"x": 42}, context=ctx) diff --git a/tests/server/mcpserver/test_url_elicitation_error_throw.py b/tests/server/mcpserver/test_url_elicitation_error_throw.py index 2d2993799..65a747ff6 100644 --- a/tests/server/mcpserver/test_url_elicitation_error_throw.py +++ b/tests/server/mcpserver/test_url_elicitation_error_throw.py @@ -106,4 +106,6 @@ async def failing_tool(ctx: Context[ServerSession, None]) -> 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 "An unexpected error occurred executing tool failing_tool" in result.content[0].text + # Verify the original exception details are NOT leaked to the client + assert "Something went wrong" not in result.content[0].text From 5a93a4e904d0f2cdfdf6b683e43b95da6e9f5063 Mon Sep 17 00:00:00 2001 From: Varun Sharma Date: Mon, 2 Mar 2026 16:53:36 +0530 Subject: [PATCH 2/5] fix: add coverage for ToolError passthrough and defensive handler - Add test for ToolError passthrough (covers tools/base.py except ToolError) - Mark unreachable defensive except in _handle_call_tool as no cover Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/mcp/server/mcpserver/server.py | 2 +- tests/server/mcpserver/test_server.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/mcp/server/mcpserver/server.py b/src/mcp/server/mcpserver/server.py index cc2084098..7906bee58 100644 --- a/src/mcp/server/mcpserver/server.py +++ b/src/mcp/server/mcpserver/server.py @@ -306,7 +306,7 @@ async def _handle_call_tool( raise except ToolError as e: return CallToolResult(content=[TextContent(type="text", text=str(e))], is_error=True) - except Exception: + except Exception: # pragma: no cover logger.exception(f"Error calling tool {params.name}") return CallToolResult( content=[TextContent(type="text", text=f"An unexpected error occurred calling tool {params.name}")], diff --git a/tests/server/mcpserver/test_server.py b/tests/server/mcpserver/test_server.py index d82a7a59d..582e7a7e3 100644 --- a/tests/server/mcpserver/test_server.py +++ b/tests/server/mcpserver/test_server.py @@ -212,6 +212,10 @@ def error_tool_fn() -> None: raise ValueError("Test error") +def tool_error_fn() -> None: + raise ToolError("Intentional tool error") + + def image_tool_fn(path: str) -> Image: return Image(path) @@ -287,6 +291,17 @@ async def test_tool_error_details(self): assert "Test error" not in content.text assert result.is_error is True + async def test_tool_error_passthrough(self): + """Test that ToolError raised in tool is passed through with its message.""" + mcp = MCPServer() + mcp.add_tool(tool_error_fn) + async with Client(mcp) as client: + result = await client.call_tool("tool_error_fn", {}) + assert result.is_error is True + content = result.content[0] + assert isinstance(content, TextContent) + assert "Intentional tool error" in content.text + async def test_tool_return_value_conversion(self): mcp = MCPServer() mcp.add_tool(tool_fn) From a648537d9fbb8a13d4c9344c41b274f3aea78c68 Mon Sep 17 00:00:00 2001 From: Varun Sharma Date: Sat, 7 Mar 2026 12:18:02 +0530 Subject: [PATCH 3/5] fix: address review comments - remove duplicate logging and add defensive handler test - Remove logger.exception from base.py Tool.run() to avoid duplicate logging (server.py's defensive handler already logs) - Remove unused logging import from base.py - Remove '# pragma: no cover' from server.py defensive exception handler - Add test_handle_call_tool_defensive_exception_handler to verify the defensive handler returns a generic error and does not leak internal exception details Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/mcp/server/mcpserver/server.py | 2 +- src/mcp/server/mcpserver/tools/base.py | 4 ---- tests/server/mcpserver/test_server.py | 20 ++++++++++++++++++++ 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/mcp/server/mcpserver/server.py b/src/mcp/server/mcpserver/server.py index 7906bee58..cc2084098 100644 --- a/src/mcp/server/mcpserver/server.py +++ b/src/mcp/server/mcpserver/server.py @@ -306,7 +306,7 @@ async def _handle_call_tool( raise except ToolError as e: return CallToolResult(content=[TextContent(type="text", text=str(e))], is_error=True) - except Exception: # pragma: no cover + except Exception: logger.exception(f"Error calling tool {params.name}") return CallToolResult( content=[TextContent(type="text", text=f"An unexpected error occurred calling tool {params.name}")], diff --git a/src/mcp/server/mcpserver/tools/base.py b/src/mcp/server/mcpserver/tools/base.py index 495268d29..2d9691541 100644 --- a/src/mcp/server/mcpserver/tools/base.py +++ b/src/mcp/server/mcpserver/tools/base.py @@ -2,7 +2,6 @@ import functools import inspect -import logging from collections.abc import Callable from functools import cached_property from typing import TYPE_CHECKING, Any @@ -16,8 +15,6 @@ from mcp.shared.tool_name_validation import validate_and_warn_tool_name from mcp.types import Icon, ToolAnnotations -logger = logging.getLogger(__name__) - if TYPE_CHECKING: from mcp.server.context import LifespanContextT, RequestT from mcp.server.mcpserver.server import Context @@ -118,7 +115,6 @@ async def run( except ToolError: raise except Exception as e: - logger.exception(f"Error executing tool {self.name}") raise ToolError(f"An unexpected error occurred executing tool {self.name}") from e diff --git a/tests/server/mcpserver/test_server.py b/tests/server/mcpserver/test_server.py index 582e7a7e3..52392ba2e 100644 --- a/tests/server/mcpserver/test_server.py +++ b/tests/server/mcpserver/test_server.py @@ -302,6 +302,26 @@ async def test_tool_error_passthrough(self): assert isinstance(content, TextContent) assert "Intentional tool error" in content.text + async def test_handle_call_tool_defensive_exception_handler(self): + """Test that _handle_call_tool returns generic error when call_tool raises unexpected Exception.""" + mcp = MCPServer() + mcp.add_tool(tool_fn) + + original_call_tool = mcp.call_tool + + async def patched_call_tool(name: str, arguments: dict[str, Any]) -> Any: + raise RuntimeError("internal db connection string leaked") + + async with Client(mcp) as client: + mcp.call_tool = patched_call_tool # type: ignore[assignment] + result = await client.call_tool("tool_fn", {"x": 1, "y": 2}) + mcp.call_tool = original_call_tool # type: ignore[assignment] + assert result.is_error is True + content = result.content[0] + assert isinstance(content, TextContent) + assert "unexpected error" in content.text.lower() + assert "internal db connection string leaked" not in content.text + async def test_tool_return_value_conversion(self): mcp = MCPServer() mcp.add_tool(tool_fn) From 55732712198b25d5c348450d60ad85fc4317b12b Mon Sep 17 00:00:00 2001 From: Varun Sharma Date: Sat, 7 Mar 2026 12:32:21 +0530 Subject: [PATCH 4/5] fix: use AsyncMock to fix coverage tracking in CI Replace inner async function with patch.object + AsyncMock to avoid coverage blind spot when the mock runs inside the server's task context with pytest-xdist parallel workers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/server/mcpserver/test_server.py | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/tests/server/mcpserver/test_server.py b/tests/server/mcpserver/test_server.py index bffa77953..c49aa8750 100644 --- a/tests/server/mcpserver/test_server.py +++ b/tests/server/mcpserver/test_server.py @@ -307,20 +307,16 @@ async def test_handle_call_tool_defensive_exception_handler(self): mcp = MCPServer() mcp.add_tool(tool_fn) - original_call_tool = mcp.call_tool - - async def patched_call_tool(name: str, arguments: dict[str, Any]) -> Any: - raise RuntimeError("internal db connection string leaked") - async with Client(mcp) as client: - mcp.call_tool = patched_call_tool # type: ignore[assignment] - result = await client.call_tool("tool_fn", {"x": 1, "y": 2}) - mcp.call_tool = original_call_tool # type: ignore[assignment] - assert result.is_error is True - content = result.content[0] - assert isinstance(content, TextContent) - assert "unexpected error" in content.text.lower() - assert "internal db connection string leaked" not in content.text + with patch.object( + mcp, "call_tool", new_callable=AsyncMock, side_effect=RuntimeError("internal db leak") + ): + result = await client.call_tool("tool_fn", {"x": 1, "y": 2}) + assert result.is_error is True + content = result.content[0] + assert isinstance(content, TextContent) + assert "unexpected error" in content.text.lower() + assert "internal db leak" not in content.text async def test_tool_return_value_conversion(self): mcp = MCPServer() From e61a0acb1dc243a012c51e864889b7a969dc2af2 Mon Sep 17 00:00:00 2001 From: Varun Sharma Date: Sat, 7 Mar 2026 12:42:51 +0530 Subject: [PATCH 5/5] style: fix ruff formatting for patch.object call Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/server/mcpserver/test_server.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/server/mcpserver/test_server.py b/tests/server/mcpserver/test_server.py index c49aa8750..c745efbb5 100644 --- a/tests/server/mcpserver/test_server.py +++ b/tests/server/mcpserver/test_server.py @@ -308,9 +308,7 @@ async def test_handle_call_tool_defensive_exception_handler(self): mcp.add_tool(tool_fn) async with Client(mcp) as client: - with patch.object( - mcp, "call_tool", new_callable=AsyncMock, side_effect=RuntimeError("internal db leak") - ): + with patch.object(mcp, "call_tool", new_callable=AsyncMock, side_effect=RuntimeError("internal db leak")): result = await client.call_tool("tool_fn", {"x": 1, "y": 2}) assert result.is_error is True content = result.content[0]