Skip to content

fix(streaming): sanitize generic stream failure boundaries - #12457

Merged
diegosouzapw merged 5 commits into
release/v3.8.51from
fix/v3851-streamhandler-public-errors
Sep 3, 2026
Merged

diegosouzapw merged 5 commits into
release/v3.8.51from
fix/v3851-streamhandler-public-errors

Conversation

@diegosouzapw

@diegosouzapw diegosouzapw commented Sep 2, 2026 •

Copy link
Copy Markdown
Owner

Summary

The generic stream finalizer passed raw upstream Error.message text to public SSE frames and to the stream diagnostic log. A provider error containing credentials, filesystem paths, or a stack tail could therefore escape after the HTTP response had already started.

Area Problem Correction Proof Compatibility / risk
OpenAI Chat SSE Raw upstream messages were emitted in chat.completion.chunk.error.message Project the message through buildErrorBody() before encoding Hostile token, API key, POSIX path, and stack assertions in the isolated six-case child fixture Existing finish_reason: "error", status-derived type/code, and [DONE] are unchanged
Responses SSE Raw messages were emitted in response.failed Reuse the same public projection Exact response.failed event/type/code assertions Event name and response shape are unchanged
Claude SSE Raw messages were emitted in event: error Reuse the same public projection Exact error + message_stop assertions Claude error type and terminal message_stop are unchanged
Internal failure state Public hardening could accidentally erase provider diagnostics used by fallback/cooldown persistence Keep the original Error, raw message, and status on onError; sanitize only at public/log boundaries Callback identity/raw-message assertions Provider classification behavior is unchanged
Diagnostic logging logStream("error: ...") logged the raw provider message Log the same sanitized projection used by the wire Captured-log regression excludes token, key, path, and stack Normal non-sensitive first-line diagnostics remain visible
Client disconnects Disconnect races must not become provider failures No control-flow change; explicit regression locks this behavior Disconnect callback plus disconnect/deadline regressions No cooldown/fallback change for client aborts
Test isolation The original focused test mutated DATA_DIR and plugin state at module scope under --test-isolation=none Keep the parent test as a Node-only wrapper and run all repository imports, DB state, logging, drains, and cleanup inside a child fixture with a minimal environment allowlist Deterministic RED state probe, child 6/6, wrapper 1/1, real wrapper + call-log-drain pair 3/3 Production streamHandler.ts stayed byte-identical during the repair

One intentional security behavior change: multiline public diagnostics now retain only the first sanitized line. This removes stack tails while preserving status, protocol framing, and the raw internal failure object.

Reconciliation summary

Item Result
Live release tip used e243b04de22da2d78900c27fde5f83f4fbc3d7f9
Candidate SHA 866f4df88bf7bf85f2b0669150104ed1e9d7429f
Candidate tree e786e061e9f9ddcb42bdd2de7db58d8bfb73eb05
Merge method Normal merge of origin/release/v3.8.51; no rebase or force-push
Owned-path conflicts None
Semantic overlap review PASS: the release did not change streamHandler.ts or either isolated boundary test, and did not contain a central stream/public-error helper on which this PR could accidentally depend
Production preservation PASS: open-sse/utils/streamHandler.ts retained git blob 7abafbf080f92a8107962da544409fb3fd31526e across reconciliation

Validation

Receipt Result
Original TDD RED on base 7802f6ea163f18f349348dbfd8710a159ac32e15 RED 0/1 — hostile token/path appeared in the Chat SSE wire and diagnostic log
Test-isolation contamination probe on the previously published test RED 5/6 — the inherited DATA_DIR was replaced by the boundary test's module-scope temp directory
Process-isolated child fixture PASS 6/6 — includes exact DB/data/plugin path assertions and the five public-boundary cases
Reconciled stream-handler regression matrix PASS 35/35, fail 0, on candidate 866f4df88b
Pure parent wrapper PASS 1/1 within the 35-test matrix
Real test:unit:fast-shape pair: wrapper + call-log-save-drain.test.ts, absolute paths, --test-isolation=none, no concurrency override PASS 3/3, fail 0
Complementary state probe after isolation repair PASS 2/2 — inherited env, DB paths, directory state, and call-log drain remained intact
npm run typecheck:core PASS
Focused ESLint PASS across production code and all three owned test files
Prettier + git diff --check PASS across all five owned paths
npm run check:changelog-integrity PASS — no release-base bullets lost against origin/release/v3.8.51
npm run check:secrets PASS, secretFindings=0
GitHub CI HOLD — awaiting checks on reconciled candidate 866f4df88bf7bf85f2b0669150104ed1e9d7429f
Live provider validation HOLD — intentionally not run; no provider/network credential is required for the deterministic security paths

The child receives only a minimal runtime environment allowlist plus synthetic test configuration; it does not inherit credentials, HOME, or CODEX_HOME. It asserts call-log drain completion, closes the artifact writer and shared logger, resets the DB, and removes its process-owned data only after those resources are closed. No external provider request or real credential was used.

Independence

This PR is standalone and does not depend on any other open PR. It intentionally leaves merge, deployment, and release decisions to the maintainer.

@diegosouzapw
diegosouzapw marked this pull request as ready for review September 3, 2026 23:58
@diegosouzapw
diegosouzapw merged commit 9469fa5 into release/v3.8.51 Sep 3, 2026
21 checks passed
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…apw#12457)

Validado em lote numa worktree combinada com os 14 PRs desta campanha de error-boundary sobre o tip de `release/v3.8.51`: `typecheck:core` limpo e **120/120** nos 23 arquivos de teste que os PRs trazem.

Um ponto que só apareceu no tree combinado: **diegosouzapw#12465 e diegosouzapw#12466 criam o mesmo arquivo novo** `open-sse/utils/streamReadiness.ts` (que não existe no tip) com desenhos divergentes de cancelamento — `cancelled` + `releaseLock` imediato num, `readInFlight`/`cancelRequested` com `cancelReader` fire-and-forget no outro. Adotei a versão do diegosouzapw#12466, que difere e defere o release do lock para quando a leitura em voo termina, e validei a escolha rodando as suítes dos **dois** PRs contra ela: 21/21 no readiness compartilhado e 22/22 incluindo o boundary do Perplexity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant