feat(node): export toWebRequest(), the IncomingMessage→Request conversion inside toNodeHandler#2390
feat(node): export toWebRequest(), the IncomingMessage→Request conversion inside toNodeHandler#2390felixweinberger wants to merge 6 commits into
Conversation
…sion inside toNodeHandler
Routing on isLegacyRequest from a plain Node `(req, res)` handler required
hand-assembling a `globalThis.Request` from `req.headers` (header-array and
HTTP/2 pseudo-header pitfalls included), even though toNodeHandler already
contains the correct conversion as a private helper.
Export that helper as `toWebRequest(req, parsedBody?, options?)` from
`@modelcontextprotocol/node`. The function body is unchanged; toNodeHandler
now calls it with `{ signal }` in an options bag instead of a positional
AbortSignal, so the adapter's behavior is unchanged. Pass `parsedBody` when a
body parser already consumed the Node stream; `options.signal` ties the
constructed request to client disconnect the way toNodeHandler does.
Adds unit tests for both body paths (the parsedBody case asserts the Node
stream is never touched), header copying, the signal option, and the
clone-readability contract isLegacyRequest relies on. README + changeset.
isLegacyRequest(request) is the whole API: the body is read from an internal clone, so the caller's request stays readable for whichever handler it is routed to. That fact was the final paragraph of a long doc comment, after the routing semantics, which made the optional `parsedBody` read like a required companion. Move it into a lead paragraph: the single-argument form first, `parsedBody` as a perf escape hatch for a body the caller already holds parsed, and the one case it is genuinely needed (a Request whose own body was already read, where the internal clone would throw). Also point Node `(req, res)` callers at `toWebRequest(req)` — and `toWebRequest(req, req.body)` behind a body parser, since the Node stream is already drained there — from the companion `@modelcontextprotocol/node` change. Documentation only; no behavior change.
…hand-built globalThis.Request
The legacy-routing and elicitation servers each hand-read the Node body into
Buffer chunks, JSON.parsed it, and constructed a `globalThis.Request` from
`req.headers` (cast to `Record<string, string>`) just to call
isLegacyRequest — with a second argument the predicate does not need.
Use the exported `toWebRequest(req[, parsedBody])` from
`@modelcontextprotocol/node` instead, and call the predicate with just the
request. In legacy-routing (Express 5) the conversion is always handed a
parsed body (`req.body ?? {}` — body-parser leaves `req.body` undefined and
the stream unread for a non-JSON content type), so it never consumes a
stream the legacy transport still needs to read. In elicitation (plain
`node:http`) `toWebRequest` reads the stream once and both routing arms take
the body from the resulting `Request`; the predicate runs first, since it
classifies an internal clone and leaves the request readable.
No change to what either example demonstrates: for every input class the
same arm fires with the same body value as before.
🦋 Changeset detectedLatest commit: 48b9c90 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… unparsed body
A `{}` probe classifies as an invalid JSON-RPC body (a reject), not as
legacy, so a POST that `express.json()` does not parse routes to the strict
modern arm — the previous comment claimed the legacy routing was preserved.
The code is unchanged: `?? {}` is still the right call, because the
alternative (no `parsedBody`) makes `toWebRequest` read the Node stream out
from under the legacy transport, which still needs it. Say what the line
does instead of what it preserves.
…ts docs
Drop the `?? {}`. It existed to keep `toWebRequest` from reading the Node
stream for the one input where Express leaves `req.body` undefined AND the
stream unread — a POST whose content type `express.json()` will not parse.
That is not valid MCP traffic, the example's client never sends it, and the
old code already routed it differently across Express majors (an unparsed
body is `{}` on Express 4 but `undefined` on Express 5), so there was no
stable behavior to preserve and no justification for a special case in the
canonical example.
`toWebRequest(req, req.body)` is the form the function's own docs and the
changeset teach; the example now matches them, and for everything
`express.json()` parses it routes exactly as before.
Keep what it is, where it came from, a two-line usage, and the one sharp edge (no `parsedBody` -> the Node stream is read to completion, so read the body from the returned `Request`, not from `req`; with `parsedBody` nothing is read). Drop the URL/header/method mechanics inventory, the entity-header rewrite detail (an implementation detail nobody programs against), the full Express route duplicated from examples/legacy-routing, and the essay on the `signal` option whose useful content is one line.
| * | ||
| * With no `parsedBody` the Node stream is read to completion — read the body | ||
| * from the returned `Request` afterwards, not from `req`. When a body parser | ||
| * already consumed the stream (`express.json()`), pass the parsed value as | ||
| * `parsedBody` and nothing is read from `req`. | ||
| */ | ||
| export async function toWebRequest(req: NodeIncomingMessageLike, parsedBody?: unknown, options?: ToWebRequestOptions): Promise<Request> { | ||
| const method = (req.method ?? 'GET').toUpperCase(); | ||
| const host = singleHeaderValue(req.headers['host']) ?? 'localhost'; | ||
| const url = `http://${host}${req.url ?? '/'}`; |
There was a problem hiding this comment.
🟡 toWebRequest derives the constructed URL's host only from req.headers['host'] (falling back to 'localhost') and the header loop below skips ':'-prefixed pseudo-headers, so a Node http2-compat request — which carries its authority only as ':authority', with no 'host' entry — converts to 'http://localhost/' with no Host header. Since this PR promotes the conversion to a documented public API (and the new 'skips HTTP/2 pseudo-headers' test only passes because it supplies a synthetic 'host' alongside ':authority', a combination real HTTP/2 clients don't send), consider adding a one-line fallback to ':authority' — ?? singleHeaderValue(req.headers[':authority']) — mirroring Node's own request.authority. (Behavior is pre-existing in the private helper; not blocking.)
Extended reasoning...
What the bug is. toWebRequest builds the URL of the constructed Request from singleHeaderValue(req.headers['host']) ?? 'localhost' and then copies headers in a loop that explicitly skips any ':'-prefixed name (the HTTP/2 pseudo-headers :authority, :path, :method, :scheme). That skip — and the new test 'skips HTTP/2 pseudo-headers' — show that Node http2-compat requests are an intended input (an Http2ServerRequest satisfies the duck-typed NodeIncomingMessageLike). But over HTTP/2 the request authority normally arrives only as :authority: RFC 9113 §8.3.1 directs clients converting HTTP/1.1 requests to use :authority and omit Host, and Node's http2 compat layer does not synthesize a host entry in request.headers — only the separate request.authority getter falls back across the two. One verifier confirmed this empirically against a local node:http2 server: req.headers contains {':method', ':path', ':authority', ':scheme', ...} and no host.\n\nConcrete walk-through. A client opens an HTTP/2 connection to api.example.test and sends POST /mcp. Node's compat layer hands the handler a request whose headers are {':method': 'POST', ':path': '/mcp', ':authority': 'api.example.test', ':scheme': 'https', 'content-type': 'application/json'}. toWebRequest(req) then: (1) reads req.headers['host'] → undefined → falls back to 'localhost', so url = 'http://localhost/mcp'; (2) the header-copy loop skips every ':'-prefixed entry, so :authority is dropped and the resulting Request has no Host header at all (the fetch Request constructor does not synthesize one from the URL); (3) the real authority api.example.test is gone from the converted request entirely.\n\nWhy nothing else catches it. The pseudo-header skip is correct on its own (Headers rejects ':'-prefixed names), but nothing re-injects the authority before the URL is built. The new unit test for this exact scenario supplies both host: 'h2.example.test' and ':authority': 'h2.example.test', so it asserts the URL host is right while exercising a header combination genuine HTTP/2 clients do not send — it documents the pseudo-header skip but masks the missing-host case.\n\nImpact. Anything downstream that reads the converted request's URL or Host sees the wrong authority: ctx.requestInfo handed to the consumer's factory, logging, absolute-URL construction, and any user-side origin/host logic run against the converted Request. One impact claim from the original finding should be tempered rather than amplified: the SDK's own hostHeaderValidationResponse reads request.headers.get('host'), which is null here, so it fails closed with a 403 'missing Host header' — i.e. host validation is not bypassed, but HTTP/2 requests converted this way also cannot pass it. So the practical effect is wrong URL/host fidelity plus web-standard host validation becoming impossible for direct node:http2 serving, not a security bypass.\n\nWhy this belongs on this PR even though the conversion body is unchanged. The same behavior existed in the private nodeRequestToFetchRequest, so toNodeHandler users were already exposed; what this PR changes is the audience. It exports the function as toWebRequest, documents it in the README/JSDoc/changeset as the way to feed isLegacyRequest() and handler.fetch() from hand-wired handlers ('an Express req works'), and adds the HTTP/2-specific test. Fixing it now — while the API is being published — is much cheaper than after users start relying on it.\n\nFix. A one-liner that mirrors Node's own request.authority getter:\n\nts\nconst host = singleHeaderValue(req.headers['host']) ?? singleHeaderValue(req.headers[':authority']) ?? 'localhost';\n\n\nOptionally also headers.set('host', host) when it came from :authority, so the converted request carries a Host header like an HTTP/1.1 request would. And the 'skips HTTP/2 pseudo-headers' test would be more honest split in two: one case with only pseudo-headers (asserting the URL host comes from :authority), one asserting the pseudo-header names never reach the Headers object.
1 participant
Routing on
isLegacyRequestfrom a plain Node(req, res)handler currently means hand-building aglobalThis.Requestout ofreq.headers:That's a smell.
toNodeHandleralready contains the correct conversion (header arrays, HTTP/2pseudo-headers, the parsed-body re-serialization) as a private helper — users just can't reach it. It also
nudges people toward the two-argument
isLegacyRequest(probe, body), which reads as if the second argumentwere required. It isn't: the predicate clones internally, so
isLegacyRequest(request)is the whole API.Our own examples taught the same two-argument pattern, each behind ~16 lines of buffer-read +
JSON.parse+hand-built
Request.This exports the conversion as
toWebRequest()and moves both examples onto it. The Express one(
legacy-routing) becomes:and the plain
node:httpone (elicitation):How:
toWebRequest(req, parsedBody?, options?)is the existing private conversion, exported. The function bodyis unchanged, and
toNodeHandlernow calls it with the abort signal in the options bag instead of apositional argument, so the adapter's behavior is identical.
Promise<Request>(it reads the Node stream). PassparsedBodywhen a body parser alreadyconsumed the stream — it's re-serialized as the
Requestbody and the entity headers are rewritten. Passoptions.signalto tie the constructed request to client disconnect, the waytoNodeHandlerdoes.isLegacyRequest's docs now lead with the single-argument form and presentparsedBodyas the perfescape hatch it is, not a required companion.
Motivation and Context
isLegacyRequesttakes a web-standardRequest, but the Node hosting path only has anIncomingMessage.The conversion between the two already existed inside
toNodeHandler; exporting it removes the last reasonto build a
globalThis.Requestby hand — including in our own examples, which are what people copy.How Has This Been Tested?
New unit tests in
packages/middleware/node/test/toWebRequest.test.tscover the stream-read andparsedBodybody paths (the latter asserts the Node stream is never touched), the Host-derived URL, headercopying (multi-valued append, HTTP/2 pseudo-header skipping), the
signaloption, and the clone-readabilitycontract
isLegacyRequestrelies on. The existingtoNodeHandlersuite passes unchanged, which is theno-behavior-change check for the adapter path. The full example e2e suite passes (both rewritten stories
across all their transport × era legs), along with
typecheck:all, lint, and the docs build.Breaking Changes
None. New export;
toNodeHandlerbehavior is unchanged; theisLegacyRequestchange is documentation only;the examples route every input
express.json()parses identically.Types of changes
Checklist
Additional context
One edge worth naming: for a POST
express.json()doesn't parse (a non-JSON content type — not valid MCP),body-parser 2.x leaves
req.bodyundefined and the Node stream unread, sotoWebRequestreads it and thepredicate classifies the real bytes (the old hand-built, header-only probe never saw that body at all). Not
meaningful for spec traffic, and the old code already routed it differently between Express 4 and 5 (an
unparsed body is
{}on 4 butundefinedon 5, and the two classify differently).One deliberate nuance: the example's predicate now reads the body through the same
TextDecoderpath theentry itself uses, where the old hand-rolled read used
Buffer#toString('utf8'). The only observabledifference is a leading UTF-8 BOM, which the old read left in (so
JSON.parsefailed and the request fellto the legacy arm) and is now stripped, so the body classifies on its content.
The only other
Requestconstruction inexamples/isauthServer.ts'snew Request(new URL(req.originalUrl, issuer)), which is left alone on purpose: it synthesizes an OAuthdiscovery URL from
issuer, not a faithful conversion of the inbound request, sotoWebRequestwouldchange it.
@modelcontextprotocol/nodeREADME and changeset included.isLegacyRequest's rewritten docs are preciseabout the one case
parsedBodyis needed rather than just faster: aRequestwhose own body was alreadyread (the internal clone would throw). Behind a body parser the right fix is
toWebRequest(req, req.body)— the predicate still takes one argument.