From b43b57f65730a562a734535b0994ab2ac7fda159 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 28 May 2026 22:17:50 +0200 Subject: [PATCH] feat(security): pin resolved IP on outbound fetches to defeat DNS rebinding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes issue 217. The isPublicUrl input-time guard catches every RFC1918 / CGNAT / loopback / link-local alt-notation, but a low-TTL DNS record can flip between accept and dispatch. The new pinnedPublicDispatcher resolves the hostname literally inside undici.Agent's connect hook, refuses any address isPublicIp would reject, and pins the connection to the first valid public address. The five outbound paths that accept a user-supplied host route through the dispatcher via safeFetch's requirePublicHost flag — moodlog sync, moodlog reverse-push, the moodlog credentials probe, the local AI client (already gated in commit 1), and the ntfy sender. The flag now buys both the input-time check and the connect-time pin in one call. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../api/integrations/moodlog/test/route.ts | 6 +- .../__tests__/safe-fetch-dispatcher.test.ts | 170 ++++++++++++++++++ src/lib/moodlog/push.ts | 6 +- src/lib/moodlog/sync.ts | 7 +- src/lib/notifications/senders/ntfy.ts | 7 +- src/lib/safe-fetch-dispatcher.ts | 106 +++++++++++ src/lib/safe-fetch.ts | 18 +- src/lib/validations/notifications.ts | 68 +++++++ 8 files changed, 381 insertions(+), 7 deletions(-) create mode 100644 src/lib/__tests__/safe-fetch-dispatcher.test.ts create mode 100644 src/lib/safe-fetch-dispatcher.ts diff --git a/src/app/api/integrations/moodlog/test/route.ts b/src/app/api/integrations/moodlog/test/route.ts index 35eb80ed5..36b20a09e 100644 --- a/src/app/api/integrations/moodlog/test/route.ts +++ b/src/app/api/integrations/moodlog/test/route.ts @@ -116,7 +116,9 @@ export const POST = apiHandler(async (request: NextRequest) => { try { // safeFetch keeps the manual-redirect guard (the apiKey would // otherwise leak on a 302 to 169.254.169.254 / RFC1918) and adds - // the AbortSignal.timeout convention. + // the AbortSignal.timeout convention. requirePublicHost layers in + // the connect-time IP pin so a DNS rebinding cannot flip the host + // between accept and dispatch (issue #217). const res = await safeFetch( probeUrl.toString(), { @@ -124,7 +126,7 @@ export const POST = apiHandler(async (request: NextRequest) => { headers: { Authorization: `Bearer ${apiKey}` }, cache: "no-store", }, - { timeoutMs: TIMEOUT_MS }, + { timeoutMs: TIMEOUT_MS, requirePublicHost: true }, ); const latencyMs = Math.round(performance.now() - start); diff --git a/src/lib/__tests__/safe-fetch-dispatcher.test.ts b/src/lib/__tests__/safe-fetch-dispatcher.test.ts new file mode 100644 index 000000000..c2c455ce8 --- /dev/null +++ b/src/lib/__tests__/safe-fetch-dispatcher.test.ts @@ -0,0 +1,170 @@ +import { describe, it, expect, afterEach, vi } from "vitest"; +import dns from "node:dns"; +import { isPublicIp } from "@/lib/validations/notifications"; +import { + getPinnedPublicDispatcher, + _resetPinnedDispatcherForTests, +} from "../safe-fetch-dispatcher"; + +describe("isPublicIp", () => { + it("accepts a public IPv4 address", () => { + expect(isPublicIp("203.0.113.5")).toBe(true); + expect(isPublicIp("8.8.8.8")).toBe(true); + }); + + it("rejects RFC1918 IPv4", () => { + expect(isPublicIp("10.0.0.1")).toBe(false); + expect(isPublicIp("192.168.0.1")).toBe(false); + expect(isPublicIp("172.16.0.1")).toBe(false); + }); + + it("rejects loopback + reserved IPv4", () => { + expect(isPublicIp("127.0.0.1")).toBe(false); + expect(isPublicIp("0.0.0.0")).toBe(false); + }); + + it("rejects 169.254/16 cloud-metadata", () => { + expect(isPublicIp("169.254.169.254")).toBe(false); + }); + + it("rejects CGNAT 100.64/10", () => { + expect(isPublicIp("100.64.0.1")).toBe(false); + expect(isPublicIp("100.127.255.255")).toBe(false); + }); + + it("rejects IPv6 loopback + link-local + ULA", () => { + expect(isPublicIp("::1")).toBe(false); + expect(isPublicIp("fe80::1")).toBe(false); + expect(isPublicIp("fc00::1")).toBe(false); + expect(isPublicIp("fd12:3456:789a::1")).toBe(false); + }); + + it("accepts a public IPv6 address", () => { + expect(isPublicIp("2001:db8::1")).toBe(true); + expect(isPublicIp("2606:4700:4700::1111")).toBe(true); + }); + + it("rejects IPv4-mapped IPv6 that wraps a private IPv4", () => { + expect(isPublicIp("::ffff:127.0.0.1")).toBe(false); + expect(isPublicIp("::ffff:10.0.0.1")).toBe(false); + }); + + it("accepts IPv4-mapped IPv6 that wraps a public IPv4", () => { + expect(isPublicIp("::ffff:8.8.8.8")).toBe(true); + }); + + it("rejects empty input", () => { + expect(isPublicIp("")).toBe(false); + }); +}); + +describe("pinnedPublicDispatcher", () => { + afterEach(() => { + vi.restoreAllMocks(); + _resetPinnedDispatcherForTests(); + }); + + it("refuses a hostname that resolves to a private IP", async () => { + // Mock dns.lookup to return 169.254.169.254 (cloud metadata). + const lookupSpy = vi.spyOn(dns, "lookup"); + lookupSpy.mockImplementation( + (( + _hostname: string, + _opts: dns.LookupAllOptions, + callback: ( + err: NodeJS.ErrnoException | null, + addresses: dns.LookupAddress[], + ) => void, + ) => { + callback(null, [{ address: "169.254.169.254", family: 4 }]); + }) as unknown as typeof dns.lookup, + ); + + const dispatcher = getPinnedPublicDispatcher(); + // Drive the dispatcher through `fetch` so the integration mirrors + // what safeFetch does. The expected outcome is a connect error + // (the lookup wrapper raised ENOTFOUND). + await expect( + fetch( + "https://attacker-controlled.example.test/probe", + // Node's fetch RequestInit accepts undici's `dispatcher`; the + // DOM types do not, hence the cast. + { dispatcher } as RequestInit & { dispatcher: typeof dispatcher }, + ), + ).rejects.toThrow(); + }, 20_000); + + it("forwards a public-resolving hostname to the real connector", async () => { + // Mock dns.lookup to claim the host resolves to 192.0.2.1 — a + // documented public range (TEST-NET-1) that is also unroutable. + // The expected outcome is therefore NOT ENOTFOUND but a connect + // failure further down the stack (the IP refuses or times out). + const lookupSpy = vi.spyOn(dns, "lookup"); + lookupSpy.mockImplementation( + (( + _hostname: string, + _opts: dns.LookupAllOptions, + callback: ( + err: NodeJS.ErrnoException | null, + addresses: dns.LookupAddress[], + ) => void, + ) => { + callback(null, [{ address: "192.0.2.1", family: 4 }]); + }) as unknown as typeof dns.lookup, + ); + + const dispatcher = getPinnedPublicDispatcher(); + let caught: unknown; + try { + await fetch("https://example.test/probe", { + signal: AbortSignal.timeout(500), + dispatcher, + } as RequestInit & { dispatcher: typeof dispatcher }); + } catch (e) { + caught = e; + } + + // The pinned lookup must have ACCEPTED the address (no ENOTFOUND); + // the connect failure surfaces as a timeout / connect refused. + expect(caught).toBeDefined(); + const code = (caught as NodeJS.ErrnoException)?.code; + expect(code === "ENOTFOUND").toBe(false); + }, 20_000); + + it("filters a mixed result so only the public address is pinned", async () => { + // Mock dns.lookup to return one private + one public address. The + // dispatcher must drop the private one and pin to the public one. + const lookupSpy = vi.spyOn(dns, "lookup"); + lookupSpy.mockImplementation( + (( + _hostname: string, + _opts: dns.LookupAllOptions, + callback: ( + err: NodeJS.ErrnoException | null, + addresses: dns.LookupAddress[], + ) => void, + ) => { + callback(null, [ + { address: "10.0.0.5", family: 4 }, + { address: "192.0.2.1", family: 4 }, + ]); + }) as unknown as typeof dns.lookup, + ); + + const dispatcher = getPinnedPublicDispatcher(); + let caught: unknown; + try { + await fetch("https://example.test/probe", { + signal: AbortSignal.timeout(500), + dispatcher, + } as RequestInit & { dispatcher: typeof dispatcher }); + } catch (e) { + caught = e; + } + + // Again: the dispatch must NOT short-circuit with ENOTFOUND — the + // public alternate is what gets dialled. + const code = (caught as NodeJS.ErrnoException)?.code; + expect(code === "ENOTFOUND").toBe(false); + }, 20_000); +}); diff --git a/src/lib/moodlog/push.ts b/src/lib/moodlog/push.ts index a4c2382a7..32363bdbe 100644 --- a/src/lib/moodlog/push.ts +++ b/src/lib/moodlog/push.ts @@ -114,6 +114,10 @@ export async function pushMoodEntriesToMoodLog( const start = performance.now(); let response: Response; try { + // requirePublicHost adds the DNS-rebinding pin to the input-time + // isPublicUrl guard (issue #217). The reverse-sync path attaches a + // user-supplied bearer; a rebinding flip would leak it on the + // connect call to the swapped private address. response = await safeFetch( url.toString(), { @@ -124,7 +128,7 @@ export async function pushMoodEntriesToMoodLog( }, body: JSON.stringify(body), }, - { timeoutMs: 10_000 }, + { timeoutMs: 10_000, requirePublicHost: true }, ); } catch (err) { const message = err instanceof Error ? err.message : String(err); diff --git a/src/lib/moodlog/sync.ts b/src/lib/moodlog/sync.ts index ccd238057..b23e3b64b 100644 --- a/src/lib/moodlog/sync.ts +++ b/src/lib/moodlog/sync.ts @@ -90,7 +90,10 @@ export async function syncMoodLogEntries( // safeFetch keeps the no-redirect-follow guard (the apiKey would // otherwise leak on an attacker-controlled 302 hop) and adds a hard - // upper bound via AbortSignal.timeout. + // upper bound via AbortSignal.timeout. requirePublicHost routes the + // dial through the pinned-IP dispatcher so a DNS rebinding cannot + // flip a public hostname to 169.254.169.254 / RFC1918 between the + // input-time accept and the connect call. const timeoutMs = opts?.fullSync ? 60_000 : 15_000; let response: Response; const fetchStart = performance.now(); @@ -98,7 +101,7 @@ export async function syncMoodLogEntries( response = await safeFetch( url.toString(), { headers: { Authorization: `Bearer ${apiKey}` } }, - { timeoutMs }, + { timeoutMs, requirePublicHost: true }, ); } catch (err) { getEvent()?.addExternalCall({ diff --git a/src/lib/notifications/senders/ntfy.ts b/src/lib/notifications/senders/ntfy.ts index 608cb8c43..05248d381 100644 --- a/src/lib/notifications/senders/ntfy.ts +++ b/src/lib/notifications/senders/ntfy.ts @@ -39,6 +39,11 @@ export async function sendViaNtfy( // Strip HTML tags for ntfy (plain text only) const body = payload.message.replace(/<[^>]*>/g, ""); + // requirePublicHost adds the DNS-rebinding pin (issue #217) on top + // of the input-time isPublicUrl guard already enforced when the + // user saved the ntfy serverUrl in their channel config. The + // authToken would otherwise leak on a rebinding flip to a private + // address. const res = await safeFetch( url, { @@ -46,7 +51,7 @@ export async function sendViaNtfy( headers, body, }, - { timeoutMs: 5_000 }, + { timeoutMs: 5_000, requirePublicHost: true }, ); getEvent()?.addExternalCall({ diff --git a/src/lib/safe-fetch-dispatcher.ts b/src/lib/safe-fetch-dispatcher.ts new file mode 100644 index 000000000..1919674cb --- /dev/null +++ b/src/lib/safe-fetch-dispatcher.ts @@ -0,0 +1,106 @@ +import dns from "node:dns"; +import { Agent } from "undici"; +import { isPublicIp } from "@/lib/validations/notifications"; + +/** + * Custom `undici.Agent` that resolves the request hostname literally, + * vets every returned address against `isPublicIp`, and pins the + * connection to the first allowed address. + * + * This closes the DNS-rebinding gap that `isPublicUrl` cannot reach + * (issue #217). The input-time check accepts a hostname that resolves + * to a public address; the resolver is then free to flip the record to + * `169.254.169.254` (cloud metadata) or `10.0.0.x` (operator admin + * panel) by the time the connection dials. With a TTL of 0 the attack + * is reliable. + * + * Mechanism: + * + * 1. Override `connect.lookup` with a wrapper that always runs + * `dns.lookup({ all: true })` so we see every address the resolver + * would have returned. + * 2. Filter that list through `isPublicIp` — same allowlist + * `isPublicUrl` enforces, applied to a literal IP address rather + * than the input string. + * 3. If at least one address passes, hand the single first survivor + * to undici. Undici then pins the TCP connect to that exact IP, + * side-stepping any further resolver hop. + * 4. If no address passes, fail closed with `dns.NOTFOUND` so the + * dispatch surfaces as a normal connect error rather than a silent + * bypass. + * + * Wired into `safeFetch` only when `opts.requirePublicHost` is true. + * The five outbound paths that accept a user-supplied host all use + * that flag. + */ +function pinnedLookup( + hostname: string, + options: dns.LookupOptions, + callback: ( + err: NodeJS.ErrnoException | null, + address: string | dns.LookupAddress[], + family?: number, + ) => void, +): void { + // Always ask for every address so the filter sees the full set; + // discarding alternates a low-TTL attacker could swap in is exactly + // the point of the pin. + const opts: dns.LookupAllOptions = { + family: options.family ?? 0, + hints: options.hints, + verbatim: options.verbatim, + all: true, + }; + + dns.lookup(hostname, opts, (err, addresses) => { + if (err) { + callback(err, ""); + return; + } + const allowed = addresses.filter((a) => isPublicIp(a.address)); + if (allowed.length === 0) { + const refused: NodeJS.ErrnoException = new Error( + `safeFetch refused private resolved address for ${hostname}`, + ); + refused.code = "ENOTFOUND"; + callback(refused, ""); + return; + } + + const picked = allowed[0]; + if (options.all) { + // Caller asked for the array shape — pass through the survivors. + callback(null, allowed); + return; + } + callback(null, picked.address, picked.family); + }); +} + +/** + * Lazily-instantiated singleton. `undici.Agent` keeps an internal pool + * per origin, so reusing one instance across calls preserves the keep- + * alive benefit while still pinning each new connection through the + * vetted lookup. + */ +let cached: Agent | null = null; + +export function getPinnedPublicDispatcher(): Agent { + if (cached) return cached; + cached = new Agent({ + connect: { + lookup: pinnedLookup, + }, + }); + return cached; +} + +/** + * Test helper. The dispatcher is module-state; tests that mock + * `dns.lookup` need to clear the cached instance so the next call + * binds the mock. + */ +export function _resetPinnedDispatcherForTests(): void { + cached?.destroy().catch(() => {}); + cached = null; +} diff --git a/src/lib/safe-fetch.ts b/src/lib/safe-fetch.ts index 86b0754b5..af06ec094 100644 --- a/src/lib/safe-fetch.ts +++ b/src/lib/safe-fetch.ts @@ -1,4 +1,5 @@ import { isPublicUrl } from "@/lib/validations/notifications"; +import { getPinnedPublicDispatcher } from "@/lib/safe-fetch-dispatcher"; /** * Defaults applied to every `safeFetch` call. Centralised so the values @@ -115,8 +116,23 @@ export async function safeFetch( opts.timeoutMs ?? DEFAULT_TIMEOUT_MS, ); + // When the caller asks for the public-host guard, also route through + // the pinned-IP dispatcher so a DNS rebinding cannot flip the host + // between the input-time `isPublicUrl` accept and the connect call. + // Node's `fetch` accepts an undici `dispatcher` on RequestInit; the + // DOM types don't reflect that, so we cast at the boundary. + const dispatcher = opts.requirePublicHost + ? getPinnedPublicDispatcher() + : undefined; + const finalInit = { + ...init, + redirect, + signal, + ...(dispatcher ? { dispatcher } : {}), + } as RequestInit; + try { - return await fetch(target, { ...init, redirect, signal }); + return await fetch(target, finalInit); } catch (err) { // `AbortSignal.timeout` aborts with `TimeoutError` (`DOMException` // named "TimeoutError"); a caller-side abort surfaces as diff --git a/src/lib/validations/notifications.ts b/src/lib/validations/notifications.ts index 8f3f563fe..55d420063 100644 --- a/src/lib/validations/notifications.ts +++ b/src/lib/validations/notifications.ts @@ -61,6 +61,74 @@ function isPrivateIpv4(ip: [number, number, number, number]): boolean { return false; } +/** + * Decide whether a raw IP address string (as returned by `dns.lookup`) + * points at a public range — i.e. is safe to dial. + * + * Used by the pinned dispatcher (issue #217) inside undici's connect + * hook to defeat DNS rebinding: even when `isPublicUrl` accepted the + * hostname at input time, the resolver can flip between accept and + * dispatch. This second check runs against the literal address the + * connection would target. + * + * Accepts: + * - dotted-quad IPv4 ("203.0.113.5", "10.0.0.1") + * - IPv6 literal ("2001:db8::1", "::1", "fe80::1", "::ffff:127.0.0.1") + * + * Rejects every range `isPublicUrl` would reject at input time plus + * IPv4-mapped IPv6 (resolvers occasionally emit these). + */ +export function isPublicIp(ip: string): boolean { + if (!ip) return false; + const lower = ip.toLowerCase(); + + // IPv4 dotted-quad (what dns.lookup with family=4 returns). + if (/^\d+\.\d+\.\d+\.\d+$/.test(lower)) { + const parsed = parseIpv4Strict(lower); + if (!parsed) return false; + return !isPrivateIpv4(parsed); + } + + // IPv6. Only strings with a colon make it past the previous check. + if (lower.includes(":")) { + if (lower === "::1" || lower === "::") return false; + if (lower.startsWith("fe80:")) return false; + if (/^fc[0-9a-f]{0,2}:/.test(lower)) return false; + if (/^fd[0-9a-f]{0,2}:/.test(lower)) return false; + + // IPv4-mapped IPv6 dotted form ("::ffff:127.0.0.1"). Some resolvers + // emit this when an AAAA falls through to a synthesized A. + const mappedDotted = lower.match(/^(?:::ffff:)(\d+\.\d+\.\d+\.\d+)$/); + if (mappedDotted) { + const parsed = parseIpv4Strict(mappedDotted[1]); + if (!parsed || isPrivateIpv4(parsed)) return false; + return true; + } + + // IPv4-mapped IPv6 hex pair form ("::ffff:7f00:1"). + const mappedHex = lower.match( + /^(?:::ffff:)([0-9a-f]{1,4}):([0-9a-f]{1,4})$/, + ); + if (mappedHex) { + const high = parseInt(mappedHex[1], 16); + const low = parseInt(mappedHex[2], 16); + const parsed: [number, number, number, number] = [ + (high >> 8) & 0xff, + high & 0xff, + (low >> 8) & 0xff, + low & 0xff, + ]; + if (isPrivateIpv4(parsed)) return false; + return true; + } + return true; + } + + // Anything else (non-IP string, malformed) — refuse rather than + // dispatch. + return false; +} + export function isPublicUrl(url: string): boolean { try { // Pre-URL guard #1: the WHATWG URL parser interprets leading-zero IPv4