Skip to content

Commit 7cf339c

Browse files
Varun SharmaCopilot
andcommitted
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>
1 parent 62575ed commit 7cf339c

7 files changed

Lines changed: 28 additions & 11 deletions

File tree

src/mcp/server/mcpserver/server.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
from mcp.server.lowlevel.helper_types import ReadResourceContents
3434
from mcp.server.lowlevel.server import LifespanResultT, Server, request_ctx
3535
from mcp.server.lowlevel.server import lifespan as default_lifespan
36-
from mcp.server.mcpserver.exceptions import ResourceError
36+
from mcp.server.mcpserver.exceptions import ResourceError, ToolError
3737
from mcp.server.mcpserver.prompts import Prompt, PromptManager
3838
from mcp.server.mcpserver.resources import FunctionResource, Resource, ResourceManager
3939
from mcp.server.mcpserver.tools import Tool, ToolManager
@@ -304,8 +304,14 @@ async def _handle_call_tool(
304304
result = await self.call_tool(params.name, params.arguments or {})
305305
except MCPError:
306306
raise
307-
except Exception as e:
307+
except ToolError as e:
308308
return CallToolResult(content=[TextContent(type="text", text=str(e))], is_error=True)
309+
except Exception:
310+
logger.exception(f"Error calling tool {params.name}")
311+
return CallToolResult(
312+
content=[TextContent(type="text", text=f"An unexpected error occurred calling tool {params.name}")],
313+
is_error=True,
314+
)
309315
if isinstance(result, CallToolResult):
310316
return result
311317
if isinstance(result, tuple) and len(result) == 2:

src/mcp/server/mcpserver/tools/base.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import functools
44
import inspect
5+
import logging
56
from collections.abc import Callable
67
from functools import cached_property
78
from typing import TYPE_CHECKING, Any
@@ -15,6 +16,8 @@
1516
from mcp.shared.tool_name_validation import validate_and_warn_tool_name
1617
from mcp.types import Icon, ToolAnnotations
1718

19+
logger = logging.getLogger(__name__)
20+
1821
if TYPE_CHECKING:
1922
from mcp.server.context import LifespanContextT, RequestT
2023
from mcp.server.mcpserver.server import Context
@@ -112,8 +115,11 @@ async def run(
112115
# Re-raise UrlElicitationRequiredError so it can be properly handled
113116
# as an MCP error response with code -32042
114117
raise
118+
except ToolError:
119+
raise
115120
except Exception as e:
116-
raise ToolError(f"Error executing tool {self.name}: {e}") from e
121+
logger.exception(f"Error executing tool {self.name}")
122+
raise ToolError(f"An unexpected error occurred executing tool {self.name}") from e
117123

118124

119125
def _is_async_callable(obj: Any) -> bool:

tests/client/test_list_roots_callback.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,4 +45,4 @@ async def test_list_roots(context: Context[None], message: str):
4545
result = await client.call_tool("test_list_roots", {"message": "test message"})
4646
assert result.is_error is True
4747
assert isinstance(result.content[0], TextContent)
48-
assert result.content[0].text == "Error executing tool test_list_roots: List roots not supported"
48+
assert result.content[0].text == "An unexpected error occurred executing tool test_list_roots"

tests/client/test_sampling_callback.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ async def test_sampling_tool(message: str):
5454
result = await client.call_tool("test_sampling", {"message": "Test message for sampling"})
5555
assert result.is_error is True
5656
assert isinstance(result.content[0], TextContent)
57-
assert result.content[0].text == "Error executing tool test_sampling: Sampling not supported"
57+
assert result.content[0].text == "An unexpected error occurred executing tool test_sampling"
5858

5959

6060
@pytest.mark.anyio

tests/server/mcpserver/test_server.py

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,8 @@ async def test_tool_exception_handling(self):
258258
assert len(result.content) == 1
259259
content = result.content[0]
260260
assert isinstance(content, TextContent)
261-
assert "Test error" in content.text
261+
assert "unexpected error" in content.text.lower()
262+
assert "Test error" not in content.text
262263
assert result.is_error is True
263264

264265
async def test_tool_error_handling(self):
@@ -269,19 +270,21 @@ async def test_tool_error_handling(self):
269270
assert len(result.content) == 1
270271
content = result.content[0]
271272
assert isinstance(content, TextContent)
272-
assert "Test error" in content.text
273+
assert "unexpected error" in content.text.lower()
274+
assert "Test error" not in content.text
273275
assert result.is_error is True
274276

275277
async def test_tool_error_details(self):
276-
"""Test that exception details are properly formatted in the response"""
278+
"""Test that exception details are NOT exposed to the client"""
277279
mcp = MCPServer()
278280
mcp.add_tool(error_tool_fn)
279281
async with Client(mcp) as client:
280282
result = await client.call_tool("error_tool_fn", {})
281283
content = result.content[0]
282284
assert isinstance(content, TextContent)
283285
assert isinstance(content.text, str)
284-
assert "Test error" in content.text
286+
assert "error_tool_fn" in content.text
287+
assert "Test error" not in content.text
285288
assert result.is_error is True
286289

287290
async def test_tool_return_value_conversion(self):

tests/server/mcpserver/test_tool_manager.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -410,7 +410,7 @@ def tool_with_context(x: int, ctx: Context[ServerSessionT, None]) -> str:
410410

411411
mcp = MCPServer()
412412
ctx = mcp.get_context()
413-
with pytest.raises(ToolError, match="Error executing tool tool_with_context"):
413+
with pytest.raises(ToolError, match="unexpected error occurred executing tool tool_with_context"):
414414
await manager.call_tool("tool_with_context", {"x": 42}, context=ctx)
415415

416416

tests/server/mcpserver/test_url_elicitation_error_throw.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,4 +106,6 @@ async def failing_tool(ctx: Context[ServerSession, None]) -> str:
106106
assert result.is_error is True
107107
assert len(result.content) == 1
108108
assert isinstance(result.content[0], types.TextContent)
109-
assert "Something went wrong" in result.content[0].text
109+
assert "An unexpected error occurred executing tool failing_tool" in result.content[0].text
110+
# Verify the original exception details are NOT leaked to the client
111+
assert "Something went wrong" not in result.content[0].text

0 commit comments

Comments
 (0)