Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesWebSocket handshake validation
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change consistently rejects oversized WebSocket handshake headers while preserving pipelined frame handling, with no substantiated merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:01 AM PT - Sep 6th, 2026
❌ @robobun, your commit 8901702 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 41569That installs a local version of the PR into your bun-41569 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the completed-parse arm now enforces the same max_http_header_size() cap the ShortRead arm already had, using head_len so pipelined frame bytes aren't counted.
What was reviewed:
- Confirmed the new guard compares
head_len(response.bytes_read, header bytes only) with the same>operator and helper as the existingShortReadcheck, and that arm ordering makes it take precedence overOk(done). - Checked the neighboring "large frame pipelined after split 101 header" test still holds: its head is small and only the trailing frame is >16KB, so
head_lenstays under the cap and that case still opens. - New test mirrors the adjacent cap-rejection test's shape (
usingserver,port: 0,Buffer.alloc,Promise.withResolvers, all failure events wired to reject) and the 16000/21000 split keeps the first read under the cap so the oldShortReadcheck never fires.
Extended reasoning...
Overview
The PR adds one match-arm guard to buffer_and_parse_head in src/http_jsc/websocket_client/WebSocketUpgradeClient.rs so that a fully-parsed 101 response head whose head_len exceeds bun_http::max_http_header_size() is treated as HeadParse::Invalid. Previously the cap was only enforced in the ShortRead arm, so a large head that completed in a single accumulated parse (or whose intermediate buffered size never crossed the cap before the terminator arrived) was accepted. A new test in test/js/web/websocket/websocket-client-short-read.test.ts sends a ~21KB 101 head split so the first write is 16000 bytes (< 16384), and asserts the client fires onerror with "Invalid response" instead of opening.
Security risks
This is a hardening fix: it tightens an existing resource bound so it applies regardless of TCP read segmentation. The change only makes the client reject more inputs, never accept more, so there is no new attack surface. The comparison uses head_len (header bytes only) rather than full.len(), which correctly avoids counting pipelined WebSocket frame bytes against the header cap — verified against the adjacent test that pipelines a 20KB frame after a small header and must still open.
Level of scrutiny
Low-to-moderate. The Rust change is 3 effective lines that mirror the existing ShortRead arm's check byte-for-byte (same helper, same > operator). Match-arm ordering is correct (the guarded Ok arm precedes the catch-all Ok(done)). The changed paths are not covered by CODEOWNERS. The bug hunt ran to dry_streak with no findings and no ruled-out candidates.
Other factors
The new test is a near-exact copy of the neighboring "incomplete header larger than the cap is still rejected" test: using for the listener, port: 0, Buffer.alloc(n, fill) for padding, Promise.withResolvers, and every unexpected event (open, message, clean close) wired to reject. The setTimeout(.., 50) between writes matches the pattern used by both adjacent tests in this file for forcing a read boundary on loopback. On the unfixed build the first 16000-byte read passes the ShortRead cap check and the second read completes the head with no size check, so the connection opens and the test rejects with "unexpected open" — it fails for the right reason.
0b1d17b to
2ea0a8c
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the comment-narration nit from the last round is addressed, and the guard itself is a straight mirror of the existing ShortRead cap check.
What was reviewed:
buffer_and_parse_head: newDonearm compareshead_len(picohttpbytes_read, header bytes only) againstmax_http_header_size(), so pipelined frame bytes infullaren't counted — consistent with theShortReadarm's rationale comment.- Generic
impl<SSL>covers both TLS and plain sockets, so no sibling site needs the same guard. - New test matches the file's existing split-write
setTimeoutpattern (lines 55/158/277) and wiresonopen/onmessage/cleanoncloseto reject.
Extended reasoning...
Overview
The PR adds a single match-arm guard in WebSocketUpgradeClient::buffer_and_parse_head so that a fully-parsed 101 response head whose head_len exceeds bun_http::max_http_header_size() is treated as HeadParse::Invalid. Previously the cap was only enforced in the ShortRead arm, so a head that completed in one parse call — even if accumulated across multiple reads each individually under the cap — bypassed the limit. The accompanying test sends a ~21 KB head split at 16000 bytes and asserts the client fires onerror with "Invalid response".
Security risks
This is a resource-limit hardening change: it closes a gap where a peer could push an oversized handshake head past the buffering cap by segmenting writes. The fix compares response.bytes_read (header bytes only), not the full accumulated buffer, so it does not over-reject when pipelined WebSocket frames follow the head in the same read. No new unsafe code, no allocation, no user-input parsing beyond what picohttp already does.
Level of scrutiny
Low-to-moderate. The Rust change is three lines that exactly mirror the sibling ShortRead arm's cap check and return the same HeadParse::Invalid sentinel that the existing malformed-response arm uses, so downstream handling is already exercised. The function is generic over the SSL type parameter, so both TLS and plaintext paths share this code — no parallel site to patch.
Other factors
Since the previous review, commit 8901702 rewrote the test's block comment to describe the invariant (oversized head, first write under the cap) and dropped the "The old code…" diff narration I'd flagged. The new test copies the file's established conventions verbatim: Bun.listen with port: 0, Buffer.alloc(n, fill), the same server-side setTimeout split-write pattern used by three sibling tests in the file, Promise.withResolvers with all failure events (onopen, onmessage, clean onclose) wired to reject, and cleanup in finally. Nothing outstanding from other reviewers in the timeline beyond bot comments the author self-resolved, all of which correspond to the comment-wording commits.
|
Status: the diff is ready for review. Reproduced with the new case in The red lanes in that build are unrelated to this change. They are RSS leak tests and timeouts on the x64-asan and alpine lanes ( |
The scratch was sized from the line count of the response head alone. With a raised --max-http-header-size, a server could make each read of an incomplete head allocate 16 bytes of scratch per buffered byte. Clamp the slot count to 2000, the default of Node's maxHeadersCount, so the scratch is at most 64 KB whatever the header size cap is. A head with more fields fails the parse as before. The clamp makes the head size check before the parse unnecessary, so the ShortRead check is back as it was on main and a head over the size cap that completes in one read is again left to #41569.
|
Closing in favor of #41573. It carries the same change to |
1 participant
Problem
new WebSocket()accepts a 101 handshake head larger than the cap (--max-http-header-size, default 16 KB) whenever norecv()boundary lands inside the incomplete head. A 60 KB head split so the first read stays under the cap opens the connection.ShortReadarm ofbuffer_and_parse_head(src/http_jsc/websocket_client/WebSocketUpgradeClient.rs:717). TheOk(done)arm, which runs when the head parse completes, never checks the size. So the bound depends on read segmentation, not on the head size.Fix
Ok(done)arm too. Comparehead_len(the header size alone), so pipelined WebSocket frames after the head are not counted.ShortReadcheck and Node and undici, which enforce a fixed 16 KB regardless of segmentation.test/js/web/websocket/websocket-client-short-read.test.ts(new case, stock bun opens the connection and fails). Also ran the other 4 cases in that file, including the pipelined large-frame case that must still open.Background
buffer_and_parse_headaccumulates bytes and runspicohttp::Response::parse.parsereturnsShortReadwhile no\r\n\r\nis found, andDoneonce the head is complete.head_len(response.bytes_read) is the head size. Bytes after it are pipelined WebSocket frames.max_http_header_size(default 16 KB) bounds the head so a peer cannot force unbounded buffering.[human-review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file