Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe HTTP server now accepts a ChangesHTTP server block list
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:06 AM PT - Oct 1st, 2026
❌ @robobun, your commit e389035 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38261That installs a local version of the PR into your bun-38261 --bun |
|
Status: reproduced and fixed. Ready for a maintainer.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds IP-based connection filtering (access control) and ships with an acknowledged divergence from Node — for https, a blocked peer completes the TLS handshake before being closed, whereas Node closes at TCP accept — a maintainer should sign off on that trade-off.
Checked: the onServerConnection gate mirrors the existing maxConnections path (tracked?.delete + socket.destroy()) and the net.Server blockList pattern exactly; isIP guards unix-socket peers; constructor validation matches net.ts; the unix-socket test follows the same un-gated pattern as node-http.test.ts:1209.
Extended reasoning...
Overview
Two files: ~20 source lines in src/js/node/_http_server.ts (read/validate options.blockList in the TLS constructor branch, and enforce server.blockList in onServerConnection right after the maxConnections gate) plus a new 7-case test file. The implementation copies the pattern already used at four sites in src/js/node/net.ts (BlockList.isBlockList validation, isIP → check(addr, 'ipv' + type), silent destroy).
Security risks
This is IP-based access control — security-adjacent by nature. The change is strictly tightening (adds enforcement where there was none), reads BlockList/isIP at module load (tamper-resistant), and validates the option with BlockList.isBlockList before storing it. The notable point is the documented divergence: for https, Bun invokes onServerConnection post-handshake (same as the existing maxConnections gate), so a blocked peer still completes a TLS handshake and consumes those cycles before being closed, whereas Node closes at TCP accept. The PR description is upfront about this and explains that fixing it requires a native uws hook. It's not a regression — it's the same limitation maxConnections already has here — but it's a design call a human should confirm is acceptable to ship as-is.
Level of scrutiny
Medium-high: small, mechanical change following an established in-tree pattern, but it's connection-level access control in the HTTP server hot path. The tests are thorough (both constructors, non-matching list served, invalid option rejected, post-listen() assignment, unix socket bypass, plain-http ctor ignored) and the PR includes empirical Node-vs-Bun output.
Other factors
No prior reviewer comments. The assigned-after-construction path (server.blockList = ...) is not validated at assignment time — same as Node, which also reads self.blockList unchecked in onconnection. The if (blockList) truthiness gate on the option matches net.ts (falsy values silently ignored). Given the security-adjacent nature and the embedded design decision about the handshake timing, deferring rather than approving.
|
On the handshake timing raised in the review: that is the trade-off described in the "Known difference" bullet above. The accept callback this uses is the one the native listener already provides, and it fires at the same point for |
329984e to
0a59ea8
Compare
There was a problem hiding this comment.
LGTM — small, well-scoped Node.js compat fix that mirrors the existing net.Server blockList pattern.
What was reviewed:
- The new
onServerConnectiongate mirrors themaxConnectionspath just above it (sametracked?.delete+socket.destroy()shape) and theblockList.check(addr, 'ipvN')call matchesnet.tsexactly. - The
isIPguard prevents a crash on unix-socket peers whereremoteAddressis undefined; the unix-socket test was checked for Windows compatibility (Bun supports AF_UNIX on Windows and CI passed all lanes). - Constructor validation is TLS-only, matching Node's layering where
tls.Serverforwards options tonet.Serverbuthttp.Serverdoes not; the truthyif (blockList)gate matchesnet.ts:1763andnet.ts:3525. - The known TLS-handshake-before-close timing difference is consistent with how
maxConnectionsalready behaves at the same hook.
Extended reasoning...
Overview
This PR adds blockList support to Bun's node:http / node:https server (src/js/node/_http_server.ts, ~23 lines). Two changes: (1) the Server constructor reads and validates options.blockList for TLS-backed servers and stores it as server.blockList; (2) onServerConnection checks this.blockList after the existing maxConnections gate and destroys matching peers before any 'connection' event or request handling. A new 7-case test file covers both https constructors, non-matching lists, invalid-option rejection, post-listen() assignment, unix sockets, and the plain-http constructor being ignored (matching Node).
Security risks
None introduced. The change adds enforcement of an IP deny list where previously it was silently ignored — a strict security improvement. The one documented gap (blocked https peers complete the TLS handshake before being closed, since the native hook fires post-handshake) is not a regression: it is the same point maxConnections is already enforced, no request bytes are dispatched, and no events fire. blockList.check() is called on a user-supplied net.BlockList instance, validated via BlockList.isBlockList — identical to the existing net.Server path.
Level of scrutiny
Low-to-moderate. onServerConnection runs on every accepted connection, but the added code is a single property read that short-circuits when no list is set, so the common path is unaffected. The implementation is a near-verbatim copy of the pattern already in src/js/node/net.ts (lines ~1152, ~1764, ~3524, ~4200), which reduces novelty risk. The constructor change is inside the existing TLS-only branch and cannot affect plain http servers.
Other factors
- Tests are hermetic (
port: 0, local TLS certs fromharness, try/finally cleanup, no sleeps, no external network) and were verified to fail onUSE_SYSTEM_BUN=1and pass on the debug build across 8 runs. - CI build 95053 passed the new test on every lane; the two red tests reported are pre-existing on main and unrelated (bake deinit segfault, worker-transfer stress).
- The one candidate issue raised during multi-agent review (unix-socket test on Windows) was refuted — Bun supports AF_UNIX on Windows and the test passed there in CI.
- No prior human or bot review comments are outstanding on this PR.
0a59ea8 to
266616a
Compare
There was a problem hiding this comment.
LGTM — small, well-scoped Node compat fix that mirrors the existing net.Server blockList pattern.
What was reviewed:
onServerConnectiongate: placement aftermaxConnections,tracked?.delete+socket.destroy()cleanup matches the sibling path just above; no'drop'emission (matches Node'sonconnection).- Constructor validation is TLS-only and uses the same lazy
$rust("node_net_binding.rs", "BlockList").isBlockListpattern asdgram.ts;isIPguard prevents throwing on unix-socket peers. - Tests cover both constructors, non-matching list, invalid option, post-
listen()assignment + clearing, unix socket, and plain-http ignored — CI passed on all platforms (the two red jobs were darwin agent-pool timeouts).
Extended reasoning...
Overview
This PR adds net.BlockList enforcement to Bun's node:http/node:https Server, which is backed by Bun.serve and therefore never passes through net.Server's existing blockList handling. Two changes to src/js/node/_http_server.ts (~23 lines): (1) inside the TLS branch of the Server constructor, validate options.blockList with BlockList.isBlockList and store it on this.blockList; (2) in onServerConnection, after the existing maxConnections gate, read this.blockList and destroy the socket if check(remoteAddress, 'ipv4'|'ipv6') matches. Plus a 151-line test file.
Security risks
None introduced. The change tightens behavior (drops connections that would previously have been served). blockList.check() is invoked on a user-supplied object read from a plain property, but that object is set by the server owner themselves — same as Node, and same as Bun's existing net.Server path at src/js/node/net.ts:4214. The documented difference that a blocked https peer completes the TLS handshake before being closed is inherited from where onServerConnection fires (same point maxConnections is enforced today) and is called out in the PR; the request handler still never runs.
Level of scrutiny
Low-to-medium. This is a Node-compat gap fill in the JS builtins layer with no native changes. Every line follows an established in-tree pattern verbatim: the lazy isBlockList helper is copied from dgram.ts:165-169; the ipv${addressType} check + isIP guard + silent destroy is the same shape as net.ts:4214-4223; the ERR_INVALID_ARG_TYPE throw matches net.ts:3540. The tracked?.delete(socket); socket.destroy(); return; cleanup mirrors the maxConnections branch immediately above it, so the tracking bookkeeping stays consistent.
Other factors
Test coverage is thorough — 7 cases spanning both https constructors, allowed vs blocked, validation, post-listen assignment and clearing, unix socket (exercises the isIP guard), and the intentional "plain http constructor ignores the option" Node quirk. The PR description shows 4/7 fail on the released binary and all pass with the fix, plus repeat-run stability and no regressions in node-http.test.ts or the related test-net-server-blocklist.js parallel tests. CI build 95423 had 177/179 green with the two failures being darwin agent-pool expirations (infra), and the previous build of the same diff passed that lane. No prior human or bot review comments to address.
https.createServer({ blockList }) accepted the option and served blocked
peers: the node:http Server constructor never read it, and nothing on the
accept path consulted server.blockList.
Store and validate options.blockList for TLS-backed servers, matching
Node where tls.Server passes its options to net.Server and http.Server
does not. In the native accept callback, right after the maxConnections
gate, close a peer whose address is on server.blockList without emitting
'connection' or 'drop', like Node's net onconnection. Reading the
property per connection also covers a list assigned after construction.
266616a to
e389035
Compare
| // Node's onconnection checks server.blockList next and closes a blocked peer silently: no 'drop', no 'connection'. | ||
| const blockList = this.blockList; | ||
| if (blockList) { | ||
| const remoteAddress = socket.remoteAddress; | ||
| const addressType = isIP(remoteAddress); | ||
| if (addressType && blockList.check(remoteAddress, `ipv${addressType}`)) { | ||
| tracked?.delete(socket); | ||
| socket.destroy(); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 (optional) Operators who put abusive IPs on an https server's blockList still pay a full server-side TLS handshake per blocked connection; Node closes at TCP accept. The check at src/js/node/_http_server.ts:1283-1291 runs from the uws open filter (1), which for TLS fires only in onHandshake after the handshake completed. The PR calls this a known difference needing a new accept hook, but uws already fires the filter with 2 at TCP accept for both transports and Rust already consumes it for connection counting; only the thunk at server_body.rs:3655 drops it. Fix: evaluate server.blockList against the peer address at the accept (2) notification, before any TLS work, for both http and https, while keeping 'connection'/'secureConnection' at the open (1) notification.
Why this was flagged
An https server created with { key, cert, blockList } receives a connection from a listed IP. HttpContext.h:224-227 fires every filter with 2 at TCP accept; the Rust thunk registered by server_set_on_connection at src/runtime/server/server_body.rs:3655 returns unless opened == 1, and for SSL the 1 notification is sent from onHandshake at HttpContext.h:173-177 after the handshake finished. So onServerConnection at src/js/node/_http_server.ts:1283-1291 only sees the peer once the server has done the key exchange and signed the handshake. The blocked peer is then destroyed. On Node the same peer is closed in onconnection before any TLS state is created, so a blocked IP costs no asymmetric crypto; after this merge a blocked IP can force the server into one full handshake per connection attempt. The PR text says a pre-handshake hook would need to be added to the uws/Rust accept path; src/runtime/server/mod.rs:498-519 shows on_connection_filter already receives and handles the 2 event, so the hook exists and only the node:http forwarding ignores it.
Verification: The Rust filter thunk at src/runtime/server/server_body.rs:3650-3657 does if opened != 1 { return; }. For SSL, uws sends 1 only from onHandshake at packages/bun-uws/src/HttpContext.h:172-177 (after isAuthorized = success). So a blocked https peer completes the full server-side TLS handshake before being destroyed, whereas Node's net.js onconnection closes at TCP accept.
There was a problem hiding this comment.
Confirmed on current main. HttpContext::onOpen fires every filter with 2 at TCP accept for both transports (packages/bun-uws/src/HttpContext.h:223-227), and the node:http thunk in server_set_on_connection forwards only 1. #38660 added that event on 2026-08-17, after this PR was opened. The sentence in the description about a missing accept hook was out of date, and I corrected it.
The accept notification is the better place for this check. Node closes there, the peer never starts a TLS handshake, and a blocked peer can also skip the JS socket object. The accept loop already checks for a socket that closed during open, and onOpen receives the peer address. The remaining work is native: evaluate the list in the 2 handler, and return from onOpen before 1 fires for a socket that the handler closed.
This PR does not contain that change. The head is the diff that was verified for merge, rebased with no change to the added lines. I will send the accept-time check as a follow-up, or add it here first if a maintainer prefers to hold the merge.
| if (blockList) { | ||
| const remoteAddress = socket.remoteAddress; | ||
| const addressType = isIP(remoteAddress); | ||
| if (addressType && blockList.check(remoteAddress, `ipv${addressType}`)) { |
There was a problem hiding this comment.
🟡 (optional) Operators whose assigned server.blockList throws from check get the blocked peer served anyway, where Node never dispatches the request. The call at src/js/node/_http_server.ts:1287 runs inside the native accept callback with no guard; a throw leaves the socket undestroyed, skips the return, and the native dispatcher goes on to parse and serve the request after dispatch::fold reports the error (src/runtime/server/server_body.rs:3662). Any process with an 'uncaughtException' handler keeps running with the deny list bypassed. Fix: make the gate fail closed: destroy the socket when the check throws (try/finally around the check, or validate on assignment via a blockList accessor) so an erroring blockList never lets a connection through.
Why this was flagged
Trigger: server.blockList = obj after construction (the PR explicitly supports post-listen assignment, test case 'assigned after listen') where obj.check is missing or throws, or a check implementation that throws for a given address. onServerConnection at src/js/node/_http_server.ts:1284-1292 calls blockList.check(remoteAddress, ...) with no try; the exception propagates out of the callback invoked at src/runtime/server/server_body.rs:3509 and is folded by crate::dispatch::fold (src/runtime/dispatch.rs:1308-1318) into report_error_or_terminate. With an 'uncaughtException' handler installed (common in long-running servers) the process continues, the socket created at line 1260 is still live and tracked, and the native server proceeds to dispatch the request to the handler as if no list existed. In Node the throw happens before the client handle is wrapped, so the request is never served. REVIEW.md: security checks fail closed; if the check's prerequisite fails, fail the operation. Remedy: destroy the socket on any throw from check, or validate the value at assignment time.
Verification: Triggered when the operator assigns a server.blockList whose check throws and the process has an 'uncaughtException' handler. src/js/node/_http_server.ts:1283-1292 calls blockList.check(...) with no try/catch; a throw skips socket.destroy(); return;. fold (src/runtime/dispatch.rs:1308) reports and returns, so the request handler runs. Base does not enforce blockList at all.
There was a problem hiding this comment.
Reproduced. With server.blockList = { check() { throw new Error() } } and an uncaughtException handler, Node v26.3.0 never serves the peer, and this branch answers 200 for http and https.
The fix is to destroy the socket before the error propagates:
if (addressType) {
// The native listener serves this socket unless it is destroyed here, so a check() that throws must not admit it.
let blocked = true;
try {
blocked = !!blockList.check(remoteAddress, `ipv${addressType}`);
} finally {
if (blocked) {
tracked?.delete(socket);
socket.destroy();
}
}
if (blocked) return;
}The test runs an http server and an https server in a child process that handles uncaughtException. Without the change both serve the peer (200, one request). With it both close the connection (ECONNRESET, zero requests), and the error still reaches the handler.
An accessor that validates on assignment would reject objects that Node accepts, so the guard stays at the call.
The change is written and passes locally. It is not pushed, because the head is the diff that was verified for merge. It goes in with the accept-time check from the other thread, or into this PR first if a maintainer prefers. The description lists this case under Downsides until then.
|
Superseded by #44618, which consolidates the open TLS pull requests. This fix and its tests are in there, either as written, rewritten smaller, or merged with the other PRs that patched the same cause (see the "By area" list in that PR). Closing in favor of it. |
The native listener bypasses net.Server's accept path, so a blockList given to https.createServer() or assigned to server.blockList was never consulted. Check it in onServerConnection next to the maxConnections gate and close a listed peer without an event, as Node does. A check() that throws closes the connection too: the native listener serves any socket that is still open when the callback returns. https.Server validates and stores the option whether or not it has a certificate; http.Server ignores it like Node.
The native listener bypasses net.Server's accept path, so a blockList given to https.createServer() or assigned to server.blockList was never consulted. Check it in onServerConnection next to the maxConnections gate and close a listed peer without an event, as Node does. A check() that throws closes the connection too: the native listener serves any socket that is still open when the callback returns. https.Server validates and stores the option whether or not it has a certificate; http.Server ignores it like Node.
The native listener bypasses net.Server's accept path, so a blockList given to https.createServer() or assigned to server.blockList was never consulted. Check it in onServerConnection next to the maxConnections gate and close a listed peer without an event, as Node does. A check() that throws closes the connection too: the native listener serves any socket that is still open when the callback returns. https.Server validates and stores the option whether or not it has a certificate; http.Server ignores it like Node.
The native listener bypasses net.Server's accept path, so a blockList given to https.createServer() or assigned to server.blockList was never consulted. Check it in onServerConnection next to the maxConnections gate and close a listed peer without an event, as Node does. A check() that throws closes the connection too: the native listener serves any socket that is still open when the callback returns. https.Server validates and stores the option whether or not it has a certificate; http.Server ignores it like Node.
…ps, WebSocket, SQL) (#44618) ### What does this PR do? Consolidates the open TLS pull requests into one. Each was reproduced on `main` and, for `node:*` behavior, on Node v26.3.0 first. About a third are ported as written, the rest are rewritten smaller or merged into one fix where several PRs patched the same cause. One commit per fix, so it can be read commit by commit. Fixes #43520, fixes #31396, fixes #43635, fixes #37193, fixes #43846, fixes #17932, fixes #41061, fixes #36887, fixes #31810, fixes #35240, fixes #32234, fixes #44365, fixes #43807, fixes #42280, fixes #44517. Addresses #41856 (SNI and `servername`; not `checkServerIdentity` for SQL), #24845 (the spin is gone, shown with fault injection on Linux; not run on macOS), #19754 (node-fetch forwards the agent's TLS options; the Kubernetes client itself was not run). #### The ones that matter most | | On `main` | PRs | |---|---|---| | Client certificate disclosure | `https.request()` with a client certificate sends it to a server it then refuses (wrong name, `checkServerIdentity`, `destroy()` in `'secureConnect'`, `terminate()` in `handshake`). A server can force it with a junk record behind its Finished | #43946 | | False `authorized` | Over a Duplex, `secureConnect` with `authorized === true` for a peer that failed the key proof; `secureConnect` for a plaintext peer with `rejectUnauthorized: false` | #44422, #32929 | | Cleartext https | `https.createServer()` without a usable key/cert answers plain HTTP | #41672, #33539 | | Revoked client certificates | An https mTLS server never sees `crl`, so a revoked client is `authorized` | #41641 | | Pooled sockets | Requests with different client certificates or CAs share an `https.Agent` socket and session | #42498 | | Silent plaintext | `tls: [...]` given to `Bun.listen` / `Bun.connect` is plain TCP | #41490 | | Server weakened by a client knob | `NODE_TLS_REJECT_UNAUTHORIZED=0` turns off a server's client-certificate enforcement | #35245 | | Pins never checked | `WebSocket` never calls `tls.checkServerIdentity` and ignores `tls.serverName` | #41648 | | `verify-full` dropped | `PGSSLMODE=verify-*` is lost next to a `TLS_*` URL variable; `tls: true` sends no SNI | #44498 | | Crashes | use-after-free from `destroy()` in `ALPNCallback` over a Duplex; `abort()` on a late `setSession()`; SIGABRT in `fetch` with an https proxy from the environment and a `Bun.file()` body | #44462, #41671, #44458 | | Stream corruption | A TLS `write()` can lose 16 KiB it reported as written while another socket on the loop is stalled | #44529 | | Hangs and spins | 100% CPU on a failing `send()`; a fatal `SSL_write` leaves the socket open forever; `idleTimeout` never sheds a TLS client that ignores `close_notify` | #34510, #38176, #42336 | | Wrong certificate (regression since 1.3.14) | Connections accepted before `stop()` / `close()` get the default certificate and skip their entry's `requestCert` / `ca` | #42355 | | Quadratic Duplex / proxy tunnel | Reading one chunk over a Duplex, CPU: 8 MB 0.88 s → 0.14 s, 16 MB 3.18 s → 0.23 s, 32 MB 11.75 s → 0.39 s; `fetch` upload through CONNECT: 1.7 s → 0.18 s (debug build) | #44464 | #### By area - **fd engine, write path** (`openssl.c`, `socket.c`): #42352, #34510 + #38176 + #42336 as one change, #44529, #44458, #44192. A rejected `send()` ends the write side only and closes at the next writable event unless the peer's bytes are still queued (a 413 sent before a reset is still read). No new per-socket state. Also, on kqueue, **a FIN no longer ends a socket that waits in the low-priority queue** (`loop.c`): with more than 5 TLS handshakes at once, a client that ended right after its handshake could be reset and its server socket report `socket hang up`, because the eof that the sentinel read knote reports was acted on ahead of the unread Finished. That is on `main` too (the macOS entry for `node-tls-server.test.ts` in `test/flaky-tests.txt`: 7 of 48 recent builds of other branches), and this branch made it likelier (6 of 8 builds), since Finished now leaves in one segment with the close_notify. - **Error reporting, both engines**: #44422, #32929, #44516, #37094, #41272 + #42324 + #44223 as one change, #44021, #37472, #43946, #33630. One channel: a fatal error on an established session is reported, then **the engine closes the connection itself**, whatever the owner does with the report. `test/js/bun/net/tls-fatal-error-closes.test.ts` asserts closed-and-nothing-delivered for every owner (node:tls, `Bun.connect`, `Bun.listen`, `fetch` direct and through CONNECT, `Bun.serve`, `WebSocket` direct and through a proxy, Postgres, MySQL, Valkey, Duplex). - **Duplex engine** (`SSLWrapper`, `UpgradedDuplex`): #44462, #43529, #42332, #44464. #43877 + #44394 were in and are **out again**, see "Worth a look" 5. - **node:tls wrap lifecycle** (`net.ts`, `tls.ts`): #38007, #38058, #38028 + #38122 + #38076 as one change (six copies of the attach code become two helpers), #38311, #39008, #38154, #42340 + #42343 + #42339 + #42453 as one change, #43791, #42425, #44085, #42683, #39088, #39040, #40375, and what was still real of #36534. - **SNI, ALPN, server contexts**: #43080, #42050, #37195 + #43849 as one change (**one** SNI matcher for TCP and HTTP/3), #42355, #42285, #33253, part of #37896, part of #37013. A `tls.Server` has one `SSL_CTX`. - **Verification and options**: #44738, #41490, #37005 + the cwd pin of #40984, #31811, #43982, #33483 + #35245, #41810, #32235, #44441, #38092. - **node:tls API and CA store**: #41671, #38145, #32824, #43594, #39997, #41696, #33534, #34748, #42991, #42996, #42970. - **node:https, Agent, `ws`, node-fetch**: #41672 (https half), #41641, #38261, #42498, #44346, #35609, #31397, #42325. - **WebSocket client**: #41648, #37487 + #43048 as one change. - **SQL, Redis**: #33666, #41711, #44498, part of #42054. - **Tests only**: #41426, #40040, #44395, #44016, #37860, #40591, #44440, #41424. Found on the way and fixed here: an upload that a TLS 1.2 server interrupts with a renegotiation never completes on `main` (0 of 32 runs over `https.request`, `fetch`, `node:tls` and `Bun.connect`: the renegotiation ClientHello lands inside an application record that is still unsent, or the socket gets no `drain` again) and completes here, with two tests from robobun; the fix for #40653 (final flight and first write in one segment) stopped working whenever another TLS socket on the loop was stalled, on `main` too; the `tls.Server` prototype pinned the last server constructed and every `SSL_CTX` it owned; `Object.create(process.env).NODE_TLS_REJECT_UNAUTHORIZED = "0"` turned verification off process-wide once a `SHARE_ENV` worker existed; two debug panics when wrapping a shut-down or still-connecting socket; a `fetch` POST through a proxy sent its headers twice when the origin renegotiated; `BlockList` ignored IPv6 zone ids; a test now ties `root_certs.der` to `certdata.txt`. #### Behavior changes - **A server's `ca` without `requestCert` no longer asks for a client certificate** (`Bun.serve`, `Bun.listen`, HTTP/3, node:tls). It matches the docs and Node. On `main` such a server refused clients with no certificate but served any unrelated self-signed one, so it was never authentication. **Set `requestCert: true` to require a certificate.** A matrix test pins that `requestCert: true` still refuses no certificate and an untrusted one on 8 kinds of server, TLS 1.2 and 1.3, with `NODE_TLS_REJECT_UNAUTHORIZED` unset and `0`. - `NODE_TLS_REJECT_UNAUTHORIZED=0` no longer relaxes a server. - `Bun.connect` / `Bun.listen` hear of a fatal TLS error after the handshake through `error(socket, err)`. With no `error` handler the socket just closes. - HTTP/3 server names match like TCP: `*.` covers exactly one label, case is ignored, a trailing dot is ignored, the last registration of a name wins. - `requestCert` on node:https is `=== true`, as in Node. - An array where a generated options dictionary is expected throws (`tls: []`, `jest.useFakeTimers([])`). - `key` / `cert` arrays serve every identity. A client that can use both gets ECDSA, where `main` served whichever pair came last. - `ecdhCurve` is forwarded by node:https, `ws` and node-fetch now, so a group BoringSSL lacks (`X448`) throws there as it already does in `tls.createServer`. - A wrapped socket's error is re-emitted on the TLS socket as in Node, so `raw.destroy(err)` with a listener on `raw` only is uncaught, as in Node. - `sql.options.tls` is always an object, never `true`. `RedisClient` sends SNI. - `tls: { secureContext }` alone asks for TLS on `Bun.listen` / `Bun.connect` (it was plain TCP), and a value that is not a `SecureContext` throws. The context is served as it is: the `requestCert` / `rejectUnauthorized` it was created with hold whatever the options next to it say, and `requestCert` in the options over a context that does not ask throws at `listen()`. - `tls.DEFAULT_CIPHERS` reaches every client once assigned (`fetch`, `WebSocket`, `Bun.connect`, `RedisClient`, `Bun.SQL`, `S3Client`, proxy tunnels) and servers again. A list that selects no cipher throws `ERR_SSL_NO_CIPHER_MATCH` at the assignment. `fetch.preconnect()` dials nothing after an assignment. - The warning for an unreadable `NODE_EXTRA_CA_CERTS` is Node's one line, without the `warn:` prefix. - `BUN_CONFIG_WS_CLOSE_TIMEOUT` (default 30 s): how long a `WebSocket` client waits for the server to close the connection after the closing handshake. #### Worth a look in review 1. **#44529**: the kernel-refused remainder of a TLS write moves from the loop's one slot onto the connection (in the existing rare struct), so the write BIO never refuses a sealed record. Nothing is allocated on an unstalled path (200 writes: 0 appends, same `send()` count as `main`), memory with 16 stalled writers is lower than on `main` (276 KB vs 340 KB, which `main` holds inside BoringSSL's buffers), `us_socket_t` stays 80 bytes. It needs a bound on how long a deferred close waits, or a peer that stops reading pins the fd past `destroy()`: `US_SSL_CLOSE_AFTER_SPILL_TIMEOUT` is a fixed 10 s, not re-armed on progress. Separate commits, but the fix that keeps the client certificate off the wire beside a stalled socket builds on them. 2. **The default name check of node:tls also runs inside the handshake**, so a wrong-name server gets no client certificate on TLS 1.2 either. JS still runs it after every successful handshake, so a difference between the two matchers can only refuse. Error objects are byte-identical. 3. **#44441** widens trust by design: a self-issued leaf whose `keyUsage` lacks `keyCertSign` (`dotnet dev-certs`) is its own anchor when the store holds a byte-identical copy. No BoringSSL change. Expired pin, same subject with another key, wrong EKU and a pinned intermediate are tested to fail. 4. **#32235** only adds Ed25519 and ECDSA P-521 to the verify list. A captured ClientHello shows `main`'s list with the two inserted; `rsa_pkcs1_sha1` stays. 5. **A stream that a TLS socket wraps, when that TLS socket closes.** An earlier state of this branch lost data here while CI was green (found by #44709's report): with the peer closing first, 4 of 8 MiB arrived with TLS in TLS, 4 of 32 MiB on the http2 `emit("connection")` path, and a `write()` with no `'error'` listener ended the process. Three Node-parity changes only hold together: destroying the wrapped stream at the close (#38028 + #38122 + #38076, #38154) is safe only if every write has really completed (#43877), which in turn needs Node's handling of the peer's close_notify, which needs half-open sockets that the GC can collect. So: - #43877 + #44394 are reverted and reopened. A write over a stream completes once the stream has taken the ciphertext, as on `main`. - Until the verdict on the peer lets the session through, the application cannot have written over it. There the wrapped stream is destroyed as in Node, with the sessions below it. That keeps the release of the connection after a failed handshake, a rejected certificate and an early `destroy()`. The same for an http2 socket the application never got, and for `resetAndDestroy()`. - After that it is `main`'s teardown: a `net.Socket` only gets the engine's `end()`, closes at its peer's FIN, keeps its own timeout and reports its own errors. Any other stream is destroyed with the TLS socket. The regular suites cannot see any of this (999 files were green on every broken variant), so it was steered by eleven seeded differential fuzzers run on this build, `main`, Node v26.3.0 and the earlier state: close, `end()`, `destroy()`, `destroySoon()`, resets, hung and half-open peers, paused writers, timeouts, two and three sessions deep, over TCP and over Duplexes, before, at and after the handshake, and http2 requests. See "How did you verify". #### Known limits - `fetch` with a `checkServerIdentity` function still sends the client certificate (not the request) to a server the function refuses. On TLS 1.2 any verdict a JS callback gives is too late, as in Node. - `addContext()` / `SNICallback` still do not apply to a server-side socket on the stream engine (`emit("connection", duplex)`, TLS in TLS, unflushed writes, named pipes), as on `main`. - A CA bundled in a pfx extends an explicit `ca` only, for `ws` / node-fetch / `WebSocket`: the native `ca` can only replace the default store, and that store keeps `SSL_CERT_FILE` / `SSL_CERT_DIR`. - P-521 leaves work on TLS 1.3 only. TLS 1.2 needs secp521r1 in every ClientHello (`it.todo`). - Once `tls.DEFAULT_CIPHERS` is assigned, `fetch(url, { protocol: "http3" })` is `HTTP3Unsupported`, as with an explicit `ciphers`. - `addCACert()` by hand does not extend the chains of a context with several identities. - A throwing `ALPNCallback` sends `no_application_protocol` on both engines. Node sends nothing and its client sees `ECONNRESET`. - TLS in TLS, peer FIN while the outer handshake runs: the inner socket gets one `write EPIPE`, where Node gives `ECONNRESET` (`main` gives it no error at all). - `@SECLEVEL` in `ciphers` is dropped by the `ws` / node-fetch shims, which used to ignore `ciphers`. node:tls keeps throwing `ERR_SSL_INVALID_COMMAND`. - Beside a stalled TLS socket only the first record (16 KiB) of the first write leaves with the handshake flight. The rest goes record by record, which is what bounds the memory of stalled writers. - After a fatal error on an established session the socket emits `'error'` and then `'close'`. Node emits `'error'` and leaves the socket open. - A paused reader whose own write the kernel rejects loses what it had not read yet, with an `EPIPE`, as on Node. `main` reports no error there and delivers it. - On `main` too: a `Bun.listen` socket without `allowHalfOpen` that has unsent ciphertext when the client's `shutdown()` arrives loses that ciphertext (32 KiB), and over plain TCP `end()` with the peer still sending is a close over unread input, so a reset. - Differences from both `main` and Node that the differential runs below found and that stay, all with a peer that aborts: `ECONNRESET` instead of a clean `'end'` after the socket's own `'finish'` when the peer destroyed with unread data; under TLS 1.2, a zero-length `write()` followed by `destroy()` in `'secureConnection'` leaves the client without `'secureConnect'` (a plain `destroy()` there matches Node); a TLS 1.2 client that destroys in `'secureConnect'` gets no `'session'`; a `ClientRequest` whose handshake fails with an alert emits `'error'` and `'close'` but no `'finish'` (`writableFinished` is true). - Once `tls.DEFAULT_CIPHERS` is assigned, `fetch.preconnect()` opens nothing: `fetch()` then uses a context of its own, and a socket warmed under the default one would never be picked up. - A TLS `send()` that the kernel refuses outside a `write()` call (the drain of unsent ciphertext) is reported with the close, as `read EPIPE` / `read ECONNRESET`. Node says `write EPIPE`. `main` does not report it at all. - Once the application has a session over a `net.Socket` (TLS in TLS, http2 `emit("connection")`), a peer that never sends its FIN holds that socket after the TLS socket closed, as on `main`. Node destroys it. Two tests of #38154 are `todo` for this. Closing it any earlier (at its `'finish'`, say) makes the kernel drop what it has not sent yet as soon as the peer's close_notify arrives. - Plaintext that was queued on a socket before it was wrapped (STARTTLS with a backlog) is dropped when the TLS socket is destroyed, or its handshake fails, before the session is accepted. Node drops it too, except on `destroySoon()`. `main` sends it. - Over a stream that is no `net.Socket`, `end()` can still cut what that stream has buffered, and there is no backpressure, both as on `main` (#43877). - `tls.secureContext` (the undocumented door node:tls uses) is not read by a Windows named pipe listener, which builds its context from the options. On `upgradeTLS({ isServer: true })` the options next to it are the policy, as with Node's `SetVerifyMode`. - `selectServerName()` rebuilds the name tree per ClientHello for injected sockets of a server with `addContext()` entries: 0.4 µs for 1 entry, 3.7 µs for 10, 41 µs for 100, against 631–1111 µs for a handshake. #### Not included Left open, because they need a decision or are not TLS: #43877 + #44394 (see "Worth a look" 5; #43874 stays open with them), #38548, #38591 (both shrink who is trusted), #41589 (`verify-full` vs `NODE_TLS_REJECT_UNAUTHORIZED=0`), #37197, #41706, #43216, #33487, #33545, #36707, #32435, #37255, #28691, #40275, #30314 (features), #38120 (needs the BoringSSL fork, as did #33517, which the stale bot has closed since), #38529 (needs a Windows measurement), #34342, #38232, #43089, #44454, #40451, #42710, #44527, #38088, #38093, #41898. #37896, #42054 and #37013 stay open for the halves not taken. `http.createServer({ key, cert })` keeps serving TLS on purpose. One open question: `tls: {}` (an object that names no TLS option) is plain TCP on `Bun.listen` / `Bun.connect`, here and on `main`. It is the same trap as `tls: []`, but changing it changes a Bun default, so it is left alone. ### How did you verify your code works? - Every new test fails on `main` for the stated reason and passes here, except guards that pin existing behavior, each shown to fail when its clause is removed. `node:*` tests also pass on Node v26.3.0; the few that cannot say which Node version has the behavior. - 212 test files that touch TLS, sockets, http, http2, fetch, WebSocket, SQL, Valkey and workers: 6154 pass, 2 fail. Both are seen on `main` too: `serve.test.ts` "root range port" (the box runs as root), and `worker_threads.test.ts` "terminate(): nothing of the worker's runs after the request", which is flaky there and passed in the run below. - 58 of those files the way the ASAN lane runs them (LeakSanitizer + `BUN_JSC_validateExceptionChecks`): 58 files, 48 of them with leak checking, 4132 pass, 3 fail. All three also fail on `main`: `serve.test.ts` "root range port", `node-net.test.ts` "should not leak when connect({path}) fails synchronously on a reused handle" (times out under this environment), `worker_threads.test.ts` "process.exit() with a shell cp in flight" (a `ShellCpTask` leak). - 647 vendored `test-tls-*`, `test-https-*`, `test-net-*`, `test-http2-*`: the only two failures also fail on `main`. - The SNI matcher was diffed against both old matchers: 3 seeds × 1.23 M lookups × 3 registration flavours, every difference in one of the intended classes, TCP and HTTP/3 identical on every lookup. - The headline rows were also driven by hand with scripts against this build, `main` and Node v26.3.0: cleartext https, `crl`, `tls: []` / `{ secureContext }`, the `ca` / `requestCert` matrix, the client certificate on a wrong-name server, late `setSession()`, `destroy()` in `ALPNCallback`, `[rsa, ec]` identities with an intermediate from `ca`, `WebSocket` `checkServerIdentity`, a corrupted record, the Duplex read above, `tls.DEFAULT_CIPHERS`. - The `setSession()` guard was checked against the real `abort()` at 43 handshake states. - `bun run rust:check-all`: 12 of 12 targets. `tsc`, oxlint, source lints, prettier, rustfmt, mordant clean. - usockets' `_Nonnull` is compiled out of debug builds, so 105 of those files were also run on a local release ASAN build with the CI runner's environment (92 with leak checking): 4595 pass, 1 fail, `child_process.test.ts` "spawn reports EPERM after dropping privileges", which cannot pass as root and fails on `main` too. - The close of a TLS socket over another stream ("Worth a look" 5): eleven seeded differential fuzzers, 8,424 scenarios compared, each run on a release ASAN build of this branch, on `main`, on Node v26.3.0 and on the earlier state of the branch. Against `main`: - Data that `main` delivers in full is cut in 5 scenarios, and about 150 that `main` cuts arrive in full. Of the 5, in 2 `main` never notices the peer's close and keeps the socket for good, 2 call `end()` on the middle one of three sessions over an in-memory Duplex, and 1 does the same on Node. - No dead timeout, no silent reset and no uncaught error that `main` does not have (4 uncaught errors fewer). - A socket stays open where `main` closes it in 109, and closes where `main` keeps it in 295. 92 of the 109 do the same on Node or on the earlier state (a `destroy()` that an in-memory Duplex does not show its peer, half-open peers). 14 wait for a peer that paused reading and so does not read the FIN (#42332's backpressure, as in Node); the socket's own timeout fires there. 3 are left: one on a 5 ms timer, two with three sessions over an in-memory Duplex. - The earlier state of the branch cut data in 173 of the 400 scenarios of one of them, where `main` cuts none and this cuts none. - 23 new tests pin what they found. Each earlier attempt at this fix fails the ones that describe it, the earlier state of the branch fails 7, and all pass on Node. - After that change: 999 test files on the release ASAN build (20,246 pass; the 11 files that fail need a database, Docker, DNS or a non-root user, or share a temp directory with a parallel run and pass alone), 61 on the debug build. - TLS over a file descriptor (`openssl.c`, the path of `fetch`, `Bun.serve`, `tls.connect`, `Bun.connect`) got the same treatment after the rebase: seeded differential fuzzers on CI's release build of this branch, on `main` and, for `node:*`, on Node v26.3.0. Every runtime also against itself for the noise floor, injected faults and known bugs of `main` as positive controls, and a difference counts only if it shows in 5 of 5 fresh processes. - `node:tls` over TCP: 11,500 scenarios (one connection with Node as the oracle line by line; 2 to 60 connections beside stalled neighbours; raw peers that break the handshake). HTTPS: about 136,000 runs over `Bun.serve` + `fetch`, `node:https`, `node:http2` and `wss://`, also with the two ends in different runtimes. `Bun.connect` / `Bun.listen` / `upgradeTLS`: 11,500 scenarios and 720 slow connections, with writers driven by what `write()` returns, beside up to 6 stalled, dripping, closing or resetting neighbours, and plain TCP as a second oracle. No crash, hang, duplication, reordering or silent truncation, and no change in time or in connection reuse. - They found six things that `main` does better, none of which any test showed. All are fixed, each with a test that fails on the build before: what the peer sent lost behind a rejected `send()` (23 scenarios, and an early HTTPS response lost with only `EPIPE`), the same silently for a paused reader, `server.close()` never calling back on a half-open server after a ClientHello and a reset (17), `closeAllConnections()` taking 12 s with a stalled client, `end()` losing up to 1.3 of 4 MiB that `write()` had reported while the peer still uploads, and `end()` a little after a stall never closing beside other stalled TLS sockets. The last two fixes also deliver the 1 to 2 MiB that `main` loses there, and close the socket that `main` keeps for good without such neighbours. - All of them again after every fix, on CI's release build of it. That caught one regression of a fix itself (a reader stopped for backpressure lost 86,385 bytes, 1 of 6,000 scenarios), fixed too. On the last build: scenarios that lose data where `main` does not 23 → 2, and Node loses it in both, with the same `EPIPE`; `server.close()` that never calls back 17 → 0; connections held 4 → 0; requests that end in an error only where `main` has a response 6 → 0. With a Node server in another process, a request ends in an error only in 8 and 10 of 1,500 scenarios here, 5 and 3 on `main`, 10 with Node as the client. - `Bun.connect` / `Bun.listen` on the last build against `main`, in scenarios: hangs 0 against 1,031, sockets and fds never released 0 against 965, corrupted data 0 against 345, `abort()` 0 against 26 (`setSession()` after the handshake), writers that never close 0 against 101 of 720 connections. No kind of failure shows here and not on `main`. About a third of the slow connections close later than on `main`, in 1 to 16 s instead of at once, waiting for unsent ciphertext or for the peer's close_notify, and 79 more of them deliver all that `write()` reported. RSS and time with 16 to 256 stalled writers are the same. - What they found that `main` does worse: a `WebSocket` that calls `close()` with sends pending loses messages in 81 of 999 scenarios (0 here), 37 server sockets left open, 10 `server.close()` that never call back, 20 write callbacks that never run. - The kqueue fix cannot be run on Linux. The `connectionListener` count test now says what became of a missing connection, which is how the cause was found (`'tlsClientError'` "socket hang up", then `read ECONNRESET` at the client of the same port, after its `'secureConnect'`). On macOS x64 it failed every attempt of the three builds before the fix and passed at the first attempt of the build with it. - Windows and macOS were only run by CI. Four new tests asserted what only the Linux kernel does (a FIN read ahead of a reset, unread bytes surviving a reset, loopback buffer sizes, `fstat()` on a socket) and now say so per platform. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
2 participants
Problem
https.createServer({ key, cert, blockList })serves peers that are on the list with a 200. Node v26.3.0 closes them at accept.server.blockList = liston anhttp.Serveris ignored too.Serverconstructor insrc/js/node/_http_server.tsnever readsoptions.blockList, and the accept callbackonServerConnectiononly checksmaxConnections.Fix
options.blockListfor TLS servers.http.createServer({ blockList })stays ignored, as in Node.onServerConnectiondestroys a peer thatserver.blockList.check()matches and emits no event, like Node.test/js/node/http/node-http-server-blocklist.test.ts(released binary fails 4 of 7),node-http.test.ts, and the upstream blocklist tests.Background
net.BlockListis Node's IP deny list.net.Serverchecks it at accept.tls.Serverpasses its options tonet.Server, andhttp.Serverdoes not.Bun.serve. Its native listener callsonServerConnectionat TCP accept for http and after the TLS handshake for https.Downsides
check()that throws does not block if the program handlesuncaughtException. A realnet.BlockListdoes not throw.perfandvalgrindare missing in the build container.Notes
Known gaps in detail
HttpContext::onOpenfires the connection filters with2at TCP accept for both transports (packages/bun-uws/src/HttpContext.h). Thenode:httpthunk inserver_set_on_connectionforwards only1, which for TLS fires after the handshake. A check at2needs a native change: evaluate the list in that handler, and return fromonOpenbefore1fires for a socket that the handler closed.check()that throws. Reproduced withserver.blockList = { check() { throw new Error() } }and anuncaughtExceptionhandler. Node v26.3.0: the handler never runs and the connection stays open (the client timed out after 2 s). This branch: 200 for http and https, because the native listener serves the socket unless the callback destroys it. A try/finally that destroys the socket before the error propagates closes the gap. It is written with a test and is not in this diff.blockListonly when TLS material is present, so the list is dropped.https.Serveris the same class ashttp.Serverin Bun, so the constructor has no other signal.server.emit('connection', socket)bypass the list. They do in Node too.remoteAddresscomes fromgetpeername()with no normalization in both runtimes. A v4 peer on a::listener is checked as::ffff:a.b.c.dwith'ipv6'.Repro output (Node v26.3.0, Bun 1.4.0, this branch)
Script:
https.createServer({ key, cert, blockList })with a list covering127.0.0.0/8, onehttps.getfrom127.0.0.1. Then the same withserver.blockListassigned on anhttp.Server. Thenhttp.createServer({ blockList }).Other checks
https.createServer()andnew https.Server()with a blocked peer (no handler, no'connection','secureConnection'or'drop', clientECONNRESET), a list that does not match, an invalid option, anhttp.Serverlist assigned afterlisten()and then cleared, a unix socket peer, andhttp.createServer({ blockList }).isIPguard was checked by removing it. The unix socket test then fails withTypeError: The "address" property must be of type string, got undefinedfromonServerConnection.4b02e10: the test file passes 7 of 7 in 5 runs, and the released binary fails 4 of 7.node-http.test.ts,node-http-server-timeouts.test.tsandnode-http-server-abort-events.test.ts: 384 pass, 0 fail.test-net-server-blocklist.js,test-net-blocklist.jsandtest-http-server-drop-connections-in-cluster.jspass.require("node:net")from_http_server.ts. This change keeps that:isIPcomes frominternal/net/isIP(already loaded byinternal/http), and theBlockListclass comes from the native binding on first use, the same helper shape as innode:dgram.[human-review] gate passed · iteration 4 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 4
evidence per changed file