From 19d67b7eff710c359ee980a4db2354ab7b45ab2a Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Wed, 7 Oct 2026 15:19:39 +0900 Subject: [PATCH] Redact the Authorization header from the errors the HTTP transport retains ## 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]`. --- lib/mcp/client/http.rb | 71 +++++++-- test/mcp/client/http_test.rb | 189 +++++++++++++++++++++++ test/mcp/client/oauth/http_oauth_test.rb | 32 ++++ 3 files changed, 281 insertions(+), 11 deletions(-) diff --git a/lib/mcp/client/http.rb b/lib/mcp/client/http.rb index 0a45b8e6..f224508c 100644 --- a/lib/mcp/client/http.rb +++ b/lib/mcp/client/http.rb @@ -418,8 +418,10 @@ def send_request(request:) yield if block_given? response = begin - client.post("", request, session_headers.merge(request_metadata_headers(method, params))) do |req| - req.options.on_data = stream.on_data + redacting_authorization_header do + client.post("", request, session_headers.merge(request_metadata_headers(method, params))) do |req| + req.options.on_data = stream.on_data + end end rescue StreamAbort nil @@ -527,7 +529,7 @@ def send_request(request:) def send_notification(notification:) method = notification[:method] || notification["method"] - client.post("", notification, session_headers) + redacting_authorization_header { client.post("", notification, session_headers) } nil rescue Faraday::Error => e raise RequestHandlerError.new( @@ -555,7 +557,7 @@ def close end begin - client.delete("", nil, session_headers) + redacting_authorization_header { client.delete("", nil, session_headers) } rescue Faraday::ClientError => e raise unless [404, 405].include?(e.response&.dig(:status)) ensure @@ -1050,6 +1052,51 @@ def remaining_reconnection_budget(deadline) [deadline - Process.clock_gettime(Process::CLOCK_MONOTONIC), 0.001].max end + # Runs a request on the transport's connection and, when Faraday raises, replaces the value of the `Authorization` header + # in the request headers the exception retains before it leaves the transport. + # Faraday's `raise_error` middleware keeps a reference to `env.request_headers` in `Faraday::Error#response` + # under `:request`, so the exception that becomes `RequestHandlerError#original_error` (and its `cause`) would otherwise print + # the bearer token through `inspect`, which is what error reporters and log lines do with it. The request is complete by + # the time the exception surfaces, and every request builds its own headers from the connection's defaults, + # so the rewrite reaches neither those defaults nor a later request. + def redacting_authorization_header + yield + rescue Faraday::Error => e + redact_authorization_header!(e) + raise + end + + # `raise_error` retains the request as a Hash under `:request`; a JSON middleware the customizer added raises + # `Faraday::ParsingError` with the `Faraday::Response` itself, whose `env` holds the request headers. + def redact_authorization_header!(error) + response = error.response + + if response.is_a?(Hash) + request = response[:request] + redact_authorization_header_in!(request, :headers) if request.is_a?(Hash) + elsif response.respond_to?(:env) && response.env.respond_to?(:request_headers) + redact_authorization_header_in!(response.env, :request_headers) + end + end + + # The headers are a `Faraday::Utils::Headers`, which spells the name `Authorization` whatever the caller wrote, + # unless a customizer middleware replaced them with a plain Hash or froze them: the name is matched regardless + # of case so such a Hash is covered too, and a frozen object is swapped for a copy, so the redaction never + # raises in place of the error it is redacting. + def redact_authorization_header_in!(holder, key) + headers = holder[key] + return unless headers.is_a?(Hash) + + names = headers.each_key.select { |name| name.to_s.casecmp?("authorization") } + return if names.empty? + + headers = holder[key] = headers.dup if headers.frozen? + + names.each do |name| + headers[name] = "[redacted]" + end + end + def require_faraday! require "faraday" rescue LoadError @@ -1162,7 +1209,7 @@ def dispatch_server_request(message) end def send_client_response(response) - client.post("", response, session_headers) + redacting_authorization_header { client.post("", response, session_headers) } end def parse_json_buffer(buffer, method, params) @@ -1225,12 +1272,14 @@ def await_response_after_disconnect(stream, method, params) read_timeout = remaining_reconnection_budget(deadline) reconnect_response = begin - client.get("") do |req| - req.headers.update(session_headers) - req.headers["Accept"] = SSE_ACCEPT_HEADER - req.headers[LAST_EVENT_ID_HEADER] = stream.last_event_id if stream.last_event_id - req.options.read_timeout = read_timeout - req.options.on_data = stream.on_data + redacting_authorization_header do + client.get("") do |req| + req.headers.update(session_headers) + req.headers["Accept"] = SSE_ACCEPT_HEADER + req.headers[LAST_EVENT_ID_HEADER] = stream.last_event_id if stream.last_event_id + req.options.read_timeout = read_timeout + req.options.on_data = stream.on_data + end end rescue StreamAbort # The awaited response arrived on the reconnected stream. diff --git a/test/mcp/client/http_test.rb b/test/mcp/client/http_test.rb index 57828e65..682939d4 100644 --- a/test/mcp/client/http_test.rb +++ b/test/mcp/client/http_test.rb @@ -417,6 +417,174 @@ def test_send_request_raises_unauthorized_error assert_equal({ method: "tools/list", params: nil }, error.request) end + def test_send_request_redacts_the_authorization_header_from_the_original_error + # Faraday keeps the request headers on the error it raises, so without redaction the bearer token + # would travel into logs through `original_error` (also the `cause`) and its `inspect`. + client = HTTP.new(url: url, headers: { "Authorization" => "Bearer secret-token" }) + stub_request(:post, url).to_return(status: 401) + + error = assert_raises(RequestHandlerError) do + client.send_request(request: { jsonrpc: "2.0", id: "test_id", method: "tools/list" }) + end + + assert_equal("[redacted]", error.original_error.response[:request][:headers]["Authorization"]) + assert_same(error.original_error, error.cause) + refute_includes(error.original_error.inspect, "secret-token") + end + + def test_send_notification_redacts_the_authorization_header_from_the_original_error + client = HTTP.new(url: url, headers: { "Authorization" => "Bearer secret-token" }) + stub_request(:post, url).to_return(status: 500) + + error = assert_raises(RequestHandlerError) do + client.send_notification(notification: { jsonrpc: "2.0", method: "notifications/initialized" }) + end + + assert_equal("[redacted]", error.original_error.response[:request][:headers]["Authorization"]) + refute_includes(error.original_error.inspect, "secret-token") + end + + def test_resuming_a_stream_redacts_the_authorization_header_from_the_original_error + # The server closes the stream after a priming event (SEP-1699), so the client resumes it with + # a GET carrying `Last-Event-ID`; that GET runs after the initial POST completes, still inside `send_request`. + client = HTTP.new(url: url, headers: { "Authorization" => "Bearer secret-token" }) + request = { jsonrpc: "2.0", id: "test_id", method: "tools/call", params: { name: "test_tool", arguments: {} } } + stub_request(:post, url).with(body: request.to_json).to_return( + status: 200, + headers: { "Content-Type" => "text/event-stream" }, + body: "id: event-1\nretry: 10\ndata:\n\n", + ) + stub_request(:get, url).with(headers: { "Last-Event-ID" => "event-1" }).to_return(status: 500) + + error = assert_raises(RequestHandlerError) do + client.send_request(request: request) + end + + assert_equal("[redacted]", error.original_error.response[:request][:headers]["Authorization"]) + assert_same(error.original_error, error.cause) + refute_includes(error.original_error.inspect, "secret-token") + end + + def test_answering_a_server_request_redacts_the_authorization_header_from_the_original_error + # The resumed stream carries a request from the server, which the client answers with a POST of its own. + client = HTTP.new(url: url, headers: { "Authorization" => "Bearer secret-token" }) + request = { jsonrpc: "2.0", id: "test_id", method: "tools/call", params: { name: "test_tool", arguments: {} } } + stub_request(:post, url).with(body: request.to_json).to_return( + status: 200, + headers: { "Content-Type" => "text/event-stream" }, + body: "id: event-1\nretry: 10\ndata:\n\n", + ) + stub_request(:get, url).with(headers: { "Last-Event-ID" => "event-1" }).to_return( + status: 200, + headers: { "Content-Type" => "text/event-stream" }, + body: "event: message\nid: event-2\ndata: {\"jsonrpc\":\"2.0\",\"id\":\"server_id\",\"method\":\"roots/list\"}\n\n", + ) + stub_request(:post, url).with { |answer| answer.body.include?("server_id") }.to_return(status: 500) + + error = assert_raises(RequestHandlerError) do + client.send_request(request: request) + end + + assert_equal("[redacted]", error.original_error.response[:request][:headers]["Authorization"]) + assert_same(error.original_error, error.cause) + refute_includes(error.original_error.inspect, "secret-token") + end + + def test_answering_a_buffered_server_request_redacts_the_authorization_header_from_the_original_error + # An adapter without streaming support, like the test adapter, hands the SSE body over whole once + # the POST has completed, so the answer to a server request it carries is sent outside that POST + # and only the answer's own redaction covers it. + stubs = Faraday::Adapter::Test::Stubs.new do |stub| + stub.post("/") do |env| + if env.body.include?("server_id") + [500, {}, ""] + else + [ + 200, + { "Content-Type" => "text/event-stream" }, + "event: message\ndata: {\"jsonrpc\":\"2.0\",\"id\":\"server_id\",\"method\":\"roots/list\"}\n\n", + ] + end + end + end + client = HTTP.new(url: url, headers: { "Authorization" => "Bearer secret-token" }) do |faraday| + faraday.adapter(:test, stubs) + end + + error = assert_raises(RequestHandlerError) do + client.send_request(request: { jsonrpc: "2.0", id: "test_id", method: "tools/list" }) + end + + assert_equal("[redacted]", error.original_error.response[:request][:headers]["Authorization"]) + assert_same(error.original_error, error.cause) + refute_includes(error.original_error.inspect, "secret-token") + end + + def test_send_request_redacts_the_authorization_header_from_a_parsing_error + # A JSON middleware added through the connection block raises `Faraday::ParsingError` on a malformed body, + # and that error retains the `Faraday::Response` itself rather than the Hash `raise_error` builds. + # The body reaches the middleware through an adapter without streaming support, like the test adapter. + stubs = Faraday::Adapter::Test::Stubs.new do |stub| + stub.post("/") { [200, { "Content-Type" => "application/json" }, "{"] } + end + client = HTTP.new(url: url, headers: { "Authorization" => "Bearer secret-token" }) do |faraday| + faraday.response(:json) + faraday.adapter(:test, stubs) + end + + error = assert_raises(RequestHandlerError) do + client.send_request(request: { jsonrpc: "2.0", id: "test_id", method: "tools/list" }) + end + + assert_instance_of(Faraday::ParsingError, error.original_error) + assert_equal("[redacted]", error.original_error.response.env.request_headers["Authorization"]) + refute_includes(error.original_error.inspect, "secret-token") + end + + def test_send_request_redacts_frozen_request_headers_through_a_copy + # A middleware that freezes the request headers must not turn the redaction into a `FrozenError` raised + # in place of the HTTP error, which would also skip the OAuth retry and the `RequestHandlerError` wrapping. + freezing = Class.new(Faraday::Middleware) do + def call(env) + env.request_headers.freeze + @app.call(env) + end + end + client = HTTP.new(url: url, headers: { "Authorization" => "Bearer secret-token" }) do |faraday| + faraday.use(freezing) + end + stub_request(:post, url).to_return(status: 401) + + error = assert_raises(RequestHandlerError) do + client.send_request(request: { jsonrpc: "2.0", id: "test_id", method: "tools/list" }) + end + + assert_equal("[redacted]", error.original_error.response[:request][:headers]["Authorization"]) + refute_includes(error.original_error.inspect, "secret-token") + end + + def test_send_request_redacts_a_lowercase_authorization_key_in_replaced_request_headers + # A middleware that replaces the headers with a plain Hash loses the canonical spelling + # `Faraday::Utils::Headers` guarantees, so the name is matched regardless of case. + downcasing = Class.new(Faraday::Middleware) do + def call(env) + env.request_headers = env.request_headers.to_h.transform_keys(&:downcase) + @app.call(env) + end + end + client = HTTP.new(url: url, headers: { "Authorization" => "Bearer secret-token" }) do |faraday| + faraday.use(downcasing) + end + stub_request(:post, url).to_return(status: 401) + + error = assert_raises(RequestHandlerError) do + client.send_request(request: { jsonrpc: "2.0", id: "test_id", method: "tools/list" }) + end + + assert_equal("[redacted]", error.original_error.response[:request][:headers]["authorization"]) + refute_includes(error.original_error.inspect, "secret-token") + end + def test_send_request_raises_forbidden_error request = { jsonrpc: "2.0", @@ -2010,6 +2178,27 @@ def test_close_propagates_unauthorized_and_still_clears_state assert_nil(client.session_id) end + def test_close_redacts_the_authorization_header_from_the_propagated_error + client = HTTP.new(url: url, headers: { "Authorization" => "Bearer secret-token" }) + stub_request(:post, url).to_return( + status: 200, + headers: { "Content-Type" => "application/json", "Mcp-Session-Id" => "session-abc" }, + body: { jsonrpc: "2.0", result: { protocolVersion: "2025-11-25" } }.to_json, + ) + client.send_request(request: { jsonrpc: "2.0", id: "1", method: "initialize" }) + stub_request(:delete, url).to_return(status: 401) + + error = assert_raises(Faraday::UnauthorizedError) do + client.close + end + + assert_equal("[redacted]", error.response[:request][:headers]["Authorization"]) + + # Only the `Authorization` header is replaced; the session header stays as it was sent. + assert_equal("session-abc", error.response[:request][:headers]["Mcp-Session-Id"]) + refute_includes(error.inspect, "secret-token") + end + def test_close_propagates_connection_failure_and_still_clears_state initialize_session stub_request(:delete, url).to_raise(Faraday::ConnectionFailed.new("connection refused")) diff --git a/test/mcp/client/oauth/http_oauth_test.rb b/test/mcp/client/oauth/http_oauth_test.rb index 0a3cd384..4f047bc6 100644 --- a/test/mcp/client/oauth/http_oauth_test.rb +++ b/test/mcp/client/oauth/http_oauth_test.rb @@ -308,6 +308,38 @@ def test_send_request_runs_the_oauth_flow_through_the_provider_customizer ) end + def test_send_request_keeps_the_challenge_and_the_retried_bearer_through_the_redaction + # The 401 error is redacted before the OAuth flow reads its `WWW-Authenticate` challenge, and the retry + # builds its own headers, so the flow still runs and the retried request carries the token it produced. + # The challenge names a metadata URL off the well-known paths, which are left unstubbed: were the challenge + # lost with the redaction, discovery would fall back to those paths and the flow would fail. + @prm_url = "https://srv.example.com/oauth/protected-resource" + stub_step_up_authorization_server + stub_request(:post, @mcp_url).with( + headers: { "Authorization" => "Bearer initial-token" } + ).to_return( + status: 401, + headers: { "WWW-Authenticate" => %(Bearer error="invalid_token", resource_metadata="#{@prm_url}") }, + body: "", + ) + stub_request(:post, @mcp_url).with( + headers: { "Authorization" => "Bearer escalated-token" } + ).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(jsonrpc: "2.0", id: "1", result: { ok: true }), + ) + provider = build_step_up_provider + + transport = HTTP.new(url: @mcp_url, oauth: provider) + response = transport.send_request(request: { jsonrpc: "2.0", id: "1", method: "tools/list" }) + + assert_equal({ "ok" => true }, response["result"]) + assert_equal("escalated-token", provider.access_token) + assert_requested(:post, @mcp_url, headers: { "Authorization" => "Bearer initial-token" }, times: 1) + assert_requested(:post, @mcp_url, headers: { "Authorization" => "Bearer escalated-token" }, times: 1) + end + def test_send_request_does_not_follow_a_resource_metadata_challenge_off_the_server_origin # End to end over the transport, which is where the header is actually parsed: # a server that answers 401 must not be able to name an unrelated host in