Conversation
… req.headers Once request.headers exists on a Bun.serve Request, server.upgrade() now reads Sec-WebSocket-Key/-Protocol/-Extensions/-Version and Upgrade only from it. Before, a field missing from the Headers object fell back to the raw uWS request line, so a header the handler deleted came back, but only when upgrade() ran before the first await. After an await the raw request is gone and the deletion was honored. Both paths now agree.
|
Warning Review limit reached
On-demand reviews are free for the next 13 days. After that, they cost $0.25 per reviewed file. Or wait 57 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 6:41 PM PT - Sep 7th, 2026
❌ @Jarred-Sumner, your commit ec18fdd has 8 failures in
🧪 To try this PR locally: bunx bun-pr 41821That installs a local version of the PR into your bun-41821 --bun |
|
Status Reproduced on canary Fail-before / pass-after: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused fix at the right layer with a test that would have caught the original bug.
What was reviewed:
- The
if→else ifconversion inserver_body.rs: theupgraderbinding used at theelse ifis the live shared ref from line 1803; nothing between there and 1862 runs user JS or invalidates it, so dropping the redundant re-borrow is safe. - Considered whether a
Requestcould reach this path with aFetchHeadersthat was not built from the raw uWS request — it cannot, because any Request without the matchingrequest_contextbails at the tagged-pointer check above. - The new test reuses the describe's
server/afterEach(stop)andrawHandshakehelper, covers both sync and detached (setImmediate) paths, all four handshake headers, and includes a control case that still upgrades and selects the first subprotocol.
Extended reasoning...
Overview
This PR fixes server.upgrade() in Bun.serve so that WebSocket handshake headers deleted from req.headers by the fetch handler are not silently resurrected from the underlying uWS raw request. The Rust change in src/runtime/server/server_body.rs is a five-line control-flow tweak: the two header-source blocks (JS FetchHeaders vs. raw uWS request) become mutually exclusive via else if instead of sequential with a per-field is_empty() fallback. A redundant unsafe { &*upgrader_ptr } re-borrow and the now-vacuous if value.is_empty() guard are removed, and a four-line comment records the invariant that once FetchHeaders exists it was built from every raw field, so a missing field means the handler deleted it. A new it.each in test/js/bun/websocket/websocket-server.test.ts exercises both the synchronous and post-setImmediate paths for each of the four handshake headers plus a control case.
Security risks
None identified. The change tightens behavior — a handler that removes Sec-WebSocket-Key, Upgrade, or Sec-WebSocket-Version now correctly causes the upgrade to be rejected (400/426) instead of succeeding via the raw-request fallback. No new untrusted-input parsing is introduced; the RFC 6455 validation logic below the changed block is untouched.
Level of scrutiny
Low-to-moderate. The diff is small, mechanical, and localized to one function. I traced the removed unsafe re-borrow: the upgrader reference used at the new else if site is the shared borrow established at line 1803, and no user-JS re-entry occurs between there and line 1862 (the fast_get calls are pure C++ FFI header lookups), so the borrow remains valid and the ref_() at line 1820 keeps the object alive. I also checked whether the "FetchHeaders is authoritative" assumption could be violated by a user-constructed Request — it cannot, because such a request has no ServerRequestContext and returns false at the tagged-pointer match well before this block.
Other factors
The test is placed in the existing describe block for handshake validation, reuses its server/afterEach/rawHandshake scaffolding, uses port: 0, awaits real events (no timing sleeps), and includes a positive control so the "before any await" case demonstrably fails on the pre-fix code path. The PR description's claim that the post-await path already behaved correctly is consistent with the code (raw upgrader.req is None after detach). Bug-hunt exit reason was dry_streak with no findings and no ruled-out candidates. No prior reviews or outstanding objections on the timeline.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Once `request.headers` exists it is the whole handshake: it was built | ||
| // from every field of the uWS request, so a field missing from it now | ||
| // was deleted by the handler and must not be read back from the raw | ||
| // request (the detached, post-await path never could). | ||
| // |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Superseded by #41962. That PR removes the raw-request fallback in |
2 participants
Problem
Bun.servehandler,req.headers.delete("sec-websocket-protocol")thenserver.upgrade(req)in the same tick still answers101withSec-WebSocket-Protocol: chat. Deletingsec-websocket-key,sec-websocket-versionorupgradestill upgrades. With anyawaitbeforeupgrade()the same code honors the deletion (101without a protocol,400,426,400).upgrade()(src/runtime/server/server_body.rs:1836-1881). It reads the five handshake fields from the Request'sFetchHeaders, then fills each field that is still empty fromr.header(name)on the live uWS request. A deleted field is empty, so the raw line comes back. After anawaitthe uWS request is gone and there is no fallback.Fix
request.headersexists, read the handshake from it alone. Read the raw uWS request only when no Headers object was ever created.Bun.serveRequest builds itsFetchHeadersfrom every field of the uWS request (Request.rs:252). A field missing from it later was removed by the handler. The post-awaitpath already worked this way.test/js/bun/websocket/websocket-server.test.ts(newit.each, the "before any await" case fails on canaryf42e98025). Also ran the rest of that file and the otherserver.upgrade()test files.Background
server.upgrade(req)validates the opening handshake (RFC 6455 §4.2.1) from these fields, echoes the first offered subprotocol, and derivesSec-WebSocket-Acceptfrom the key.Bun.serveRequest is lazy:req.headersis created from the uWS request on first access. Untilfetch()awaits, the Request points at the live uWS request. Whenfetch()returns a pending promise, the server snapshots url and headers and detaches it.Notes
Repro matrix on canary
f42e98025(raw request offersSec-WebSocket-Protocol: chat, superchat), synchronousupgrade():req.headers.delete(...)sec-websocket-protocol101,Sec-WebSocket-Protocol: chat101, no protocol headersec-websocket-key101400(upgrade()returns false)sec-websocket-version101426 Upgrade Requiredupgrade101400(upgrade()returns false)The
NodeHTTPResponsebranch ofupgrade()(node:http servers) is a separate code path and is not touched.An
await Promise.resolve()beforeupgrade()does not reach the detached path: the microtask runs before the request leaves the HTTP parser's stack. The test usessetImmediatefor the "after an await" case.Other suites run locally on the debug build:
websocket-server-upgrade-reentrant.test.ts,websocket-upgrade-signal-gc.test.ts,bun-server.test.ts,test/js/web/websocket/websocket.test.js. Under the debug ASAN build,ServerWebSocket > send() > (benchmark)(330k compressed echo messages) times out at 30 s in this container regardless of this change. It never touchesreq.headersand takes the unchanged raw-request branch.no test proof · iteration 0 · 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