From 807d483b89a9acb06b6f925e2df818a751ccbc68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 25 Oct 2026 13:05:58 +0100 Subject: [PATCH] fix(egress): check every redirect hop and close the reserved-range gaps safeFetch with requirePublicHost vetted the first URL and pinned DNS answers in the connector, but Node never looks up an IP literal, so a followed redirect to a literal private or metadata address was dialled unchecked. No caller follows redirects to a user-chosen host today; the floor did not hold for the next one that would. With requirePublicHost, redirects are now followed one hop at a time and every location goes through isPublicUrl before it is dialled, at most five hops. isPublicIp also refuses the benchmarking, protocol-assignment, multicast, reserved and broadcast IPv4 ranges, site-local and multicast IPv6 and the IPv4-translated spelling of a private address, and isPublicUrl strips a trailing dot before the hostname denylist. --- src/lib/__tests__/safe-fetch-pinned.test.ts | 42 +++++++++++++ src/lib/safe-fetch.ts | 62 +++++++++++++++++++ .../__tests__/notifications.test.ts | 52 ++++++++++++++++ src/lib/validations/notifications.ts | 18 ++++++ 4 files changed, 174 insertions(+) diff --git a/src/lib/__tests__/safe-fetch-pinned.test.ts b/src/lib/__tests__/safe-fetch-pinned.test.ts index b7bf3af8c..e331ac2a9 100644 --- a/src/lib/__tests__/safe-fetch-pinned.test.ts +++ b/src/lib/__tests__/safe-fetch-pinned.test.ts @@ -73,6 +73,16 @@ function mockLookupTo(addresses: dns.LookupAddress[]): void { beforeAll(async () => { server = http.createServer((req, res) => { requestedPaths.push(req.url ?? ""); + if (req.url === "/redirect-literal") { + res.writeHead(302, { location: `http://127.0.0.1:${port}/secret` }); + res.end(); + return; + } + if (req.url === "/redirect-public") { + res.writeHead(302, { location: `http://pinned.test:${port}/api` }); + res.end(); + return; + } if (req.url === "/redirect") { res.writeHead(302, { location: `http://redirect-target.test:${port}/api`, @@ -192,6 +202,38 @@ describe("safeFetch pinned path — real transport", () => { expect(requestedPaths).toEqual(["/redirect"]); }, 20_000); + it("checks a followed redirect to an IP literal, which the DNS pin never sees", async () => { + mockLookupTo([{ address: "127.0.0.1", family: 4 }]); + // The input-time floor, as it really decides for a literal: the harness + // host passes, the loopback literal the redirect names does not. + notificationMocks.isPublicUrl.mockImplementation( + (url: string) => !new URL(url).hostname.startsWith("127."), + ); + requestedPaths.length = 0; + + await expect( + safeFetch( + `http://pinned.test:${port}/redirect-literal`, + {}, + { requirePublicHost: true, followRedirects: true, timeoutMs: 5000 }, + ), + ).rejects.toMatchObject({ kind: "private_host" }); + expect(requestedPaths).toEqual(["/redirect-literal"]); + notificationMocks.isPublicUrl.mockImplementation(() => true); + }, 20_000); + + it("follows a redirect to a public host, one checked hop at a time", async () => { + mockLookupTo([{ address: "127.0.0.1", family: 4 }]); + requestedPaths.length = 0; + const res = await safeFetch( + `http://pinned.test:${port}/redirect-public`, + {}, + { requirePublicHost: true, followRedirects: true, timeoutMs: 5000 }, + ); + expect(res.status).toBe(200); + expect(requestedPaths).toEqual(["/redirect-public", "/api"]); + }, 20_000); + it("streams a chunked response body via getReader() on the pinned path", async () => { mockLookupTo([{ address: "127.0.0.1", family: 4 }]); diff --git a/src/lib/safe-fetch.ts b/src/lib/safe-fetch.ts index 05684822e..2b34e6186 100644 --- a/src/lib/safe-fetch.ts +++ b/src/lib/safe-fetch.ts @@ -99,6 +99,64 @@ function hasPrivateHostPolicyCause(error: unknown): boolean { ); } +/** Hops a followed redirect chain may take before it is refused. */ +const MAX_REDIRECTS = 5; + +/** + * Follow redirects one hop at a time, putting every new location through + * `isPublicUrl` before it is dialled. + * + * The pinned dispatcher vets an address in its DNS lookup, and Node never + * looks up an IP literal, so a redirect to `http://169.254.169.254/` would + * have been dialled without a check: the input-time `isPublicUrl` had only + * seen the first URL. A 301, 302 or 303 continues as a GET without a body, + * as fetch does; a 307 or 308 repeats the request only when it has no body + * to send again. + */ +async function followCheckingEachHop( + start: string, + init: RequestInit, + dispatcher: NonNullable[1]>["dispatcher"], +): Promise { + let url = start; + let current: RequestInit = { ...init, redirect: "manual" }; + for (let hop = 0; ; hop++) { + const res = (await undiciFetch(url, { + ...current, + dispatcher, + } as Parameters[1])) as unknown as Response; + const location = res.headers.get("location"); + if (res.status < 300 || res.status >= 400 || !location) return res; + void res.body?.cancel().catch(() => {}); + if (hop >= MAX_REDIRECTS) { + throw new SafeFetchError( + "safeFetch followed too many redirects", + "network", + ); + } + const next = new URL(location, url).toString(); + if (!isPublicUrl(next)) { + throw new SafeFetchError( + "safeFetch refused a redirect to a private or non-public host", + "private_host", + ); + } + if (res.status === 307 || res.status === 308) { + if (current.body != null) { + throw new SafeFetchError( + "safeFetch cannot repeat a request body across a redirect", + "network", + ); + } + } else { + const { body: _body, ...rest } = current; + void _body; + current = { ...rest, method: "GET" }; + } + url = next; + } +} + /** * Default-safe outbound fetch wrapper. * @@ -217,6 +275,9 @@ export async function safeFetch( // locks the dispatcher and the fetch engine to one undici version. // The cast bridges undici's RequestInit (which carries `dispatcher`) // and its Response back to the DOM types this module exposes. + if (opts.requirePublicHost && redirect === "follow") { + return await followCheckingEachHop(target, finalInit, dispatcher); + } const res = await undiciFetch(target, { ...finalInit, dispatcher, @@ -225,6 +286,7 @@ export async function safeFetch( } return await fetch(target, finalInit); } catch (err) { + if (err instanceof SafeFetchError) throw err; if (hasPrivateHostPolicyCause(err)) { throw new SafeFetchError( "safeFetch refused a private resolved destination", diff --git a/src/lib/validations/__tests__/notifications.test.ts b/src/lib/validations/__tests__/notifications.test.ts index e559887d4..63e6305b8 100644 --- a/src/lib/validations/__tests__/notifications.test.ts +++ b/src/lib/validations/__tests__/notifications.test.ts @@ -258,3 +258,55 @@ describe("webPushSubscriptionSchema SSRF guard (V3 audit)", () => { expect(r.success).toBe(false); }); }); + +/** + * Ranges the floor used to call public: benchmarking, IETF protocol + * assignments, multicast, reserved and broadcast IPv4; site-local and + * multicast IPv6; the IPv4-translated spelling of a private address; and a + * denylisted name written fully qualified with a trailing dot. + */ +describe("isPublicIp / isPublicUrl — reserved ranges and spellings", () => { + it("refuses the reserved IPv4 ranges", () => { + for (const ip of [ + "198.18.0.1", + "198.19.255.254", + "192.0.0.1", + "224.0.0.1", + "239.255.255.250", + "240.0.0.1", + "255.255.255.255", + ]) { + expect(isPublicIp(ip), ip).toBe(false); + } + for (const ip of ["198.17.0.1", "198.20.0.1", "192.0.1.1", "223.255.0.1"]) { + expect(isPublicIp(ip), ip).toBe(true); + } + }); + + it("refuses site-local, multicast and IPv4-translated IPv6", () => { + for (const ip of [ + "fec0::1", + "feff::1", + "ff02::1", + "::ffff:0:7f00:1", + "::ffff:0:a9fe:a9fe", + ]) { + expect(isPublicIp(ip), ip).toBe(false); + } + expect(isPublicIp("::ffff:0:808:808")).toBe(true); + }); + + it("refuses a denylisted name written with a trailing dot", () => { + for (const url of [ + "http://localhost./", + "http://localhost../x", + "http://metadata.google.internal./", + "http://printer.local./", + "http://127.0.0.1./", + "http://[::ffff:0:7f00:1]/", + ]) { + expect(isPublicUrl(url), url).toBe(false); + } + expect(isPublicUrl("https://example.com./")).toBe(true); + }); +}); diff --git a/src/lib/validations/notifications.ts b/src/lib/validations/notifications.ts index d2d3745fb..19baa3356 100644 --- a/src/lib/validations/notifications.ts +++ b/src/lib/validations/notifications.ts @@ -75,6 +75,9 @@ function isPrivateIpv4(ip: [number, number, number, number]): boolean { if (a === 172 && b >= 16 && b <= 31) return true; // 172.16.0.0/12 RFC1918 if (a === 192 && b === 168) return true; // 192.168.0.0/16 RFC1918 if (a === 100 && b >= 64 && b <= 127) return true; // 100.64.0.0/10 CGNAT + if (a === 192 && b === 0 && ip[2] === 0) return true; // 192.0.0.0/24 IETF protocol assignments + if (a === 198 && (b === 18 || b === 19)) return true; // 198.18.0.0/15 benchmarking + if (a >= 224) return true; // 224/4 multicast, 240/4 reserved, broadcast return false; } @@ -155,6 +158,14 @@ function embeddedIpv4( return [bytes[12], bytes[13], bytes[14], bytes[15]]; } + // IPv4-translated (`::ffff:0:a.b.c.d`, RFC 2765). + if ( + bytesEqual(bytes, 0, Array(8).fill(0)) && + bytesEqual(bytes, 8, [0xff, 0xff, 0x00, 0x00]) + ) { + return [bytes[12], bytes[13], bytes[14], bytes[15]]; + } + if (bytesEqual(bytes, 0, Array(12).fill(0))) { return [bytes[12], bytes[13], bytes[14], bytes[15]]; } @@ -219,6 +230,10 @@ export function isPublicIp(ip: string): boolean { return false; } if (bytes[0] === 0xfe && (bytes[1] & 0xc0) === 0x80) return false; + // Site-local (fec0::/10, deprecated but still routed inside some + // networks) and multicast (ff00::/8). + if (bytes[0] === 0xfe && (bytes[1] & 0xc0) === 0xc0) return false; + if (bytes[0] === 0xff) return false; if ((bytes[0] & 0xfe) === 0xfc) return false; const embedded = embeddedIpv4(bytes); @@ -320,6 +335,9 @@ export function isPublicUrl(url: string): boolean { } let h = parsed.hostname.toLowerCase(); if (h.startsWith("[") && h.endsWith("]")) h = h.slice(1, -1); + // A fully qualified name ends in a dot (`localhost.`, `foo.internal.`); + // the resolver treats it as the same name, so the denylist must too. + h = h.replace(/\.+$/, ""); // Hostname denylist (literal strings). if (