diff --git a/package-lock.json b/package-lock.json index 755e3cb87..01575bd55 100644 --- a/package-lock.json +++ b/package-lock.json @@ -36,7 +36,8 @@ "react": "^18.3.1", "sqlite-vec": "^0.1.9", "tinyld": "^1.3.4", - "turndown": "^7.2.4" + "turndown": "^7.2.4", + "undici": "^7.28.0" }, "bin": { "wigolo": "dist/index.js" @@ -57,7 +58,7 @@ "vitest": "^4.1.4" }, "engines": { - "node": ">=20" + "node": ">=20.18.1" }, "optionalDependencies": { "@napi-rs/keyring": "^1.3.0", @@ -9842,7 +9843,6 @@ "version": "7.28.0", "resolved": "https://registry.npmjs.org/undici/-/undici-7.28.0.tgz", "integrity": "sha512-cRZYrTDwWznlnRiPjggAGxZXanty6M8RV1ff8Wm4LWXBp7/IG8v5DnOm74DtUBp9OONpK75YlPnIjQqX0dBDtA==", - "dev": true, "license": "MIT", "engines": { "node": ">=20.18.1" diff --git a/package.json b/package.json index 9d704f6a5..d72e02f43 100644 --- a/package.json +++ b/package.json @@ -94,7 +94,7 @@ ] }, "engines": { - "node": ">=20" + "node": ">=20.18.1" }, "dependencies": { "@anthropic-ai/sdk": "^0.91.1", @@ -124,7 +124,8 @@ "react": "^18.3.1", "sqlite-vec": "^0.1.9", "tinyld": "^1.3.4", - "turndown": "^7.2.4" + "turndown": "^7.2.4", + "undici": "^7.28.0" }, "devDependencies": { "@seriousme/openapi-schema-validator": "2.9.0", diff --git a/src/fetch/http-client.ts b/src/fetch/http-client.ts index d77a9610e..86cca9b02 100644 --- a/src/fetch/http-client.ts +++ b/src/fetch/http-client.ts @@ -2,6 +2,8 @@ import { getConfig } from '../config.js'; import { createLogger } from '../logger.js'; import { anySignal } from '../util/abort.js'; import { guardFetchUrl, guardResolvedHost } from '../watch/ssrf.js'; +import { createPinnedAgent } from './pinned-dispatcher.js'; +import type { Agent } from 'undici'; export interface HttpFetchOptions { headers?: Record; @@ -141,6 +143,17 @@ class HttpFetchError extends Error { } } +/** + * Drive one fetch to completion, following redirects manually so every hop can be re-guarded. + * + * `redirect: 'manual'` is deliberate: an automatic redirect would connect to the next host without + * the SSRF checks below ever seeing it. Each hop therefore re-runs the literal-URL guard, the + * resolved-IP guard, and — since #207 — pins the socket to the addresses that guard just cleared, + * so the connection cannot be re-resolved onto a different address after validation. + * + * The response body is fully consumed here (text, or buffered for PDFs), which is what makes it + * safe to close the per-hop dispatchers in the `finally`. + */ async function fetchWithRedirects( originalUrl: string, options: HttpFetchOptions, @@ -152,8 +165,16 @@ async function fetchWithRedirects( const visited = new Set(); let currentUrl = originalUrl; let redirectCount = 0; + // Every Agent created across the redirect chain, kept only so the finally below can close them + // once the body has been consumed. An Agent left open is a socket leak, and the body is read + // inside this function, so closing on the way out is safe. + const agents: Agent[] = []; + try { while (true) { + // Per HOP, not per request: an IP-literal hop takes no pin, and leaving the previous hop's + // Agent in scope would send that request through a dispatcher built for a different host. + let pinnedAgent: Agent | undefined; if (visited.has(currentUrl)) { throw new HttpFetchError(`Redirect loop detected at ${currentUrl}`, false); } @@ -176,6 +197,28 @@ async function fetchWithRedirects( if (!resolved.ok) { throw new HttpFetchError(resolved.reason, false); } + // FAIL CLOSED. `guardResolvedHost` reports ok with no addresses when the host did not + // resolve, and the old reasoning was that there is then no IP to connect to. That holds + // only if both lookups get the same answer. They are two separate queries, so an attacker + // who controls the authority can answer the validation query with NXDOMAIN or an empty + // set and the connect query with a private address — which would sail through here + // unpinned and reinstate the rebinding path this change exists to close. + // + // Retryable, because at crawl scale a transient resolver blip is far more common than an + // attack, and the retry budget is bounded. Flip to `false` for fail-fast on bad hostnames. + if (!resolved.addresses?.length) { + throw new HttpFetchError( + `Could not resolve ${rhost} to a validated address; refusing to connect without pinning`, + true, + ); + } + // PIN the socket to what we just validated. Without this the connection resolves DNS a + // second time, and an attacker controlling the resolver can answer with a public IP for + // the check above and a private one for the connect (DNS rebinding). Pinning removes the + // second resolution, so there is no window to race. Literal IPs need no pinning — there + // is no name to re-resolve. + pinnedAgent = createPinnedAgent(rhost, resolved.addresses); + agents.push(pinnedAgent); } } @@ -210,6 +253,9 @@ async function fetchWithRedirects( headers: mergedHeaders, redirect: 'manual', signal, + // `dispatcher` is undici's extension to RequestInit; Node's global fetch honours it but + // the DOM typings do not declare it, hence the cast. + ...(pinnedAgent ? ({ dispatcher: pinnedAgent } as Record) : {}), }); } catch (err) { const isTimeout = err instanceof Error && err.name === 'TimeoutError'; @@ -238,6 +284,15 @@ async function fetchWithRedirects( } if (REDIRECT_STATUSES.has(response.status)) { + // Let go of the body before doing anything else with this hop. We never read a redirect + // body, and an unread one keeps the request in flight, which stalls the Agent cleanup in + // the `finally` for a full timeoutMs. Placed above the error branches too, so the + // no-location and too-many-redirects throws release it as well. + try { + await response.body?.cancel(); + } catch { + /* already closed, or never had a body */ + } const location = response.headers.get('location'); if (!location) { throw new HttpFetchError(`Redirect with no location header at ${currentUrl}`, false); @@ -265,6 +320,12 @@ async function fetchWithRedirects( } if (RETRYABLE_STATUSES.has(response.status)) { + // Same reasoning as the redirect branch: this throws without reading the body. + try { + await response.body?.cancel(); + } catch { + /* already closed, or never had a body */ + } throw new HttpFetchError(`HTTP ${response.status} from ${currentUrl}`, true); } @@ -314,4 +375,17 @@ async function fetchWithRedirects( rawBuffer, }; } + } finally { + for (const a of agents) { + try { + // destroy(), not close(): close() waits for in-flight requests, so a body we failed to + // release anywhere above would stall this cleanup for a full timeoutMs. By the time we + // reach here the body has either been read in full or deliberately cancelled, so there is + // nothing left worth waiting for, and this way a missed path cannot cost latency. + await a.destroy(); + } catch { + /* tearing down a pool must never mask the real result */ + } + } + } } diff --git a/src/fetch/pinned-dispatcher.ts b/src/fetch/pinned-dispatcher.ts new file mode 100755 index 000000000..4e5951389 --- /dev/null +++ b/src/fetch/pinned-dispatcher.ts @@ -0,0 +1,106 @@ +import { Agent, type buildConnector } from 'undici'; +import { lookup as nodeLookup } from 'node:dns'; + +/** + * Pin a connection to the addresses SSRF validation already cleared. + * + * `guardResolvedHost` resolves a hostname and checks every returned address, but the socket then + * resolves DNS *again* at connect time. An attacker who controls the authoritative resolver can + * answer with a public IP for our check and a private one for the connect, inside the TTL window — + * classic DNS rebinding, and the resolved-IP re-check cannot see it because it has already run. + * + * The fix is to stop the second resolution happening at all: hand the socket a `lookup` that + * returns the exact addresses we validated, so there is no window to race. + * + * WHY A LOOKUP HOOK RATHER THAN REWRITING THE URL TO THE IP + * -------------------------------------------------------- + * Rewriting `https://example.com/x` to `https://93.184.216.34/x` would also avoid re-resolution, + * and it would break TLS: the certificate is checked against the name in the URL, so the request + * would either fail verification or have to disable it. A `lookup` hook changes only which address + * the socket dials. The SNI name, the Host header and certificate verification all still use the + * real hostname, so pinning costs nothing in transport security. + */ + +/** The `dns.lookup` shape `net.connect` accepts. Both callback arities are in play — see below. */ +export type PinnedLookup = ( + hostname: string, + options: { family?: number; all?: boolean; hints?: number }, + callback: ( + err: NodeJS.ErrnoException | null, + address: string | { address: string; family: number }[], + family?: number, + ) => void, +) => void; + +export interface ValidatedAddress { + address: string; + family: number; +} + +/** + * Build the lookup hook. Exported separately from the Agent so it can be unit-tested without + * opening a socket. + * + * Two behaviours worth stating, because both are security-relevant: + * + * - **A different hostname is never pinned.** If the socket asks for a host we did not validate, + * we fall through to real DNS rather than handing back this host's addresses. Returning them + * would send a request for host B to host A's IP, which is a worse bug than the one being + * fixed. In practice the caller builds a fresh hook per validated host, so this is a guard + * against future misuse rather than a path we expect to hit. + * - **An empty address set is never pinned.** Callers must not construct this with `[]`; if they + * do, we fall back to real DNS instead of failing the connection in a way that looks like a + * network error. + */ +export function createPinnedLookup( + hostname: string, + addresses: ValidatedAddress[], + realLookup: typeof nodeLookup = nodeLookup, +): PinnedLookup { + return (host, options, callback) => { + if (host !== hostname || addresses.length === 0) { + // Not ours to pin — defer to the real resolver. + (realLookup as unknown as PinnedLookup)(host, options, callback); + return; + } + + // Honour an explicit family request; `family: 0` / undefined means "either". + const wanted = options?.family; + const matching = + wanted === 4 || wanted === 6 ? addresses.filter((a) => a.family === wanted) : addresses; + + if (matching.length === 0) { + // We hold addresses, but none in the family the socket asked for. Report it as a resolution + // failure rather than silently widening to a family the caller rejected. + const err: NodeJS.ErrnoException = new Error( + `no validated IPv${wanted} address for ${host}`, + ); + err.code = 'ENOTFOUND'; + callback(err, ''); + return; + } + + // `all: true` wants the array form; otherwise the (address, family) arity. + if (options?.all) { + callback(null, matching.map((a) => ({ address: a.address, family: a.family }))); + return; + } + callback(null, matching[0].address, matching[0].family); + }; +} + +/** + * An undici Agent whose sockets dial only the validated addresses. + * + * The caller owns the returned Agent and must `close()` (or `destroy()`) it once the response body + * has been consumed — an Agent holds a connection pool, so one per request that is never closed is + * a socket leak. + */ +export function createPinnedAgent(hostname: string, addresses: ValidatedAddress[]): Agent { + // `BuildOptions` is a union whose TCP member carries `lookup`; one cast at the boundary keeps + // that from leaking into the rest of the module, where the hook stays fully typed. + const connect = { + lookup: createPinnedLookup(hostname, addresses), + } as unknown as buildConnector.BuildOptions; + return new Agent({ connect }); +} diff --git a/src/watch/ssrf.ts b/src/watch/ssrf.ts index dc0960413..af979d72e 100644 --- a/src/watch/ssrf.ts +++ b/src/watch/ssrf.ts @@ -373,7 +373,14 @@ export type LookupAll = ( callback: (err: NodeJS.ErrnoException | null, addresses: { address: string; family: number }[]) => void, ) => void; -export type ResolveGuardResult = { ok: true } | SsrfRejection; +/** + * `addresses` carries the records that passed validation, so a caller can PIN the socket to + * them (see `src/fetch/pinned-dispatcher.ts`). It is absent when the host did not resolve — + * there is nothing to pin to, and that is not a bypass. + */ +export type ResolveGuardResult = + | { ok: true; addresses?: { address: string; family: number }[] } + | SsrfRejection; /** * Fetch-time SSRF re-check. `guardFetchUrl` validates only the LITERAL hostname, @@ -384,10 +391,11 @@ export type ResolveGuardResult = { ok: true } | SsrfRejection; * resolved-IP policy is identical to the literal-IP policy — no drift. * * Call this right before the actual fetch (and on each redirect hop) for any - * non-IP-literal host. For full rebinding (TOCTOU) safety the connection should - * additionally be pinned to the validated address — a `lookup` hook / custom - * dispatcher — which callers can layer on; this guard closes the static-record - * bypass (the metadata-credential-theft case) on its own. + * non-IP-literal host. On success it returns the validated `addresses`; pass them to + * `createPinnedAgent` (src/fetch/pinned-dispatcher.ts) so the socket dials one of those + * addresses instead of re-resolving, which is what closes the rebinding (TOCTOU) window. + * Without pinning this guard still closes the static-record bypass (the + * metadata-credential-theft case) on its own. */ /** Resolve every A/AAAA record for a host, or null on NXDOMAIN / timeout / empty. */ async function resolveAll( @@ -417,8 +425,14 @@ function isResolvedLoopback(address: string): boolean { * Fetch-time SSRF re-check for the plain fetch/crawl policy. Resolves the host and runs EVERY * resolved address back through `guardFetchUrl` (as an `http:///` literal), so the resolved-IP * policy is identical to the literal-IP one (metadata/RFC-1918 blocked, loopback allowed for the - * local-dev promise). A host that does not resolve is NOT a bypass — there is no IP to connect to, - * so we fall through (`ok: true`) and let the real fetch surface the natural DNS error. + * local-dev promise). + * + * A host that does not resolve returns `ok: true` with NO `addresses`. That is not an allow — + * it means there was nothing to validate, and therefore nothing to pin to. **Callers that go on to + * open a connection MUST fail closed in that case.** The lookup done here and the lookup done by + * the socket are two separate queries, so an attacker controlling the authority can answer this + * one with NXDOMAIN or an empty set and the connect one with a private address; proceeding + * unpinned would hand them exactly the rebinding window pinning is meant to remove. */ export async function guardResolvedHost( hostname: string, @@ -437,7 +451,7 @@ export async function guardResolvedHost( }; } } - return { ok: true }; + return { ok: true, addresses }; } /** @@ -475,5 +489,5 @@ export async function guardResolvedServeTarget( }; } } - return { ok: true }; + return { ok: true, addresses }; } diff --git a/tests/fetch/pinned-dispatcher.test.ts b/tests/fetch/pinned-dispatcher.test.ts new file mode 100755 index 000000000..9533d24a1 --- /dev/null +++ b/tests/fetch/pinned-dispatcher.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, it } from 'vitest'; +import { createPinnedLookup, type ValidatedAddress } from '../../src/fetch/pinned-dispatcher.js'; + +/** + * The lookup hook is tested directly rather than through a socket: it is the whole security + * boundary, it is pure, and testing it here means the rebinding case can be expressed exactly — + * "the second resolution returns something different" — which is impossible to stage reliably + * against a real resolver. + */ + +/** A real-DNS stand-in that returns whatever the attacker would answer on the SECOND lookup. */ +function rebindingResolver(addr: string, family = 4) { + return ((host: string, options: { all?: boolean }, cb: (...a: unknown[]) => void) => { + if (options?.all) cb(null, [{ address: addr, family }]); + else cb(null, addr, family); + }) as never; +} + +/** + * Promisify one call to the lookup hook. + * + * The hook is callback-shaped with two arities — `(err, address, family)` normally and + * `(err, addresses[])` under `all: true` — so both are captured and asserted on per test rather + * than normalised away here, since picking the wrong arity is itself a way the pin could break. + */ +function callLookup( + fn: ReturnType, + host: string, + options: { family?: number; all?: boolean } = {}, +): Promise<{ err: NodeJS.ErrnoException | null; address: unknown; family?: number }> { + return new Promise((resolve) => { + fn(host, options, (err, address, family) => resolve({ err, address, family })); + }); +} + +const VALIDATED: ValidatedAddress[] = [{ address: '93.184.216.34', family: 4 }]; + +describe('createPinnedLookup (DNS-rebinding pin, issue #207)', () => { + it('returns the validated address instead of re-resolving — the rebinding case', async () => { + // The attacker's resolver would now answer with the metadata IP. The pin must never ask it. + const fn = createPinnedLookup( + 'rebind.evil.example', + VALIDATED, + rebindingResolver('169.254.169.254'), + ); + const r = await callLookup(fn, 'rebind.evil.example'); + expect(r.err).toBeNull(); + expect(r.address).toBe('93.184.216.34'); + expect(r.family).toBe(4); + }); + + it('returns the array shape when the socket asks for all: true', async () => { + const fn = createPinnedLookup('example.com', VALIDATED, rebindingResolver('169.254.169.254')); + const r = await callLookup(fn, 'example.com', { all: true }); + expect(r.err).toBeNull(); + expect(r.address).toEqual([{ address: '93.184.216.34', family: 4 }]); + }); + + it('does NOT pin a different hostname — it defers to real DNS', async () => { + // Handing host B the addresses validated for host A would be a worse bug than the one we are + // fixing, so this is the must-not-do control. + const fn = createPinnedLookup('example.com', VALIDATED, rebindingResolver('203.0.113.9')); + const r = await callLookup(fn, 'other.example'); + expect(r.err).toBeNull(); + expect(r.address).toBe('203.0.113.9'); + }); + + it('falls back to real DNS when the validated set is empty rather than pinning to nothing', async () => { + const fn = createPinnedLookup('example.com', [], rebindingResolver('203.0.113.9')); + const r = await callLookup(fn, 'example.com'); + expect(r.err).toBeNull(); + expect(r.address).toBe('203.0.113.9'); + }); + + it('honours an explicit family request', async () => { + const both: ValidatedAddress[] = [ + { address: '93.184.216.34', family: 4 }, + { address: '2606:2800:220:1:248:1893:25c8:1946', family: 6 }, + ]; + const fn = createPinnedLookup('example.com', both, rebindingResolver('169.254.169.254')); + const v6 = await callLookup(fn, 'example.com', { family: 6 }); + expect(v6.address).toBe('2606:2800:220:1:248:1893:25c8:1946'); + const v4 = await callLookup(fn, 'example.com', { family: 4 }); + expect(v4.address).toBe('93.184.216.34'); + }); + + it('fails the lookup when no validated address matches the requested family', async () => { + // Widening to a family the caller excluded would be silently ignoring the request. + const fn = createPinnedLookup('example.com', VALIDATED, rebindingResolver('::1', 6)); + const r = await callLookup(fn, 'example.com', { family: 6 }); + expect(r.err).toBeTruthy(); + expect(r.err?.code).toBe('ENOTFOUND'); + }); + + it('passes every validated address through when the socket wants all of them', async () => { + const many: ValidatedAddress[] = [ + { address: '93.184.216.34', family: 4 }, + { address: '93.184.216.35', family: 4 }, + ]; + const fn = createPinnedLookup('example.com', many, rebindingResolver('169.254.169.254')); + const r = await callLookup(fn, 'example.com', { all: true }); + expect(r.address).toEqual(many); + }); +}); diff --git a/tests/fetch/pinned-e2e.test.ts b/tests/fetch/pinned-e2e.test.ts new file mode 100755 index 000000000..7d6cf3a54 --- /dev/null +++ b/tests/fetch/pinned-e2e.test.ts @@ -0,0 +1,49 @@ +import { createServer, type Server } from 'node:http'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { httpFetch } from '../../src/fetch/http-client.js'; + +/** + * The unit tests for `createPinnedLookup` prove the hook returns the right address. They do NOT + * prove the hook is actually wired into a real request, or that wiring it does not break one — + * and every other http-client test fetches an IP literal (`http://127.0.0.1:PORT/`), which skips + * the pinned path completely. + * + * So this fetches by NAME. `localhost` resolves to loopback, which the fetch policy allows for the + * local-dev promise, so the guard passes, returns its validated addresses, and the request goes + * out through a pinned Agent. If pinning were broken — wrong callback arity, wrong family, a + * dispatcher that never connects — this test hangs or fails while every IP-literal test keeps + * passing. + */ +describe('http tier: a request by hostname goes through the pinned dispatcher (issue #207)', () => { + let server: Server; + let port: number; + let hits = 0; + + beforeAll(async () => { + server = createServer((_req, res) => { + hits++; + res.writeHead(200, { 'content-type': 'text/html' }); + res.end('pinned ok'); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + port = (server.address() as { port: number }).port; + }); + + afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); + }); + + it('fetches successfully by name, not by IP literal', async () => { + const res = await httpFetch(`http://localhost:${port}/`); + expect(res.statusCode).toBe(200); + expect(res.html).toContain('pinned ok'); + expect(hits).toBeGreaterThan(0); + }); + + it('still fetches by IP literal, where no pinning applies', async () => { + const before = hits; + const res = await httpFetch(`http://127.0.0.1:${port}/`); + expect(res.statusCode).toBe(200); + expect(hits).toBe(before + 1); + }); +}); diff --git a/tests/fetch/pinned-failclosed.test.ts b/tests/fetch/pinned-failclosed.test.ts new file mode 100755 index 000000000..043679df9 --- /dev/null +++ b/tests/fetch/pinned-failclosed.test.ts @@ -0,0 +1,42 @@ +import { createServer, type Server } from 'node:http'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { httpFetch } from '../../src/fetch/http-client.js'; + +/** + * The must-reject control for the pin. + * + * `guardResolvedHost` reports ok with no addresses when a host does not resolve. Before this + * change the caller shrugged and let `fetch` re-resolve, on the reasoning that an unresolvable + * host has no IP to connect to. That only holds if both lookups get the same answer, and they are + * two separate queries — an attacker controlling the authority can answer the validation query + * with nothing and the connect query with 169.254.169.254. + * + * `.invalid` is reserved by RFC 6761 and guaranteed never to resolve, so it stands in for the + * "validation produced nothing" case without needing a hostile resolver. + */ +describe('fail closed when there is nothing to pin to', () => { + let server: Server; + let port: number; + + beforeAll(async () => { + server = createServer((_q, r) => r.end('should not be reached')); + await new Promise((res) => server.listen(0, '127.0.0.1', () => res())); + port = (server.address() as { port: number }).port; + }); + + afterAll(async () => { + await new Promise((res) => server.close(() => res())); + }); + + it('refuses a hostname that produced no validated addresses', async () => { + await expect(httpFetch('http://nothing-resolves-here.invalid/')).rejects.toThrow( + /refusing to connect without pinning/i, + ); + }, 30000); + + it('MUST-PASS control: a hostname that does resolve is still fetched normally', async () => { + // Without this, a change that refused *everything* would pass the test above and look correct. + const res = await httpFetch(`http://localhost:${port}/`); + expect(res.statusCode).toBe(200); + }, 30000); +}); diff --git a/tests/fetch/pinned-rebind.test.ts b/tests/fetch/pinned-rebind.test.ts new file mode 100755 index 000000000..0e6853ffc --- /dev/null +++ b/tests/fetch/pinned-rebind.test.ts @@ -0,0 +1,96 @@ +import { createServer, type Server } from 'node:http'; +import { Agent, request } from 'undici'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { createPinnedLookup } from '../../src/fetch/pinned-dispatcher.js'; + +/** + * The discriminating test: does pinning actually change which host the socket dials? + * + * Two servers on the same port, different loopback addresses — 127.0.0.1 is the address SSRF + * validation cleared, 127.0.0.2 stands in for the address an attacker's resolver flips to inside + * the TTL window. A "rebinding" lookup always answers 127.0.0.2. + * + * - through a plain Agent using that lookup, the request lands on the ATTACKER server + * - through a pinned Agent, it lands on the VALIDATED one + * + * That is DNS rebinding reproduced over a real socket, and it is the assertion that fails if the + * pin is ever removed or wired wrong. The `expect(attacker.hits)` line is the control: without it + * a broken pin that simply never connected would look the same as a working one. + */ +describe('pinning changes which address the socket dials (issue #207)', () => { + const VALIDATED_IP = '127.0.0.1'; + const ATTACKER_IP = '127.0.0.2'; + let validated: Server; + let attacker: Server; + let port = 0; + const hits = { validated: 0, attacker: 0 }; + let bothBound = false; + + beforeAll(async () => { + validated = createServer((_q, r) => { + hits.validated++; + r.writeHead(200, { 'content-type': 'text/plain' }); + r.end('validated'); + }); + attacker = createServer((_q, r) => { + hits.attacker++; + r.writeHead(200, { 'content-type': 'text/plain' }); + r.end('attacker'); + }); + await new Promise((res) => validated.listen(0, VALIDATED_IP, res)); + port = (validated.address() as { port: number }).port; + // 127.0.0.2 is bindable on Linux and Windows; if a platform refuses it, skip rather than + // report a pass we did not actually get. + bothBound = await new Promise((res) => { + attacker.once('error', () => res(false)); + attacker.listen(port, ATTACKER_IP, () => res(true)); + }); + }); + + afterAll(async () => { + await new Promise((res) => validated.close(() => res())); + if (bothBound) await new Promise((res) => attacker.close(() => res())); + }); + + /** A resolver that always answers with the attacker's address — the second, hostile resolution. */ + const rebindingLookup = (( + _host: string, + options: { all?: boolean }, + cb: (...a: unknown[]) => void, + ) => { + if (options?.all) cb(null, [{ address: ATTACKER_IP, family: 4 }]); + else cb(null, ATTACKER_IP, 4); + }) as never; + + it('WITHOUT pinning the rebinding resolver wins — the attacker server is reached', async (ctx) => { + // Skip VISIBLY where 127.0.0.2 will not bind. A silent `return` here would report a pass + // having asserted nothing, which is the failure mode this whole test exists to prevent. + if (!bothBound) ctx.skip(); + const agent = new Agent({ connect: { lookup: rebindingLookup } as never }); + const res = await request(`http://pinned.test:${port}/`, { dispatcher: agent }); + const body = await res.body.text(); + expect(body).toBe('attacker'); + expect(hits.attacker).toBeGreaterThan(0); + await agent.close(); + }); + + it('WITH pinning the validated address wins, even though DNS would say otherwise', async (ctx) => { + if (!bothBound) ctx.skip(); + const before = { ...hits }; + const agent = new Agent({ + connect: { + lookup: createPinnedLookup( + 'pinned.test', + [{ address: VALIDATED_IP, family: 4 }], + rebindingLookup, + ), + } as never, + }); + const res = await request(`http://pinned.test:${port}/`, { dispatcher: agent }); + const body = await res.body.text(); + expect(body).toBe('validated'); + expect(hits.validated).toBe(before.validated + 1); + expect(hits.attacker).toBe(before.attacker); // the attacker got nothing + await agent.close(); + }); +}); diff --git a/tests/fetch/pinned-redirect-body.test.ts b/tests/fetch/pinned-redirect-body.test.ts new file mode 100755 index 000000000..97c8d3783 --- /dev/null +++ b/tests/fetch/pinned-redirect-body.test.ts @@ -0,0 +1,86 @@ +import { createServer, type Server } from 'node:http'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { httpFetch } from '../../src/fetch/http-client.js'; + +/** + * A redirect whose body never ends must not be able to hold up the fetch. + * + * The redirect path reads `location` and continues without consuming the response body. That was + * untidy before this branch; it became costly once the per-hop Agents started being torn down on + * the way out, because an unread body keeps the request in flight. Measured at the time: 2038ms + * against `timeoutMs=2000`, i.e. a full timeout added to every such redirect. + * + * WHY /done IS GATED ON THE REDIRECT SOCKET CLOSING + * ------------------------------------------------- + * An elapsed-time assertion alone does NOT test what it looks like it tests. The fix has two + * halves — `response.body.cancel()` on the redirect path, and `Agent.destroy()` rather than + * `close()` in the cleanup — and `destroy()` does not wait for in-flight requests. So with the + * cancel deleted and only `destroy()` left, the call still returns immediately and a naive timing + * test passes. Verified by deleting it: the test passed in 202ms with no cancel anywhere. + * + * Gating `/done` on the redirect response's `close` event makes the second hop *depend* on the + * first hop's body actually being released. With the cancel in place the socket closes at once and + * this finishes in milliseconds. Without it, nothing closes until the per-hop AbortSignal fires, + * so `/done` cannot answer and elapsed climbs to `timeoutMs` — the assertion fails, by design. + */ +describe('a never-ending redirect body must not hold up the fetch', () => { + let server: Server; + let port: number; + let stopWriting: (() => void) | undefined; + let onRedirectClosed: (() => void) | undefined; + let redirectClosed: Promise; + + beforeAll(async () => { + redirectClosed = new Promise((resolve) => { + onRedirectClosed = resolve; + }); + + server = createServer((req, res) => { + if (req.url === '/redir') { + res.writeHead(302, { + location: `http://127.0.0.1:${port}/done`, + 'content-type': 'text/plain', + }); + // Never end this response. It stays in flight until the client lets go of the body. + const timer = setInterval(() => { + try { + res.write('.'.repeat(1024)); + } catch { + /* peer gone */ + } + }, 10); + stopWriting = () => clearInterval(timer); + res.on('close', () => { + clearInterval(timer); + onRedirectClosed?.(); + }); + return; + } + + // The second hop only answers once the first hop's body has actually been released. + void redirectClosed.then(() => { + res.writeHead(200, { 'content-type': 'text/html' }); + res.end('done'); + }); + }); + + await new Promise((res) => server.listen(0, '127.0.0.1', () => res())); + port = (server.address() as { port: number }).port; + }); + + afterAll(async () => { + stopWriting?.(); + onRedirectClosed?.(); + await new Promise((res) => server.close(() => res())); + }); + + it('releases the redirect body instead of leaving it in flight', async () => { + const started = Date.now(); + const res = await httpFetch(`http://localhost:${port}/redir`, { timeoutMs: 2000 }); + const elapsed = Date.now() - started; + expect(res.statusCode).toBe(200); + expect(res.html).toContain('done'); + console.log(` elapsed ${elapsed}ms for timeoutMs=2000`); + expect(elapsed).toBeLessThan(1500); + }, 20000); +}); diff --git a/tests/fetch/pinned-redirect.test.ts b/tests/fetch/pinned-redirect.test.ts new file mode 100755 index 000000000..f716fa7ba --- /dev/null +++ b/tests/fetch/pinned-redirect.test.ts @@ -0,0 +1,60 @@ +import { createServer, type Server } from 'node:http'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { httpFetch } from '../../src/fetch/http-client.js'; + +/** + * Redirect chains that mix a named hop with an IP-literal hop. + * + * A named hop takes a pinned dispatcher; an IP-literal hop takes none. Before the per-hop fix the + * dispatcher was declared outside the loop, so the literal hop inherited the previous hop's Agent. + * + * Be straight about what this test is: a REGRESSION GUARD, not a discriminating test. The buggy + * version passes it too, because `createPinnedLookup` defers to real DNS whenever the host it is + * asked for is not the host it was built for — so the inherited Agent behaved like a plain one and + * produced the right answer by accident. That fallthrough is what kept this a correctness problem + * rather than a routing one. The test is here so the mixed chain stays exercised, since nothing + * else in the suite covers it. + */ +describe('redirect chains mixing named and IP-literal hops', () => { + let server: Server; + let port: number; + const seen: string[] = []; + + beforeAll(async () => { + server = createServer((req, res) => { + seen.push(req.url ?? ''); + if (req.url === '/to-literal') { + res.writeHead(302, { location: `http://127.0.0.1:${port}/done` }); + res.end(); + return; + } + if (req.url === '/to-name') { + res.writeHead(302, { location: `http://localhost:${port}/done` }); + res.end(); + return; + } + res.writeHead(200, { 'content-type': 'text/html' }); + res.end('arrived'); + }); + await new Promise((res) => server.listen(0, '127.0.0.1', () => res())); + port = (server.address() as { port: number }).port; + }); + + afterAll(async () => { + await new Promise((res) => server.close(() => res())); + }); + + it('name -> IP literal: the literal hop must not inherit the named hop dispatcher', async () => { + const res = await httpFetch(`http://localhost:${port}/to-literal`); + expect(res.statusCode).toBe(200); + expect(res.html).toContain('arrived'); + expect(res.finalUrl).toBe(`http://127.0.0.1:${port}/done`); + }, 30000); + + it('IP literal -> name: the named hop still gets its own pin', async () => { + const res = await httpFetch(`http://127.0.0.1:${port}/to-name`); + expect(res.statusCode).toBe(200); + expect(res.html).toContain('arrived'); + expect(res.finalUrl).toBe(`http://localhost:${port}/done`); + }, 30000); +});