Skip to content

fix(mcp): bound HTTP request bodies and per-request collection lists - #1091

Open
rymalia wants to merge 1 commit into
tobi:mainfrom
rymalia:fix/http-body-limit
Open

rymalia wants to merge 1 commit into
tobi:mainfrom
rymalia:fix/http-body-limit

Conversation

@rymalia

@rymalia rymalia commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

collectBody() in src/mcp/server.ts buffers the entire request body before JSON.parse and schema validation, with no size limit:

for await (const chunk of req) chunks.push(chunk as Buffer);
return Buffer.concat(chunks).toString();

As a result, one request to qmd mcp --http can make the server hold an arbitrarily large body in memory, on every route that reads a body (/mcp, /query, /search and /metadata). The collections arrays on the MCP query and metadata tools and on the REST endpoints have no length limit either.

This is also where the memory advisories that npm audit reports for the pinned dependencies would be triggered: @modelcontextprotocol/server 2.0.0 (memory exhaustion during request validation) and zod 4.2.1 (memory exhaustion on oversized constrained collections). #1085 bumps the SDK, and zod is pinned on purpose (#382). Bounding the input in qmd itself limits the impact of both, whatever the dependency versions.

As far as I can tell, the SDK's other advisory, one HTTP client's messages delivered to another, doesn't apply: createMcpHandler(() => createMcpServer(store)) builds a fresh server and transport for each request.

Changes

  • 1 MiB body limit on every collectBody() call site.
    • Counted per chunk while streaming, so a chunked upload with no Content-Length is bounded too.
    • Content-Length early-out: a declared length over the limit is rejected before anything is read.
    • 413 response: {"error": "Request body too large"} on REST, and a JSON-RPC parse error on /mcp (-32700, id: null), the same code as for unparseable bodies.
  • The 413 reaches clients that are still uploading. Throwing out of the default async iterator destroys the request, so collectBody() now reads with req.iterator({ destroyOnReturn: false }) and drains the rest of the body after responding.
    • Old pattern: in a standalone probe, a client still uploading got EPIPE instead of the 413 in roughly 20–40% of 1–8 MB uploads, on Node 24.22.1 and 26.12.1.
    • Connection: close instead of draining: produced EPIPE/ECONNRESET at every size.
    • Drained form: delivered the 413 in every run: 200/200 on each Node version and 100/100 on Bun 1.4.4.
  • At most 1000 collections per request (one constant).
    • MCP: .max() on the query and metadata schemas, so over-limit lists get an input validation error.
    • REST: explicit checks on /query, /search and /metadata, which return a 400 {"error": "Invalid field: collections (maximum 1000 items)"}.
  • Behavior change: REST /query now returns a 400 for a non-array collections value, where it used to fall back to the default collections silently. This matches what /metadata already does.
  • Docs: the README states the 1 MiB limit and the collections limit, and the changelog has an entry under [Unreleased] → Fixed.

REST searches is deliberately left alone, because #1002 already validates it with subSearchSchema (min 1, max 10).

Tests

All in test/mcp.test.ts:

  • Fixed-length bodies: a body of exactly 1 MiB − 1 byte is accepted, and an over-limit body gets the 413 on /query, /metadata and /mcp, with the exact response bodies.
  • Chunked upload with no Content-Length: gets the 413 on /query and /mcp. This is the only test that reaches the per-chunk counting, since fetch() always sends Content-Length for a string body.
  • Over-limit collections: a 400 on REST /query and /metadata, and an input validation error on MCP query and metadata.

I checked that the tests fail without the change: removing the per-chunk check fails the chunked test, and removing the /metadata 413 handling fails the fixed-length test.

Known limits:

  • The EPIPE race isn't tested. It doesn't reproduce under vitest (the old code passed 50/50 there), so it rests on the probe results above.
  • Draining has no byte or time limit of its own. A client that keeps uploading holds the connection, but not memory, until Node's default request timeout. An explicit drain timeout could follow if you'd like one for servers reachable off-host.

Verification

collectBody() buffered the whole request before JSON.parse and schema
validation, so a single request could make the HTTP server hold an
arbitrarily large body in memory. The collections arrays on the MCP
query and metadata tools and on REST /query, /search and /metadata had
no length limit either.

Cap request bodies at 1 MiB on /mcp, /query, /search and /metadata.
The limit is counted per chunk while the body streams in, so a chunked
upload with no Content-Length is bounded too; an over-limit
Content-Length is rejected before reading. Oversized bodies get a 413:
plain JSON on REST, a JSON-RPC parse error (-32700) on /mcp.

Read with req.iterator({ destroyOnReturn: false }) and drain the rest
of an oversized body. Throwing out of the default iterator destroys the
request; in a standalone probe on Node 24 and 26 a client still
uploading then got EPIPE instead of the 413 in roughly a fifth to two
fifths of 1-8 MB uploads, while the drained form delivered the 413 in
every run on Node 24, Node 26 and Bun 1.4.4. The race does not
reproduce under vitest, so the chunked test guards the per-chunk limit,
not the race.

Allow at most 1000 collections per request. Over-limit lists get a 400
on REST and an input validation error on MCP, and a non-array REST
/query collections value now gets a 400 instead of silently falling
back to the default collections, matching /metadata.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant