websocket: only cap header bytes, not pipelined frames, during upgrade

[robobun]

Problem

The hardening pass in #31175 added a max_http_header_size cap (default 16384) to the WebSocket upgrade client's response accumulator at all three entry points (handle_data, handle_proxy_response, handle_decrypted_data). The cap was applied to total bytes received before parsing:

if !me.body.is_empty() {
    if me.body.len().saturating_add(data.len()) > bun_http::max_http_header_size() {
        Self::terminate(this, ErrorCode::InvalidResponse);
        return;
    }
    me.body.extend_from_slice(data);
}

This counts any WebSocket frame bytes the server pipelines after the 101 response in the same TCP segment as the header tail. Failing sequence:

1. First read delivers a partial 101 status line → picohttp ShortRead → buffered to self.body
2. Second read delivers rest-of-header plus a >16KB initial WS frame → body.len() + data.len() > 16384 → terminate(InvalidResponse)

The Zig reference (WebSocketUpgradeClient.zig lines 594-598 / 616-620 / 631-634 / 656-658 / 802-805 / 824-826) has no size cap; it appends and continues, parses the header, and hands the trailing bytes to processResponse as remain_buf.

Reproduction

error: ws error: WebSocket connection to 'ws://127.0.0.1:.../' failed: Invalid response

Raw TCP server that writes "HTTP/1.1 101 ", flushes, then after a short delay writes the header tail plus a 20000-byte binary frame in one packet. 10/10 failures on the released build.

Fix

Move the cap to the ShortRead arm, where picohttp has confirmed no \r\n\r\n exists and therefore every byte in the accumulator is provably header:

Err(picohttp::ParseResponseError::ShortRead) => {
    if me.body.is_empty() {
        me.body.extend_from_slice(data);
    }
    if me.body.len() > bun_http::max_http_header_size() {
        Self::terminate(this, ErrorCode::InvalidResponse);
    }
    return;
}

Applied identically to handle_data, handle_proxy_response, and handle_decrypted_data. This keeps the DoS protection from #31175 (a server that never sends \r\n\r\n is still rejected once the accumulator exceeds 16KB) while matching the Zig reference for well-formed responses that pipeline large frames. Worst-case allocation before rejection is max_http_header_size + one socket read, bounded by the kernel recv buffer.

Tests

Added to test/js/web/websocket/websocket-client-short-read.test.ts:

• large frame pipelined after split 101 header is not counted against the header-size cap: fails with Invalid response on the unfixed build, passes with the fix (verified 5x stable).
• incomplete header larger than the cap is still rejected: confirms the retained DoS guard still fires when 20KB of header bytes arrive with no terminator.

[robobun]

^{Updated 5:13 AM PT - Jun 16th, 2026}

✅ @robobun, your commit ad3dc7d226328f5b6927f0d8826bec63c935ef41 passed in Build #62791! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 32394

That installs a local version of the PR into your bun-32394 executable, so you can run:

bun-32394 --bun

[coderabbitai]

Walkthrough

In WebSocketUpgradeClient.rs, the upfront max-header-size guard before buffering is removed from handle_data, handle_proxy_response, and handle_decrypted_data. Header-size enforcement is moved into the ShortRead parse-error branch, where the buffer contains only incomplete header bytes and not pipelined WebSocket frames. A new test file adds two TCP-level tests covering split-read handshake acceptance and oversized incomplete header rejection.

Changes

WebSocket upgrade ShortRead header-size cap refactor

