test(web): cover SSRF guard resolution bypasses salvaged from #1428

[groupthinking]

Canonical issue

Closes #1495

Outcome

Locks in two SSRF-guard behaviours that nothing currently tests: a private destination must not survive resolution by hiding inside an alternate IPv6 spelling, or behind a public sibling DNS record.

main already handles both correctly — this adds no fix, only the regression lock. A regression in either would be silent: assertPublicHttpUrl would return a URL and the caller would fetch it.

Why this exists

#1428 set out to close the same CWE-209 DNS oracle as #1427. That fix landed first through #1381, which rewrote ssrf-guard.ts and collapsed every DNS-path rejection into one NOT_PUBLIC literal. #1428's security outcome is therefore fully superseded.

Its detection cases were not superseded, and no other test covers them. The existing 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.

Ported to the merged API — #1428's cases were written against a SsrfGuardError type carrying a reason field, which #1381 did not introduce.

Scope

• Included: apps/web/src/lib/__tests__/ssrf-guard-resolution-bypass.test.ts (new)
• Explicitly excluded: production code changes; IPv6 literal bracket fix (fix(web): range-check bracketed IPv6 literals in the SSRF guard #1486 / ssrf-guard: bracketed IPv6 literals skip the private-IP branch, and the module has no tests #1484)

Risk

• Risk level: low (tests only)
• Failure mode: flaky DNS mocking would fail CI; tests use controlled mocks
• Rollback: git revert of the test file only

Verification

• Focused tests pass locally against main's assertPublicHttpUrl
• Full apps/web vitest suite green in CI (test-frontend once ci: run apps/web vitest as required test-frontend job (#1449) #1480 lands)

Production evidence

N/A — test-only change.

Agent handoff

Land after #1480 so new tests are actually gated. Complementary to #1486.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
v0-uvai	Ready	Preview, v0	Aug 7, 2026 9:14pm

[coderabbitai]

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)

• [‘architecture-gap’, ‘bug’, ‘ci-cd’, ‘ci/cd’, ‘copilot-rabbit’, ‘documentation’, ‘duplicate’, ‘enhancement’, ‘frontend’, ‘github_actions’, ‘good first issue’, ‘help wanted’, ‘high-priority’, ‘invalid’, ‘javascript’, ‘ml-model’, ‘needs-triage’, ‘pipeline-critical’, ‘placeholder-code’, ‘priority:high’, ‘python’, ‘python:uv’, ‘question’, ‘styling’, ‘tests’, ‘v0’]

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a76d36ef-1f97-40d5-8eca-a5efa0df6df4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[groupthinking]

@coderabbitai review

Two things worth attacking here, since this PR's whole value is that the tests are meaningful:

1. Are the assertions actually load-bearing? Each test drives assertPublicHttpUrl through a mocked dns.lookup and asserts a throw of the NOT_PUBLIC literal. If the guard rejected these URLs for some other reason — a hostname blocklist hit, a scheme check, a parse failure — the tests would still pass while testing nothing about resolution handling. I believe the hostnames used (sneaky.example.com, mixed.example.com, tail.example.com, public.example.com) reach the DNS branch, but that is the assumption most worth checking.

2. Is NOT_PUBLIC duplicated as a literal a liability? The test hardcodes 'Host does not resolve to a public address' because ssrf-guard.ts scopes that constant inside the function rather than exporting it. If someone reworded the message, these tests would fail for a cosmetic reason. I chose not to export it — that would be a production change in a test-only PR — but if you think the coupling is wrong, say so.

Context you may not have: this is salvage from #1428, which I closed as superseded by #1381 (feae3d3). No production code changes; ssrf-guard.ts is byte-identical to main. I verified non-vacuity by mutation (details in the PR body), so the useful thing from you is whether the tests test what they claim, not whether they pass.

Note for the record: auto-review skipped again with "Auto reviews are limited based on label configuration", listing 26 labels under a Required labels (at least one) (1) header. That (1) is the count — one required label whose literal value is the whole bracketed string. .coderabbit.yaml on main already sets reviews.auto_review.labels: [] (merged in #1425) specifically to clear that inherited gate, and this PR was opened after that landed, so the empty-list override is evidently not clearing a gate inherited from the Repository/Organization UI. That looks like it needs a dashboard change rather than a YAML one — flagging it on #1424/#1425 rather than here.

---

Generated by Claude Code

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== Changed files and diff ==='
git diff --no-ext-diff --unified=80 "$(git merge-base HEAD origin/main)" HEAD -- \
  'apps/web/src/lib/__tests__/ssrf-guard-resolution-bypass.test.ts' \
  'apps/web/src/lib/ssrf-guard.ts' || true

echo '=== Candidate files ==='
fd -t f 'ssrf-guard' apps/web/src/lib 2>/dev/null || true

echo '=== SSRF guard outline ==='
guard_file="$(fd -t f -a '^ssrf-guard\.ts$' apps/web/src/lib | head -n 1)"
if [ -n "${guard_file:-}" ]; then
  ast-grep outline "$guard_file" --view expanded || true
  echo '=== SSRF guard source ==='
  cat -n "$guard_file"
fi

echo '=== Resolution-bypass test outline and source ==='
test_file="$(fd -t f -a '^ssrf-guard-resolution-bypass\.test\.ts$' apps/web/src/lib | head -n 1)"
if [ -n "${test_file:-}" ]; then
  ast-grep outline "$test_file" --view expanded || true
  cat -n "$test_file"
fi

echo '=== Existing SSRF tests using assertPublicHttpUrl ==='
rg -n -C 5 --glob '*ssrf-guard*.test.ts' \
  '\bassertPublicHttpUrl\s*\(|Host does not resolve to a public address|dns\.lookup' \
  apps/web/src/lib || true

Length of output: 35991

---

@groupthinking I verified the current control flow.

The four hostnames reach dns.lookup. They are valid HTTPS URLs. They are not in BLOCKED_HOSTNAMES. They do not end in .internal or .local. They are not IP literals. Therefore, the current tests exercise the DNS-resolution branch before they receive NOT_PUBLIC.

The mutation results are consistent with this control flow. The assertions are load-bearing for the target conditions on the current implementation.

I found one test-durability improvement. Each rejection test should also assert that dns.lookup was called with its hostname and { all: true }. Without that assertion, a future hostname blocklist change could reject only sneaky.example.com, mixed.example.com, or tail.example.com before resolution. The affected test would still pass, but it would no longer test the resolution bypass.

For example, after each rejection assertion:

expect(dns.lookup).toHaveBeenCalledWith('sneaky.example.com', { all: true });

Apply the equivalent assertion to the other three cases. The all-public control already prevents an unconditional rejection regression. The call assertions prevent a per-host pre-DNS rejection from becoming a false positive.

I do not consider the duplicated NOT_PUBLIC literal a liability in this PR. The literal is the DNS-path contract that prevents a DNS oracle. The adjacent ssrf-guard-dns-leakage.test.ts already tests the same contract. Exporting a production constant only to remove this test literal would add API surface without improving runtime behavior.

---

✏️ Learnings added

Learnt from: groupthinking
URL: https://github.com/groupthinking/EventRelay/pull/1483

Timestamp: 2026-08-07T20:59:45.804Z
Learning: In the EventRelay repository, `reviews.auto_review.labels: []` in `.coderabbit.yaml` did not clear an inherited required-label gate configured in the CodeRabbit Repository or Organization UI. The UI configuration must be changed when auto-review remains blocked by that inherited gate.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[groupthinking]

Review round resolved — call assertion applied in 6b64f30

Taken: the dns.lookup call assertion, on all four cases.

Worth recording why it's right, because the stated rationale isn't quite the mechanism. A pre-DNS blocklist rejection today throws 'Blocked host', and rejects.toThrow(NOT_PUBLIC) does substring matching — so that case would fail loudly rather than pass silently. The assertion earns its place one step further out: the moment those pre-DNS literals are also flattened to NOT_PUBLIC, the short-circuit becomes indistinguishable from a resolution rejection and the test goes quiet.

That is not a hypothetical refactor, which is what settled it for me. #1428 — the PR these cases came from — unified all six rejection paths onto a single message, and main's transcription-service.ts already flattens all five at the call-site boundary. Pushing that flattening down into the guard is a plausible next step, and it would have silently hollowed out three of these four tests.

Verified the new assertions aren't vacuous, using exactly that scenario: short-circuit sneaky.example.com before resolution and flatten its message to NOT_PUBLIC. The message assertion still passes; only the call assertion catches it.

AssertionError: expected "vi.fn()" to be called with arguments:
[ 'sneaky.example.com', { all: true } ]
     54|     expect(dns.lookup).toHaveBeenCalledWith('sneaky.example.com', { al…
  Tests  1 failed | 3 passed (4)

Guard restored bit-for-bit afterwards — git diff against main for ssrf-guard.ts is empty. Still zero production changes.

Not taken: exporting NOT_PUBLIC to remove the duplicated literal. Agreed on the reasoning — API surface without runtime benefit. I'd raised it as a possible liability; you talked me out of it.

Also confirming the control-flow check, since it was the assumption I most wanted verified: all four hostnames are valid HTTPS, absent from BLOCKED_HOSTNAMES, not .internal/.local, not IP literals, so they do reach dns.lookup(host, { all: true }) at ssrf-guard.ts:144.

npx tsc --noEmit and eslint clean; 9 passed across both SSRF guard files on 6b64f30.

Not re-triggering a review — the retry hit the 50-minute rate limit, and the push above will pick up an incremental pass once it resets. The substantive review completed under run 4a85530b; the limited one was a redundant re-trigger, not a failed review.

Terminal state remains HALTED(awaiting_merge_approval) pending CI on the new head.

---

Generated by Claude Code

[groupthinking]

Promoting from draft: residual SSRF resolution-bypass regression tests salvaged from #1428. Complements main's fix; pure test coverage, no behavior change.

[groupthinking]

Superseded by #1486 — holding open until that merges

#1486 was opened ~90 seconds after this PR and independently salvaged the same two cases from #1428. Its rejects a private address hiding behind an IPv4-mapped IPv6 spelling and rejects when any resolved address is private, even if another is public are the same scenarios as two of the four here.

It should win. It carries a real production fix this PR does not: URL.hostname keeps the brackets on an IPv6 literal, net.isIP('[::1]') is therefore 0, and every bracketed IPv6 literal was falling through to dns.lookup instead of the range check — blocked only by the accident of the resolver erroring on a bracketed name, while public IPv6 literals were wrongly rejected. Its test set is otherwise a superset of this one, adding the DNS-oracle indistinguishability cases and operator-side log assertions that are better than what I wrote here.

Not closing yet. #1486 is unmerged, and closing a superseded PR before its replacement lands risks losing the coverage entirely if the replacement stalls — the distinction I applied to #1428, which I closed only because #1381 had already merged. I'll close this once #1486 is in.

Two assertions here aren't in #1486, and I've offered both on that PR rather than duplicating files:

1. expect(lookup).toHaveBeenCalledWith(host, { all: true }) on the resolution cases — which fix(web): range-check bracketed IPv6 literals in the SSRF guard #1486 makes more valuable, not less, since it introduces a new pre-DNS branch that could silently absorb these hostnames.
2. A three-address case with the private address in tail position, killing a prefix-only scan rather than just a resolved[0]-only one.

On the "no competing PR" checkbox

I ticked it in this PR's Agent handoff, and at the time it was accurate — #1486 did not exist. It is no longer accurate, and this comment is the correction rather than a silent edit to the body.

CI

Still not a useful signal on either PR: every Actions job on 6b64f30 has sat queued since 21:01 with none starting. Nothing failing, nothing passing. The repo opened 10 PRs in the 12 minutes around this one, so the runner pool is saturated — worth knowing before reading any green/red on the current batch.

Terminal state: HALTED(superseded_pending_#1486).

---

Generated by Claude Code

[groupthinking]

Independent verification on 6b64f30 — and the gating dependency is now cleared

Triggered by ready_for_review. Re-verified from scratch rather than taking the thread's claims on trust.

The tests are non-vacuous. Confirmed by mutation against the real guard, not by inspection:

Mutation to ssrf-guard.ts	Result
Remove the expanded IPv4-mapped IPv6 decoding in ipIsPrivate	✅ killed — rejects a loopback address written in expanded IPv4-mapped IPv6 form
for (const r of resolved.slice(0, 1)) — inspect only the first answer	✅ killed — both the multi-record and tail-position cases

Worth noting: ssrf-guard-dns-leakage.test.ts caught neither mutation. It passed clean through both. That is the strongest evidence for this PR's premise — these are genuinely uncovered paths, not a second angle on covered ones. Guard restored bit-for-bit afterwards; git diff origin/main 6b64f30 -- apps/web/src/lib/ssrf-guard.ts is empty, and the diff excluding __tests__/ is empty too, so "test-only" holds.

#1480 merged at 21:09:37, so test-frontend (ci.yml:103, npx vitest run, hermetic env per #1230) is now a live job on main. The "land after #1480" ordering in the PR body is satisfied.

That mattered more than it looks: before #1480, ci.yml's test job ran pytest tests/unit/ only — no job executed apps/web vitest at all. Merging this PR beforehand would have added four tests that nothing ran.

Full suite green on the merged tree, run under the exact env test-frontend uses:

Test Files  57 passed (57)
     Tests  330 passed (330)

Action taken: this PR's head predated #1480, so test-frontend had never run against it — which is why the last open checkbox could not be ticked. I've triggered an update from main; the job will now report on the new head.

No new findings. Terminal state stays HALTED(awaiting_merge_approval) — the merge itself is a human call, and I'm not taking it.

---

Generated by Claude Code

[groupthinking]

Closing — this duplicates code that was already in its own base commit

Correcting what I said earlier. I described this PR as salvaging cases from a "superseded" #1428. In fact #1428 merged at 20:57:02 as 75ef3ca, having been reworked by a parallel session into precisely that salvage. This PR was created at 20:57:54 — 52 seconds later — with 75ef3ca as its base. So two of its four tests were already present in the commit it branched from:

Case	Already in base 75ef3ca	Re-derived here
Loopback via expanded 0:0:0:0:0:ffff:7f00:1	✅	✅
Multi-answer, public first + 10.0.0.5 private	✅ (same addresses)	✅

I was working from a fetch taken before that merge and didn't re-read the PR before acting on it. My comment on #1428 claiming to close it was also a no-op — it was already closed-as-merged.

My earlier note here that this was "superseded by #1486" was likewise imprecise: #1486 duplicates it too, but the thing that actually superseded both is main itself.

The delta that is real

Two things here are not in main and not in #1486, and I've offered both on #1486 rather than keeping a third overlapping file alive:

1. expect(lookup).toHaveBeenCalledWith(host, { all: true }) on the resolution cases, so a future pre-DNS branch can't silently absorb these hostnames and leave the tests passing but vacuous. fix(web): range-check bracketed IPv6 literals in the SSRF guard #1486 adds exactly such a branch, which is what makes it worth having.
2. A three-address case with the private address in tail position (172.16.31.9), killing a prefix-only scan rather than just a resolved[0]-only one, and exercising the 172.16/12 range nothing else touches.

Neither justifies a separate PR against an area that already has one merged file and one open PR.

Not a wasted verification

The mutation testing done here still holds and transfers: breaking the mapped-IPv4 decode, restricting the loop to resolved[0], and short-circuiting before DNS with a flattened message each kill exactly their target case. That third mutant is the argument for the call assertion, and it's the reason I'm handing it to #1486 rather than dropping it.

Terminal state: CLOSED(duplicates_merged_#1428).

---

Generated by Claude Code

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA 3646a90.

Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Scanned Files

None