fix(adk): stop leaking Authorization headers in MCP tool error responses - #2439
Open
voltagebots wants to merge 1 commit into
Open
fix(adk): stop leaking Authorization headers in MCP tool error responses#2439voltagebots wants to merge 1 commit into
voltagebots wants to merge 1 commit into
Conversation
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 <franklin.okpako075@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Reworks
ConnectionSafeMcpToolerror handling so failure responses and logs never stringify an exception that may carry request metadata:_connection_error_responseno longer interpolatesstr(error)into the model-visible message and no longer passesexc_info=errorto the logger; it reports only the exception type._extract_http_statuswalks the exception chain and anyBaseExceptionGroup(via__cause__/__context__/.exceptions, with cycle protection) to find anhttpx.HTTPStatusErrorwithout stringifying it, and surfaces a sanitized HTTP-status message.run_asyncnow handles barehttpx.HTTPStatusError,McpErrorwrapping an HTTP status, andBaseExceptionGroupwrapping an HTTP status.Why
An MCP transport failure could previously place
str(error)— which may include the outboundAuthorizationheader — into both the LLM-visible error text and the logs. This is a credential-leak path. The fix keeps error responses actionable (type name / HTTP status / "do not retry") while removing the leak surface.Verified against the pinned transport (
mcp==1.29.0)The credential-leak sanitization is proven by tests to hold regardless of how the error is wrapped (bare
HTTPStatusError,McpError-wrapped,ExceptionGroup-wrapped, or a rawConnectErrorcarrying anAuthorizationheader) — in every case the secret and header name are absent from both the returned dict and the logged output. The connection-error classification itself was checked against the actual pinnedmcppackage source:streamable_http's POST handler runs inside ananyiotask group, which propagates a non-2xxraise_for_status()failure as aBaseExceptionGroupwrappinghttpx.HTTPStatusError— exactly the shape the group-walking branch above handles.Testing
uv run pytest packages/kagent-adk/tests/unittests/test_mcp_connection_error_handling.py— 16/16 passed (python 3.13.15), including all 4 new tests and all 12 pre-existing tests with zero regressions.