Apply the request body limit to the SSE and OAuth endpoints - #3336
Conversation
SseServerTransport now takes max_request_body_size (default 4 MiB, the same default and validation as StreamableHTTPSessionManager) and answers 413 before session lookup or parsing when a POST declares or streams a larger body. The message endpoint only ever handled POST bodies, so it now answers 405 (Allow: POST) to other methods instead of treating them like a POST. MCPServer.sse_app(), run_sse_async() and run(transport="sse") expose the keyword, mirroring streamable_http_app().
create_auth_routes now wraps its endpoints in RequestBodyLimitMiddleware, so /token, /revoke, /register and POST /authorize answer 413 to bodies over the 4 MiB default before any form or JSON parsing. The limit sits inside the CORS wrapper so browser clients still get CORS headers on the 413; GET and OPTIONS requests pass through untouched.
📚 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 finding, I also looked at whether the new SSE RequestBodyLimitMiddleware buffering the request body before the DNS-rebinding/session-ownership checks creates a new resource risk — the buffer is capped at max_request_body_size (4 MiB default) per request, so memory is bounded; it does mean up to that much body is read before the security checks that previously rejected without reading any body bytes, which is a behavior change worth a human glance rather than a bug.
Extended reasoning...
Findings were reported, so this is the brief ruled-out note only. The SSE path in src/mcp/server/sse.py now routes POSTs through RequestBodyLimitMiddleware before _handle_post_message runs its transport-security and session checks; I read the middleware in src/mcp/server/streamable_http_manager.py and confirmed it rejects once the accumulated body exceeds max_body_size, so buffering is bounded and not a memory-exhaustion vector — only an ordering change (body read before header-based rejections). The hunt exited on max_rounds and touches auth/security paths, so approval is off the table regardless; the inline comment already signals human review is needed.
RequestBodyLimitMiddleware only inspected POST requests, but some of the routes it wraps accept other methods whose handlers read the body as well (OPTIONS on the token, registration and revocation endpoints, HEAD on the authorization endpoint). Enforce the limit for every HTTP request. Also take the limit back out of cors_middleware, which returns to plain CORS wrapping, and compose the CORS and body-limit wrappers explicitly where the OAuth routes are declared.
There was a problem hiding this comment.
I re-reviewed the latest push and found no bugs — the method bypass flagged in my earlier review is resolved: RequestBodyLimitMiddleware now applies to every HTTP method, and the new parametrized tests exercise OPTIONS/HEAD against the OAuth endpoints directly. Since this changes request handling on the OAuth authorization-server routes, a human look is still worthwhile.
What was reviewed:
- The method-agnostic limit in
RequestBodyLimitMiddleware— declared Content-Length and streamed-chunk counting both still 413, and non-HTTP scopes pass straight through. - CORS layering on
/token,/register,/revoke— the 413 is produced inside the CORS wrapper so browsers can read it; preflights are still answered by the CORS layer. - Checked that the OAuth endpoints hard-coding the 4 MiB default (rather than taking the operator's
max_request_body_size) is a scoping choice, not a wiring bug — the configurable knob is documented for the transports only. - The SSE message endpoint's new 405-for-non-POST guard and the
max_request_body_sizeplumbing throughsse_app()/run()— defaults and validation match the streamable-HTTP manager.
Extended reasoning...
Overview
The PR extends the existing streamable-HTTP request-body limit to the two other body-reading HTTP surfaces: the SSE message endpoint (src/mcp/server/sse.py, plumbed through sse_app()/run(transport="sse") in src/mcp/server/mcpserver/server.py) and the OAuth authorization-server routes (src/mcp/server/auth/routes.py). The final commit also changes RequestBodyLimitMiddleware in src/mcp/server/streamable_http_manager.py to apply to all HTTP methods instead of POST only, which was the fix for the bypass I flagged on the previous push: /token, /register, and /revoke accept OPTIONS (and /authorize accepts GET/HEAD via Starlette), and their handlers read the body unconditionally, so a non-POST request with a huge body previously skipped the limit entirely.
Security risks
This is security-hardening code on auth-adjacent paths, so the main risks are (a) the limit being bypassable and (b) the wrapping breaking legitimate OAuth flows. On (a), the middleware now checks every HTTP request — declared Content-Length first, then actual streamed bytes — and the tests cover GET/PUT/OPTIONS/HEAD/DELETE at the middleware level plus OPTIONS/HEAD end-to-end against the OAuth routes. On (b), CORS is deliberately kept as the outermost wrapper so a 413 still carries access-control-allow-origin, and there are tests for preflight handling, a small body reaching the form parser intact, and the cross-origin 413. One candidate issue was investigated and ruled out this run: the OAuth endpoints use the hard-coded DEFAULT_MAX_REQUEST_BODY_SIZE rather than the operator-configured max_request_body_size — that is a deliberate scoping choice (OAuth request bodies have no reason to approach 4 MiB, and the docs commit explicitly scopes the configurable knob to the transports).
Level of scrutiny
High. The change touches src/mcp/server/auth/routes.py and reworks how every OAuth endpoint is composed (CORS over body-limit over handler), and it changes middleware behavior for all HTTP methods including the standalone GET SSE stream on streamable HTTP. I checked that the eager receive() in the middleware is safe for bodyless GETs (ASGI servers deliver an empty http.request, and any trailing message such as http.disconnect is replayed), and that the SSE endpoint's new 405 guard does not change behavior for non-HTTP scopes relative to the old Request(scope, ...) assertion. Because this is security-sensitive auth-path code, approval is not appropriate even with zero findings — hence defer.
Other factors
The bug-hunt exited on dry_streak with no findings, and the test additions are thorough: 8 parametrized method/path cases for the OAuth 413, chunked-body and default-limit cases for SSE, the 405 guard with its Allow header, ValueError on non-positive limits, and the CORS-layering assertions. Docs (docs/run/index.md, docs/migration.md) were updated consistently with the behavior change. My prior red finding is the one substantive thing that changed since my last review, and confirming its resolution is the new information that justifies posting rather than staying silent.
…dule The middleware and DEFAULT_MAX_REQUEST_BODY_SIZE are now used by the SSE transport and the OAuth routes as well, so they move next to the other shared HTTP request checks in mcp.server.transport_security. Both names remain importable from mcp.server.streamable_http_manager. The middleware's own unit tests move with it; no behaviour change.
1 participant
#3095 added
RequestBodyLimitMiddlewareand applied it to the Streamable HTTP endpoint. This does the same for the other two places that accept request bodies, so every HTTP entry point shares the one 4 MiB default.Motivation and Context
SseServerTransporttakesmax_request_body_size(default 4 MiB, same validation asStreamableHTTPSessionManager), andMCPServer.sse_app()/run(transport="sse")pass it through, mirroringstreamable_http_app(). The message endpoint now answers 405 to anything that isn't a POST instead of treating it as one.create_auth_routesendpoints (/token,/revoke,/register, POST/authorize) use the default limit. On the CORS-enabled routes it sits inside the CORS wrapper, so a 413 still carries CORS headers and preflights are untouched. These endpoints use the 4 MiB default rather than a configurable value; their payloads are a few kilobytes.OPTIONS/HEADand their handlers read the body either way.RequestBodyLimitMiddlewareandDEFAULT_MAX_REQUEST_BODY_SIZEmove tomcp.server.transport_security, next to the other shared HTTP request checks, now that three modules use them. Both remain importable frommcp.server.streamable_http_manager.Nothing changes for requests under the limit.
How Has This Been Tested?
New tests in
tests/server/test_sse_security.py,tests/server/auth/test_error_handling.py,tests/server/test_transport_security.pyandtests/server/mcpserver/test_server.py: over-limit bodies (declared and streamed, across methods) get 413, bodies under the limit still reach session lookup / form parsing, CORS preflights are still answered and a 413 on a CORS route keeps its CORS headers, non-POST to the message endpoint gets 405, andsse_app()applies the configured value. Full suite, pyright and ruff pass locally.Breaking Changes
None. The new keyword is optional and defaults to the limit
streamable_http_app()already uses; the observable differences are a 413 for request bodies over 4 MiB on these endpoints and a 405 for non-POST requests to the SSE message endpoint.Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
docs/run/index.mddescribesmax_request_body_sizeas the largest accepted request body; no new documentation for the SSE transport.AI Disclaimer