Add Streamable HTTP request body limits#3095
Conversation
📚 Documentation preview
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, two other candidates were examined and ruled out this run: (1) the middleware buffering the full POST body before the Origin/Host/session checks in handle_request run — the buffer is bounded by max_request_body_size, so it doesn't add unbounded exposure ahead of those checks; (2) the plain-text 413 body being inconsistent with the manager's JSON-RPC error responses — a formatting nit, not a correctness issue.
Extended reasoning...
Two bugs were confirmed and posted as inline comments (the public handle_request bypass of the limit, and per-message dict caching allowing memory amplification via tiny chunks), so this PR needs human review and I am not approving. This note only records the additional candidates that finder agents raised and verifiers refuted, so a later pass or the author doesn't re-explore them from scratch. It is informational, not a guarantee of correctness elsewhere in the diff.
| self.security_settings = security_settings | ||
| self.retry_interval = retry_interval | ||
| self.session_idle_timeout = session_idle_timeout | ||
| self.max_request_body_size = max_request_body_size | ||
| self.asgi_app = RequestBodyLimitMiddleware(self.handle_request, max_request_body_size) | ||
|
|
||
| # Session tracking (only used if not stateless) | ||
| self._session_creation_lock = anyio.Lock() |
There was a problem hiding this comment.
🔴 The new max_request_body_size limit is enforced only through the new manager.asgi_app wrapper — StreamableHTTPSessionManager.handle_request remains a public, documented ASGI entry point with no size check, so existing code that mounts handle_request directly (as the SDK's own tests and, until this PR, examples/stories/legacy_routing/README.md do) silently gets no limit. That contradicts docs/migration.md's unconditional claim that "V2 applies a 4 MiB default limit" and the class docstring's promise of a 413 "before parsing or session creation". Either enforce the limit inside handle_request itself, or explicitly document that direct handle_request mounts bypass the limit and must switch to asgi_app.
Extended reasoning...
The bug. The body limit lives in RequestBodyLimitMiddleware, which is instantiated once in StreamableHTTPSessionManager.__init__ as self.asgi_app = RequestBodyLimitMiddleware(self.handle_request, max_request_body_size) (src/mcp/server/streamable_http_manager.py:103). Only callers that go through asgi_app get the check. handle_request itself — still a public method with a docstring ("Process ASGI request with proper session handling and transport setup") — dispatches straight to handle_modern_request / _handle_stateless_request / _handle_stateful_request with no size enforcement anywhere on that path.
The code path that triggers it. Any integration that mounts the manager's method directly, e.g. Mount("/mcp", app=session_manager.handle_request), receives a POST whose body is read without bound. This is not a hypothetical pattern: the SDK's own tests use exactly it (tests/shared/test_streamable_http.py:354, tests/server/test_streamable_http_security.py:29, tests/client/test_http_unicode.py:111), examples/stories/stateless_legacy/README.md:35 describes it, and this very PR had to change examples/stories/legacy_routing/README.md from modern_manager.handle_request to modern_manager.asgi_app to pick up the limit — demonstrating the trap against the SDK's own examples.
Why existing code doesn't prevent it. Nothing downstream of handle_request re-checks size: _handle_stateful_request constructs a Request(scope, receive) and the transport eventually reads the full body; the modern path likewise parses whatever arrives. The middleware is purely a wrapper, and handle_request never routes through it.
The impact. A user upgrading to v2 with an existing direct handle_request mount (previously the correct integration) reads docs/migration.md — "V2 applies a 4 MiB default limit to Streamable HTTP POST bodies and returns HTTP 413 before parsing the JSON or creating a session" — and the max_request_body_size docstring on the class itself — oversized requests "receive a 413 response before parsing or session creation" — and reasonably concludes they are protected. They are not: their server still buffers arbitrarily large POST bodies. A silently absent security control backed by an unconditional documented guarantee is worse than no control, because it discourages the user from adding their own limit at the reverse proxy.
Step-by-step proof.
manager = StreamableHTTPSessionManager(app=server, max_request_body_size=8); mountMount("/mcp", app=manager.handle_request)(the pattern in tests/shared/test_streamable_http.py:354).- Client sends
POST /mcpwith a 9-byte (or 9-GB) body. - Starlette routes the scope to
handle_request, which checksself._task_group, reads the protocol-version header, and dispatches to_handle_stateful_request. _handle_stateful_requestbuildsRequest(scope, receive), creates a transport/session, andhttp_transport.handle_requestreads the entire body. No 413 is ever produced; contrast with the new testtest_oversized_content_length_is_rejected_before_body_read_or_session_creation, which passes only because it callsmanager.asgi_appinstead.
How to fix. Simplest: enforce the limit inside handle_request itself (e.g. have handle_request delegate through the middleware, with the unlimited internals renamed to a private _handle_request) — then asgi_app and handle_request behave identically and every mount is protected. Alternatively, if direct mounts are meant to be unprotected, the max_request_body_size docstring and the migration-guide section must say explicitly that the limit applies only via asgi_app / streamable_http_app() and that direct handle_request mounts must switch to asgi_app. All four verifiers confirmed this finding (none refuted); mitigation noted by two of them — every documented v2 mounting path (streamable_http_app → StreamableHTTPASGIApp → asgi_app) is protected — but the migration guide's claim is addressed precisely at users with pre-existing v1-style integrations, which are the ones that bypass it.
| cached_messages: deque[Message] = deque() | ||
| received_size = 0 | ||
| while True: | ||
| message = await receive() | ||
| if message["type"] != "http.request": | ||
| cached_messages.append(message) | ||
| break | ||
|
|
||
| received_size += len(message.get("body", b"")) | ||
| if received_size > self.max_body_size: | ||
| response = Response("Request body too large", status_code=413) | ||
| return await response(scope, receive, send) | ||
| cached_messages.append(message) | ||
| if not message.get("more_body", False): | ||
| break |
There was a problem hiding this comment.
🔴 RequestBodyLimitMiddleware caps only the sum of body bytes but caches one ASGI message dict (~220 bytes of object overhead) per http.request message, so a client trickling a chunked POST in 1-byte segments can force ~4M cached dicts (~1 GB resident) per request before the default 4 MiB limit trips — a ~200x amplification of exactly the memory bound this PR adds. Coalescing the cached body chunks into a single bytearray and replaying one synthesized message would bound memory at max_body_size plus a small constant.
Extended reasoning...
What the bug is
RequestBodyLimitMiddleware.__call__ enforces the limit by summing body bytes (received_size += len(message.get("body", b""))) while appending every ASGI message dict to cached_messages (src/mcp/server/streamable_http_manager.py:395-409). The limit therefore bounds the payload bytes, but the memory actually held is dominated by per-message Python object overhead: each cached message is a dict (~184 bytes) plus a small bytes object (~34 bytes for a 1-byte body) plus a deque slot — roughly 220+ bytes per message. Nothing bounds the number of cached messages except the byte count, so the smaller the chunks, the worse the amplification.
The code path that triggers it
An attacker sends POST /mcp with Transfer-Encoding: chunked (no Content-Length, so the header fast-path at lines 381-391 is skipped) and trickles the body as 1-byte chunks, paced one per TCP segment. Uvicorn (h11 or httptools) emits one http.request message per socket read, so each body byte arrives as its own ASGI message with more_body=True. Each loop iteration increments received_size by 1 and appends one dict. The 413 only fires once received_size > 4 * 1024 * 1024.
Step-by-step proof
- Client opens
POST /mcpwithTransfer-Encoding: chunkedand sends1\r\nx\r\n(one 1-byte chunk), then pauses briefly, then repeats. - Uvicorn performs one socket read per segment and emits
{"type": "http.request", "body": b"x", "more_body": True}for each. - Middleware loop, per message:
received_size += 1;cached_messages.append(message)— ~220 bytes retained per 1 payload byte. - After 4,194,304 chunks,
received_sizefinally exceeds the limit and the 413 is sent — but at that instantcached_messagesholds ~4.2M dicts ≈ 0.9-1 GB resident, all alive simultaneously. - Wire cost to the attacker is ~28 MB (6 bytes of chunked framing per payload byte); concurrent requests multiply the server-side footprint. The verifiers measured the per-message costs empirically (dict 184 B, 1-byte
bytes34 B) and confirmed the ~200-250x figure.
Why nothing else prevents it
The middleware eagerly buffers the whole body before invoking the wrapped app, so no downstream code runs while the deque grows. Notably, the downstream readers it guards are more frugal: Starlette's Request.stream() skips empty chunks (if body: yield body) and body() accumulates bare bytes objects (~34 B overhead each, ~6x cheaper than caching full message dicts). So the new control's own buffering is the most expensive accumulation on the path.
A secondary observation from the original report — zero-length http.request messages with more_body=True never increment received_size, leaving the loop bounded only by the attacker continuing to send — is real in the code but hard to reach in practice: in HTTP/1.1 chunked encoding a zero-size chunk is the body terminator, h11 never emits empty Data events, and h2 carries empty-DATA-flood mitigations (CVE-2019-9518). The ASGI spec does permit such messages, though, so a security control shouldn't rely on server-specific coalescing behavior; the fix below neutralizes this case for free.
Impact and fix
This PR's stated purpose is to bound Streamable HTTP request-body memory and return 413 as DoS protection, and the docs tell operators the default limit is 4 MiB. As written, an adversary can still drive ~1 GB of transient allocations per request while staying under that documented bound — undermining the guarantee the feature advertises. The fix is small and local: accumulate body bytes into a single bytearray (bounded by max_body_size plus a constant), and have replay() hand the app one synthesized http.request message (plus the terminating non-body message, if any). That caps memory at the configured limit regardless of chunking, and incidentally bounds the empty-chunk case as well since empty chunks then add nothing to the buffer.
2 participants
Summary
max_request_body_sizeoption for Streamable HTTP serversValidation
./scripts/testuv run --frozen pre-commit run --all-files