Sync high-value upstream fixes: response queue poisoning (GHSA-35p6-xmwp-9g52), multi-byte setEncoding, null queue guard - #5
Merged
Conversation
…-xmwp-9g52) Port the response-queue-poisoning defense from upstream (6ea54ef, 89323ff, c9fbe9d, ac5394b), adapted to this fork's h1-only client and older parser handler interface. A misbehaving or malicious peer can write an unsolicited/early response onto an idle keep-alive socket. Without validation the next request dispatched on that socket reads the stale bytes as its response, poisoning the response queue. Before binding a pending request to a previously-used idle socket, resumeH1 now proactively reads from it once and opens a one event-loop-tick validation window (native timer, unref'd). busy() reports the socket as busy while validation is pending so no request is dispatched meanwhile. Any bytes that arrive while nothing is inflight are turned into a 'bad response' socket teardown by the new kRunning === 0 guards in onMessageBegin/onHeadersComplete, so the socket is discarded and a fresh one is used. Validation state is reset in writeH1 (request bound) and cleared on socket close. Adds test/response-queue-poisoning.js from upstream as a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ding() Port upstream 74a2299 (nodejs#5003). setEncoding() previously assigned _readableState.encoding directly, so no StringDecoder was installed and each chunk was decoded independently — a multi-byte UTF-8 sequence split across a chunk boundary produced U+FFFD replacement characters on the for-await / on('data') path. Delegate to super.setEncoding() so Node installs a StringDecoder that stitches split sequences correctly. Because that also rewrites already-buffered chunks into decoded strings (which would break Buffer.concat on the consume path), preserve the raw Buffers first under kPreservedBuffer and prefer them in consumeStart() for byte-accurate .text()/.json(). Adds test/readable-setencoding-multibyte.js as a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Port upstream 6df53c5 (nodejs#5410). When failing the queue in kDestroy and onError, completed slots may already be nulled out (and this fork's own pipelined-error handling splices/nulls queue entries too). Skip null entries before calling util.errorRequest so a torn-down slot can't throw while erroring the queue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR syncs three upstream fixes into this fork: a security hardening for HTTP/1.1 keep-alive socket reuse (response queue poisoning / GHSA-35p6-xmwp-9g52), correctness for multi-byte UTF-8 handling in BodyReadable#setEncoding(), and robustness improvements for null queue entries during error teardown.
Changes:
- Add idle-socket validation + “bad response while idle” teardown to prevent response queue poisoning on keep-alive reuse.
- Fix
BodyReadable#setEncoding()to delegate toReadable#setEncoding()(StringDecoder) while preserving raw bytes for.text()/.json()consume paths. - Guard
util.errorRequest()loops against already-nulled queue entries; add regression tests for both security and encoding behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/dispatcher/client-h1.js |
Adds idle-socket validation state machine and inflight guards to prevent response queue poisoning on socket reuse. |
lib/api/readable.js |
Adjusts setEncoding() to use Node’s decoder and preserves raw buffers for byte-accurate consumption APIs. |
lib/dispatcher/client.js |
Skips null/undefined queue slots when erroring requests during destroy/error flows. |
test/response-queue-poisoning.js |
Regression test ensuring unsolicited bytes on an idle keep-alive socket don’t poison the next request’s response. |
test/readable-setencoding-multibyte.js |
Regression test ensuring UTF-8 multibyte sequences split across chunks decode correctly after setEncoding('utf8'). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two issues raised on the idle-socket-validation / setEncoding changes (both also present upstream): - client-h1.js: kIdleSocketValidation could stay stuck at 1/2 if the pending request that triggered validation was aborted before being written, causing the socket to skip the poisoning check on its next reuse. Reset validation state in resumeH1 when the socket goes fully idle (kSize === 0) so it is revalidated on reuse. (GHSA-35p6-xmwp-9g52) - readable.js: consumeStart() is a plain function, so `this` is undefined in strict mode; the state.endEmitted fast-path dereferenced this[kConsume] / this._readableState and would throw if reached. Use the consume argument and captured state instead. (Currently guarded by the isUnusable check in consume(), but corrected defensively.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Cherry-picks three upstream fixes that landed after our last sync and that apply to this fork's scope (h1-only core client; no h2/interceptors/web). Each is ported and adapted to our diverged code, with regression tests.
1. 🔴 Security — response queue poisoning (GHSA-35p6-xmwp-9g52)
Upstream
6ea54ef8,89323ff9,c9fbe9d2,ac5394b8.Our
client-h1.jshad no idle-socket validation, so a misbehaving/malicious peer could write an unsolicited response onto an idle keep-alive socket and poison the next request's response.busy()holds off dispatch while validation is pending.kRunning === 0guards inonMessageBegin/onHeadersCompletetear the socket down ('bad response') if bytes arrive while nothing is inflight.writeH1and cleared on socket close.test/response-queue-poisoning.js(from upstream).2. 🟡 Correctness — multi-byte UTF-8 in
setEncoding()Upstream
74a2299e(nodejs#5003).setEncoding()set_readableState.encodingdirectly, so noStringDecoderwas installed and a multi-byte char split across a chunk boundary decoded toU+FFFD. Now delegates tosuper.setEncoding(), preserving raw Buffers underkPreservedBufferfor byte-accurate.text()/.json().test/readable-setencoding-multibyte.js.3. 🟡 Robustness — null queue entries in
errorRequestloopsUpstream
6df53c50(nodejs#5410).Skip already-nulled queue slots before
util.errorRequestinkDestroy/onError— relevant now that our pipelined-error handling splices/nulls queue entries.Out of scope (not ported)
h2, fetch, websocket, cookies, cache-web, proxy/socks, dns/retry/mock interceptors — none exist in this fork.
e71624eb(nodejs#5356) is already covered differently by our86330728.Testing
eslintclean; 190 tests pass, 0 fail innode:25Docker across the new tests plus the full readable / pipelining / keep-alive / request / client / dispatcher / pool / socket blast radius.🤖 Generated with Claude Code