fix(mcp): strip URL credentials from server names in raised errors - #4000
fix(mcp): strip URL credentials from server names in raised errors#4000dimaosipa wants to merge 6 commits into
Conversation
MCPServerSse and MCPServerStreamableHttp default their name to the connection
url ("sse: {url}"), which commonly embeds credentials (user:password@ or
?api_key=). every raised connection/tool-call error interpolated that raw name,
so a transient network failure surfaced the credential to the caller. unlike the
log paths, raised errors are not governed by DONT_LOG_TOOL_DATA, so there was no
way to suppress it.
- add MCPServer._error_name, which runs the existing get_mcp_server_log_name
helper. credentials, query, and fragment are stripped; host and path stay so
the error still says which server failed.
- use it for the 16 raised-error sites in server.py, and strip the server name
in the two raised errors in mcp/util.py that sit next to already-redacted
log calls.
- functional uses of server.name (tool namespacing, span identity) are
unchanged.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a36d63d0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The underlying credential-exposure issue is valid, and reusing the existing MCP URL sanitizer is directionally correct. Before we can merge this, please make the redaction cover the complete caller-visible exception graph, not only str() of the outer exception.
The current HTTP wrappers still chain the credential-bearing httpx exception, while the generic MCPUtil wrapper copies and chains the raw exception. Please extract only safe diagnostic fields and raise the sanitized SDK exception without retaining sensitive __cause__ or __context__ data. When tool-data redaction is enabled, generic exception details must also remain hidden.
Please add regression tests asserting that the credentials are absent from the outer message, recursive cause/context chain, and rendered traceback. Once those focused changes are covered, this should be ready for another review.
the first pass only sanitized str() of the outer exception. the httpx error was still chained, so e.request.url (with credentials) stayed reachable through __cause__/__context__, and the generic MCPUtil wrapper copied the raw exception text into the message. - extract only safe diagnostic fields (status code, reason phrase, exception class name) and never retain the raw transport exception while tool-data redaction is on. _raise_user_error_for_http_error becomes _build_user_error_for_http_error so the raise happens outside the except block; setting __context__ before raising does not work, python re-sets it. - cover connect(), cleanup(), list_tools(), call_tool() and the streamable-http override, plus invoke_mcp_tool in mcp/util.py. - hide generic exception details under DONT_LOG_TOOL_DATA. the four tests that assert the failure reason reaches the model now enable tool-data logging explicitly, which is the mode that behavior belongs to. - regression tests assert the credentials are absent from the outer message, the recursive cause/context chain, and the rendered traceback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 075f3bf384
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
the previous pass dropped __cause__/__context__ but still kept the raw exception alive in the frame locals of the raising frames, and still read the exception type name while redacting. - decide redaction where the exception is caught (_transport_error_cause), so nothing holds the httpx error while tool data is redacted. python already clears the "except ... as e" binding, so the only remaining references were the explicit locals, which are now dropped too (connect, cleanup, and the streamable-http exception-group branch). - invoke_mcp_tool no longer inspects the caught exception at all while redacting: a custom MCPServer controls the message, the attribute access, and the type name, so the redacted message is fixed text. - tests assert no SDK traceback frame local holds the credentialed request URL, and that an exception whose type name is a secret and whose __str__/__repr__ raise never reaches the caller-visible message.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79638d0032
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…cting three remaining retention paths from the latest review: - invoke_mcp_tool kept call_task and finished_task in the raising frame. a completed task holds its exception and repr(task) renders it, so the payload was still reachable from frame locals. both references are dropped before the raise, and the frame-locals test now walks task objects too (it fails without the fix). - cleanup kept the exception-group loop target exc after the loop, which outlives it like the named transport locals; cleared alongside them. - the redacted invoke_mcp_tool message no longer reads server.name. an explicit name is caller supplied and a URL-derived one keeps its path, and this message reaches the model through the default failure formatter. this matches get_mcp_server_log_message, which also drops the name while redacting. the transport errors raised from server.py still carry the sanitized server name, since those surface to the developer and need to say which server failed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f43e9119b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
these errors are not developer-only: invoke_mcp_tool re-raises UserError and MCPToolCancellationError unchanged, and default_tool_error_function puts str(error) into the model-visible tool result. so every identifier they carry is model-visible under the default policy. - _error_name returns a fixed "<redacted>" label while tool data is redacted, and the sanitized name (credentials, query, and fragment stripped) only when tool-data logging is enabled. an explicit name is caller supplied and a URL-derived one keeps its path, so neither is safe by default. - the redacted invoke_mcp_tool message and the server-initiated cancellation error drop the tool and server names too. this mirrors the "Invoking MCP tool" debug log in the same function, which already omits both while redacting. - safe diagnostics still survive: status code, reason phrase, and the failure kind stay in the message. - tests cover both modes for default and explicit names.
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for addressing the exception-chain feedback. The latest revision now goes substantially beyond the URL-credential issue: it turns OPENAI_AGENTS_DONT_LOG_TOOL_DATA from a documented logging control into runtime exception policy, hides otherwise safe server and tool identifiers, and adds machinery for task and traceback-frame locals plus hostile exception class names.
Please reset this PR to the narrower contract: always sanitize URL-derived server names independently of the logging flag; ensure auth credentials are absent from the outer message, normal rendered traceback, __cause__, and __context__; and preserve existing non-secret names and diagnostics. Do not attempt to guarantee redaction of arbitrary custom names, exception type names, or third-party frame-local capture.
Please replace the current broad machinery and combinatorial tests with focused regression coverage for that contract.
2 participants
Summary
MCPServerSse/MCPServerStreamableHttpdefault their name to the connection url (sse: {url}), which commonly embeds credentials (user:password@hostor?api_key=...). every raised connection and tool-call error interpolated that raw name, so any transient failure surfaced the credential to the caller.DONT_LOG_TOOL_DATA, so there was no way to suppress this even on the default redact-by-default setting.get_mcp_server_log_name), andserver.pyalready used it for logs. the raised errors just never got it.fix:
MCPServer._error_name, a thin wrapper over that helper. credentials, query, and fragment are stripped; host and path stay so the error still tells you which server failed.server.name(tool namespacing, span identity) are deliberately unchanged.before:
after:
Test plan
_error_nameproperty (default url name redacted, explicit name untouched), the connect / list_tools / call_tool raised errors, and theinvoke_mcp_toolAgentsException. each asserts the password and api key are gone and the host is kept.make formatandmake lint: clean.make tests: full suite green (5770 + 28 passed, 0 failed). tests/mcp/ specifically: 239 passed.make typecheck: adds 0 new errors..agents/skills/code-change-verification/scripts/run.sh: format and lint pass, then it stops at typecheck on 31 pre-existing errors unrelated to this change (vercel extension name-collision, py3.12-only apis on a 3.11 checkout, boto3 not installed, redistype: ignoreskew).Issue number
Checks
.agents/skills/code-change-verification/scripts/run.sh