Merge pull request #591 from koic/redact_the_bearer_token_from_retained_transport_errors

Redact the Authorization header from the errors the HTTP transport retains

Author: koic

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.

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"))

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