fix(web): stop leaking upstream and Stripe error details to clients - #1381
Conversation
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>
|
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe API routes now suppress raw Stripe, OpenAI, transcription-provider, and parser details from responses. They return stable error codes and preserve full diagnostics in server logs and Kaizen traces. Tests cover sensitive-token exclusion and operator-only diagnostics. ChangesAPI error response sanitization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Sentry Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped 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
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 |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
There was a problem hiding this comment.
Pull request overview
Prevents upstream OpenAI and Stripe error details from leaking through public API responses.
Changes:
- Replaces raw provider errors with static codes and safe HTTP statuses.
- Retains detailed errors in server-side observability.
- Adds regression tests covering ten disclosure scenarios.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
api/transcribe/route.ts |
Sanitizes parse and pipeline failures. |
api/realtime/session/route.ts |
Sanitizes OpenAI failures and returns 502. |
api/billing/checkout/route.ts |
Hides Stripe checkout errors. |
api/billing/activate/route.ts |
Hides Stripe activation errors. |
api/billing/renew/route.ts |
Hides Stripe renewal errors. |
api/billing/webhook/route.ts |
Hides webhook verification and handler errors. |
__tests__/transcribe-route.test.ts |
Tests transcription error sanitization. |
__tests__/realtime-session-route.test.ts |
Tests OpenAI error sanitization. |
__tests__/billing-checkout-route.test.ts |
Tests checkout error sanitization. |
__tests__/billing-activate-route.test.ts |
Tests activation error sanitization. |
__tests__/billing-renew-route.test.ts |
Tests renewal error sanitization. |
__tests__/billing-webhook-route.test.ts |
Tests webhook error sanitization. |
groupthinking
left a comment
groupthinking
left a comment
There was a problem hiding this comment.
Review — LGTM, no blocking issues
Reviewed at head 1adbed6. This is a clean, well-scoped fix for the two information-disclosure paths, and the test coverage is genuinely good.
What's right:
- Every patched route now logs the raw upstream/SDK text server-side (
console.error+kaizenObserve) and returns a static{ error, code }body. Verified no leaked token can reach the client on any of the six routes. realtime/sessioncorrectly stops mirroringupstream.status(fixed502). That's the subtle-but-correct part — a mirrored401/429is itself an oracle for the server's OpenAI account state, so collapsing to502closes that side channel too.- Tests assert both directions per route: leaked tokens (
sk_live,sk-proj,org-…,acct_…,10.0.3.14,cus_live_…) are absent from the response and the raw text still reachesconsole.error/kaizenObserve. That's the right shape for a redaction test — it can't silently regress into dropping the operator-side signal. activategainingconsole.error+kaizenObserve(it previously had zero observability on its catch) is a nice incidental improvement.formatApiErrorimport removal intranscribeis safe — it's still exported and consumed bysrc/app/api/route.ts.
Two non-blocking notes (both already called out in the PR description, flagging for the record):
- Dropping
detailsfrom/api/realtime/sessionmakes the client'sgetFriendlyRealtimeErrorinsufficient_quota/invalid_offerbranches (keyed offdetails.error.code) dead code for this path — end users now see the generic message and operators read quota state from logs. Reasonable trade for closing the leak; worth a follow-up to prune the now-dead client branches so they don't rot. - The fixed
502changes behavior for any caller that special-cased the upstream429for backoff. Intentional and documented — noting only so it isn't a surprise later.
CI: mergeable_state: unstable is solely the known repo-wide agent-completion/truth-gate red (pre-existing on main and every PR; the fix for it lives in #1377). The billing-chat-gating timeout is pre-existing on the base commit and unrelated to this diff. Everything this PR touches is green.
No changes requested. Leaving the merge/approval decision to a human maintainer — this is a protected-branch, security-sensitive change and I'm posting as an automated reviewer.
Generated by Claude Code
groupthinking
added
documentation
|
@coderabbitai review Auto-review was skipped on this PR ("Review skipped: excluded by label configuration") because the required labels ( Requesting a review specifically because this is an unreviewed security change: it removes third-party error text (OpenAI upstream bodies, Stripe Generated by Claude Code |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/app/api/__tests__/billing-activate-route.test.ts`:
- Around line 25-31: Declare an explicit NextRequest return type on the
activateRequest helper in
apps/web/src/app/api/__tests__/billing-activate-route.test.ts:25-31 and the
webhookRequest helper in
apps/web/src/app/api/__tests__/billing-webhook-route.test.ts:33-39, preserving
their existing request construction behavior.
- Around line 35-67: Complete the machine-readable error-code contract across
the billing API tests and route handlers: update billing-activate-route.test.ts,
billing-checkout-route.test.ts, billing-renew-route.test.ts, and
billing-webhook-route.test.ts to assert the missing error cases include both
error and code, then align the corresponding route handlers in
billing/activate/route.ts, billing/checkout/route.ts, billing/renew/route.ts,
and billing/webhook/route.ts so every error response returns a stable code
matching its error condition. In billingActivate and the related POST handlers,
cover invalid_json plus the specific branch errors noted in the review
(session_id_required, not_eligible, Turnstile rejection, missing-secret,
missing-signature) and keep the existing response shapes otherwise unchanged.
In `@apps/web/src/app/api/__tests__/transcribe-route.test.ts`:
- Around line 14-20: Update the transcribeRequest helper’s signature to
explicitly declare its return type as Request, leaving its request construction
and behavior unchanged.
- Around line 1-2: Add the required copilot-rabbit label to the pull request and
obtain an explicit GitHub Copilot approval, ensuring Copilot’s review status is
APPROVED rather than COMMENTED before merging.
In `@apps/web/src/app/api/realtime/session/route.ts`:
- Around line 57-60: Update the missing-key error branches near the route
handlers and the invalid-SDP branch to include stable `code` values alongside
`error`, matching the response contract established by `upstreamFailure`. Add
regression tests covering each branch and assert that every response contains
both the expected error message and code.
- Around line 57-60: Update the GET and POST upstream fetch calls to catch
rejected requests before accessing response status or body; log each rejection
server-side and return the existing fixed 502 error shape with the same static
message and code via upstreamFailure or equivalent handling. Add tests covering
rejected GET and POST upstream requests using mockRejectedValue.
- Around line 57-60: Update the upstreamFailure function declaration to
explicitly return Response, preserving its existing Response.json error response
and behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5f4afb63-b23e-420d-8a8b-54e48c396747
📒 Files selected for processing (12)
apps/web/src/app/api/__tests__/billing-activate-route.test.tsapps/web/src/app/api/__tests__/billing-checkout-route.test.tsapps/web/src/app/api/__tests__/billing-renew-route.test.tsapps/web/src/app/api/__tests__/billing-webhook-route.test.tsapps/web/src/app/api/__tests__/realtime-session-route.test.tsapps/web/src/app/api/__tests__/transcribe-route.test.tsapps/web/src/app/api/billing/activate/route.tsapps/web/src/app/api/billing/checkout/route.tsapps/web/src/app/api/billing/renew/route.tsapps/web/src/app/api/billing/webhook/route.tsapps/web/src/app/api/realtime/session/route.tsapps/web/src/app/api/transcribe/route.ts
📜 Review details
⚠️ CI failures not shown inline (1)
GitHub Check: PR Governance: Canonical delivery contract blocked
Conclusion: failure
## Canonical issue is missing or still contains only template placeholders; ## Outcome is missing or still contains only template placeholders; ## Risk is missing or still contains only template placeholders; ## Production evidence is missing or still contains only template placeholders; exactly one closing reference is required: Closes #<issue>
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{py,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,js,jsx,ts,tsx}: Use Python 3.9+ and Node 18+ for development
Never hardcode API keys, database URLs, or secrets in code
Make minimal, surgical changes and avoid deleting working code unless fixing security issues
Files:
apps/web/src/app/api/billing/activate/route.tsapps/web/src/app/api/billing/checkout/route.tsapps/web/src/app/api/__tests__/transcribe-route.test.tsapps/web/src/app/api/__tests__/billing-webhook-route.test.tsapps/web/src/app/api/__tests__/billing-checkout-route.test.tsapps/web/src/app/api/__tests__/billing-activate-route.test.tsapps/web/src/app/api/billing/renew/route.tsapps/web/src/app/api/billing/webhook/route.tsapps/web/src/app/api/__tests__/billing-renew-route.test.tsapps/web/src/app/api/transcribe/route.tsapps/web/src/app/api/__tests__/realtime-session-route.test.tsapps/web/src/app/api/realtime/session/route.ts
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain >80% code coverage for new features
Files:
apps/web/src/app/api/billing/activate/route.tsapps/web/src/app/api/billing/checkout/route.tsapps/web/src/app/api/__tests__/transcribe-route.test.tsapps/web/src/app/api/__tests__/billing-webhook-route.test.tsapps/web/src/app/api/__tests__/billing-checkout-route.test.tsapps/web/src/app/api/__tests__/billing-activate-route.test.tsapps/web/src/app/api/billing/renew/route.tsapps/web/src/app/api/billing/webhook/route.tsapps/web/src/app/api/__tests__/billing-renew-route.test.tsapps/web/src/app/api/transcribe/route.tsapps/web/src/app/api/__tests__/realtime-session-route.test.tsapps/web/src/app/api/realtime/session/route.ts
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,ts,tsx}: Keep frontend and backend data models synchronized using matching Pydantic (backend) and TypeScript (frontend) interfaces
Use type-safe interfaces for backend-frontend data exchange
**/*.{py,ts,tsx}: Production code must use real behavior only: no mock delays, fake data, or simulated responses.
Maintain strict type safety: mypy strict mode for Python and TypeScript strict mode for the frontend.
Name events using the<domain>.<entity>.<action>format.
Files:
apps/web/src/app/api/billing/activate/route.tsapps/web/src/app/api/billing/checkout/route.tsapps/web/src/app/api/__tests__/transcribe-route.test.tsapps/web/src/app/api/__tests__/billing-webhook-route.test.tsapps/web/src/app/api/__tests__/billing-checkout-route.test.tsapps/web/src/app/api/__tests__/billing-activate-route.test.tsapps/web/src/app/api/billing/renew/route.tsapps/web/src/app/api/billing/webhook/route.tsapps/web/src/app/api/__tests__/billing-renew-route.test.tsapps/web/src/app/api/transcribe/route.tsapps/web/src/app/api/__tests__/realtime-session-route.test.tsapps/web/src/app/api/realtime/session/route.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: TypeScript runs in strict mode: do not use implicitanyor unsafe assertions.
Allfetch()calls must useAbortSignal.timeout().
Do not use a barecatch; errors must be logged when caught.Use strict TypeScript mode.
Files:
apps/web/src/app/api/billing/activate/route.tsapps/web/src/app/api/billing/checkout/route.tsapps/web/src/app/api/__tests__/transcribe-route.test.tsapps/web/src/app/api/__tests__/billing-webhook-route.test.tsapps/web/src/app/api/__tests__/billing-checkout-route.test.tsapps/web/src/app/api/__tests__/billing-activate-route.test.tsapps/web/src/app/api/billing/renew/route.tsapps/web/src/app/api/billing/webhook/route.tsapps/web/src/app/api/__tests__/billing-renew-route.test.tsapps/web/src/app/api/transcribe/route.tsapps/web/src/app/api/__tests__/realtime-session-route.test.tsapps/web/src/app/api/realtime/session/route.ts
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Strictly verify that GitHub Copilot has explicitly reviewed and approved the pull request; human approvals alone must not satisfy this check.
Before allowing a merge, require thecopilot-rabbitlabel and AI-generated unit tests committed alongside the code changes; fail the check if either is missing.For Vercel-specific work, include
https://vercel.com/docs/llms-full.txtin the AI assistant context set.
Files:
apps/web/src/app/api/billing/activate/route.tsapps/web/src/app/api/billing/checkout/route.tsapps/web/src/app/api/__tests__/transcribe-route.test.tsapps/web/src/app/api/__tests__/billing-webhook-route.test.tsapps/web/src/app/api/__tests__/billing-checkout-route.test.tsapps/web/src/app/api/__tests__/billing-activate-route.test.tsapps/web/src/app/api/billing/renew/route.tsapps/web/src/app/api/billing/webhook/route.tsapps/web/src/app/api/__tests__/billing-renew-route.test.tsapps/web/src/app/api/transcribe/route.tsapps/web/src/app/api/__tests__/realtime-session-route.test.tsapps/web/src/app/api/realtime/session/route.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use strict TypeScript mode.
Use ESLint with the shared Next.js configuration.
Use Tailwind CSS for styling.
Use the@/*path alias for imports mapped tosrc/*.
Do not usedangerouslySetInnerHTMLin React.
apps/web/**/*.{ts,tsx}: Use the Next.js ESLint rules and the shared configuration frompackages/eslint-config/.
Use the@/*path alias for imports mapped toapps/web/src/*.
Never usedangerouslySetInnerHTML.
Files:
apps/web/src/app/api/billing/activate/route.tsapps/web/src/app/api/billing/checkout/route.tsapps/web/src/app/api/__tests__/transcribe-route.test.tsapps/web/src/app/api/__tests__/billing-webhook-route.test.tsapps/web/src/app/api/__tests__/billing-checkout-route.test.tsapps/web/src/app/api/__tests__/billing-activate-route.test.tsapps/web/src/app/api/billing/renew/route.tsapps/web/src/app/api/billing/webhook/route.tsapps/web/src/app/api/__tests__/billing-renew-route.test.tsapps/web/src/app/api/transcribe/route.tsapps/web/src/app/api/__tests__/realtime-session-route.test.tsapps/web/src/app/api/realtime/session/route.ts
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not commit secrets; store keys and credentials in gitignored
.envfiles.
Files:
apps/web/src/app/api/billing/activate/route.tsapps/web/src/app/api/billing/checkout/route.tsapps/web/src/app/api/__tests__/transcribe-route.test.tsapps/web/src/app/api/__tests__/billing-webhook-route.test.tsapps/web/src/app/api/__tests__/billing-checkout-route.test.tsapps/web/src/app/api/__tests__/billing-activate-route.test.tsapps/web/src/app/api/billing/renew/route.tsapps/web/src/app/api/billing/webhook/route.tsapps/web/src/app/api/__tests__/billing-renew-route.test.tsapps/web/src/app/api/transcribe/route.tsapps/web/src/app/api/__tests__/realtime-session-route.test.tsapps/web/src/app/api/realtime/session/route.ts
**/*.{py,pyi,ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
**/*.{py,pyi,ts,tsx}: Preserve the single workflow: YouTube link → transcript → events → agents → outputs; do not introduce alternative flows or manual triggers that bypass it.
Use event names following<domain>.<entity>.<action>, such asyoutube.video.captured.
Make surgical, precise changes and do not delete working code without justification.
Files:
apps/web/src/app/api/billing/activate/route.tsapps/web/src/app/api/billing/checkout/route.tsapps/web/src/app/api/__tests__/transcribe-route.test.tsapps/web/src/app/api/__tests__/billing-webhook-route.test.tsapps/web/src/app/api/__tests__/billing-checkout-route.test.tsapps/web/src/app/api/__tests__/billing-activate-route.test.tsapps/web/src/app/api/billing/renew/route.tsapps/web/src/app/api/billing/webhook/route.tsapps/web/src/app/api/__tests__/billing-renew-route.test.tsapps/web/src/app/api/transcribe/route.tsapps/web/src/app/api/__tests__/realtime-session-route.test.tsapps/web/src/app/api/realtime/session/route.ts
**/*.ts
⚙️ CodeRabbit configuration file
This is a TypeScript/Next.js project. Focus on type safety, null checks, async/await error handling, and SSE stream lifecycle management. Flag any fetch() calls without AbortSignal.timeout. Check for proper error boundaries. Flag any TODO, placeholder, or stub implementations that are not production-ready. Enforce TypeScript strict mode compliance — flag implicit any, missing return types, and unsafe type assertions.
Files:
apps/web/src/app/api/billing/activate/route.tsapps/web/src/app/api/billing/checkout/route.tsapps/web/src/app/api/__tests__/transcribe-route.test.tsapps/web/src/app/api/__tests__/billing-webhook-route.test.tsapps/web/src/app/api/__tests__/billing-checkout-route.test.tsapps/web/src/app/api/__tests__/billing-activate-route.test.tsapps/web/src/app/api/billing/renew/route.tsapps/web/src/app/api/billing/webhook/route.tsapps/web/src/app/api/__tests__/billing-renew-route.test.tsapps/web/src/app/api/transcribe/route.tsapps/web/src/app/api/__tests__/realtime-session-route.test.tsapps/web/src/app/api/realtime/session/route.ts
apps/web/src/app/api/**
⚙️ CodeRabbit configuration file
Next.js API routes including SSE streaming endpoints. Critical path — check for stream lifecycle issues, controller.close() always being called, and fire-and-forget patterns for non-blocking post-processing. This was the source of a production 95% hang bug (issue
#139).
Files:
apps/web/src/app/api/billing/activate/route.tsapps/web/src/app/api/billing/checkout/route.tsapps/web/src/app/api/__tests__/transcribe-route.test.tsapps/web/src/app/api/__tests__/billing-webhook-route.test.tsapps/web/src/app/api/__tests__/billing-checkout-route.test.tsapps/web/src/app/api/__tests__/billing-activate-route.test.tsapps/web/src/app/api/billing/renew/route.tsapps/web/src/app/api/billing/webhook/route.tsapps/web/src/app/api/__tests__/billing-renew-route.test.tsapps/web/src/app/api/transcribe/route.tsapps/web/src/app/api/__tests__/realtime-session-route.test.tsapps/web/src/app/api/realtime/session/route.ts
🔍 Remote MCP GitHub Copilot, Linear
Additional review context
-
The PR is
#1381(groupthinking/EventRelay), open withmergeable_state: unstable, head1adbed61; it changes 12 files (+430/−26). -
Blocking issue:
transcribe/route.tsstill returnsresult.errordirectly in its!result.successbranches (400, 429, and 503), and returns adetailsfield on the billing branch. The new tests cover thrown exceptions and malformed JSON, but not these returned-failure paths. This violates the stated “no upstream text” criterion. -
An existing review correctly flags that the webhook comment’s claim that an API-key fragment could help forge a signature is inaccurate: Stripe API keys and webhook signing secrets are distinct. The sanitization remains valid; only the rationale should be corrected.
-
Several existing error responses still lack the newly required
codefield, including realtime missing-key/invalid-SDP responses and webhook missing-secret/missing-signature responses. Confirm whether the acceptance criterion applies to all route errors or only modified upstream-failure branches. -
The reported billing-chat test failure is independently documented as pre-existing: an ambient AI Gateway key causes an unmocked live request and a 5-second timeout.
🔇 Additional comments (8)
apps/web/src/app/api/billing/webhook/route.ts (2)
27-29: Correct the signature-forging rationale.Stripe API keys do not authenticate webhook signatures.
STRIPE_WEBHOOK_SECRETauthenticates webhook signatures. Keep the response sanitized, but remove the claim that an API-key fragment could help forge a payload.Source: MCP tools
30-35: LGTM!Also applies to: 59-66
apps/web/src/app/api/billing/activate/route.ts (1)
93-100: LGTM!apps/web/src/app/api/billing/checkout/route.ts (1)
37-46: LGTM!apps/web/src/app/api/billing/renew/route.ts (1)
38-45: LGTM!apps/web/src/app/api/transcribe/route.ts (2)
3-3: LGTM!Also applies to: 31-36, 106-115
17-18: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winInformation Disclosure (CWE-209): Generation of Error Message Containing Sensitive Information
Reachability: External
Reachability path
● Entry apps/web/src/app/api/__tests__/transcribe-route.test.ts │ ▼ ● Sink apps/web/src/app/api/transcribe/route.tsSanitize and test resolved transcription failures.
fetchTranscriptcan resolve withsuccess: falseand provider diagnostic text inerror. The route serializes that text in multiple HTTP responses. The outercatchdoes not protect this path.
apps/web/src/app/api/transcribe/route.ts#L17-L18: logresult.error, replace every resolved-failure response with a fixed message andcode, and removedetails.apps/web/src/app/api/__tests__/transcribe-route.test.ts#L22-L70: mock resolved failures for all status branches and assert provider tokens never appear in responses.apps/web/src/app/api/__tests__/transcribe-route.test.ts (1)
3-13: LGTM!
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
CodeRabbit triage — head
|
| # | Finding | Verdict | In scope? |
|---|---|---|---|
| A | Rejected fetch() bypasses upstreamFailure |
Confirmed — real | Yes |
| B | Missing-key / invalid-SDP branches return no code |
Confirmed | Yes |
| C | upstreamFailure has no return type |
Confirmed | Yes (trivial) |
| D | Test helpers have no return type | Confirmed | Yes (trivial) |
| E | Add code to all pre-existing billing branches |
Accurate but out of scope | No |
| F | "Required AI merge gate" / copilot-rabbit label |
Unverified — likely wrong | No |
A — the one that matters (realtime/session/route.ts)
GET:96 and POST:152 both do a bare await fetch(...). Neither handler has an outer try/catch. A rejected fetch — DNS failure, connection reset, TLS error, timeout — throws before upstream.ok is ever read, so upstreamFailure never runs and the contracted 502 {error, code} is never produced.
To be precise about severity, since CodeRabbit's framing implies a leak: in production Next.js does not serialize an uncaught handler throw to the client, so this is not an information-disclosure hole — the PR's CWE-209 objective still holds. It is a robustness and contract defect:
- the caller gets an unstructured 500 instead of the documented 502
{error, code}; use-realtime-voice.ts::getFriendlyRealtimeErrorparses the JSON body, so it degrades badly on a non-JSON 500;- in dev, the stack is surfaced;
- undici's rejection
causecarries the target host.
- const upstream = await fetch(OPENAI_REALTIME_CLIENT_SECRETS_URL, { … });
+ let upstream: Response;
+ try {
+ upstream = await fetch(OPENAI_REALTIME_CLIENT_SECRETS_URL, { … });
+ } catch (err) {
+ return upstreamFailure(
+ 'client_secret_transport', 0, String(err),
+ 'OpenAI Realtime client secret creation failed.',
+ 'realtime_client_secret_failed',
+ );
+ }Same shape for POST:152 with sdp_exchange_transport / realtime_session_failed. Worth two mockRejectedValue tests — the existing +10 only cover !ok, never a rejection, which is exactly why this slipped through.
B, C, D — quick wins, same file
{ error: 'OPENAI_API_KEY is not configured on the server.' } at GET:90 and POST:142, and { error: 'A valid WebRTC SDP offer is required.' } at POST:133, carry no code — inconsistent with the contract this PR introduces in this very file. Suggest server_misconfigured and invalid_sdp_offer. C is function upstreamFailure(...): Response. D is : NextRequest / : Request on the three test helpers.
E — accurate, but decline
CodeRabbit is factually right that activate/route.ts:27,31,52 (invalid_json, session_id_required, not_eligible) and the equivalents in checkout/renew/webhook return error without code. But those branches are pre-existing and untouched by this diff — they never leaked third-party text, which is what this PR is scoped to. Adding codes plus tests across four routes turns a surgical CWE-209 fix into a broad refactor. Recommend a follow-up issue.
F — does not check out
CodeRabbit asserts a required merge gate needing the copilot-rabbit label and an explicit Copilot approval. I could not substantiate it: there is no such gate in .coderabbit.yaml, mergeable_state is unstable (not blocked), and agent-completion/truth-gate passes. Its own evidence is also off — it reports this PR's labels as javascript, security, tests when they are javascript, tests. Treating as a false positive; don't add a label on its say-so.
One correction to my own earlier note in passing: I previously attributed the unstable state to a repo-wide truth-gate red. That gate is green here — unstable is from non-required checks only.
Status
Not posting @coderabbitai resolve — findings A–D are genuinely unaddressed, and resolving would dismiss them. Note .coderabbit.yaml sets request_changes_workflow: true, so @coderabbitai approve would register a real GitHub approval; deliberately not doing that with A open.
Recommendation: hold the merge until A is fixed. B–D are cheap enough to fold into the same commit. Still not auto-merging — protected main, no automerge label, and I can't push to this PR's branch from the remediation runner.
Generated by Claude Code
|
Tip For best results, initiate chat on the files or code changes.
Finding A is a release blocker. A rejected B through D are appropriate to include in the same change. They keep the route response contract consistent and make the helper and test interfaces explicit. I agree that E is out of scope for this CWE-209 remediation. Those pre-existing validation responses do not expose third-party error details. A separate follow-up issue is the correct place to standardize error codes across the billing routes. I also retract F. The available evidence does not support a required Hold merge until A is fixed and covered by tests. 🐇 You are interacting with an AI system. |
|
@coderabbitai review New head
Still worth a full pass on the substance: this removes third-party error text from six API routes, five of which are on the unauthenticated public allowlist. Verified at Generated by Claude Code |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff summary ==='
git diff --stat "$(git merge-base HEAD origin/main)" HEAD || true
git diff --name-status "$(git merge-base HEAD origin/main)" HEAD || true
echo '=== Route outlines ==='
ast-grep outline apps/web/src/app/api/transcribe/route.ts --items all
ast-grep outline apps/web/src/app/api/realtime/session/route.ts --items all
echo '=== Transcribe result-failure and response paths ==='
rg -n -C 8 --type ts \
'result\.(success|error)|NextResponse\.json|fetchTranscript|catch\s*\(' \
apps/web/src/app/api/transcribe/route.ts
echo '=== Realtime fetch and error paths ==='
rg -n -C 10 --type ts \
'await\s+fetch\s*\(|upstreamFailure|try\s*\{|catch\s*\(|NextResponse\.json' \
apps/web/src/app/api/realtime/session/route.ts
echo '=== Relevant test coverage ==='
rg -n -C 5 --type ts \
'mockRejectedValue|mockResolvedValue|fetchTranscript|realtime_client_secret_failed|realtime_session_failed|result\.error' \
apps/web/src/app/api/__tests__/transcribe-route.test.ts \
apps/web/src/app/api/__tests__/realtime-session-route.test.tsLength of output: 19783 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Locate transcription service ==='
fd -a 'transcription-service' .
echo '=== Service outline ==='
ast-grep outline apps/web/src/lib/transcription-service.ts --items all 2>/dev/null || true
echo '=== Error-result construction and provider error propagation ==='
rg -n -C 8 --type ts \
'fetchTranscript|error\s*:|message|\.text\(\)|\.json\(\)|throw\s+new\s+Error|return\s+\{' \
apps/web/src/lib/transcription-service.ts 2>/dev/null || true
echo '=== All imports and uses of fetchTranscript ==='
rg -n -C 3 --type ts \
"import\s+\{?\s*fetchTranscript|fetchTranscript\s*\(" \
apps/web/srcLength of output: 14671 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Locate SSRF guard ==='
fd -a 'ssrf-guard' apps/web/src
echo '=== SSRF guard outline ==='
ast-grep outline apps/web/src/lib/ssrf-guard.ts --items all 2>/dev/null || true
echo '=== Thrown errors and URL/DNS handling ==='
rg -n -C 8 --type ts \
'throw\s+new\s+Error|Error\s*\(|assertPublicHttpUrl|dns|lookup|hostname|address|url' \
apps/web/src/lib/ssrf-guard.ts 2>/dev/null || trueLength of output: 3294
This failure returns an unstructured Next.js 500 response instead of the required Wrap each I verified the ✏️ Learnings added
|
Verified CodeRabbit's blocking finding — it does not holdI requested the CodeRabbit review that produced the The claim:
The second sentence is false, and it is the one the finding rests on.
Every string that can reach
The single interpolated exception message is So the docblock this PR added at lines 19–21 is accurate as written, and the What CodeRabbit got rightIts non-blocking points are fair and worth a follow-up, though none block a merge:
StateHead is now Terminal state: One outstanding decision before that: whether CodeRabbit's Generated by Claude Code |
…able 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
Confirmed and fixed in Fix. Both handlers wrap the fetch in One extension beyond the report: Tests — the two you asked for, plus the body-read case. Each verified non-vacuous by stashing the route fix and re-running: Scope note for the record: this gap predates the PR — the original code had no Verified at Also confirming your A follow-up incremental review is rate-limited for ~49 min; I'll re-request it once the window resets rather than leave this head unreviewed. Generated by Claude Code |
🔍 PR Validation |
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
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
fix(web): stop /api/transcribe leaking probe status and key config Stacked onto #1381 as its own description directs. Removes three caller- and policy-derived values from transcription-service.ts error branches (upstream probe status, provider-key configuration state, and which SSRF guard rule fired) plus the residual `details` field on the billing branch.
Scope update — #1440 merged into this branchHead is now Recording it here rather than editing the description, because gate 5 (provenance) compares the declared scope to the actual diff and the description above predates the fold-in. Read the two together. What the merge addedDiff grew from 16 files / +1103−89 to 18 files / +1300−96. The three new/changed paths:
This closes the gap that the earlier docblock thread on this PR identified but scoped out: the Unchanged by this merge
Generated by Claude Code |
|
#1440 merged into this branch. Head is now
What #1440 addedIts starting point was CodeRabbit's
All three fixed at the source in Four regression tests, each failing without the fix. Two assert on the count of distinct messages ( The third item was found by CodeRabbit reviewing #1440 — my own first pass replaced your overstated invariant with another overstated one. The docblock now carries an explicit "what this deliberately does NOT claim" paragraph instead, because the choice among the literals still tells a caller whether their host cleared the guard. Coarse, inherent to the guard's design, and this route is session-gated — accepted and written down rather than glossed. Two things to check before merging
Generated by Claude Code |
|
Correcting myself, and it affects how this PR's evidence should be read. CI on But I checked what the green No CI job runs Filed as #1449 with the full job-by-job breakdown and a suggested What this means for this PR specifically: your body cites That is not an argument against merging. The tests exist, they're well-shaped — asserting both that the tokens are absent and that the raw text still reaches Merging this is still strictly better than not — five unauthenticated billing routes stop leaking Stripe text either way. #1449 is what makes it stay fixed. Generated by Claude Code |
CI gate satisfied + red-team pass on
|
| Route | Line | Pattern |
|---|---|---|
agents/dispatch/route.ts |
86 | details: String(error) |
agents/status/route.ts |
39 | details: String(error) |
training/trigger/route.ts |
235, 252 | details: String(uploadError) / String(error) |
training/status/route.ts |
42 | details: String(error) |
video/search/route.ts |
56 | error.message || 'Internal server error…' — returned directly |
All six pre-exist on origin/main and are untouched by this PR — confirmed by diffing each file against origin/main. This is a scope-boundary note, not a regression, and not a reason to hold the merge.
Why they are worth their own issue rather than a shrug
None are on PUBLIC_API_EXACT, so the natural read is "authenticated-only, low severity." That read depends on a gate that fails open:
// apps/web/src/proxy.ts:37-38
const AUTH_SECRET = process.env.NEXTAUTH_SECRET;
const AUTH_ENABLED = !!AUTH_SECRET;
// :229
if (AUTH_ENABLED && request.method !== 'OPTIONS' && needsAuthentication(pathname)) {When NEXTAUTH_SECRET is unset, needsAuthentication is never consulted and every non-public route is served unauthenticated. In that configuration these six become exactly the class of unauthenticated information disclosure this PR's own training/trigger and video/search in particular surface storage and vector-backend errors.
#1229 ("fail closed when NEXTAUTH_SECRET is missing in production") is the fix for that fail-open, and it is currently a draft. The two together are what determines whether the table above is LOW or MEDIUM; separately, each looks benign.
Suggested disposition: merge this PR, then file one follow-up issue for the six routes, referencing #898 and #1229. Happy to open it if that's wanted.
Generated by Claude Code
…o-review (#1425) `reviews.auto_review.labels` is now set explicitly to `[]` — the schema default — to override a required-labels list inherited from the dashboard / org config. The inherited gate demanded at least one of ~26 labels before a review would start. Four of those (`architecture-gap`, `ci-cd`, `pipeline-critical`, `placeholder-code`) are labels CodeRabbit applies itself, as part of a review, via `auto_apply_labels: true`. A PR opens unlabelled, so the gate skipped it, and nothing then applied a label to un-skip it. Labelling after the fact does not retro-trigger. Observed on both currently-open non-draft PRs: the `CodeRabbit` commit status reads "Review skipped: excluded by label configuration" on #1381 (head 8628b5e) and #1410 (head 3f72b75). Every review this repo has had was requested by hand with `@coderabbitai full review`. The file's header claimed to be the single source of truth for settings that used to live only in the dashboard, but this gate was not in it — and with `inheritance: true`, anything left unset can be overridden from there. Header now says so, and recommends setting values explicitly even at their default. No other setting changed; `drafts: false` still keeps draft PRs out. Claude-Session: https://claude.ai/code/session_016mDBbUAmEHFXrYboYd9TzR 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>
* fix(web): range-check bracketed IPv6 literals in the SSRF guard 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 * fix(web): block IPv6 transition prefixes that encode a private IPv4 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 * test(web): pin the DNS branch and a tail-position private answer 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 --------- Co-authored-by: Claude <noreply@anthropic.com>
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
Canonical issue
Closes #1383
Sibling to #912 under the #898 CWE-209 governance program. #912 owns the Python surface (
src/agents/**,src/youtube_extension/**,src/uvai/ml); this PR owns the disjoint Next.js/TypeScript surface (apps/web/src/app/api/**). No file overlap.Outcome
Six API routes no longer return third-party error text to the caller. Every suppressed message is preserved server-side via
console.error(andkaizenObservewhere the route already used it), so operator diagnostics are unchanged.realtime/session—details: bodyremoved from both GET and POST; a newupstreamFailure()helper logs status + body and returns a fixed 502 instead of mirroringupstream.status.billing/checkout·activate·renew·webhook(×2 branches) — raw Stripeerr.messagereplaced with a static string plus a machine-readablecode.activatepreviously had no observability on its catch and gained it.transcribe— same raw-upstream pattern removed from the parse branch and the catch-all.Client contract is unchanged: existing
data.error ?? '<snake_case_fallback>'consumers receive the exact strings they already fall back to.Risk
Low. No success path, status code, or non-error HTTP semantic is modified outside the deliberate
realtime/session401/429 → 502 normalization.realtime/sessionno longer mirrorsupstream.status. This is the fix, not a side effect — the upstream status is itself a signal about the server's OpenAI account state (401 = bad key, 429 = out of quota).use-realtime-voice.ts::getFriendlyRealtimeErrorhasinsufficient_quota/invalid_offerbranches keyed offdetails.error.code. Withdetailsgone these are unreachable for this path and it falls back toparsed.error, rendering the static message. Accepted trade-off; operators see quota errors in server logs.ProCheckoutButton.tsx:98,ProRenewPanel.tsx:38,CheckoutSuccessActivator.tsx:24,action-agent-store.ts:124,dashboard-store.ts:319— none readdetails.Production evidence
Verified against base
a631194facb9.Baseline on clean
a631194facb9before any edit was 251 passed / 1 failed. The single failure —src/app/api/__tests__/billing-chat-gating.test.ts:35"blocks free tier after daily quota", 5000 ms timeout — is pre-existing and unrelated; it fails identically on the untouched base commit and is tracked in #1116. Net: +10 passing tests, zero regressions.Each of the +10 tests asserts the response body contains none of the specific leaked tokens (
sk-proj-****ABCD,sk_live_****ABCD,org-eventrelay-prod,proj_abc123,price_1QLiveSecret,acct_1QLive,cus_live_secret,10.0.3.14:6379) and that the raw text still reachesconsole.error/kaizenObserve— i.e. they prove suppression without proving silence.CI on head
1adbed61:build,test,guards,trivy,bandit,CodeQL,Security Scan - javascript,Security Scan - python,gitleaks (working tree),dependency-review,Vercel,agent-completion/truth-gate,Agent completion enforcementall pass.Patches two live information-disclosure vulnerabilities on
main(basea631194facb9). Both returned third-party error text verbatim to the caller. No open PR covered them.All five billing routes below are on the public allowlist
PUBLIC_API_EXACTinapps/web/src/lib/auth-paths.ts(lines 13–35), so any internet user could trigger these error branches and read the leaked Stripe text without credentials:/api/billing/checkout/api/billing/activate/api/billing/renew/api/billing/webhook/api/billing/statusis on the same allowlist but did not leak)Vuln 1 — MEDIUM — raw OpenAI upstream body returned to client
apps/web/src/app/api/realtime/session/route.tsBoth handlers did
const body = await upstream.text()and, on!upstream.ok, returned that text verbatim asdetails: bodywhile mirroringupstream.status. OpenAI error bodies echo org/project IDs, quota state, and on 401 a partial key — e.g.Incorrect API key provided: sk-proj-****ABCD. There was no guard or redaction.+49–61upstreamFailure(stage, status, body, error, code)helper.console.errors[realtime] <stage> failedwith the full status + body, returnsResponse.json({ error, code }, { status: 502 }).112–117(was98–105)upstreamFailure('client_secret', …, 'OpenAI Realtime client secret creation failed.', 'realtime_client_secret_failed')161–166(was147–154)upstreamFailure('sdp_exchange', …, 'OpenAI Realtime session creation failed.', 'realtime_session_failed')detailsis gone from both.upstream.statusis no longer mirrored — a fixed502is returned, because the upstream status is itself a signal about the server's OpenAI account state (401 = bad key, 429 = out of quota).Vuln 2 — MEDIUM — raw Stripe SDK
err.messageon unauthenticated routesEach returned raw
err.message, leakingNo such price: 'price_...',Invalid API Key provided: sk_live_****ABCD, and signature-verification internals (tolerance window, scheme). Each nowconsole.errors the raw message, records it viakaizenObserve, and returns a static string + machine-readable code.apps/web/src/app/api/billing/checkout/route.ts37–46(was36–38)err.message→{ error: 'checkout_failed', code: 'checkout_failed' }, 500apps/web/src/app/api/billing/activate/route.ts93–100(was92–93)err.message→{ error: 'activation_failed', code: 'activation_failed' }, 500apps/web/src/app/api/billing/renew/route.ts38–45(was37–39)err.message→{ error: 'renewal_failed', code: 'renewal_failed' }, 500apps/web/src/app/api/billing/webhook/route.ts27–35(was26–28){ error: 'invalid_payload', code: 'invalid_payload' }, 400apps/web/src/app/api/billing/webhook/route.ts59–66(was51–53){ error: 'webhook_handler_failed', code: 'webhook_handler_failed' }, 500activatepreviously had no observability at all on its catch — it gainedconsole.error+kaizenObserve('billing', 'activate_error', …)alongside the fix.Also fixed —
apps/web/src/app/api/transcribe/route.tsSame raw-upstream pattern.
3formatApiErrorfrom the import (still exported and used bysrc/app/api/route.ts)17–1831–39(was34–35)console.errorthe parse error; return{ success: false, error: 'Invalid JSON in request body', code: 'invalid_json', transcript: '' }, 400.detailsremoved.106–118(was112)console.error; returns{ success: false, error: 'Transcription failed', code: 'transcription_failed', transcript: '' }, 500.details/formatted.messageremoved.Conventions followed (not reinvented)
error(invalid_json,not_eligible,missing_signature,webhook_not_configured) and logged viakaizenObserve('billing', '<stage>', msg, { fix }). That is preserved; an explicitconsole.errorwas added for the raw text.errorkeeps the existing snake_case value and a duplicatecodefield is added alongside. Same value in both is deliberate — it satisfies the requirement with zero client-contract breakage.realtime/session/route.tshas no logger, so plainconsole.erroris used there.Client compatibility (audited, no behavior change)
ProCheckoutButton.tsx:98,ProRenewPanel.tsx:38,CheckoutSuccessActivator.tsx:24already dodata.error ?? '<same_snake_case_fallback>'— the exact strings now returned. UI is unchanged.use-realtime-voice.ts::getFriendlyRealtimeErrorfalls back toparsed.errorwhendetailsis absent, so it renders the static message. Itsinsufficient_quota/invalid_offerbranches keyed offdetails.error.codeare now dead for this path — an intentional trade-off; operators see quota errors in server logs.detailsfrom/api/transcribe(action-agent-store.ts:124,dashboard-store.ts:319read onlyerror/success/transcript).Tests
+10 tests. Each asserts the response body contains none of the leaked tokens, and that the raw text still reaches
console.error/kaizenObserve.realtime-session-route.test.ts(+68) — GET and POST fed a 401/429 body containingsk-proj-****ABCD,org-eventrelay-prod,proj_abc123; asserts 502, static payload, no leaked tokens.billing-checkout-route.test.ts(+31) — Stripe rejects withNo such price: 'price_1QLiveSecret'; Invalid API Key provided: sk_live_****ABCD.billing-renew-route.test.ts(+27) —Invalid API Key provided: sk_live_****ABCD (account acct_1QLive).billing-activate-route.test.ts(new, 67) — leakygetCheckoutSessionrejection + malformed-body case.billing-webhook-route.test.ts(new, 92) — signature-verification internals (tolerance/scheme/key) and handler internals (Redis connection refused at 10.0.3.14:6379 for customer cus_live_secret).transcribe-route.test.ts(new, 70) — leaky upstream throw + malformed-body case.Verification
Baseline on
a631194facb9before any edits was 251 passed / 1 failed. The single failure —src/app/api/__tests__/billing-chat-gating.test.ts:35"blocks free tier after daily quota", 5000 ms timeout — is pre-existing and unrelated to this change; it fails identically on the clean base commit. Net effect: +10 passing tests, zero regressions.Out of scope
.github/workflows/pr-checks.yml,scripts/ci/agent_completion_gate.py, and.github/agent-lock/trusted-publishers.jsonare untouched — handled separately. An unrelatedpackage-lock.jsondrift introduced bynpm install(tailwindcss ^4.3.2→^4.3.3) was reverted to keep this PR surgical.