Catch sensitive values split across response chunks - #981
Conversation
The ServerResponse patch scanned each write()/end() chunk on its own, and detection is a substring match against complete values, so a secret split across a chunk boundary passed both scans. Streaming SSR flushes at arbitrary points, so this is the normal case rather than an edge case. Each chunk is now scanned with the tail of the previous one, and trailing text that looks like the start of a sensitive value is held back until the next chunk (flushed on a short timer so a paused stream is not stalled). The same carry-over is applied to the ReadableStream scanner used by the edge/Response paths. Also fixed alongside it: - end() now handles compressed chunks, which were never decompressed or scanned - binary chunks decode with a streaming decoder, so a multi-byte character split across chunks no longer decodes to replacement characters - end() redacts under redactInsteadOfThrow instead of always throwing, and recomputes Content-Length when redaction shortens the body
|
The changes in this PR will be included in the next version bump.
|
📦 Bundle size
dist/ only; native binaries are versioned separately and not counted here. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Compressed Unicode secrets can still bypass the new boundary scan, and split redaction can leave an invalid Content-Length that hangs the client. Both paths need correction before merge.
Reviewed changes across the Node and Web streaming leak scanners, response redaction behavior, focused runtime tests, documentation, and release metadata.
- Cross-chunk scanning: Adds per-response carry and pending state for Node responses, plus carry and streaming UTF-8 decoding for Web
ReadableStreambodies. - Response redaction: Holds possible secret prefixes briefly, redacts final chunks in Next.js development, and updates
Content-Lengthfor rewritten final bodies when possible. - Compressed responses: Accumulates compressed chunks, scans newly decoded output, and adds final compressed-chunk scanning.
- Regression coverage: Adds ASCII split-boundary, gzip, multibyte pass-through, timed-flush, redaction, and response-framing tests.
- User-facing guidance: Documents cross-chunk detection and the 100 ms holdback, with a patch release entry.
azure/gpt-5.6-sol ｜ 𝕏
commit: |
|
Addressed and resolved both review threads in commit Task list (5/5 completed)
|
1 participant
Varlock's response leak scanner checked each
ServerResponse.write()/end()chunk in isolation, and detection is a substring match against complete values. A sensitive value split across a chunk boundary was therefore present in neither scan and went out to the client, even though the same value in a single chunk was blocked. Streaming SSR flushes at arbitrary points, so this is the normal case for the workload the patch was written for, not an edge case.The application controls where chunks break, not a remote caller, so this is a gap in a safety net rather than a new way to reach secrets. It still means the net silently fails open on its main use case.
What changed
write→end,write→write, and the gzip/br/zstd delta path.getRedactionHoldbackLength, already used by CLI output redaction.ReadableStreamscanner inenv.ts, which the edgeResponsepatch and the Cloudflare integration use.Fixed alongside it, in the same code paths:
end()never decompressed compressed chunks, so a secret in a final compressed chunk was not scanned at all.TextDecoder. A multi-byte character split across chunks previously decoded to replacement characters, a second source of missed matches and a corruption risk once chunks get rewritten.end()now redacts underredactInsteadOfThrowinstead of always throwing, matchingwrite(). This is required for the splitwrite→endcase to be redactable, and it replaces the hung request the old code left behind (there was a TODO on that line about it). It affects Next.js dev only; production and every other integration still throw.Content-Lengthis recomputed. Next.js setsContent-Lengthfor non-streamed payloads and sends them in a singleend(), so a stale length left the client waiting on bytes that never arrived.Clean responses stay byte-for-byte identical: the outgoing chunk is only rewritten when something was actually redacted or held back.
Testing
9 new detection tests across the three runtime test files, each confirmed to fail against the unpatched source and pass with the fix. Regression guards cover held-back text delivered intact, multi-byte splits, the timed flush arriving before
end(),Content-Lengthcorrection, and text that merely starts like a secret not being flagged.Full framework test suite run locally: 670 passed across Astro 5/6/7, Next.js 14/15/16 (webpack + turbopack), Vite 5/6/7/8, Cloudflare, TanStack Start, SvelteKit, Expo, and vanilla-node. Two suites hit harness
beforeAlltimeouts under concurrent load and passed on a targeted re-run (200/200).Reported by @7thParkk.