Repository navigation
fix(gateway): redact stealth provider errors on mid-stream read faults - #3348
Merged
steebchen merged 4 commits intoAug 2, 2026
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
WalkthroughThe streaming error normalizer now supports optional redaction. Stealth-provider read errors use this mode. Client SSE responses omit provider details and buffered upstream data while internal diagnostics remain unchanged. ChangesStreaming error redaction
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Gateway
participant UpstreamProvider
participant Client
Gateway->>UpstreamProvider: request streaming response
UpstreamProvider-->>Gateway: truncated SSE data and socket read failure
Gateway->>Gateway: determine provider redaction policy
Gateway-->>Client: sanitized stealth error or raw non-stealth error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
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/gateway/src/chat/tools/normalize-streaming-error.ts`:
- Around line 192-209: Update the NormalizedStreamingError client details type
so details.errorName is optional, allowing the redacted branch to include only
statusCode and statusText while preserving the existing undefined runtime value
asserted by the redaction test.
🪄 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: 9bad9b54-7fbb-4849-963d-93c313f84016
📒 Files selected for processing (3)
apps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/normalize-streaming-error.spec.tsapps/gateway/src/chat/tools/normalize-streaming-error.ts
The mid-stream read-fault error branch wrote the normalized error to the SSE
client verbatim, with no shouldRedactProviderError() check — unlike every
sibling branch (the HTTP-error branch at chat.ts:8335, the in-stream error
event at chat.ts:9254, and the non-streaming normalize-client-error path),
which all redact for stealth providers.
For a stealth provider that leaked its identity three ways in the client
payload built by normalizeStreamingError: the raw message ("Streaming
error: ..."), details.cause (extractErrorCause walks the undici cause chain and
embeds the secret host via ENOTFOUND/ECONNREFUSED/TLS errors), and responseText
(the buffered upstream body, up to 5000 chars of vendor-branded content).
Add an optional redact flag to normalizeStreamingError; when set (chat.ts passes
shouldRedactProviderError(usedProvider) on the upstream_read branch) the
client payload is reduced to a status-only body via redactedProviderErrorText,
dropping message, cause, buffered body, errorName and errorCode — matching
redactErrorDetails on the sibling branches. The internal log payload is left
raw, consistent with the other branches and logs.ts.
Non-stealth providers are unaffected. Adds unit coverage in
normalize-streaming-error.spec.ts (redacted variant scrubs identity + drops the
Node failure identifiers; redact-off path unchanged; internal log stays raw).
pacocartones
force-pushed
the
fix/redact-stealth-errors-midstream
branch
from
August 2, 2026 10:38
4b1c62a to
2aec253
Compare
Contributor
Author
|
Applied the review nit: |
Adds route-level coverage for the leak this PR fixes: the leaky mock now kills the socket after a half-written, vendor-branded SSE event, so the gateway fails inside reader.read() with those raw bytes still in its buffer. Verified to fail without the redact flag. Also adds a non-stealth control (same fault, openai) asserting the buffered body and UND_ERR_SOCKET detail still reach the client, plus unit coverage for the redacted 502 termination path and an explicit redact: false case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
steebchen
enabled auto-merge
August 2, 2026 17:09
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/gateway/src/stealth-error-redaction.spec.ts (1)
306-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert internal diagnostic retention in the route test.
This test verifies public-log redaction. It does not verify that the integration path stores
TRUNCATED_LEAKY_MARKERandUND_ERR_SOCKETininternalErrorDetails. ReuseexpectRedactedLog()and assert both values.Proposed test update
- const log = await waitForLogByRequestId(requestId); - expect(log.hasError).toBe(true); - expectNoLeak(JSON.stringify(log.errorDetails)); + const log = await expectRedactedLog(requestId); + expect(JSON.stringify(log.internalErrorDetails)).toContain( + TRUNCATED_LEAKY_MARKER, + ); + expect(JSON.stringify(log.internalErrorDetails)).toContain("UND_ERR_SOCKET");🤖 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/stealth-error-redaction.spec.ts` around lines 306 - 336, The test around the mid-stream read fault currently checks only public log redaction; update it to reuse expectRedactedLog() and assert that log.internalErrorDetails retains both TRUNCATED_LEAKY_MARKER and UND_ERR_SOCKET, while preserving the existing SSE leak assertions.
🤖 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/stealth-error-redaction.spec.ts`:
- Around line 306-336: The test around the mid-stream read fault currently
checks only public log redaction; update it to reuse expectRedactedLog() and
assert that log.internalErrorDetails retains both TRUNCATED_LEAKY_MARKER and
UND_ERR_SOCKET, while preserving the existing SSE leak assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 31de686d-5711-43a4-bacd-54d9e5fefc12
📒 Files selected for processing (2)
apps/gateway/src/chat/tools/normalize-streaming-error.spec.tsapps/gateway/src/stealth-error-redaction.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The mid-stream read-fault error branch wrote the normalized error to the SSE client verbatim, with no
shouldRedactProviderError()check — unlike every sibling branch, which all redact for stealth providers:chat.ts~8335) →redactedProviderErrorText(...)chat.ts~9254)normalize-client-error.ts)So a network fault after the stream started (socket reset / partial vendor-branded body) leaked a stealth provider's identity three ways through the
clientpayload built innormalize-streaming-error.ts:message—"Streaming error: <raw>"details.cause—extractErrorCause()walks the undici cause chain and embeds the secret host (getaddrinfo ENOTFOUND <host>,connect ECONNREFUSED <host>:443, TLS cert host)responseText—bufferSnapshot, up to 5000 chars of the raw upstream body (vendor-branded)This is exactly the class of leak the recent stealth-provider compliance work guards against.
Change
Add an optional
redactflag tonormalizeStreamingError.chat.tspassesredact: shouldRedactProviderError(usedProvider)on theupstream_readbranch. When set, the client payload becomes a status-only body viaredactedProviderErrorText(statusCode), droppingmessage,cause, the buffered body, and the Node-levelerrorName/errorCode— matchingredactErrorDetailson the sibling branches. The internallogpayload is left raw (feeds the internal-only log columns, consistent with the other branches andlogs.ts).Non-stealth providers are unaffected (the flag is only set when
shouldRedactProviderErroris true).Tests
normalize-streaming-error.spec.ts:details.cause/errorName/errorCodeare dropped, and that only the gateway-derived status survives; the internallogstill carries the raw detail.Scope note: coverage is at the
normalizeStreamingErrorunit boundary (where the client payload is built and emitted verbatim by the route). An end-to-end harness test (stealth-error-redaction.spec.ts) would need the Postgres/Redis test harness; happy to add it if you'd like.AI-assisted; reviewed and verified by the author.
Summary by CodeRabbit
Bug Fixes
Tests