Add Streamable HTTP request body limits - #3095
Conversation
Sorry, something went wrong.
📚 Documentation preview
|
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Both findings from my previous review are verified fixed in the current revision: handle_request now delegates through asgi_app, and the middleware coalesces chunks into a single bytearray replayed as one message. Beyond the inline pre-existing SSE note, this run also examined and ruled out: buffering up to the limit before the transport's 421/403/404 fail-fast checks (bounded at 4 MiB, so not an amplification); zero-length more_body=True chunks spinning the read loop (memory stays bounded by the coalesced buffer; connection lifetime is the server's slow-request concern); and body buffering happening before DNS-rebinding validation (same bounded-cost reasoning).
Extended reasoning...This run produced one inline finding, explicitly pre-existing and non-blocking (the legacy SSE POST endpoint still buffers bodies unbounded — untouched by this PR, and the PR's docs correctly scope the new limit to Streamable HTTP). The previous run's two blocking findings were both fixed by commit 54b0cc4, which I verified against the current diff. The remaining candidate issues raised by finders this run were all refuted on bounded-cost grounds: the middleware never retains more than max_request_body_size bytes plus a small constant regardless of chunking, ordering relative to the transport's security checks only shifts a bounded buffer, and the zero-length-chunk loop cannot grow memory. Since real (fixed-now) issues were found on earlier revisions and this adds new security-relevant middleware to every Streamable HTTP request path, a human should still make the final call; this note just records what has already been examined.
Sorry, something went wrong.
| class RequestBodyLimitMiddleware: | ||
| """Reject oversized HTTP request bodies before invoking an ASGI application.""" |
There was a problem hiding this comment.
🟣 Pre-existing issue, not introduced by this PR: the legacy SSE transport's POST endpoint (SseServerTransport.handle_post_message, src/mcp/server/sse.py:243) still buffers the entire request body unbounded via await request.body() — the same DoS vector this PR closes for Streamable HTTP. Since the new RequestBodyLimitMiddleware is a generic ASGI wrapper, applying it to the SSE message endpoint would be a natural one-line follow-up (or the docs could note explicitly that the limit covers Streamable HTTP only).
Extended reasoning...What the issue is
This PR adds max_request_body_size enforcement (default 4 MiB, HTTP 413) for the Streamable HTTP transport via the new RequestBodyLimitMiddleware. The legacy SSE transport has the exact same unbounded-buffering exposure, and it remains open: SseServerTransport.handle_post_message (src/mcp/server/sse.py:205) reads the full POST body with body = await request.body() (src/mcp/server/sse.py:243) before any JSON validation, with no size check of any kind.
The code path
The SSE transport is still shipped and mountable — mcp.run(transport="sse") and sse_app() both wire sse.handle_post_message onto the message endpoint (see the Mount(message_path, app=sse.handle_post_message) routes in src/mcp/server/mcpserver/server.py). A request to that endpoint passes only header-level checks (transport security, session_id lookup) before line 243 buffers the entire body into memory. Starlette's Request.body() accumulates every chunk until the stream ends, so nothing bounds it.
Step-by-step proof
- Start any server with mcp.run(transport="sse") (or mount sse_app()).
- Open a legitimate SSE connection via GET /sse to obtain a valid session_id (or skip this — the body is read even for requests that will fail JSON validation, as long as the session check passes; with a valid session it definitely proceeds to line 243).
- POST /messages/?session_id=<id> with an arbitrarily large body (e.g. a multi-GB chunked upload).
- await request.body() at line 243 buffers the whole thing in server memory before types.JSONRPCMessage.model_validate_json(body) ever runs. Concurrent requests multiply the footprint. This is precisely the memory-exhaustion vector the PR's own migration-guide entry describes closing for Streamable HTTP.
Why this is pre-existing and non-blocking
The PR does not touch sse.py, and both documentation additions correctly scope the new limit: docs/migration.md says "Streamable HTTP request bodies are limited to 4 MiB" and the docs/run/index.md bullet sits under the Streamable HTTP section. So nothing in this PR is wrong or misleading — the gap predates it and is untouched by it. (For completeness: both earlier claude[bot] findings on this PR — handle_request bypassing the limit, and per-message dict memory amplification — are already fixed in the current revision: handle_request now delegates to asgi_app, and chunks coalesce into a single bytearray replayed as one message.)
How to fix (follow-up)
RequestBodyLimitMiddleware is deliberately transport-agnostic — it wraps any ASGI app and only inspects POST requests. Wrapping the SSE message endpoint, e.g. Mount(message_path, app=RequestBodyLimitMiddleware(sse.handle_post_message, max_request_body_size)) in sse_app(), would close the gap with essentially one line. Alternatively (or additionally), a one-line doc note stating that the 4 MiB limit applies to Streamable HTTP only would make the scope explicit for operators still serving SSE clients. Given SSE is legacy/discouraged (docs/run/index.md answers "When" with "You don't"), a follow-up issue is the right vehicle — this should not block the PR.
Sorry, something went wrong.
Summary
Validation