Bun.serve: read every handshake header in server.upgrade() from req.headers

Closed
RO
1mo ago
+95-38

Problem

  • server.upgrade(req) accepts a request with two Sec-WebSocket-Key fields when the handler leaves req.headers alone, and rejects it once the handler reads any header. A handshake field the handler deleted from req.headers comes back in a synchronous upgrade().
  • Cause: the fallback loop in upgrade() (server_body.rs:1859 on main) read the materialized FetchHeaders, then the raw uWS request for each handshake header that view lacked. The raw request reports only the first field of a repeated name, and still reports a deleted one.

Fix

  • upgrade() takes the headers from Request::get_fetch_headers_unless_empty(), which materializes req.headers if the handler never touched it, and reads the five handshake headers only there. The raw fallback is gone.
  • Correct because the handshake now reads what the handler saw. A repeated key joins to K1, K2, which fails is_valid_sec_websocket_key (RFC 6455 §11.3.1). No new allocation: a successful upgrade materialized the headers anyway.
  • Supersedes #41821 (its test rows are included) and the duplicate-key half of #35870, whose Connection: upgrade check is separate.
  • Verified: test/js/bun/websocket/websocket-server.test.ts (four new tests, two fail on stock bun). Self-reviewed: two code concerns addressed, one (the same pattern in Request::get_content_type) left for a follow-up.

Background

  • req.headers is lazy: the first access (or leaving the handler's stack) copies the uWS HttpRequest fields into a WebCore FetchHeaders, which joins a repeated field with ", ". uWS getHeader returns the first field only.
  • uWS calls the upgrade handler only when Sec-WebSocket-Key is 24 bytes, so plain requests return false before any header read.
Notes

Observed on 1.4.2 and 1.4.3-canary (f42e98025) with a raw TCP client:

  • Sec-WebSocket-Key: K twice, handler does not touch req.headers: 101, sec-websocket-accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo= (the accept for the first key). Same after await 0 or after reading only req.url.
  • Same request after req.headers.get("host"), iteration, req.clone(), or await Bun.sleep(): upgrade() returns false.
  • req.headers.delete("sec-websocket-protocol") then a synchronous upgrade(): the 101 still carries sec-websocket-protocol: chat. After an await the delete is honoured. req.headers.set(...) always won, because the fallback only ran for absent entries. The same holds for sec-websocket-key, sec-websocket-version and upgrade.

Why plain requests pay nothing: uWS dispatches to the upgrade handler only when req->getHeader("sec-websocket-key").length() == 24 (packages/bun-uws/src/App.h:578), and UpgradeState::Pending is set only there. upgrade() returns false at the Pending check for anything else, so if (server.upgrade(req)) return; on a plain request never materializes headers. On a real handshake the old code already called createFromUWS before detaching the request, so the allocation only moves earlier.

#41821 fixes the deleted-header case alone by turning the fallback into an else if. The untouched path there still reads the raw first field, so the repeated-key request is still accepted. #35870 keeps the raw path and adds a C++ shim that counts Sec-WebSocket-Key field lines, plus a Connection: upgrade token requirement. This PR removes the second reader instead, which covers both cases with less code. If the Connection token check is wanted, it can read Connection from req.headers on top of this.

server.upgrade() validates Upgrade, Sec-WebSocket-Key and Sec-WebSocket-Version since #35298 and echoes Sec-WebSocket-Protocol / -Extensions into the 101 response. Those checks are unchanged. Only their input changed.

The review also flagged Request::get_content_type (used by formData() and the blob type), which reads the raw request first and FetchHeaders second, so it has the same flips-on-an-unrelated-await shape. That is a separate change and is not folded in here.

Suites run locally with the debug build: test/js/bun/websocket/websocket-server.test.ts (the ServerWebSocket > send* benchmark cluster times out under debug+ASAN on this machine with and without the change, release passes), websocket-server-upgrade-reentrant, websocket-upgrade-signal-gc, websocket-server-unmasked-frames, test/js/web/websocket/websocket-subprotocol-strict, websocket-custom-headers, websocket-upgrade, test/regression/issue/3613, test/js/first_party/ws/ws.test.ts, ws-upgrade-events, test/js/bun/http/bun-server.test.ts.

no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/websocket/websocket-server.test.ts