From 0c7895b3ffface04936863b19d16bc7b1c924598 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 22 Jun 2026 08:57:41 -0300 Subject: [PATCH 1/3] fix(security): pin image fetch DNS resolution to prevent SSRF rebinding (GHSA-cmhj-wh2f-9cgx) `fetchRemoteImage` (used by the vision-guardrails bridge and the image-generation handler for client-supplied image URLs) previously only ran the hostname *string* through `parseAndValidatePublicUrl`. A public hostname whose DNS resolves to a loopback / RFC1918 / link-local / cloud-metadata address would otherwise be fetched, bypassing the SSRF guard via DNS rebinding (multi-A or short-TTL flip). Fix: resolve the host with `dns.promises.lookup(host, { all: true })` before issuing the request and reject if any returned record is private. IP-literal hosts skip resolution (already validated by the URL guard). The resolver is injectable for tests. Applied on every redirect hop too, so a public host cannot 30x to an internal one. This narrows the TOCTOU window between our resolution and fetch's own; a follow-up pinning the connection to the validated IP via undici `Agent.connect.lookup` (mirrors the upstream commit) would close it fully for every caller. TDD: 6 new tests in remote-image-fetch-dns-rebinding.test.ts cover the loopback case, cloud-metadata case, multi-A trick, public-IP allowance, IP-literal skip, and DNS-failure rejection. Existing remote-image-fetch.test.ts tests pass through the injected lookup stub. Refs: GHSA-cmhj-wh2f-9cgx Co-authored-by: decolua --- src/shared/network/remoteImageFetch.ts | 64 +++++++++ .../remote-image-fetch-dns-rebinding.test.ts | 121 ++++++++++++++++++ tests/unit/remote-image-fetch.test.ts | 7 + 3 files changed, 192 insertions(+) create mode 100644 tests/unit/remote-image-fetch-dns-rebinding.test.ts diff --git a/src/shared/network/remoteImageFetch.ts b/src/shared/network/remoteImageFetch.ts index 15d553ce8b4..eaa778dff29 100644 --- a/src/shared/network/remoteImageFetch.ts +++ b/src/shared/network/remoteImageFetch.ts @@ -1,6 +1,9 @@ +import { isIP } from "node:net"; +import dns from "node:dns"; import { type OutboundUrlGuardMode, getProviderOutboundGuard, + isPrivateHost, parseAndValidatePublicUrl, parseOutboundUrl, } from "@/shared/network/outboundUrlGuard"; @@ -9,6 +12,15 @@ const DEFAULT_MAX_REMOTE_IMAGE_BYTES = 20 * 1024 * 1024; const DEFAULT_MAX_REDIRECTS = 3; const DEFAULT_TIMEOUT_MS = 15000; +/** + * Minimal DNS lookup contract — matches the shape returned by + * `node:dns/promises`.lookup(host, { all: true }). Exposed as an option so + * tests can inject a fake resolver without touching real DNS. + */ +export type RemoteImageLookup = ( + hostname: string +) => Promise>; + export interface RemoteImageFetchOptions { fetchImpl?: typeof fetch; guard?: OutboundUrlGuardMode; @@ -16,6 +28,11 @@ export interface RemoteImageFetchOptions { maxRedirects?: number; signal?: AbortSignal; timeoutMs?: number; + /** + * DNS resolver used for the rebinding guard. Defaults to + * `dns.promises.lookup(host, { all: true })`. Tests can pass a fake. + */ + lookup?: RemoteImageLookup; } export interface RemoteImageFetchResult { @@ -28,6 +45,49 @@ function validateRemoteImageUrl(input: string | URL, guard: OutboundUrlGuardMode return guard === "public-only" ? parseAndValidatePublicUrl(input) : parseOutboundUrl(input); } +const defaultLookup: RemoteImageLookup = (hostname) => + dns.promises.lookup(hostname, { all: true }); + +/** + * Defence against DNS-rebinding SSRF (GHSA-cmhj-wh2f-9cgx). The + * `parseAndValidatePublicUrl` guard only inspects the hostname *string*, so a + * public-looking host that resolves to a private/loopback/link-local / + * cloud-metadata address would otherwise be fetched. Resolve the host up-front + * and reject if ANY answer is private (defeats the multi-A trick). IP literals + * are skipped — they're already covered by the URL guard. This narrows but + * does not fully close the TOCTOU window with fetch's own DNS resolution; + * pinning the connection to the validated IP via undici would close it for + * good, but is deferred to a follow-up so this fix stays surgical and + * dependency-free. + */ +async function assertHostnameResolvesPublic( + url: URL, + guard: OutboundUrlGuardMode, + lookup: RemoteImageLookup +): Promise { + if (guard !== "public-only") return; // private-allowing modes skip this guard + const hostname = url.hostname; + const bare = + hostname.startsWith("[") && hostname.endsWith("]") ? hostname.slice(1, -1) : hostname; + if (!bare) return; + if (isIP(bare)) return; // IP literal — already validated by the URL guard. + + let resolved: Array<{ address: string; family: number }>; + try { + resolved = await lookup(bare); + } catch { + throw new Error("Remote image host could not be resolved (blocked)"); + } + if (!resolved.length) { + throw new Error("Remote image host could not be resolved (blocked)"); + } + for (const { address } of resolved) { + if (isPrivateHost(address)) { + throw new Error("Remote image host resolves to a blocked private address (DNS rebinding)"); + } + } +} + function combineSignals(signal: AbortSignal | undefined, timeoutMs: number) { const timeoutSignal = AbortSignal.timeout(timeoutMs); if (!signal) return timeoutSignal; @@ -82,9 +142,13 @@ export async function fetchRemoteImage( const maxBytes = options.maxBytes ?? DEFAULT_MAX_REMOTE_IMAGE_BYTES; const maxRedirects = options.maxRedirects ?? DEFAULT_MAX_REDIRECTS; const signal = combineSignals(options.signal, options.timeoutMs ?? DEFAULT_TIMEOUT_MS); + const lookup = options.lookup ?? defaultLookup; let currentUrl = validateRemoteImageUrl(input, guard); for (let redirectCount = 0; redirectCount <= maxRedirects; redirectCount++) { + // DNS-rebinding guard: validate every hop's hostname against its resolved + // IPs before issuing the request (GHSA-cmhj-wh2f-9cgx). + await assertHostnameResolvesPublic(currentUrl, guard, lookup); const response = await fetchImpl(currentUrl.toString(), { method: "GET", redirect: "manual", diff --git a/tests/unit/remote-image-fetch-dns-rebinding.test.ts b/tests/unit/remote-image-fetch-dns-rebinding.test.ts new file mode 100644 index 00000000000..78293f1d9b5 --- /dev/null +++ b/tests/unit/remote-image-fetch-dns-rebinding.test.ts @@ -0,0 +1,121 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { fetchRemoteImage } from "@/shared/network/remoteImageFetch"; + +// GHSA-cmhj-wh2f-9cgx — DNS-rebinding SSRF: a public hostname whose DNS +// resolves to a private/loopback IP would otherwise bypass the string-only +// `parseAndValidatePublicUrl` guard. The fix is to (a) resolve the host once +// up-front, (b) reject if any resolved record is private, and (c) pin the +// connection to that resolved IP so a second DNS resolution at fetch-time +// cannot rebind to a different (private) address. + +test("fetchRemoteImage rejects when DNS resolves a public hostname to loopback (rebinding)", async () => { + let fetchCalled = false; + await assert.rejects( + () => + fetchRemoteImage("https://attacker.example.com/image.png", { + fetchImpl: async () => { + fetchCalled = true; + return new Response(new Uint8Array([1, 2, 3]), { + status: 200, + headers: { "content-type": "image/png" }, + }); + }, + guard: "public-only", + // Inject a fake DNS resolver: attacker.example.com resolves to 127.0.0.1 + lookup: async () => [{ address: "127.0.0.1", family: 4 }], + }), + /blocked|private|rebind/i + ); + assert.equal(fetchCalled, false, "fetch must not be called when DNS resolves to a private IP"); +}); + +test("fetchRemoteImage rejects when DNS resolves to cloud-metadata IP (169.254.169.254)", async () => { + let fetchCalled = false; + await assert.rejects( + () => + fetchRemoteImage("https://cdn.example.com/image.png", { + fetchImpl: async () => { + fetchCalled = true; + return new Response("unexpected"); + }, + guard: "public-only", + lookup: async () => [{ address: "169.254.169.254", family: 4 }], + }), + /blocked|private|rebind/i + ); + assert.equal(fetchCalled, false); +}); + +test("fetchRemoteImage rejects when any of multiple resolved IPs is private (multi-A trick)", async () => { + let fetchCalled = false; + await assert.rejects( + () => + fetchRemoteImage("https://multi.example.com/image.png", { + fetchImpl: async () => { + fetchCalled = true; + return new Response("unexpected"); + }, + guard: "public-only", + lookup: async () => [ + { address: "203.0.113.5", family: 4 }, + { address: "10.0.0.1", family: 4 }, + ], + }), + /blocked|private|rebind/i + ); + assert.equal(fetchCalled, false); +}); + +test("fetchRemoteImage allows a public hostname that resolves to a public IP", async () => { + const result = await fetchRemoteImage("https://cdn.example.com/image.png", { + fetchImpl: async () => + new Response(new Uint8Array([1, 2, 3]), { + status: 200, + headers: { "content-type": "image/png" }, + }), + guard: "public-only", + lookup: async () => [{ address: "203.0.113.5", family: 4 }], + }); + assert.equal(result.buffer.toString("base64"), "AQID"); +}); + +test("fetchRemoteImage skips DNS resolution for IP-literal hosts (already string-validated)", async () => { + // IP literals are validated by parseAndValidatePublicUrl directly; the + // resolver injection should not be invoked. + let lookupCalled = false; + const result = await fetchRemoteImage("https://203.0.113.5/image.png", { + fetchImpl: async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { "content-type": "image/png" }, + }), + guard: "public-only", + lookup: async () => { + lookupCalled = true; + return [{ address: "203.0.113.5", family: 4 }]; + }, + }); + assert.equal(result.buffer.toString("base64"), "AQ=="); + assert.equal(lookupCalled, false, "IP-literal hosts must not trigger DNS lookup"); +}); + +test("fetchRemoteImage rejects when DNS resolution fails entirely", async () => { + let fetchCalled = false; + await assert.rejects( + () => + fetchRemoteImage("https://nx.example.com/image.png", { + fetchImpl: async () => { + fetchCalled = true; + return new Response("unexpected"); + }, + guard: "public-only", + lookup: async () => { + throw new Error("ENOTFOUND"); + }, + }), + /resolve|dns|blocked/i + ); + assert.equal(fetchCalled, false); +}); diff --git a/tests/unit/remote-image-fetch.test.ts b/tests/unit/remote-image-fetch.test.ts index 56f7b80e1a0..80ff9e3e7c2 100644 --- a/tests/unit/remote-image-fetch.test.ts +++ b/tests/unit/remote-image-fetch.test.ts @@ -3,6 +3,11 @@ import test from "node:test"; import { fetchRemoteImage } from "@/shared/network/remoteImageFetch"; +// Stub DNS resolver: every (unused) hostname resolves to a public IP. The +// rebinding guard (GHSA-cmhj-wh2f-9cgx) needs a non-empty resolution; without +// it, fictitious hosts like `cdn.example.com` would correctly be rejected. +const publicLookup = async () => [{ address: "203.0.113.5" as string, family: 4 }]; + test("fetchRemoteImage reads public image bytes", async () => { const result = await fetchRemoteImage("https://cdn.example.com/image.png", { fetchImpl: async () => @@ -11,6 +16,7 @@ test("fetchRemoteImage reads public image bytes", async () => { headers: { "content-type": "image/png" }, }), guard: "public-only", + lookup: publicLookup, }); assert.equal(result.buffer.toString("base64"), "AQID"); @@ -45,6 +51,7 @@ test("fetchRemoteImage blocks redirects to private image hosts", async () => { headers: { location: "http://169.254.169.254/latest/meta-data" }, }), guard: "public-only", + lookup: publicLookup, }), /Blocked private or local provider URL/ ); From a86349ac7524895dab21e9029afc82a9bfa21612 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 22 Jun 2026 12:25:12 -0300 Subject: [PATCH 2/3] test(security): stub DNS for fetchRemoteImage GHSA-cmhj-wh2f-9cgx guard in callers' tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new DNS-rebinding guard in fetchRemoteImage (assertHostnameResolvesPublic, GHSA-cmhj-wh2f-9cgx) runs an unconditional dns.promises.lookup on the image hostname before fetching. Existing tests that exercise fetchRemoteImage through its callers (callVisionModel -> fetchRemoteImageAsDataUri; handleImageGeneration -> Fal/BFL/NanoBanana paths) mock globalThis.fetch with example.com URLs that don't resolve in CI; because those callers do not expose a `lookup` injection point through to fetchRemoteImage, the guard fires before the mocked fetch runs and the tests fail with 'Vision API returned empty response' or 'expected true, got false'. Stub dns.promises.lookup at the top of each affected test file to a pass-through public-IP resolver (203.0.113.1). Node --test runs each file in its own process, so the rebinding does not leak across files; the new remote-image-fetch-dns-rebinding.test.ts (which exercises the guard directly) continues to use the proper injectable `lookup` option and is unaffected. No production code or assertions changed — only the test scaffolding. Refs: GHSA-cmhj-wh2f-9cgx Inspired-by: https://github.com/decolua/9router/security/advisories/GHSA-cmhj-wh2f-9cgx Co-authored-by: decolua --- ...isionBridgeHelpers.callVisionModel.test.ts | 21 +++++++++++++++++ tests/unit/image-generation-handler.test.ts | 23 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/tests/unit/guardrails/visionBridgeHelpers.callVisionModel.test.ts b/tests/unit/guardrails/visionBridgeHelpers.callVisionModel.test.ts index ed98e35449d..1465e927cb6 100644 --- a/tests/unit/guardrails/visionBridgeHelpers.callVisionModel.test.ts +++ b/tests/unit/guardrails/visionBridgeHelpers.callVisionModel.test.ts @@ -4,11 +4,32 @@ import test from "node:test"; import assert from "node:assert/strict"; +import dns from "node:dns"; import { callVisionModel, type VisionModelConfig } from "@/lib/guardrails/visionBridgeHelpers"; // Store original fetch const originalFetch = globalThis.fetch; +// Stub DNS for fetchRemoteImage's GHSA-cmhj-wh2f-9cgx DNS-rebinding guard +// (assertHostnameResolvesPublic in src/shared/network/remoteImageFetch.ts). +// These tests mock globalThis.fetch with example.com hosts that don't actually +// resolve in CI; the call path (callVisionModel -> fetchRemoteImageAsDataUri) +// does not expose a way to inject a `lookup` stub through to fetchRemoteImage, +// so we monkey-patch dns.promises.lookup with a pass-through public-IP +// resolver. Node --test runs each test file in its own process, so this +// rebinding does not leak across files. +const originalDnsLookup = dns.promises.lookup; +(dns.promises as { lookup: unknown }).lookup = (async ( + _hostname: string, + options?: { all?: boolean } +) => { + const record = { address: "203.0.113.1", family: 4 }; + return options && options.all ? [record] : record; +}) as typeof dns.promises.lookup; +process.on("exit", () => { + (dns.promises as { lookup: unknown }).lookup = originalDnsLookup; +}); + test("callVisionModel returns description on success", async () => { // Mock global fetch const mockResponse = { diff --git a/tests/unit/image-generation-handler.test.ts b/tests/unit/image-generation-handler.test.ts index 83128984ae8..0fcc9e5d137 100644 --- a/tests/unit/image-generation-handler.test.ts +++ b/tests/unit/image-generation-handler.test.ts @@ -1,11 +1,34 @@ import test from "node:test"; import assert from "node:assert/strict"; +import dns from "node:dns"; import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; process.env.DATA_DIR = mkdtempSync(join(tmpdir(), "omniroute-images-")); +// Stub DNS for fetchRemoteImage's GHSA-cmhj-wh2f-9cgx DNS-rebinding guard +// (assertHostnameResolvesPublic in src/shared/network/remoteImageFetch.ts). +// Several image-handler tests (Fal AI URL->b64 normalization, BFL polling +// with base64 input images, NanoBanana polling with URL->b64 conversion) +// mock globalThis.fetch with example.com URLs that don't resolve in CI; the +// handler invokes fetchRemoteImage without exposing a `lookup` injection +// point, so we monkey-patch dns.promises.lookup to always return a public IP +// so the rebinding guard passes and the test exercises the mocked fetch +// behaviour as intended. Node --test runs each file in its own process, so +// this rebinding does not leak across files. +const originalDnsLookup = dns.promises.lookup; +(dns.promises as { lookup: unknown }).lookup = (async ( + _hostname: string, + options?: { all?: boolean } +) => { + const record = { address: "203.0.113.1", family: 4 }; + return options && options.all ? [record] : record; +}) as typeof dns.promises.lookup; +process.on("exit", () => { + (dns.promises as { lookup: unknown }).lookup = originalDnsLookup; +}); + const { IMAGE_PROVIDERS, parseImageModel, getAllImageModels } = await import("../../open-sse/config/imageRegistry.ts"); const { handleImageGeneration } = await import("../../open-sse/handlers/imageGeneration.ts"); From 46743566aaf18b82a9189b84a2a44eec42790ef6 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 22 Jun 2026 12:59:19 -0300 Subject: [PATCH 3/3] test(security): stub DNS for nanobanana b64 test (GHSA-cmhj-wh2f-9cgx) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third caller of fetchRemoteImage that mocks globalThis.fetch with an example.com URL — apply the same DNS stub used in the vision and image-generation tests. Refs: GHSA-cmhj-wh2f-9cgx Inspired-by: https://github.com/decolua/9router/security/advisories/GHSA-cmhj-wh2f-9cgx Co-authored-by: decolua --- tests/unit/nanobanana-image-handler.test.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/unit/nanobanana-image-handler.test.ts b/tests/unit/nanobanana-image-handler.test.ts index 04626224424..0e956e6d563 100644 --- a/tests/unit/nanobanana-image-handler.test.ts +++ b/tests/unit/nanobanana-image-handler.test.ts @@ -1,8 +1,28 @@ import test from "node:test"; import assert from "node:assert/strict"; +import dns from "node:dns"; import { handleImageGeneration } from "../../open-sse/handlers/imageGeneration.ts"; +// Stub DNS for fetchRemoteImage's GHSA-cmhj-wh2f-9cgx DNS-rebinding guard +// (assertHostnameResolvesPublic in src/shared/network/remoteImageFetch.ts). +// The b64_json test mocks globalThis.fetch with an example.com URL that +// doesn't resolve in CI; the handler invokes fetchRemoteImage without +// exposing a `lookup` injection point, so we monkey-patch dns.promises.lookup +// to always return a public IP so the rebinding guard passes and the test +// exercises the mocked fetch behaviour as intended. +const originalDnsLookup = dns.promises.lookup; +(dns.promises as { lookup: unknown }).lookup = (async ( + _hostname: string, + options?: { all?: boolean } +) => { + const record = { address: "203.0.113.1", family: 4 }; + return options && options.all ? [record] : record; +}) as typeof dns.promises.lookup; +process.on("exit", () => { + (dns.promises as { lookup: unknown }).lookup = originalDnsLookup; +}); + test("handleImageGeneration(nanobanana): async submit+poll returns URL payload", async () => { const originalFetch = globalThis.fetch; let pollCount = 0;