fix(web): range-check bracketed IPv6 literals in the SSRF guard - #1486
Conversation
Closes #1484 `URL.hostname` keeps the brackets on an IPv6 literal — `http://[::1]/` yields `[::1]` — and `net.isIP` does not accept that spelling, so the IP-literal branch was skipped and every IPv6 literal was handed to `dns.lookup` instead. `ipIsPrivate` never saw it. `http://[::1]/` was still rejected, but only incidentally: the resolver errors on a bracketed name and the DNS branch turns that into a rejection. The loopback range check played no part, which made all of the IPv6 handling in `ipIsPrivate`/`ipv6ToHextets` dead code for literals supplied in a URL. It also rejected *public* IPv6 literals, which this guard is meant to allow. Also adds the module's first tests. #1381 rewrote every rejection path here to close the CWE-209 DNS oracle and shipped without any, leaving the indistinguishability property it argued for unpinned. Writing them is what exposed the bracket bug. The tests assert the DNS branches against *each other* rather than against a literal message — a test that only checked "some static string is returned" would still pass if two branches returned two different static strings, which is the same oracle. They also assert the operator-side log still distinguishes the causes, so the reason is moved to the logs rather than lost, and pin #1381's deliberate choice to keep the IP-literal message distinct. Non-vacuity measured, not inferred: - pre-#1381 guard (feae3d3^): 9 of 11 fail; the 2 that pass are the controls (public host allowed, IP-literal distinct) - current main without the bracket fix: 3 of 11 fail - this head: 11 pass Full web suite at this head: 56 files, 334 passed, 0 failed. tsc --noEmit clean; eslint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FAaz25m9uWf7BnXGaytGpx
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
groupthinking
added
javascript
Claude
|
Confirmed, and worth recording precisely because it caveats this PR's own Verification section: no job in
So the 11 tests added here would sit at green on every required check without being executed once. The One consequence worth flagging for #1480's scope: the same gap means the No action needed from me — I'll leave this parked and won't push to it. Happy to rebase onto Generated by Claude Code |
|
@coderabbitai full review Generated by Claude Code |
|
|
|
Heads-up on a collision: #1483 and this PR independently salvaged the same two cases from #1428, within about 90 seconds of each other. Two of your
— are the same scenarios as two of the four in This PR should be the survivor. It carries a real production fix that #1483 doesn't have, and its test set is otherwise a superset. I'll close #1483 once this merges rather than before, so the coverage isn't lost if this stalls. I checked the guard change and it looks right. Two assertions from #1483 worth absorbing, if you want them:
Neither is a blocker — your Generated by Claude Code |
|
Red-team pass on this diff, since CodeRabbit is rate-limited (next slot ~45 min) and never actually reviewed it. The bracket fix itself is correct — all six control cases behave exactly as the description claims. But the widening reaches further than the description accounts for, and one case lands on the address this module's own docstring names as its reason for existing. Finding: the widening allows IPv6 encodings of private IPv4Before this change every IPv6 literal was rejected (incidentally — the resolver chokes on the bracketed name). After it, literals are range-checked and public ones are allowed. Measured by extracting
Severity — deliberately not overstated. This is not a live exploit on the current deployment: reaching those IPv4 destinations requires the egress path to have a NAT64 or 6to4 relay, which Vercel's serverless egress almost certainly does not. It is a latent gap in a defense-in-depth control, and it is newly reachable — these literals were all rejected before this PR. Worth closing while the file is already open rather than leaving it for whoever next widens the guard. Suggested patchIn // Transition prefixes embed an IPv4 destination inside a syntactically
// public IPv6 address. Decode the embedded address and re-check it, so a
// NAT64/6to4 spelling can't launder a private or metadata target.
if (h[0] === 0x0064 && h[1] === 0xff9b) { // 64:ff9b::/96 (NAT64, RFC6052)
return ipIsPrivate(`${h[6] >> 8}.${h[6] & 0xff}.${h[7] >> 8}.${h[7] & 0xff}`);
}
if (h[0] === 0x2002) { // 2002::/16 (6to4, RFC3056)
return ipIsPrivate(`${h[1] >> 8}.${h[1] & 0xff}.${h[2] >> 8}.${h[2] & 0xff}`);
}
if ((h[0] & 0xffc0) === 0xfec0) return true; // fec0::/10 site-local (deprecated)
if (h[0] === 0x0100 && h[1] === 0 && h[2] === 0 && h[3] === 0) return true; // 100::/64 discardThe Two things I did not do
Separately, confirming the sequencing note above from the workflow files rather than by inspection: no job in Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Candidate files =='
fd -a -i 'ssrf-guard|ci\.ya?ml|e2e-tests\.ya?ml' . | sort
echo '== Source/test outlines =='
while IFS= read -r f; do
case "$f" in
*ssrf-guard*.ts)
echo "--- $f"
ast-grep outline "$f" --items all || true
;;
esac
done < <(fd -a -i 'ssrf-guard' . | sort)
echo '== SSRF guard and test contents =='
while IFS= read -r f; do
echo "--- $f"
cat -n "$f"
done < <(fd -a -i 'ssrf-guard' . | sort)
echo '== Workflow vitest and web-test references =='
rg -n -i -C 3 'vitest|apps/web|npm run test|continue-on-error' .github/workflows 2>/dev/null || trueLength of output: 1234 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== SSRF guard outline =='
ast-grep outline apps/web/src/lib/ssrf-guard.ts --items all || true
echo '== SSRF guard =='
cat -n apps/web/src/lib/ssrf-guard.ts
echo '== Primary SSRF tests =='
cat -n apps/web/src/lib/__tests__/ssrf-guard.test.ts
echo '== DNS leakage SSRF tests =='
cat -n apps/web/src/lib/__tests__/ssrf-guard-dns-leakage.test.ts
echo '== Workflow files and web-test references =='
fd -a -t f . .github/workflows | sort
rg -n -i -C 3 'vitest|apps/web|npm run test|continue-on-error' .github/workflows || trueLength of output: 73821
This change makes The fix should block this PR until it includes range checks and regression tests for:
One correction to the proposed NAT64 condition: it must check the exact The I also confirmed the CI concern. ✏️ Learnings added
You are interacting with an AI system. |
Correction to my earlier comment — and #1483 is now closedI framed the overlap as being between this PR and #1483. That was wrong about the source. #1428 merged at 20:57:02 as So the duplication to check is against
That's two of your eleven tests already covered on None of that touches your production fix, which is the part The two additions I offered earlier still stand and are unaffected by this correction — the Generated by Claude Code |
Fixes a regression introduced by the bracket strip in 393f197, caught by CodeRabbit on #1486. Routing IPv6 literals into `ipIsPrivate` exposed that its IPv6 branch ends in `return false` for any form it does not recognise. The transition prefixes are syntactically public but carry an IPv4 destination in their bits, so a NAT64- or 6to4-capable egress path translates them to the address they encode. `64:ff9b::a9fe:a9fe` reaches 169.254.169.254. Before 393f197 these were rejected, but only incidentally — the bracketed name went to `dns.lookup` and the resolver errored. Making the literal branch work therefore turned an accidental block into an allow, which is strictly worse than the bug it fixed. Now decoded and re-checked against the IPv4 rules: - 64:ff9b::/96 NAT64 (well-known prefix) - 2002::/16 6to4, whose IPv4 sits in h[1]/h[2], not the low bits - ::ffff:0:0:0/96 IPv4-translated — h[4] holds the 0xffff, so the existing mapped/compatible test did not cover it And blocked outright: - fec0::/10 site-local - 100::/64 discard-only - the rest of 64:ff9b::/32, e.g. RFC 8215 local-use 64:ff9b:1::/48, which is local-use by definition The NAT64 test matches the exact /96 by requiring h[2]..h[5] to be zero. Testing only h[0]/h[1] would claim 64:ff9b::/32, which is a wider assertion than "the low 32 bits are an IPv4 address". Tests: +3 cases, including public-embedded controls for NAT64 and 6to4 so the checks cannot pass by over-blocking, and one asserting the same rules apply to a *resolved* address — otherwise the fix would just move the bypass one DNS lookup away. Non-vacuity measured: against 393f197 (bracket fix, no transition prefixes) exactly the 2 new blocking tests fail and the other 12 pass. Full web suite at this head: 56 files, 337 passed, 0 failed. tsc --noEmit clean; eslint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FAaz25m9uWf7BnXGaytGpx
|
Confirmed and fixed in I traced it before acting rather than taking the report on faith. For The uncomfortable part is the direction of travel: before Now decoded and re-checked against the IPv4 rules:
Blocked outright: On your correction about the NAT64 prefix — you're right, and I implemented it as specified. The check requires I also took the Verification, measured:
Your CI observation matches mine independently — Generated by Claude Code |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure 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 FilesNone |
|
The transition-prefix finding above is fixed in #1512, on its own branch off One correction to my own red-team framing: I called it "a security regression in this PR," and CodeRabbit agreed. That framing is too narrow. The gap is on That changes what should happen to this PR: the finding is not yours to fix, and shouldn't block you. #1512 fixes the range checks ( Both of CodeRabbit's corrections were adopted rather than my original sketch:
Public embedded addresses are decoded and allowed rather than blanket-blocked, so a legitimate NAT64/6to4 translation of a public host still passes — three control tests cover that. Verification on #1512: reverting only Neither PR can currently show a green CI, and that is not about either diff. GitHub Actions is not executing jobs for this repo right now — 278 runs queued, one Sequencing unchanged from the earlier note: this one still wants #1480 first, since no job in Generated by Claude Code |
CI has no concurrency group, so pushing a new commit to a PR leaves the previous run queued or in flight. Those runs still count against the account's concurrent-job allowance while testing a SHA that is no longer the head of anything. Measured on the live queue: 29 CI runs queued across 17 branches, i.e. 12 duplicates of a branch already represented. Five were PR branches holding a superseded run alongside their current head (#1483 and #1486 among them, both being actively pushed to). Meanwhile main's own post-merge runs had been queued 29 minutes. Group key is the PR number for `pull_request` and the ref for `push`, so a PR's runs only ever cancel each other, never another PR's. `cancel-in-progress` is gated to pull_request events on purpose: each commit landing on main must keep its own post-merge run, because that run is the record of whether main was green at that SHA. A rapid series of merges must not cancel each other — which an ungated `cancel-in-progress: true` would do. This reclaims wasted capacity; it does not raise the ceiling. A queue saturated by genuinely distinct branches still waits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017yTj3erqg8xRb79v2djrmH
Merge the current head, not
|
| New case | Already covered by |
|---|---|
| reports a non-existent host and a privately-resolving host identically | dns-leakage: "does not reveal whether a probed internal name exists" |
| gives every DNS outcome the same caller-visible message | dns-leakage: "is indistinguishable from a transient resolver failure" |
| keeps the hostname, errno, resolver text out of the message | dns-leakage: "does not surface resolver text when the lookup rejects" |
| does not leak the resolved private address to the caller | dns-leakage: "still blocks hosts that resolve to a private address" |
| allows a host that resolves to a public address | dns-leakage: "still allows a genuinely public host" |
| rejects a private address hiding behind an IPv4-mapped IPv6 spelling | private-address-detection: "rejects loopback hiding behind an expanded IPv4-mapped IPv6 answer" |
| rejects when any resolved address is private, even if another is public | private-address-detection: "rejects when any answer is private, even if a public one comes first" |
The other 7 are genuinely new and are the ones worth keeping — the two bracket-literal cases, the pre-DNS assertion, the three transition-prefix cases (including the control that proves it isn't just over-blocking), and the pinning of #1381's deliberate IP-literal message split.
Related: #1483 was closed unmerged, and that looks right — ssrf-guard-private-address-detection.test.ts on main already carries its cases. Its one case with no equivalent anywhere is "a private address that appears after several public ones" (a 3-record answer guarding against an early-exit that only inspects a prefix), if you want to salvage that single test into this file.
Terminal state
HALTED(awaiting_merge_approval) — no automerge label, main is protected, so per §8 of the runbook this stops here rather than merging. No changes pushed to this branch.
Generated by Claude Code
Absorbs the two assertions offered from #1483 before it was closed, so the coverage survives that PR rather than being lost with it. 1. `expect(lookup).toHaveBeenCalledWith(host, { all: true })` on all four resolution cases. Today a pre-DNS rejection throws `Blocked host` or `Blocked private IP literal`, so `rejectionOf` plus the `loggedText()` assertion would already fail loudly. It stops being load-bearing the moment those literals are flattened to `NOT_PUBLIC` too — then a hostname absorbed by a pre-DNS branch is indistinguishable from a resolution rejection and the test goes quiet. This PR is what makes that reachable: it adds a new pre-DNS branch (the bracket strip) that did not exist before. 2. A three-address case with the private address in tail position. The existing two-address case kills a `resolved[0]`-only scan; this additionally kills a "check a prefix of the answers" bug, and is the only case here exercising 172.16/12. Both verified non-vacuous by mutation against the real guard, not by inspection: - `for (const r of resolved.slice(0, 2))` — prefix-only scan: 1 failed | 14 passed, and the one failure is the new tail case. - short-circuit the four hostnames before the bracket strip, throwing the flattened `NOT_PUBLIC`: 4 failed | 11 passed. Every failure is a `toHaveBeenCalledWith` assertion; not one message assertion caught it, which is precisely the silent-hollowing scenario these guard against. Guard restored bit-for-bit after each mutation; `git diff` against the branch tip for `ssrf-guard.ts` is empty. No production code changes in this commit. Also merges `main` (5 commits) so `test-frontend` (#1480) reports against a current base. Note that `main` now carries `ssrf-guard-private-address-detection.test.ts` from #1428, which overlaps two cases with this file — called out on the PR rather than reorganised here. apps/web vitest: 57 files, 341 passed, 0 failed. tsc --noEmit and eslint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GG1fyRobfHwhdFaqgqKvP
Absorbed the two offered assertions —
|
| Mutation | Result |
|---|---|
for (const r of resolved.slice(0, 2)) — prefix-only scan |
1 failed | 14 passed; the one failure is the new tail case. The existing two-address case survives, which is exactly why it doesn't cover this. |
Short-circuit all four hostnames before the bracket strip, throwing the flattened NOT_PUBLIC |
4 failed | 11 passed — and every failure is a toHaveBeenCalledWith. Not one message assertion caught it. |
That second row is the whole argument. Today a pre-DNS rejection throws Blocked host or Blocked private IP literal, so rejectionOf + loggedText() would fail loudly and the call assertion looks redundant. It stops being redundant the moment those literals are flattened to NOT_PUBLIC as well — then a hostname absorbed by a pre-DNS branch is indistinguishable from a resolution rejection and the test goes quiet while still passing. This PR is what makes that reachable: the bracket strip is a new pre-DNS branch that did not exist before it.
git diff against the previous branch tip for ssrf-guard.ts is empty — test-only commit.
Verification on a61062e
npx vitest run(apps/web) — 57 files, 341 passed, 0 failednpx tsc --noEmit— cleannpm run lint— clean
Two notes on state, not on the diff
main was already merged in by a parallel push (fbed5f8) while I was working; I rebased onto it rather than pushing a second merge. Gate 7 is satisfied — main merges cleanly, and test-frontend (#1480, merged 21:09) now reports against a current base.
Three SSRF test files now coexist on the merged tree. main carries ssrf-guard-private-address-detection.test.ts from #1428, which overlaps two cases with this file (expanded IPv4-mapped loopback; multi-answer public-first + 10.0.0.5, same addresses). Not a correctness problem and not worth churning this PR over — flagging it so the duplication is a recorded decision rather than an accident. ssrf-guard-dns-leakage.test.ts is a distinct concern and should stay separate.
Terminal state: HALTED(awaiting_merge_approval). Required checks are still running on the new head; the merge itself is a human call under MERGE_POLICY.md §8 and I am not taking it.
Generated by Claude Code
|
Red-team pass on Two findings about the surrounding state rather than this diff. 1. There are now three test files for
The commit message calls out the #1428 overlap ("overlaps two cases with this file") and that is accurate — So roughly 6 of 23 cases across the three files are duplicates. Not a merge blocker — but the pattern is that each PR touching this module adds a fourth file, and the cost lands on whoever next changes the guard's contract. 2. The three files disagree about how to pin the rejection message, and the disagreement is invisible while everything passes. Counting assertions on the literal
This PR's file is deliberately literal-free; its stated design is to compare branches against each other, because a test asserting "some static string is returned" would still pass if two branches returned two different static strings — the same oracle #1381 set out to close. That reasoning is sound. But the two files already on The consequence is specific to the Worth deciding before that redesign starts, not during it: either consolidate the three files, or export the message as a constant so the literal exists once and the rename can't split the suite. Neither finding changes my read that this PR should land. CI on this head is still queued behind a saturated Actions backlog (249 runs queued against ~22 concurrent slots at the time of writing), so I have not verified the suite independently — the 341-passed figure is the author's, and Generated by Claude Code |
The inline hostname list on POST /api/workflows/video-to-actions enumerated localhost, 127.0.0.1, 0.0.0.0, ::1, .local and .internal. Measured against the predicate itself, that allowed: http://169.254.169.254/ -> "169.254.169.254" ALLOWED http://10.0.0.1/ -> "10.0.0.1" ALLOWED http://192.168.1.1/ -> "192.168.1.1" ALLOWED http://[::1]/ -> "[::1]" ALLOWED http://[0:0:0:0:0:ffff:7f00:1]/ -> "[::ffff:7f00:1]" ALLOWED Two separate defects. The list omits every RFC1918 range, 169.254/16 and CGNAT. And `host === '::1'` is unreachable: `URL.hostname` keeps the brackets on an IPv6 literal, so the comparison never matches — the same bug #1486 fixed in ssrf-guard.ts, re-created here in a new file. assertPublicHttpUrl already covers all of it, including the NAT64/6to4/ IPv4-translated encodings that #1486 added, so this deletes the list rather than extending it. #1486 merged at 21:23, which is what makes the guard a settled dependency to import. The catch is deliberately bare. Interpolating the guard's message would re-open the CWE-209 DNS oracle #1381 closed on /api/transcribe: "does not resolve" and "resolves to a private address" must be indistinguishable to the caller. The guard already logs the real cause for operators. New: apps/web/src/app/api/__tests__/workflows-video-to-actions-route.test.ts, 12 cases. Verified non-vacuous by restoring the old hand-rolled check and re-running — 8 failed | 4 passed, and the eight failures are exactly the seven literals above plus the resolves-to-private case. The four survivors are the ones the old list did handle (127.0.0.1, localhost), the indistinguishability assertion, and the public-host control. Also asserts `start` is never called on a rejected target: a refused URL that still kicked off a durable run would move the fetch somewhere harder to see rather than prevent it. Merges main (#1486) for the guard. apps/web vitest: 60 files, 361 passed, 0 failed. tsc --noEmit and eslint clean. Not addressed here, and left on the PR thread: the status route returns err.message plus a config hint to the caller, and getRun(runId) has no ownership binding. The first is the same CWE-209 pattern; the second is a design question about whether runs are per-user. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014GG1fyRobfHwhdFaqgqKvP
…e AI budget Self-review follow-up. The exemption looked the prefix up with the same loose startsWith used for class membership, so a future sibling surface whose name merely starts with an exempted prefix -- /api/workflows-admin -- would silently inherit the GET carve-out and drop onto the looser budget. No route in the tree does this today (checked every directory under apps/web/src/app/api), so this is latent rather than live. It is worth closing while the file is open: the same shape, an incidental block quietly becoming an allow, is what #1486 had to fix in the SSRF guard. Class membership keeps its original loose matching. Narrowing that would move routes off the stricter budget, which this change has no business doing; the exemption is the widening, so only the exemption is tightened.
…t bucket (#1518) * fix(web): stop workflow status polls draining the shared AI rate-limit bucket #1507 added /api/workflows to AI_ROUTE_PREFIXES. isAiRoute classified by path prefix alone, so the polled GET status endpoint was metered against the AI budget (default 12/min) while pollVideoToActions polled at 40/min. The 12th request 429'd ~17s into a 30s window, before a transcript fetch plus an agent call could finish. The bucket is keyed by class, not path, so every AI prefix shares one ai:<ip> counter -- a single Studio run also 429'd /api/chat, /api/transcribe and /api/pipeline as collateral. Move the classifier into auth-paths.ts, which exists as the home for path policy free of Next.js request types so vitest can import it offline, and make it method-aware. GET/HEAD on /api/workflows falls to the general budget; POST stays AI-class because starting a run does real model work. The exemption is keyed per-prefix rather than exempting GET globally, so it cannot widen another route that later serves model work over GET. An omitted method defaults to POST so the failure mode is the stricter limit. Also retune the poller to 30 attempts x 2s: 30 req/min leaves roughly half the general allowance for the rest of the page, and the wall-clock window doubles to 60s, which better fits the work the run actually does. Closes #1517 * fix(web): require a segment boundary before exempting a route from the AI budget Self-review follow-up. The exemption looked the prefix up with the same loose startsWith used for class membership, so a future sibling surface whose name merely starts with an exempted prefix -- /api/workflows-admin -- would silently inherit the GET carve-out and drop onto the looser budget. No route in the tree does this today (checked every directory under apps/web/src/app/api), so this is latent rather than live. It is worth closing while the file is open: the same shape, an incidental block quietly becoming an allow, is what #1486 had to fix in the SSRF guard. Class membership keeps its original loose matching. Narrowing that would move routes off the stricter budget, which this change has no business doing; the exemption is the widening, so only the exemption is tightened. --------- Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Canonical issue
Closes #1484
Outcome
An IPv6 literal in a URL is now range-checked by
ipIsPrivateinstead of being sent to the resolver.URL.hostnamekeeps the brackets on an IPv6 literal —new URL('http://[::1]/').hostnameis[::1]— andnet.isIPdoes not accept that spelling. The literal branch was therefore skipped for every IPv6 literal:http://[::1]/was still rejected, but only incidentally — the resolver errors on a bracketed name and the DNS branch converts that into a rejection. The loopback range check played no part, which made the IPv6 handling inipIsPrivate/ipv6ToHextets(link-local, unique-local, IPv4-mapped in any spelling) dead code for literals supplied in a URL. It also rejected public IPv6 literals, which this guard is meant to allow.This also adds the module's first tests. #1381 rewrote every rejection path here to close the CWE-209 DNS oracle and shipped without any, leaving the indistinguishability property it argued for unpinned. Writing them is what exposed the bracket bug.
Scope
ssrf-guard.ts— strip surrounding brackets beforenet.isIP, with a comment recording why. Six lines of logic, plus the comment.__tests__/ssrf-guard.test.ts— new file, 11 tests. First coverage this module has had.ipIsPrivateandipv6ToHextetsare untouched — the fix is about which inputs reach them.SsrfGuardErrorredesign. test(security): cover SSRF private-address detection main's tests miss #1428 proposes a typed error with a diagnosticreasonand collapsing all six rejection paths onto one exported constant. That is a live design question and stays test(security): cover SSRF private-address detection main's tests miss #1428's to settle; this PR deliberately preserves fix(web): stop leaking upstream and Stripe error details to clients #1381's merged behaviour, including its documented choice to keep the IP-literal message distinct.Risk
assertPublicHttpUrlhas one live call site,transcription-service.ts).Verification
Head
393f197. Measured, not inferred.Focused tests — 11 new cases, all passing.
Non-vacuity proven against two baselines, by running the suite against each rather than reasoning about them:
feae3d3^) — the original DNS oraclemain(5934cbf), bracket fix reverteddns.lookupbeing called with"[::1]"The load-bearing assertion is indistinguishability. The DNS tests compare the branches against each other, not against a literal message — a test that only checked "some static string is returned" would still pass if two branches returned two different static strings, which is the same oracle. A separate assertion requires the operator-side log to still differ, so the cause is moved to the logs rather than lost.
fix(web): stop leaking upstream and Stripe error details to clients #1381's deliberate exception is pinned, not overridden. A test asserts the IP-literal message stays distinct from the DNS one, so a future uniformity pass has to change a failing test — and read the comment explaining why — rather than silently flip a decision that was already reviewed and merged.
Full web suite — 56 files, 334 passed, 0 failed. No regressions. (The
billing-chat-gatingtimeout previously tracked in billing-chat-gating free-tier test depends on ambient AI_GATEWAY_API_KEY and passes vacuously in CI #1116 no longer reproduces; test: make web suite hermetic against ambient AI gateway keys #1230's hermetic gateway-key defaults are onmain.)npx tsc --noEmit— cleannpm run lint— cleanRequired CI — pending first run on this head.
Review threads resolved — none open yet.
Production evidence
Not applicable as a deployed artefact. This is a library-level change with no UI surface, and the observable effect is a rejection path taken for a different reason — which the regression tests measure directly. Demonstrating it against a preview would mean submitting an internal address to
/api/transcribe.Agent handoff
Note for reviewers
This PR came out of a PR-remediation sweep, not a feature request. Two things worth flagging beyond the diff:
feae3d3and changed both files it touches (ssrf-guard.ts+38,transcription-service.ts+39). test(security): cover SSRF private-address detection main's tests miss #1428 is nowmergeable_state: dirty, and fix(web): stop leaking upstream and Stripe error details to clients #1381 already delivers its headline outcome — collapsing the four DNS outcomes onto one constant with the cause logged server-side. Its remaining delta is theSsrfGuardErrorAPI and uniformity across all six paths, which is a real but narrower question than its description implies. It needs a reconciliation decision under gate 6 ofMERGE_POLICY.md, not a rebase.mainneeds regardless of how test(security): cover SSRF private-address detection main's tests miss #1428 is settled — the bracket bug is present onmaintoday and is orthogonal to theSsrfGuardErrordesign.Generated by Claude Code