diff --git a/README.md b/README.md index 12cd9a695..2b057e9a9 100644 --- a/README.md +++ b/README.md @@ -499,8 +499,14 @@ These two are read at each CONNECT, so `updateConfig` changes them for the tunne - `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) 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 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, 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, 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`; 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/docs/srt-proxy.md b/docs/srt-proxy.md index 51924b339..27373fe2c 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 ae4246037..e28c017a0 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, + respondRefusedTarget, + type AmbiguousRequestTarget, type DenyMark, applyHeaderEdits, notifyResponse, @@ -305,6 +307,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 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. + */ + ambiguousRequestTarget?: AmbiguousRequestTarget + /** * Refuse, with 400 and before the allowlist is consulted, a request * whose destination host is not spelled in plain ASCII: letters, digits, @@ -815,6 +828,7 @@ export function createHttpProxyServer(options: HttpProxyServerOptions): Server { foldHopByHop: options.foldHopByHop, denyHeader: options.denyHeader, requestTargetAsSpelled: options.requestTargetAsSpelled, + ambiguousRequestTarget: options.ambiguousRequestTarget, byteBudget, timeouts: options.tunnelTimeouts, onEstablished: () => clearTimeout(deadline), @@ -1075,23 +1089,38 @@ 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, + options.ambiguousRequestTarget, + ) + : undefined const requestTarget = options.requestTargetAsSpelled ? requestTargetAsSpelled(req.url!, { absolute: true }) - : options.filterRequest - ? normalizeRequestTarget(req.url!, req.method) + : read + ? read.forwarded : `${url.pathname}${url.search}` if (requestTarget === undefined) { - respondDenied( + const reason = respondRefusedTarget( res, - 'malformed request-target', + read?.refusal, markFor('bad_request'), - 400, ) + options.onFilterRequestDenied?.({ + method: req.method ?? 'GET', + url: `${url.protocol}//${authority}${read?.spelled ?? ''}`, + reason, + encodedCommand: auth.encodedCommand, + }) return } const absUrl = `${url.protocol}//${authority}${requestTarget}` @@ -1160,7 +1189,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/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 fb0568285..baa0e4183 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' @@ -114,14 +115,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, - * and the TLS-terminated path forwards the client's bytes unchanged.) + * The request-target in origin form (path and query) as the client + * 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 + * 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 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 + /** + * 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 +540,197 @@ 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' + +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, 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; + * - 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. (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, + method: string | undefined, + mode?: AmbiguousRequestTarget, +): { + spelled?: string + normalized?: string + ambiguous?: boolean + forwarded?: string + refusal?: string +} { + 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(target)) { + return { + spelled, + refusal: `${MALFORMED_TARGET}: it holds bytes that are not UTF-8, or U+FFFD`, + } + } + const [a, b] = [spelled, normalized].map(t => + t.replace(/["'<>^`{}]/g, percentEncoded), + ) + 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, + } +} + +/** + * 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, + refusal = MALFORMED_TARGET, + mark?: DenyMark, +): string { + logForDebugging(`[proxy] ${refusal}`, { level: 'warn' }) + respondDenied( + res, + refusal, + mark ?? + (refusal === AMBIGUOUS_TARGET ? 'ambiguous-request-target' : undefined), + 400, + ) + 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`, + ) + // 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 +} + /** * 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 bdcde9ec0..2d35158b5 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 ' + + '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({ caCertPath: z diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index 9ee0e15b1..815b92eef 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. */ @@ -744,6 +738,7 @@ async function startMuxProxyServer( stripResponseHeaders: config?.network.stripResponseHeaders, refuseOpaqueTunnels: config?.network.refuseOpaqueTunnels, requireHostMatch: config?.network.requireHostMatch, + ambiguousRequestTarget: config?.network.ambiguousRequestTarget, // Read at each CONNECT, so that updateConfig changes them. get maxTerminatedTunnels() { return config?.network.tlsTerminate?.maxTunnels diff --git a/src/sandbox/tls-terminate-proxy.ts b/src/sandbox/tls-terminate-proxy.ts index 15af7f61c..d7265d399 100644 --- a/src/sandbox/tls-terminate-proxy.ts +++ b/src/sandbox/tls-terminate-proxy.ts @@ -36,8 +36,11 @@ import { import type { MitmCA } from './mitm-ca.js' import { decideAndRespond, - normalizeRequestTarget, + readRequestTarget, + refuseUnparsedRequest, requestTargetAsSpelled, + respondRefusedTarget, + type AmbiguousRequestTarget, type DenyMark, applyHeaderEdits, notifyResponse, @@ -333,6 +336,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). */ @@ -438,6 +443,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 @@ -446,26 +457,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. @@ -1011,25 +1018,45 @@ 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 + // 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, + target.ambiguousRequestTarget, + ) + : undefined const path = target.requestTargetAsSpelled ? requestTargetAsSpelled(req.url ?? '/') - : filterRequest - ? normalizeRequestTarget(req.url ?? '/', req.method) + : 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 @@ -1047,18 +1074,15 @@ 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) + // 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 } @@ -1070,11 +1094,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 } @@ -1101,13 +1121,15 @@ async function forwardUpstream( filterRequest, req, res, - `https://${authority}${path}`, + url, ac.signal, target.onFilterRequestDeny, { 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 acc1a1595..6d7bd17a6 100644 --- a/test/config-validation.test.ts +++ b/test/config-validation.test.ts @@ -226,6 +226,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/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-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..a6987c88e 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' @@ -8,11 +8,29 @@ 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 { createHttpProxyServer } from '../../src/sandbox/http-proxy.js' +import { PassThrough } from 'node:stream' +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, + refuseUnparsedRequest, + 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' +import { spawnAsync } from '../helpers/spawn.js' const FIXTURE_DIR = join(import.meta.dir, '..', 'fixtures', 'tls-terminate') const CA_CERT = join(FIXTURE_DIR, 'ca.crt') @@ -47,6 +65,93 @@ 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'], + // 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 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 = [ + '/', + '/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', + // 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', + `/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 +159,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 +170,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 +178,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 +231,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 +269,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,60 +283,1220 @@ 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, + 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) }) - 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.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 in the sandbox's configuration is "refuse")`, + ) + expect(reply.forwarded).toBeUndefined() + expect(hookSeen.length).toBe(asked) + }) + }) + + 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('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('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']) + }) + + // 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 { 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('#') + } + }) + + 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/../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) => { + 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) + }, + ) }) - 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) + 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), + }), + ) + denied.length = 0 + 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-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: 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 => { + 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.each(PLAIN_UNCHANGED)('plain HTTP %s', async sent => { - const forwarded = await viaPlain(hookPort, sent) - expect(hookSeen.at(-1)).toBe(forwarded) + 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())) + 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) - 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') + 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), + ) + }, + ) + + 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)) + }, + ) + }) + }) + + // 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 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, + }) + }) + + test.each([ + ['*', '*'], + ['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') + }) + + 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 read as %s', (raw, left) => { + for (const mode of MODES) { + 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'), + ) + } + }) + + 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'], + ])('%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, or U+FFFD', + }) + } + }) + + 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', + '#x', + '*#x', + '/a b', + '/a\tb', + '/a\x7f', + ])('%p has no spelling to forward', raw => { + for (const mode of MODES) { + expect(readRequestTarget(raw, 'GET', mode)).toEqual({ + refusal: 'malformed request-target', + }) + } + }) + + // 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 is ended at once, and goes a second later', async () => { + const socket = new PassThrough({ autoDestroy: false }) + socket.resume() + refuseUnparsedRequest(parseError('x', 0), socket, true) + 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) + }) + + 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', () => { + 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?k=SECRET'], + ['normalize', 200, '/b?k=SECRET'], + ] 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 store = SandboxManager.getSandboxViolationStore() + store.clear() + seen.length = 0 + const reply = await exchange( + socket, + `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) + expect(seen).toEqual(forwarded === undefined ? [] : [forwarded]) + // 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?…`] + : [], + ) + expect(JSON.stringify(store.getViolations())).not.toContain('SECRET') + }) + + test.each([ + ['/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({ + 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) + 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)', + ]) + }, + ) + + // 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, + ) })