Repository navigation
Conversation
…tains ## Motivation and Context When the MCP server answers with an HTTP error, `MCP::Client::HTTP` raises `RequestHandlerError` with Faraday's exception as `original_error`, which is also its `cause`. Faraday's `raise_error` middleware keeps the request headers on that exception, bearer token included, and `Faraday::Error#inspect` prints them, so the token reached any log line or error report that inspected the error, while the transport already keeps credentials out of the URLs it quotes. The same exception escapes `close` unwrapped when the server refuses the `DELETE`. The transport now replaces the value of the `Authorization` header in the retained request headers before such an exception leaves it, on requests, notifications, and session termination alike. The request is complete by then, and each request builds its own headers from the connection's defaults, so neither those defaults nor a later request observes the change; the response headers, which carry the `WWW-Authenticate` challenge the OAuth flow reads, are untouched. The `GET` that resumes a stream the server closed early, and the `POST` that answers a request the server sent on such a stream, run while the caller's request is still in flight and can fail the same way, so they are covered too. A JSON middleware installed through the connection block raises `Faraday::ParsingError` on a malformed body, with the `Faraday::Response` itself in place of the Hash `raise_error` builds, so the header is replaced there as well. A middleware installed the same way may have replaced the request headers with a plain Hash or frozen them: the header name is matched regardless of case, and a frozen object is swapped for a copy, so the redaction never raises in place of the error it is redacting. Only the `Authorization` header is replaced; other request headers, `Mcp-Session-Id` among them, stay as they were sent. ## How Has This Been Tested? New tests in `test/mcp/client/http_test.rb` send a request, a notification, and a `DELETE` with a bearer token to a server that answers with an error, and check that the retained error carries `[redacted]` in place of the header and that `inspect` no longer contains the token. All three fail against the previous library. Two more resume a stream the server closed early with a `GET` that fails, and answer a request the server sent on that stream with a `POST` that fails, and check the same; both fail against the previous library. One more answers a server request carried by an SSE body an adapter without streaming support hands over whole, with a `POST` that fails, and checks the same; it fails against the previous library, and also when only the answer's own redaction is removed. Three more install a middleware that parses JSON bodies and answer with a malformed one, freeze the request headers, or replace them with a plain Hash holding a lowercase `authorization` key, and check the same; all three fail against the previous library. One more, in `test/mcp/client/oauth/http_oauth_test.rb`, answers a bearer request with a 401 challenge and checks that the OAuth flow still runs and the retried request carries the token it produced. The challenge names a metadata URL off the well-known paths, which stay unstubbed, so a challenge lost with the redaction would fail the flow. ## Breaking Changes None. `original_error.response[:request][:headers]["Authorization"]` now reads `[redacted]`.
This branch has not been deployed
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.
Motivation and Context
When the MCP server answers with an HTTP error,
MCP::Client::HTTPraisesRequestHandlerErrorwith Faraday's exception asoriginal_error, which is also itscause. Faraday'sraise_errormiddleware keeps the request headers on that exception, bearer token included, andFaraday::Error#inspectprints them, so the token reached any log line or error report that inspected the error, while the transport already keeps credentials out of the URLs it quotes. The same exception escapescloseunwrapped when the server refuses theDELETE.The transport now replaces the value of the
Authorizationheader in the retained request headers before such an exception leaves it, on requests, notifications, and session termination alike. The request is complete by then, and each request builds its own headers from the connection's defaults, so neither those defaults nor a later request observes the change; the response headers, which carry theWWW-Authenticatechallenge the OAuth flow reads, are untouched.The
GETthat resumes a stream the server closed early, and thePOSTthat answers a request the server sent on such a stream, run while the caller's request is still in flight and can fail the same way, so they are covered too. A JSON middleware installed through the connection block raisesFaraday::ParsingErroron a malformed body, with theFaraday::Responseitself in place of the Hashraise_errorbuilds, so the header is replaced there as well. A middleware installed the same way may have replaced the request headers with a plain Hash or frozen them: the header name is matched regardless of case, and a frozen object is swapped for a copy, so the redaction never raises in place of the error it is redacting. Only theAuthorizationheader is replaced; other request headers,Mcp-Session-Idamong them, stay as they were sent.How Has This Been Tested?
New tests in
test/mcp/client/http_test.rbsend a request, a notification, and aDELETEwith a bearer token to a server that answers with an error, and check that the retained error carries[redacted]in place of the header and thatinspectno longer contains the token. All three fail against the previous library. Two more resume a stream the server closed early with aGETthat fails, and answer a request the server sent on that stream with aPOSTthat fails, and check the same; both fail against the previous library. One more answers a server request carried by an SSE body an adapter without streaming support hands over whole, with aPOSTthat fails, and checks the same; it fails against the previous library, and also when only the answer's own redaction is removed.Three more install a middleware that parses JSON bodies and answer with a malformed one, freeze the request headers, or replace them with a plain Hash holding a lowercase
authorizationkey, and check the same; all three fail against the previous library.One more, in
test/mcp/client/oauth/http_oauth_test.rb, answers a bearer request with a 401 challenge and checks that the OAuth flow still runs and the retried request carries the token it produced. The challenge names a metadata URL off the well-known paths, which stay unstubbed, so a challenge lost with the redaction would fail the flow.Breaking Changes
None.
original_error.response[:request][:headers]["Authorization"]now reads[redacted].Types of changes
Checklist