diff --git a/apps/web/src/app/api/transcribe/route.ts b/apps/web/src/app/api/transcribe/route.ts index ef63574f0..3d375e956 100644 --- a/apps/web/src/app/api/transcribe/route.ts +++ b/apps/web/src/app/api/transcribe/route.ts @@ -19,11 +19,29 @@ 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 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, + * - a message that varied on whether provider API keys were configured, which + * 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` @@ -94,7 +112,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..b961c56f8 --- /dev/null +++ b/apps/web/src/lib/__tests__/transcription-error-disclosure.test.ts @@ -0,0 +1,143 @@ +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 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', () => ({ + 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'; +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 }; + +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 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', + vi.fn(async () => new Response('{}', { status: 500 })), + ); + + delete process.env.OPENAI_API_KEY; + 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/auJzb1D-fag' }); + + 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..9d8adea39 100644 --- a/apps/web/src/lib/transcription-service.ts +++ b/apps/web/src/lib/transcription-service.ts @@ -273,18 +273,35 @@ ${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: '', }; } 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 +373,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: '', }; }