fix(web): stop /api/transcribe leaking probe status and key config - #1440
Conversation
#1381 sanitized the thrown and JSON-parse paths of /api/transcribe but left the `!result.success` branches returning `fetchTranscript`'s message verbatim, on the stated grounds that every such value is an app-authored literal. Two were not, and both are reachable by a caller who controls the request body: - `Failed to fetch audio: ${audioResponse.status}` echoed the HTTP status of the caller-supplied `audioUrl`. The SSRF guard admits public hosts, so this returned a cross-origin read (401 vs 403 vs 404 vs 500) that the browser's same-origin policy would otherwise deny — a probe oracle, not upstream text, which is why it survived the original pass. - The all-strategies-failed message branched on whether provider keys were set, disclosing server configuration and naming both the variables and the hosting platform. Both are fixed at the source in transcription-service.ts rather than masked at the route, so any future caller of fetchTranscript inherits the fix. The real status and the real key state are logged for operators, where they were the only thing actionable. Also drops the residual `details` field from the billing branch, which named this deployment's cloud and model vendors — #1381 stated `details` was gone from this route, and this is the one that was left. The route docblock carried the claim these values disproved; it now states the invariant and what enforces it. Verification (head e9bf62d, apps/web): - 3 new tests in transcription-error-disclosure.test.ts; all 3 fail against the unfixed service and pass with it — the status test asserts four distinct upstream statuses collapse to one message, so a partial fix cannot pass it. - vitest run: 1 failed | 290 passed (291). The single failure, billing-chat-gating "blocks free tier after daily quota", is pre-existing and unrelated — confirmed failing identically on the untouched e9bf62d with this change stashed. Tracked in #1116; #1230 makes the suite hermetic against it. - npm run type-check: clean. npm run lint: clean. - package-lock.json drift from npm install reverted, per #1381's convention. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EX3tLZJJfbDUM6XXi6Uefm
|
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 |
github-actions
Bot
added
javascript
|
@coderabbitai review Auto-review skipped because this PR opened with no labels. Added Worth noting for #1425, which is about this gate: the deadlock isn't only "inherited config excludes the PR." A PR opened with zero labels is skipped too, and the repo's Generated by Claude Code |
|
The zero-label skip can leave a successful but unreviewed check. The label application race makes this condition non-deterministic. I recorded this review-gating behavior for future reviews. ✏️ Learnings added
|
|
Correction to my comment above, plus a concrete defect behind it. I said "the The actual defect is in on:
pull_request:
types: [opened, reopened, synchronized]
Two consequences:
The one-word fix is Generated by Claude Code |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
The new disclosure tests used dQw4w9WgXcQ. `.github/copilot-instructions.md` requires auJzb1D-fag for all test data, and 60 files follow it against 6 stragglers — this file was one I had just added to that tail. Flagged by Vercel VADE review on #1440. The ID is inert here: fetch is stubbed, so the URL is never dereferenced and only reaches the strategy dispatch. 3/3 tests still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EX3tLZJJfbDUM6XXi6Uefm
|
Retracting my label-race diagnosis for this PR — I tested it and it's wrong. This PR now carries Two constraints in reviews:
auto_review:
drafts: false # ← this PR is a draft
base_branches:
- main # ← this PR is based on groupthinking-fix-upstream-error-leakageEither one alone is sufficient. I can't tell from outside which one CodeRabbit actually acted on — its skip message names labels in all cases, which is itself misleading — so I'm not going to guess a third time. Both are working as configured; neither is a bug. This PR is a stacked draft off a feature branch, which is exactly the shape auto-review is meant to skip. @coderabbitai — the learning you recorded from my earlier comment ("CodeRabbit auto-review can skip a pull request that opens with zero labels… the check can report success with 'Review skipped'") is built on that retracted inference. The zero-label state was real, but it was not shown to be the cause. Please drop or amend that learning. Two things from earlier that do still stand, both independently verified:
For #1425: the interesting case is #1381 — non-draft, based on Generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Tip For best results, initiate chat on the files or code changes.
I cannot remove that saved learning from this thread because its learning UUID is not available. The displayed Your corrected diagnosis is precise:
🐇🔎 ✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai full review This PR has never been reviewed. Its Worth noting because it sharpens #1424: #1440 carries Requesting review explicitly since this is a security change (CWE-209 information disclosure) that would otherwise merge with zero review coverage. Reviewers: the substantive question here is whether fixing the two caller-derived values in Generated by Claude Code |
|
I will verify that
|
Red-team pass on
|
| Guard message | Varies on |
|---|---|
Invalid URL |
caller input |
Blocked URL scheme: ${u.protocol} |
caller input, interpolated |
Blocked host |
server-side host policy |
Blocked private IP literal |
caller input |
NOT_PUBLIC |
unified — resolution outcome no longer distinguishable |
Why I am calling this low and not blocking: the dangerous case is closed. Every DNS-resolution path collapses to NOT_PUBLIC, so the resolve-time oracle #1381 set out to kill is genuinely dead — I checked all three throw sites. What remains is mostly derivable from the caller's own URL. The one residual is Blocked host vs NOT_PUBLIC, which reveals whether a hostname sits on a server-side blocklist. That is configuration disclosure, not a cross-origin read.
Why it is still worth fixing: the sentence is load-bearing. It is immediately followed by an instruction to future maintainers —
Keep that invariant when adding a strategy: anything interpolated into a
fetchTranscripterror reaches the client verbatim through the branches below.
— so it hands the next person a guarantee the file does not actually maintain, two lines above a live interpolation. Someone auditing this file later will read "every value is a fixed literal," trust it, and skip line 278.
Suggested change, docblock only: say that the branches carry app-authored values, and that line 278 is the one interpolation, with the resolve-oracle closed and the residual being host-policy disclosure. That makes the file honest without touching the guard.
I have not pushed this. Two reasons, both deliberate:
- This is not my branch, and the correct place for a
ssrf-guard.tsfix is contested. test(security): cover SSRF private-address detection main's tests miss #1428 introducesSsrfGuardErrorwith a publicmessageand a privatereason, which is the structurally right fix for line 278 and strictly better than anything I would patch in here. Editing the guard on this branch would deepen the fix(web): stop leaking upstream and Stripe error details to clients #1381/test(security): cover SSRF private-address detection main's tests miss #1428 conflict this PR already flags. - The docblock edit alone is a one-line change to a comment; batching it with whichever of fix(web): stop leaking upstream and Stripe error details to clients #1381/test(security): cover SSRF private-address detection main's tests miss #1428 wins is cheaper than a round trip now.
Nothing here blocks merge on its own. Recording it so it is not lost when the two SSRF branches are reconciled.
Generated by Claude Code
|
@coderabbitai full review Requesting explicitly because auto-review did not complete: the CodeRabbit commit status on head Worth flagging for the reviewer: this PR targets Generated by Claude Code |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Pull request metadata =='
gh pr view 1440 --repo groupthinking/EventRelay --json number,headRefName,headRefOid,baseRefName,baseRefOid,title
echo '== Incremental changed files and diff stat =='
git diff --stat "$(gh pr view 1440 --repo groupthinking/EventRelay --json baseRefOid --jq .baseRefOid)" HEAD
git diff --name-status "$(gh pr view 1440 --repo groupthinking/EventRelay --json baseRefOid --jq .baseRefOid)" HEAD
echo '== Incremental diff =='
git diff --find-renames --find-copies --unified=80 \
"$(gh pr view 1440 --repo groupthinking/EventRelay --json baseRefOid --jq .baseRefOid)" HEAD -- \
apps/web/src/lib/transcription-service.ts \
apps/web/src/app/api/transcribe/route.ts \
apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts
echo '== Relevant outlines =='
ast-grep outline apps/web/src/lib/transcription-service.ts --items all --view expanded
ast-grep outline apps/web/src/app/api/transcribe/route.ts --items all --view expanded
ast-grep outline apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts --items all --view expanded
echo '== fetchTranscript definitions and callers =='
rg -n -C 5 --glob '*.{ts,tsx,js,jsx}' '\bfetchTranscript\s*\(' apps/webLength of output: 32053 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== fetchTranscript implementation =='
sed -n '60,390p' apps/web/src/lib/transcription-service.ts
echo '== SSRF guard API and throw paths =='
ast-grep outline apps/web/src/lib/ssrf-guard.ts --items all --view expanded
sed -n '1,360p' apps/web/src/lib/ssrf-guard.ts
echo '== Transcribe route error-handling tests =='
fd -t f . apps/web/src | rg '(transcribe|transcription).*(test|spec)\.(ts|tsx|js|jsx)$|(__tests__.*(transcribe|transcription))'
rg -n -C 8 --glob '*.{test,spec}.{ts,tsx,js,jsx}' \
'(POST|/api/transcribe|transcription_unavailable|billing_not_configured|input_required|invalid_json)' apps/web/src
echo '== Production caller handling =='
sed -n '360,420p' apps/web/src/app/api/pipeline/route.ts
sed -n '260,305p' apps/web/src/app/api/pipeline/stream/route.ts
sed -n '245,275p' apps/web/src/lib/gemini-video-analyzer.ts
echo '== All dynamic error values from current service =='
rg -n -C 3 --type ts \
'error\s*:\s*(`|\$\{|.*\+)|error\s*:\s*[A-Za-z_$][A-Za-z0-9_$]*|throw new' \
apps/web/src/lib/transcription-service.ts apps/web/src/lib/ssrf-guard.tsLength of output: 169613 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Changed route documentation with line numbers =='
nl -ba apps/web/src/app/api/transcribe/route.ts | sed -n '8,45p'
echo '== Client-visible SSRF guard interpolation with line numbers =='
nl -ba apps/web/src/lib/transcription-service.ts | sed -n '268,286p'
echo '== Guard messages that can reach the interpolation =='
nl -ba apps/web/src/lib/ssrf-guard.ts | sed -n '104,158p'
echo '== Existing DNS disclosure regression test assertions =='
sed -n '1,180p' apps/web/src/lib/__tests__/ssrf-guard-dns-leakage.test.tsLength of output: 952 🏁 Script executed: #!/bin/bash
set -euo pipefail
print_range() {
local file="$1"
local start="$2"
local end="$3"
awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
}
echo '== Changed route documentation with line numbers =='
print_range apps/web/src/app/api/transcribe/route.ts 8 45
echo '== Client-visible SSRF guard interpolation with line numbers =='
print_range apps/web/src/lib/transcription-service.ts 268 286
echo '== Guard messages that can reach the interpolation =='
print_range apps/web/src/lib/ssrf-guard.ts 104 158
echo '== Existing DNS disclosure regression test assertions =='
print_range apps/web/src/lib/__tests__/ssrf-guard-dns-leakage.test.ts 1 180Length of output: 13151
P2 — Do not claim a fixed-error invariant while forwarding SSRF guard messages. The SSRF guard returns different messages for different outcomes. In particular, Return one fixed client-facing error from the guard rejection branch, such as The status and provider-key fixes are correctly placed in
|
Raised by CodeRabbit on #1440 (P2), and correct. The previous commit's docblock asserted that every `fetchTranscript` error value reaching the client is a fixed app-authored literal. That was false: the guard branch returned `Rejected audioUrl: ${guardErr.message}` verbatim. `assertPublicHttpUrl` throws five distinguishable messages — `Invalid URL`, `Blocked URL scheme: <protocol>`, `Blocked host`, `Blocked private IP literal`, and `Host does not resolve to a public address`. Forwarding them told the caller WHICH rule fired. `Blocked host` confirms a hostname-blocklist match; the resolution message confirms only that DNS returned nothing public. That difference is a policy oracle, and it sharpens as BLOCKED_HOSTNAMES grows. All five rejections now return one fixed `Rejected audioUrl`; the real reason is logged. With this, all seven of the service's error values are fixed literals — verified by enumeration, not assumed. The docblock overstated in the same way #1381's did, which is what this PR was opened to correct, so it has been rewritten to state what is actually true and to name what it does NOT claim: the choice among literals still tells a caller whether their host cleared the guard. That residue is inherent to a guard that refuses some inputs and attempts others, is far coarser than naming the rule, and the route is session-gated — accepted, and now written down rather than glossed. Complementary to #1428, which fixes this at the guard level with SsrfGuardError (public message + private reason). This is the call-site boundary and does not touch ssrf-guard.ts, so the two do not conflict. Verification (head 8d756b6 + this, apps/web): - New test drives all five real guard messages through fetchTranscript and asserts the caller-visible error is identical. Fails without the fix (`expected 5 to be 1`), passes with it. - vitest run: 1 failed | 291 passed (292). The failure, billing-chat-gating "blocks free tier after daily quota", is pre-existing and unrelated — confirmed earlier on the untouched base. Tracked in #1116. - npm run type-check: clean. npm run lint: clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EX3tLZJJfbDUM6XXi6Uefm
|
P2 confirmed and fixed in The docblock I wrote claimed every Fix: all five guard rejections — Regression test (as requested): drives all five real guard messages through I also enumerated rather than assumed this time — all seven One thing I did not do, deliberately: collapse Relationship to #1428: complementary, not overlapping. #1428 fixes this at the guard level with
Noting for the record that your review ran under "Review rate limited / Action not completed" and produced this finding anyway — the account is at its Fair Usage limit, which is worth knowing separately since it means auto-review is unreliable across this repo right now. Generated by Claude Code |
8e22f20
into
groupthinking-fix-upstream-error-leakage
…1381) * fix(web): stop leaking upstream and Stripe error details to clients Two information-disclosure issues were returning third-party error text verbatim to callers. 1. /api/realtime/session (GET + POST) returned the raw OpenAI response body as `details` while mirroring `upstream.status`. OpenAI bodies echo org/project IDs, quota state, and on 401 a partial key (`Incorrect API key provided: sk-proj-****ABCD`). Both handlers now log the body server-side and return a static message with a fixed 502. 2. Five billing routes returned raw Stripe SDK `err.message`. All five are on the unauthenticated PUBLIC_API_EXACT allowlist, so any internet caller could trigger and read them. Each now returns a static string plus a machine-readable code; the raw message stays in console.error and kaizenObserve. /api/transcribe had the same raw-upstream pattern in its JSON-parse and catch-all branches and is fixed the same way. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs(web): correct two inaccurate security comments flagged in review Applies the two fixes agreed in the Copilot review threads on #1381. Both are comment-only; no behavior, status code, or response body changes. transcribe/route.ts — the docblock claimed all error responses are static and machine-readable. The four `!result.success` branches return `fetchTranscript`'s message verbatim and carry no `code`, and the billing branch still returns a static `details` hint. Those messages are app-authored (every `error` in transcription-service.ts is a literal, a numeric status, or our own SSRF-guard text), so the CWE-209 objective holds — but the docblock overstated it. Scoped it to the two sanitized paths and described the rest accurately. billing/webhook/route.ts — the rationale was wrong on two counts. `constructEvent` verifies Stripe-Signature against the webhook signing secret, not the API key, so an API-key fragment cannot forge a signature; and StripeSignatureVerificationError describes tolerance/scheme/digests without echoing key material. Rewrote it to state the real risk: on an unauthenticated route, verification internals are an oracle for tuning replay/timestamp attacks. Verified at this head: type-check clean, eslint clean, the six route test files pass (17 tests), full web suite 261 passed / 1 failed — the failure being the pre-existing billing-chat-gating timeout tracked in #1116, which fails identically on the base commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jsfy3ts3U1ggGsJqFB2KSC * fix(web): return the static 502 when the Realtime upstream is unreachable CodeRabbit flagged this on #1381 and it reproduces: neither GET nor POST wrapped its `fetch`, so a transport-level failure — DNS, TLS, connection reset, timeout — rejected before `upstreamFailure` could run. The throw escaped to Next.js's own unstructured 500 instead of the `{ error, code }` JSON the route promises, `use-realtime-voice.ts` got a payload it cannot parse, and no `[realtime]` line was logged for the operator. Both handlers now wrap the fetch and the body read in try/catch and route transport failures through a new `upstreamUnreachable` helper, which logs the cause server-side and returns the same fixed 502 as a non-OK upstream response. The cause is an Error from our own transport rather than upstream text, so logging it introduces no disclosure. `await upstream.text()` is inside the try as well — a truncated body rejects there for the same reason. The gap predates this PR, but this PR is what establishes the contract that these routes always answer with a static structured error, so it belongs here. Three regression tests, each verified non-vacuous — they fail against the unwrapped route and pass against the fix: GET with a rejected fetch (ENOTFOUND), POST with a rejected fetch (socket hang up), and GET with a response whose body read rejects mid-stream. Verified at this head: tsc clean, eslint clean, full web suite 264 passed / 1 failed — the pre-existing billing-chat-gating timeout tracked in #1116, identical on the base commit. Up from 261 passed by exactly these 3 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jsfy3ts3U1ggGsJqFB2KSC * fix(web): complete the error-code contract across the patched routes CodeRabbit's review of 1adbed6 found the `code` field was added only to the branches that were sanitized for CWE-209, leaving the rest of each route's error surface without a machine-readable key. A client cannot branch on a contract that only half the branches honour. Every remaining error branch on the six patched routes now returns `code` alongside its existing `error` value: - realtime/session — `realtime_not_configured` (GET and POST missing-key), `invalid_sdp_offer` - billing/activate — `invalid_json`, `session_id_required`, `not_eligible` - billing/checkout — `invalid_json`, `turnstile_rejected` - billing/renew — `invalid_json` - billing/webhook — `webhook_not_configured`, `missing_signature` `error` values are unchanged, so no client contract breaks. The checkout Turnstile branch additionally gains a fallback: `turnstile.error` is optional on `TurnstileVerifyResult`, so the previous `{ error: turnstile.error }` could serialize to `{}`. Its values are all our own literals, never Cloudflare response text, so returning it verbatim remains correct. Also from the same review: - `upstreamFailure` and `upstreamUnreachable` declare `: Response`. - `activateRequest`/`webhookRequest` declare `: NextRequest`, `transcribeRequest` declares `: Request`. The review's remaining finding — that a rejected `fetch()` skips `upstreamFailure` entirely — was already fixed in ae2be83, which landed after the review ran. +9 tests covering each newly-coded branch. `npm test` 271 passed / 1 failed; the single failure is the pre-existing `billing-chat-gating` 5000 ms timeout (#1116), identical on the base commit. `npm run lint` and `npm run type-check` clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DSkV9zjdhpkfUSMeLhrFGy * fix(web): annotate remaining helper return types and cover the Turnstile fallback Absorbs the three items #1402 carried that this branch did not, so the two lines of work converge here rather than diverging: - `getOpenAiHeaders` declares `: Record<string, string> | null` - `sdpRequest` declares `: Request`, `upstreamResponse` declares `: Response` - a test for `verifyTurnstileToken` returning `{ ok: false }` with no `error`, which the optional-field fallback added in 5e69b7e handles but nothing exercised #1402's `fetchUpstream` rewrite is deliberately not taken. It guards only the `fetch` call, leaving `await upstream.text()` outside the try — so a connection reset mid-body still escapes the route. `upstreamUnreachable` (ae2be83) wraps both, which is the stricter guarantee. `npm test` 272 passed / 1 failed — the pre-existing `billing-chat-gating` timeout (#1116), identical on the base commit. `npm run lint` and `npm run type-check` clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DSkV9zjdhpkfUSMeLhrFGy * fix(web): stop verifyTurnstileToken rejecting into an unstructured 500 CodeRabbit's review of the previous head found this and it reproduces. verifyTurnstileToken returns a TurnstileVerifyResult on every other exit, so its sole caller — the unauthenticated /api/billing/checkout route — awaits it outside any try/catch. Two awaits inside it break that contract: the siteverify fetch rejects on a transport failure, and res.json() rejects when Cloudflare answers 2xx with a truncated or non-JSON body. Either escaped POST as an unstructured framework 500 with no kaizenObserve trace and no `code` — the same disclosure-shaped hole ae2be83 closed on the realtime route. Fixed at the source rather than at the call site: the function now honours its own Promise<TurnstileVerifyResult> signature, so every present and future caller is covered. The reject reason is logged server-side — undici puts the resolved host and port in it (`connect ECONNREFUSED 10.0.3.14:443`) — and collapsed into the app-authored literal `turnstile_verification_unavailable`. The route's existing 403 branch handles it unchanged, so no status code or response shape moves. Note the 403 is now also reachable when Cloudflare is merely unreachable rather than rejecting; keeping the existing status was the narrower choice, and the distinction is visible to operators in the log and to clients in the `error` literal. Five tests, verified non-vacuous — reverting the guard fails three of them: rejected fetch, rejected res.json(), and a non-rejection contract check, plus the accept and non-2xx paths. They assert the resolved host/port never rides out on the result while the reason still reaches console.error. Verified at this head: tsc clean, eslint clean, full web suite 277 passed / 1 failed — the pre-existing billing-chat-gating 5000 ms timeout (#1116), which is also intermittently joined by a second timeout under load. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jsfy3ts3U1ggGsJqFB2KSC * fix(web): complete the error-code contract on the transcribe route CodeRabbit flagged the `!result.success` branches of /api/transcribe as a CWE-209 leak. Traced every value `fetchTranscript` can put in `error` before changing anything: each one is an app-authored literal (transcription-service 75, 296, 303, 314), a numeric HTTP status (287), our own SSRF-guard message (278, and every throw in ssrf-guard interpolates only the caller's own URL), or a conditional literal (363). Upstream throws are swallowed at console.warn and never reach `error`. So the leak half of that finding does not hold. The contract half does. This PR established `error` + a stable machine-readable `code` on every error branch of the five billing routes and realtime/session; transcribe was the last surface still missing it. The docblock documented the gap instead of closing it. All five error branches now carry a code — input_required, rate_limited, billing_not_configured, transcription_unavailable, alongside the existing invalid_json and transcription_failed. The `error` strings are untouched, so the human-facing contract is unchanged, matching the precedent already accepted for the Turnstile branch: dynamic message, pinned code. The `details` hint on the billing branch is kept. It is an app-authored operator string, not upstream text; the docblock no longer claims otherwise. Tests: +5. One asserting the missing-input branch short-circuits before fetchTranscript, and four table-driven cases pinning each resolved-failure branch's status and code while asserting no provider-shaped token reaches the client. Also added vi.clearAllMocks() to the afterEach — restoreAllMocks only restores spies, so the module-factory vi.fn() was leaking call history between tests in this file. apps/web: type-check clean, eslint clean, 282 passed / 1 failed — the pre-existing billing-chat-gating timeout tracked in #1116. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y1xcdTagvCU4K7ZwTgGQF6 * fix(web): stop the SSRF guard leaking DNS resolver text to the caller CodeRabbit found the last CWE-209 path on this PR's surface, and it is a real one. `assertPublicHttpUrl` throws app-authored literals at every exit except `dns.lookup`, which was awaited bare: ssrf-guard.ts:121 await dns.lookup(host, { all: true }) -> rejects `getaddrinfo ENOTFOUND <submitted-hostname>` transcription-service.ts:273-280 -> `Rejected audioUrl: ${guardErr.message}` transcribe/route.ts:104-113 -> returns that verbatim as `error` in a 503 So a caller could name any host and read back the server's resolver verdict on it. That is system error text reaching a client, which is this PR's whole objective — but the sharper problem is that it is a DNS oracle. ENOTFOUND vs EAI_AGAIN vs the existing 'Host resolves to a private address' distinguishes "no such name" from "exists but private" from "exists and public", letting a caller map internal names from the server's network position. That is exactly the reconnaissance this guard exists to block, leaking out of the guard. Fixed at the source rather than at either call site, so present and future callers of `assertPublicHttpUrl` are covered: the lookup is wrapped, the reject reason is logged server-side, and every resolver failure collapses into 'Host does not resolve' — the literal a zero-result lookup already threw, so the oracle closes in both directions. This also corrects a claim made earlier on this PR. The transcribe docblock thread asserted every `result.error` value was app-authored and listed line 278 as "our own SSRF-guard message". That held for the five explicit `throw new Error(...)` sites but not for the `dns.lookup` rejection, which is Node's. The docblock scoping stands; the reasoning behind it was incomplete. Severity is bounded: `/api/transcribe` is not on `PUBLIC_API_EXACT`, so this needs an authenticated caller, unlike the billing routes. Four tests, verified non-vacuous — reverting the try/catch fails exactly the two leakage tests (resolver text suppressed but still logged; ENOTFOUND and EAI_AGAIN indistinguishable from an empty result) while the two behaviour tests still pass, since private-address blocking and the public-host allow path do not depend on the catch. Verified at this head: tsc clean, eslint clean, web suite 286 passed / 1 failed — the pre-existing billing-chat-gating 5000 ms timeout (#1116). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DSkV9zjdhpkfUSMeLhrFGy * fix(web): close the last DNS oracle in the SSRF guard Vercel's review bot found this on 8628b5e and it is correct — 36645ad closed only half the hole it described. That commit collapsed the resolver-failure cases (ENOTFOUND, EAI_AGAIN, zero results) into one literal, but left the private-address branch throwing a distinct one: Host does not resolve <- the name does not exist Host resolves to a private address <- the name EXISTS, and is internal That difference is the sharpest oracle of the set. `fetchTranscript` interpolates the message into `Rejected audioUrl: ${guardErr.message}` (transcription-service.ts:278) and the transcribe route returns it verbatim on the `!result.success` path, so an unauthenticated caller could guess internal hostnames and read back whether each one exists and points at internal infrastructure — the reconnaissance this guard exists to prevent. 36645ad's own comment names "Host resolves to a private address" as part of the oracle and then does not unify it. All four non-public outcomes now throw one literal, `Host does not resolve to a public address`, which is true of every one of them. The resolved address is logged for operators instead. The IP-literal branch keeps its own distinct message deliberately: the caller supplied that address, so naming it private reveals nothing they did not already know, and no hostname is confirmed or denied. Two tests, verified non-vacuous — restoring the old literal fails both: one asserts the private address never rides out on the rejection while still reaching console.error, the other feeds the same hostname down the does-not-exist and exists-but-internal paths and asserts the caller cannot tell them apart. npm test 287 passed / 1 failed — the pre-existing billing-chat-gating timeout (#1116). lint and type-check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DSkV9zjdhpkfUSMeLhrFGy * fix(web): stop /api/transcribe leaking probe status and key config #1381 sanitized the thrown and JSON-parse paths of /api/transcribe but left the `!result.success` branches returning `fetchTranscript`'s message verbatim, on the stated grounds that every such value is an app-authored literal. Two were not, and both are reachable by a caller who controls the request body: - `Failed to fetch audio: ${audioResponse.status}` echoed the HTTP status of the caller-supplied `audioUrl`. The SSRF guard admits public hosts, so this returned a cross-origin read (401 vs 403 vs 404 vs 500) that the browser's same-origin policy would otherwise deny — a probe oracle, not upstream text, which is why it survived the original pass. - The all-strategies-failed message branched on whether provider keys were set, disclosing server configuration and naming both the variables and the hosting platform. Both are fixed at the source in transcription-service.ts rather than masked at the route, so any future caller of fetchTranscript inherits the fix. The real status and the real key state are logged for operators, where they were the only thing actionable. Also drops the residual `details` field from the billing branch, which named this deployment's cloud and model vendors — #1381 stated `details` was gone from this route, and this is the one that was left. The route docblock carried the claim these values disproved; it now states the invariant and what enforces it. Verification (head e9bf62d, apps/web): - 3 new tests in transcription-error-disclosure.test.ts; all 3 fail against the unfixed service and pass with it — the status test asserts four distinct upstream statuses collapse to one message, so a partial fix cannot pass it. - vitest run: 1 failed | 290 passed (291). The single failure, billing-chat-gating "blocks free tier after daily quota", is pre-existing and unrelated — confirmed failing identically on the untouched e9bf62d with this change stashed. Tracked in #1116; #1230 makes the suite hermetic against it. - npm run type-check: clean. npm run lint: clean. - package-lock.json drift from npm install reverted, per #1381's convention. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EX3tLZJJfbDUM6XXi6Uefm * test: use the repo's standard fixture video ID The new disclosure tests used dQw4w9WgXcQ. `.github/copilot-instructions.md` requires auJzb1D-fag for all test data, and 60 files follow it against 6 stragglers — this file was one I had just added to that tail. Flagged by Vercel VADE review on #1440. The ID is inert here: fetch is stubbed, so the URL is never dereferenced and only reaches the strategy dispatch. 3/3 tests still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EX3tLZJJfbDUM6XXi6Uefm * fix(web): stop forwarding the SSRF guard's rejection reason Raised by CodeRabbit on #1440 (P2), and correct. The previous commit's docblock asserted that every `fetchTranscript` error value reaching the client is a fixed app-authored literal. That was false: the guard branch returned `Rejected audioUrl: ${guardErr.message}` verbatim. `assertPublicHttpUrl` throws five distinguishable messages — `Invalid URL`, `Blocked URL scheme: <protocol>`, `Blocked host`, `Blocked private IP literal`, and `Host does not resolve to a public address`. Forwarding them told the caller WHICH rule fired. `Blocked host` confirms a hostname-blocklist match; the resolution message confirms only that DNS returned nothing public. That difference is a policy oracle, and it sharpens as BLOCKED_HOSTNAMES grows. All five rejections now return one fixed `Rejected audioUrl`; the real reason is logged. With this, all seven of the service's error values are fixed literals — verified by enumeration, not assumed. The docblock overstated in the same way #1381's did, which is what this PR was opened to correct, so it has been rewritten to state what is actually true and to name what it does NOT claim: the choice among literals still tells a caller whether their host cleared the guard. That residue is inherent to a guard that refuses some inputs and attempts others, is far coarser than naming the rule, and the route is session-gated — accepted, and now written down rather than glossed. Complementary to #1428, which fixes this at the guard level with SsrfGuardError (public message + private reason). This is the call-site boundary and does not touch ssrf-guard.ts, so the two do not conflict. Verification (head 8d756b6 + this, apps/web): - New test drives all five real guard messages through fetchTranscript and asserts the caller-visible error is identical. Fails without the fix (`expected 5 to be 1`), passes with it. - vitest run: 1 failed | 291 passed (292). The failure, billing-chat-gating "blocks free tier after daily quota", is pre-existing and unrelated — confirmed earlier on the untouched base. Tracked in #1116. - npm run type-check: clean. npm run lint: clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EX3tLZJJfbDUM6XXi6Uefm --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
) A `pull_request.branches:` allowlist filters on the PR's *base* ref. Six gates carried `branches: [main]` (or `[main, develop]`), so a PR stacked onto another PR's branch matched none of them and ran with no CI, no CodeQL, no security scan, no coverage, no e2e and no dependency review. This is not hypothetical. #1440 -- "fix(web): stop /api/transcribe leaking probe status and key config", a non-draft security fix -- targets #1381's branch and has zero CI runs. Its only green checks are the Vercel deployment and CodeRabbit. Every test gate in the repo silently vanished because of the base it happens to target. Dropping the allowlist from the `pull_request` trigger makes each gate fire for every PR regardless of base. The `push` triggers are untouched and stay pinned to main, so branch pushes do not start running the full suite twice. e2e-tests.yml keeps its existing `vars.E2E_BASE_URL` job-level guard, so its PR behaviour is unchanged apart from now being reachable at all. Non-vacuous: tests/unit/test_workflow_pr_gate_coverage.py run against the pre-fix workflows from origin/main gives 6 failed, 12 passed -- one failure per gate, exactly the allowlist assertion. Against the fix: 18 passed. The push-trigger test is what fails if a future edit widens the `push` scope while removing the PR filter. Verified: 27 passed across the two workflow-YAML test modules (test_pr_governance_workflow.py unchanged at 9); all 32 workflow files still parse as YAML. Claude-Session: https://claude.ai/code/session_014ubMueWZScQy2KgsQ4uHQi Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Canonical issue
Closes #1441
Filed after
linear-code[bot]marked this PR ready for review, which armed the fullPR Governancecontract on what had been an intentionally-draft stacked branch. The gate's ask was legitimate and achievable, so #1441 records the defect properly rather than working around the check.This targets
groupthinking-fix-upstream-error-leakagerather thanmain— it stacks onto #1381 instead of competing with it. Merge this into #1381, then merge #1381.Outcome
/api/transcribe's!result.successbranches no longer return caller-derived or policy-derived information.#1381 sanitized the thrown and JSON-parse paths, and its docblock argued the remaining branches were safe because "every one of those values is an app-authored literal, a numeric HTTP status, or our own SSRF-guard message, so none is upstream text."
That reasoning is right about upstream text and right about most branches — but it scopes the problem to the wrong category. Three values leak something other than upstream text, which is why they survived the first pass:
Failed to fetch audio: ${audioResponse.status}audioUrl. The SSRF guard admits public hosts, so this hands back a cross-origin read (401 vs 403 vs 404 vs 500) that the browser's same-origin policy would otherwise deny. A probe oracle.Rejected audioUrl: ${guardErr.message}Blocked hostconfirms a hostname-blocklist match;Host does not resolve to a public addressconfirms only that DNS returned nothing public. A policy oracle that sharpens asBLOCKED_HOSTNAMESgrows. (Found by CodeRabbit on this PR — see "Honest note" below.)All three are fixed at the source in
transcription-service.tsrather than masked at the route, so every current and future caller offetchTranscriptinherits the fix. The real status, key state, and guard reason are logged for operators — the only place any of them was actionable.All seven
error:values intranscription-service.tsare now fixed literals, verified by enumeration rather than assumed:grep -nE "error:\s*\"` returns nothing across both service files.Also drops the residual
detailsfield from the billing branch, which named this deployment's cloud and model vendors. #1381's description statesdetailswas removed from this route; this is the one instance that was left.Honest note on the docblock
The first revision of this PR replaced #1381's overstated invariant with another overstated invariant — it claimed every value was a fixed literal while
transcription-service.ts:278still forwardedguardErr.message. CodeRabbit caught it (P2). Fixed ine511781, and the docblock now also carries an explicit "what this deliberately does NOT claim" paragraph: the choice among literals still tells a caller whether their host cleared the guard. That residue is inherent to a guard that refuses some inputs and attempts others, is far coarser than naming the rule, and the route is session-gated — accepted, and written down rather than glossed.Scope
apps/web/src/lib/transcription-service.ts(3 error values),apps/web/src/app/api/transcribe/route.ts(docblock +details), andapps/web/src/lib/__tests__/transcription-error-disclosure.test.ts(new, 4 tests).ssrf-guard.tsitself. This fixes the call-site boundary; test(security): cover SSRF private-address detection main's tests miss #1428 fixes the same class at the guard level withSsrfGuardError(publicmessage+ privatereason), which is the better long-term shape. The two are complementary and touch no common file.ssrf-guard.ts— see the note at the bottom.billing-chat-gatingtimeout (pre-existing, billing-chat-gating free-tier test depends on ambient AI_GATEWAY_API_KEY and passes vacuously in CI #1116, addressed by test: make web suite hermetic against ambient AI gateway keys #1230) and the static-literal branches, which are genuinely fine as fix(web): stop leaking upstream and Stripe error details to clients #1381 argued.auto-label.ymlsynchronized→synchronizetypo found while working here. It belongs on a branch offmain; committing it here would land a CI fix inside fix(web): stop leaking upstream and Stripe error details to clients #1381. Written up in the thread below.Risk
code, which is unchanged; nocode, status, or success-path semantic is touched.git revert. No migration, config, or schema change.Verification
Head
e511781, basee9bf62d4. Measured inapps/web, not inferred.Focused tests — 4 tests, each failing against the unfixed service and passing with it. Two assert on the count of distinct messages, so a partial fix cannot pass them:
The guard test drives all five real messages
assertPublicHttpUrlthrows —Invalid URL,Blocked URL scheme: <protocol>,Blocked host,Blocked private IP literal,Host does not resolve to a public address— and asserts one distinct caller-visible error.Full suite —
npx vitest run: 1 failed | 291 passed (292). The single failure isbilling-chat-gating.test.ts"blocks free tier after daily quota" (5000 ms timeout), pre-existing and unrelated — confirmed failing identically on the untouchede9bf62d4with this change stashed. Tracked in billing-chat-gating free-tier test depends on ambient AI_GATEWAY_API_KEY and passes vacuously in CI #1116.npm run type-check— clean, exit 0.npm run lint— clean, exit 0.package-lock.jsondrift fromnpm installreverted, per fix(web): stop leaking upstream and Stripe error details to clients #1381's own convention.Required checks on
e511781—PR Governance✅,Canonical issue and evidence✅,gitleaks (working tree)✅,validate✅.Review threads — Vercel VADE's fixture-ID finding fixed in
8d756b6(markedISSUE_RESOLVED). CodeRabbit's P2 fixed ine511781. CodeRabbit auto-review is skipped on this PR by.coderabbit.yaml(drafts: false,base_branches: [main]), so its reviews here are manually triggered.Production evidence
Vercel preview builds green on this branch. This diff is under
apps/web/**, soMERGE_POLICY.mdgate 4 applies and the preview is the relevant artifact. No runtime behavior is observable from the preview itself — the changed paths are error branches reachable only with provider credentials configured and a caller-suppliedaudioUrl— so the regression tests above carry the real evidence.Agent handoff
branch-cleanup.ymlis a push-event workflow, not a PR check: it fails identically on every branch in the repo because of an invalidworkflows: writepermission scope, and fix(ci): drop invalidworkflowspermission scope from branch-cleanup #1420 fixes it. It does not gate this PR.Agent provenance
Agent-authored, under the PR remediation runbook. Found by verifying #1381's outstanding CodeRabbit review rather than trusting its status.
#1381 and #1428 both rewrite
apps/web/src/lib/ssrf-guard.tsto close the same CWE-209 DNS oracle, and neither references the other:NOT_PUBLICmessage and deliberately keeps the IP-literal branch distinct, with a documented rationale.SsrfGuardErrorcarrying a publicmessageplus a privatereason, and unifies every branch including scheme, blocked host, and IP literal.They will conflict. #1428's approach is the more complete of the two, so the cheaper order is probably to land #1381 (with this PR merged into it) and then rebase #1428 onto it, keeping #1428's
SsrfGuardError— rather than the reverse. Flagging rather than acting: which design wins is a call for a human.