diff --git a/src/sandbox/mux-proxy.ts b/src/sandbox/mux-proxy.ts index 6c69d1173..e270bc4ee 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 answered = false + upstream.once('data', () => { + answered = true + }) upstream.on('error', err => { + // 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 + } // 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..8b8d98cd0 100644 --- a/test/sandbox/mux-proxy.test.ts +++ b/test/sandbox/mux-proxy.test.ts @@ -120,6 +120,63 @@ 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.each([ + // 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', + 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) + }) + 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 // 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, + ) })