From 34209cd0dea5e3a067aa023065126a83a45ce95c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 20:56:57 +0000 Subject: [PATCH 1/2] test(web): cover SSRF guard resolution bypasses salvaged from #1428 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1428 set out to close the SSRF guard's DNS oracle (CWE-209). That fix landed first via #1381 (feae3d3), which rewrote the same two files and collapsed every DNS-path rejection into one `NOT_PUBLIC` literal. #1428 is therefore superseded on the security outcome and now conflicts with main. Its detection cases were not superseded, and nothing else covers them. `ssrf-guard-dns-leakage.test.ts` asks whether a rejection tells the caller too much; these ask the prior question of whether the guard rejects at all. Both scenarios are ways a private destination survives a resolution that looks public: one hides the address in an alternate IPv6 spelling, the other behind a public sibling record. A regression in either is silent — the guard returns a URL and the caller fetches it. Ported to the merged API, since #1428's cases were written against a `SsrfGuardError` type that #1381 did not introduce. Verified non-vacuous by mutation rather than assertion: - replacing the mapped-IPv4 decode with `return false` fails only the expanded IPv4-mapped IPv6 case (1 failed, 3 passed) - inspecting only `resolved[0]` fails only the two multi-answer cases (2 failed, 2 passed) The all-public control survives both, which is what makes it a control. Guard restored bit-for-bit after each mutation; no production code changes. 9 passed across both SSRF guard files; tsc --noEmit and eslint clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01MdQ7ABmZMShRGQznQbPS4v --- .../ssrf-guard-resolution-bypass.test.ts | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 apps/web/src/lib/__tests__/ssrf-guard-resolution-bypass.test.ts diff --git a/apps/web/src/lib/__tests__/ssrf-guard-resolution-bypass.test.ts b/apps/web/src/lib/__tests__/ssrf-guard-resolution-bypass.test.ts new file mode 100644 index 000000000..18cea6010 --- /dev/null +++ b/apps/web/src/lib/__tests__/ssrf-guard-resolution-bypass.test.ts @@ -0,0 +1,87 @@ +import { describe, it, expect, afterEach, vi } from 'vitest'; + +vi.mock('node:dns/promises', () => ({ + lookup: vi.fn(), +})); + +import * as dns from 'node:dns/promises'; +import { assertPublicHttpUrl } from '@/lib/ssrf-guard'; + +afterEach(() => { + vi.restoreAllMocks(); + vi.mocked(dns.lookup).mockReset(); +}); + +/** + * Detection-robustness coverage for `assertPublicHttpUrl`, distinct from + * `ssrf-guard-dns-leakage.test.ts`. + * + * That file asks whether a rejection *tells the caller too much*. These ask the + * prior question: does the guard reject at all? Both cases below are ways a + * private destination can survive a resolution that looks public at a glance — + * one hides the address inside an alternate IPv6 spelling, the other hides it + * behind a public sibling record. Neither is exercised anywhere else, and both + * are silent failures if they regress: the guard would return a URL and the + * caller would fetch it. + * + * Salvaged from #1428, whose CWE-209 fix was superseded by #1381 (feae3d3) but + * whose detection cases were not carried over. Ported to the merged API, where + * every DNS-path rejection is the single `NOT_PUBLIC` literal. + */ +const NOT_PUBLIC = 'Host does not resolve to a public address'; + +describe('assertPublicHttpUrl resolution bypasses', () => { + it('rejects a loopback address written in expanded IPv4-mapped IPv6 form', async () => { + // `0:0:0:0:0:ffff:7f00:1` is 127.0.0.1 spelled without `::` compression and + // without dotted-quad notation. A check that pattern-matched `::ffff:` or + // looked for dots would pass it straight through to a fetch of localhost. + vi.mocked(dns.lookup).mockResolvedValue([ + { address: '0:0:0:0:0:ffff:7f00:1', family: 6 }, + ] as never); + + await expect(assertPublicHttpUrl('https://sneaky.example.com/a.mp3')).rejects.toThrow( + NOT_PUBLIC, + ); + }); + + it('rejects when any resolved address is private, even if another is public', async () => { + // Multi-record DNS: the guard must scan every answer, not just the first. + // Returning after one public hit would let an attacker pair a real public A + // record with an internal one and win whichever the fetch layer picks. + vi.mocked(dns.lookup).mockResolvedValue([ + { address: '93.184.216.34', family: 4 }, + { address: '10.0.0.5', family: 4 }, + ] as never); + + await expect(assertPublicHttpUrl('https://mixed.example.com/a.mp3')).rejects.toThrow( + NOT_PUBLIC, + ); + }); + + it('rejects a private address that appears after several public ones', async () => { + // Guards against an off-by-one or early-exit that only inspects a prefix of + // the answer set. + vi.mocked(dns.lookup).mockResolvedValue([ + { address: '93.184.216.34', family: 4 }, + { address: '151.101.1.140', family: 4 }, + { address: '172.16.31.9', family: 4 }, + ] as never); + + await expect(assertPublicHttpUrl('https://tail.example.com/a.mp3')).rejects.toThrow( + NOT_PUBLIC, + ); + }); + + it('still allows a host whose answers are all public', async () => { + // The control: without it, a guard that rejected everything would pass the + // three assertions above. + vi.mocked(dns.lookup).mockResolvedValue([ + { address: '93.184.216.34', family: 4 }, + { address: '151.101.1.140', family: 4 }, + ] as never); + + const url = await assertPublicHttpUrl('https://public.example.com/a.mp3'); + + expect(url.hostname).toBe('public.example.com'); + }); +}); From 6b64f30d453c5bb835c7cf67cf2e01d7ab3995f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 21:01:28 +0000 Subject: [PATCH 2/2] test(web): assert the DNS branch is reached, not just its message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit's review finding on #1483. Each case asserted only that `assertPublicHttpUrl` throws the `NOT_PUBLIC` literal. That is load-bearing today, because a pre-DNS rejection throws a different literal (`Blocked host`) and would fail the assertion loudly. It stops being load-bearing the moment those literals are flattened into `NOT_PUBLIC` as well — then a hostname added to `BLOCKED_HOSTNAMES` would short-circuit before `dns.lookup` and the test would keep passing while testing nothing about resolution. That refactor is not hypothetical. #1428, the PR these cases came from, unified all six rejection paths onto one message, and `main`'s transcription-service already flattens all five at the call-site boundary. Asserting `dns.lookup` was called with the hostname and `{ all: true }` pins the branch under test, so the coverage survives that change. Verified the new assertions are themselves non-vacuous: short-circuiting `sneaky.example.com` before resolution *and* flattening its message to `NOT_PUBLIC` leaves the `rejects.toThrow` assertion passing and is caught only by the call assertion (1 failed, 3 passed) — AssertionError: expected "vi.fn()" to be called with arguments: [ 'sneaky.example.com', { all: true } ] Guard restored bit-for-bit; `git diff` against main is empty. 9 passed across both SSRF guard files; tsc --noEmit and eslint clean. Not taken from the same review: exporting `NOT_PUBLIC` from ssrf-guard.ts to remove the duplicated literal. CodeRabbit and I agree it would add API surface without improving runtime behaviour. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01MdQ7ABmZMShRGQznQbPS4v --- .../__tests__/ssrf-guard-resolution-bypass.test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/apps/web/src/lib/__tests__/ssrf-guard-resolution-bypass.test.ts b/apps/web/src/lib/__tests__/ssrf-guard-resolution-bypass.test.ts index 18cea6010..03ebd6975 100644 --- a/apps/web/src/lib/__tests__/ssrf-guard-resolution-bypass.test.ts +++ b/apps/web/src/lib/__tests__/ssrf-guard-resolution-bypass.test.ts @@ -27,6 +27,15 @@ afterEach(() => { * Salvaged from #1428, whose CWE-209 fix was superseded by #1381 (feae3d3) but * whose detection cases were not carried over. Ported to the merged API, where * every DNS-path rejection is the single `NOT_PUBLIC` literal. + * + * Each case also asserts that `dns.lookup` was reached. Today a pre-DNS + * rejection throws a different literal (`Blocked host`, `Blocked private IP + * literal`), so it would fail the message assertion loudly — but if those + * literals were ever flattened into `NOT_PUBLIC` too, a hostname added to + * `BLOCKED_HOSTNAMES` would short-circuit before resolution and these tests + * would keep passing while no longer testing resolution at all. That is not a + * hypothetical refactor: #1428 unified all six rejection paths exactly that + * way. The call assertion is what keeps the test honest through it. */ const NOT_PUBLIC = 'Host does not resolve to a public address'; @@ -42,6 +51,7 @@ describe('assertPublicHttpUrl resolution bypasses', () => { await expect(assertPublicHttpUrl('https://sneaky.example.com/a.mp3')).rejects.toThrow( NOT_PUBLIC, ); + expect(dns.lookup).toHaveBeenCalledWith('sneaky.example.com', { all: true }); }); it('rejects when any resolved address is private, even if another is public', async () => { @@ -56,6 +66,7 @@ describe('assertPublicHttpUrl resolution bypasses', () => { await expect(assertPublicHttpUrl('https://mixed.example.com/a.mp3')).rejects.toThrow( NOT_PUBLIC, ); + expect(dns.lookup).toHaveBeenCalledWith('mixed.example.com', { all: true }); }); it('rejects a private address that appears after several public ones', async () => { @@ -70,6 +81,7 @@ describe('assertPublicHttpUrl resolution bypasses', () => { await expect(assertPublicHttpUrl('https://tail.example.com/a.mp3')).rejects.toThrow( NOT_PUBLIC, ); + expect(dns.lookup).toHaveBeenCalledWith('tail.example.com', { all: true }); }); it('still allows a host whose answers are all public', async () => { @@ -83,5 +95,6 @@ describe('assertPublicHttpUrl resolution bypasses', () => { const url = await assertPublicHttpUrl('https://public.example.com/a.mp3'); expect(url.hostname).toBe('public.example.com'); + expect(dns.lookup).toHaveBeenCalledWith('public.example.com', { all: true }); }); });