From 9ceb8800ce16fd826b4ff7b47d744d08a17fc428 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Mon, 12 Oct 2026 16:25:40 +0000 Subject: [PATCH 1/5] Never rewrite a request target silently While a filterRequest is set the request target was forwarded normalized. A request signed over its path then no longer verifies, and with a masked AWS credential it is re-signed, validly, for another path. network.ambiguousRequestTarget says what becomes of a target that names another path once normalized: "refuse" (default) answers 400 before the callback is asked, "asSpelled" forwards the spelling, "normalize" forwards the normalized form. Every other target goes out byte for byte. The callback gets both forms, and re-signing covers the bytes sent. --- README.md | 6 +- docs/srt-proxy.md | 4 +- src/sandbox/aws-sigv4.ts | 4 +- src/sandbox/http-proxy.ts | 54 +- src/sandbox/request-filter.ts | 74 ++- src/sandbox/sandbox-config.ts | 17 +- src/sandbox/sandbox-manager.ts | 1 + src/sandbox/tls-terminate-proxy.ts | 52 +- test/config-validation.test.ts | 25 + test/sandbox/request-rewriting.test.ts | 10 +- .../sandbox/request-target-forwarding.test.ts | 605 ++++++++++++++++-- 11 files changed, 773 insertions(+), 79 deletions(-) diff --git a/README.md b/README.md index cc760232c..c99f424e9 100644 --- a/README.md +++ b/README.md @@ -498,8 +498,12 @@ These two are read when the proxy starts; `updateConfig` does not change them. - `network.stripResponseHeaders` - Response headers never passed back to the sandboxed client, matched case-insensitively with `-`, `_` and `.` folded (e.g. `["set-cookie"]`). - `network.refuseOpaqueTunnels` - Refuse every tunnel `filterRequest` cannot see into: an HTTP CONNECT that would not be TLS-terminated or carries no TLS, and every SOCKS CONNECT. Requires `tlsTerminate` to serve HTTPS at all, and stops CONNECT-carried SSH (such as `GIT_SSH_COMMAND`) through the proxy. Refusals are recorded as violations. - `network.requireHostMatch` - Answer 421 to a request whose Host header, TLS server name or absolute-form authority does not name the host and port it is sent to, before `filterRequest` is asked. Refusals are recorded as violations. +- `network.ambiguousRequestTarget` - What becomes of a request whose target is ambiguous: normalised (dot segments and `%2e` resolved, `\` in the path read as `/`, a run of leading slashes collapsed to one, a `#` and what follows dropped) it names another path than as the client spelled it. Upstreams differ in which of the two they act on, so a policy that judged one can be got around through the other. + - `"refuse"` (default) - Answered 400 with `X-Proxy-Error: ambiguous-request-target` before `filterRequest` is asked. Nothing is forwarded, and the refusal is recorded as a violation. + - `"asSpelled"` - Forwarded as the client spelled it. The caller takes on that its `filterRequest` holds for both forms: `/public/../admin` is `/admin` to an upstream that resolves it, and `/admin/../public/x` is under `/admin/` to one that does not. Needs Node, or Bun 1.4 or later: an older Bun's HTTP client resolves dot segments on the way out. + - `"normalize"` - Forwarded normalised. A signature that covers the path no longer verifies (or, where the proxy re-signs for a masked AWS credential, is valid for the other path), and an upstream that takes the path literally, such as an object store where `a/../b` is a key, acts on another resource than the client named. -While `filterRequest` is set, the request target is normalised once (dot segments and `%2e` resolved, a run of leading slashes collapsed to one), and that value is both what the callback sees and what is forwarded, on plain HTTP and inside a terminated tunnel. A target that is not origin form, an absolute `http(s)` URI, or `*` for `OPTIONS` is answered 400. +While `filterRequest` is set, on plain HTTP and inside a terminated tunnel, the target goes upstream byte for byte as the client spelled it (except under `"normalize"`), and the callback's second argument carries both forms, `requestTarget` as spelled and `normalizedRequestTarget`, and `requestTargetIsAmbiguous`: under `"asSpelled"` a callback can refuse an ambiguous target itself, for the hosts and with the status it chooses. `request.url` is a parsed URL, so, as in earlier releases, its path has dot segments already resolved and is not the spelling. Not ambiguous, and forwarded as spelled: `//` after the first segment, `%2F`, `;` parameters, dots in the query, a character the URL parser would only escape (`{`, `"`, `'`), and an empty query. A target that is not origin form, an absolute `http(s)` URI, or `*` for `OPTIONS`, or that holds a byte outside printable ASCII, is answered 400. While `filterRequest` is set, `TRACE` and `TRACK` requests are answered 405 before it is asked, on plain HTTP and inside a terminated tunnel, because their response would echo a header the decision set back to the client. Node's and Bun 1.4's HTTP parsers already answer `TRACK` 400 before the proxy sees it; under Bun 1.3 the connection is closed without an answer. Refusals are recorded as violations. diff --git a/docs/srt-proxy.md b/docs/srt-proxy.md index 828b9a943..4fadcb4c6 100644 --- a/docs/srt-proxy.md +++ b/docs/srt-proxy.md @@ -149,7 +149,9 @@ ids start at 1 and strictly increase. - `path` and `query` are the request target **as the client spelled it**: dot segments and percent escapes are not resolved. A decider that matches paths exactly therefore refuses an unusual spelling of an allowed path. - `query` is left out when there is none. + One that matches by prefix has to refuse dot segments (`..`, `%2e%2e`) + itself: `/public/../admin` begins with `/public/`, and the upstream may + resolve it. `query` is left out when there is none. - `headers` maps each lower-case header name to its values, in order, with `Host` apart. diff --git a/src/sandbox/aws-sigv4.ts b/src/sandbox/aws-sigv4.ts index 79bcfc11c..a6917ccce 100644 --- a/src/sandbox/aws-sigv4.ts +++ b/src/sandbox/aws-sigv4.ts @@ -163,8 +163,8 @@ export function uriEncode(s: string): string { * awslabs/aws-sigv4-proxy, which disables path escaping for s3 only. * * rawPath is the target the proxy forwards, so the signature covers the - * path on the wire: the client's bytes unchanged when no filterRequest is - * configured, the normalized target when one is. + * path on the wire: the client's bytes unchanged, or the normalized target + * under ambiguousRequestTarget 'normalize'. */ export function canonicalUri(rawPath: string, service: string): string { if (rawPath === '') return '/' diff --git a/src/sandbox/http-proxy.ts b/src/sandbox/http-proxy.ts index 740881e38..65d87df18 100644 --- a/src/sandbox/http-proxy.ts +++ b/src/sandbox/http-proxy.ts @@ -17,8 +17,10 @@ import { import { CRL_PATH, type MitmCA } from './mitm-ca.js' import { decideAndRespond, - normalizeRequestTarget, + readRequestTarget, requestTargetAsSpelled, + respondAmbiguousTarget, + type AmbiguousRequestTarget, type DenyMark, applyHeaderEdits, notifyResponse, @@ -304,6 +306,17 @@ export interface HttpProxyServerOptions { */ requestTargetAsSpelled?: boolean + /** + * What becomes of a request filterRequest would judge whose target is + * ambiguous (see readRequestTarget), on both paths. `'refuse'`, the + * default: 400 before filterRequest is asked, reported through + * onFilterRequestDenied. `'asSpelled'`: forwarded as the client spelled + * it; filterRequest has to judge both forms. `'normalize'`: forwarded + * normalized, which breaks a signature over the path and names another + * resource to an upstream that takes the path literally. + */ + ambiguousRequestTarget?: AmbiguousRequestTarget + /** * Refuse, with 400 and before the allowlist is consulted, a request * whose destination host is not spelled in plain ASCII: letters, digits, @@ -775,6 +788,7 @@ export function createHttpProxyServer(options: HttpProxyServerOptions): Server { foldHopByHop: options.foldHopByHop, denyHeader: options.denyHeader, requestTargetAsSpelled: options.requestTargetAsSpelled, + ambiguousRequestTarget: options.ambiguousRequestTarget, byteBudget, onEstablished: () => clearTimeout(deadline), onHandshakeProbe: isDone => { @@ -1034,15 +1048,23 @@ export function createHttpProxyServer(options: HttpProxyServerOptions): Server { // forwarding the client's raw req.url. This ensures the upstream proxy // sees exactly the host we allowlist-checked, closing URL-parser // differential bypasses. - // With filterRequest, the request target is normalized before the - // hook sees it and that normalized target is what is forwarded; - // without it, the parsed path and query are forwarded unchanged. - // requestTargetAsSpelled comes first: it forwards the target as the - // client spelled it, hook or no hook. + // With filterRequest, the hook gets the target as spelled and + // normalized, the spelling is what is forwarded, and a target whose + // two forms can name different resources is refused unless + // ambiguousRequestTarget says which to send (see forwardUpstream in + // tls-terminate-proxy.ts); without it, the parsed path and query are + // forwarded unchanged. requestTargetAsSpelled comes first: it forwards + // the target as the client spelled it, hook or no hook. + const read = + options.filterRequest && !options.requestTargetAsSpelled + ? readRequestTarget(req.url!, req.method) + : undefined const requestTarget = options.requestTargetAsSpelled ? requestTargetAsSpelled(req.url!, { absolute: true }) : options.filterRequest - ? normalizeRequestTarget(req.url!, req.method) + ? options.ambiguousRequestTarget === 'normalize' + ? read?.normalized + : read?.spelled : `${url.pathname}${url.search}` if (requestTarget === undefined) { respondDenied( @@ -1054,6 +1076,20 @@ export function createHttpProxyServer(options: HttpProxyServerOptions): Server { return } const absUrl = `${url.protocol}//${authority}${requestTarget}` + if ( + read?.ambiguous && + options.ambiguousRequestTarget !== 'asSpelled' && + options.ambiguousRequestTarget !== 'normalize' + ) { + const reason = respondAmbiguousTarget(res, markFor('bad_request')) + options.onFilterRequestDenied?.({ + method: req.method ?? 'GET', + url: absUrl, + reason, + encodedCommand: auth.encodedCommand, + }) + return + } // Per-request filter applies to plain HTTP too — otherwise a sandboxed // client could bypass it by using http:// where the upstream serves it. @@ -1119,7 +1155,9 @@ export function createHttpProxyServer(options: HttpProxyServerOptions): Server { : undefined, { target: { host: hostname, port }, - requestTarget, + requestTarget: read?.spelled ?? requestTarget, + normalizedRequestTarget: read?.normalized, + requestTargetIsAmbiguous: read?.ambiguous, rawHeaders: [...req.rawHeaders], scheme: 'http', }, diff --git a/src/sandbox/request-filter.ts b/src/sandbox/request-filter.ts index 371a68c0b..22e9e2fea 100644 --- a/src/sandbox/request-filter.ts +++ b/src/sandbox/request-filter.ts @@ -114,14 +114,29 @@ export type RequestInfo = { */ target?: { host: string; port: number } /** - * The request-target in origin form (path and query), exactly as it is - * forwarded upstream; path rules should key on this. It is normalized - * once on both paths (see normalizeRequestTarget) and that exact value - * is forwarded. (With no filterRequest configured there is no hook to - * judge the target: plain HTTP forwards the URL-parsed path and query, + * The request-target in origin form (path and query) as the client + * spelled it. These are the bytes forwarded upstream, unless the proxy + * was created with `ambiguousRequestTarget: 'normalize'`. `request.url` + * is a parsed URL and cannot hold them: its path has dot segments + * resolved already. (With no filterRequest configured there is no hook + * to judge the target: plain HTTP forwards the URL-parsed path and query, * and the TLS-terminated path forwards the client's bytes unchanged.) */ requestTarget?: string + /** + * The same target normalized (see normalizeRequestTarget): what an + * upstream that resolves the spelling acts on, and what is forwarded + * under `'normalize'`. Where it names another path than `requestTarget` + * (only under `'asSpelled'` and `'normalize'`), a path rule has to hold + * for both. + */ + normalizedRequestTarget?: string + /** + * Whether the two forms can name different resources (see + * readRequestTarget): what `'refuse'` refuses. They also differ, harmlessly, + * by a character the URL parser escapes and by an empty query. + */ + requestTargetIsAmbiguous?: boolean /** * A copy of the client's header lines as received: name, value, name, * value, ... Some runtimes lower-case the names. @@ -524,6 +539,55 @@ export function normalizeRequestTarget( return `${url.pathname.replace(/^\/{2,}/, '/')}${url.search}` } +/** What a proxy with a filterRequest does with an ambiguous request-target. */ +export type AmbiguousRequestTarget = 'refuse' | 'asSpelled' | 'normalize' + +/** + * Both forms of a request-target that a filterRequest judges: `spelled`, in + * origin form as the client wrote it, and `normalized` (see + * normalizeRequestTarget). It is `ambiguous` when the two can name different + * resources, so that an upstream may act on another one than a policy + * judged: a dot segment (`%2e` included), a run of leading slashes, `\` in + * the path, or `#`. Not ambiguous, because every reader takes both forms for + * the same resource: a character the URL parser escapes (which ones differs + * by runtime), an empty query it drops, and the `/` an empty path becomes. + * Undefined for a shape normalizeRequestTarget refuses, an authority that + * ends elsewhere than the URL parser ends it, and a byte outside printable + * ASCII (a runtime that decodes those does not send them back unchanged). + */ +export function readRequestTarget( + raw: string, + method: string | undefined, +): { spelled: string; normalized: string; ambiguous: boolean } | undefined { + const normalized = normalizeRequestTarget(raw, method) + if (normalized === undefined) return undefined + let spelled = raw.replace(/^https?:\/\/[^/?#\\]+/i, '') + if (spelled === '' || spelled.startsWith('?')) spelled = '/' + spelled + if (!/^(?:\*|\/[!-~]*)$/.test(spelled)) return undefined + const [a, b] = [spelled, normalized].map(t => + t.replace( + /["'<>^`{}]/g, + c => '%' + c.charCodeAt(0).toString(16).toUpperCase(), + ), + ) + return { spelled, normalized, ambiguous: a !== b && a !== b + '?' } +} + +const AMBIGUOUS_TARGET_REASON = + 'ambiguous request-target: as spelled and normalized it names different ' + + 'paths (a dot segment, leading slashes, "\\" or "#"), and ' + + 'network.ambiguousRequestTarget is "refuse"; nothing was forwarded' + +/** Refuse an ambiguous request-target: 400, and the reason for `onDeny`. */ +export function respondAmbiguousTarget( + res: ServerResponse, + mark: DenyMark = 'ambiguous-request-target', +): string { + logForDebugging(`[proxy] ${AMBIGUOUS_TARGET_REASON}`, { level: 'warn' }) + respondDenied(res, AMBIGUOUS_TARGET_REASON, mark, 400) + return AMBIGUOUS_TARGET_REASON +} + /** * A request-target as the client spelled it, for a proxy that forwards it * unchanged and lets its policy match it exactly: dot segments and diff --git a/src/sandbox/sandbox-config.ts b/src/sandbox/sandbox-config.ts index e74d69c0c..a026a1dd1 100644 --- a/src/sandbox/sandbox-config.ts +++ b/src/sandbox/sandbox-config.ts @@ -779,7 +779,7 @@ export const NetworkConfigSchema = z.object({ 'request is denied. Applies to plain HTTP through the proxy and, ' + 'when tlsTerminate is configured, to terminated HTTPS. SRT does not ' + 'provide a policy language; library consumers own matching. This and ' + - 'the four options after it are read when the proxy starts; ' + + 'the five options after it are read when the proxy starts; ' + 'updateConfig does not change them.', ), allowPlaintextHeaderSet: z @@ -819,6 +819,21 @@ export const NetworkConfigSchema = z.object({ 'to, before filterRequest is asked. Refusals are recorded as ' + 'violations.', ), + ambiguousRequestTarget: z + .enum(['refuse', 'asSpelled', 'normalize']) + .optional() + .describe( + 'While filterRequest is set: what becomes of a request whose target ' + + 'names another path once normalized than as the client spelled it ' + + '(a dot segment, "%2e" included, leading slashes, "\\" or "#"), so ' + + 'that the upstream may act on another resource than the callback ' + + 'judged. "refuse" (default): 400 before filterRequest is asked, ' + + 'recorded as a violation. "asSpelled": forwarded as spelled; the ' + + 'callback has to judge both forms, which it gets as requestTarget ' + + 'and normalizedRequestTarget. "normalize": forwarded normalized, ' + + 'which breaks a signature over the path and names another resource ' + + 'to an upstream that takes the path literally.', + ), tlsTerminate: z .object({ caCertPath: z diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index 2bdda4b9c..407d25635 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -744,6 +744,7 @@ async function startMuxProxyServer( stripResponseHeaders: config?.network.stripResponseHeaders, refuseOpaqueTunnels: config?.network.refuseOpaqueTunnels, requireHostMatch: config?.network.requireHostMatch, + ambiguousRequestTarget: config?.network.ambiguousRequestTarget, maxTerminatedTunnels: config?.network.tlsTerminate?.maxTunnels, tlsHandshakeTimeoutMs: config?.network.tlsTerminate?.handshakeTimeoutMs, onFilterRequestDenied: ({ method, url, reason, encodedCommand }) => { diff --git a/src/sandbox/tls-terminate-proxy.ts b/src/sandbox/tls-terminate-proxy.ts index ef1e315ab..774726d6a 100644 --- a/src/sandbox/tls-terminate-proxy.ts +++ b/src/sandbox/tls-terminate-proxy.ts @@ -30,8 +30,10 @@ import { import type { MitmCA } from './mitm-ca.js' import { decideAndRespond, - normalizeRequestTarget, + readRequestTarget, requestTargetAsSpelled, + respondAmbiguousTarget, + type AmbiguousRequestTarget, type DenyMark, applyHeaderEdits, notifyResponse, @@ -297,6 +299,8 @@ export type TerminateTarget = { denyHeader?: string /** Forward the request-target as spelled (see HttpProxyServerOptions.requestTargetAsSpelled). */ requestTargetAsSpelled?: boolean + /** What becomes of an ambiguous request-target (see HttpProxyServerOptions.ambiguousRequestTarget). */ + ambiguousRequestTarget?: AmbiguousRequestTarget /** The TLS server name the client sent (set by terminateAndForward). */ clientServerName?: string /** The ClientHello carries a server name that could not be read (see serverNameFromClientHello). */ @@ -859,22 +863,31 @@ async function forwardUpstream( // yields a well-formed URL, and discard any client-supplied authority so // the CONNECT-verified target stays authoritative (same rationale as the // Host-header note below). - // With filterRequest, the request target is normalized before the hook - // sees it and that normalized target is what is forwarded: some runtimes' - // HTTP clients normalize it again on the way out, and the hook must judge - // exactly what is sent. A target of any other shape could make the judged - // URL name another host, so it is refused. Without filterRequest nothing - // judges the path, so the target is forwarded unchanged (and a masked AWS - // credential is re-signed over those same bytes). requestTargetAsSpelled - // comes first: it forwards the target as the client spelled it. + // With filterRequest, the hook gets the target as spelled and normalized, + // and the spelling is what is forwarded: rewriting it would break a + // signature over the path, or have a masked AWS credential re-signed for + // a resource the client did not name. Where the two forms can name + // different resources the hook may have judged another one than the + // upstream acts on, so the request is refused unless + // ambiguousRequestTarget says which form to send. A target of any other + // shape could make the judged URL name another host, so it is refused. + // Without filterRequest nothing judges the path, so the target is + // forwarded unchanged. requestTargetAsSpelled comes first: it forwards the + // target as the client spelled it. const markFor = (code: string): DenyMark | undefined => target.denyHeader !== undefined ? { name: target.denyHeader, value: code } : undefined + const read = + filterRequest && !target.requestTargetAsSpelled + ? readRequestTarget(req.url ?? '/', req.method) + : undefined const path = target.requestTargetAsSpelled ? requestTargetAsSpelled(req.url ?? '/') : filterRequest - ? normalizeRequestTarget(req.url ?? '/', req.method) + ? target.ambiguousRequestTarget === 'normalize' + ? read?.normalized + : read?.spelled : originFormPath(req.url) if (path === undefined) { respondDenied(res, 'malformed request-target', markFor('bad_request'), 400) @@ -897,6 +910,21 @@ async function forwardUpstream( } // The tunnel target as it goes on the wire: filterRequest URL, Host, SigV4. const authority = formatAuthority(target.hostname, target.port, 443) + if ( + read?.ambiguous && + target.ambiguousRequestTarget !== 'asSpelled' && + target.ambiguousRequestTarget !== 'normalize' + ) { + const why = respondAmbiguousTarget(res, markFor('bad_request')) + target.onFilterRequestDeny?.( + req.method ?? 'GET', + `https://${authority}${path}`, + why, + ) + // Like a denial: a body still to come must not keep the tunnel. + destroyAfterDenial(req, res) + return + } let body: Readable = req let decision: RequestDecision | undefined const sni = target.clientServerName ?? serverNameOf(req) @@ -955,7 +983,9 @@ async function forwardUpstream( { sni, target: { host: target.hostname, port: target.port }, - requestTarget: path, + requestTarget: read?.spelled ?? path, + normalizedRequestTarget: read?.normalized, + requestTargetIsAmbiguous: read?.ambiguous, rawHeaders: [...req.rawHeaders], scheme: 'https', }, diff --git a/test/config-validation.test.ts b/test/config-validation.test.ts index 1f5b963c9..63e6b83af 100644 --- a/test/config-validation.test.ts +++ b/test/config-validation.test.ts @@ -207,6 +207,31 @@ describe('Config Validation', () => { } }) + test('network.ambiguousRequestTarget takes its three values and no other', () => { + const parse = (ambiguousRequestTarget: unknown) => + SandboxRuntimeConfigSchema.safeParse({ + network: { + allowedDomains: [], + deniedDomains: [], + ambiguousRequestTarget, + }, + filesystem: { denyRead: [], allowWrite: [], denyWrite: [] }, + }) + for (const value of [ + undefined, + 'refuse', + 'asSpelled', + 'normalize', + ] as const) { + const result = parse(value) + expect(result.success).toBe(true) + expect(result.data?.network.ambiguousRequestTarget).toBe(value) + } + for (const value of ['', 'normalized', 'asspelled', true, null]) { + expect(parse(value).success).toBe(false) + } + }) + test('should validate config with enableWeakerNetworkIsolation', () => { const config = { network: { diff --git a/test/sandbox/request-rewriting.test.ts b/test/sandbox/request-rewriting.test.ts index db9a80bbf..f890ef73a 100644 --- a/test/sandbox/request-rewriting.test.ts +++ b/test/sandbox/request-rewriting.test.ts @@ -739,9 +739,10 @@ describe('normalizeRequestTarget', () => { }) }) -describe('the request-target the callback sees is the one forwarded', () => { +describe("'normalize': the normalized request-target the callback sees is the one forwarded", () => { // Every case runs a TLS-terminating proxy: skipped where none can run. const test = testWithTls + const normalize = { ambiguousRequestTarget: 'normalize' } as const // Each spelling would pass a naive "/public/" prefix rule, or dodge a // "/admin" deny rule, if the callback saw it raw while the proxy (or the // runtime's HTTP client) forwarded a normalized form. @@ -759,7 +760,7 @@ describe('the request-target the callback sees is the one forwarded', () => { const recordTarget = (sink: { got?: string }): FilterRequestCallback => async (_req, info) => { - sink.got = info?.requestTarget + sink.got = info?.normalizedRequestTarget return { action: 'allow' } } for (const [label, send] of [ @@ -770,7 +771,7 @@ describe('the request-target the callback sees is the one forwarded', () => { test(`${label}: ${spelling}`, async () => { const sink: { got?: string } = {} const before = seen.length - await withProxy(recordTarget(sink), {}, async port => { + await withProxy(recordTarget(sink), normalize, async port => { expect((await send(port, spelling)).status).toBe(200) }) expect(seen.length).toBe(before + 1) @@ -779,7 +780,7 @@ describe('the request-target the callback sees is the one forwarded', () => { } test(`${label}: dot segments and leading slashes are normalized before the callback sees them`, async () => { const sink: { got?: string } = {} - await withProxy(recordTarget(sink), {}, async port => { + await withProxy(recordTarget(sink), normalize, async port => { await send(port, '/public/%2e%2e/admin') expect(sink.got).toBe('/admin') expect(lastSeen().url).toBe('/admin') @@ -1410,6 +1411,7 @@ describe('network config', () => { stripResponseHeaders: ['set-cookie'], refuseOpaqueTunnels: true, requireHostMatch: true, + ambiguousRequestTarget: 'asSpelled', }) expect(ok.success).toBe(true) expect( diff --git a/test/sandbox/request-target-forwarding.test.ts b/test/sandbox/request-target-forwarding.test.ts index 8b223ce9b..fd8a29d83 100644 --- a/test/sandbox/request-target-forwarding.test.ts +++ b/test/sandbox/request-target-forwarding.test.ts @@ -1,4 +1,4 @@ -import { test, expect, beforeAll, afterAll } from 'bun:test' +import { test, expect, beforeAll, afterAll, spyOn } from 'bun:test' // Every case here runs a proxy configured to terminate TLS, so each is // skipped where this runtime cannot terminate it in-process. import { describeWithTls as describe } from '../helpers/emitted-connections.js' @@ -10,9 +10,23 @@ import { } from 'node:tls' import { readFileSync } from 'node:fs' import { join } from 'node:path' -import { createHttpProxyServer } from '../../src/sandbox/http-proxy.js' +import { + createHttpProxyServer, + type HttpProxyServerOptions, +} from '../../src/sandbox/http-proxy.js' import { createMitmCA } from '../../src/sandbox/mitm-ca.js' import { mintLeafCert } from '../../src/sandbox/mitm-leaf.js' +import { + readRequestTarget, + type RequestInfo, +} from '../../src/sandbox/request-filter.js' +import { SentinelRegistry } from '../../src/sandbox/credential-sentinel.js' +import { + AwsPairRegistry, + createSigv4Planner, +} 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' const FIXTURE_DIR = join(import.meta.dir, '..', 'fixtures', 'tls-terminate') const CA_CERT = join(FIXTURE_DIR, 'ca.crt') @@ -47,6 +61,50 @@ const PLAIN_UNCHANGED: Case[] = [ ['/a;p=1/b;q?x=1', '/a;p=1/b;q?x=1'], ] +// With filterRequest. Ambiguous: as spelled, and normalized. +const AMBIGUOUS: Case[] = [ + ['/a/../b', '/b'], + ['/a/./b', '/a/b'], + ['/a/..', '/'], + ['/a/.', '/a/'], + ['/a/%2e%2e/b', '/b'], + ['/a/.%2E/b', '/b'], + ['/a/%2e/b', '/a/b'], + ['/a;/../b', '/b'], + ['/a//../b', '/a/b'], + ['//k/x', '/k/x'], + ['///k?q=1', '/k?q=1'], + ['/a\\b', '/a/b'], + ['/\\k', '/k'], + ['/a#frag', '/a'], + ['/a?x#frag', '/a?x'], +] + +// Not ambiguous: normalizing leaves them alone, or only escapes a character +// or drops an empty query. +const UNAMBIGUOUS = [ + '/', + '/a/b/?x=1', + '/a//b', + '/a/b//', + '/a%2Fb', + '/a%2f..%2fb', + '/a/..%2fb', + '/a/%252e%252e/b', + '/a/...', + '/a/..b', + '/a/.hidden', + '/a/..;/b', + '/a;p=1/b;q?x=1', + '/a?x=../y&z=//&w=\\', + '/%61%7e', + '/a%zz', + '/a?', + '/a??', + '/a{b}|c^d"eg`h', + `/a?q='x'&r="y"`, +] + const lookup = ((_hostname, options, callback) => { const all = typeof options === 'object' && options.all if (all) callback(null, [{ address: '127.0.0.1', family: 4 }]) @@ -54,7 +112,10 @@ const lookup = ((_hostname, options, callback) => { }) as LookupFunction /** Records the request target of every request head, off the wire. */ -function recordTargets(seen: string[]): (socket: Socket) => void { +function recordTargets( + seen: string[], + heads: string[] = [], +): (socket: Socket) => void { return socket => { let buf = '' socket.on('error', () => {}) @@ -62,6 +123,7 @@ function recordTargets(seen: string[]): (socket: Socket) => void { buf += chunk.toString('latin1') for (let end; (end = buf.indexOf('\r\n\r\n')) >= 0; ) { seen.push(buf.slice(0, buf.indexOf('\r\n')).split(' ')[1]!) + heads.push(buf.slice(0, end)) buf = buf.slice(end + 4) socket.write('HTTP/1.1 200 OK\r\ncontent-length: 0\r\n\r\n') } @@ -69,32 +131,46 @@ function recordTargets(seen: string[]): (socket: Socket) => void { } } -/** Writes one request head and resolves with the reply's status code. */ -function exchange(socket: Socket, head: string): Promise { +type Reply = { status: number; text: string } + +/** Writes one request head and resolves with the reply, read to its end. */ +function exchange(socket: Socket, head: string): Promise { return new Promise((resolve, reject) => { let buf = '' socket.on('data', chunk => { buf += chunk.toString('latin1') if (!buf.includes('\r\n\r\n')) return + if ( + /^transfer-encoding: chunked/im.test(buf) && + !/\r\n0\r\n\r\n$/.test(buf) + ) + return socket.destroy() - resolve(Number(/^HTTP\/1\.1 (\d+)/.exec(buf)?.[1] ?? 0)) + resolve({ + status: Number(/^HTTP\/1\.1 (\d+)/.exec(buf)?.[1] ?? 0), + text: buf, + }) }) socket.once('error', reject) socket.once('close', () => reject(new Error('closed before a reply'))) - socket.write(head) + socket.write(Buffer.from(head, 'latin1')) }) } describe('request target forwarding', () => { const ca = createMitmCA({ caCertPath: CA_CERT, caKeyPath: CA_KEY }) const tlsSeen: string[] = [] + const tlsHeads: string[] = [] const plainSeen: string[] = [] - const hookSeen: (string | undefined)[] = [] + const hookSeen: { url: string; info?: RequestInfo }[] = [] + const denied: { method: string; url: string; reason: string }[] = [] const servers: Server[] = [] let tlsPort: number let plainPort: number let noHookPort: number - let hookPort: number + /** A proxy with an allow-all hook, by its ambiguousRequestTarget. */ + const hookPort = { default: 0, refuse: 0, asSpelled: 0, normalize: 0 } + let base: HttpProxyServerOptions async function listen(server: Server): Promise { servers.push(server) @@ -108,25 +184,36 @@ describe('request target forwarding', () => { /-----BEGIN CERTIFICATE-----[\s\S]*?-----END CERTIFICATE-----\r?\n?/, )![0] tlsPort = await listen( - createTlsServer({ cert, key: leaf.keyPem }, recordTargets(tlsSeen)), + createTlsServer( + { cert, key: leaf.keyPem }, + recordTargets(tlsSeen, tlsHeads), + ), ) plainPort = await listen(createServer(recordTargets(plainSeen))) - const base = { + base = { filter: () => true, mitmCA: ca, tlsTerminateUpstreamCA: CA_PEM, lookupFor: () => lookup, } noHookPort = await listen(createHttpProxyServer(base)) - hookPort = await listen( - createHttpProxyServer({ - ...base, - filterRequest: async (_request, info) => { - hookSeen.push(info?.requestTarget) - return { action: 'allow' } - }, - }), - ) + for (const mode of [ + 'default', + 'refuse', + 'asSpelled', + 'normalize', + ] as const) { + hookPort[mode] = await listen( + createHttpProxyServer({ + ...base, + ambiguousRequestTarget: mode === 'default' ? undefined : mode, + filterRequest: async (request, info) => { + hookSeen.push({ url: request.url, info }) + return { action: 'allow' } + }, + }), + ) + } }) afterAll(async () => { @@ -135,8 +222,10 @@ describe('request target forwarding', () => { } }) - /** One GET over a fresh TLS-terminated tunnel; the target upstream saw. */ - async function viaTls(proxyPort: number, target: string): Promise { + type Sent = Reply & { forwarded: string | undefined } + + /** A fresh TLS-terminated tunnel to the upstream. */ + async function tunnel(proxyPort: number): Promise { const authority = `${UPSTREAM_NAME}:${tlsPort}` const raw = await new Promise((resolve, reject) => { const socket = connect(proxyPort, '127.0.0.1', () => { @@ -147,22 +236,32 @@ describe('request target forwarding', () => { socket.once('error', reject) socket.once('data', () => resolve(socket)) }) - const tls = await new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { const t = tlsConnect( { socket: raw, ca: CA_PEM, servername: UPSTREAM_NAME }, () => resolve(t), ) t.once('error', reject) }) + } + + /** One request over a fresh tunnel, and the target upstream saw. */ + async function sendTls( + proxyPort: number, + target: string, + headers: string[] = [], + ): Promise { + const authority = `${UPSTREAM_NAME}:${tlsPort}` + const tls = await tunnel(proxyPort) const before = tlsSeen.length - const head = `GET ${target} HTTP/1.1\r\nHost: ${authority}\r\n\r\n` - expect(await exchange(tls, head)).toBe(200) - expect(tlsSeen.length).toBe(before + 1) - return tlsSeen[before]! + const lines = [`GET ${target} HTTP/1.1`, `Host: ${authority}`, ...headers] + const reply = await exchange(tls, lines.join('\r\n') + '\r\n\r\n') + expect(tlsSeen.length - before).toBe(reply.status === 200 ? 1 : 0) + return { ...reply, forwarded: tlsSeen[before] } } - /** One absolute-form GET through the proxy; the target upstream saw. */ - async function viaPlain(proxyPort: number, target: string): Promise { + /** One absolute-form GET through the proxy, and the target upstream saw. */ + async function sendPlain(proxyPort: number, target: string): Promise { const authority = `127.0.0.1:${plainPort}` const before = plainSeen.length const head = `GET http://${authority}${target} HTTP/1.1\r\nHost: ${authority}\r\n\r\n` @@ -170,37 +269,451 @@ describe('request target forwarding', () => { const s = connect(proxyPort, '127.0.0.1', () => resolve(s)) s.once('error', reject) }) - expect(await exchange(socket, head)).toBe(200) - expect(plainSeen.length).toBe(before + 1) - return plainSeen[before]! + const reply = await exchange(socket, head) + expect(plainSeen.length - before).toBe(reply.status === 200 ? 1 : 0) + return { ...reply, forwarded: plainSeen[before] } } + const LEGS = [ + ['TLS-terminated', sendTls], + ['plain HTTP', sendPlain], + ] as const + describe('no hook: target forwarded unchanged', () => { test.each(TLS_UNCHANGED)('TLS-terminated %s', async (sent, forwarded) => { - expect(await viaTls(noHookPort, sent)).toBe(forwarded) + expect((await sendTls(noHookPort, sent)).forwarded).toBe(forwarded) }) test.each(PLAIN_UNCHANGED)('plain HTTP %s', async (sent, forwarded) => { - expect(await viaPlain(noHookPort, sent)).toBe(forwarded) + expect((await sendPlain(noHookPort, sent)).forwarded).toBe(forwarded) }) }) - describe('with a hook: the target it judges is the target forwarded', () => { - test.each(TLS_UNCHANGED)('TLS-terminated %s', async sent => { - const forwarded = await viaTls(hookPort, sent) - expect(hookSeen.at(-1)).toBe(forwarded) + describe.each(LEGS)('with a hook, %s', (_leg, send) => { + // The upstream may act on either form, and the hook has not said that it + // judged both: neither is sent. + describe.each(['default', 'refuse'] as const)('%s', mode => { + test.each(AMBIGUOUS)('%s is refused, loudly', async sent => { + const asked = hookSeen.length + const reply = await send(hookPort[mode], sent) + expect(reply.status).toBe(400) + expect(reply.text).toMatch( + /^x-proxy-error: ambiguous-request-target\r$/im, + ) + expect(reply.text).toContain('network.ambiguousRequestTarget') + expect(reply.forwarded).toBeUndefined() + expect(hookSeen.length).toBe(asked) + }) }) - test.each(PLAIN_UNCHANGED)('plain HTTP %s', async sent => { - const forwarded = await viaPlain(hookPort, sent) - expect(hookSeen.at(-1)).toBe(forwarded) + test.each(AMBIGUOUS)( + 'asSpelled: %s goes out as spelled, and the hook gets both forms', + async (sent, normalized) => { + expect((await send(hookPort.asSpelled, sent)).forwarded).toBe(sent) + expect(hookSeen.at(-1)!.info?.requestTarget).toBe(sent) + expect(hookSeen.at(-1)!.info?.normalizedRequestTarget).toBe(normalized) + }, + ) + + test.each(AMBIGUOUS)( + 'normalize: %s goes out as %s, and the hook gets both forms', + async (sent, normalized) => { + const reply = await send(hookPort.normalize, sent) + expect(reply.forwarded).toBe(normalized) + expect(hookSeen.at(-1)!.info?.requestTarget).toBe(sent) + expect(hookSeen.at(-1)!.info?.normalizedRequestTarget).toBe(normalized) + expect(hookSeen.at(-1)!.url.endsWith(normalized)).toBe(true) + }, + ) + + test.each(UNAMBIGUOUS)('%s goes out byte for byte', async sent => { + for (const mode of ['default', 'asSpelled'] as const) { + expect((await send(hookPort[mode], sent)).forwarded).toBe(sent) + expect(hookSeen.at(-1)!.info?.requestTarget).toBe(sent) + } + const { forwarded } = await send(hookPort.normalize, sent) + expect(forwarded).toBe(hookSeen.at(-1)!.info?.normalizedRequestTarget) + }) + + // What a rule on request.url alone judges is the parsed URL. By default + // nothing goes out whose spelling says anything else. + test('a prefix rule on request.url is not got around in either direction', async () => { + const port = await listen( + createHttpProxyServer({ + ...base, + filterRequest: async request => ({ + action: new URL(request.url).pathname.startsWith('/allowed/') + ? 'allow' + : 'deny', + }), + }), + ) + expect((await send(port, '/allowed/x')).forwarded).toBe('/allowed/x') + for (const target of [ + '/allowed/../secret', + '/allowed/%2e%2e/secret', + '/allowed\\..\\secret', + '//secret', + '/secret', + // Under /secret/ to an upstream that takes the path literally. + '/secret/../allowed/x', + '/secret/%2E%2E/allowed/x', + '/secret\\..\\allowed/x', + ]) { + const reply = await send(port, target) + expect([400, 403]).toContain(reply.status) + expect(reply.forwarded).toBeUndefined() + } + }) + + test('asSpelled: a hook can refuse an ambiguous target itself, where and how it likes', async () => { + const port = await listen( + createHttpProxyServer({ + ...base, + ambiguousRequestTarget: 'asSpelled', + filterRequest: async (_request, info) => + info!.requestTargetIsAmbiguous === false + ? { action: 'allow' } + : { action: 'deny', status: 422 }, + }), + ) + expect((await send(port, `/a{b}?q='x'`)).status).toBe(200) + expect((await send(port, '/a/../b')).status).toBe(422) }) - test('dot segments and a leading // are resolved before the hook', async () => { - expect(await viaTls(hookPort, '//a/../b')).toBe('/b') - expect(hookSeen.at(-1)).toBe('/b') - expect(await viaPlain(hookPort, '//k/x')).toBe('/k/x') - expect(hookSeen.at(-1)).toBe('/k/x') + test('a value that is none of the three refuses', async () => { + const port = await listen( + createHttpProxyServer({ + ...base, + ambiguousRequestTarget: 'spelled' as 'asSpelled', + filterRequest: async () => ({ action: 'allow' }), + }), + ) + expect((await send(port, '/a/../b')).status).toBe(400) }) + + test('a refusal leaves a warn-level debug line', async () => { + const warn = spyOn(console, 'warn').mockImplementation(() => {}) + const error = spyOn(console, 'error').mockImplementation(() => {}) + process.env.SRT_DEBUG = '1' + try { + await send(hookPort.default, '/a/../b') + expect(warn.mock.calls.flat().join('\n')).toContain( + 'ambiguous request-target', + ) + } finally { + delete process.env.SRT_DEBUG + warn.mockRestore() + error.mockRestore() + } + }) + + test('a refusal is reported like a denial of the hook', async () => { + const port = await listen( + createHttpProxyServer({ + ...base, + filterRequest: async () => ({ action: 'allow' }), + onFilterRequestDenied: d => denied.push(d), + denyHeader: 'X-Deny-Reason', + }), + ) + denied.length = 0 + const reply = await send(port, '/a/../b?q=1') + expect(reply.status).toBe(400) + expect(reply.text).toMatch(/^x-deny-reason: bad_request\r$/im) + expect(denied).toHaveLength(1) + expect(denied[0]!.method).toBe('GET') + expect(denied[0]!.url).toMatch(/^https?:\/\/[^/]+\/a\/\.\.\/b\?q=1$/) + expect(denied[0]!.reason).toContain('ambiguous request-target') + }) + }) + + test('a refused upload does not keep its tunnel', async () => { + const tls = await tunnel(hookPort.default) + const closed = new Promise(r => tls.once('close', () => r())) + let reply = '' + tls.on('error', () => {}) + tls.on('data', chunk => (reply += chunk.toString('latin1'))) + // A body announced and never sent: nothing else ends this connection. + tls.write( + `PUT /a/../b HTTP/1.1\r\nHost: ${UPSTREAM_NAME}:${tlsPort}\r\n` + + 'Content-Length: 1000000\r\n\r\nstart', + ) + await closed + expect(reply).toStartWith('HTTP/1.1 400 ') + }, 3000) + + describe('with a hook: an absolute-form target', () => { + test('in a tunnel, its path and query are read like an origin-form target', async () => { + const abs = `https://${UPSTREAM_NAME}:${tlsPort}` + expect((await sendTls(hookPort.default, `${abs}/a//b?x`)).forwarded).toBe( + '/a//b?x', + ) + expect((await sendTls(hookPort.default, abs)).forwarded).toBe('/') + expect((await sendTls(hookPort.default, `${abs}?x`)).forwarded).toBe( + '/?x', + ) + expect((await sendTls(hookPort.default, `${abs}/a/../b`)).status).toBe( + 400, + ) + expect( + (await sendTls(hookPort.asSpelled, `${abs}//a/../b`)).forwarded, + ).toBe('//a/../b') + expect(hookSeen.at(-1)!.info?.normalizedRequestTarget).toBe('/b') + }) + }) + + // The service takes the path literally: `a/../b.txt` is a key, and the + // signature covers the path as it arrives. + describe('a request signed over its path', () => { + const REAL = { + accessKeyId: 'AKIAIOSFODNN7EXAMPLE', + secretAccessKey: 'wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY', + } + const SIGNED: Case[] = [ + ['//lead.txt', '/lead.txt'], + ['/a/../b.txt', '/b.txt'], + ['/a/..', '/'], + ['/bucket/../up.txt', '/up.txt'], + ] + const amz = { + 'x-amz-content-sha256': UNSIGNED_PAYLOAD, + 'x-amz-date': '20260101T000000Z', + } + const authorizationFor = (target: string, creds: typeof REAL): string => + signSigv4({ + method: 'GET', + requestTarget: target, + headers: amz, + hostHeader: `${UPSTREAM_NAME}:${tlsPort}`, + signedHeaders: ['host', ...Object.keys(amz)], + payloadHash: UNSIGNED_PAYLOAD, + amzDate: amz['x-amz-date'], + scope: { date: '20260101', region: 'us-east-1', service: 's3' }, + ...creds, + }).authorization + const sendSigned = (port: number, target: string, creds: typeof REAL) => + sendTls(port, target, [ + ...Object.entries(amz).map(([k, v]) => `${k}: ${v}`), + `Authorization: ${authorizationFor(target, creds)}`, + ]) + const receivedAuthorization = (): string | undefined => + /^authorization: (.*)$/im.exec(tlsHeads.at(-1)!)?.[1] + + describe('by the client', () => { + test.each(SIGNED)('asSpelled: %s still verifies', async sent => { + const reply = await sendSigned(hookPort.asSpelled, sent, REAL) + expect(reply.forwarded).toBe(sent) + expect(receivedAuthorization()).toBe(authorizationFor(sent, REAL)) + }) + + test.each(SIGNED)('default: %s is refused', async sent => { + expect((await sendSigned(hookPort.default, sent, REAL)).status).toBe( + 400, + ) + }) + + test.each(SIGNED)( + 'normalize: %s arrives as %s, which it is not signed for', + async (sent, normalized) => { + const reply = await sendSigned(hookPort.normalize, sent, REAL) + expect(reply.forwarded).toBe(normalized) + expect(receivedAuthorization()).not.toBe( + authorizationFor(normalized, REAL), + ) + }, + ) + }) + + describe('again by the proxy, for a masked credential', () => { + const sentinels = new SentinelRegistry() + const pairs = new AwsPairRegistry() + const eq = (h: string, p: string): boolean => h === p + const FAKE = { + accessKeyId: sentinels.register('AKID', REAL.accessKeyId, [ + UPSTREAM_NAME, + ]), + secretAccessKey: sentinels.register('SECRET', REAL.secretAccessKey, [ + UPSTREAM_NAME, + ]), + } + pairs.register({ + accessKeyIdSentinel: FAKE.accessKeyId, + realAccessKeyId: REAL.accessKeyId, + realSecretAccessKey: REAL.secretAccessKey, + injectHosts: [UPSTREAM_NAME], + }) + const resigning = ( + ambiguousRequestTarget: HttpProxyServerOptions['ambiguousRequestTarget'], + ): Promise => + listen( + createHttpProxyServer({ + ...base, + ambiguousRequestTarget, + filterRequest: async () => ({ action: 'allow' }), + mutateHeaders: (headers, host) => + sentinels.substituteInHeaders(headers, host, eq), + planSigv4: createSigv4Planner(pairs, undefined, eq), + }), + ) + + test.each(SIGNED)( + 'asSpelled: %s is signed for the path the client named', + async sent => { + const reply = await sendSigned( + await resigning('asSpelled'), + sent, + FAKE, + ) + expect(reply.forwarded).toBe(sent) + expect(receivedAuthorization()).toBe(authorizationFor(sent, REAL)) + }, + ) + + test.each(SIGNED)('default: %s is never signed', async sent => { + const reply = await sendSigned(await resigning(undefined), sent, FAKE) + expect(reply.status).toBe(400) + }) + + test.each(SIGNED)( + 'normalize: %s is signed for %s, the bytes forwarded', + async (sent, normalized) => { + const reply = await sendSigned( + await resigning('normalize'), + sent, + FAKE, + ) + expect(reply.forwarded).toBe(normalized) + expect(receivedAuthorization()).toBe( + authorizationFor(normalized, REAL), + ) + }, + ) + }) + }) + + // A runtime that takes such a byte decodes it, and would not send the + // client's bytes back out. + test.each(['default', 'asSpelled', 'normalize'] as const)( + 'with a hook, %s: a byte outside printable ASCII is never forwarded', + async mode => { + for (const target of ['/\xc3\xa9', '/a?q=\xe9']) { + const before = plainSeen.length + // Some runtimes' parsers refuse it themselves. + const reply = await sendPlain(hookPort[mode], target).catch(() => null) + expect([undefined, 400]).toContain(reply?.status) + expect(plainSeen.length).toBe(before) + } + }, + ) +}) + +describe('readRequestTarget', () => { + test.each(AMBIGUOUS)('%s is ambiguous', (raw, normalized) => { + const want = { spelled: raw, normalized, ambiguous: true } + expect(readRequestTarget(raw, 'GET')).toEqual(want) + expect(readRequestTarget(`http://h.test:8080${raw}`, 'GET')).toEqual(want) + }) + + test.each(UNAMBIGUOUS)('%s is not', raw => { + expect(readRequestTarget(raw, 'GET')).toMatchObject({ + spelled: raw, + ambiguous: false, + }) + }) + + test.each([ + ['*', '*'], + ['http://h.test', '/'], + ['HTTPS://u:p@h.test?x', '/?x'], + ['http://[::1]:80/a?', '/a?'], + ])('%s is spelled %s', (raw, spelled) => { + expect(readRequestTarget(raw, 'OPTIONS')).toMatchObject({ + spelled, + ambiguous: false, + }) + }) + + // Only an escape the parser ADDS is the same resource: `%2F` is not `/`. + test('an escape is not its character', () => { + expect(readRequestTarget('/a%7Bb', 'GET')?.normalized).toBe('/a%7Bb') + expect(readRequestTarget('/a%5Cb', 'GET')?.ambiguous).toBe(false) + expect(readRequestTarget('/a%5C../b', 'GET')?.spelled).toBe('/a%5C../b') + }) + + test.each([ + '*', + '*x', + 'h.test/x', + 'http://[::1/x', + 'ftp://h.test/x', + // The URL parser reads another authority, or another path, than the + // spelling has. + 'http:///h.test/x', + 'http:/h.test/x', + 'http://h.test\\x', + 'http://h.test\\@other.test/x', + 'http://h.test#x', + '/é', + '/a b', + '/a\tb', + '/a\x7f', + ])('%p has no spelling to forward', raw => { + expect(readRequestTarget(raw, 'GET')).toBeUndefined() + }) +}) + +describe('network.ambiguousRequestTarget', () => { + const seen: string[] = [] + const upstream = createServer(recordTargets(seen)) + let port: number + + beforeAll(async () => { + await new Promise(r => upstream.listen(0, '127.0.0.1', () => r())) + port = (upstream.address() as AddressInfo).port + }) + + afterAll(async () => { + await SandboxManager.reset() + await new Promise(r => upstream.close(() => r())) + }) + + test.each([ + [undefined, 400, undefined], + ['refuse', 400, undefined], + ['asSpelled', 200, '/a/../b'], + ['normalize', 200, '/b'], + ] as const)('%p: %d', async (ambiguousRequestTarget, status, forwarded) => { + await SandboxManager.reset() + await SandboxManager.initialize({ + network: { + allowedDomains: ['127.0.0.1'], + deniedDomains: [], + filterRequest: async () => ({ action: 'allow' }), + ambiguousRequestTarget, + }, + filesystem: { denyRead: [], allowWrite: [], denyWrite: [] }, + }) + const auth = Buffer.from(`srt:${SandboxManager.getProxyAuthToken()}`) + const socket = await new Promise((resolve, reject) => { + const s = connect(SandboxManager.getProxyPort()!, '127.0.0.1', () => + resolve(s), + ) + s.once('error', reject) + }) + const recorded = (): number => + SandboxManager.getSandboxViolationStore() + .getViolations() + .filter(v => v.line.includes('ambiguous request-target')).length + const before = recorded() + seen.length = 0 + const reply = await exchange( + socket, + `GET http://127.0.0.1:${port}/a/../b HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\n` + + `Proxy-Authorization: Basic ${auth.toString('base64')}\r\n\r\n`, + ) + expect(reply.status).toBe(status) + expect(seen).toEqual(forwarded === undefined ? [] : [forwarded]) + expect(recorded() - before).toBe(status === 400 ? 1 : 0) }) }) From a728e987ce5f862997c6cf0c79446a0f8d3f6c86 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Mon, 12 Oct 2026 16:26:15 +0000 Subject: [PATCH 2/5] The lint comment main needs The rule against direct realpath calls and the one call that is made on purpose met on main. --- src/sandbox/windows-sandbox-utils.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/sandbox/windows-sandbox-utils.ts b/src/sandbox/windows-sandbox-utils.ts index a170d0e9a..56a77854a 100644 --- a/src/sandbox/windows-sandbox-utils.ts +++ b/src/sandbox/windows-sandbox-utils.ts @@ -1774,6 +1774,9 @@ function windowsPathKey(p: string): string { let resolved = p if (!isUncPath(p)) { try { + // The final-path lookup itself, which also resolves 8.3 names and the + // case on disk. + // eslint-disable-next-line no-restricted-properties resolved = fs.realpathSync.native(p) } catch { // Not there, so nothing lies beneath it either. From 0f82db25769c9d556be1d4e6527b36029c9b6dd4 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 13 Oct 2026 02:55:17 +0000 Subject: [PATCH 3/5] A target outside ASCII goes percent-encoded, "#" is refused, and every refusal is recorded A plain-HTTP target with raw UTF-8 bytes, which only Bun's listener lets through, was answered 400. It is forwarded percent-encoded, as in 0.0.79. Bytes that are not UTF-8 are refused: the runtime has replaced them, so what the client named is lost. A raw "#" is refused in every mode: a request-target has no fragment, no client sends one, and upstreams differ in what they make of it. Every refusal of a target is recorded as a violation, and the line shows the URL as spelled, its query or fragment cut. --- README.md | 8 +- src/sandbox/http-proxy.ts | 33 +- src/sandbox/request-filter.ts | 134 ++++--- src/sandbox/sandbox-config.ts | 16 +- src/sandbox/sandbox-manager.ts | 26 +- src/sandbox/tls-terminate-proxy.ts | 42 +-- .../sandbox/request-target-forwarding.test.ts | 326 +++++++++++++++--- 7 files changed, 419 insertions(+), 166 deletions(-) diff --git a/README.md b/README.md index c99f424e9..6df1f046c 100644 --- a/README.md +++ b/README.md @@ -498,12 +498,14 @@ These two are read when the proxy starts; `updateConfig` does not change them. - `network.stripResponseHeaders` - Response headers never passed back to the sandboxed client, matched case-insensitively with `-`, `_` and `.` folded (e.g. `["set-cookie"]`). - `network.refuseOpaqueTunnels` - Refuse every tunnel `filterRequest` cannot see into: an HTTP CONNECT that would not be TLS-terminated or carries no TLS, and every SOCKS CONNECT. Requires `tlsTerminate` to serve HTTPS at all, and stops CONNECT-carried SSH (such as `GIT_SSH_COMMAND`) through the proxy. Refusals are recorded as violations. - `network.requireHostMatch` - Answer 421 to a request whose Host header, TLS server name or absolute-form authority does not name the host and port it is sent to, before `filterRequest` is asked. Refusals are recorded as violations. -- `network.ambiguousRequestTarget` - What becomes of a request whose target is ambiguous: normalised (dot segments and `%2e` resolved, `\` in the path read as `/`, a run of leading slashes collapsed to one, a `#` and what follows dropped) it names another path than as the client spelled it. Upstreams differ in which of the two they act on, so a policy that judged one can be got around through the other. +- `network.ambiguousRequestTarget` - What becomes of a request whose target is ambiguous: normalised (dot segments and `%2e` resolved, `\` in the path read as `/`, a run of leading slashes collapsed to one) it names another path than as the client spelled it. Upstreams differ in how they read such a spelling, so a policy that judged one reading can be got around through another. - `"refuse"` (default) - Answered 400 with `X-Proxy-Error: ambiguous-request-target` before `filterRequest` is asked. Nothing is forwarded, and the refusal is recorded as a violation. - - `"asSpelled"` - Forwarded as the client spelled it. The caller takes on that its `filterRequest` holds for both forms: `/public/../admin` is `/admin` to an upstream that resolves it, and `/admin/../public/x` is under `/admin/` to one that does not. Needs Node, or Bun 1.4 or later: an older Bun's HTTP client resolves dot segments on the way out. + - `"asSpelled"` - Forwarded as the client spelled it. The caller takes on that its `filterRequest` holds for every reading an upstream may make of the spelling: `/public/../admin` is `/admin` to one that resolves it, `/admin/../public/x` is under `/admin/` to one that does not, and `/a\b/../admin`, normalised `/a/admin`, is `/admin` to one that keeps the `\`. Needs Node, or Bun 1.4 or later: an older Bun's HTTP client resolves dot segments on the way out. - `"normalize"` - Forwarded normalised. A signature that covers the path no longer verifies (or, where the proxy re-signs for a masked AWS credential, is valid for the other path), and an upstream that takes the path literally, such as an object store where `a/../b` is a key, acts on another resource than the client named. -While `filterRequest` is set, on plain HTTP and inside a terminated tunnel, the target goes upstream byte for byte as the client spelled it (except under `"normalize"`), and the callback's second argument carries both forms, `requestTarget` as spelled and `normalizedRequestTarget`, and `requestTargetIsAmbiguous`: under `"asSpelled"` a callback can refuse an ambiguous target itself, for the hosts and with the status it chooses. `request.url` is a parsed URL, so, as in earlier releases, its path has dot segments already resolved and is not the spelling. Not ambiguous, and forwarded as spelled: `//` after the first segment, `%2F`, `;` parameters, dots in the query, a character the URL parser would only escape (`{`, `"`, `'`), and an empty query. A target that is not origin form, an absolute `http(s)` URI, or `*` for `OPTIONS`, or that holds a byte outside printable ASCII, is answered 400. +While `filterRequest` is set, on plain HTTP and inside a terminated tunnel, the target goes upstream byte for byte as the client spelled it (except under `"normalize"`), and the callback's second argument carries both forms, `requestTarget` as spelled and `normalizedRequestTarget`, and `requestTargetIsAmbiguous`: under `"asSpelled"` a callback can refuse an ambiguous target itself, for the hosts and with the status it chooses. `request.url` is a parsed URL, so, as in earlier releases, its path has dot segments already resolved and is not the spelling. Not ambiguous, and forwarded as spelled: `//` after the first segment, `%2F`, `;` parameters, dots in the query, a character the URL parser would only escape (`{`, `"`, `'`), and an empty query. A byte outside ASCII, which only Bun's listener for plain HTTP lets through, is percent-encoded in `requestTarget` and on the wire, as in earlier releases. + +Answered 400 in every mode, and recorded as a violation: a target that is not origin form, an absolute `http(s)` URI, or `*` for `OPTIONS`; one with a `#`, since a request target has no fragment and upstreams differ in what they make of one; and one with bytes that are not UTF-8, which the runtime has replaced by the time the proxy reads them. While `filterRequest` is set, `TRACE` and `TRACK` requests are answered 405 before it is asked, on plain HTTP and inside a terminated tunnel, because their response would echo a header the decision set back to the client. Node's and Bun 1.4's HTTP parsers already answer `TRACK` 400 before the proxy sees it; under Bun 1.3 the connection is closed without an answer. Refusals are recorded as violations. diff --git a/src/sandbox/http-proxy.ts b/src/sandbox/http-proxy.ts index 65d87df18..553506e9d 100644 --- a/src/sandbox/http-proxy.ts +++ b/src/sandbox/http-proxy.ts @@ -19,7 +19,7 @@ import { decideAndRespond, readRequestTarget, requestTargetAsSpelled, - respondAmbiguousTarget, + respondRefusedTarget, type AmbiguousRequestTarget, type DenyMark, applyHeaderEdits, @@ -311,7 +311,7 @@ export interface HttpProxyServerOptions { * ambiguous (see readRequestTarget), on both paths. `'refuse'`, the * default: 400 before filterRequest is asked, reported through * onFilterRequestDenied. `'asSpelled'`: forwarded as the client spelled - * it; filterRequest has to judge both forms. `'normalize'`: forwarded + * it; filterRequest has to hold for every reading. `'normalize'`: forwarded * normalized, which breaks a signature over the path and names another * resource to an upstream that takes the path literally. */ @@ -1057,39 +1057,32 @@ export function createHttpProxyServer(options: HttpProxyServerOptions): Server { // the target as the client spelled it, hook or no hook. const read = options.filterRequest && !options.requestTargetAsSpelled - ? readRequestTarget(req.url!, req.method) + ? readRequestTarget( + req.url!, + req.method, + options.ambiguousRequestTarget, + ) : undefined const requestTarget = options.requestTargetAsSpelled ? requestTargetAsSpelled(req.url!, { absolute: true }) - : options.filterRequest - ? options.ambiguousRequestTarget === 'normalize' - ? read?.normalized - : read?.spelled + : read + ? read.forwarded : `${url.pathname}${url.search}` if (requestTarget === undefined) { - respondDenied( + const reason = respondRefusedTarget( res, - 'malformed request-target', + read?.refusal, markFor('bad_request'), - 400, ) - return - } - const absUrl = `${url.protocol}//${authority}${requestTarget}` - if ( - read?.ambiguous && - options.ambiguousRequestTarget !== 'asSpelled' && - options.ambiguousRequestTarget !== 'normalize' - ) { - const reason = respondAmbiguousTarget(res, markFor('bad_request')) options.onFilterRequestDenied?.({ method: req.method ?? 'GET', - url: absUrl, + url: `${url.protocol}//${authority}${read?.spelled ?? ''}`, reason, encodedCommand: auth.encodedCommand, }) return } + const absUrl = `${url.protocol}//${authority}${requestTarget}` // Per-request filter applies to plain HTTP too — otherwise a sandboxed // client could bypass it by using http:// where the upstream serves it. diff --git a/src/sandbox/request-filter.ts b/src/sandbox/request-filter.ts index 22e9e2fea..8ac6ad11f 100644 --- a/src/sandbox/request-filter.ts +++ b/src/sandbox/request-filter.ts @@ -115,20 +115,20 @@ export type RequestInfo = { target?: { host: string; port: number } /** * The request-target in origin form (path and query) as the client - * spelled it. These are the bytes forwarded upstream, unless the proxy - * was created with `ambiguousRequestTarget: 'normalize'`. `request.url` - * is a parsed URL and cannot hold them: its path has dot segments - * resolved already. (With no filterRequest configured there is no hook - * to judge the target: plain HTTP forwards the URL-parsed path and query, - * and the TLS-terminated path forwards the client's bytes unchanged.) + * spelled it, a byte outside ASCII percent-encoded. These are the bytes + * forwarded upstream, unless the proxy was created with + * `ambiguousRequestTarget: 'normalize'`. `request.url` is a parsed URL + * and cannot hold them: its path has dot segments resolved already. (With + * no filterRequest configured there is no hook to judge the target: plain + * HTTP forwards the URL-parsed path and query, and the TLS-terminated + * path forwards the client's bytes unchanged.) */ requestTarget?: string /** - * The same target normalized (see normalizeRequestTarget): what an - * upstream that resolves the spelling acts on, and what is forwarded - * under `'normalize'`. Where it names another path than `requestTarget` - * (only under `'asSpelled'` and `'normalize'`), a path rule has to hold - * for both. + * The same target normalized (see normalizeRequestTarget): what is + * forwarded under `'normalize'`, and one of the readings an upstream may + * make of an ambiguous spelling. `/a\b/../c` is `/a/c` here, and `/c` to + * an upstream that keeps the `\`. */ normalizedRequestTarget?: string /** @@ -542,50 +542,106 @@ export function normalizeRequestTarget( /** What a proxy with a filterRequest does with an ambiguous request-target. */ export type AmbiguousRequestTarget = 'refuse' | 'asSpelled' | 'normalize' +const MALFORMED_TARGET = 'malformed request-target' +const AMBIGUOUS_TARGET = + 'ambiguous request-target: as spelled and normalized it names different ' + + 'paths (a dot segment, leading slashes or "\\"); nothing was forwarded ' + + '(network.ambiguousRequestTarget in the sandbox\'s configuration is "refuse")' + +const percentEncoded = (text: string): string => + Buffer.from(text).toString('hex').toUpperCase().replace(/../g, '%$&') + /** * Both forms of a request-target that a filterRequest judges: `spelled`, in * origin form as the client wrote it, and `normalized` (see * normalizeRequestTarget). It is `ambiguous` when the two can name different * resources, so that an upstream may act on another one than a policy - * judged: a dot segment (`%2e` included), a run of leading slashes, `\` in - * the path, or `#`. Not ambiguous, because every reader takes both forms for - * the same resource: a character the URL parser escapes (which ones differs - * by runtime), an empty query it drops, and the `/` an empty path becomes. - * Undefined for a shape normalizeRequestTarget refuses, an authority that - * ends elsewhere than the URL parser ends it, and a byte outside printable - * ASCII (a runtime that decodes those does not send them back unchanged). + * judged: a dot segment (`%2e` included), a run of leading slashes, or `\` + * in the path. Not ambiguous, because every reader takes both forms for the + * same resource: a character the URL parser escapes (which ones differs by + * runtime), an empty query it drops, and the `/` an empty path becomes. + * + * `forwarded` is the form `mode` sends. There is none, and `refusal` says + * why, for an ambiguous target unless `mode` picks a form, and in every mode + * for: + * - a shape normalizeRequestTarget refuses, an authority that ends elsewhere + * than the URL parser ends it, a space or a control character; + * - `#`: a request-target has no fragment, no client sends one, and + * upstreams differ in whether it ends the path or belongs to it; + * - bytes that are not UTF-8: a runtime that lets a byte outside ASCII + * through has decoded it, and has put U+FFFD for these, so what the client + * named is lost. + * + * Bytes that are UTF-8 are spelled percent-encoded, which names the same + * resource to every reader and is what an upstream's parser is sure to take. */ export function readRequestTarget( raw: string, method: string | undefined, -): { spelled: string; normalized: string; ambiguous: boolean } | undefined { - const normalized = normalizeRequestTarget(raw, method) - if (normalized === undefined) return undefined - let spelled = raw.replace(/^https?:\/\/[^/?#\\]+/i, '') + mode?: AmbiguousRequestTarget, +): { + spelled?: string + normalized?: string + ambiguous?: boolean + forwarded?: string + refusal?: string +} { + const ascii = raw.replace(/[^\0-\x7f]+/g, percentEncoded) + const normalized = normalizeRequestTarget(ascii, method) + let spelled = ascii.replace(/^https?:\/\/[^/?#\\]+/i, '') if (spelled === '' || spelled.startsWith('?')) spelled = '/' + spelled - if (!/^(?:\*|\/[!-~]*)$/.test(spelled)) return undefined + if (normalized === undefined || !/^(?:\*|\/[!-~]*)$/.test(spelled)) { + return { refusal: MALFORMED_TARGET } + } + if (/[\ufffd\ud800-\udfff]/u.test(raw)) { + return { + spelled, + refusal: `${MALFORMED_TARGET}: it holds bytes that are not UTF-8`, + } + } + if (spelled.includes('#')) { + return { + spelled, + refusal: `${MALFORMED_TARGET}: a request-target has no fragment ("#")`, + } + } const [a, b] = [spelled, normalized].map(t => - t.replace( - /["'<>^`{}]/g, - c => '%' + c.charCodeAt(0).toString(16).toUpperCase(), - ), + t.replace(/["'<>^`{}]/g, percentEncoded), ) - return { spelled, normalized, ambiguous: a !== b && a !== b + '?' } + const ambiguous = a !== b && a !== b + '?' + const forwarded = + mode === 'normalize' + ? normalized + : mode === 'asSpelled' || !ambiguous + ? spelled + : undefined + return { + spelled, + normalized, + ambiguous, + forwarded, + refusal: forwarded === undefined ? AMBIGUOUS_TARGET : undefined, + } } -const AMBIGUOUS_TARGET_REASON = - 'ambiguous request-target: as spelled and normalized it names different ' + - 'paths (a dot segment, leading slashes, "\\" or "#"), and ' + - 'network.ambiguousRequestTarget is "refuse"; nothing was forwarded' - -/** Refuse an ambiguous request-target: 400, and the reason for `onDeny`. */ -export function respondAmbiguousTarget( +/** + * Refuse a request-target: 400 and a warn-level line. Returns the reason, + * for the caller to report as it reports a denial. + */ +export function respondRefusedTarget( res: ServerResponse, - mark: DenyMark = 'ambiguous-request-target', + refusal = MALFORMED_TARGET, + mark?: DenyMark, ): string { - logForDebugging(`[proxy] ${AMBIGUOUS_TARGET_REASON}`, { level: 'warn' }) - respondDenied(res, AMBIGUOUS_TARGET_REASON, mark, 400) - return AMBIGUOUS_TARGET_REASON + logForDebugging(`[proxy] ${refusal}`, { level: 'warn' }) + respondDenied( + res, + refusal, + mark ?? + (refusal === AMBIGUOUS_TARGET ? 'ambiguous-request-target' : undefined), + 400, + ) + return refusal } /** diff --git a/src/sandbox/sandbox-config.ts b/src/sandbox/sandbox-config.ts index a026a1dd1..ceefba43a 100644 --- a/src/sandbox/sandbox-config.ts +++ b/src/sandbox/sandbox-config.ts @@ -825,14 +825,14 @@ export const NetworkConfigSchema = z.object({ .describe( 'While filterRequest is set: what becomes of a request whose target ' + 'names another path once normalized than as the client spelled it ' + - '(a dot segment, "%2e" included, leading slashes, "\\" or "#"), so ' + - 'that the upstream may act on another resource than the callback ' + - 'judged. "refuse" (default): 400 before filterRequest is asked, ' + - 'recorded as a violation. "asSpelled": forwarded as spelled; the ' + - 'callback has to judge both forms, which it gets as requestTarget ' + - 'and normalizedRequestTarget. "normalize": forwarded normalized, ' + - 'which breaks a signature over the path and names another resource ' + - 'to an upstream that takes the path literally.', + '(a dot segment, "%2e" included, leading slashes or "\\"), so that ' + + 'the upstream may act on another resource than the callback judged. ' + + '"refuse" (default): 400 before filterRequest is asked, recorded as ' + + 'a violation. "asSpelled": forwarded as spelled; the callback has to ' + + 'hold for every reading of it, and gets requestTarget, ' + + 'normalizedRequestTarget and requestTargetIsAmbiguous. "normalize": ' + + 'forwarded normalized, which breaks a signature over the path and ' + + 'names another resource to an upstream that takes the path literally.', ), tlsTerminate: z .object({ diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index 407d25635..796b3b520 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -434,25 +434,19 @@ const directLookup: DirectLookup = ) /** - * The request URL as it should appear in a model-visible violation line: - * origin + path, with any query string reduced to a `?…` marker (origin - * carries no userinfo, so an embedded `name:pass@` is dropped too). Query - * strings routinely carry credentials (api_key=, access_token=, signed - * URLs) that the sandboxed client interpolated at runtime and that were - * never in the model's context, so they must not enter the transcript via - * the block. The full URL still reaches the embedder's + * The request URL as it should appear in a model-visible violation line: as + * spelled (parsed, a refused `/a/../b` would read `/b`), with a query string + * or fragment reduced to a `?…` or `#…` marker. (The proxy rebuilds the + * authority, so an embedded `name:pass@` is gone already.) Query strings + * routinely carry credentials (api_key=, access_token=, signed URLs) that + * the sandboxed client interpolated at runtime and that were never in the + * model's context, so they must not enter the transcript via the + * block. The full URL still reaches the embedder's * filterRequest callback and its own debug logging. */ function redactUrlForViolation(url: string): string { - try { - const u = new URL(url) - return `${u.origin}${u.pathname}${u.search ? '?…' : ''}` - } catch { - // Not an absolute URL (shouldn't happen for proxy requests) — drop - // anything after '?' rather than risk leaking it. - const q = url.indexOf('?') - return q === -1 ? url : `${url.slice(0, q)}?…` - } + const cut = url.search(/[?#]/) + return cut === -1 ? url : `${url.slice(0, cut + 1)}…` } /** Whether an ask-callback answer is one of the two documented denials. */ diff --git a/src/sandbox/tls-terminate-proxy.ts b/src/sandbox/tls-terminate-proxy.ts index 774726d6a..34abfef3a 100644 --- a/src/sandbox/tls-terminate-proxy.ts +++ b/src/sandbox/tls-terminate-proxy.ts @@ -32,7 +32,7 @@ import { decideAndRespond, readRequestTarget, requestTargetAsSpelled, - respondAmbiguousTarget, + respondRefusedTarget, type AmbiguousRequestTarget, type DenyMark, applyHeaderEdits, @@ -878,19 +878,30 @@ async function forwardUpstream( target.denyHeader !== undefined ? { name: target.denyHeader, value: code } : undefined + // The tunnel target as it goes on the wire: filterRequest URL, Host, SigV4. + const authority = formatAuthority(target.hostname, target.port, 443) const read = filterRequest && !target.requestTargetAsSpelled - ? readRequestTarget(req.url ?? '/', req.method) + ? readRequestTarget( + req.url ?? '/', + req.method, + target.ambiguousRequestTarget, + ) : undefined const path = target.requestTargetAsSpelled ? requestTargetAsSpelled(req.url ?? '/') - : filterRequest - ? target.ambiguousRequestTarget === 'normalize' - ? read?.normalized - : read?.spelled + : read + ? read.forwarded : originFormPath(req.url) if (path === undefined) { - respondDenied(res, 'malformed request-target', markFor('bad_request'), 400) + const why = respondRefusedTarget(res, read?.refusal, markFor('bad_request')) + target.onFilterRequestDeny?.( + req.method ?? 'GET', + `https://${authority}${read?.spelled ?? ''}`, + why, + ) + // Like a denial: a body still to come must not keep the tunnel. + destroyAfterDenial(req, res) return } // RFC 9112 3.2: an HTTP/1.1 request without Host is answered 400. Not @@ -908,23 +919,6 @@ async function forwardUpstream( ) return } - // The tunnel target as it goes on the wire: filterRequest URL, Host, SigV4. - const authority = formatAuthority(target.hostname, target.port, 443) - if ( - read?.ambiguous && - target.ambiguousRequestTarget !== 'asSpelled' && - target.ambiguousRequestTarget !== 'normalize' - ) { - const why = respondAmbiguousTarget(res, markFor('bad_request')) - target.onFilterRequestDeny?.( - req.method ?? 'GET', - `https://${authority}${path}`, - why, - ) - // Like a denial: a body still to come must not keep the tunnel. - destroyAfterDenial(req, res) - return - } let body: Readable = req let decision: RequestDecision | undefined const sni = target.clientServerName ?? serverNameOf(req) diff --git a/test/sandbox/request-target-forwarding.test.ts b/test/sandbox/request-target-forwarding.test.ts index fd8a29d83..d07e5e0fb 100644 --- a/test/sandbox/request-target-forwarding.test.ts +++ b/test/sandbox/request-target-forwarding.test.ts @@ -76,8 +76,35 @@ const AMBIGUOUS: Case[] = [ ['///k?q=1', '/k?q=1'], ['/a\\b', '/a/b'], ['/\\k', '/k'], - ['/a#frag', '/a'], - ['/a?x#frag', '/a?x'], + // Two kinds at once. + ['//k/../b', '/b'], + ['//k\\b', '/k/b'], + ['/\\/k', '/k'], + ['/a\\..\\b', '/b'], + ['/a\\%2e%2e\\b', '/b'], + ['/a\\b/../c', '/a/c'], + // One kind, beside the escape of another. + ['/a%5cb/../c', '/c'], + ['//k%5cb', '/k%5cb'], + ['/a%23/../b', '/b'], + ['/a%23b\\c', '/a%23b/c'], + ['//k%23', '/k%23'], +] + +// A fragment, alone and with each ambiguous kind. +const FRAGMENT = [ + '/a#frag', + '/a?x#frag', + '/#', + '/a?#', + '/a#?x', + '/a/../b#x', + '/a/%2e%2e/b#x', + '//k#x', + '/a\\b#x', + '/a\\b/../c#x', + '/a#/../b', + '/a%23b#x', ] // Not ambiguous: normalizing leaves them alone, or only escapes a character @@ -99,6 +126,14 @@ const UNAMBIGUOUS = [ '/a?x=../y&z=//&w=\\', '/%61%7e', '/a%zz', + // The escape of `\\` or `#` is data. + '/a%5cb', + '/a%5c..%5cb', + '/a/..%5cb', + '/a%23b', + '/a/..%23', + '/a/%2e%2e%23', + '/a?x=%23', '/a?', '/a??', '/a{b}|c^d"eg`h', @@ -261,10 +296,14 @@ describe('request target forwarding', () => { } /** One absolute-form GET through the proxy, and the target upstream saw. */ - async function sendPlain(proxyPort: number, target: string): Promise { + async function sendPlain( + proxyPort: number, + target: string, + scheme = 'http://', + ): Promise { const authority = `127.0.0.1:${plainPort}` const before = plainSeen.length - const head = `GET http://${authority}${target} HTTP/1.1\r\nHost: ${authority}\r\n\r\n` + const head = `GET ${scheme}${authority}${target} HTTP/1.1\r\nHost: ${authority}\r\n\r\n` const socket = await new Promise((resolve, reject) => { const s = connect(proxyPort, '127.0.0.1', () => resolve(s)) s.once('error', reject) @@ -300,7 +339,9 @@ describe('request target forwarding', () => { expect(reply.text).toMatch( /^x-proxy-error: ambiguous-request-target\r$/im, ) - expect(reply.text).toContain('network.ambiguousRequestTarget') + expect(reply.text).toContain( + `(network.ambiguousRequestTarget in the sandbox's configuration is "refuse")`, + ) expect(reply.forwarded).toBeUndefined() expect(hookSeen.length).toBe(asked) }) @@ -392,41 +433,94 @@ describe('request target forwarding', () => { expect((await send(port, '/a/../b')).status).toBe(400) }) - test('a refusal leaves a warn-level debug line', async () => { - const warn = spyOn(console, 'warn').mockImplementation(() => {}) - const error = spyOn(console, 'error').mockImplementation(() => {}) - process.env.SRT_DEBUG = '1' - try { - await send(hookPort.default, '/a/../b') - expect(warn.mock.calls.flat().join('\n')).toContain( - 'ambiguous request-target', - ) - } finally { - delete process.env.SRT_DEBUG - warn.mockRestore() - error.mockRestore() + test('requestTargetAsSpelled: what it refuses is malformed, and reported', async () => { + const port = await listen( + createHttpProxyServer({ + ...base, + requestTargetAsSpelled: true, + filterRequest: async () => ({ action: 'allow' }), + onFilterRequestDenied: d => denied.push(d), + }), + ) + denied.length = 0 + expect((await send(port, '/a/../b')).forwarded).toBe('/a/../b') + expect((await send(port, '//k')).status).toBe(400) + expect(denied.map(d => d.reason)).toEqual(['malformed request-target']) + }) + + // No client sends one, and an upstream may end the path at it or take + // it into the path: there is no spelling to keep and nothing to drop. + test.each(FRAGMENT)('%s is refused in every mode', async sent => { + const asked = hookSeen.length + for (const mode of ['default', 'asSpelled', 'normalize'] as const) { + const reply = await send(hookPort[mode], sent) + expect(reply.status).toBe(400) + expect(reply.text).toContain('a request-target has no fragment') + expect(reply.text).not.toMatch(/ambiguous-request-target/i) } + expect(hookSeen.length).toBe(asked) }) - test('a refusal is reported like a denial of the hook', async () => { + test.each([ + ['/a/../b?q=1', '/a/../b?q=1', 'ambiguous request-target'], + ['/a?q=1#x', '/a?q=1#x', 'malformed request-target: a request-target'], + ])( + 'the refusal of %s is reported like a denial of the hook, and logged', + async (sent, shown, reason) => { + const port = await listen( + createHttpProxyServer({ + ...base, + filterRequest: async () => ({ action: 'allow' }), + onFilterRequestDenied: d => denied.push(d), + denyHeader: 'X-Deny-Reason', + }), + ) + denied.length = 0 + const warn = spyOn(console, 'warn').mockImplementation(() => {}) + const error = spyOn(console, 'error').mockImplementation(() => {}) + process.env.SRT_DEBUG = '1' + try { + const reply = await send(port, sent) + expect(reply.status).toBe(400) + expect(reply.text).toMatch(/^x-deny-reason: bad_request\r$/im) + expect(warn.mock.calls.flat().join('\n')).toContain(reason) + } finally { + delete process.env.SRT_DEBUG + warn.mockRestore() + error.mockRestore() + } + expect(denied).toHaveLength(1) + expect(denied[0]!.method).toBe('GET') + expect(denied[0]!.url.replace(/^https?:\/\/[^/]+/, '')).toBe(shown) + expect(denied[0]!.reason).toStartWith(reason) + }, + ) + }) + + test.each([ + ['TLS-terminated', 'https://upstream.test:'], + ['plain HTTP', 'http://127.0.0.1:'], + ])( + 'with a hook, %s: a target of no known shape is reported too', + async (leg, origin) => { const port = await listen( createHttpProxyServer({ ...base, filterRequest: async () => ({ action: 'allow' }), onFilterRequestDenied: d => denied.push(d), - denyHeader: 'X-Deny-Reason', }), ) denied.length = 0 - const reply = await send(port, '/a/../b?q=1') + const reply = + leg === 'plain HTTP' + ? await sendPlain(port, '/x', 'http:///') + : await sendTls(port, `http:///${UPSTREAM_NAME}/x`) expect(reply.status).toBe(400) - expect(reply.text).toMatch(/^x-deny-reason: bad_request\r$/im) expect(denied).toHaveLength(1) - expect(denied[0]!.method).toBe('GET') - expect(denied[0]!.url).toMatch(/^https?:\/\/[^/]+\/a\/\.\.\/b\?q=1$/) - expect(denied[0]!.reason).toContain('ambiguous request-target') - }) - }) + expect(denied[0]!.url.replace(/\d+$/, '')).toBe(origin) + expect(denied[0]!.reason).toBe('malformed request-target') + }, + ) test('a refused upload does not keep its tunnel', async () => { const tls = await tunnel(hookPort.default) @@ -592,33 +686,73 @@ describe('request target forwarding', () => { }) }) - // A runtime that takes such a byte decodes it, and would not send the - // client's bytes back out. - test.each(['default', 'asSpelled', 'normalize'] as const)( - 'with a hook, %s: a byte outside printable ASCII is never forwarded', - async mode => { - for (const target of ['/\xc3\xa9', '/a?q=\xe9']) { - const before = plainSeen.length - // Some runtimes' parsers refuse it themselves. - const reply = await sendPlain(hookPort[mode], target).catch(() => null) - expect([undefined, 400]).toContain(reply?.status) - expect(plainSeen.length).toBe(before) - } + // Bun's listener lets a byte outside ASCII through, decoded. (Node's + // parser, and Bun's for a tunnel's connection, refuse the request.) + describe.each(['default', 'asSpelled', 'normalize'] as const)( + 'with a hook, %s: a byte outside ASCII, plain HTTP', + mode => { + test.each([ + ['/caf\xc3\xa9?q=caf\xc3\xa9', '/caf%C3%A9?q=caf%C3%A9'], + ['/\xe2\x82\xac/\xf0\x9f\x98\x80', '/%E2%82%AC/%F0%9F%98%80'], + ['/\xc3\xa9%c3%a9', '/%C3%A9%c3%a9'], + ])('UTF-8 goes out percent-encoded: %p', async (sent, encoded) => { + expect((await sendPlain(hookPort[mode], sent)).forwarded).toBe(encoded) + expect(hookSeen.at(-1)!.info).toMatchObject({ + requestTarget: encoded, + normalizedRequestTarget: encoded, + requestTargetIsAmbiguous: false, + }) + }) + + // U+FFFD stands where they were: percent-encoding that would name + // another resource. + test.each(['/caf\xe9', '/a?q=\xe9', '/a\xc3', '/a\xc0\xafb'])( + 'bytes that are not UTF-8 are refused: %p', + async sent => { + const asked = hookSeen.length + const reply = await sendPlain(hookPort[mode], sent) + expect(reply.status).toBe(400) + expect(reply.text).toContain('bytes that are not UTF-8') + expect(hookSeen.length).toBe(asked) + }, + ) }, ) + + test('with a hook: a dot segment beside a byte outside ASCII is still ambiguous', async () => { + const reply = await sendPlain(hookPort.default, '/\xc3\xa9/../b') + expect(reply.text).toMatch(/^x-proxy-error: ambiguous-request-target\r$/im) + }) }) describe('readRequestTarget', () => { test.each(AMBIGUOUS)('%s is ambiguous', (raw, normalized) => { - const want = { spelled: raw, normalized, ambiguous: true } - expect(readRequestTarget(raw, 'GET')).toEqual(want) - expect(readRequestTarget(`http://h.test:8080${raw}`, 'GET')).toEqual(want) + const forms = { spelled: raw, normalized, ambiguous: true } + for (const r of [raw, `http://h.test:8080${raw}`]) { + const refused = readRequestTarget(r, 'GET') + expect(refused).toMatchObject(forms) + expect(refused.forwarded).toBeUndefined() + expect(refused.refusal).toStartWith('ambiguous request-target') + expect(readRequestTarget(r, 'GET', 'refuse')).toEqual(refused) + expect(readRequestTarget(r, 'GET', 'asSpelled')).toEqual({ + ...forms, + forwarded: raw, + refusal: undefined, + }) + expect(readRequestTarget(r, 'GET', 'normalize')).toEqual({ + ...forms, + forwarded: normalized, + refusal: undefined, + }) + } }) test.each(UNAMBIGUOUS)('%s is not', raw => { expect(readRequestTarget(raw, 'GET')).toMatchObject({ spelled: raw, ambiguous: false, + forwarded: raw, + refusal: undefined, }) }) @@ -627,18 +761,55 @@ describe('readRequestTarget', () => { ['http://h.test', '/'], ['HTTPS://u:p@h.test?x', '/?x'], ['http://[::1]:80/a?', '/a?'], + ['/caf\u00e9?q=\u20ac', '/caf%C3%A9?q=%E2%82%AC'], + ['http://caf\u00e9.test/\u{1f600}', '/%F0%9F%98%80'], ])('%s is spelled %s', (raw, spelled) => { expect(readRequestTarget(raw, 'OPTIONS')).toMatchObject({ spelled, ambiguous: false, + forwarded: spelled, }) }) // Only an escape the parser ADDS is the same resource: `%2F` is not `/`. test('an escape is not its character', () => { - expect(readRequestTarget('/a%7Bb', 'GET')?.normalized).toBe('/a%7Bb') - expect(readRequestTarget('/a%5Cb', 'GET')?.ambiguous).toBe(false) - expect(readRequestTarget('/a%5C../b', 'GET')?.spelled).toBe('/a%5C../b') + expect(readRequestTarget('/a%7Bb', 'GET').normalized).toBe('/a%7Bb') + expect(readRequestTarget('/a%5Cb', 'GET').ambiguous).toBe(false) + expect(readRequestTarget('/a%5C../b', 'GET').spelled).toBe('/a%5C../b') + }) + + test('a character that only looks like a dot or a slash is data', () => { + expect(readRequestTarget('/a/\uff0e\uff0e/b\uff0fc', 'GET')).toMatchObject({ + spelled: '/a/%EF%BC%8E%EF%BC%8E/b%EF%BC%8Fc', + ambiguous: false, + }) + }) + + const MODES = [undefined, 'refuse', 'asSpelled', 'normalize'] as const + + test.each(FRAGMENT)('%s is refused for its fragment', raw => { + for (const mode of MODES) { + expect(readRequestTarget(raw, 'GET', mode)).toEqual({ + spelled: raw, + refusal: + 'malformed request-target: a request-target has no fragment ("#")', + }) + } + }) + + test.each([ + ['/caf\ufffd', '/caf%EF%BF%BD'], + ['/a?q=\ufffd', '/a?q=%EF%BF%BD'], + ['/\ud800', '/%EF%BF%BD'], + ['/\udc00x', '/%EF%BF%BDx'], + ['/\ufffd#x', '/%EF%BF%BD#x'], + ])('%p is refused for what the runtime replaced', (raw, spelled) => { + for (const mode of MODES) { + expect(readRequestTarget(raw, 'GET', mode)).toEqual({ + spelled, + refusal: 'malformed request-target: it holds bytes that are not UTF-8', + }) + } }) test.each([ @@ -654,12 +825,15 @@ describe('readRequestTarget', () => { 'http://h.test\\x', 'http://h.test\\@other.test/x', 'http://h.test#x', - '/é', '/a b', '/a\tb', '/a\x7f', ])('%p has no spelling to forward', raw => { - expect(readRequestTarget(raw, 'GET')).toBeUndefined() + for (const mode of MODES) { + expect(readRequestTarget(raw, 'GET', mode)).toEqual({ + refusal: 'malformed request-target', + }) + } }) }) @@ -681,8 +855,8 @@ describe('network.ambiguousRequestTarget', () => { test.each([ [undefined, 400, undefined], ['refuse', 400, undefined], - ['asSpelled', 200, '/a/../b'], - ['normalize', 200, '/b'], + ['asSpelled', 200, '/a/../b?k=SECRET'], + ['normalize', 200, '/b?k=SECRET'], ] as const)('%p: %d', async (ambiguousRequestTarget, status, forwarded) => { await SandboxManager.reset() await SandboxManager.initialize({ @@ -701,19 +875,59 @@ describe('network.ambiguousRequestTarget', () => { ) s.once('error', reject) }) - const recorded = (): number => - SandboxManager.getSandboxViolationStore() - .getViolations() - .filter(v => v.line.includes('ambiguous request-target')).length - const before = recorded() + const store = SandboxManager.getSandboxViolationStore() + store.clear() seen.length = 0 const reply = await exchange( socket, - `GET http://127.0.0.1:${port}/a/../b HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\n` + + `GET http://u:p@127.0.0.1:${port}/a/../b?k=SECRET HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\n` + `Proxy-Authorization: Basic ${auth.toString('base64')}\r\n\r\n`, ) expect(reply.status).toBe(status) expect(seen).toEqual(forwarded === undefined ? [] : [forwarded]) - expect(recorded() - before).toBe(status === 400 ? 1 : 0) + // The line names what was refused, not what it would resolve to. + expect(store.getViolations().map(v => v.line.replace(/ \(.*/, ''))).toEqual( + status === 400 + ? [`deny http-request GET http://127.0.0.1:${port}/a/../b?…`] + : [], + ) + }) + + test.each([ + ['/a#t=SECRET', '/a#…', 'a request-target has no fragment'], + ['/a?k=SECRET#x', '/a?…', 'a request-target has no fragment'], + ['/caf\xe9', '/caf%EF%BF%BD', 'bytes that are not UTF-8'], + ])('%p is recorded as %s', async (sent, shown, reason) => { + await SandboxManager.reset() + await SandboxManager.initialize({ + network: { + allowedDomains: ['127.0.0.1'], + deniedDomains: [], + filterRequest: async () => ({ action: 'allow' }), + ambiguousRequestTarget: 'asSpelled', + }, + filesystem: { denyRead: [], allowWrite: [], denyWrite: [] }, + }) + const auth = Buffer.from(`srt:${SandboxManager.getProxyAuthToken()}`) + const socket = await new Promise((resolve, reject) => { + const s = connect(SandboxManager.getProxyPort()!, '127.0.0.1', () => + resolve(s), + ) + s.once('error', reject) + }) + const store = SandboxManager.getSandboxViolationStore() + store.clear() + const reply = await exchange( + socket, + `GET http://127.0.0.1:${port}${sent} HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\n` + + `Proxy-Authorization: Basic ${auth.toString('base64')}\r\n\r\n`, + ) + expect(reply.status).toBe(400) + const lines = store.getViolations().map(v => v.line) + expect(lines).toHaveLength(1) + expect(lines[0]).toStartWith( + `deny http-request GET http://127.0.0.1:${port}${shown} (malformed request-target: `, + ) + expect(lines[0]).toContain(reason) }) }) From 2de68a685eba430ff1faf01ac78ecb8092f4609e Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 13 Oct 2026 11:11:43 +0000 Subject: [PATCH 4/5] A fragment is cut off, and a request the parser refuses in a tunnel is answered and recorded A raw "#" in a request-target was answered 400. Python's urllib, Perl's LWP and Java's HttpClient leave the fragment in the absolute form they send a proxy, and 0.0.79 dropped it there. It is cut before anything is read, judged, signed, reported or sent, in every mode and on both legs. A fragment right after the authority leaves the root. In a terminated tunnel, a request the runtime's HTTP parser refuses (raw bytes outside ASCII in the target, as curl sends a query) closed the connection with no answer and no record. It is answered 400 with what to do, and recorded. Nothing is written while an earlier response is under way. --- README.md | 4 +- src/sandbox/request-filter.ts | 124 +++- src/sandbox/tls-terminate-proxy.ts | 43 +- .../sandbox/request-target-forwarding.test.ts | 573 ++++++++++++++++-- 4 files changed, 666 insertions(+), 78 deletions(-) diff --git a/README.md b/README.md index 6df1f046c..7ae6c431f 100644 --- a/README.md +++ b/README.md @@ -503,9 +503,9 @@ These two are read when the proxy starts; `updateConfig` does not change them. - `"asSpelled"` - Forwarded as the client spelled it. The caller takes on that its `filterRequest` holds for every reading an upstream may make of the spelling: `/public/../admin` is `/admin` to one that resolves it, `/admin/../public/x` is under `/admin/` to one that does not, and `/a\b/../admin`, normalised `/a/admin`, is `/admin` to one that keeps the `\`. Needs Node, or Bun 1.4 or later: an older Bun's HTTP client resolves dot segments on the way out. - `"normalize"` - Forwarded normalised. A signature that covers the path no longer verifies (or, where the proxy re-signs for a masked AWS credential, is valid for the other path), and an upstream that takes the path literally, such as an object store where `a/../b` is a key, acts on another resource than the client named. -While `filterRequest` is set, on plain HTTP and inside a terminated tunnel, the target goes upstream byte for byte as the client spelled it (except under `"normalize"`), and the callback's second argument carries both forms, `requestTarget` as spelled and `normalizedRequestTarget`, and `requestTargetIsAmbiguous`: under `"asSpelled"` a callback can refuse an ambiguous target itself, for the hosts and with the status it chooses. `request.url` is a parsed URL, so, as in earlier releases, its path has dot segments already resolved and is not the spelling. Not ambiguous, and forwarded as spelled: `//` after the first segment, `%2F`, `;` parameters, dots in the query, a character the URL parser would only escape (`{`, `"`, `'`), and an empty query. A byte outside ASCII, which only Bun's listener for plain HTTP lets through, is percent-encoded in `requestTarget` and on the wire, as in earlier releases. +While `filterRequest` is set, on plain HTTP and inside a terminated tunnel, the target goes upstream byte for byte as the client spelled it, without a fragment (except under `"normalize"`), and the callback's second argument carries both forms, `requestTarget` as spelled and `normalizedRequestTarget`, and `requestTargetIsAmbiguous`: under `"asSpelled"` a callback can refuse an ambiguous target itself, for the hosts and with the status it chooses. `request.url` is a parsed URL, so, as in earlier releases, its path has dot segments already resolved and is not the spelling. Not ambiguous, and forwarded as spelled: `//` after the first segment, `%2F`, `;` parameters, dots in the query, a character the URL parser would only escape (`{`, `"`, `'`), and an empty query. A byte outside ASCII, which only Bun's listener for plain HTTP lets through, is percent-encoded in `requestTarget` and on the wire, as in earlier releases. A `#` and what follows it are cut off first, in every mode: a fragment is no part of a request target, but some clients leave it in the absolute form they send a proxy, and upstreams differ in what they make of one. So the callback, a signature, the upstream and a violation line all get the target without it; `%23` is data and stays. -Answered 400 in every mode, and recorded as a violation: a target that is not origin form, an absolute `http(s)` URI, or `*` for `OPTIONS`; one with a `#`, since a request target has no fragment and upstreams differ in what they make of one; and one with bytes that are not UTF-8, which the runtime has replaced by the time the proxy reads them. +Answered 400 in every mode, and recorded as a violation: a target that is not origin form, an absolute `http(s)` URI, or `*` for `OPTIONS`; and one with bytes that are not UTF-8, which the runtime has replaced by the time the proxy reads them. So is, inside a terminated tunnel and with or without `filterRequest`, a request the runtime's own HTTP parser refuses, which ends the tunnel: a raw byte outside ASCII in the target (`/café` has to be sent as `/caf%C3%A9`), a space, a malformed header. While `filterRequest` is set, `TRACE` and `TRACK` requests are answered 405 before it is asked, on plain HTTP and inside a terminated tunnel, because their response would echo a header the decision set back to the client. Node's and Bun 1.4's HTTP parsers already answer `TRACK` 400 before the proxy sees it; under Bun 1.3 the connection is closed without an answer. Refusals are recorded as violations. diff --git a/src/sandbox/request-filter.ts b/src/sandbox/request-filter.ts index 69d4b726b..0e0c1cbb7 100644 --- a/src/sandbox/request-filter.ts +++ b/src/sandbox/request-filter.ts @@ -9,13 +9,14 @@ */ import { endRequestAfterResponse } from './emitted-connection.js' -import type { - IncomingHttpHeaders, - IncomingMessage, - ServerResponse, +import { + METHODS, + type IncomingHttpHeaders, + type IncomingMessage, + type ServerResponse, } from 'node:http' import { isIP } from 'node:net' -import { PassThrough, Readable } from 'node:stream' +import { type Duplex, PassThrough, Readable } from 'node:stream' import { logForDebugging } from '../utils/debug.js' import { isResolvedAddressDenied } from './resolved-address-guard.js' import { sanitizeDenialReason } from './sandbox-violation-store.js' @@ -115,8 +116,8 @@ export type RequestInfo = { target?: { host: string; port: number } /** * The request-target in origin form (path and query) as the client - * spelled it, a byte outside ASCII percent-encoded. These are the bytes - * forwarded upstream, unless the proxy was created with + * spelled it, without a fragment, a byte outside ASCII percent-encoded. + * These are the bytes forwarded upstream, unless the proxy was created with * `ambiguousRequestTarget: 'normalize'`. `request.url` is a parsed URL * and cannot hold them: its path has dot segments resolved already. (With * no filterRequest configured there is no hook to judge the target: plain @@ -566,14 +567,18 @@ const percentEncoded = (text: string): string => * for: * - a shape normalizeRequestTarget refuses, an authority that ends elsewhere * than the URL parser ends it, a space or a control character; - * - `#`: a request-target has no fragment, no client sends one, and - * upstreams differ in whether it ends the path or belongs to it; * - bytes that are not UTF-8: a runtime that lets a byte outside ASCII * through has decoded it, and has put U+FFFD for these, so what the client - * named is lost. + * named is lost. (A U+FFFD of the client's own looks the same.) * * Bytes that are UTF-8 are spelled percent-encoded, which names the same * resource to every reader and is what an upstream's parser is sure to take. + * + * A `#` and what follows it are in neither form. A fragment is the client's + * own and no part of a request-target, but some clients leave it in the + * absolute form they send a proxy; upstreams differ in whether it ends the + * path or belongs to it, so it is cut before anything is read, judged, + * signed, reported or sent. */ export function readRequestTarget( raw: string, @@ -586,23 +591,18 @@ export function readRequestTarget( forwarded?: string refusal?: string } { - const ascii = raw.replace(/[^\0-\x7f]+/g, percentEncoded) + const target = raw.replace(/#.*/s, '') + const ascii = target.replace(/[^\0-\x7f]+/g, percentEncoded) const normalized = normalizeRequestTarget(ascii, method) let spelled = ascii.replace(/^https?:\/\/[^/?#\\]+/i, '') if (spelled === '' || spelled.startsWith('?')) spelled = '/' + spelled if (normalized === undefined || !/^(?:\*|\/[!-~]*)$/.test(spelled)) { return { refusal: MALFORMED_TARGET } } - if (/[\ufffd\ud800-\udfff]/u.test(raw)) { - return { - spelled, - refusal: `${MALFORMED_TARGET}: it holds bytes that are not UTF-8`, - } - } - if (spelled.includes('#')) { + if (/[\ufffd\ud800-\udfff]/u.test(target)) { return { spelled, - refusal: `${MALFORMED_TARGET}: a request-target has no fragment ("#")`, + refusal: `${MALFORMED_TARGET}: it holds bytes that are not UTF-8, or U+FFFD`, } } const [a, b] = [spelled, normalized].map(t => @@ -644,6 +644,92 @@ export function respondRefusedTarget( return refusal } +/** How many bytes of a request-target the runtime's parser refused are reported. */ +const MAX_REPORTED_TARGET_LENGTH = 200 + +/** + * Handle a server's 'clientError'. A request the runtime's own parser gave + * up on, which no request handler gets to see, is refused: 400 with the + * reason on the bare socket, which ends, and a warn-level line. Returns what + * the caller is to report of it, as it reports a denial. Any other error (a + * reset, a client that left) destroys the socket, with nothing to report. + * + * Nothing is written, and the socket is destroyed, where `mayAnswer` is + * false (the caller's: a response to an earlier request is under way, and + * these bytes would land inside it) or the socket takes no write. On a + * socket that was ended nothing is done: bytes that follow a refused head + * fail the parser again while the answer is on its way out, and that is one + * refusal. + * + * The one thing read of the client's bytes is the request line, and only + * where the parser stopped inside its target and the line is whole: the + * method if the runtime knows it, and an origin-form target, cut, with + * every byte outside printable ASCII percent-encoded. + */ +export function refuseUnparsedRequest( + err: Error, + socket: Duplex, + mayAnswer: boolean, + mark: DenyMark = DEFAULT_DENY_TAG, +): { method: string; target: string; reason: string } | undefined { + const { code, rawPacket, bytesParsed } = err as { + code?: unknown + rawPacket?: unknown + bytesParsed?: unknown + } + if ( + typeof code !== 'string' || + !code.startsWith('HPE_') || + // The input ended in the middle of a message: the client left. + code === 'HPE_INVALID_EOF_STATE' + ) { + socket.destroy() + return undefined + } + if (socket.writableEnded) return undefined + const refused = { + method: '-', + target: '', + reason: `malformed request (${err.message})`, + } + if (code === 'HPE_INVALID_URL') { + refused.reason = `${MALFORMED_TARGET}: percent-encode its bytes outside printable ASCII` + if (Buffer.isBuffer(rawPacket) && typeof bytesParsed === 'number') { + const text = rawPacket.toString('latin1') + const line = /([A-Z-]+) (\/[^ \r\n]*) HTTP\/1\.[01]\r?\n/y + line.lastIndex = text.slice(0, bytesParsed).lastIndexOf('\n') + 1 + const [, method, target] = line.exec(text) ?? [] + if (method !== undefined && METHODS.includes(method)) { + refused.method = method + refused.target = target! + .slice(0, MAX_REPORTED_TARGET_LENGTH) + .replace( + /[^!-~]/g, + c => + `%${c.charCodeAt(0).toString(16).toUpperCase().padStart(2, '0')}`, + ) + } + } + } + logForDebugging(`[proxy] ${refused.reason}`, { level: 'warn' }) + if (!mayAnswer || !socket.writable) { + socket.destroy() + return refused + } + // The peer may reset while the answer flushes. + socket.on('error', () => {}) + const [name, value] = denyMarkHeader(mark) + socket.end( + '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. + setTimeout(() => socket.destroy(), 1000).unref?.() + return refused +} + /** * A request-target as the client spelled it, for a proxy that forwards it * unchanged and lets its policy match it exactly: dot segments and diff --git a/src/sandbox/tls-terminate-proxy.ts b/src/sandbox/tls-terminate-proxy.ts index 0017ec7dd..671f66877 100644 --- a/src/sandbox/tls-terminate-proxy.ts +++ b/src/sandbox/tls-terminate-proxy.ts @@ -32,6 +32,7 @@ import type { MitmCA } from './mitm-ca.js' import { decideAndRespond, readRequestTarget, + refuseUnparsedRequest, requestTargetAsSpelled, respondRefusedTarget, type AmbiguousRequestTarget, @@ -403,6 +404,12 @@ export function terminateAndForward( maxSigv4BodyBytes, ) }) + // Responses under way on the tunnel's connection. + let answering = 0 + inner.on('request', (_req, res) => { + answering++ + res.once('close', () => answering--) + }) inner.on('clientError', (err, sock) => { // Parse errors and resets on the tunnel would otherwise escalate to an // uncaughtException, same class as the request-stream errors handled @@ -411,26 +418,22 @@ export function terminateAndForward( `[tls-terminate] client connection error for ${target.hostname}: ${err.message}`, { level: 'error' }, ) - if ( - target.maxHeaderSize !== undefined && - (err as NodeJS.ErrnoException).code === 'HPE_HEADER_OVERFLOW' && - sock.writable - ) { - // A head past the caller's own limit is a request past its limits: - // 400, flushed before the socket goes. - sock.on('error', () => {}) - const mark = - target.denyHeader !== undefined - ? `${target.denyHeader}: bad_request\r\n` - : '' - sock.end( - `HTTP/1.1 400 Bad Request\r\n${mark}Content-Length: 0\r\nConnection: close\r\n\r\n`, - () => sock.destroy(), - ) - setTimeout(() => sock.destroy(), 1000).unref?.() - return - } - sock.destroy() + // A request the parser refuses is refused like any other: answered, and + // reported under the tunnel's own target. + const refused = refuseUnparsedRequest( + err, + sock, + answering === 0, + target.denyHeader !== undefined + ? { name: target.denyHeader, value: 'bad_request' } + : undefined, + ) + if (refused === undefined) return + target.onFilterRequestDeny?.( + refused.method, + `https://${formatAuthority(target.hostname, target.port, 443)}${refused.target}`, + refused.reason, + ) }) inner.on('upgrade', (_req, sock) => { // WebSocket / non-HTTP over TLS — out of scope for now. diff --git a/test/sandbox/request-target-forwarding.test.ts b/test/sandbox/request-target-forwarding.test.ts index d07e5e0fb..63d57d591 100644 --- a/test/sandbox/request-target-forwarding.test.ts +++ b/test/sandbox/request-target-forwarding.test.ts @@ -10,6 +10,7 @@ import { } from 'node:tls' import { readFileSync } from 'node:fs' import { join } from 'node:path' +import { PassThrough } from 'node:stream' import { createHttpProxyServer, type HttpProxyServerOptions, @@ -18,6 +19,7 @@ import { createMitmCA } from '../../src/sandbox/mitm-ca.js' import { mintLeafCert } from '../../src/sandbox/mitm-leaf.js' import { readRequestTarget, + refuseUnparsedRequest, type RequestInfo, } from '../../src/sandbox/request-filter.js' import { SentinelRegistry } from '../../src/sandbox/credential-sentinel.js' @@ -91,22 +93,30 @@ const AMBIGUOUS: Case[] = [ ['//k%23', '/k%23'], ] -// A fragment, alone and with each ambiguous kind. -const FRAGMENT = [ - '/a#frag', - '/a?x#frag', - '/#', - '/a?#', - '/a#?x', - '/a/../b#x', - '/a/%2e%2e/b#x', - '//k#x', - '/a\\b#x', - '/a\\b/../c#x', - '/a#/../b', - '/a%23b#x', +// A fragment is cut off before anything is read: as sent, and what is left. +const FRAGMENT: Case[] = [ + ['/p#f', '/p'], + ['/p#', '/p'], + ['/p#f?q', '/p'], + ['/p?q#f', '/p?q'], + ['/p?#f', '/p?'], + ['/#f', '/'], + ['/p#f#g', '/p'], + ['/p%23a#f', '/p%23a'], + // What would be ambiguous in the path is not, in the fragment. + ['/p#/../x', '/p'], + ['/p#\\x', '/p'], ] +// What is left is ambiguous as it would be without one: and normalized. +const FRAGMENT_AMBIGUOUS = [ + ['/a/../b#x', '/a/../b', '/b'], + ['/a/%2e%2e/b#x', '/a/%2e%2e/b', '/b'], + ['//k#x', '//k', '/k'], + ['/a\\b#x', '/a\\b', '/a/b'], + ['/a\\b/../c?q#x', '/a\\b/../c?q', '/a/c?q'], +] as const + // Not ambiguous: normalizing leaves them alone, or only escapes a character // or drops an empty query. const UNAMBIGUOUS = [ @@ -448,22 +458,44 @@ describe('request target forwarding', () => { expect(denied.map(d => d.reason)).toEqual(['malformed request-target']) }) - // No client sends one, and an upstream may end the path at it or take - // it into the path: there is no spelling to keep and nothing to drop. - test.each(FRAGMENT)('%s is refused in every mode', async sent => { - const asked = hookSeen.length + // Some clients leave it in the absolute form they send a proxy. An + // upstream may end the path at it or take it into the path, so no mode + // sends it, and the hook judges what is sent. + test.each(FRAGMENT)('%s goes out as %s', async (sent, left) => { for (const mode of ['default', 'asSpelled', 'normalize'] as const) { - const reply = await send(hookPort[mode], sent) - expect(reply.status).toBe(400) - expect(reply.text).toContain('a request-target has no fragment') - expect(reply.text).not.toMatch(/ambiguous-request-target/i) + const { forwarded } = await send(hookPort[mode], sent) + const { url, info } = hookSeen.at(-1)! + expect(forwarded).toBe( + mode === 'normalize' ? info?.normalizedRequestTarget : left, + ) + expect(info?.requestTarget).toBe(left) + expect(info?.requestTargetIsAmbiguous).toBe(false) + expect(url).not.toContain('#') } - expect(hookSeen.length).toBe(asked) }) + test.each(FRAGMENT_AMBIGUOUS)( + '%s is judged as %s', + async (sent, left, normalized) => { + const refused = await send(hookPort.default, sent) + expect(refused.text).toMatch( + /^x-proxy-error: ambiguous-request-target\r$/im, + ) + expect((await send(hookPort.asSpelled, sent)).forwarded).toBe(left) + expect(hookSeen.at(-1)!.info).toMatchObject({ + requestTarget: left, + normalizedRequestTarget: normalized, + requestTargetIsAmbiguous: true, + }) + expect((await send(hookPort.normalize, sent)).forwarded).toBe( + normalized, + ) + }, + ) + test.each([ ['/a/../b?q=1', '/a/../b?q=1', 'ambiguous request-target'], - ['/a?q=1#x', '/a?q=1#x', 'malformed request-target: a request-target'], + ['/a/../b?q=1#x', '/a/../b?q=1', 'ambiguous request-target'], ])( 'the refusal of %s is reported like a denial of the hook, and logged', async (sent, shown, reason) => { @@ -516,12 +548,227 @@ describe('request target forwarding', () => { ? await sendPlain(port, '/x', 'http:///') : await sendTls(port, `http:///${UPSTREAM_NAME}/x`) expect(reply.status).toBe(400) + expect(reply.text).toMatch( + /^x-proxy-error: blocked-by-sandbox-runtime\r$/im, + ) expect(denied).toHaveLength(1) expect(denied[0]!.url.replace(/\d+$/, '')).toBe(origin) expect(denied[0]!.reason).toBe('malformed request-target') }, ) + test.each(['default', 'asSpelled', 'normalize'] as const)( + 'with a hook, %s: a fragment right after the authority leaves the root', + async mode => { + for (const rest of ['#x', '#x/y', '#', '#@other.test/x']) { + const reply = await sendPlain(hookPort[mode], rest) + expect([rest, reply.status, reply.forwarded]).toEqual([rest, 200, '/']) + } + }, + ) + + // No request handler sees these: the runtime's HTTP parser gives up on + // them. Left to itself the tunnel would close without a word. + describe('a request the parser refuses, in a tunnel', () => { + const host = (): string => `Host: ${UPSTREAM_NAME}:${tlsPort}\r\n` + const ADVICE = + 'malformed request-target: percent-encode its bytes outside printable ASCII' + + /** Writes the chunks on a fresh tunnel; all it answers, once it closes. */ + async function untilClosed( + proxyPort: number, + ...chunks: string[] + ): Promise { + const tls = await tunnel(proxyPort) + const closed = new Promise(r => tls.once('close', () => r())) + let reply = '' + tls.on('error', () => {}) + tls.on('data', chunk => (reply += chunk.toString('latin1'))) + for (const chunk of chunks) { + tls.write(Buffer.from(chunk, 'latin1')) + await new Promise(r => setTimeout(r, 50)) + } + await closed + return reply + } + + const reporting = ( + extra: Partial = {}, + ): Promise => { + denied.length = 0 + return listen( + createHttpProxyServer({ + ...base, + filterRequest: async (request, info) => { + hookSeen.push({ url: request.url, info }) + return { action: 'allow' } + }, + onFilterRequestDenied: d => denied.push(d), + ...extra, + }), + ) + } + + test.each([ + ['/caf\xc3\xa9', '/caf%C3%A9'], + ['/x?q=caf\xc3\xa9&k=v#f', '/x?q=caf%C3%A9&k=v#f'], + ['/caf\xe9', '/caf%E9'], + ['/a\tb\x00c\x7f', '/a%09b%00c%7F'], + ])( + '%p is answered 400 with what to do, and reported as %s', + async (sent, shown) => { + const port = await reporting() + const [asked, arrived] = [hookSeen.length, tlsSeen.length] + const reply = await untilClosed( + port, + `POST ${sent} HTTP/1.1\r\n${host()}\r\n`, + ) + expect(reply).toBe( + 'HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain\r\n' + + 'X-Proxy-Error: blocked-by-sandbox-runtime\r\n' + + `Content-Length: ${ADVICE.length + 1}\r\nConnection: close\r\n\r\n` + + `${ADVICE}\n`, + ) + expect(denied).toEqual([ + { + method: 'POST', + url: `https://${UPSTREAM_NAME}:${tlsPort}${shown}`, + reason: ADVICE, + }, + ]) + expect([hookSeen.length, tlsSeen.length]).toEqual([asked, arrived]) + }, + ) + + test.each([ + ['a space in the target', `GET /a b HTTP/1.1\r\n`, ''], + ['a header name with a space', 'GET /a HTTP/1.1\r\n', 'Bad Name: x\r\n'], + ['a control byte in a header', 'GET /a HTTP/1.1\r\n', 'X: a\x01b\r\n'], + ['a method it does not know', 'FROB /a HTTP/1.1\r\n', ''], + [ + 'two lengths', + 'POST /a HTTP/1.1\r\n', + 'Content-Length: 1\r\n'.repeat(2), + ], + [ + 'a head too long', + 'GET /a HTTP/1.1\r\n', + `X: ${'y'.repeat(200000)}\r\n`, + ], + ])('%s: 400, and only the tunnel is named', async (_what, line, header) => { + const port = await reporting() + const reply = await untilClosed(port, `${line}${host()}${header}\r\n`) + expect(reply).toStartWith('HTTP/1.1 400 Bad Request\r\n') + expect(reply).toMatch( + /\r\n\r\nmalformed request \(Parse Error: [ -~]+\)\n$/, + ) + expect(reply.match(/HTTP\/1\.1 /g)).toHaveLength(1) + expect(denied).toHaveLength(1) + expect(denied[0]).toMatchObject({ + method: '-', + url: `https://${UPSTREAM_NAME}:${tlsPort}`, + }) + expect(denied[0]!.reason).toStartWith('malformed request (Parse Error: ') + }) + + test('it is answered whoever listens, and without a hook, and the tunnel ends at once', async () => { + for (const port of [hookPort.default, noHookPort]) { + const began = Date.now() + expect( + await untilClosed(port, `GET /\xc3\xa9 HTTP/1.1\r\n${host()}\r\n`), + ).toEndWith(`${ADVICE}\n`) + expect(Date.now() - began).toBeLessThan(700) + } + }) + + test('a proxy that marks its refusals marks this one', async () => { + const port = await reporting({ denyHeader: 'X-Deny-Reason' }) + const reply = await untilClosed( + port, + `GET /\xc3\xa9 HTTP/1.1\r\n${host()}\r\n`, + ) + expect(reply).toContain('\r\nX-Deny-Reason: bad_request\r\n') + expect(reply).not.toMatch(/x-proxy-error/i) + }) + + test('it leaves a warn-level debug line', async () => { + const warn = spyOn(console, 'warn').mockImplementation(() => {}) + const error = spyOn(console, 'error').mockImplementation(() => {}) + process.env.SRT_DEBUG = '1' + try { + await untilClosed( + hookPort.default, + `GET /\xc3\xa9 HTTP/1.1\r\n${host()}\r\n`, + ) + expect(warn.mock.calls.flat().join('\n')).toContain(ADVICE) + } finally { + delete process.env.SRT_DEBUG + warn.mockRestore() + error.mockRestore() + } + }) + + test('after an answered request it is answered in its turn', async () => { + const port = await reporting() + const reply = await untilClosed( + port, + `GET /ok HTTP/1.1\r\n${host()}\r\n`, + `GET /\xc3\xa9 HTTP/1.1\r\n${host()}\r\n`, + ) + expect(reply.match(/HTTP\/1\.1 \d+/g)).toEqual([ + 'HTTP/1.1 200', + 'HTTP/1.1 400', + ]) + expect(denied.map(d => d.url)).toEqual([ + `https://${UPSTREAM_NAME}:${tlsPort}/%C3%A9`, + ]) + }) + + // The 400 would be read as the answer to the request before it. + test('behind a request still being answered, nothing is written', async () => { + const port = await reporting() + const reply = await untilClosed( + port, + `GET /ok HTTP/1.1\r\n${host()}\r\nGET /\xc3\xa9 HTTP/1.1\r\n${host()}\r\n`, + ) + expect(reply).not.toContain(' 400 ') + // The line the parser stopped in, not the packet's first. + expect(denied.map(d => d.url)).toEqual([ + `https://${UPSTREAM_NAME}:${tlsPort}/%C3%A9`, + ]) + }) + + test('a client that goes away in the middle of a head is no refusal', async () => { + const port = await reporting() + const error = spyOn(console, 'error').mockImplementation(() => {}) + process.env.SRT_DEBUG = '1' + try { + const tls = await tunnel(port) + tls.on('error', () => {}) + tls.write(`GET /a HTTP/1.1\r\n${host()}X: `) + await new Promise(r => setTimeout(r, 50)) + tls.destroy() + await new Promise(r => setTimeout(r, 100)) + // The server was told, so this is the path that reports nothing. + expect(error.mock.calls.flat().join('\n')).toContain( + '[tls-terminate] client connection error', + ) + } finally { + delete process.env.SRT_DEBUG + error.mockRestore() + } + expect(denied).toEqual([]) + }) + + test('one answer and one report, whatever follows', async () => { + const port = await reporting() + const bad = `GET /\xc3\xa9 HTTP/1.1\r\n${host()}\r\n` + const reply = await untilClosed(port, bad + bad, bad, bad) + expect(reply.match(/HTTP\/1\.1 /g)).toHaveLength(1) + expect(denied).toHaveLength(1) + }) + }) + test('a refused upload does not keep its tunnel', async () => { const tls = await tunnel(hookPort.default) const closed = new Promise(r => tls.once('close', () => r())) @@ -683,6 +930,19 @@ describe('request target forwarding', () => { ) }, ) + + test.each([undefined, 'asSpelled', 'normalize'] as const)( + '%p: a fragment is in neither the path sent nor the path signed', + async mode => { + const reply = await sendSigned( + await resigning(mode), + '/k.txt#f', + FAKE, + ) + expect(reply.forwarded).toBe('/k.txt') + expect(receivedAuthorization()).toBe(authorizationFor('/k.txt', REAL)) + }, + ) }) }) @@ -787,13 +1047,31 @@ describe('readRequestTarget', () => { const MODES = [undefined, 'refuse', 'asSpelled', 'normalize'] as const - test.each(FRAGMENT)('%s is refused for its fragment', raw => { + test.each(FRAGMENT)('%s is read as %s', (raw, left) => { for (const mode of MODES) { - expect(readRequestTarget(raw, 'GET', mode)).toEqual({ - spelled: raw, - refusal: - 'malformed request-target: a request-target has no fragment ("#")', - }) + for (const origin of ['', 'http://h.test']) { + expect(readRequestTarget(origin + raw, 'GET', mode)).toEqual( + readRequestTarget(left, 'GET', mode), + ) + } + } + expect(readRequestTarget(raw, 'GET').forwarded).toBe(left) + }) + + test.each(FRAGMENT_AMBIGUOUS)('%s is read as %s', (raw, left) => { + for (const mode of MODES) { + expect(readRequestTarget(raw, 'GET', mode)).toEqual( + readRequestTarget(left, 'GET', mode), + ) + } + expect(readRequestTarget(raw, 'GET').ambiguous).toBe(true) + }) + + test('what the fragment holds is not read', () => { + for (const fragment of ['\ufffd', '\ud800', '\u00e9', ' ', '\n#']) { + expect(readRequestTarget(`/p?q#${fragment}`, 'GET')).toEqual( + readRequestTarget('/p?q', 'GET'), + ) } }) @@ -802,12 +1080,13 @@ describe('readRequestTarget', () => { ['/a?q=\ufffd', '/a?q=%EF%BF%BD'], ['/\ud800', '/%EF%BF%BD'], ['/\udc00x', '/%EF%BF%BDx'], - ['/\ufffd#x', '/%EF%BF%BD#x'], + ['/\ufffd#x', '/%EF%BF%BD'], ])('%p is refused for what the runtime replaced', (raw, spelled) => { for (const mode of MODES) { expect(readRequestTarget(raw, 'GET', mode)).toEqual({ spelled, - refusal: 'malformed request-target: it holds bytes that are not UTF-8', + refusal: + 'malformed request-target: it holds bytes that are not UTF-8, or U+FFFD', }) } }) @@ -824,7 +1103,8 @@ describe('readRequestTarget', () => { 'http:/h.test/x', 'http://h.test\\x', 'http://h.test\\@other.test/x', - 'http://h.test#x', + '#x', + '*#x', '/a b', '/a\tb', '/a\x7f', @@ -835,6 +1115,177 @@ describe('readRequestTarget', () => { }) } }) + + // What a client that leaves the fragment in sends for a URL with no path. The + // URL parser ends the authority at the `#` too. + test.each([ + 'http://h.test#x', + 'http://h.test#x/y', + 'http://h.test#', + 'http://h.test#@other.test/x', + ])('%p is the root of its authority', raw => { + expect(new URL(raw).host).toBe('h.test') + for (const mode of MODES) { + expect(readRequestTarget(raw, 'GET', mode)).toEqual({ + spelled: '/', + normalized: '/', + ambiguous: false, + forwarded: '/', + refusal: undefined, + }) + } + }) +}) + +describe('refuseUnparsedRequest', () => { + const parseError = ( + packet: string | undefined, + at: number | undefined, + code = 'HPE_INVALID_URL', + ): Error => + Object.assign(new Error('Parse Error: x'), { + code, + bytesParsed: at, + rawPacket: + packet === undefined ? undefined : Buffer.from(packet, 'latin1'), + }) + /** What is reported of a packet the parser stopped in at `at`. */ + const named = (packet: string | undefined, at?: number, code?: string) => { + const refused = refuseUnparsedRequest( + parseError(packet, at, code), + new PassThrough(), + false, + )! + return `${refused.method} ${refused.target}` + } + + test('the line the parser stopped in, with nothing raw', () => { + expect(named('GET /\xe9 HTTP/1.1\r\nHost: h\r\n\r\n', 5)).toBe('GET /%E9') + expect(named('M-SEARCH /\xe9 HTTP/1.0\n', 10)).toBe('M-SEARCH /%E9') + expect( + named('GET /ok HTTP/1.1\r\n\r\nPUT /\x01<>\x80 HTTP/1.1\r\n', 25), + ).toBe('PUT /%01<>%80') + }) + + test('at most 200 bytes of it', () => { + expect(named(`GET /${'\xe9'.repeat(300)} HTTP/1.1\r\n`, 5)).toBe( + `GET /${'%E9'.repeat(199)}`, + ) + }) + + test.each([ + ['no packet', undefined, 5], + ['no offset', 'GET /\xe9 HTTP/1.1\r\n', undefined], + ['a line that began in another packet', '\xe9 HTTP/1.1\r\nHost: h\r\n', 0], + ['a line that ends in another packet', 'GET /\xe9', 5], + ['a line whose end is in another packet', 'GET /\xe9 HTTP/1.1', 5], + ['a method the runtime does not know', 'FROB /\xe9 HTTP/1.1\r\n', 6], + ['absolute form', 'GET http://o.test/\xe9 HTTP/1.1\r\n', 18], + ['a space in the target', 'GET /\xe9 b HTTP/1.1\r\n', 5], + ['another version', 'GET /\xe9 HTTP/2.0\r\n', 5], + ['a request line further on', '\xe9\nGET /x HTTP/1.1\r\n', 0], + ['a request line after the byte', '\nGET /\xe9 HTTP/1.1\r\n', 0], + ['two lines that end like one', 'GET /\xe9\r\nX HTTP/1.1\r\n', 5], + ['an offset that is no number', 'GET /\xe9 HTTP/1.1\r\n', '5' as never], + ['a request line further back', 'GET /x HTTP/1.1\r\nX: \xe9\r\n', 20], + ])('%s: nothing of the packet', (_what, packet, at) => { + expect(named(packet, at)).toBe('- ') + }) + + test('only where it stopped in the target', () => { + const packet = 'GET /x HTTP/1.1\r\nBad Name: y\r\n' + expect(named(packet, 5, 'HPE_INVALID_HEADER_TOKEN')).toBe('- ') + }) + + test("an error that is not the parser's ends the socket, unanswered", () => { + for (const code of [ + 'ECONNRESET', + 'EPIPE', + 'HPE_INVALID_EOF_STATE', + undefined, + 7, + ]) { + for (const ended of [false, true]) { + const socket = new PassThrough() + if (ended) socket.end() + expect( + refuseUnparsedRequest( + Object.assign(new Error('x'), { code }), + socket, + true, + ), + ).toBeUndefined() + expect(socket.destroyed).toBe(true) + expect(socket.readableLength).toBe(0) + } + } + }) + + test('what fails the parser after a refusal is the same refusal', () => { + const socket = new PassThrough({ highWaterMark: 1 }) + expect( + refuseUnparsedRequest(parseError('x', 0), socket, true), + ).toBeDefined() + const answer = socket.readableLength + const warn = spyOn(console, 'warn').mockImplementation(() => {}) + process.env.SRT_DEBUG = '1' + try { + for (const mayAnswer of [true, false]) { + expect( + refuseUnparsedRequest(parseError('x', 0), socket, mayAnswer), + ).toBeUndefined() + } + expect(warn).not.toHaveBeenCalled() + } finally { + delete process.env.SRT_DEBUG + warn.mockRestore() + } + // The answer is still on its way out. + expect(socket.destroyed).toBe(false) + expect(socket.readableLength).toBe(answer) + }) + + test('the socket goes as soon as the answer is out', async () => { + const socket = new PassThrough({ autoDestroy: false }) + socket.resume() + refuseUnparsedRequest(parseError('x', 0), socket, true) + await new Promise(r => setTimeout(r, 50)) + expect(socket.destroyed).toBe(true) + }) + + test('no write on a socket that takes none, or where it may not answer', () => { + const errored = new PassThrough() + errored.on('error', () => {}) + errored.destroy(new Error('x')) + for (const [socket, mayAnswer] of [ + [errored, true], + [new PassThrough(), false], + ] as const) { + const write = spyOn(socket, 'write') + const end = spyOn(socket, 'end') + expect( + refuseUnparsedRequest(parseError('x', 0), socket, mayAnswer), + ).toBeDefined() + expect(write).not.toHaveBeenCalled() + expect(end).not.toHaveBeenCalled() + expect(socket.destroyed).toBe(true) + } + }) + + test("a reset while the answer flushes is nobody's exception", () => { + const socket = new PassThrough() + refuseUnparsedRequest(parseError('x', 0), socket, true) + expect(() => socket.emit('error', new Error('reset'))).not.toThrow() + }) + + test('a socket whose flush never ends is destroyed all the same', async () => { + const stuck = new PassThrough({ highWaterMark: 1 }) + stuck.cork() + refuseUnparsedRequest(parseError('x', 0), stuck, true) + expect(stuck.destroyed).toBe(false) + await new Promise(r => setTimeout(r, 1100)) + expect(stuck.destroyed).toBe(true) + }) }) describe('network.ambiguousRequestTarget', () => { @@ -880,7 +1331,7 @@ describe('network.ambiguousRequestTarget', () => { seen.length = 0 const reply = await exchange( socket, - `GET http://u:p@127.0.0.1:${port}/a/../b?k=SECRET HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\n` + + `GET http://u:SECRET@127.0.0.1:${port}/a/../b?k=SECRET#SECRET HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\n` + `Proxy-Authorization: Basic ${auth.toString('base64')}\r\n\r\n`, ) expect(reply.status).toBe(status) @@ -891,12 +1342,12 @@ describe('network.ambiguousRequestTarget', () => { ? [`deny http-request GET http://127.0.0.1:${port}/a/../b?…`] : [], ) + expect(JSON.stringify(store.getViolations())).not.toContain('SECRET') }) test.each([ - ['/a#t=SECRET', '/a#…', 'a request-target has no fragment'], - ['/a?k=SECRET#x', '/a?…', 'a request-target has no fragment'], - ['/caf\xe9', '/caf%EF%BF%BD', 'bytes that are not UTF-8'], + ['/caf\xe9', '/caf%EF%BF%BD', 'bytes that are not UTF-8, or U+FFFD'], + ['/caf\xe9#t=SECRET', '/caf%EF%BF%BD', 'bytes that are not UTF-8'], ])('%p is recorded as %s', async (sent, shown, reason) => { await SandboxManager.reset() await SandboxManager.initialize({ @@ -929,5 +1380,53 @@ describe('network.ambiguousRequestTarget', () => { `deny http-request GET http://127.0.0.1:${port}${shown} (malformed request-target: `, ) expect(lines[0]).toContain(reason) + expect(lines[0]).not.toContain('SECRET') }) + + test.each([ + ['/caf\xc3\xa9?k=SECRET#SECRET', '/caf%C3%A9?…'], + ['/caf\xc3\xa9#t=SECRET', '/caf%C3%A9#…'], + ])( + 'in a tunnel, %p that the parser refuses is recorded as %s', + async (sent, shown) => { + 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 auth = Buffer.from(`srt:${SandboxManager.getProxyAuthToken()}`) + const raw = await new Promise((resolve, reject) => { + const s = connect(SandboxManager.getProxyPort()!, '127.0.0.1', () => + s.write( + `CONNECT 127.0.0.1:${port} HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\n` + + `Proxy-Authorization: Basic ${auth.toString('base64')}\r\n\r\n`, + ), + ) + s.once('error', reject) + s.once('data', () => resolve(s)) + }) + const tls = await new Promise((resolve, reject) => { + const t = tlsConnect( + { socket: raw, ca: CA_PEM, checkServerIdentity: () => undefined }, + () => resolve(t), + ) + t.once('error', reject) + }) + const store = SandboxManager.getSandboxViolationStore() + store.clear() + const reply = await exchange( + tls, + `GET ${sent} HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\n\r\n`, + ) + expect(reply.status).toBe(400) + expect(store.getViolations().map(v => v.line)).toEqual([ + `deny http-request GET https://127.0.0.1:${port}${shown} ` + + '(malformed request-target: percent-encode its bytes outside printable ASCII)', + ]) + }, + ) }) From 9ce832fccc4101265b13edc87f2e5ae238f161fa Mon Sep 17 00:00:00 2001 From: ronleizrowice-ant Date: Wed, 14 Oct 2026 03:07:21 +0100 Subject: [PATCH 5/5] A refused upload keeps its answer, no clear-text 502 in a tunnel, and OPTIONS * keeps its host (#702) * 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). * 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. * 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. --- 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 | 57 ++++++++++++++ .../sandbox/request-target-forwarding.test.ts | 76 ++++++++++++++++++- 5 files changed, 150 insertions(+), 16 deletions(-) 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, + ) })