From 18a787a55be41ffe3a61ff3a28f51a819c22e985 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 19:09:45 +0000 Subject: [PATCH 1/3] fix(web): stop /api/transcribe leaking probe status and key config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 e9bf62d4, 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 e9bf62d4 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 Claude-Session: https://claude.ai/code/session_01EX3tLZJJfbDUM6XXi6Uefm --- apps/web/src/app/api/transcribe/route.ts | 20 +++- .../transcription-error-disclosure.test.ts | 104 ++++++++++++++++++ apps/web/src/lib/transcription-service.ts | 28 ++++- 3 files changed, 141 insertions(+), 11 deletions(-) create mode 100644 apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts diff --git a/apps/web/src/app/api/transcribe/route.ts b/apps/web/src/app/api/transcribe/route.ts index ef63574f0..e77251521 100644 --- a/apps/web/src/app/api/transcribe/route.ts +++ b/apps/web/src/app/api/transcribe/route.ts @@ -19,11 +19,17 @@ export const maxDuration = 120; * carry raw upstream provider text (account IDs, quota state, partial API keys) * — that text is logged server-side only. * - * The `!result.success` branches keep `fetchTranscript`'s own message in `error`: - * 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. Preserving them verbatim - * keeps the human-facing string contract intact while `code` carries the stable - * key for programmatic handling. + * The `!result.success` branches keep `fetchTranscript`'s own message in `error`. + * That is safe only because every one of those values is now a fixed, + * app-authored literal. Two of them were not, and were removed at the source in + * `transcription-service.ts` rather than masked here: + * - the caller-supplied `audioUrl`'s HTTP status, which made this route a + * cross-origin probe, and + * - a message that varied on whether provider API keys were configured, which + * disclosed server configuration. + * Both are logged server-side instead. Keep that invariant when adding a + * strategy: anything interpolated into a `fetchTranscript` error reaches the + * client verbatim through the branches below. * - 400: Invalid input (missing URL, malformed JSON) — `input_required` / `invalid_json` * - 429: Rate limited (implement backoff or upgrade service plan) — `rate_limited` * - 500: Service unavailable (check API keys and billing) — `billing_not_configured` @@ -94,7 +100,9 @@ export async function POST(request: Request) { success: false, error: result.error, code: 'billing_not_configured', - details: 'Configure billing in Google Cloud and OpenAI console', + // No `details` here: the remediation text named this deployment's + // cloud and model vendors, and the PR that sanitized this route + // stated `details` was gone from it. `code` is what clients branch on. transcript: '', }, { status: 500 } diff --git a/apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts b/apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts new file mode 100644 index 000000000..c4bbc7120 --- /dev/null +++ b/apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts @@ -0,0 +1,104 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; + +// `fetchTranscript`'s error string is returned to the caller verbatim by +// `/api/transcribe`'s `!result.success` branches. That is only safe while every +// value it can carry is a fixed, app-authored literal. These tests pin the two +// values that were not: the caller-supplied `audioUrl`'s HTTP status, and a +// message that varied on whether provider API keys were configured. + +vi.mock('server-only', () => ({})); +vi.mock('@/lib/ssrf-guard', () => ({ + assertPublicHttpUrl: vi.fn(async (input: string) => new URL(input)), +})); +vi.mock('@/lib/youtube-metadata', () => ({ + fetchYouTubeMetadata: vi.fn(async () => null), + formatMetadataAsContext: vi.fn(() => ''), +})); +vi.mock('@/lib/gemini-client', () => ({ + getGeminiClient: vi.fn(() => null), + hasGeminiKey: vi.fn(() => false), +})); +vi.mock('@/lib/vercel-ai-gateway', () => ({ + gatewayChat: vi.fn(async () => null), + hasAiGatewayKey: vi.fn(() => false), + toGatewayModelId: vi.fn((m: string) => m), +})); + +import { fetchTranscript } from '@/lib/transcription-service'; + +const AUDIO_URL = 'https://cdn.example.com/clip.mp3'; + +let consoleError: ReturnType; +const savedEnv = { ...process.env }; + +beforeEach(() => { + consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.spyOn(console, 'warn').mockImplementation(() => {}); +}); + +afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + process.env = { ...savedEnv }; +}); + +describe('fetchTranscript error disclosure', () => { + it('does not echo the status of a caller-supplied audioUrl', async () => { + process.env.OPENAI_API_KEY = 'sk-test-key'; + // 403 from a host the caller chose. Echoing it back is a cross-origin read + // the browser's same-origin policy would otherwise deny them. + vi.stubGlobal( + 'fetch', + vi.fn(async () => new Response('forbidden', { status: 403 })), + ); + + const result = await fetchTranscript({ audioUrl: AUDIO_URL }); + + expect(result.success).toBe(false); + expect(result.error).toBe('Could not retrieve the audio file'); + // The probe result must not survive anywhere in the caller-visible payload. + expect(JSON.stringify(result)).not.toContain('403'); + // ...but an operator can still see it. + expect(consoleError).toHaveBeenCalledWith(expect.stringContaining('403')); + }); + + it('reports the same status-free message whatever the upstream status is', async () => { + process.env.OPENAI_API_KEY = 'sk-test-key'; + const errors: string[] = []; + + for (const status of [401, 403, 404, 500]) { + vi.stubGlobal( + 'fetch', + vi.fn(async () => new Response('nope', { status })), + ); + const result = await fetchTranscript({ audioUrl: AUDIO_URL }); + errors.push(result.error ?? ''); + } + + // One distinguishable message per status would rebuild the oracle. + expect(new Set(errors).size).toBe(1); + }); + + it('does not disclose whether provider API keys are configured', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => new Response('{}', { status: 500 })), + ); + + delete process.env.OPENAI_API_KEY; + const withoutKeys = await fetchTranscript({ url: 'https://youtu.be/dQw4w9WgXcQ' }); + + process.env.OPENAI_API_KEY = 'sk-test-key'; + const withKeys = await fetchTranscript({ url: 'https://youtu.be/dQw4w9WgXcQ' }); + + expect(withoutKeys.success).toBe(false); + expect(withKeys.success).toBe(false); + // Configuration state is not a caller-facing fact. + expect(withoutKeys.error).toBe(withKeys.error); + expect(withoutKeys.error).not.toMatch(/OPENAI_API_KEY|GEMINI_API_KEY|Vercel/i); + // The distinction stays available to operators. + expect(consoleError).toHaveBeenCalledWith( + expect.stringContaining('no OPENAI_API_KEY or GEMINI_API_KEY configured'), + ); + }); +}); diff --git a/apps/web/src/lib/transcription-service.ts b/apps/web/src/lib/transcription-service.ts index 06333497c..fed35d6bc 100644 --- a/apps/web/src/lib/transcription-service.ts +++ b/apps/web/src/lib/transcription-service.ts @@ -282,9 +282,17 @@ ${metadataContext ? `\nKNOWN METADATA:\n${metadataContext}` : ''}`, const audioResponse = await fetch(audioUrl, { signal: AbortSignal.timeout(30_000) }); if (!audioResponse.ok) { + // The status belongs to a caller-supplied `audioUrl`. Echoing it turns + // this route into a probe: the caller learns 401 vs 403 vs 404 vs 500 + // for any host the SSRF guard admits, which is a cross-origin read the + // browser same-origin policy would otherwise deny them. Log it, and + // report only that the fetch failed. + console.error( + `[transcription] audioUrl fetch failed with status ${audioResponse.status}`, + ); return { success: false, - error: `Failed to fetch audio: ${audioResponse.status}`, + error: 'Could not retrieve the audio file', transcript: '', }; } @@ -356,13 +364,23 @@ ${metadataContext ? `\nKNOWN METADATA:\n${metadataContext}` : ''}`, } } - // No strategy succeeded + // No strategy succeeded. + // + // Which provider keys this deployment holds is server configuration, not + // caller-facing detail: branching the client message on `hasKeys` told any + // caller whether OPENAI_API_KEY/GEMINI_API_KEY were set, and named the + // variables and the hosting platform. Both outcomes now report the same + // string; the distinction survives in the operator log, which is the only + // place it was ever actionable. const hasKeys = !!(process.env.OPENAI_API_KEY || hasGeminiKey()); + console.error( + hasKeys + ? '[transcription] all strategies failed with provider keys configured' + : '[transcription] all strategies failed: no OPENAI_API_KEY or GEMINI_API_KEY configured', + ); return { success: false, - error: hasKeys - ? 'Could not transcribe video — all strategies failed' - : 'No AI API key configured. Set OPENAI_API_KEY or GEMINI_API_KEY in Vercel environment variables.', + error: 'Could not transcribe video — all strategies failed', transcript: '', }; } From 8d756b606449db5a675e9f574fde1950711bb824 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 19:14:16 +0000 Subject: [PATCH 2/3] test: use the repo's standard fixture video ID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01EX3tLZJJfbDUM6XXi6Uefm --- .../src/lib/__tests__/transcription-error-disclosure.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts b/apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts index c4bbc7120..a1e170b08 100644 --- a/apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts +++ b/apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts @@ -86,10 +86,10 @@ describe('fetchTranscript error disclosure', () => { ); delete process.env.OPENAI_API_KEY; - const withoutKeys = await fetchTranscript({ url: 'https://youtu.be/dQw4w9WgXcQ' }); + const withoutKeys = await fetchTranscript({ url: 'https://youtu.be/auJzb1D-fag' }); process.env.OPENAI_API_KEY = 'sk-test-key'; - const withKeys = await fetchTranscript({ url: 'https://youtu.be/dQw4w9WgXcQ' }); + const withKeys = await fetchTranscript({ url: 'https://youtu.be/auJzb1D-fag' }); expect(withoutKeys.success).toBe(false); expect(withKeys.success).toBe(false); From e511781603bed6086d8385ef70e0275b4dfaa850 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 19:40:40 +0000 Subject: [PATCH 3/3] fix(web): stop forwarding the SSRF guard's rejection reason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: `, `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 Claude-Session: https://claude.ai/code/session_01EX3tLZJJfbDUM6XXi6Uefm --- apps/web/src/app/api/transcribe/route.ts | 28 ++++++++---- .../transcription-error-disclosure.test.ts | 45 +++++++++++++++++-- apps/web/src/lib/transcription-service.ts | 11 ++++- 3 files changed, 72 insertions(+), 12 deletions(-) diff --git a/apps/web/src/app/api/transcribe/route.ts b/apps/web/src/app/api/transcribe/route.ts index e77251521..3d375e956 100644 --- a/apps/web/src/app/api/transcribe/route.ts +++ b/apps/web/src/app/api/transcribe/route.ts @@ -20,16 +20,28 @@ export const maxDuration = 120; * — that text is logged server-side only. * * The `!result.success` branches keep `fetchTranscript`'s own message in `error`. - * That is safe only because every one of those values is now a fixed, - * app-authored literal. Two of them were not, and were removed at the source in - * `transcription-service.ts` rather than masked here: + * That holds only while every value it can return is a fixed, app-authored + * literal — verified as of this change, all seven are. Three were not, and were + * corrected at the source in `transcription-service.ts` rather than masked here: * - the caller-supplied `audioUrl`'s HTTP status, which made this route a - * cross-origin probe, and + * cross-origin probe, * - a message that varied on whether provider API keys were configured, which - * disclosed server configuration. - * Both are logged server-side instead. Keep that invariant when adding a - * strategy: anything interpolated into a `fetchTranscript` error reaches the - * client verbatim through the branches below. + * disclosed server configuration, and + * - the SSRF guard's rejection reason, which named which guard rule fired and + * so leaked hostname-blocklist policy. + * All three are logged server-side instead. + * + * Keep that invariant when adding a strategy: anything interpolated into a + * `fetchTranscript` error reaches the client verbatim through the branches + * below. `${}` in an error value is the thing to look for. + * + * What this deliberately does NOT claim: the *choice among* those literals is + * still informative. `Rejected audioUrl` versus `Could not retrieve the audio + * file` tells a caller whether their host cleared the SSRF guard — i.e. whether + * it resolves to a public address. That residue is inherent to a guard that + * refuses some inputs and attempts others, it is far coarser than naming the + * rule, and this route is session-gated (`apps/web/src/lib/auth-paths.ts`), so + * it is accepted rather than papered over. * - 400: Invalid input (missing URL, malformed JSON) — `input_required` / `invalid_json` * - 429: Rate limited (implement backoff or upgrade service plan) — `rate_limited` * - 500: Service unavailable (check API keys and billing) — `billing_not_configured` diff --git a/apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts b/apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts index a1e170b08..b961c56f8 100644 --- a/apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts +++ b/apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts @@ -2,9 +2,10 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; // `fetchTranscript`'s error string is returned to the caller verbatim by // `/api/transcribe`'s `!result.success` branches. That is only safe while every -// value it can carry is a fixed, app-authored literal. These tests pin the two -// values that were not: the caller-supplied `audioUrl`'s HTTP status, and a -// message that varied on whether provider API keys were configured. +// value it can carry is a fixed, app-authored literal. These tests pin the three +// values that were not: the caller-supplied `audioUrl`'s HTTP status, a message +// that varied on whether provider API keys were configured, and the SSRF guard's +// rejection reason, which named which guard rule fired. vi.mock('server-only', () => ({})); vi.mock('@/lib/ssrf-guard', () => ({ @@ -25,9 +26,21 @@ vi.mock('@/lib/vercel-ai-gateway', () => ({ })); import { fetchTranscript } from '@/lib/transcription-service'; +import { assertPublicHttpUrl } from '@/lib/ssrf-guard'; const AUDIO_URL = 'https://cdn.example.com/clip.mp3'; +// The real messages `assertPublicHttpUrl` throws, as of ssrf-guard.ts. Each +// names a different guard rule; the point of the test below is that the caller +// cannot tell them apart. +const GUARD_REJECTIONS = [ + 'Invalid URL', + 'Blocked URL scheme: file:', + 'Blocked host', + 'Blocked private IP literal', + 'Host does not resolve to a public address', +]; + let consoleError: ReturnType; const savedEnv = { ...process.env }; @@ -79,6 +92,32 @@ describe('fetchTranscript error disclosure', () => { expect(new Set(errors).size).toBe(1); }); + it('does not reveal which SSRF guard rule rejected the audioUrl', async () => { + process.env.OPENAI_API_KEY = 'sk-test-key'; + const errors: string[] = []; + + for (const message of GUARD_REJECTIONS) { + vi.mocked(assertPublicHttpUrl).mockRejectedValueOnce(new Error(message)); + const result = await fetchTranscript({ audioUrl: AUDIO_URL }); + errors.push(result.error ?? ''); + } + + // A blocklist match and a DNS rejection must be indistinguishable: the first + // confirms a hostname is on the server's blocklist, the second only that DNS + // returned nothing public. Distinguishing them is a policy oracle. + expect(new Set(errors).size).toBe(1); + expect(errors[0]).toBe('Rejected audioUrl'); + // No guard rule name survives into the caller-visible payload. + for (const message of GUARD_REJECTIONS) { + expect(errors[0]).not.toContain(message); + } + // Operators still get the real reason. + expect(consoleError).toHaveBeenCalledWith( + '[transcription] audioUrl rejected by SSRF guard:', + expect.objectContaining({ message: 'Blocked host' }), + ); + }); + it('does not disclose whether provider API keys are configured', async () => { vi.stubGlobal( 'fetch', diff --git a/apps/web/src/lib/transcription-service.ts b/apps/web/src/lib/transcription-service.ts index fed35d6bc..9d8adea39 100644 --- a/apps/web/src/lib/transcription-service.ts +++ b/apps/web/src/lib/transcription-service.ts @@ -273,9 +273,18 @@ ${metadataContext ? `\nKNOWN METADATA:\n${metadataContext}` : ''}`, try { await assertPublicHttpUrl(audioUrl); } catch (guardErr) { + // The guard throws five distinguishable messages — `Invalid URL`, + // `Blocked URL scheme: `, `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, while the resolution message confirms only + // that DNS gave nothing public. That difference is a policy oracle, and + // it sharpens as BLOCKED_HOSTNAMES grows. One fixed message for every + // rejection; the real reason goes to the log. + console.error('[transcription] audioUrl rejected by SSRF guard:', guardErr); return { success: false, - error: `Rejected audioUrl: ${guardErr instanceof Error ? guardErr.message : 'blocked'}`, + error: 'Rejected audioUrl', transcript: '', }; }