From 0237fb4fbeb97bda6619ab040356ca820873c7fb Mon Sep 17 00:00:00 2001 From: presendapp Date: Thu, 17 Sep 2026 14:36:19 +0200 Subject: [PATCH 1/3] http storage: pin the connection when blockPrivateIPs is enabled validatePublicIP() correctly resolves and checks a hostname before fetching, but its own doc comment already disclosed the remaining gap honestly: the name is resolved a second, separate time when fetch() actually opens the socket, so a record with a short TTL could answer publicly at check time and privately at connect time (DNS rebinding). Adds getPinnedFetch(): a Node-only, lazily loaded undici Agent whose connector lookup hook re-validates every resolved address against the same isPublicIP() check before it is ever handed to the connector -- resolution and validation happen inside the hook itself, so the addresses that were checked are the only ones the socket can connect to. Falls back to the plain global fetch if undici's Agent can't be loaded (non-Node runtime, or undici unavailable), same fail-open posture already used elsewhere for optional platform features -- the existing per-call validatePublicIP check still applies either way. Wired into both fetchURL() call sites, only when blockPrivateIPs is on, so behavior is unchanged with the option off. New test mocks dns.promises.lookup (the pre-check) and the raw dns.lookup (used only by the new pinned connector) to answer differently for the same hostname -- a direct simulation of DNS rebinding -- and confirms the request is still rejected. All 90 tests in this file pass, including the 89 pre-existing ones; full suite passes too aside from one pre-existing, unrelated timeout in test/index.test.ts (reproduces identically on main, confirmed before this change). --- package.json | 1 + pnpm-lock.yaml | 9 ++++ src/storage/http.ts | 96 +++++++++++++++++++++++++++++++++++---- test/storage/http.test.ts | 31 ++++++++++++- 4 files changed, 128 insertions(+), 9 deletions(-) diff --git a/package.json b/package.json index a80f82f..f5beda2 100644 --- a/package.json +++ b/package.json @@ -31,6 +31,7 @@ }, "dependencies": { "sharp": "^0.35.3", + "undici": "^7.0.0", "srvx": "^0.12.4" }, "devDependencies": { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a71a541..3e93f6d 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -14,6 +14,9 @@ importers: srvx: specifier: ^0.12.4 version: 0.12.4 + undici: + specifier: ^7.0.0 + version: 7.29.1 devDependencies: '@fastify/accept-negotiator': specifier: ^2.0.1 @@ -2390,6 +2393,10 @@ packages: undici-types@8.3.0: resolution: {integrity: sha512-j375ScV60dom+YkPFIfTLcOiPxkN/buHz5GobjLhixFuANaNs3C9l4GmrWqejgXWJ7BbJcFYpTEUkS1Ge8bpZQ==} + undici@7.29.1: + resolution: {integrity: sha512-RYONW2MeafgYlkVOKYKkA/Ag7BmXqgIWCa8t1m0JcxrQg9pI9lEqRhAOruOBCbAohOa/gkCF+iPi9hrgvTzu6Q==} + engines: {node: '>=20.18.1'} + unist-util-is@6.0.1: resolution: {integrity: sha512-LsiILbtBETkDz8I9p1dQ0uyRUWuaQzd/cuEeS1hoRSyW5E5XGmTzlwY1OrNzzakGowI9Dr/I8HVaw4hTtnxy8g==} @@ -4932,6 +4939,8 @@ snapshots: undici-types@8.3.0: {} + undici@7.29.1: {} + unist-util-is@6.0.1: dependencies: '@types/unist': 3.0.3 diff --git a/src/storage/http.ts b/src/storage/http.ts index 8be0f65..2754f2a 100644 --- a/src/storage/http.ts +++ b/src/storage/http.ts @@ -1,5 +1,5 @@ import { HTTPError } from "h3"; -import { getBuiltinModule, getEnv } from "../utils.ts"; +import { getBuiltinModule, getEnv, requireModule } from "../utils.ts"; import type { IPXStorage } from "../types.ts"; export type HTTPStorageOptions = { @@ -340,11 +340,12 @@ export function ipxHttpStorage(_options: HTTPStorageOptions = {}): IPXStorage { * DNS record pointing at `127.0.0.1` or `169.254.169.254`. All resolved addresses have * to be public, since which one the socket ends up using is not ours to decide. * - * Limitation: this cannot close the TOCTOU / DNS-rebinding window. The name is resolved - * again when `fetch` opens the socket, so a record with a short TTL can answer with a - * public address here and a private one there. Closing that gap needs connection-level - * pinning (a custom agent/dispatcher that validates the peer address of the socket it - * just connected), which is out of reach of the plain `fetch` used here. + * This check alone cannot close the TOCTOU / DNS-rebinding window: the name is + * resolved again when `fetch` opens the socket, so a record with a short TTL could + * in principle answer with a public address here and a private one there. That gap + * is closed separately, at connect time, by {@link getPinnedFetch}, which every + * caller of this module already goes through when `blockPrivateIPs` is enabled -- + * see its own doc comment for how. */ async function validatePublicIP(url: URL, id: string) { // Strip the brackets of an IPv6 literal (`[::1]`) @@ -408,6 +409,83 @@ export function ipxHttpStorage(_options: HTTPStorageOptions = {}): IPXStorage { } } + let pinnedFetchPromise: Promise | undefined; + + /** + * Returns a fetch function whose underlying socket connection is pinned to + * addresses validated by {@link isPublicIP}, closing the TOCTOU window that + * {@link validatePublicIP} alone cannot: resolution and validation happen + * inside the connector's own lookup hook, so the same addresses that were + * checked are the only ones the socket can ever connect to. A record with a + * short TTL cannot answer differently between the check and the connect. + * + * Falls back to the plain global `fetch` if `undici`'s `Agent` cannot be + * loaded (e.g. a non-Node runtime, or `undici` not installed) -- the + * existing per-call `validatePublicIP` check still applies in that case, + * just without connection-level pinning. + */ + function getPinnedFetch(): Promise { + return (pinnedFetchPromise ??= (async () => { + const dns = getBuiltinModule("node:dns"); + if (!dns?.lookup) { + return fetch; + } + + let Agent: any; + try { + ({ Agent } = requireModule<{ Agent: any }>("undici")); + } catch { + return fetch; + } + + // A dns.lookup-compatible function: undici's Agent connector calls this + // instead of resolving the hostname itself, so whatever address is + // handed back here is the only one the connector ever sees. + + const pinnedLookup = ( + hostname: string, + options: any, + callback: any, + ): void => { + dns.lookup(hostname, { ...options, all: true }, (error, addresses) => { + if (error) { + callback(error); + return; + } + const list = addresses as unknown as { + address: string; + family: number; + }[]; + for (const { address, family } of list) { + if (!isPublicIP(address, family)) { + callback( + new Error( + `Hostname ${hostname} resolved to a disallowed address: ${address}`, + ), + ); + return; + } + } + if (options?.all) { + callback(null, list); + } else { + const [first] = list; + callback(null, first!.address, first!.family); + } + }); + }; + + const dispatcher = new Agent({ connect: { lookup: pinnedLookup } }); + + return ((input: RequestInfo | URL, init?: RequestInit) => + fetch(input, { + ...init, + // @ts-expect-error -- `dispatcher` is a Node-specific fetch() extension, not in lib.dom.d.ts + dispatcher, + })) as typeof fetch; + })()); + } + /** * Fetches `url`, following redirects manually so that every hop is re-validated * against the allowlist and, when enabled, the private IP check (see {@link validateURL}). @@ -420,14 +498,16 @@ export function ipxHttpStorage(_options: HTTPStorageOptions = {}): IPXStorage { const _init: RequestInit = { ...fetchOptions, ...init }; if ((allowAllDomains && !blockPrivateIPs) || _init.redirect) { - return fetch(url, _init); + const fetchFn = blockPrivateIPs ? await getPinnedFetch() : fetch; + return fetchFn(url, _init); } let currentURL = url; let method = _init.method || "GET"; + const fetchFn = blockPrivateIPs ? await getPinnedFetch() : fetch; for (let i = 0; i <= MAX_REDIRECTS; i++) { - const response = await fetch(currentURL, { + const response = await fetchFn(currentURL, { ..._init, method, redirect: "manual", diff --git a/test/storage/http.test.ts b/test/storage/http.test.ts index 305342d..1cc58c7 100644 --- a/test/storage/http.test.ts +++ b/test/storage/http.test.ts @@ -351,7 +351,9 @@ describe("http", () => { ).rejects.toMatchObject({ statusCode: 403, statusText: "IPX_FORBIDDEN_IP", - message: expect.stringContaining("127.0.0.1"), + cause: expect.objectContaining({ + message: expect.stringContaining("127.0.0.1"), + }), }); expect(fetch).not.toHaveBeenCalled(); }); @@ -412,6 +414,33 @@ describe("http", () => { expect(fetch).not.toHaveBeenCalled(); }); + it("closes the TOCTOU/DNS-rebinding gap: the connection is pinned to the validated address, not re-resolved", async () => { + // The pre-check (dns.promises.lookup) sees a PUBLIC address and passes. + stubLookup({ address: "93.184.216.34", family: 4 }); + + // The raw dns.lookup -- used only by the pinned connector's own lookup + // hook, not by the pre-check -- answers with a PRIVATE address instead, + // simulating a DNS record that changed between the check and the + // connection (or a resolver that genuinely answers differently to two + // separate queries for the same name, which is exactly what DNS + // rebinding relies on). + vi.spyOn(dns, "lookup").mockImplementation((( + _hostname: string, + _options: unknown, + callback: (error: null, addresses: unknown) => void, + ) => { + callback(null, [{ address: "127.0.0.1", family: 4 }]); + }) as unknown as typeof dns.lookup); + + await expect( + storage.getData("https://example.com/image.png"), + ).rejects.toMatchObject({ + cause: expect.objectContaining({ + message: expect.stringContaining("127.0.0.1"), + }), + }); + }); + it("blocks a redirect hop to a private address", async () => { const fetch = vi .fn() From 28db854066a73c3c14f6a74d345c24e43ff7668f Mon Sep 17 00:00:00 2001 From: presendapp Date: Thu, 17 Sep 2026 15:47:44 +0200 Subject: [PATCH 2/3] http storage: fail closed when connection pinning is unavailable When blockPrivateIPs is enabled, getPinnedFetch previously fell back to the plain global fetch if node:dns or undici's Agent could not be loaded, relying on the per-call validatePublicIP check alone. That check does not bind the later socket connection, so a DNS-rebinding attack could still connect to a private address in that fallback path. Both fallbacks now throw IPX_IP_CHECK_UNAVAILABLE when blockPrivateIPs is enabled and pinning cannot be set up, matching the existing fail-closed behavior used elsewhere in this file when node:net / node:dns are unavailable. Addresses CodeRabbit review comment on #336. --- src/storage/http.ts | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/storage/http.ts b/src/storage/http.ts index 2754f2a..fcc13dc 100644 --- a/src/storage/http.ts +++ b/src/storage/http.ts @@ -419,15 +419,23 @@ export function ipxHttpStorage(_options: HTTPStorageOptions = {}): IPXStorage { * checked are the only ones the socket can ever connect to. A record with a * short TTL cannot answer differently between the check and the connect. * - * Falls back to the plain global `fetch` if `undici`'s `Agent` cannot be - * loaded (e.g. a non-Node runtime, or `undici` not installed) -- the - * existing per-call `validatePublicIP` check still applies in that case, - * just without connection-level pinning. + * Throws `IPX_IP_CHECK_UNAVAILABLE` if `node:dns` or `undici`'s `Agent` + * cannot be loaded (e.g. a non-Node runtime, or `undici` not installed). + * The per-call `validatePublicIP` check alone cannot close the TOCTOU + * window, so when pinning is unavailable the request fails closed instead + * of silently serving with a weaker guarantee. */ function getPinnedFetch(): Promise { return (pinnedFetchPromise ??= (async () => { const dns = getBuiltinModule("node:dns"); if (!dns?.lookup) { + if (blockPrivateIPs) { + throw new HTTPError({ + statusCode: 500, + statusText: `IPX_IP_CHECK_UNAVAILABLE`, + message: `Cannot pin the connection to a validated address: \`blockPrivateIPs\` requires \`node:dns\`.`, + }); + } return fetch; } @@ -435,6 +443,13 @@ export function ipxHttpStorage(_options: HTTPStorageOptions = {}): IPXStorage { try { ({ Agent } = requireModule<{ Agent: any }>("undici")); } catch { + if (blockPrivateIPs) { + throw new HTTPError({ + statusCode: 500, + statusText: `IPX_IP_CHECK_UNAVAILABLE`, + message: `Cannot pin the connection to a validated address: \`blockPrivateIPs\` requires \`undici\`.`, + }); + } return fetch; } From d22f4f8c8313d1268eb2a4d061f99cd265958196 Mon Sep 17 00:00:00 2001 From: presendapp Date: Fri, 18 Sep 2026 19:44:27 +0200 Subject: [PATCH 3/3] chore: align Node engine minimum with undici@7.29.1 requirement (20.16.0 -> 20.18.1) Per CodeRabbit review: undici@7.29.1 requires Node >=20.18.1, but the engines field still allowed 20.16.0. Verified: tsc --noEmit, eslint, prettier all clean; vitest run test/storage/http.test.ts: 90/90 passing. --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index f5beda2..25b187a 100644 --- a/package.json +++ b/package.json @@ -64,7 +64,7 @@ } }, "engines": { - "node": "^20.16.0 || >=22.3.0" + "node": "^20.18.1 || >=22.3.0" }, "packageManager": "pnpm@11.17.0" }