From c96e0e5041f35b47bed12a053a712bbe90b6828b Mon Sep 17 00:00:00 2001 From: Franklin Okpako Date: Thu, 13 Aug 2026 21:23:41 +0100 Subject: [PATCH] fix(adk): stop leaking Authorization headers in MCP tool error responses Reworks ConnectionSafeMcpTool error handling so failure responses and logs never stringify an exception that may carry request metadata. _connection_error_response no longer interpolates str(error) or passes exc_info=error; a new _extract_http_status walks the exception chain and any BaseExceptionGroup to find an httpx.HTTPStatusError without stringifying it, surfacing a sanitized HTTP-status message instead. Covers bare httpx.HTTPStatusError, McpError wrapping an HTTP status, and BaseExceptionGroup wrapping an HTTP status (confirmed against the pinned mcp==1.29.0 transport, which propagates via an anyio task group). Signed-off-by: Franklin Okpako --- .../kagent-adk/src/kagent/adk/_mcp_toolset.py | 56 +++++++++- .../test_mcp_connection_error_handling.py | 104 +++++++++++++++++- 2 files changed, 154 insertions(+), 6 deletions(-) diff --git a/python/packages/kagent-adk/src/kagent/adk/_mcp_toolset.py b/python/packages/kagent-adk/src/kagent/adk/_mcp_toolset.py index d3e7b9b65..b4f165422 100644 --- a/python/packages/kagent-adk/src/kagent/adk/_mcp_toolset.py +++ b/python/packages/kagent-adk/src/kagent/adk/_mcp_toolset.py @@ -71,6 +71,28 @@ def _enrich_cancelled_error(error: BaseException) -> asyncio.CancelledError: return asyncio.CancelledError(message) +def _extract_http_status(error: BaseException) -> Optional[int]: + """Return the HTTP status code carried by an httpx.HTTPStatusError found + anywhere in the exception's chain or group, without stringifying the error + (which could leak Authorization headers). Returns None if absent.""" + seen: set[int] = set() + stack: list[BaseException] = [error] + while stack: + current = stack.pop() + if id(current) in seen: + continue + seen.add(id(current)) + if isinstance(current, httpx.HTTPStatusError): + return current.response.status_code + if isinstance(current, BaseExceptionGroup): + stack.extend(current.exceptions) + if current.__cause__ is not None: + stack.append(current.__cause__) + if current.__context__ is not None: + stack.append(current.__context__) + return None + + class ConnectionSafeMcpTool(McpTool): """McpTool wrapper that catches connection errors and returns them as error text to the LLM instead of raising. @@ -96,14 +118,30 @@ def __init__(self, inner_tool: McpTool): def __getattr__(self, name: str) -> Any: return getattr(self._inner_tool, name) - def _connection_error_response(self, error: Exception) -> dict[str, Any]: + def _connection_error_response(self, error: BaseException) -> dict[str, Any]: error_message = ( - f"MCP tool '{self.name}' failed due to a connection error: " - f"{type(error).__name__}: {error}. " + f"MCP tool '{self.name}' failed due to a connection error " + f"({type(error).__name__}). " "The MCP server may be unreachable. " "Do not retry this tool — inform the user about the failure." ) - logger.error(error_message, exc_info=error) + logger.error( + "MCP tool '%s' failed due to a connection error (%s)", + self.name, + type(error).__name__, + ) + return {"error": error_message} + + def _http_status_error_response(self, status_code: int) -> dict[str, Any]: + error_message = ( + f"MCP tool '{self.name}' failed because the upstream returned HTTP " + f"{status_code}. Do not retry this tool — inform the user about the failure." + ) + logger.warning( + "MCP tool '%s' received upstream HTTP status %s", + self.name, + status_code, + ) return {"error": error_message} async def run_async( @@ -116,10 +154,20 @@ async def run_async( return await self._inner_tool.run_async(args=args, tool_context=tool_context) except _CONNECTION_ERROR_TYPES as error: return self._connection_error_response(error) + except httpx.HTTPStatusError as error: + return self._http_status_error_response(error.response.status_code) except McpError as error: + status_code = _extract_http_status(error) + if status_code is not None: + return self._http_status_error_response(status_code) if not _is_transport_mcp_error(error): raise return self._connection_error_response(error) + except BaseExceptionGroup as error: + status_code = _extract_http_status(error) + if status_code is not None: + return self._http_status_error_response(status_code) + raise class KAgentMcpToolset(McpToolset): diff --git a/python/packages/kagent-adk/tests/unittests/test_mcp_connection_error_handling.py b/python/packages/kagent-adk/tests/unittests/test_mcp_connection_error_handling.py index d1adbfadb..253c851f0 100644 --- a/python/packages/kagent-adk/tests/unittests/test_mcp_connection_error_handling.py +++ b/python/packages/kagent-adk/tests/unittests/test_mcp_connection_error_handling.py @@ -49,7 +49,7 @@ async def test_connection_reset_error_returns_error_dict(): assert "error" in result assert "ConnectionResetError" in result["error"] - assert "Connection reset by peer" in result["error"] + assert "Connection reset by peer" not in result["error"] assert "Do not retry" in result["error"] @@ -117,7 +117,7 @@ async def test_transport_mcp_error_returns_error_dict(): assert "error" in result assert "McpError" in result["error"] - assert "session read timeout" in result["error"] + assert "session read timeout" not in result["error"] @pytest.mark.asyncio @@ -197,3 +197,103 @@ async def mock_super_get_tools(self_arg, readonly_context=None): # Model-visible app tool is recorded; app-only tool is not. assert "get_weather" in app_tool_names assert "refresh_dashboard" not in app_tool_names + + +@pytest.mark.asyncio +async def test_http_status_error_returns_sanitized_error_dict_and_logs_warning(): + """HTTP status failures are returned without leaking request metadata.""" + credential = "super-secret-token" + request = httpx.Request( + "POST", + "http://x", + headers={"Authorization": f"Bearer {credential}"}, + ) + response = httpx.Response(401, request=request) + tool = _make_connection_safe_tool( + httpx.HTTPStatusError("Unauthorized", request=request, response=response) + ) + + with patch("kagent.adk._mcp_toolset.logger") as mock_logger: + result = await tool.run_async(args={}, tool_context=MagicMock()) + + assert isinstance(result, dict) + assert "error" in result + assert "401" in result["error"] + assert credential not in result["error"] + assert "Authorization" not in result["error"] + mock_logger.warning.assert_called_once() + assert credential not in str(mock_logger.warning.call_args) + assert "Authorization" not in str(mock_logger.warning.call_args) + + +@pytest.mark.asyncio +async def test_mcp_error_wrapping_http_status_returns_sanitized_dict(): + """An McpError whose cause is an httpx.HTTPStatusError is reported as an HTTP + status failure without leaking the Authorization header.""" + credential = "super-secret-token" + request = httpx.Request( + "POST", + "http://x", + headers={"Authorization": f"Bearer {credential}"}, + ) + response = httpx.Response(403, request=request) + http_error = httpx.HTTPStatusError("Forbidden", request=request, response=response) + mcp_error = McpError(ErrorData(code=-1, message="upstream error")) + mcp_error.__cause__ = http_error + tool = _make_connection_safe_tool(mcp_error) + + result = await tool.run_async(args={}, tool_context=MagicMock()) + + assert "error" in result + assert "403" in result["error"] + assert credential not in result["error"] + assert "Authorization" not in result["error"] + + +@pytest.mark.asyncio +async def test_exception_group_wrapping_http_status_returns_sanitized_dict(): + """An ExceptionGroup carrying an httpx.HTTPStatusError is reported as an HTTP + status failure without leaking the Authorization header.""" + credential = "super-secret-token" + request = httpx.Request( + "POST", + "http://x", + headers={"Authorization": f"Bearer {credential}"}, + ) + response = httpx.Response(502, request=request) + http_error = httpx.HTTPStatusError("Bad Gateway", request=request, response=response) + group = ExceptionGroup("mcp session failed", [http_error]) + tool = _make_connection_safe_tool(group) + + result = await tool.run_async(args={}, tool_context=MagicMock()) + + assert "error" in result + assert "502" in result["error"] + assert credential not in result["error"] + assert "Authorization" not in result["error"] + + +@pytest.mark.asyncio +async def test_connection_error_response_does_not_leak_wrapped_credentials(): + """A connection error carrying an httpx request with an Authorization header + must not leak that header via str(error) or exc_info.""" + credential = "super-secret-token" + request = httpx.Request( + "GET", + "http://x", + headers={"Authorization": f"Bearer {credential}"}, + ) + error = httpx.ConnectError("connection refused", request=request) + + tool = _make_connection_safe_tool(error) + with patch("kagent.adk._mcp_toolset.logger") as mock_logger: + result = await tool.run_async(args={}, tool_context=MagicMock()) + + assert "error" in result + assert "ConnectError" in result["error"] + assert credential not in result["error"] + assert "Authorization" not in result["error"] + mock_logger.error.assert_called_once() + assert mock_logger.error.call_args.kwargs.get("exc_info") is None + assert credential not in str(mock_logger.error.call_args) + assert "Authorization" not in str(mock_logger.error.call_args)