diff --git a/.changeset/node-adapter-request-target-host-confusion.md b/.changeset/node-adapter-request-target-host-confusion.md new file mode 100644 index 00000000..e44f24a9 --- /dev/null +++ b/.changeset/node-adapter-request-target-host-confusion.md @@ -0,0 +1,5 @@ +--- +'mppx': patch +--- + +Fixed host confusion in the Node adapter (`Request.fromNodeListener`/`toNodeListener`). Protocol-relative (`//evil.com/x`), triple-slash (`///evil.com/x`), backslash (`/\evil.com/x`), and embedded-authority (`//a//evil.com/x`) request targets could previously override the request host derived from the `Host` header, which in turn poisoned the auto-detected challenge `realm`. The adapter now copies only the parsed path and query onto a trusted origin, so the request target's authority can never influence the resulting URL host. diff --git a/src/server/Mppx.test.ts b/src/server/Mppx.test.ts index e3030194..bf291048 100644 --- a/src/server/Mppx.test.ts +++ b/src/server/Mppx.test.ts @@ -3983,6 +3983,80 @@ describe('realm auto-detection', () => { } }) + // A raw request line can carry a protocol-relative or embedded-authority + // target that WHATWG URL would resolve into a foreign host, which would then + // be auto-detected as the challenge realm. The realm must stay bound to the + // Host header. + test.each([ + { name: 'protocol-relative', path: '//evil.com/resource' }, + { name: 'embedded authority', path: '//first.example//evil.com/resource' }, + ])('ignores $name request targets when deriving realm in node listeners', async ({ path }) => { + const handler = Mppx.create({ methods: [mockMethod], secretKey }) + const server = await Http.createServer(async (req, res) => { + const result = await Mppx.toNodeListener( + handler.charge({ + amount: '100', + currency: '0x0000000000000000000000000000000000000001', + recipient: '0x0000000000000000000000000000000000000002', + }), + )(req, res) + + if (result.status !== 402) res.end('OK') + }) + + try { + const rawResponse = await new Promise<{ + body: string + headers: http.IncomingHttpHeaders + statusCode: number + }>((resolve, reject) => { + const request = http.request( + { + host: '127.0.0.1', + port: server.port, + method: 'GET', + path, + headers: { Host: `localhost:${server.port}` }, + }, + (response) => { + const chunks: Buffer[] = [] + response.on('data', (chunk) => chunks.push(Buffer.from(chunk))) + response.on('end', () => { + resolve({ + body: Buffer.concat(chunks).toString('utf8'), + headers: response.headers, + statusCode: response.statusCode ?? 0, + }) + }) + }, + ) + + request.on('error', reject) + request.end() + }) + + const headers = new Headers() + for (const [name, value] of Object.entries(rawResponse.headers)) { + if (Array.isArray(value)) { + for (const item of value) headers.append(name, item) + } else if (value !== undefined) { + headers.append(name, value) + } + } + + const challenge = Challenge.fromResponse( + new Response(rawResponse.body, { + status: rawResponse.statusCode, + headers, + }), + ) + + expect(challenge.realm).toBe('localhost') + } finally { + server.close() + } + }) + test('credential verifies across different casing of same host', async () => { const handler = Mppx.create({ methods: [mockMethod], secretKey }) diff --git a/src/server/Request.test.ts b/src/server/Request.test.ts index b3e42cbb..60214896 100644 --- a/src/server/Request.test.ts +++ b/src/server/Request.test.ts @@ -1,5 +1,8 @@ import { EventEmitter } from 'node:events' +import { createServer } from 'node:http' import type { IncomingMessage, ServerResponse } from 'node:http' +import type { AddressInfo } from 'node:net' +import { connect } from 'node:net' import { Request } from 'mppx/server' import { describe, expect, test } from 'vp/test' @@ -73,6 +76,32 @@ describe('fromNodeListener', () => { expect(request.url).toBe('http://example.com/api/resource?q=1') }) + // The request target's authority must never override the trusted host + // (options.host > Host > :authority > localhost). A raw request line can + // carry protocol-relative, triple-slash, backslash, or embedded-authority + // targets that WHATWG URL would otherwise resolve into a foreign host. + test.each([ + ['protocol-relative', '//evil.com/path?q=1', 'http://example.com/path?q=1'], + ['triple-slash', '///evil.com/x', 'http://example.com/x'], + ['backslash', '/\\evil.com/x', 'http://example.com/x'], + ['userinfo authority', '//user:pass@evil.com/path', 'http://example.com/path'], + ['ipv6 authority', '//[2001:db8::1]:8443/path', 'http://example.com/path'], + ['embedded authority', '//first.example//evil.com/path', 'http://example.com//evil.com/path'], + [ + 'absolute embedded authority', + 'http://first.example//evil.com/p', + 'http://example.com//evil.com/p', + ], + ['encoded slashes stay path', '/%2F%2Fevil.com/p', 'http://example.com/%2F%2Fevil.com/p'], + ])('binds host to Host header for %s request targets', (_name, url, expected) => { + const [req, res] = createMockRequest({ url, rawHeaders: ['Host', 'example.com'] }) + + const request = Request.fromNodeListener(req, res) + + expect(request.url).toBe(expected) + expect(new URL(request.url).host).toBe('example.com') + }) + test('uses explicit protocol and host overrides', () => { const [req, res] = createMockRequest({ url: '/api/resource', @@ -172,3 +201,55 @@ describe('fromNodeListener', () => { expect(await request.text()).toBe('hello') }) }) + +// Conformance harness: a normal HTTP client cannot emit these request targets, +// so they are driven through Node's real HTTP parser over a raw socket to prove +// the adapter neutralizes host confusion end-to-end (not just via mocks). +describe('toNodeListener (raw request target)', () => { + async function captureRequestUrl(rawRequestLine: string): Promise { + let resolveUrl!: (url: string) => void + const observed = new Promise((resolve) => { + resolveUrl = resolve + }) + + const server = createServer( + Request.toNodeListener(async (request) => { + resolveUrl(request.url) + return new Response('ok') + }), + ) + + try { + await new Promise((resolve) => server.listen(0, '127.0.0.1', () => resolve())) + const { port } = server.address() as AddressInfo + const socket = connect(port, '127.0.0.1') + await new Promise((resolve, reject) => { + socket.once('connect', () => resolve()) + socket.once('error', reject) + }) + socket.write(rawRequestLine) + const url = await observed + socket.destroy() + return url + } finally { + await new Promise((resolve) => server.close(() => resolve())) + } + } + + test('binds host to Host header for a protocol-relative raw request target', async () => { + const url = await captureRequestUrl( + 'GET //evil.com/protected?q=1 HTTP/1.1\r\nHost: example.com\r\nConnection: close\r\n\r\n', + ) + + expect(url).toBe('http://example.com/protected?q=1') + expect(new URL(url).host).toBe('example.com') + }) + + test('binds host to Host header for an embedded-authority raw request target', async () => { + const url = await captureRequestUrl( + 'GET //first.example//evil.com/protected HTTP/1.1\r\nHost: example.com\r\nConnection: close\r\n\r\n', + ) + + expect(new URL(url).host).toBe('example.com') + }) +}) diff --git a/src/server/Request.ts b/src/server/Request.ts index 8a7ea4e1..49a001ff 100644 --- a/src/server/Request.ts +++ b/src/server/Request.ts @@ -86,7 +86,7 @@ export function fromNodeListener( headers.get('Host') ?? (req.headers as Record)[':authority'] ?? 'localhost' - const url = new URL(normalizeRequestTarget(req.url), `${protocol}//${host}`) + const url = createRequestUrl(req.url, `${protocol}//${host}`) const init: RequestInit & { duplex?: string } = { method, @@ -116,17 +116,31 @@ function hasBody(headers: Headers): boolean { return (contentLength !== null && contentLength !== '0') || headers.has('transfer-encoding') } -function normalizeRequestTarget(url: string | undefined): string { - if (!url) return '/' +/** + * Builds the request `URL` from a request target and a trusted origin. + * + * Only the parsed `pathname`/`search` are copied onto the trusted origin, so + * the target's authority can never override the host (protocol-relative, + * `///`, backslash, absolute-form, or embedded-authority targets). Components + * are copied onto a `URL` object rather than concatenated and re-parsed, since + * a normalized path can itself begin with `//` and be read as an authority. + */ +function createRequestUrl(target: string | undefined, origin: string): URL { + const url = new URL(origin) + if (!target) return url + let parsed: URL try { - const absoluteUrl = new URL(url) - // Absolute-form request targets can carry a different origin than the socket host. - // Keep only path+query so realm detection stays bound to Host/:authority. - if (absoluteUrl.protocol === 'http:' || absoluteUrl.protocol === 'https:') - return `${absoluteUrl.pathname}${absoluteUrl.search}` - } catch {} + parsed = new URL(target, 'http://mppx.invalid') + } catch { + throw new TypeError('Invalid request target') + } + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') + throw new TypeError('Unsupported request target protocol') + url.pathname = parsed.pathname + url.search = parsed.search + url.hash = '' return url }