Bound Request Body and Frame Reads to Prevent Memory-Exhaustion DoS
## Motivation and Context Both transports read a peer-controlled message fully into memory with no upper bound, so a single unauthenticated message can allocate gigabytes and OOM the worker: - `StreamableHTTPTransport#handle_post` did `request.body.read` with no size limit before `JSON.parse`. A single ~512 MB POST grew a worker from 44 MB to 1.66 GB RSS in the reporter's measurement. The path runs before session validation, so it is reachable unauthenticated in both stateful and `stateless: true` modes. - `StdioTransport` and `MCP::Client::Stdio` read frames with `IO#gets` and no `limit:`, so a peer that never emits a newline grows a single String until the process is OOM-killed. This matters when a host pipes an untrusted server's stdout into an unconstrained process. The spec does not mandate a request size limit, and the reference SDKs vary (the TypeScript SDK caps at a 4 MB default; Python/Go delegate to the web framework). A finite default here bounds the allocation at the transport layer regardless of the deployment, matching the TypeScript SDK's default. - `StreamableHTTPTransport` gains `max_request_bytes:` (default 4 MiB). `handle_post` rejects a declared `Content-Length` over the cap before reading, and also bounds the actual read to one byte past the cap so a missing or spoofed `Content-Length` (chunked transfer) is still caught, returning HTTP 413. `parse_request_body` now passes `max_nesting:` (64) so a deeply nested body cannot exhaust the stack (complements the byte cap). The constructor validates the new option as a positive Integer, since `nil` or a non-positive value makes the underlying read unbounded and would silently disable the protection. - The stdio `max_line_bytes:` bound (default 4 MiB) has since been merged upstream through a separate advisory, so this change carries the remaining piece of the reported stdio fix: an over-limit frame leaves leftover bytes in the pipe, desyncing the stream beyond recovery, so `MCP::Client::Stdio` now closes the transport before raising and a later `send_request` fails cleanly instead of parsing a truncated frame. ## How Has This Been Tested? New tests in `test/mcp/server/transports/streamable_http_transport_test.rb` cover the 413 rejection with and without `Content-Length`, the boundary body being accepted, the nesting-depth rejection, and the constructor validation. The client stdio over-limit test now also asserts the transport is closed afterward (`refute_predicate(transport, :connected?)`). `bundle exec rake` (tests, RuboCop, and conformance baseline) passes. Verified end-to-end that a 256 MB POST with no declared `Content-Length` is rejected with 413 without the transport reading the whole body (a throttled `rack.input` that raises if read far past the cap is never tripped). ## Breaking Changes None. The 4 MiB defaults comfortably fit a typical JSON-RPC message (a 4 MiB JSON string decodes to roughly 3 MiB of base64 payload), and the new keyword arguments are additive and optional.
This commit is contained in:
Koichi ITO
parent
0ede5fc146
commit
772e0cb1f9
2 changed files with 143 additions and 3 deletions
|
|
@ -1111,6 +1111,9 @@ module MCP
|
|||
|
||||
assert_match(/exceeds 1024 bytes without a newline/, error.message)
|
||||
assert_equal(:internal_error, error.error_type)
|
||||
# The stream is desynced (leftover bytes in the pipe), so the transport is closed
|
||||
# rather than left resumable on a corrupt stream.
|
||||
refute_predicate(transport, :connected?)
|
||||
ensure
|
||||
server_thread.join
|
||||
stdin_read.close
|
||||
|
|
|
|||
|
|
@ -4441,6 +4441,73 @@ module MCP
|
|||
refute headers.frozen?, "SSE response headers should not be frozen"
|
||||
end
|
||||
|
||||
test "rejects a request body over max_request_bytes with 413 without reading it all" do
|
||||
transport = StreamableHTTPTransport.new(@server, max_request_bytes: 1024)
|
||||
oversized = { jsonrpc: "2.0", method: "initialize", id: "1", params: { x: "A" * 4096 } }.to_json
|
||||
|
||||
# Content-Length present: rejected before reading.
|
||||
request = create_rack_request(
|
||||
"POST",
|
||||
"/",
|
||||
{ "CONTENT_TYPE" => "application/json", "CONTENT_LENGTH" => oversized.bytesize.to_s },
|
||||
oversized,
|
||||
)
|
||||
response = transport.handle_request(request)
|
||||
|
||||
assert_equal 413, response[0]
|
||||
assert_equal JsonRpcHandler::ErrorCode::INVALID_REQUEST, JSON.parse(response[2][0]).dig("error", "code")
|
||||
end
|
||||
|
||||
test "rejects an oversized body even when Content-Length is absent" do
|
||||
transport = StreamableHTTPTransport.new(@server, max_request_bytes: 1024)
|
||||
oversized = { jsonrpc: "2.0", method: "initialize", id: "1", params: { x: "A" * 4096 } }.to_json
|
||||
|
||||
# No CONTENT_LENGTH: the bounded read must still catch it.
|
||||
request = create_rack_request(
|
||||
"POST",
|
||||
"/",
|
||||
{ "CONTENT_TYPE" => "application/json" },
|
||||
oversized,
|
||||
)
|
||||
response = transport.handle_request(request)
|
||||
|
||||
assert_equal 413, response[0]
|
||||
end
|
||||
|
||||
test "accepts a request body at the max_request_bytes boundary" do
|
||||
transport = StreamableHTTPTransport.new(@server, max_request_bytes: 4096)
|
||||
body = { jsonrpc: "2.0", method: "initialize", id: "1" }.to_json
|
||||
assert_operator body.bytesize, :<=, 4096
|
||||
|
||||
request = create_rack_request(
|
||||
"POST",
|
||||
"/",
|
||||
{ "CONTENT_TYPE" => "application/json" },
|
||||
body,
|
||||
)
|
||||
response = transport.handle_request(request)
|
||||
|
||||
assert_equal 200, response[0]
|
||||
end
|
||||
|
||||
test "rejects deeply nested JSON beyond the nesting cap as a parse error" do
|
||||
depth = StreamableHTTPTransport::MAX_JSON_NESTING + 5
|
||||
nested = ("[" * depth) + ("]" * depth)
|
||||
body = %({"jsonrpc":"2.0","method":"initialize","id":"1","params":{"deep":#{nested}}})
|
||||
|
||||
request = create_rack_request("POST", "/", { "CONTENT_TYPE" => "application/json" }, body)
|
||||
response = @transport.handle_request(request)
|
||||
|
||||
assert_equal 400, response[0]
|
||||
assert_equal JsonRpcHandler::ErrorCode::PARSE_ERROR, JSON.parse(response[2][0]).dig("error", "code")
|
||||
end
|
||||
|
||||
test "raises ArgumentError for an invalid max_request_bytes" do
|
||||
[nil, 0, -1, 1.5, "1024"].each do |invalid|
|
||||
assert_raises(ArgumentError) { StreamableHTTPTransport.new(@server, max_request_bytes: invalid) }
|
||||
end
|
||||
end
|
||||
|
||||
test "handle_request returns 403 for POST when Origin is not in allowed_origins" do
|
||||
transport = StreamableHTTPTransport.new(@server, allowed_origins: ["https://app.example.com"])
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue