websocket: fail the client handshake on a missing subprotocol or an invalid Sec-WebSocket-Extensions response - #32825
websocket: fail the client handshake on a missing subprotocol or an invalid Sec-WebSocket-Extensions response#32825robobun wants to merge 9 commits into
Conversation
WalkthroughThe WebSocket upgrade path now rejects duplicate or unsupported extension tokens, fails handshakes when a requested subprotocol is not selected, and updates protocol-related close handling. Tests were expanded for stricter extension and subprotocol behavior. ChangesWebSocket handshake strictness
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:02 AM PT - Jul 14th, 2026
❌ @robobun, your commit f4a1439 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 32825That installs a local version of the PR into your bun-32825 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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-subprotocol-strict.test.ts`:
- Around line 61-65: The async disposer in the server setup returns before the
socket is fully closed, so update the [Symbol.asyncDispose] handler to wait for
server.close() to complete before resolving. Make the fix in the helper that
returns { port, [Symbol.asyncDispose] } so the websocket test cleanup is
deterministic and all open handles are released before the test exits.
🪄 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: d7914060-48c5-4640-9684-89cd008092f8
📒 Files selected for processing (3)
src/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/jsc/bindings/webcore/WebSocket.cpptest/js/web/websocket/websocket-subprotocol-strict.test.ts
There was a problem hiding this comment.
Thanks for adding the duplicate-permessage-deflate rejection — that resolves my earlier note. I didn't find anything else, but since this tightens client handshake validation in a way that will start failing connections that previously opened (and the description itself flags a couple of related calls for a maintainer), I'll leave the sign-off to a human.
Extended reasoning...
Overview
This PR tightens RFC 6455 / RFC 7692 client-side handshake validation in three places: (1) WebSocketUpgradeClient.rs now terminates with MissingClientProtocol when the client offered subprotocols but the server's 101 didn't select one, (2) the Sec-WebSocket-Extensions loop now rejects any token other than permessage-deflate and rejects a duplicate permessage-deflate (the latter added in 849ad55 in response to my earlier comment), and (3) WebSocket.cpp's missing_client_protocol/mismatch_client_protocol arms now match the other handshake-failure arms (NotClean, isConnectionError = true) so an error event fires and wasClean is false. The test file is substantially refactored and extended to 29 cases covering both the new rejections and should-still-succeed controls.
Security risks
None apparent. The change makes the client more strict about server responses, which if anything reduces the surface for a misbehaving server to slip an unexpected extension or protocol past the handshake. No new parsing of untrusted input beyond what already existed; the new branches are bounded comparisons inside the existing comma-split loop.
Level of scrutiny
Moderate-to-high. This is protocol-level logic in the WebSocket client's handshake path (Rust with unsafe FFI termination calls, plus C++ event-semantics changes), and it is a user-visible behavior change: connections that previously fired open against non-compliant servers will now fail with error + close. The change is spec-correct and matches Node/ws, but it can break code in the wild that depended on Bun's previously-lax behavior. The PR author also explicitly defers two adjacent compatibility questions (1002 vs 1006, binaryType default) to a maintainer, which reinforces that a human should weigh in on the compat posture here.
Other factors
- My one prior concern (silent first-wins → last-wins on duplicate
permessage-deflate) was addressed exactly as suggested, with two new regression tests. - The new
unsafeblocks follow the sameSelf::terminate(this, ...)+returnpattern used by every sibling validation inprocess_response, and the newsubprotocols.is_empty()read mirrors existing short-lived(*this)borrows in the same function. - Test coverage is thorough (failure cases, control cases, trailing-comma edge case, duplicate across one vs two headers), and the helper now asserts the full
error/close/wasCleancontract. - No CODEOWNERS coverage for these paths.
Given the behavior-change nature and the author's own flag for a maintainer call, I'm deferring rather than approving.
There was a problem hiding this comment.
I didn't find any issues and the implementation looks correct, but since this tightens client-side handshake validation (rejecting connections that previously opened against non-compliant servers), it's worth a maintainer sign-off on the compat tradeoff.
Extended reasoning...
Overview
This PR tightens RFC 6455/7692 enforcement in the WebSocket client handshake across three files: WebSocketUpgradeClient.rs adds three checks in process_response (reject duplicate permessage-deflate, reject any non-permessage-deflate extension token, and fail when the client requested subprotocols but the server's 101 selects none); WebSocket.cpp flips the missing_client_protocol / mismatch_client_protocol arms to CleanStatus::NotClean + isConnectionError = true to match every sibling handshake-failure arm; and websocket-subprotocol-strict.test.ts is restructured with shared helpers and ~13 new tests covering both rejections and should-still-succeed controls. My earlier inline note about duplicate permessage-deflate handling was addressed in 849ad55, and the CodeRabbit server.close() await was addressed in 6c1d916 — both threads are resolved.
Security risks
None identified. The change parses server-supplied response headers more strictly (fail-closed rather than fail-open), which if anything reduces attack surface. The new unsafe blocks are short-lived raw-pointer reads/calls following the exact same pattern already used throughout process_response, with the same SAFETY comments.
Level of scrutiny
This is production-critical networking code on the WebSocket client connect path, and it's a user-visible behavior change: connections that previously opened (e.g. to a server that ignores the requested subprotocol, or echoes an unrecognized extension) will now fail with an error event. The change is spec-correct and matches Node/ws, but the author themselves flagged adjacent divergences (1002 vs 1006, binaryType default) as "maintainer calls" — the same compat-vs-spec judgment applies here, so a human should confirm the stricter behavior is desired.
Other factors
The implementation is small (~25 lines of production code), follows established terminate(this, ErrorCode::X); return; patterns, and is thoroughly tested (31 tests, including controls verifying the happy paths still open). I verified subprotocols is populated from both the constructor argument and any user-supplied Sec-WebSocket-Protocol header, so the new is_empty() check is sound. No CODEOWNERS cover these paths. The single CI failure on the prior commit was a Windows-aarch64 agent provisioning error, not a test failure.
|
CI status: build 72921 (head
This diff touches Locally against the rebased base: 40 fail / 29 pass with |
There was a problem hiding this comment.
No issues found and my earlier feedback was addressed — deferring only because this tightens client handshake acceptance (previously-opened connections to servers that omit the subprotocol or send an unoffered extension will now fail), which is a spec-vs-compat call on a core API that's worth a maintainer signing off on.
Extended reasoning...
Overview
Three files: WebSocketUpgradeClient.rs (process_response) adds three new termination paths — duplicate permessage-deflate, any non-permessage-deflate extension token, and a missing Sec-WebSocket-Protocol when the client requested one. WebSocket.cpp flips the missing_client_protocol / mismatch_client_protocol close arms to NotClean + isConnectionError=true so they fire error and report wasClean: false like every other handshake failure. The test file is refactored to share helpers and adds ~13 new cases (6 rejections, 3 controls, 2 missing-subprotocol, 2 empty-list controls).
Security risks
None. The change makes the client stricter about what server responses it accepts; it does not loosen any validation, expose data, or touch auth/crypto. The unsafe blocks added are the same Self::terminate(this, ...) pattern used throughout the surrounding header loop.
Level of scrutiny
Moderate. The implementation is small, follows existing patterns exactly, and is thoroughly tested (31 cases, including should-still-succeed controls). What warrants a human look is the product decision: new WebSocket(url, ["proto"]) against a server that doesn't echo a subprotocol used to open and now fails, and any server that advertises a non-permessage-deflate extension now fails the handshake. That's RFC-6455-correct and matches Node 26 / npm ws, but it can break real users talking to non-compliant servers. The author themselves flagged adjacent divergences as "left for a maintainer call," so this one belongs in the same bucket.
Other factors
My one prior inline comment (duplicate permessage-deflate becoming last-wins after the break removal) was addressed exactly as suggested in 849ad55 and has two dedicated tests. CodeRabbit's server.close() await nit was addressed in 6c1d916. All review threads are resolved. CI red on build 65393 is darwin-aarch64 artifact-download infra timeouts, not test failures. No CODEOWNERS cover these paths.
robobun
changed the title
7dd043b to
dcbcea3
Compare
There was a problem hiding this comment.
Both earlier review threads are addressed and I found nothing new, but this tightens client-side handshake validation in a way that will start failing connections that used to open (missing subprotocol / invalid extension params), so I'd like a maintainer to confirm the stricter behavior before it lands.
Extended reasoning...
Overview
The PR replaces the inline Sec-WebSocket-Extensions parser in WebSocketUpgradeClient.rs with a strict accept_extensions_response() + parse_window_bits() + unquote() (~100 new lines of Rust), adds a MissingClientProtocol check after the header loop, adds an InvalidExtensionsHeader error code across the Rust enum / WebSocketErrorCode.h / the didFailWithErrorCode switch in WebSocket.cpp, and flips missing_client_protocol / mismatch_client_protocol to NotClean + isConnectionError=true. Two test files gain ~250 lines covering 20 invalid and 5+ valid extension responses plus the new subprotocol/error-event semantics.
Security risks
The new code parses untrusted server bytes, but it is a straightforward split/trim/match over a bounded header value with no allocation driven by attacker-controlled sizes, and every unrecognized or malformed shape now fails closed where the old parser silently accepted. I don't see injection, auth, or memory-safety concerns; the change is strictly more defensive than what it replaces.
Level of scrutiny
This is a deliberate, spec-correct behavior change to a production networking code path: connections to servers that omit Sec-WebSocket-Protocol after a requested subprotocol, or that send an unoffered/malformed permessage-deflate parameter, previously opened and will now fail with 1002. That matches RFC 6455/7692, Chrome, Firefox, and npm ws, and the author verified Bun's own server never emits a rejected shape — but it is still the kind of compat-affecting tightening a maintainer should sign off on. The PR description itself defers two adjacent divergences ("1002 vs 1006", binaryType default) to "a maintainer call", which reinforces that expectation.
Other factors
All three prior review threads (duplicate permessage-deflate, parse_int leniency, awaiting server.close()) are resolved in the current diff and covered by new tests. The bug-hunting pass found nothing. CI redness is documented as darwin-aarch64 agent-fleet unavailability, not test failures. The change is clean and well-tested; my only reason for not approving is that it's a non-trivial protocol behavior change rather than a mechanical fix.
dcbcea3 to
605bee2
Compare
…noffered extension RFC 6455 section 4.1 and the WHATWG "establish a WebSocket connection" algorithm both require the client to fail the connection when the server's 101: - does not select one of the subprotocols the client requested, or - indicates an extension the client's handshake did not offer. Bun's client opened the connection in both cases. The missing_client_protocol error code already existed but was never emitted. The extension guard only ran when the client offered nothing, and a `break` after the first permessage-deflate token skipped every token after it. Also make the missing/mismatch subprotocol branches fire the error event and report wasClean: false, matching every other handshake-failure branch.
…ponse Per RFC 7692 section 5 a server must not list a per-message compression extension more than once, whether in one Sec-WebSocket-Extensions value or across several. Terminate the handshake instead of merging the duplicate entries' parameters.
…poser The async disposer returned before the listening socket was actually closed, so cleanup was nondeterministic and could leak open handles into later tests.
…ake response The Sec-WebSocket-Extensions parser accepted any parameter list on a permessage-deflate response: unknown parameters were ignored, a missing, malformed, or out-of-range *_max_window_bits value was silently dropped (server_max_window_bits=20 became the default 15), parameters could repeat, and the no_context_takeover parameters could carry values. RFC 7692 section 8.1 requires failing the connection in each of those cases, and node ws, Chrome, and Firefox all do. The response parser is now a single validator that accepts exactly one permessage-deflate element whose parameters are the four defined ones, each at most once, with window bits restricted to a decimal 8..15 (required in a response, unlike in an offer). Extension negotiation failures now close with 1002 and a dedicated reason, "Invalid Sec-WebSocket-Extensions header", instead of the generic "Invalid response".
The value parser delegated to strings::parse_int, which also accepts a leading `+`, embedded `_` separators, and leading zeroes, so `server_max_window_bits=+10`, `=1_0`, and `=08` passed validation. The RFC 7692 section 8.1.2 grammar is a decimal integer without leading zeroes; reject those forms before parsing.
Hardening round 11 (#33072) landed the same "client requested subprotocols, server selected none" check upstream, so this branch's copy became a second, unreachable guard. Keep the upstream one and move this branch's RFC citation onto it.
The check that Hardening round 11 (#33072) added asserted the reason string "Missing client protocol", which this branch renames to "Server sent no subprotocol" because the old wording reads as if the client were at fault. Assert the new string, and also assert wasClean is false, the semantics this branch fixes.
605bee2 to
97a17c7
Compare
There was a problem hiding this comment.
No bugs found, but this is a behavior-changing protocol-validation rewrite in the WebSocket client handshake path — connections that previously opened will now fail — so it's worth a maintainer look.
Checked: accept_extensions_response against RFC 7692 §8.1 (unknown/repeated/valued/out-of-range params all reject via the _ => return false fallthrough); HeaderValueIterator skips empty list elements so the trailing-comma accept case holds; the new InvalidExtensionsHeader = 38 discriminant matches on both sides of the Rust/C++ FFI pair and the C++ switch handles it; the CleanStatus::NotClean + isConnectionError = true change to the two protocol arms matches every sibling handshake-failure arm. My two earlier inline findings (duplicate permessage-deflate merge, lenient parse_int) were both fixed and are covered by tests.
Extended reasoning...
Overview
The PR replaces the lenient inline Sec-WebSocket-Extensions parser in WebSocketUpgradeClient.rs::process_response with a strict accept_extensions_response() helper (~70 lines) plus parse_window_bits/unquote helpers, adds a new InvalidExtensionsHeader error code across the mirrored Rust/C++ ErrorCode enums, wires it into WebSocket::didFailWithErrorCode, and flips the missing_client_protocol/mismatch_client_protocol arms from CleanStatus::Clean to NotClean + isConnectionError = true. Two test files gain ~35 new cases (20 invalid extension responses, 5 valid controls, missing-subprotocol cases, and a strengthened expectConnectionFailure helper that now asserts exactly one error event and wasClean: false).
Security risks
None identified. The change tightens validation of server-controlled input (fails closed rather than silently accepting malformed extension parameters). No new allocation driven by untrusted lengths; parsing is bounded by the header value already parsed by picohttp.
Level of scrutiny
Medium-high. This is production WebSocket client code and a deliberate behavior change: servers whose 101 responses were previously tolerated (unoffered extension, out-of-range server_max_window_bits, valued no_context_takeover, etc.) will now fail the handshake. The PR description argues this is spec-mandated and matches Chrome/Firefox/npm ws, and confirms Bun's own server never emits a rejected shape — but the compatibility surface is real and the author explicitly flags two adjacent divergences as "left for a maintainer call". That, plus the ~100 lines of new protocol parsing, puts it above the auto-approve bar.
Other factors
The bug-hunting system found nothing this run. My two prior inline findings (silent first-wins→last-wins on duplicate permessage-deflate; strings::parse_int accepting +/_/leading zeros) were both addressed with tests. All review threads are resolved. Test coverage is thorough — negative and positive controls, quoted values, cross-header duplicates, and a post-negotiation inflate round-trip. The rebase-conflict resolution (discriminant 37→38, dedup of the subprotocol check that landed independently in #33072, reason-string rename) is correctly described and reflected in the diff. The renamed "Server sent no subprotocol" reason string is a user-visible change but the old string only became reachable after #33072 landed, so exposure is minimal.
`server.listen(0)` without a hostname plus `ws://localhost:` can fail
on a dual-stack agent whose `localhost` resolution is broken at the
time, surfacing as a `{ code: 1006, reason: "Failed to connect" }`
after the upgrade timeout. Bind to and connect over 127.0.0.1 like the
neighboring WebSocket test files do.
There was a problem hiding this comment.
No issues found, but this rewrites the Sec-WebSocket-Extensions response parser and changes the error/wasClean semantics for two handshake-failure codes, so it's worth a human look before merging.
Checked: the new accept_extensions_response covers unknown/repeated/valued-where-forbidden/out-of-range parameters with a rejecting fall-through; the ErrorCode enum discriminant 38 matches on both the Rust and C++ sides after the rebase; HeaderValueIterator skips empty list elements so a bare or trailing comma doesn't spuriously reject; and Bun.serve's own WebSocketExtensions.h output stays within what the new validator accepts. Both of my earlier inline findings (duplicate-element handling, parse_int leniency on +/_/leading zeroes) are fixed and now have tests.
Extended reasoning...
Overview
This PR tightens RFC 6455 §4.1 / RFC 7692 §8.1 enforcement in the WebSocket client handshake. It replaces the inline Sec-WebSocket-Extensions parser in WebSocketUpgradeClient.rs with a strict accept_extensions_response() that rejects unoffered extensions, duplicate permessage-deflate elements, and unknown/repeated/malformed parameters. It adds a new InvalidExtensionsHeader error code (Rust enum + WebSocketErrorCode.h + WebSocket.cpp switch arm), and flips missing_client_protocol/mismatch_client_protocol from CleanStatus::Clean to NotClean with isConnectionError = true so they fire an error event like every other handshake failure. ~470 lines changed across 6 files, roughly two-thirds of it tests.
Security risks
None introduced. The change validates untrusted server input more strictly than before; there is no new parsing of attacker-controlled data that could overflow or allocate unboundedly (the parser operates on borrowed slices of an already-bounded picohttp header value). The 1*DIGIT guard added for window-bits values closes a minor spec-leniency gap rather than opening one.
Level of scrutiny
Medium-high. This is production network protocol code on a hot path (process_response runs for every WebSocket client connection), and it's a behavior-tightening change: servers whose responses were previously accepted (out-of-range window bits silently clamped, unknown extensions silently ignored) will now fail the handshake. The PR description argues convincingly that Bun's own server, npm ws, and browsers already conform, and the adjacent test suites are unchanged — but a maintainer should confirm the compat trade-off is acceptable.
Other factors
- Test coverage is thorough: 20 invalid + 5 valid extension responses, duplicate-element in one header vs across two headers, a real inflate round-trip after negotiation, and the shared
expectConnectionFailurehelper now assertserror-event count andwasClean: falsefor all 11 pre-existing subprotocol-mismatch tests. - Both of my earlier inline findings on this PR were confirmed, fixed with dedicated commits, and covered by new test cases; all threads are resolved.
- The FFI enum pair is an easy place to silently break — verified
InvalidExtensionsHeader = 38on the Rust side matchesinvalid_extensions_header = 38in the C++ header after the rebase collision with #33395'sunexpected_rsv1 = 37. - The reason-string change ("Missing client protocol" → "Server sent no subprotocol") is technically a user-visible change, but the PR notes the old string was unreachable until #33072 landed, so nothing depends on it.
- CI (build 72895) is green on 278+ jobs; the one red test is an unrelated musl
net.createConnectionGC assertion.
Not approving because the parser rewrite, event-semantics change, and validation tightening are substantive enough that a maintainer should sign off — this isn't a mechanical change.
1 participant
Problem
RFC 6455 section 4.1 and the WHATWG "establish a WebSocket connection" algorithm both require the client to fail the WebSocket connection when the server's 101 response either:
RFC 7692 section 8.1 adds a third case for the one extension Bun offers: a
permessage-deflateresponse that carries a parameter not defined for use in a response, a repeated parameter, or a parameter with an invalid value must also fail the connection.Bun's client opened the connection in all of these cases. The parameter one is the dangerous one: extension parameters define how subsequent frames are compressed, and an accepted-but-ignored
server_max_window_bits=20(out of range, so Bun silently kept its default of 15) means a conforming server can compress with a window the client never agreed to. That surfaces later as inflate failures and abrupt 1006 disconnects instead of a clean handshake failure at the one moment it is debuggable.A fourth, related bug: the subprotocol-mismatch failure was the only handshake failure that skipped the
errorevent and reportedwasClean: true.Repro
Against a raw TCP server that replies with a valid 101 plus the crafted header (node
ws, Chrome, and Firefox fail all of these):After this PR, Bun fails 1 through 4 with an
errorevent followed by acloseevent withwasClean: falseand a descriptive 1002 reason, and 5 gets the same event semantics as every other handshake failure.Cause
process_responsetrackedprotocol_header_seenbut never checked it after the header loop, so "client requested protocols, server confirmed none" was never detected. Themissing_client_protocolerror code had existed in the enum the whole time with nothing emitting it. Hardening round 11 (Hardening round 11: input validation, bounds checks, lifetimes #33072) has since landed that detection on main, so it is no longer part of this diff; it also means the broken close semantics in the third bullet below became reachable for the first time.Sec-WebSocket-Extensionsguard only fired when the client offered no extensions. The default client offerspermessage-deflate, so any other token in the response was silently ignored. The parameter loop had no rejection path at all: unknown keys fell through an if/else chain with no final else, and a missing, malformed, or out-of-range*_max_window_bitsvalue just skipped the assignment and left the default.WebSocket::didFailWithErrorCode, themissing_client_protocolandmismatch_client_protocolarms passedCleanStatus::Cleanand omittedisConnectionError, unlike every sibling handshake-failure arm, so noerrorevent fired andCloseEvent.wasCleanwastrue.What does this PR do?
src/http_jsc/websocket_client/WebSocketUpgradeClient.rsaccept_extensions_response(): the response must contain exactly onepermessage-deflateelement (RFC 7692 section 5 forbids accepting it twice, whether in one header value or across several headers), and its parameters must be the four RFC 7692 defines, each at most once.server_no_context_takeoverandclient_no_context_takeovertake no value.server_max_window_bitsandclient_max_window_bitsrequire a decimal value between 8 and 15 in a response (only an offer may omit the value). Anything else terminates the handshake.DeflateNegotiationResult,ws.extensions, and the inflater.src/http_jsc/websocket_client.rs,src/jsc/bindings/webcore/WebSocketErrorCode.h,src/jsc/bindings/webcore/WebSocket.cppinvalid_extensions_headererror code, so extension failures close with 1002 and the reasonInvalid Sec-WebSocket-Extensions header(the wordingwsuses) instead of the genericInvalid response.missing_client_protocolandmismatch_client_protocolnow useCleanStatus::NotCleanandisConnectionError = truelike the other handshake-failure arms: theerrorevent fires andwasCleanisfalse. The previously unreachablemissing_client_protocolreason string is now "Server sent no subprotocol" (the wordingwsuses).This cannot reject Bun's own server: the
Sec-WebSocket-Extensionsline aBun.serve101 produces ispermessage-deflateplus an optionalclient_no_context_takeoverorclient_max_window_bits=Nand an optionalserver_no_context_takeoverorserver_max_window_bits=N, always valued and in range (packages/bun-uws/src/WebSocketExtensions.h).Not changed, on purpose. Two more divergences from the same report are deliberate, tested Bun behaviors, so I left them for a maintainer call rather than bundling a breaking change in here:
closecode 1002 with a descriptive reason; the spec and Node use 1006 with an empty reason. Every existing test inwebsocket-subprotocol-strict.test.tsasserts the 1002 + reason shape.binaryTypedefaults to"nodebuffer"and throwsSyntaxErroron an out-of-enum assignment; the spec says"blob"and silently ignore. Both are explicitly asserted inwebsocket-client.test.ts.How did you verify your code works?
test/js/web/websocket/websocket-subprotocol-strict.test.ts: two missing-subprotocol cases, a "strict RFC 6455 extension handling" describe (six rejections plus three should-still-succeed controls), and the sharedexpectConnectionFailurehelper asserts exactly oneerrorevent,wasClean: false, and the close reason, and rejects the instantopenfires. The eleven pre-existing subprotocol-mismatch tests thereby become the regression tests for the event-semantics fix.test/js/web/websocket/websocket-permessage-deflate-edge-cases.test.ts: a "Sec-WebSocket-Extensions response validation" describe against a byte-controlled raw 101 that also sends a frame, alongside the RSV1 frame tests main added in #33395. Twenty invalid responses (unoffered extension names, a duplicated element, unknown parameters, out-of-range, malformed, missing, signed, separator-bearing, leading-zero, and repeated window bits, valued no_context_takeover) must produce noopen, no message, and a 1002 close with the new reason. Five valid responses, including a quoted parameter value and no extensions header at all, must open, deliver the frame, and report the negotiatedws.extensions. A final case round-trips a real RSV1 deflate frame after negotiation.Against the rebased base (
src/reverted tomain, tests from this branch, which is what the two files prove between them):Every in-tree test that passes a client subprotocol connects to a server that echoes one back (
Bun.serveandws.Serverboth select the first offered protocol by default), so none are affected by the new check. The adjacent suites (regression/issue/29684.test.ts,websocket-permessage-deflate*.test.ts,websocket-custom-headers.test.ts,websocket-accept-header-validation.test.ts,websocket-close-connecting.test.ts,websocket-proxy.test.ts,test-ws-bidir-proxy.test.ts,websocket.test.js,first_party/ws/ws.test.ts) have the same pass/fail set before and after the change; the handful of pre-existing failures in them (external-network hosts, the 30 documented timeouts inws.test.ts) are identical withsrc/stashed.Rebase notes
Rebased onto
mainafter several WebSocket changes landed there. The conflicts and how they were resolved:37forunexpected_rsv1in bothWebSocketErrorCode.hand the mirrored RustErrorCodeenum, and this branch had taken37forinvalid_extensions_header. Those two enums are an FFI pair (Rust sends thei32, C++ switches on it), so a mismatch would report the wrong error. The landed value keeps37andinvalid_extensions_headermoved to38, in both files.reason === "Missing client protocol", which this branch renames to"Server sent no subprotocol"(the old wording reads as if the client were at fault;wsuses the new one). That string was unreachable until Hardening round 11: input validation, bounds checks, lifetimes #33072 landed, so nothing in the wild can depend on it. The assertion is updated and strengthened to also checkwasClean: false, which is the semantics this branch fixes. It was the only assertion on that string in the repo.websocket-permessage-deflate-edge-cases.test.ts. All are kept.The full WebSocket suite passes on the rebased branch, plus the pre-existing external-network and proxy-sandbox failures noted above, unchanged.
[review] gate passed · iteration 20 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 20
evidence per changed file