http: parse Connection/Transfer-Encoding/Content-Encoding/Upgrade as token lists - #36777
Conversation
…token lists fetch() compared these header values as whole strings, so a response with `Connection: close, keep-alive` was returned to the keep-alive pool and the next request on it failed with "socket connection was closed unexpectedly". - Connection: any `close` token disables pooling (request and response side) - Transfer-Encoding: `gzip, chunked` is chunked framing; `chunked` not last is rejected - Content-Encoding: `identity` tokens are ignored; stacked codings pass through undecoded - WebSocket client: 101 `Connection` must contain `upgrade`, across multiple headers - HeaderValueIterator now implements Iterator Fixes #31463
WalkthroughChangesHTTP header protocol handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
A response with `Connection: close` and a separate `Connection: keep-alive` line was pooled because the second line re-enabled keep-alive.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/js/web/websocket/websocket-client.test.ts`:
- Around line 632-638: Update the websocket handshake parsing in the socket data
callback to handle a missing `sec-websocket-key` match explicitly: store the
regex result, and when it is absent, call the test promise’s `reject` with the
failure instead of using a non-null assertion or throwing from the callback.
Preserve the existing accept-hash flow when the header is present.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f7b78b7-885c-4081-89b9-b3c99ee1993e
📒 Files selected for processing (8)
src/http/HeaderValueIterator.rssrc/http/lib.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/http_types/Encoding.rssrc/runtime/webcore/fetch.rstest/js/web/fetch/fetch-gzip.test.tstest/js/web/fetch/fetch-keepalive.test.tstest/js/web/websocket/websocket-client.test.ts
| sock.on("data", chunk => { | ||
| buf += chunk.toString("latin1"); | ||
| if (!buf.includes("\r\n\r\n")) return; | ||
| const key = /sec-websocket-key: *([^\r\n]+)/i.exec(buf)![1]; | ||
| const accept = createHash("sha1") | ||
| .update(key + "258EAFA5-E914-47DA-95CA-C5AB0DC85B11") | ||
| .digest("base64"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Route the missing-header case through reject instead of throwing.
The non-null assertion on .exec(buf)![1] throws inside the socket data callback if the regex fails to match. Wire this failure into the test's reject instead of letting it throw inside the event callback.
🛠️ Proposed fix
sock.on("data", chunk => {
buf += chunk.toString("latin1");
if (!buf.includes("\r\n\r\n")) return;
- const key = /sec-websocket-key: *([^\r\n]+)/i.exec(buf)![1];
+ const match = /sec-websocket-key: *([^\r\n]+)/i.exec(buf);
+ if (!match) {
+ reject(new Error("missing Sec-WebSocket-Key header"));
+ return;
+ }
+ const key = match[1];
const accept = createHash("sha1")As per path instructions, "In asynchronous tests, await the actual observable condition and wire every failure event to reject the awaited promise; never throw inside event callbacks."
📝 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.
| sock.on("data", chunk => { | |
| buf += chunk.toString("latin1"); | |
| if (!buf.includes("\r\n\r\n")) return; | |
| const key = /sec-websocket-key: *([^\r\n]+)/i.exec(buf)![1]; | |
| const accept = createHash("sha1") | |
| .update(key + "258EAFA5-E914-47DA-95CA-C5AB0DC85B11") | |
| .digest("base64"); | |
| sock.on("data", chunk => { | |
| buf += chunk.toString("latin1"); | |
| if (!buf.includes("\r\n\r\n")) return; | |
| const match = /sec-websocket-key: *([^\r\n]+)/i.exec(buf); | |
| if (!match) { | |
| reject(new Error("missing Sec-WebSocket-Key header")); | |
| return; | |
| } | |
| const key = match[1]; | |
| const accept = createHash("sha1") | |
| .update(key + "258EAFA5-E914-47DA-95CA-C5AB0DC85B11") | |
| .digest("base64"); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 635-635: Do not use weak hash functions (MD5/SHA1)
Context: createHash("sha1")
Note: [CWE-328] Use of Weak Hash.
(insecure-hash-typescript)
[warning] 635-635: Avoid SHA1 security protocol
Context: createHash("sha1")
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm (SHA-1).
(avoid-crypto-sha1-typescript)
🪛 OpenGrep (1.26.0)
[ERROR] 635-635: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 `@test/js/web/websocket/websocket-client.test.ts` around lines 632 - 638,
Update the websocket handshake parsing in the socket data callback to handle a
missing `sec-websocket-key` match explicitly: store the regex result, and when
it is absent, call the test promise’s `reject` with the failure instead of using
a non-null assertion or throwing from the callback. Preserve the existing
accept-hash flow when the header is present.
Source: Path instructions
There was a problem hiding this comment.
Thanks for addressing the multi-field-line Connection: close stickiness (050e7ee) — both the response and request arms now only ever clear, and the new close\r\nConnection: keep-alive test row covers it. No further findings; deferring to a human since this rewrites parsing for five list-valued headers in the core HTTP client path.
What was reviewed:
connection_header_keep_alive/upgrade_header_is_not_h2token semantics and all four call sites (req/resp/WS/fetch.rs).state.transfer_encodingconsumers — only ever compared toChunked/Identity, so dropping the old Gzip/Deflate/… writes is dead-code removal (and stops tripping thedebug_assertat lib.rs:4467).allow_keepalivedefaults totrue, so removing theSome(true)write is behavior-preserving for HTTP/1.1.- Content-Encoding fallthrough for unknown/
chunked/stacked codings — resolves to identity + header intact, matching the pre-PR pass-through.
Extended reasoning...
Overview
Refactors five list-valued HTTP header parses (Connection request+response, Transfer-Encoding, Content-Encoding, Upgrade, plus WebSocket 101 Connection) to iterate comma-separated tokens via HeaderValueIterator (now impl Iterator) instead of whole-string equality. Adds Encoding::from_token, connection_header_keep_alive, and upgrade_header_is_not_h2 helpers. Since my prior review the author pushed 050e7ee making close sticky across multiple Connection field lines on both request and response sides, with a covering test row.
Security risks
None identified. The change is strictly more permissive on Connection: …, Upgrade for WebSocket 101 (RFC 6455 §4.1 requires contains, not equals) and strictly more conservative on keep-alive pooling (close now wins). Transfer-Encoding rejects unknown codings and chunked-not-last, which is the fail-closed direction. Content-Encoding stacked/unknown codings pass through raw with the header intact rather than partially decoding — same exposure as before for unknown codings, and safer than half-decoding a stack.
Level of scrutiny
Moderate-to-high: this is the response-header parse loop of the production HTTP client that every fetch() traverses. The individual changes are small and RFC-cited, but there are several independent semantic shifts (TE compression codings now no-op instead of being written to a field nobody read; response keep-alive no longer explicitly sets the flag; CE stacking passes through). I traced each one against downstream consumers and found no regression, but the aggregate is enough surface that a maintainer should confirm the design calls (particularly the TE Some(_) => {} arm and the CE stacked-coding pass-through policy).
Other factors
Tests extend existing matrices in the right files and cover the new rows including the fix for my prior nit. The one candidate flagged by finders (un-awaited async toThrow racing server.close()) was verified as a non-issue — Bun's toThrow awaits async callbacks and the assertion is itself awaited inside try before finally closes the server. The state.transfer_encoding change quietly fixes a pre-existing debug_assert!(== Identity) trip for a bare Transfer-Encoding: gzip response.
1 participant
fetch()compared several list-valued headers as whole strings. The user-visible one: a response withConnection: close, keep-alivewas returned to the keep-alive pool, and the next request on that socket failed with "The socket connection was closed unexpectedly" /ECONNRESET.All sites now go through the existing
HeaderValueIterator(which nowimpl Iterator).Connection(fetch req + resp)close/keep-aliveonlyclosetoken disables pooling;closewins overkeep-aliveConnection(WebSocket 101)Upgrade; only first header readupgrade, across multipleConnectionheadersTransfer-Encoding(resp)gzip, chunked→UnsupportedTransferEncodingchunkednot-last or unknown coding rejectedContent-Encoding(resp)identity, gzip/gzip, identitynot decodedidentityignored; stacked codings (gzip, br) pass through raw with header intactUpgrade(fetch req)Tests extend the existing matrices in
fetch-keepalive.test.ts,fetch-gzip.test.ts,websocket-client.test.ts; the new rows fail on current canary.Fixes #31463
Closes #31464
Closes #33727