From 923c58edec8b893aeed9f63c793b8ecc5396c805 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 19 Aug 2026 17:05:42 -0400 Subject: [PATCH] fix(server): pin all validated addresses; keep tests out of the build Addresses the round-2 Copilot review (both suppressed comments). - createPinnedAgent now takes the whole validated address list instead of a single IP, so Node's address-family fallback still works: a dual-stack host whose first address is unreachable (IPv6 on an IPv4-only network) connects via a later one. The set net.connect may choose from is exactly the prevalidated set, so the TOCTOU guarantee is unchanged. Empty input throws ProxyTargetError rather than pinning undefined. - Tests: dual-stack pass-through, the empty-list guard, and an end-to-end request where the first pinned address is a black hole (2001:db8::1) and the connection must complete via the second. - server/tsconfig.json excludes src/**/__tests__/** so the suite is no longer emitted into build/ and published with the package (verified: build/__tests__ is gone). tsconfig.test.json keeps the suite type-checked, and `npm test` runs it before vitest. Signed-off-by: cliffhall --- server/package.json | 2 +- server/src/__tests__/proxy-security.test.ts | 60 ++++++++++++++++++--- server/src/index.ts | 8 +-- server/src/proxy-security.ts | 27 +++++++--- server/tsconfig.json | 2 +- server/tsconfig.test.json | 12 +++++ 6 files changed, 91 insertions(+), 20 deletions(-) create mode 100644 server/tsconfig.test.json diff --git a/server/package.json b/server/package.json index 063ab012..bfad46b5 100644 --- a/server/package.json +++ b/server/package.json @@ -25,7 +25,7 @@ "start": "node build/index.js", "dev": "tsx watch --clear-screen=false src/index.ts", "dev:windows": "tsx watch --clear-screen=false src/index.ts < NUL", - "test": "vitest run", + "test": "tsc -p tsconfig.test.json && vitest run", "test:watch": "vitest" }, "devDependencies": { diff --git a/server/src/__tests__/proxy-security.test.ts b/server/src/__tests__/proxy-security.test.ts index 1e07eaed..db97e6b9 100644 --- a/server/src/__tests__/proxy-security.test.ts +++ b/server/src/__tests__/proxy-security.test.ts @@ -224,18 +224,18 @@ function pinnedLookup(agent: http.Agent | https.Agent): PinnedLookup { describe("createPinnedAgent", () => { it("returns an http.Agent for http: protocol", () => { - const agent = createPinnedAgent("http:", "127.0.0.1"); + const agent = createPinnedAgent("http:", ["127.0.0.1"]); expect(agent).toBeInstanceOf(http.Agent); expect(agent).not.toBeInstanceOf(https.Agent); }); it("returns an https.Agent for https: protocol", () => { - const agent = createPinnedAgent("https:", "127.0.0.1"); + const agent = createPinnedAgent("https:", ["127.0.0.1"]); expect(agent).toBeInstanceOf(https.Agent); }); it("pinned lookup returns the IPv4 address regardless of queried hostname", () => { - const lookup = pinnedLookup(createPinnedAgent("http:", "192.0.2.99")); + const lookup = pinnedLookup(createPinnedAgent("http:", ["192.0.2.99"])); const callback = vi.fn(); lookup("example.com", {}, callback); @@ -244,7 +244,7 @@ describe("createPinnedAgent", () => { }); it("pinned lookup returns the IPv6 address and family 6", () => { - const lookup = pinnedLookup(createPinnedAgent("http:", "2001:db8::1")); + const lookup = pinnedLookup(createPinnedAgent("http:", ["2001:db8::1"])); const callback = vi.fn(); lookup("example.com", {}, callback); @@ -256,7 +256,7 @@ describe("createPinnedAgent", () => { // the hook with { all: true } and requires the ARRAY callback shape. Handing // it a scalar there fails every hostname request with ERR_INVALID_IP_ADDRESS. it("pinned lookup returns the array shape when called with { all: true }", () => { - const lookup = pinnedLookup(createPinnedAgent("http:", "192.0.2.99")); + const lookup = pinnedLookup(createPinnedAgent("http:", ["192.0.2.99"])); const callback = vi.fn(); lookup("example.com", { all: true }, callback); @@ -267,7 +267,7 @@ describe("createPinnedAgent", () => { }); it("pinned lookup returns the array shape for IPv6 too", () => { - const lookup = pinnedLookup(createPinnedAgent("http:", "2001:db8::1")); + const lookup = pinnedLookup(createPinnedAgent("http:", ["2001:db8::1"])); const callback = vi.fn(); lookup("example.com", { all: true }, callback); @@ -277,8 +277,52 @@ describe("createPinnedAgent", () => { ]); }); + // Every validated address is handed to the hook, so Node keeps its normal + // address-family fallback for a dual-stack host whose first address is + // unreachable — while still only ever seeing prevalidated addresses. + it("passes every validated address through for dual-stack fallback", () => { + const lookup = pinnedLookup( + createPinnedAgent("http:", ["2001:db8::1", "192.0.2.7"]), + ); + + const callback = vi.fn(); + lookup("dual.example", { all: true }, callback); + + expect(callback).toHaveBeenCalledWith(null, [ + { address: "2001:db8::1", family: 6 }, + { address: "192.0.2.7", family: 4 }, + ]); + }); + + it("throws when given no validated address", () => { + expect(() => createPinnedAgent("http:", [])).toThrow(ProxyTargetError); + }); + + // A dual-stack host whose first (IPv6) address is dead: the connection must + // still complete via the second, validated address. + it("falls back to a later validated address when the first is unreachable", async () => { + const server = http.createServer((_req, res) => res.end("fallback-ok")); + await new Promise((resolve) => + server.listen(0, "127.0.0.1", () => resolve()), + ); + const { port } = server.address() as AddressInfo; + + try { + // 2001:db8::1 is the documentation range — nothing answers there. + const agent = createPinnedAgent("http:", ["2001:db8::1", "127.0.0.1"]); + const response = await nodeFetch(`http://dual.invalid:${port}/`, { + agent, + }); + + expect(response.status).toBe(200); + await expect(response.text()).resolves.toBe("fallback-ok"); + } finally { + await new Promise((resolve) => server.close(() => resolve())); + } + }); + it("TOCTOU guarantee: lookup never invokes the OS resolver", () => { - const lookup = pinnedLookup(createPinnedAgent("http:", "10.0.0.1")); + const lookup = pinnedLookup(createPinnedAgent("http:", ["10.0.0.1"])); const callback = vi.fn(); lookup("any-hostname.example", {}, callback); @@ -300,7 +344,7 @@ describe("createPinnedAgent", () => { const { port } = server.address() as AddressInfo; try { - const agent = createPinnedAgent("http:", "127.0.0.1"); + const agent = createPinnedAgent("http:", ["127.0.0.1"]); const response = await nodeFetch( `http://pinned-target.invalid:${port}/`, { agent }, diff --git a/server/src/index.ts b/server/src/index.ts index 25a9309b..5e6604f7 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -888,10 +888,12 @@ async function safeProxyFetch(initialUrl: string, init?: RequestInit) { throw new ProxyTargetError("Only http/https URLs are allowed"); } - // Resolve DNS once, validate every address, and pin the connection to the - // first validated IP so node-fetch never does a second DNS lookup. + // Resolve DNS once, validate every address, and pin the connection to that + // validated set so node-fetch never does a second DNS lookup. All of them + // are passed through, so Node's address-family fallback still works while + // the reachable set stays limited to what was just validated. const validatedAddresses = await assertSafeProxyTarget(currentUrl); - const agent = createPinnedAgent(currentUrl.protocol, validatedAddresses[0]); + const agent = createPinnedAgent(currentUrl.protocol, validatedAddresses); const response = await fetch(currentUrl.toString(), { method, diff --git a/server/src/proxy-security.ts b/server/src/proxy-security.ts index 45b7510d..04f99bc3 100644 --- a/server/src/proxy-security.ts +++ b/server/src/proxy-security.ts @@ -125,20 +125,33 @@ export async function assertSafeProxyTarget(parsedUrl: URL): Promise { /** * Creates an `http.Agent` or `https.Agent` whose internal `lookup` hook always - * returns `pinnedIp`, bypassing the OS resolver entirely. + * returns `pinnedIps`, bypassing the OS resolver entirely. * * Pass this agent to `node-fetch` on every hop of `safeProxyFetch` so the TCP - * connection always goes to the IP that was validated by `assertSafeProxyTarget` + * connection always goes to an IP that was validated by `assertSafeProxyTarget` * — even if the domain's DNS record changes mid-flight. * + * Every address `assertSafeProxyTarget` returned is handed to the hook, not + * just the first, so Node keeps its normal address-family fallback: a + * dual-stack host whose first address is unreachable (IPv6 on an IPv4-only + * network, say) still connects via a later one. The set it may choose from is + * exactly the prevalidated set, so the TOCTOU guarantee is unchanged. + * * @param protocol - `"http:"` or `"https:"` (from `URL.protocol`) - * @param pinnedIp - A validated IPv4 or IPv6 address string + * @param pinnedIps - Validated IPv4/IPv6 address strings, in preference order */ export function createPinnedAgent( protocol: string, - pinnedIp: string, + pinnedIps: string[], ): http.Agent | https.Agent { - const family = isIP(pinnedIp) === 6 ? 6 : 4; + if (pinnedIps.length === 0) { + throw new ProxyTargetError("No validated address to connect to"); + } + + const resolved: dns.LookupAddress[] = pinnedIps.map((address) => ({ + address, + family: isIP(address) === 6 ? 6 : 4, + })); // Node.js lookup signature: (hostname, options, callback). // @@ -157,10 +170,10 @@ export function createPinnedAgent( ) => void, ): void => { if (opts?.all) { - callback(null, [{ address: pinnedIp, family }]); + callback(null, resolved); return; } - callback(null, pinnedIp, family); + callback(null, resolved[0].address, resolved[0].family); }; if (protocol === "https:") { diff --git a/server/tsconfig.json b/server/tsconfig.json index b5a92612..8b3ad0ca 100644 --- a/server/tsconfig.json +++ b/server/tsconfig.json @@ -12,5 +12,5 @@ "resolveJsonModule": true }, "include": ["src/**/*"], - "exclude": ["node_modules", "packages", "**/*.spec.ts"] + "exclude": ["node_modules", "packages", "**/*.spec.ts", "src/**/__tests__/**"] } diff --git a/server/tsconfig.test.json b/server/tsconfig.test.json new file mode 100644 index 00000000..e25a6b70 --- /dev/null +++ b/server/tsconfig.test.json @@ -0,0 +1,12 @@ +// The build config excludes __tests__ so the suite never lands in the +// published `build/` output. This config exists only so the tests are still +// type-checked; it emits nothing and is run by `npm test`. +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true, + "types": ["node"] + }, + "include": ["src/**/*"], + "exclude": ["node_modules", "packages"] +}