From 65531d08a4c3557dada5524e88316ec0ab950325 Mon Sep 17 00:00:00 2001 From: Cedric Karungu Date: Tue, 6 Oct 2026 17:03:24 +0200 Subject: [PATCH] fix(interceptors): answer a malformed egress request instead of dying on it --- .../survive-a-malformed-egress-request.md | 10 ++ packages/interceptors/src/egress-server.ts | 69 +++++++-- .../interceptors/test/egress-server.test.ts | 141 ++++++++++++++++++ 3 files changed, 209 insertions(+), 11 deletions(-) create mode 100644 .changeset/survive-a-malformed-egress-request.md diff --git a/.changeset/survive-a-malformed-egress-request.md b/.changeset/survive-a-malformed-egress-request.md new file mode 100644 index 00000000..365ebbfc --- /dev/null +++ b/.changeset/survive-a-malformed-egress-request.md @@ -0,0 +1,10 @@ +--- +'@memnox/core': patch +'@memnox/proxy': patch +'@memnox/interceptors': patch +'memnox': patch +--- + +One malformed request no longer ends the egress proxy. Both handlers ran with no `catch` and nothing registers an `unhandledRejection`, so a rejection took the process down — and that process can be the daemon. A mangled `Proxy-Authorization` threw `URIError` and an `https://` absolute-form request threw `TypeError: Protocol "https:" not supported`, either of which any local process could send. + +Each is answered now and the server serves the next request. A CONNECT authority is read through `URL`, so `[2001:db8::1]:443` tunnels instead of resolving to host `[2001` on port `NaN`, and an IPv6 host in absolute form has its brackets taken off before it is forwarded. An absolute-form request whose scheme this proxy cannot speak is refused with the reason, and an upstream error after the head has gone out ends the response rather than writing the head twice. diff --git a/packages/interceptors/src/egress-server.ts b/packages/interceptors/src/egress-server.ts index b3e7bce3..fd4a5278 100644 --- a/packages/interceptors/src/egress-server.ts +++ b/packages/interceptors/src/egress-server.ts @@ -96,17 +96,29 @@ export function proxyUrlFor(port: number, caller: EgressCaller = {}): string { return `http://${auth}${EGRESS_LOOPBACK}:${port}`; } +/** + * Null for a percent sequence the caller mangled, which is a credential this seam does not + * recognise rather than a reason to stop: any local process can send `%zz` and did. + */ +function unescaped(value: string): string | null { + try { + return decodeURIComponent(value); + } catch { + return null; + } +} + /** Who a request came from, read off the credentials `proxyUrlFor` put in its URL. */ export function callerOf(headers: IncomingMessage['headers']): EgressCaller { const header = headers[PROXY_AUTHORIZATION]; if (typeof header !== 'string' || !header.startsWith('Basic ')) return {}; const decoded = Buffer.from(header.slice('Basic '.length), 'base64').toString('utf8'); const colon = decoded.indexOf(':'); - const user = colon === -1 ? decoded : decoded.slice(0, colon); - const pass = colon === -1 ? '' : decoded.slice(colon + 1); + const user = unescaped(colon === -1 ? decoded : decoded.slice(0, colon)); + const pass = unescaped(colon === -1 ? '' : decoded.slice(colon + 1)); return { - ...(user === '' ? {} : { sessionId: decodeURIComponent(user) }), - ...(pass === '' ? {} : { agent: decodeURIComponent(pass) }), + ...(user === null || user === '' ? {} : { sessionId: user }), + ...(pass === null || pass === '' ? {} : { agent: pass }), }; } @@ -171,7 +183,11 @@ function firstDestinationRow(row: MemnoxEvent, host: string): MemnoxEvent { function buildServer(options: EgressProxyOptions, open: Set): Server { const server = createServer((request, response) => { - void answerRequest(options, request, response); + // An unhandled rejection ends the process, and this one can be the daemon itself. + void answerRequest(options, request, response).catch(() => { + if (!response.headersSent) response.writeHead(HTTP.BAD_REQUEST); + response.end(); + }); }); server.on('connection', (socket) => { open.add(socket); @@ -179,7 +195,8 @@ function buildServer(options: EgressProxyOptions, open: Set): Server { socket.on('close', () => open.delete(socket)); }); server.on('connect', (request: IncomingMessage, socket: Duplex, head: Buffer) => { - void answerConnect(options, { request, socket, head }); + // Nothing has been written to a tunnel yet, so the socket is the only answer there is. + void answerConnect(options, { request, socket, head }).catch(() => socket.destroy()); }); return server; } @@ -274,10 +291,17 @@ function forward(response: ServerResponse, attempt: HttpAttempt): void { return refuse(response, 'this proxy takes absolute-form requests only'); } + // https goes through CONNECT; httpRequest throws on the spot for any other scheme. + if (target.protocol !== 'http:') + return refuse( + response, + `this proxy forwards http only, and that asked for ${target.protocol}`, + ); + const upstream = httpRequest( { protocol: target.protocol, - hostname: target.hostname, + hostname: withoutBrackets(target.hostname), port: target.port, path: `${target.pathname}${target.search}`, method, @@ -289,21 +313,44 @@ function forward(response: ServerResponse, attempt: HttpAttempt): void { }, ); upstream.on('error', () => { - response.writeHead(HTTP.BAD_GATEWAY).end(); + // A partial answer may already have gone out, and writing the head twice throws. + if (!response.headersSent) response.writeHead(HTTP.BAD_GATEWAY); + response.end(); }); if (body !== undefined) upstream.write(body); upstream.end(); } +/** + * Host and port, read through URL so `[2001:db8::1]:443` parses: splitting on the colon + * gave host `[2001` and a port of NaN, which `net.connect` threw on. + */ +function authorityOf(authority: string): { host: string; port: number } | null { + let parsed: URL; + try { + parsed = new URL(`http://${authority}`); + } catch { + return null; + } + const host = withoutBrackets(parsed.hostname); + if (host === '') return null; + return { host, port: Number(parsed.port === '' ? HTTPS_PORT : parsed.port) }; +} + +/** URL keeps an IPv6 host in brackets; the socket layer wants it without. */ +function withoutBrackets(hostname: string): string { + return hostname.replace(/^\[|\]$/g, ''); +} + /** Bytes only. What travels inside is the blind spot this seam declares. */ function tunnel(authority: string, socket: Duplex, head: Buffer): void { - const [host, rawPort] = authority.split(':'); - if (host === undefined) { + const where = authorityOf(authority); + if (where === null) { socket.destroy(); return; } - const upstream = connect(Number(rawPort ?? HTTPS_PORT), host, () => { + const upstream = connect(where.port, where.host, () => { socket.write(TUNNEL_OK); upstream.write(head); upstream.pipe(socket); diff --git a/packages/interceptors/test/egress-server.test.ts b/packages/interceptors/test/egress-server.test.ts index c776db03..9b52136f 100644 --- a/packages/interceptors/test/egress-server.test.ts +++ b/packages/interceptors/test/egress-server.test.ts @@ -161,6 +161,147 @@ describe('the proxy as a server, on loopback only', () => { }); } + /* The handlers ran as `void answerRequest(...)` with no catch and nothing registers an + unhandledRejection, so one malformed request ended the process — which can be the + daemon. Each case below is answered, and the server serves the next request after it. */ + function allowAll(): EgressSeam { + return new EgressSeam({ + authorizer: new HookAuthorizer({}), + ruled: async () => undefined, + }); + } + + function tunnelTo( + proxyPort: number, + authority: string, + auth: string, + ): Promise<{ status: number; body: string }> { + return new Promise((resolve, reject) => { + const req = request({ + host: '127.0.0.1', + port: proxyPort, + method: 'CONNECT', + path: authority, + headers: { 'proxy-authorization': auth }, + }); + req.on('connect', (res, socket) => { + socket.write( + `GET /hello HTTP/1.1\r\nHost: ${authority}\r\nConnection: close\r\n\r\n`, + ); + let body = ''; + socket.on('data', (chunk: Buffer) => (body += chunk.toString('utf8'))); + socket.on('end', () => resolve({ status: res.statusCode ?? 0, body })); + }); + req.on('error', reject); + req.end(); + }); + } + + it('tunnels to an IPv6 loopback, whose authority holds colons of its own', async () => { + const server = createServer((_req, res) => res.end('upstream says hi')); + opened.push(server); + await new Promise((resolve) => server.listen(0, '::1', () => resolve())); + const address = server.address(); + const port = typeof address === 'object' && address !== null ? address.port : 0; + + const proxy = await startEgressProxy({ + seam: allowAll(), + port: 0, + log: () => undefined, + }); + opened.push(proxy); + const auth = basicOf( + proxyUrlFor(proxy.port, { sessionId: 'ses_1', agent: 'codex-cli' }), + ); + + const answer = await tunnelTo(proxy.port, `[::1]:${port}`, auth); + + expect(answer.body).toContain('upstream says hi'); + }); + + it('answers a mangled Proxy-Authorization, and serves the next request after it', async () => { + const port = await upstream(); + const proxy = await startEgressProxy({ + seam: allowAll(), + port: 0, + log: () => undefined, + }); + opened.push(proxy); + // `%zz` is not a percent sequence, and any local process can send it. + const mangled = `Basic ${Buffer.from('%zz:a', 'utf8').toString('base64')}`; + + const first = await get(proxy.port, `http://127.0.0.1:${port}/hello`, mangled); + const good = basicOf( + proxyUrlFor(proxy.port, { sessionId: 'ses_1', agent: 'codex-cli' }), + ); + const second = await get(proxy.port, `http://127.0.0.1:${port}/hello`, good); + + expect(first.status).toBeGreaterThan(0); + expect(second).toEqual({ status: 200, body: 'upstream says hi' }); + }); + + it('refuses an https absolute-form request rather than throwing on it', async () => { + const port = await upstream(); + const proxy = await startEgressProxy({ + seam: allowAll(), + port: 0, + log: () => undefined, + }); + opened.push(proxy); + const auth = basicOf( + proxyUrlFor(proxy.port, { sessionId: 'ses_1', agent: 'codex-cli' }), + ); + + const refused = await get(proxy.port, 'https://example.com/', auth); + const after = await get(proxy.port, `http://127.0.0.1:${port}/hello`, auth); + + expect(refused.status).toBe(403); + expect(refused.body).toContain('http only'); + expect(after).toEqual({ status: 200, body: 'upstream says hi' }); + }); + + it('forwards to an IPv6 host in absolute form, brackets and all', async () => { + const server = createServer((_req, res) => res.end('upstream says hi')); + opened.push(server); + await new Promise((resolve) => server.listen(0, '::1', () => resolve())); + const address = server.address(); + const port = typeof address === 'object' && address !== null ? address.port : 0; + + const proxy = await startEgressProxy({ + seam: allowAll(), + port: 0, + log: () => undefined, + }); + opened.push(proxy); + const auth = basicOf( + proxyUrlFor(proxy.port, { sessionId: 'ses_1', agent: 'codex-cli' }), + ); + + const answer = await get(proxy.port, `http://[::1]:${port}/hello`, auth); + + expect(answer).toEqual({ status: 200, body: 'upstream says hi' }); + }); + + it('destroys the socket for a CONNECT authority it cannot read', async () => { + const proxy = await startEgressProxy({ + seam: allowAll(), + port: 0, + log: () => undefined, + }); + opened.push(proxy); + const auth = basicOf( + proxyUrlFor(proxy.port, { sessionId: 'ses_1', agent: 'codex-cli' }), + ); + + await expect(tunnelTo(proxy.port, ':::::', auth)).rejects.toThrow(); + + const port = await upstream(); + expect(await get(proxy.port, `http://127.0.0.1:${port}/hello`, auth)).toEqual({ + status: 200, + body: 'upstream says hi', + }); + }); + it('forwards an allowed request, and tells the seam who asked', async () => { const port = await upstream(); const rulings: EgressRuling[] = [];