Bound OAuth response bodies in the client
## Motivation and Context `MCP::Client::HTTP` caps a single message from the server at 4 MiB, but the OAuth flow talks to authorization endpoints over its own Faraday connection, and that one had no cap. Discovery, dynamic client registration, and token responses were buffered in full and handed to `JSON.parse`, so a server that answered an OAuth request with an endless body grew the client's memory for as long as it kept sending. `Flow` and `IDJAGTokenExchange` each build their own connection, so both needed the bound. The new `BoundedBody` counts bytes in a Faraday `on_data` callback and refuses the response once it passes `MAX_RESPONSE_BYTES`, which matches the 4 MiB of `MCP::Client::HTTP::MAX_MESSAGE_BYTES` and `MCP::Client::Stdio::MAX_LINE_BYTES`. It hands back the status paired with the bounded body rather than the Faraday response, so no later caller can reach the unbounded `response.body`. Those bytes are counted after decompression. The default Net::HTTP adapter negotiates `Accept-Encoding` itself and reads the body through `Net::HTTPResponse#inflater`, so a small compressed body that expands past the cap is refused partway through the expansion. That holds only while the connection leaves `Accept-Encoding` alone, which is now recorded in a comment on both `default_http_client` definitions. An over-cap response is refused rather than truncated: a partial discovery or token document cannot be validated. Protected resource metadata is the one place where refusing is not fatal, since any PRM discovery failure already selects the legacy authorization path by design. ## How Has This Been Tested? New unit tests in `test/mcp/client/oauth/bounded_body_test.rb` cover the streaming path, the at-limit boundary, and the fallback for adapters that ignore `on_data`. One of them serves a gzip body over a real socket rather than through WebMock, which returns a stubbed body verbatim and never runs the inflater; it fails if the cap ever starts measuring compressed bytes. New tests in `test/mcp/client/oauth/flow_test.rb` and `test/mcp/client/oauth/id_jag_token_exchange_test.rb` drive each of the four request wrappers end to end with an over-cap body; all five fail without the cap in place. ## Breaking Changes An OAuth endpoint response larger than 4 MiB is now rejected instead of parsed.
This commit is contained in:
parent
ad73154697
commit
02db089038
3 changed files with 353 additions and 11 deletions
150
test/mcp/client/oauth/bounded_body_test.rb
Normal file
150
test/mcp/client/oauth/bounded_body_test.rb
Normal file
|
|
@ -0,0 +1,150 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
require "test_helper"
|
||||
require "faraday"
|
||||
require "socket"
|
||||
require "stringio"
|
||||
require "zlib"
|
||||
require "mcp/client/oauth"
|
||||
|
||||
module MCP
|
||||
class Client
|
||||
module OAuth
|
||||
class BoundedBodyTest < Minitest::Test
|
||||
def test_on_data_rejects_chunks_that_together_exceed_the_limit
|
||||
bounded = bounded_body(max_bytes: 64)
|
||||
|
||||
error = assert_raises(too_large_error) do
|
||||
4.times do
|
||||
bounded.on_data.call("a" * 32, 32, nil)
|
||||
end
|
||||
end
|
||||
|
||||
assert_includes(error.message, "exceeds 64 bytes")
|
||||
end
|
||||
|
||||
def test_on_data_accepts_a_body_that_stops_at_the_limit
|
||||
bounded = bounded_body(max_bytes: 64)
|
||||
|
||||
2.times do
|
||||
bounded.on_data.call("a" * 32, 32, nil)
|
||||
end
|
||||
|
||||
assert_equal("a" * 64, bounded.response_for(stub_response(body: "")).body)
|
||||
end
|
||||
|
||||
def test_response_for_rejects_an_over_limit_body_when_the_adapter_did_not_stream
|
||||
bounded = bounded_body(max_bytes: 64)
|
||||
|
||||
error = assert_raises(too_large_error) do
|
||||
bounded.response_for(stub_response(body: "a" * 65))
|
||||
end
|
||||
|
||||
assert_includes(error.message, "exceeds 64 bytes")
|
||||
end
|
||||
|
||||
def test_response_for_falls_back_to_the_response_body_when_the_adapter_did_not_stream
|
||||
bounded = bounded_body(max_bytes: 64)
|
||||
|
||||
assert_equal("a" * 64, bounded.response_for(stub_response(body: "a" * 64)).body)
|
||||
end
|
||||
|
||||
def test_response_for_coerces_a_non_string_body
|
||||
bounded = bounded_body(max_bytes: 64)
|
||||
|
||||
assert_equal("", bounded.response_for(stub_response(body: nil)).body)
|
||||
end
|
||||
|
||||
def test_response_for_carries_the_status_through
|
||||
bounded = bounded_body(max_bytes: 64)
|
||||
|
||||
assert_equal(404, bounded.response_for(stub_response(status: 404, body: "")).status)
|
||||
end
|
||||
|
||||
# Served over a real socket rather than WebMock: WebMock hands back the stubbed body verbatim
|
||||
# whatever its `Content-Encoding`, so the adapter's inflater never runs and a stubbed version
|
||||
# of this test would assert the opposite of what it looks like.
|
||||
def test_the_cap_counts_decompressed_bytes
|
||||
bounded = bounded_body(max_bytes: 1024 * 1024)
|
||||
compressed = gzip("a" * (5 * 1024 * 1024))
|
||||
|
||||
assert_operator(compressed.bytesize, :<, 1024 * 1024, "the compressed body must fit under the cap")
|
||||
|
||||
serving_gzip(compressed) do |url|
|
||||
assert_raises(too_large_error) do
|
||||
Faraday.new.get(url) do |req|
|
||||
req.options.on_data = bounded.on_data
|
||||
req.options.timeout = 5
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def gzip(content)
|
||||
io = StringIO.new
|
||||
|
||||
writer = Zlib::GzipWriter.new(io)
|
||||
writer.write(content)
|
||||
writer.close
|
||||
|
||||
io.string
|
||||
end
|
||||
|
||||
def serving_gzip(body)
|
||||
# Other test files load `webmock/minitest`, which blocks real connections process-wide.
|
||||
# This is the one place that needs the adapter's own request path.
|
||||
WebMock.disable! if defined?(WebMock)
|
||||
|
||||
server = TCPServer.new("127.0.0.1", 0)
|
||||
thread = Thread.new do
|
||||
socket = server.accept
|
||||
|
||||
loop do
|
||||
line = socket.gets
|
||||
|
||||
break if line.nil? || line == "\r\n"
|
||||
end
|
||||
|
||||
socket.write(
|
||||
"HTTP/1.1 200 OK\r\n" \
|
||||
"Content-Type: application/json\r\n" \
|
||||
"Content-Encoding: gzip\r\n" \
|
||||
"Content-Length: #{body.bytesize}\r\n" \
|
||||
"Connection: close\r\n\r\n",
|
||||
)
|
||||
socket.write(body)
|
||||
|
||||
socket.close
|
||||
rescue IOError, SystemCallError
|
||||
nil
|
||||
end
|
||||
|
||||
# Scoped to start after both are assigned, so the cleanup below never runs against `nil`
|
||||
# and a failure to open the socket surfaces as itself rather than as a `NoMethodError`.
|
||||
begin
|
||||
yield("http://127.0.0.1:#{server.addr[1]}/")
|
||||
ensure
|
||||
thread.kill
|
||||
server.close
|
||||
end
|
||||
ensure
|
||||
WebMock.enable! if defined?(WebMock)
|
||||
end
|
||||
|
||||
def bounded_body(max_bytes:)
|
||||
OAuth.const_get(:BoundedBody).new(max_bytes: max_bytes)
|
||||
end
|
||||
|
||||
def too_large_error
|
||||
OAuth.const_get(:BoundedBody)::TooLargeError
|
||||
end
|
||||
|
||||
def stub_response(body:, status: 200)
|
||||
Struct.new(:status, :body).new(status, body)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
@ -948,6 +948,69 @@ module MCP
|
|||
assert_match(/authorization_servers/i, error.message)
|
||||
end
|
||||
|
||||
def test_run_refuses_an_authorization_server_metadata_body_over_the_cap
|
||||
stub_request(:get, @as_metadata_url).to_return(
|
||||
status: 200,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
body: over_cap_body,
|
||||
)
|
||||
|
||||
error = assert_raises(Flow::AuthorizationError) { run_authorization_flow }
|
||||
|
||||
assert_match(/exceeds \d+ bytes/, error.message)
|
||||
end
|
||||
|
||||
def test_run_refuses_a_protected_resource_metadata_body_over_the_cap
|
||||
# PRM discovery failures select the legacy path by design, so the refusal shows up as
|
||||
# the fallback rather than as a raise. The padded document is valid JSON naming
|
||||
# an authorization server, so contacting that server is exactly what would happen
|
||||
# if the body had been read: the assertion below fails if the cap stops working.
|
||||
stub_request(:any, %r{\Ahttps://srv\.example\.com/}).to_return(status: 404)
|
||||
stub_request(:get, @prm_url).to_return(
|
||||
status: 200,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
body: JSON.generate(
|
||||
resource: "https://srv.example.com/mcp",
|
||||
authorization_servers: [@auth_base],
|
||||
padding: "a" * (4 * 1024 * 1024),
|
||||
),
|
||||
)
|
||||
|
||||
assert_raises(Flow::AuthorizationError) { run_authorization_flow }
|
||||
|
||||
assert_not_requested(:get, @as_metadata_url)
|
||||
end
|
||||
|
||||
def test_run_refuses_a_dynamic_client_registration_body_over_the_cap
|
||||
stub_request(:post, "#{@auth_base}/register").to_return(
|
||||
status: 201,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
body: over_cap_body,
|
||||
)
|
||||
|
||||
error = assert_raises(Flow::AuthorizationError) { run_authorization_flow }
|
||||
|
||||
assert_match(/exceeds \d+ bytes/, error.message)
|
||||
end
|
||||
|
||||
def test_run_refuses_a_token_endpoint_body_over_the_cap
|
||||
stub_request(:post, "#{@auth_base}/token").to_return(
|
||||
status: 200,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
body: over_cap_body,
|
||||
)
|
||||
|
||||
error = assert_raises(Flow::AuthorizationError) { run_authorization_flow }
|
||||
|
||||
assert_match(/exceeds \d+ bytes/, error.message)
|
||||
end
|
||||
|
||||
# One byte past the cap, so the three request wrappers are each proven to install the streaming callback.
|
||||
# A wrapper that forgot it would buffer this whole body and parse it.
|
||||
def over_cap_body
|
||||
"a" * (4 * 1024 * 1024 + 1)
|
||||
end
|
||||
|
||||
def test_run_raises_when_token_response_is_not_a_json_object
|
||||
# The token endpoint MUST return a JSON object per RFC 6749 §5.1.
|
||||
# A non-object body would otherwise be persisted into the provider
|
||||
|
|
|
|||
|
|
@ -101,6 +101,17 @@ module MCP
|
|||
error = assert_raises(IDJAGTokenExchange::ExchangeError) { request_exchange }
|
||||
assert_match(/not a JSON object/, error.message)
|
||||
end
|
||||
|
||||
def test_request_refuses_a_response_body_over_the_cap
|
||||
stub_request(:post, @token_endpoint).to_return(
|
||||
status: 200,
|
||||
headers: { "Content-Type" => "application/json" },
|
||||
body: "a" * (4 * 1024 * 1024 + 1),
|
||||
)
|
||||
|
||||
error = assert_raises(IDJAGTokenExchange::ExchangeError) { request_exchange }
|
||||
assert_match(/exceeds \d+ bytes/, error.message)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
|||
Loading…
Reference in a new issue