From df8769ddadbde0b2d78c0d9085130ed26fd1b6b7 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 13 Oct 2026 21:04:51 +0000 Subject: [PATCH 1/3] A refused upload keeps its answer, the mux's 502 is for a failed dial, and OPTIONS * keeps its host A request the parser refuses in a tunnel had its socket destroyed as soon as the 400 was flushed. A body still on its way, unread, turned the close into a reset, which discarded the answer. The socket is ended, and destroyed a second later, as after a denial. The mux's handler for a failed dial ran on any error of the backend connection, so a backend that went away under an upload had a clear-text 502 written into an established connection, a TLS tunnel included. Past the dial the error is logged, and the close that follows ends the client. The URL given to filterRequest for the asterisk form was the authority and the "*" put together, which reads the "*" into the host. Its path is empty (RFC 9112 3.3). --- src/sandbox/mux-proxy.ts | 11 +++ src/sandbox/request-filter.ts | 5 +- src/sandbox/tls-terminate-proxy.ts | 17 ++--- test/sandbox/mux-proxy.test.ts | 46 +++++++++++ .../sandbox/request-target-forwarding.test.ts | 76 ++++++++++++++++++- 5 files changed, 139 insertions(+), 16 deletions(-) diff --git a/src/sandbox/mux-proxy.ts b/src/sandbox/mux-proxy.ts index 6c69d1173..1aad55dac 100644 --- a/src/sandbox/mux-proxy.ts +++ b/src/sandbox/mux-proxy.ts @@ -120,7 +120,18 @@ export function createMuxProxyServer(opts: MuxProxyOptions): MuxProxyServer { const upstream = backendSocketPath ? connect(backendSocketPath) : connect(backendTcpPort!, '127.0.0.1') + let dialed = false + upstream.once('connect', () => { + dialed = true + }) upstream.on('error', err => { + // Past the dial the client may be in the middle of anything, a TLS + // tunnel included, where clear text is noise. The 'close' that follows + // ends it. + if (dialed) { + logForDebugging(`mux: HTTP backend error: ${(err as Error).message}`) + return + } // Surface a 502 with the errno code so callers (and CI logs) can tell // *why* the backend was unreachable instead of an opaque empty-reply. // Full error (including path) goes to debug logging only — the client diff --git a/src/sandbox/request-filter.ts b/src/sandbox/request-filter.ts index 0e0c1cbb7..baa0e4183 100644 --- a/src/sandbox/request-filter.ts +++ b/src/sandbox/request-filter.ts @@ -723,9 +723,10 @@ export function refuseUnparsedRequest( 'HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain\r\n' + `${name}: ${value}\r\nContent-Length: ${refused.reason.length + 1}\r\n` + `Connection: close\r\n\r\n${refused.reason}\n`, - () => socket.destroy(), ) - // A flush that never ends must not keep the socket. + // Ended, and destroyed a second later: a body still on its way is read and + // dropped meanwhile. Unread, it would turn the close into a reset, and the + // reset would discard the answer. setTimeout(() => socket.destroy(), 1000).unref?.() return refused } diff --git a/src/sandbox/tls-terminate-proxy.ts b/src/sandbox/tls-terminate-proxy.ts index f8f16bcc1..ad3e6d5a4 100644 --- a/src/sandbox/tls-terminate-proxy.ts +++ b/src/sandbox/tls-terminate-proxy.ts @@ -1066,16 +1066,15 @@ async function forwardUpstream( ) return } + // RFC 9112 3.3: the target URI of the asterisk form (`OPTIONS *`) has an + // empty path. Put behind the authority, the `*` would be read into the host. + const url = `https://${authority}${path === '*' ? '' : path}` let body: Readable = req let decision: RequestDecision | undefined const sni = target.clientServerName ?? serverNameOf(req) if (target.requireHostMatch && target.clientServerNameUnreadable) { const why = 'TLS server name could not be read' - target.onFilterRequestDeny?.( - req.method ?? 'GET', - `https://${authority}${path}`, - why, - ) + target.onFilterRequestDeny?.(req.method ?? 'GET', url, why) respondDenied(res, why, markFor('misdirected'), 421) return } @@ -1087,11 +1086,7 @@ async function forwardUpstream( absoluteFormAuthority(req.url), ) if (why !== undefined) { - target.onFilterRequestDeny?.( - req.method ?? 'GET', - `https://${authority}${path}`, - why, - ) + target.onFilterRequestDeny?.(req.method ?? 'GET', url, why) respondDenied(res, why, markFor('misdirected'), 421) return } @@ -1118,7 +1113,7 @@ async function forwardUpstream( filterRequest, req, res, - `https://${authority}${path}`, + url, ac.signal, target.onFilterRequestDeny, { diff --git a/test/sandbox/mux-proxy.test.ts b/test/sandbox/mux-proxy.test.ts index f795cff0d..00b5af258 100644 --- a/test/sandbox/mux-proxy.test.ts +++ b/test/sandbox/mux-proxy.test.ts @@ -120,6 +120,52 @@ describe('mux-proxy first-byte dispatch', () => { expect([errors, hadError]).toEqual([[], false]) }) + it.skipIf(process.platform === 'win32')( + 'answers 502, with the reason, where the backend cannot be dialled', + async () => { + mux.removeBackendSocketSync() + const reply = await send( + Buffer.from('CONNECT x:443 HTTP/1.1\r\nHost: x:443\r\n\r\n'), + ) + expect(reply.toString()).toBe( + 'HTTP/1.1 502 Bad Gateway\r\nConnection: close\r\n\r\n' + + 'mux backend dial failed (ENOENT)\n', + ) + }, + ) + + it('says nothing of its own into a connection whose backend fails once it is under way', async () => { + httpStub.removeAllListeners('connect') + httpStub.on('connect', (_req, sock) => { + sock.on('error', () => {}) + sock.resume() + sock.write('HTTP/1.1 200 Connection Established\r\n\r\n') + setTimeout(() => sock.destroy(), 50) + }) + for (let n = 0; n < 5; n++) { + const reply = await new Promise(resolve => { + let text = '' + const c = connect(port, '127.0.0.1', () => + c.write('CONNECT x:443 HTTP/1.1\r\nHost: x:443\r\n\r\n'), + ) + // Still sending when the backend goes: the write that fails then is + // the error. + const chunk = Buffer.alloc(64 << 10) + const pump = (): void => { + while (!c.destroyed && c.writable && c.write(chunk)); + } + c.on('drain', pump) + c.on('error', () => {}) + c.on('data', d => { + text += d.toString('latin1') + pump() + }) + c.on('close', () => resolve(text)) + }) + expect(reply).toBe('HTTP/1.1 200 Connection Established\r\n\r\n') + } + }) + it('routes a TLS ClientHello first byte (0x16) to HTTP, not SOCKS', async () => { // Not a valid HTTP request — the backend will 400 it — but the point // is it must NOT be treated as SOCKS. diff --git a/test/sandbox/request-target-forwarding.test.ts b/test/sandbox/request-target-forwarding.test.ts index 63d57d591..a6987c88e 100644 --- a/test/sandbox/request-target-forwarding.test.ts +++ b/test/sandbox/request-target-forwarding.test.ts @@ -8,7 +8,8 @@ import { connect as tlsConnect, createServer as createTlsServer, } from 'node:tls' -import { readFileSync } from 'node:fs' +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' import { join } from 'node:path' import { PassThrough } from 'node:stream' import { @@ -29,6 +30,7 @@ import { } from '../../src/sandbox/credential-aws-pairs.js' import { signSigv4, UNSIGNED_PAYLOAD } from '../../src/sandbox/aws-sigv4.js' import { SandboxManager } from '../../src/sandbox/sandbox-manager.js' +import { spawnAsync } from '../helpers/spawn.js' const FIXTURE_DIR = join(import.meta.dir, '..', 'fixtures', 'tls-terminate') const CA_CERT = join(FIXTURE_DIR, 'ca.crt') @@ -557,6 +559,23 @@ describe('request target forwarding', () => { }, ) + test.each(['default', 'asSpelled', 'normalize'] as const)( + 'with a hook, %s: OPTIONS * is judged under its own host, and sent as it is', + async mode => { + const authority = `${UPSTREAM_NAME}:${tlsPort}` + const [asked, arrived] = [hookSeen.length, tlsSeen.length] + const reply = await exchange( + await tunnel(hookPort[mode]), + `OPTIONS * HTTP/1.1\r\nHost: ${authority}\r\n\r\n`, + ) + expect(reply.status).toBe(200) + expect(tlsSeen.slice(arrived)).toEqual(['*']) + const [seen] = hookSeen.slice(asked) + expect(seen!.url).toBe(`https://${authority}/`) + expect(seen!.info!.requestTarget).toBe('*') + }, + ) + test.each(['default', 'asSpelled', 'normalize'] as const)( 'with a hook, %s: a fragment right after the authority leaves the root', async mode => { @@ -1245,11 +1264,14 @@ describe('refuseUnparsedRequest', () => { expect(socket.readableLength).toBe(answer) }) - test('the socket goes as soon as the answer is out', async () => { + test('the socket is ended at once, and goes a second later', async () => { const socket = new PassThrough({ autoDestroy: false }) socket.resume() refuseUnparsedRequest(parseError('x', 0), socket, true) - await new Promise(r => setTimeout(r, 50)) + expect(socket.writableEnded).toBe(true) + await new Promise(r => setTimeout(r, 500)) + expect(socket.destroyed).toBe(false) + await new Promise(r => setTimeout(r, 700)) expect(socket.destroyed).toBe(true) }) @@ -1429,4 +1451,52 @@ describe('network.ambiguousRequestTarget', () => { ]) }, ) + + // By a client in a process of its own, busy with its upload: one that shares + // this event loop reads the answer the moment it is there. And through the + // manager's proxy, mux and all. + test.skipIf(Bun.which('curl') === null)( + 'in a tunnel, one with a body on its way still gets its answer', + async () => { + await SandboxManager.reset() + await SandboxManager.initialize({ + network: { + allowedDomains: ['127.0.0.1'], + deniedDomains: [], + tlsTerminate: { caCertPath: CA_CERT, caKeyPath: CA_KEY }, + }, + filesystem: { denyRead: [], allowWrite: [], denyWrite: [] }, + }) + const store = SandboxManager.getSandboxViolationStore() + store.clear() + const dir = mkdtempSync(join(tmpdir(), 'refused-upload-')) + try { + writeFileSync(join(dir, 'body'), Buffer.alloc(256 << 10)) + for (let n = 0; n < 8; n++) { + const curl = await spawnAsync( + 'curl', + [ + ...['-sk', '-m', '10', '--noproxy', ''], + '-x', + `http://srt:${SandboxManager.getProxyAuthToken()}@127.0.0.1:${SandboxManager.getProxyPort()}`, + ...['--data-binary', `@${join(dir, 'body')}`], + // As given: what curl makes of such a byte in a URL differs by + // version. + ...['--request-target', '/caf\u00e9'], + `https://127.0.0.1:${port}/`, + ], + { encoding: 'utf8', timeout: 15_000 }, + ) + expect([curl.status, curl.stdout]).toEqual([ + 0, + 'malformed request-target: percent-encode its bytes outside printable ASCII\n', + ]) + } + expect(store.getViolations()).toHaveLength(8) + } finally { + rmSync(dir, { recursive: true, force: true }) + } + }, + 60_000, + ) }) From 014e1470947d393e75b84322b844d7ea2308a800 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 13 Oct 2026 21:32:35 +0000 Subject: [PATCH 2/3] The mux's 502 is an answer, for a client that has had none The condition was that the dial had failed. But the mux is in front of plain HTTP too, and a backend that fails after the dial and before any reply left the client a bare close where it used to get a readable 502. What makes the clear text harmful is that something has already gone back. --- src/sandbox/mux-proxy.ts | 14 ++++---- test/sandbox/mux-proxy.test.ts | 66 ++++++++++++++++++---------------- 2 files changed, 43 insertions(+), 37 deletions(-) diff --git a/src/sandbox/mux-proxy.ts b/src/sandbox/mux-proxy.ts index 1aad55dac..e270bc4ee 100644 --- a/src/sandbox/mux-proxy.ts +++ b/src/sandbox/mux-proxy.ts @@ -120,15 +120,15 @@ export function createMuxProxyServer(opts: MuxProxyOptions): MuxProxyServer { const upstream = backendSocketPath ? connect(backendSocketPath) : connect(backendTcpPort!, '127.0.0.1') - let dialed = false - upstream.once('connect', () => { - dialed = true + let answered = false + upstream.once('data', () => { + answered = true }) upstream.on('error', err => { - // Past the dial the client may be in the middle of anything, a TLS - // tunnel included, where clear text is noise. The 'close' that follows - // ends it. - if (dialed) { + // The 502 is an answer, for a client that has had none. One that has + // may be in the middle of anything, a TLS tunnel included, where clear + // text is noise. The 'close' that follows ends it. + if (answered) { logForDebugging(`mux: HTTP backend error: ${(err as Error).message}`) return } diff --git a/test/sandbox/mux-proxy.test.ts b/test/sandbox/mux-proxy.test.ts index 00b5af258..bbbefb802 100644 --- a/test/sandbox/mux-proxy.test.ts +++ b/test/sandbox/mux-proxy.test.ts @@ -134,37 +134,43 @@ describe('mux-proxy first-byte dispatch', () => { }, ) - it('says nothing of its own into a connection whose backend fails once it is under way', async () => { - httpStub.removeAllListeners('connect') - httpStub.on('connect', (_req, sock) => { - sock.on('error', () => {}) - sock.resume() - sock.write('HTTP/1.1 200 Connection Established\r\n\r\n') - setTimeout(() => sock.destroy(), 50) - }) - for (let n = 0; n < 5; n++) { - const reply = await new Promise(resolve => { - let text = '' - const c = connect(port, '127.0.0.1', () => - c.write('CONNECT x:443 HTTP/1.1\r\nHost: x:443\r\n\r\n'), - ) - // Still sending when the backend goes: the write that fails then is - // the error. - const chunk = Buffer.alloc(64 << 10) - const pump = (): void => { - while (!c.destroyed && c.writable && c.write(chunk)); - } - c.on('drain', pump) - c.on('error', () => {}) - c.on('data', d => { - text += d.toString('latin1') - pump() - }) - c.on('close', () => resolve(text)) + it.each([ + ['', 'HTTP/1.1 502 Bad Gateway\r\n'], + ['HTTP/1.1 200 Connection Established\r\n\r\n', ''], + ])( + 'a backend that fails having answered %p: the mux adds %p', + async (answer, added) => { + httpStub.removeAllListeners('connect') + httpStub.on('connect', (_req, sock) => { + sock.on('error', () => {}) + sock.resume() + if (answer !== '') sock.write(answer) + setTimeout(() => sock.destroy(), 50) }) - expect(reply).toBe('HTTP/1.1 200 Connection Established\r\n\r\n') - } - }) + for (let n = 0; n < 5; n++) { + const reply = await new Promise(resolve => { + let text = '' + // Still sending when the backend goes: the write that fails then + // is the error. + const chunk = Buffer.alloc(64 << 10) + const pump = (): void => { + while (!c.destroyed && c.writable && c.write(chunk)); + } + const c = connect(port, '127.0.0.1', () => { + c.write('CONNECT x:443 HTTP/1.1\r\nHost: x:443\r\n\r\n') + pump() + }) + c.on('drain', pump) + c.on('error', () => {}) + c.on('data', d => (text += d.toString('latin1'))) + c.on('close', () => resolve(text)) + }) + expect(reply.slice(0, answer.length)).toBe(answer) + expect(reply.slice(answer.length).slice(0, added.length)).toBe(added) + if (added === '') expect(reply).toBe(answer) + } + }, + ) it('routes a TLS ClientHello first byte (0x16) to HTTP, not SOCKS', async () => { // Not a valid HTTP request — the backend will 400 it — but the point From d64c2b42f6963011454f112c4ee2528e37c8d8f6 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 13 Oct 2026 21:48:05 +0000 Subject: [PATCH 3/3] test: the mux's 502 for a backend that fails unanswered, on Linux alone Whether a backend that goes away shows at the mux as an error is the platform's. On macOS and Windows the client in this case is just closed, so there is no 502 to tell apart. --- test/sandbox/mux-proxy.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/sandbox/mux-proxy.test.ts b/test/sandbox/mux-proxy.test.ts index bbbefb802..8b8d98cd0 100644 --- a/test/sandbox/mux-proxy.test.ts +++ b/test/sandbox/mux-proxy.test.ts @@ -135,7 +135,12 @@ describe('mux-proxy first-byte dispatch', () => { ) it.each([ - ['', 'HTTP/1.1 502 Bad Gateway\r\n'], + // Whether a backend that goes away shows here as an error at all is the + // platform's. On Linux a write fails first, and it does. On macOS and + // Windows this client is just closed, so the case would tell nothing. + ...(process.platform === 'linux' + ? [['', 'HTTP/1.1 502 Bad Gateway\r\n']] + : []), ['HTTP/1.1 200 Connection Established\r\n\r\n', ''], ])( 'a backend that fails having answered %p: the mux adds %p',