Repository navigation
fix: close stealth video error leaks - #3390
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughVideo gateway requests now redact stealth-provider HTTP and application errors. Worker persistence applies the same rule to video job failures. Provider identifiers flow through video creation and upload paths, with gateway and integration test coverage. ChangesVideo error redaction
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant VideoClient
participant Gateway
participant UpstreamProvider
participant Worker
VideoClient->>Gateway: create video or upload media
Gateway->>UpstreamProvider: send provider request
UpstreamProvider-->>Gateway: HTTP or application error
Gateway-->>VideoClient: formatted error message
Worker->>UpstreamProvider: poll video job
UpstreamProvider-->>Worker: failed job error
Worker-->>VideoClient: persisted sanitized error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/gateway/src/videos/videos.ts (1)
2791-2791: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake
providerIdrequired at this boundary.All current video call sites pass
providerContext.providerId, butproviderId?: stringallows a future call site to omit it and silently bypass stealth-provider redaction. If video requests always have a provider identifier, make the parameter required. Otherwise, document and test the intentional undefined-provider path.Suggested type change
- providerId?: string, + providerId: string,🤖 Prompt for 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. In `@apps/gateway/src/videos/videos.ts` at line 2791, Make the providerId parameter required in the affected video request boundary instead of optional, using the surrounding function signature as the change point. Preserve the existing providerContext.providerId call-site behavior and ensure the TypeScript signature no longer permits undefined values.
🤖 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.
Nitpick comments:
In `@apps/gateway/src/videos/videos.ts`:
- Line 2791: Make the providerId parameter required in the affected video
request boundary instead of optional, using the surrounding function signature
as the change point. Preserve the existing providerContext.providerId call-site
behavior and ensure the TypeScript signature no longer permits undefined values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d69e3e8d-cdc4-4094-8b44-edc3857c690f
📒 Files selected for processing (3)
apps/gateway/src/videos/upstream-error.spec.tsapps/gateway/src/videos/upstream-error.tsapps/gateway/src/videos/videos.ts
ReviewVerified the leak and the fix end-to-end against
1. The async status path still leaks (biggest gap)Video is asynchronous, so the most likely place a user sees a provider failure isn't
Network-level failures leak here too: an 2.
|
pacocartones
changed the title
|
Thanks for the thorough review — addressed in e76a8d5.
Validation: pre-commit hooks and targeted lint passed; |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/worker/src/services/video-jobs.ts`:
- Around line 582-593: Sanitize all persisted stealth-provider error data, not
only videoJob.error: update the upstreamStatusResponse writes in the relevant
job-update paths and the llmgateway_last_poll_error retry persistence to remove
provider-controlled message, URL, and provider identifier fields. Reuse
clientFacingVideoJobError or an equivalent centralized sanitizer, and add
assertions that persisted records contain none of those upstream values while
preserving unsanitized errors for non-stealth providers.
🪄 Autofix (Beta)
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 UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f829f70-9f9a-455b-9408-7974bde64d50
📒 Files selected for processing (7)
apps/gateway/src/lib/stealth-provider-errors.spec.tsapps/gateway/src/lib/stealth-provider-errors.tsapps/gateway/src/stealth-error-redaction.spec.tsapps/gateway/src/test-utils/mock-openai-server.tsapps/gateway/src/videos/videos.spec.tsapps/gateway/src/videos/videos.tsapps/worker/src/services/video-jobs.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/gateway/src/videos/videos.ts
| function clientFacingVideoJobError( | ||
| providerId: string, | ||
| error: VideoJobRecord["error"], | ||
| ): VideoJobRecord["error"] { | ||
| if (!error || !isStealthProvider(providerId as ProviderId)) { | ||
| return error; | ||
| } | ||
|
|
||
| return { | ||
| message: "Upstream provider error", | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Sanitize all persisted stealth-provider error fields.
clientFacingVideoJobError only sanitizes videoJob.error. The same updates persist the raw upstream payload in upstreamStatusResponse at lines 2509-2513 and 2596. The retry path also persists the raw polling exception in llmgateway_last_poll_error at lines 2614-2618.
For stealth providers, remove or sanitize provider-controlled error fields before every upstreamStatusResponse write. Add assertions that the persisted record does not contain the upstream message, URL, or provider identifier.
Also applies to: 2481-2484, 2588-2591
🤖 Prompt for 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.
In `@apps/worker/src/services/video-jobs.ts` around lines 582 - 593, Sanitize all
persisted stealth-provider error data, not only videoJob.error: update the
upstreamStatusResponse writes in the relevant job-update paths and the
llmgateway_last_poll_error retry persistence to remove provider-controlled
message, URL, and provider identifier fields. Reuse clientFacingVideoJobError or
an equivalent centralized sanitizer, and add assertions that persisted records
contain none of those upstream values while preserving unsanitized errors for
non-stealth providers.
Re-review at
|
|
Thanks — excellent re-review. I addressed all three points in
CI for the final commit is running now: https://github.com/theopenco/llmgateway/actions/runs/30819873638 |
|
Small test-only follow-up in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/worker/src/services/video-jobs.ts`:
- Around line 1845-1850: Update the rawVideoError assignment near extractError
so it falls back to jobToLog.error when the upstreamStatusResponse is not an
object or extractError returns no error. Preserve the extracted upstream error
whenever it exists, ensuring finalized logs retain available error details.
🪄 Autofix (Beta)
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 UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d2b78055-57de-4e1f-9ceb-a83bafbd07a4
📒 Files selected for processing (2)
apps/gateway/src/videos/videos.spec.tsapps/worker/src/services/video-jobs.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/gateway/src/videos/videos.spec.ts
| const rawVideoError = | ||
| jobToLog.upstreamStatusResponse && | ||
| typeof jobToLog.upstreamStatusResponse === "object" && | ||
| !Array.isArray(jobToLog.upstreamStatusResponse) | ||
| ? extractError(jobToLog.upstreamStatusResponse as Record<string, unknown>) | ||
| : jobToLog.error; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fall back to jobToLog.error when the upstream payload has no error.
Lines 1845-1850 select null when upstreamStatusResponse is an object without an error field. This discards a non-null jobToLog.error. The finalized log then has no public or internal error details.
Use the extracted upstream error only when it exists. Otherwise, use jobToLog.error.
Proposed fix
- const rawVideoError =
+ const upstreamVideoError =
jobToLog.upstreamStatusResponse &&
typeof jobToLog.upstreamStatusResponse === "object" &&
!Array.isArray(jobToLog.upstreamStatusResponse)
? extractError(jobToLog.upstreamStatusResponse as Record<string, unknown>)
- : jobToLog.error;
+ : null;
+ const rawVideoError = upstreamVideoError ?? jobToLog.error;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rawVideoError = | |
| jobToLog.upstreamStatusResponse && | |
| typeof jobToLog.upstreamStatusResponse === "object" && | |
| !Array.isArray(jobToLog.upstreamStatusResponse) | |
| ? extractError(jobToLog.upstreamStatusResponse as Record<string, unknown>) | |
| : jobToLog.error; | |
| const upstreamVideoError = | |
| jobToLog.upstreamStatusResponse && | |
| typeof jobToLog.upstreamStatusResponse === "object" && | |
| !Array.isArray(jobToLog.upstreamStatusResponse) | |
| ? extractError(jobToLog.upstreamStatusResponse as Record<string, unknown>) | |
| : null; | |
| const rawVideoError = upstreamVideoError ?? jobToLog.error; |
🤖 Prompt for 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.
In `@apps/worker/src/services/video-jobs.ts` around lines 1845 - 1850, Update the
rawVideoError assignment near extractError so it falls back to jobToLog.error
when the upstreamStatusResponse is not an object or extractError returns no
error. Preserve the extracted upstream error whenever it exists, ensuring
finalized logs retain available error details.
Re-review at
|
Run pnpm format over the changed files and hoist the redacted error text into a single constant, with a note on why the public status code is a hard-coded 502. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
The video pipeline forwards upstream provider error bodies to the client verbatim, skipping the stealth redaction every other pipeline already applies.
fetchUpstreamJson(apps/gateway/src/videos/videos.ts:2788) throwsHTTPException(status, { message: body.error.message })on!response.ok— with the full raw text when the error isn't JSON — and re-forwardsbody.msguntouched on the application-error branch;app.ts'sonErrorthen renders that message to the client viarenderGatewayError. Unlikechat,rerankandembeddings,videos.tsnever callsshouldRedactProviderError.For stealth providers this leaks exactly what #3340 and the
stealth-provider-errors.tsinvariant exist to protect: provider identity, hostnames and vendor markings inside the raw error body (e.g.quota exceeded at https://<secret-host>fromavalanche, whose base URL is a deployment secret).Fix
Mirror the sibling pattern (
rerank.ts:1060,embeddings.ts:1347):videos/upstream-error.ts:clientFacingUpstreamMessage(providerId, statusCode, rawMessage)—redactedProviderErrorText(statusCode)whenshouldRedactProviderError(providerId), the raw message unchanged otherwise. Same comment as the sibling sites.fetchUpstreamJsontakes an optionalproviderIdand routes both throw branches through the helper (theUpstream provider error (<status>)fallback was already generic and stays as-is). Internallogger.warnkeeps the full body, consistent with the rest of the codebase.providerContext.providerId(each already has it in scope). Non-stealth providers are byte-identical to before.Tests
New pure spec
videos/upstream-error.spec.ts(DB-light, no harness):avalanche): the raw message never reaches the client — the response is exactlyredactedProviderErrorText(500)and contains neither the secret host nor the original message;openai): raw message passes through unchanged;undefinedprovider: raw message passes through unchanged.Adversarial cycle run: commenting the guard makes the stealth test fail (
expected 'quota exceeded at https://plataforma-…' to be 'Upstream provider error (500…)'); restoring it passes again.Verification
normalize-streaming-error.spec.ts13/13 ✅ (the stealth sibling suites stay green)turbo run build --filter=gateway✅ (10/10 tasks — full typecheck ofvideos.ts+ helper + spec)Not verified: end-to-end against a real stealth provider (no DB/Docker on this machine) — the
fetchUpstreamJson→HTTPException→renderGatewayErrorpath is verified by reading, and the redaction decision itself by the new unit tests.Disclosure: an AI coding assistant helped survey the pipeline and draft the test scaffolding. The diff is small and was verified the slow way: I read every call site, confirmed
videos.tsis the only pipeline without the guard, and ran the revert-the-guard check above to prove the test actually catches the leak. I own the change and will follow up on review comments.Summary by CodeRabbit
Bug Fixes
Tests
Follow-up: async video-job redaction
This update also redacts stealth-provider failures before the worker persists the public job error, so the asynchronous status endpoint cannot expose upstream response text or network details.
It adds a full
POST /v1/videosleak-regression case and a worker-to-status regression case, moves the HTTP helper into the common stealth-error module, and requires provider context at every video upstream request.