[v1.x] fix(server): read HTTP request bodies with a size limit and bound JSON-RPC batch length - #2717
Conversation
…und JSON-RPC batch length Backport of #2698. WebStandardStreamableHTTPServerTransport (and StreamableHTTPServerTransport, which wraps it) reads the POST body with a 4 MiB limit by default and answers 413 past it, configurable with maxRequestBodySize; JSON-RPC batch arrays are capped at 100 messages. createMcpExpressApp runs its Host header validation before express.json(). Differences from #2698: 1.x has a single body read site (no createMcpHandler, toNodeHandler or hono adapter), so only the transport and the Express app change; the helper module is src/server/requestBody.ts.
|
commit: |
![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 examined whether readRequestBody bailing out on an over-limit body without cancelling the stream (it only does reader.releaseLock()) leaks memory — it does not: the unread remainder stays backpressured at the socket rather than buffered in the process, and closing the connection after the 413 is the HTTP server's responsibility, consistent with how express.json/raw-body behave.
Extended reasoning...
One candidate issue was investigated and ruled out this run: the mid-stream too-large path in src/server/requestBody.ts (line 55) returns without calling reader.cancel(), and the Content-Length fast path (line 37) never touches the body at all. Neither causes unbounded memory use — the limit's purpose is to bound what the process buffers, and unread stream data is subject to flow control, not accumulation — and discarding/tearing down the connection after an early 413 is left to the serving runtime, the same contract existing body-limiting middleware relies on. This is recorded as informational context only; the confirmed inline finding on the catch block in webStandardStreamableHttp.ts stands on its own.
| } catch { | ||
| this.onerror?.(new Error('Parse error: Invalid JSON')); | ||
| return this.createJsonErrorResponse(400, -32700, 'Parse error: Invalid JSON'); |
There was a problem hiding this comment.
🟣 Pre-existing (kept by this diff): the bare catch around the body read hard-codes 400/-32700 'Parse error: Invalid JSON' for all failures, including stream/read errors from readRequestBody, without discriminating the thrown cause — the repo review instructions (Recurring Catches: Error Handling) explicitly require flagging catch-alls that emit client-fault JSON-RPC codes for server/transport-internal failures.
Extended reasoning...
A client's connection drops (or the upstream stream errors) mid-upload of a POST body. readRequestBody propagates the stream failure, the catch at lines 757-759 swallows it, and the server answers 400 with JSON-RPC code -32700 'Parse error: Invalid JSON'. The client is told its request was malformed and may reformat/retry the same payload pointlessly, and onerror receives a fabricated parse error instead of the real network/stream error, hiding the actual cause from server operators.
Verification: pre-existing. The bare catch is real at HEAD src/server/webStandardStreamableHttp.ts:750-760: const body = await readRequestBody(req, this._maxRequestBodySize); ... rawMessage = JSON.parse(body.text); } catch { this.onerror?.(new Error('Parse error: Invalid JSON')); return this.createJsonErrorResponse(400, -32700, 'Parse error: Invalid JSON'); }. src/server/requestBody.ts's readRequestBody expli
2 participants
v1.x backport of #2698.
WebStandardStreamableHTTPServerTransport(andStreamableHTTPServerTransport, which wraps it) read the whole POST body into memory withreq.json()before doing anything with it, and accepted a JSON-RPC batch array of any length. It now reads with a 4 MiB limit by default and answers413past that, and caps batch arrays at 100 messages (400/-32600).Motivation and Context
Same as #2698: the other body reads on this line already have a ceiling (
createMcpExpressAppviaexpress.json(), the SSE transport viaraw-bodyat'4mb', stdio viamaxBufferSize); the web-standard transport was the exception.Content-Lengthover the limit is refused without reading; otherwise reading stops as soon as the limit is crossed, so chunked bodies are covered too. Unchanged when a pre-parsed body is passed asparsedBody(e.g.req.bodybehindexpress.json()), which remains the way to opt out of the SDK's read entirely.maxRequestBodySize(bytes, defaultDEFAULT_MAX_REQUEST_BODY_SIZE= 4 MiB) on the transport options; a non-positive or non-finite value throws aRangeErrorat construction.400before any element is parsed, on both the read path and a suppliedparsedBody.createMcpExpressAppnow installsexpress.json()after the Host header validation, so a request from a disallowed Host is answered403without its body being read.Differences from #2698
createMcpHandler,toNodeHandleror Hono adapter on this line, so only the transport andcreateMcpExpressAppchange.StreamableHTTPServerTransportOptionsis an alias of the web-standard options, so the Node wrapper takesmaxRequestBodySizewith no code change (one test pins it).src/server/requestBody.ts, identical to main's and importable as@modelcontextprotocol/sdk/server/requestBody.jsthrough the existing./*export;DEFAULT_MAX_REQUEST_BODY_SIZEandreadRequestBodyare the supported names.createMcpExpressAppon 1.x has no Origin validation orjsonLimit, so the moved line isexpress.json()after the Host header validation only.catch {}at the read site is kept, so a body read failure is still reported toonerroras "Parse error: Invalid JSON" on this line (main forwards the underlying error); the HTTP response is the same.How Has This Been Tested?
New tests mirroring #2698's transport tests (declared and streamed over-limit bodies →
413without the stream being pulled further;maxRequestBodySizeapplied on both read paths and validated at construction; 101-message batch →400with nothing dispatched, read path andparsedBody), one throughStreamableHTTPServerTransportovernode:httpto pin that the wrapper honours the option, and the Express ordering test. Each fails onv1.xbefore the change.npm run check,npm testpass.Breaking Changes
None to the API (one new optional transport option, one new module). Behavioural: POST bodies over 4 MiB get
413when the transport reads the body itself; batch arrays over 100 entries get400(including viaparsedBody);createMcpExpressAppanswers a disallowed Host with an invalid JSON body403rather than400.Types of changes
Checklist
AI Disclaimer