Layer / File(s)	Summary
Deferred header-size cap in all three data paths src/http_jsc/websocket_client/WebSocketUpgradeClient.rs	Removes the pre-extend max_http_header_size guard in handle_data, handle_proxy_response, and handle_decrypted_data. Each ShortRead branch now extends the buffer when initially empty and checks me.body.len() <= bun_http::max_http_header_size(), with comments clarifying that the limit covers incomplete header bytes only, not pipelined WebSocket frame bytes that arrive in the same read after the header terminator.
Split-read handshake tests test/js/web/websocket/websocket-client-short-read.test.ts	Adds the "WebSocket upgrade split across reads" suite with helpers for Sec-WebSocket-Accept computation and large binary frame construction. One test asserts that a pipelined binary frame (payload > 16 KB) co-arriving with the 101 header completion is accepted and the full payload is delivered; a second test asserts that a response followed by oversized header bytes without \r\n\r\n is rejected with "Invalid response".

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: moving the header-size cap to apply only to header bytes, not pipelined frames during WebSocket upgrade.
Description check	✅ Passed	The description is comprehensive, covering Problem, Reproduction, Fix, and Tests sections. It explains the root cause, provides a concrete failing scenario, details the solution with code examples, and documents added test cases.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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-short-read.test.ts`:
- Around line 158-172: The test uses setTimeout with a fixed delay to schedule
the second socket write, which creates time-dependent, flaky behavior under CI
load. Replace both setTimeout calls (one at lines 158-172 and another at lines
218-223) with a deterministic, condition-based approach such as waiting for an
event emitted by the socket, a promise that resolves when the connection is
ready, or another non-wall-clock trigger that indicates when the second write
should proceed. This ensures the test behaves consistently regardless of system
load and CI environment variations.

🪄 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: 303bf34d-84b3-401e-90a3-0f466ffbdf2d

📥 Commits

Reviewing files that changed from the base of the PR and between 78f0fff and ad3dc7d.

📒 Files selected for processing (2)

• src/http_jsc/websocket_client/WebSocketUpgradeClient.rs
• test/js/web/websocket/websocket-client-short-read.test.ts

[github-actions]

Found 1 issue this PR may fix:

1. @azure/service-bus WebSocket "Proxy connection failed" regression in Bun 1.3.11 (works in 1.3.5) #28599 - Azure Service Bus uses AMQP-over-WebSocket which sends a large AMQP preamble frame pipelined immediately after the 101 upgrade response, exactly the scenario where the header-size cap was incorrectly rejecting valid connections

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #28599

🤖 Generated with Claude Code

[claude]

LGTM — targeted fix that moves the header-size cap to the ShortRead arm where bytes are provably header, with regression tests for both the fix and the retained DoS guard.

Extended reasoning...

Overview

This PR fixes a regression from #31175 in the Rust WebSocket upgrade client. The header-size cap was being applied to total bytes received before parsing, which incorrectly counted pipelined WebSocket frame bytes against the 16KB header limit when a server split the 101 response across reads and appended a large initial frame to the second segment. The fix moves the cap check from the pre-parse accumulation step to the ShortRead arm of the picohttp parse result — at which point picohttp has confirmed no \\r\\n\\r\\n exists, so every accumulated byte is provably header. The same edit is applied consistently to all three entry points (handle_data, handle_proxy_response, handle_decrypted_data).

Security risks

The original cap was DoS hardening. The fix preserves it: a server that streams header bytes without ever sending \\r\\n\\r\\n still hits ShortRead on every read, accumulates into me.body, and is terminated once the buffer exceeds max_http_header_size. The second new test (incomplete header larger than the cap is still rejected) directly exercises this. The only behavioral relaxation is that we now extend_from_slice before checking the cap rather than after, so worst-case transient allocation is max_http_header_size + one socket read — bounded by the kernel recv buffer and explicitly called out in the PR description. This matches the Zig reference implementation, which has no cap at all on this path. No new attack surface.

Level of scrutiny

Moderate. This is network-facing client code with unsafe blocks, but the diff is a mechanical relocation of an existing check within the same function bodies — control flow, the terminate SAFETY invariants, and the return placement are unchanged. The reasoning (ShortRead ⇒ no header terminator ⇒ all bytes are header) is sound and well-commented inline.

Other factors

No CODEOWNERS entry covers src/http_jsc/websocket_client/. The bug-hunting system found no issues. Two focused regression tests are added: one reproducing the original failure (split 101 + 20KB pipelined frame) and one confirming the DoS guard still fires. No prior reviews or outstanding comments on the PR.

[alii]

@robobun Can you figure out if this fixes 28599?

[robobun]

No, this doesn't fix #28599.

Different error code. #28599 reports Proxy connection failed, which maps to ErrorCode::ProxyConnectFailed. That is emitted at only two sites, both in handle_proxy_response (L998 and L1033), when the proxy's CONNECT response is not a 200. This PR only moved where ErrorCode::InvalidResponse fires (the header-size cap). It never touched either ProxyConnectFailed site.

Different precondition. handle_proxy_response only runs when state == ProxyHandshake, which is entered only when me.proxy.is_some(), i.e. a non-empty proxy option reached setupProxy. #32394 doesn't touch proxy detection/config at all.

Different timeline. #28599 regressed between 1.3.5 and 1.3.11. The cap this PR relaxed was introduced by #31175, which merged after 1.3.11. So this PR is undoing a change that didn't exist when #28599 broke.

The commits in the 1.3.5→1.3.11 window that actually plausibly cause #28599 are c90c0e6 (added WebSocket proxy support, introducing the ProxyConnectFailed path) and 32a7690 (added agent option handling to the ws module, where connectOpts?.proxy || agent?.proxy is read). The reporter's follow-up that it only reproduces under the VS Code debugger points at the debugger injecting an agent/proxy into the options, which 1.3.5 ignored and 1.3.6+ now honors.