-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(node): export toWebRequest(), the IncomingMessage→Request conversion inside toNodeHandler #2390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
felixweinberger
wants to merge
6
commits into
main
Choose a base branch
from
fweinberger/to-web-request
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+233
−40
Open
feat(node): export toWebRequest(), the IncomingMessage→Request conversion inside toNodeHandler #2390
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
ecc4aca
feat(node): export toWebRequest(), the IncomingMessage→Request conver…
felixweinberger eb2d139
docs(server): lead isLegacyRequest's JSDoc with the single-argument form
felixweinberger 0d881cb
refactor(examples): route on isLegacyRequest via toWebRequest, not a …
felixweinberger 08980d6
docs(examples): correct what the legacy-routing comment claims for an…
felixweinberger dc9bc95
refactor(examples): pass req.body to toWebRequest plainly, matching i…
felixweinberger 48b9c90
docs(node): tighten the toWebRequest JSDoc to its contract
felixweinberger File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@modelcontextprotocol/server': patch | ||
| --- | ||
|
|
||
| `isLegacyRequest` docs: lead with the single-argument form. `isLegacyRequest(request)` is the whole API — the body is read from an internal clone, so the request you pass stays readable for whichever handler you route it to. `parsedBody` is an optional perf escape for a body you already hold parsed (and the way in for an already-consumed stream, e.g. behind `express.json()`), not a required companion. Documentation only; no behavior change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@modelcontextprotocol/node': minor | ||
| --- | ||
|
|
||
| Export `toWebRequest(req, parsedBody?, options?)` — the Node `IncomingMessage` → web-standard `Request` conversion `toNodeHandler` already performs internally. Use it to feed `isLegacyRequest()` (or `handler.fetch()`) from a hand-wired Node/Express `(req, res)` handler instead of assembling a `globalThis.Request` from `req.headers` by hand. When a body parser already consumed the Node stream (`express.json()`), pass the parsed value as `parsedBody`; pass `options.signal` to tie the constructed request to client disconnect, the way `toNodeHandler` does. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,158 @@ | ||
| /** | ||
| * `toWebRequest(req, parsedBody?, options?)` — the exported Node | ||
| * `IncomingMessage` → web-standard `Request` conversion. Covers the two body | ||
| * paths (the Node stream read vs. a supplied `parsedBody` re-serialized, with | ||
| * the entity headers rewritten and the stream untouched), Host-header URL | ||
| * derivation, header copying (multi-valued append, HTTP/2 pseudo-header | ||
| * skipping), the GET/HEAD no-body rule, the `signal` option, and the | ||
| * clone-readability contract `isLegacyRequest(request)` relies on. The full | ||
| * adapter exercises the same conversion end-to-end in `toNodeHandler.test.ts`. | ||
| */ | ||
| import { Readable } from 'node:stream'; | ||
|
|
||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| import type { NodeIncomingMessageLike } from '../src/toNodeHandler'; | ||
| import { toWebRequest } from '../src/toNodeHandler'; | ||
|
|
||
| function nodeRequest(init: { | ||
| method?: string; | ||
| url?: string; | ||
| headers?: Record<string, string | string[]>; | ||
| body?: string; | ||
| }): NodeIncomingMessageLike { | ||
| return Object.assign(Readable.from(init.body === undefined ? [] : [init.body]), { | ||
| method: init.method, | ||
| url: init.url, | ||
| headers: init.headers ?? {} | ||
| }); | ||
| } | ||
|
|
||
| /** A request whose Node stream rejects if anything iterates it. */ | ||
| function unreadableNodeRequest(init: { | ||
| method?: string; | ||
| url?: string; | ||
| headers?: Record<string, string | string[]>; | ||
| }): NodeIncomingMessageLike { | ||
| return { | ||
| method: init.method, | ||
| url: init.url, | ||
| headers: init.headers ?? {}, | ||
| [Symbol.asyncIterator](): AsyncIterator<unknown> { | ||
| return { next: () => Promise.reject(new Error('the Node stream must not be read when parsedBody is supplied')) }; | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| describe('toWebRequest', () => { | ||
| it('reads the Node stream as the body when no parsedBody is supplied', async () => { | ||
| const raw = JSON.stringify({ jsonrpc: '2.0', id: 2, method: 'ping' }); | ||
| const request = await toWebRequest( | ||
| nodeRequest({ | ||
| method: 'post', | ||
| url: '/mcp', | ||
| headers: { host: 'localhost:3000', 'content-type': 'application/json' }, | ||
| body: raw | ||
| }) | ||
| ); | ||
|
|
||
| expect(request.method).toBe('POST'); | ||
| expect(request.url).toBe('http://localhost:3000/mcp'); | ||
| expect(request.headers.get('content-type')).toBe('application/json'); | ||
| expect(await request.text()).toBe(raw); | ||
| }); | ||
|
|
||
| it('re-serializes a supplied parsedBody, rewrites the entity headers, and never touches the Node stream', async () => { | ||
| // A non-ASCII character keeps the byte length and the string length | ||
| // apart, so the rewritten content-length is provably the byte count. | ||
| const parsed = { jsonrpc: '2.0', id: 1, method: 'tools/call', params: { name: 'écho' } }; | ||
| const request = await toWebRequest( | ||
| unreadableNodeRequest({ | ||
| method: 'POST', | ||
| url: '/mcp', | ||
| headers: { | ||
| host: 'example.test:4321', | ||
| 'content-type': 'application/json', | ||
| 'content-length': '999', | ||
| 'content-encoding': 'gzip', | ||
| 'transfer-encoding': 'chunked', | ||
| accept: ['application/json', 'text/event-stream'] | ||
| } | ||
| }), | ||
| parsed | ||
| ); | ||
|
|
||
| expect(request.method).toBe('POST'); | ||
| expect(request.url).toBe('http://example.test:4321/mcp'); | ||
| expect(request.headers.get('content-type')).toBe('application/json'); | ||
| // Multi-valued Node headers are appended, not collapsed to the first value. | ||
| expect(request.headers.get('accept')).toBe('application/json, text/event-stream'); | ||
| // The entity headers described the original raw bytes; they are gone or rewritten. | ||
| expect(request.headers.get('content-encoding')).toBeNull(); | ||
| expect(request.headers.get('transfer-encoding')).toBeNull(); | ||
| const text = await request.text(); | ||
| expect(text).toBe(JSON.stringify(parsed)); | ||
| expect(request.headers.get('content-length')).toBe(String(text.length + 1)); | ||
| }); | ||
|
|
||
| it('produces a body-less Request when the supplied parsedBody is not JSON-serializable', async () => { | ||
| const request = await toWebRequest( | ||
| unreadableNodeRequest({ method: 'POST', url: '/mcp', headers: { host: 'localhost', 'content-length': '42' } }), | ||
| // JSON.stringify(() => {}) is undefined: there are no bytes to describe. | ||
| () => {} | ||
| ); | ||
| expect(request.body).toBeNull(); | ||
| expect(request.headers.get('content-length')).toBeNull(); | ||
| }); | ||
|
|
||
| it('derives the URL host from the Host header (falling back to localhost)', async () => { | ||
| const withHost = await toWebRequest(nodeRequest({ method: 'GET', url: '/a?b=1', headers: { host: 'api.example.test' } })); | ||
| expect(new URL(withHost.url).host).toBe('api.example.test'); | ||
| expect(new URL(withHost.url).pathname).toBe('/a'); | ||
| expect(new URL(withHost.url).search).toBe('?b=1'); | ||
|
|
||
| const withoutHost = await toWebRequest(nodeRequest({ method: 'GET', url: '/a' })); | ||
| expect(new URL(withoutHost.url).host).toBe('localhost'); | ||
| }); | ||
|
|
||
| it('skips HTTP/2 pseudo-headers, whose names Headers rejects', async () => { | ||
| const request = await toWebRequest( | ||
| nodeRequest({ | ||
| method: 'GET', | ||
| url: '/mcp', | ||
| headers: { host: 'h2.example.test', ':authority': 'h2.example.test', ':path': '/mcp', 'mcp-protocol-version': '2026-07-28' } | ||
| }) | ||
| ); | ||
| expect(new URL(request.url).host).toBe('h2.example.test'); | ||
| expect(request.headers.get('mcp-protocol-version')).toBe('2026-07-28'); | ||
| }); | ||
|
|
||
| it('produces a body-less Request for GET/HEAD even when parsedBody is supplied', async () => { | ||
| const request = await toWebRequest(nodeRequest({ method: 'GET', url: '/mcp', headers: { host: 'localhost' } }), { | ||
| ignored: true | ||
| }); | ||
| expect(request.method).toBe('GET'); | ||
| expect(request.body).toBeNull(); | ||
| }); | ||
|
|
||
| it('attaches options.signal to the constructed Request', async () => { | ||
| const controller = new AbortController(); | ||
| const request = await toWebRequest(nodeRequest({ method: 'GET', url: '/mcp', headers: { host: 'localhost' } }), undefined, { | ||
| signal: controller.signal | ||
| }); | ||
| expect(request.signal.aborted).toBe(false); | ||
| controller.abort(); | ||
| expect(request.signal.aborted).toBe(true); | ||
| }); | ||
|
|
||
| it('returns a Request whose body a clone-reader leaves readable (the isLegacyRequest contract)', async () => { | ||
| const raw = JSON.stringify({ jsonrpc: '2.0', id: 3, method: 'initialize', params: {} }); | ||
| const request = await toWebRequest( | ||
| nodeRequest({ method: 'POST', url: '/mcp', headers: { host: 'localhost', 'content-type': 'application/json' }, body: raw }) | ||
| ); | ||
| // `isLegacyRequest(request)` classifies a clone; the caller's request | ||
| // must stay readable for whichever handler it routes to. | ||
| expect(await request.clone().text()).toBe(raw); | ||
| expect(await request.text()).toBe(raw); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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.
toWebRequestbuilds the URL of the constructedRequestfromsingleHeaderValue(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 (anHttp2ServerRequestsatisfies the duck-typedNodeIncomingMessageLike). 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:authorityand omitHost, and Node's http2 compat layer does not synthesize ahostentry inrequest.headers— only the separaterequest.authoritygetter falls back across the two. One verifier confirmed this empirically against a localnode:http2server:req.headerscontains{':method', ':path', ':authority', ':scheme', ...}and nohost.\n\nConcrete walk-through. A client opens an HTTP/2 connection toapi.example.testand sendsPOST /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) readsreq.headers['host']→undefined→ falls back to'localhost', sourl = 'http://localhost/mcp'; (2) the header-copy loop skips every ':'-prefixed entry, so:authorityis dropped and the resultingRequesthas no Host header at all (the fetchRequestconstructor does not synthesize one from the URL); (3) the real authorityapi.example.testis gone from the converted request entirely.\n\nWhy nothing else catches it. The pseudo-header skip is correct on its own (Headersrejects ':'-prefixed names), but nothing re-injects the authority before the URL is built. The new unit test for this exact scenario supplies bothhost: '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-hostcase.\n\nImpact. Anything downstream that reads the converted request's URL or Host sees the wrong authority:ctx.requestInfohanded to the consumer's factory, logging, absolute-URL construction, and any user-side origin/host logic run against the convertedRequest. One impact claim from the original finding should be tempered rather than amplified: the SDK's ownhostHeaderValidationResponsereadsrequest.headers.get('host'), which isnullhere, 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 directnode:http2serving, not a security bypass.\n\nWhy this belongs on this PR even though the conversion body is unchanged. The same behavior existed in the privatenodeRequestToFetchRequest, so toNodeHandler users were already exposed; what this PR changes is the audience. It exports the function astoWebRequest, documents it in the README/JSDoc/changeset as the way to feedisLegacyRequest()andhandler.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 ownrequest.authoritygetter:\n\nts\nconst host = singleHeaderValue(req.headers['host']) ?? singleHeaderValue(req.headers[':authority']) ?? 'localhost';\n\n\nOptionally alsoheaders.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 theHeadersobject.