Conversation
https.createServer is backed by Bun.serve (idleTimeout: 0) rather than node:tls's Server, so the handshakeTimeout option was never applied: a peer that connected over TCP and never began the TLS handshake was kept open indefinitely, bounded only by the fd limit. Add a per-HttpContext TLS handshake watchdog in uWS. Accepted SSL sockets queue on an intrusive FIFO and a single one-shot timer is armed at the head's deadline; the deadlines are non-decreasing, so the head is always the next to expire. When it fires, the socket is reported through node's 'tlsClientError' with ERR_TLS_HANDSHAKE_TIMEOUT and closed. The entry is removed once the handshake settles or the socket closes. node:http forwards handshakeTimeout (default 120s, like node) into the server and validates it the same way tls.Server does.
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
Comment |
|
Updated 9:05 PM PT - Jul 6th, 2026
❌ @robobun, your commit 6ce3396 has some failures in 🧪 To try this PR locally: bunx bun-pr 33541That installs a local version of the PR into your bun-33541 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Raise the live-request handshakeTimeout to 10s (a completed handshake is dequeued immediately, so this only avoids the watchdog racing a slow debug+ASAN loopback handshake) and clear the silent-peer guard timer so it cannot fire during later scenarios and emit a stray RESULT line.
There was a problem hiding this comment.
Both test-fixture nits from the earlier pass are addressed and I didn't find further issues, but this adds a new intrusive-list + us_timer_t watchdog directly in the uWS TLS accept path with manual lifecycle management across C++/Rust/JS — worth a human look at the memory/timer ordering.
Extended reasoning...
Overview
This PR wires handshakeTimeout from https.createServer down into uWebSockets by adding a per-HttpContext TLS handshake watchdog: an intrusive doubly-linked FIFO of pending-handshake sockets (links stored in HttpResponseData), a single lazily-created us_timer_t armed at the head's deadline, and a callback that unlinks expired sockets, reports them to JS via a new on_handshake_timeout StrongOptional, and closes them. Plumbing spans HttpContext.h/HttpContextData.h/HttpResponseData.h, the C shim in libuwsockets.cpp, Rust FFI in uws_sys/App.rs and runtime/server/{mod,server_body}.rs, the NodeHTTP.cpp binding, and _http_server.ts. 13 files changed.
Security risks
The change sits directly on the TLS accept path. It hardens against a slowloris-style handshake DoS, which is good, but it also introduces new state that is mutated from onOpen/onHandshake/onClose and from a timer callback that re-enters JS. The design looks careful (unlink-before-report, handshakeDeadline == 0 as the idempotence sentinel, timer freed after us_socket_group_deinit so onClose-driven re-arms don't UAF), but a mistake here would be a UAF or list corruption on every HTTPS connection.
Level of scrutiny
High. This is not a mechanical change: it adds a hand-rolled intrusive linked list with raw us_socket_t* pointers, a timer whose ext stores a raw HttpContext*, and a JS re-entry point mid-iteration. The free() ordering, the armedHandshakeDeadline short-circuit, the if constexpr (SSL) gating, and the interaction with socket adoption/upgrade all deserve a maintainer's eye. The Rust side also adds a new StrongOptional GC root and a *mut Self stashed as user_data.
Other factors
The two flakiness nits I raised on the fixture (tight 200ms handshake budget in liveRequest, uncleared 8s guard timer in silentPeer) were both fixed in c46e0b9 and the threads are resolved. The current bug-hunting pass found nothing. Test coverage exercises the timeout firing, a successful handshake not being killed, and option validation — reasonable but doesn't stress concurrent handshakes or shutdown-during-pending. Given the breadth (uWS internals + 4-language FFI chain + TLS path), I'm deferring rather than approving.
There was a problem hiding this comment.
Both test-fixture nits from the earlier pass are addressed and I found nothing further, but this adds a native timer + intrusive socket queue to the uWS TLS accept path with lifetime spanning C++/Rust/JS — worth a human look.
Extended reasoning...
Overview
This PR implements handshakeTimeout for https.createServer by adding a TLS handshake watchdog to uWS's HttpContext: an intrusive FIFO of pending-handshake sockets driven by a single us_timer_t, plumbed through the C ABI shim (libuwsockets.cpp), Rust FFI (uws_sys/App.rs, server_body.rs), C++ bindings (NodeHTTP.cpp), and the node:http JS layer (_http_server.ts). It touches 13 files across four languages.
Prior review
On the previous revision I left two inline nits about test-fixture flakiness (tight 200ms handshake budget in liveRequest(); an uncleared guard timer in silentPeer()). Both were addressed in c46e0b9 and the threads are resolved. The current bug-hunting pass found no issues.
Security risks
The change is itself a security hardening (slowloris against the TLS handshake). It sits directly in the TLS accept path: onOpen enqueues raw us_socket_t* pointers, a loop-owned timer fires into JS (which can synchronously close arbitrary sockets), and onClose/onHandshake dequeue. The intrusive-list and timer-lifetime handling look correct to me (unlink-before-callback, head re-read each iteration, timer freed after group deinit in free()), but raw-pointer queues that survive JS reentrancy in a security-sensitive path are exactly where a maintainer should sign off.
Level of scrutiny
High. This is not a mechanical change: it introduces new native state (timer, linked list, deadline bookkeeping) with non-trivial lifetime and reentrancy invariants, a new FFI surface, a StrongOptional GC root on the server, and a design decision (single one-shot timer over a monotone FIFO rather than per-socket timers) that a maintainer should agree with.
Other factors
Test coverage exercises the three headline behaviors (silent peer dropped, live request unaffected, invalid option throws) and the earlier flakiness concerns are fixed. CI build #69415 is in progress. Given the scope and the layers touched, I'm deferring rather than approving.
|
CI status: the diff is green on every lane it affects. The red lanes across the last three runs have all been unrelated flakes on paths this PR does not touch:
Different flaky tests each run, none in node:http/https, TLS, or the uWS server path. I verified locally that the handshake-timeout test passes and the plain-HTTP UAF "drain after onWritable undefined" test does not regress with this diff. Ready for a maintainer to merge. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-06, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
Problem
https.createServer({ handshakeTimeout })never enforced the timeout. A peer that opened a TCP connection and never began the TLS handshake was kept open indefinitely, holding a socket and BoringSSL state bounded only by the fd limit (a slowloris against the handshake). Bun's owntls.createServerenforces the same option correctly, which localizes the bug:https.Serveris backed byBun.serve(idleTimeout: 0), not bynode:tls'sServer, so the option was dropped on the floor.Cause
node:tls'sServerarms a per-socket timer in JS for every accepted connection (src/js/node/net.ts).Bun.servenever surfaces a pre-handshake socket to JS, so there is nothing to hang a JS timer on, and uSockets' only per-socket timer is the 4 second sweep wheel, too coarse to express a millisecondhandshakeTimeout.Fix
Add a TLS handshake watchdog to the uWS
HttpContext. Every accepted SSL socket queues on an intrusive FIFO, and a single one-shotus_timer_tis armed at the head's deadline. Arrival order keeps the deadlines non-decreasing, so the head is always the next to expire and one timer covers the whole list. When it fires, the socket is reported through node'stlsClientErrorwithERR_TLS_HANDSHAKE_TIMEOUTand closed. The entry is removed when the handshake settles (either way) or the socket closes first; the timer outlives the socket group so the drain on shutdown is safe.node:http's server forwardshandshakeTimeout(default 120s, matching node) into the server and validates it the same waytls.Serverdoes. It is only read for TLS servers; a plainhttp.Serverhas no handshake and ignores it, like node.Verification
test/js/node/http/node-https-handshake-timeout.test.ts(driven by a single fixture process, since debug+ASAN TLS accept is slow) covers:tlsClientError: ERR_TLS_HANDSHAKE_TIMEOUThandshakeTimeoutthrowsERR_INVALID_ARG_TYPEFails on the unfixed build (peer stays open, no event, option unvalidated); passes with the fix.