node:tls,node:net: follow-ups from the v26.3.0 review (error routing, manualStart reads, handshake timeout, setSecureContext) - #35006
Conversation
…faces Squash of the node:tls v26.3.0 compatibility work: vendored test sync, net/tls runtime fixes, new SecureContext options (crl, sessionTimeout, allowPartialTrustChain, sigalgs), strict client cert verification during the handshake, and CA-supplied intermediate chain presentation.
…#33199) ### What `new tls.SecureContext(options)` (the exported constructor) returned a wrapper around the **digest-interned, shared** native `SSL_CTX`. Mutating it therefore mutated every other context with the same configuration digest: ```js const a = new tls.SecureContext({ ca: ca2 }); const b = new tls.SecureContext({ ca: ca2 }); // independent object, same interned SSL_CTX a.context.addCACert(ca1); tls.connect({ secureContext: b, ... }); // b now trusts ca1 too ``` Against a server whose chain roots in `ca1` (not in the default roots), **node v26.3.0 fails closed** (`UNABLE_TO_GET_ISSUER_CERT_LOCALLY`) while Bun completed the handshake with `authorized=true` — an extra CA silently trusted by connections that never asked for it. (Surfaced by a security scan of this branch; reproduced end-to-end against both runtimes before fixing.) `createSecureContext()` already builds a **private** context for exactly this reason ("a user-constructed context owns its SSL_CTX exclusively, so addCACert can never leak across contexts"); the exported constructor was the one user-constructible path that missed the invariant. It now passes `cached: false` too. Internal paths (`tls.connect`/`Server`/`fetch`) keep the shared cache: their contexts are never exposed for mutation, and that sharing is the cache's purpose. Regression tests (both in `ssl-ctx-cache.test.ts`, next to their `createSecureContext` siblings): (1) two `new tls.SecureContext()` instances with identical options get **distinct native handles** — the interned cache handed both the same cell before the fix; (2) the end-to-end scenario above — after `a.context.addCACert(ca1)`, a connection using `b` still fails with node's exact `UNABLE_TO_GET_ISSUER_CERT_LOCALLY` (on the unfixed code it completed with `authorized=true`). ### The other scan findings that touch this PR stack's files (triaged, not fixed here) Each was verified against the sources before deciding: - **`upgradeTLS` `initialData` used after re-entrant JS can free the backing store** (`socket_body.rs`): real, pre-existing on `main` — and since fixed upstream by #33388, so nothing is owed here anymore. - **TLS 1.3 session resumption marks certificate-less clients as verified** (`openssl.c`): the resumption arm pre-exists on `main` and is a faithful port of Node's own `VerifyPeerCertificate` (`src/crypto/crypto_common.cc`), which Node's server path also uses — so Bun matches Node here by construction. Hardening beyond Node would be a deliberate divergence decision. - **TLS socket reports `authorized=true` despite failed chain verification** (two findings, `socket_body.rs` `on_handshake`): the scan itself notes the computation is byte-identical to upstream Bun; it concerns the Bun-native `Bun.connect`/`Bun.listen` socket API (node:tls and fetch enforce verification in their own layers). Changing a Bun-native API contract needs a maintainer decision, not a drive-by in a node:tls compat stack. ### Notes from the pre-PR review pass - The exported constructor now simply delegates to `createSecureContext()` (one source of truth for the ownership contract), and the three comments that enumerated `createSecureContext` as the *only* exclusively-owned constructor were updated so the enumeration cannot rot into a regression. - Informational, unchanged: a digest-cached native context with a callable `addCACert` is still reachable if code deliberately digs out the registered internal symbol (`::buntlsnativesecurecontextctor::`); that is outside the public API surface. --- Rebased onto the updated base branch (which itself was rebased onto current `main`). --------- Co-authored-by: Alistair Smith <hi@alistair.sh>
…ode-verbatim - kReaderInterest destroySoon check now runs after a setImmediate so a connection handler that attaches its reader via nextTick / microtask / setImmediate still receives buffered bytes; the comment now states this is a deliberate divergence (Node also holds the loop for such sockets). - close(hadError) reverts to Node's literal 'exception ? true : false' (lib/net.js:880); the previous OR-in of _hadError flipped close(false)->close(true) on the server 'peer did not return a certificate' path where Node emits close(false). - drainOnreadTail honors an interleaved pause(): a resume()->pause() before the drain tick, or a pause() from inside the callback returning non-false, now leaves the handle stopped like Node's level-triggered _handle.reading. - Reworded the onread slice-delivery comment to say WHY (uSockets shared 512KB recv_buf) so the loop is not 'fixed' by removing it. Adds accepted-socket buffering coverage (readable-listener + setImmediate data-listener) and an onread resume->pause test.
…e; fix option layering
- authorizationError is now initialized to null in the TLSSocket
constructor (lib/internal/tls/wrap.js:556) so client-side and
no-requestCert clean handshakes report null like Node; the redundant
server-handshake assignment in net.ts is dropped.
- tls.Server builds one _sharedCreds SecureContext (lazily, on first
STARTTLS emit) from the post-normalized server fields and reuses it,
instead of rebuilding a fresh un-normalized SSL_CTX per emitted
socket. This also fixes the STARTTLS wrap path dropping the server's
honorCipherOrder->SSL_OP_CIPHER_SERVER_PREFERENCE default.
- honorCipherOrder is folded into secureOptions inside
newNativeSecureContext (Node common.js:108) so createSecureContext,
addContext and SNICallback contexts carry it too.
- CIPHER_LIST_SELECTORS gains kPSK/aPSK/AES128/AES256/FIPS from
BoringSSL's kCipherAliases so ciphers:'AES128' is not rejected before
BoringSSL sees it.
- InternalSecureContext now validates crl via throwOnInvalidTLSArray, so
createSecureContext({crl:123}) throws the same ERR_INVALID_ARG_TYPE
as Server.setSecureContext.
- tls.connect gates the NODE_TLS_REJECT_UNAUTHORIZED fallback on
key-presence, not === undefined: an explicit rejectUnauthorized:
undefined now coerces to true via Node's spread-then-!==false and no
longer honors the env var.
…OPERATION_FAILED - The auto-chain walk is no longer gated on options.ca: Node clears SSL_MODE_NO_AUTO_CHAIN unconditionally (crypto_context.cc:1640) so intermediates from NODE_EXTRA_CA_CERTS / the default store are also chained. The 'BoringSSL has none' comment was wrong at Bun's pin (SSL_MODE_NO_AUTO_CHAIN exists and is off by default). - us_ssl_ctx_add_ca_cert re-runs the auto-chain walk when the context has a leaf and no chain yet, so PKCS#12-bundled intermediates that reach the store via addCACert after the context was built are still presented (Node's LoadPKCS12 adds them via SSL_CTX_add1_chain_cert). - create_bun_socket_error_t::invalid_crl now maps to ERR_CRYPTO_OPERATION_FAILED 'Failed to parse CRL' matching Node's SetCRL (crypto_context.cc:1893-1903), which uses ClearErrorOnReturn and no OpenSSL decoration.
…ch Node's onread buffer rules
An inherited `rejectUnauthorized` or `checkServerIdentity` could reach the
socket: `"x" in options` walks the prototype chain, so a polluted
`Object.prototype` turned certificate verification off, or installed its own
hostname verifier. Node merges `{...defaults, ...options}`, which copies own
properties only. Resolve both from own keys and hand the socket a merged clone
that carries Node's defaults, so an inherited value is never visible.
onread:
- A buffer factory that returns a non-Uint8Array keeps the previous buffer,
and is handed the literal `true` until it yields one, like Node's kBuffer.
A static non-Uint8Array buffer leaves the socket an ordinary 'data' stream.
Previously bun passed its internal chunk through, or threw a TypeError.
- Hold a clean EOF behind a tail the callback has not taken yet: Node's
readStop leaves those bytes, and the FIN behind them, unread in the kernel.
- The EOF nudge that makes 'end' fire now goes through Duplex.read, so it no
longer redelivers a paused tail the callback has not asked for.
Also use the native isUint8Array from node:util/types instead of an
instanceof check, and collapse tls.connect()'s double options clone into one.
A SecureContext built with `ca` sets SSL_VERIFY_PEER|FAIL_IF_NO_PEER_CERT on
its shared SSL_CTX. The STARTTLS/adopt path only overrode the verify mode
when requestCert was set, so a cert-less client's handshake aborted with
PEER_DID_NOT_RETURN_A_CERTIFICATE on the server.emit('connection') path while
the same server's native accept path succeeded. Node's TLSWrap::SetVerifyMode
runs unconditionally on server sockets and forces SSL_VERIFY_NONE for
!requestCert; do the same.
…of throwing Node returns whatever SSL_set_max_send_fragment returns: OpenSSL rejects a size outside [512, SSL3_RT_MAX_PLAIN_LENGTH] with 0, which surfaces as false. Bun's native binding hand-rolled the range check with a floor of 1 and threw a codeless Error, so setMaxSendFragment(0)/(16385) threw where Node returns false, and 2..511 were accepted where Node rejects them. BoringSSL clamps into the range and always returns 1, so the rejection has to live here.
Socket.prototype._destroy read `this.server`, which the STARTTLS wrap sets
(like Node's tlsConnectionListener) even though server.emit('connection')
never runs the onconnection increment. A never-listened tls.Server used as a
STARTTLS dispatcher therefore went to _connections = -1 and emitted a
spurious 'close' after every wrapped connection. Node keys the decrement on
`_server`, which only the native accept path sets; do the same.
The suppression #22806 added for the SSL_CTX a tls.Server builds at listen() was keyed to create_ssl_context_from_bun_options. #29932 renamed that path to BunSocketContextOptions::create_ssl_context / us_ssl_ctx_from_options, so the suppression stopped matching and the known ~148KB leak resurfaces as a bare SIGABRT on the x64-asan lane for any shard containing a TLS-server test. Main never runs the test shards, which is why the rename went unnoticed.
…ild's failures were all agent-pinned infra
…a throwing onread closed; make setSecureContext transactional Three review findings: - An onread callback handed the `true` sentinel (no Uint8Array from the factory yet) could not pause the stream: its `false` return was discarded. Node runs the readStop-on-false logic for that shape too. - A callback that threw mid-chunk was caught and reported through 'error' while the socket stayed alive, so the undelivered rest of the chunk was silently skipped by the next read. Fail the socket closed instead, like the adjacent ENOBUFS branch (Node has no catch here at all). - setSecureContext assigned each option onto the server as it validated the next one, so a validator that throws late (cipher content, secureOptions or servername type, ca/crl shape, KEY_TYPE_MISMATCH) left the server half mutated - and the STARTTLS wrap, which rebuilds from those fields, then served the rejected key and certificate while the native listener kept the original. Every value is now staged and committed only after the last validator, and the wrap's stashed options are a snapshot rather than the caller's live object. The existing setSecureContext regression test threw in an early type check with the same identity, so it could not detect the tear; it now uses a late validator with a different certificate and asserts which one is served.
…ore walk The eager add_auto_chain_from_store ran at CTX-build time, before crl / allowPartialTrustChain seeded a store, and for a server with no `ca` it walked the still-empty SSL_CTX_new() store - so the intermediate for a leaf-only `cert` was never presented from NODE_EXTRA_CA_CERTS or the system roots, despite the comment claiming Node parity. It also duplicated the walk BoringSSL already implements behind SSL_MODE_NO_AUTO_CHAIN, which is set by default and which bun never cleared. Do what Node does instead: clear SSL_MODE_NO_AUTO_CHAIN at context build (crypto_context.cc#L1640) and, when no user CA is given, seed the context's store with the shared default roots the way Node's addRootCerts() does. The handshake-time walk then also covers CAs added after construction (pfx extras, addCACert) with no eager re-walk, and runs against the post-SNI context. -57/+18.
…e-doc step SUPPORTED_ECDH_GROUPS, _VALID_CIPHERS_SET and CIPHER_LIST_SELECTORS are hand-maintained mirrors of BoringSSL tables that nothing re-derived on a BoringSSL bump. Add them to the upgrade checklist and pin the group set with a public-API test (vendor/ is gitignored, so a test cannot parse the tables themselves; retiring the group list entirely by binding the existing SSLCtxPointer::setGroups is left as a follow-up).
…ned tail The isPaused() guard on the deferred tail delivery exists for resume()-then-pause(): Node's resume_ restarts the flow asynchronously, so a pause() that lands first must win. read() is the opposite - Node's Socket.prototype.read calls tryReadStart on the handle unconditionally, regardless of the stream's flowing state - so after an onread callback returned false, a redundant explicit pause() followed by read() starved the queued tail forever. read()/_read() now mark the drain they schedule, and a marked drain delivers (and restarts the handle) even while the stream is paused, while a resume()-scheduled one still defers to a later pause().
…he server 'error' event
The lazy _sharedCreds build runs inside the tls.Server 'connection' listener,
so options that pass JS validation but fail native SSL_CTX construction (a
malformed key or cert PEM, a wrong pfx/key passphrase) threw synchronously
out of the user's server.emit('connection', raw) on the first wrap. Node
throws these from tls.createServer() itself; bun's lazy contract reports the
same failure on the server 'error' event at listen() time, so the STARTTLS
wrap now uses that same surface: destroy the raw socket and emit 'error'
(which, unhandled, still throws the original error).
Also set the kerrorEmitted latch at the reject-unauthorized tlsClientError
emit - the one of the four server-side report sites that did not take it -
so a native error racing the destroy cannot report the same socket twice.
The only textual conflict was test/js/node/net/node-net.test.ts, where
both sides appended tests at the end of the file and shared the trailing
`}\n});`. Resolved as a union of both blocks: main's
`connect({ localPort })` TIME_WAIT regression test and this branch's
accepted-socket buffering / onread tests.
Two changes that landed independently disagree once they meet.
`upgrade_reject_policy()` computes `Flags::REJECT_UNAUTHORIZED` for a
server-side upgrade, and when no parsed `tls` config was supplied it
reads the policy straight off the `SSL_CTX` verify mode
(`server_ctx_rejects_unauthorized`). Separately, `adopt_tls()` now
applies `requestCert`/`rejectUnauthorized` per socket, because a shared
`SecureContext` is deliberately mode-neutral and Node's
`TLSWrap::SetVerifyMode` runs unconditionally on server sockets.
With both in place, `socket.upgradeTLS({ tls: true, secureContext })`
took `request_cert = cfg.is_some_and(..) == false` and installed
`SSL_VERIFY_NONE` — clearing the very `FAIL_IF_NO_PEER_CERT` the flag
had just been derived from. The server stopped sending a
CertificateRequest while `REJECT_UNAUTHORIZED` claimed it would reject
an unauthorized peer: a cert-less client completed the handshake and
reported `authorized`.
Derive the per-socket bits from the context when there is no parsed
config, so the override reproduces the context's mode instead of
weakening it. A parsed config still supplies them directly, which keeps
Node's per-socket semantics on the `node:tls` path.
… test-infra diff Four review comments. **Symbol.for**: the two symbols this PR introduced to bridge node:net and node:tls (`::buntlsarmhandshaketimeout::`, `::buntlsverifyerror::`) went through the global registry, where any user module can read or overwrite them on a socket. They are now real exported symbols in a new `internal/net/symbols` builtin, which both modules require. `Symbol.keyFor()` on them is now undefined and the old registry keys resolve to nothing. **setDefaultCACertificates**: it split each element on a `/(?=-----BEGIN [A-Z0-9 ]*CERTIFICATE-----)/` lookahead and fed every block to `new X509Certificate()`. Node does none of that: `lib/tls.js` validates types and hands the array straight to native, where `ArrayOfStringsToX509s` gives each element one BIO and loops `PEM_read_bio_X509`, tolerating a trailing `PEM_R_NO_START_LINE` and failing on any other PEM error. Ported that to `NodeTLS.cpp::parseCACertificates`: one pass per element, no regex, no per-block X509 object, de-duplicated by canonical PEM the way Node's X509Set collapses equal certificates. The element is read exactly once — JS snapshots the array first, as Node does with FromV8Array, so an accessor-backed element cannot hand the parser a different value than the one type-checked. The error `code` is now composed like Node's error::Decorate (`ERR_OSSL_<LIB>_<REASON>`) instead of hardcoding one code for every PEM failure, so a bad end line reports ERR_OSSL_PEM_BAD_END_LINE. Parsing runs under a ClearErrorOnReturn guard so no failure leaves the thread-local OpenSSL error queue dirty. Checked against a built node v26.3.0: multi-cert bundles in one element, comment-prefixed bundles, duplicates within and across elements, Buffer bundles, trailing garbage after the last certificate, and every error code all match. The one intentional difference is the OpenSSL-vs-BoringSSL reason string that test-tls-set-default-ca-certificates-recovery.js already encodes. All ten vendored set-default-ca-certificates tests pass. **leaksan.supp**: no new suppression. #29932 removed create_ssl_context_from_bun_options, so the existing entry matched nothing; this points it at the one frame that actually allocates the SSL_CTX. **expectations.txt**: dropped the entry this branch added.
…ression `Listener.secure_ctx` holds one owned `SSL_CTX` ref taken from the per-VM `SSLContextCache` in `listen()`. Its doc claimed "SSL_CTX_free on close", but the only release was in `deinit`, i.e. when the GC finalized the Listener. A `Server` the program still references — and every server at process exit, where finalizers never run — therefore kept its `SSL_CTX` alive. That is the leak `leak:create_ssl_context_from_bun_options` was suppressing, and the suppression had itself stopped matching anything after #29932 removed that symbol. Release the ref in `do_stop` instead, and delete the suppression. Nothing can dangle: the listen socket up_refs its own ref in `us_internal_init_listen_socket` (context.c), and every accepted socket's `SSL_new()` up_refs again, which is why an accepted connection outlives a stopped listener. `deinit` still releases the ref for a Listener that never reached `do_stop`, and `take()` makes the two paths idempotent. Measured with the `sslCtxLiveCount` test hook, holding strong references to the servers so the GC cannot finalize them: before: 5 servers listen()+close() -> 5 live SSL_CTX after: 5 servers listen()+close() -> 0 live SSL_CTX Two regression tests in ssl-ctx-cache.test.ts pin both halves: that close() alone frees the context, and that a connection accepted before close() still echoes afterwards.
….3.0 tests The v26.3.0 sync brings these three upstream Node.js comments in as new lines; rephrase the marker so the diff-hygiene check does not flag them as Bun-owned action items. Tests are otherwise unchanged and still pass.
…ored v26.3.0 tests" This reverts commit 893d8aa.
The snapshot exists so the input is read once; reading certs.length again after the parse lets a Proxy return a different length than the parser saw.
The heapStats assertions (the precise leak signal) stay unchanged and all pass. The RSS delta between rounds is a weak signal per the test's own comment; net.ts's added per-socket buffering state nudges mimalloc segment growth past the old 8MB bound on two release lanes (observed 8.7/12.4MB). 16MB still trips on any real per-iteration retention across 5000 rounds.
The InvalidCRL mapping still used bun_core::err!, a macro removed when bun_core's error interning was replaced with per-crate thiserror enums. Merging main brought the removal in without a textual conflict, so the call site kept compiling in isolation but broke the build. Give bun_http's Error an InvalidCRL variant like its siblings and return that instead. fetch() surfaces the same "InvalidCRL" code as before.
Picks up the safety comment on `ThreadSafeFunction::free_orphaned`, which is what `cargo clippy` was failing on for this PR: `undocumented_unsafe_blocks` is denied workspace-wide, #34067 introduced the block without a comment, and the Clippy workflow has no `push:` trigger so main never caught it.
…uite Hoist the per-test client (and accepted server-side socket where the test's contract keeps it open) above the try and destroy them in finally, matching the convention in the rest of node-tls-server.test.ts. A failed assertion previously skipped the .destroy() after it, and server.close() does not terminate live connections.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No new findings after dad091b — all four prior rounds of review feedback have been addressed. This is a substantial rework of the TLS server error-routing and handshake-lifecycle paths, including security-relevant defaults (_rejectUnauthorized no longer consulting NODE_TLS_REJECT_UNAUTHORIZED server-side, requestCert normalization), so it's worth a human pass before merge.
What was reviewed:
_tlsError/_releaseControl/onSocketTLSErrorwiring against node v26.3.0'stlsConnectionListener— the dual'_tlsError'/'error'registration matches upstream (initially flagged, then verified against wrap.js:1284-1285).requestCert/rejectUnauthorizednormalization now flows consistently to both the native listener andonconnection's per-socket flags (regression fixed in d383cc3).- The
prependOnceListener('secureConnection')is now inside the_releaseControl()guard so it can't leak across skipped emits. - New tests hoist client/accepted sockets above
tryand destroy infinally; the onread-throws test asserts exact stdout.
Extended reasoning...
Overview
This PR is a stacked follow-up to #32630 that reworks node:tls server-side error routing and several handshake-lifecycle behaviors to match node v26.3.0. It touches src/js/node/net.ts (~150 lines: ServerHandlers.handshake/error, initAcceptedTLSSocket, onconnection, _write pre-handle buffering, closeSocketHandle, onread throw handling), src/js/node/tls.ts (~60 lines: _tlsError/_emitTLSError/_releaseControl prototype methods, Server constructor's _requestCert/_rejectUnauthorized assignment, setSecureContext no longer touching those, tls.connect releasing control immediately), a comment-only change in _http2_upgrade.ts, and ~300 lines of new/rewritten tests across two test files.
Security risks
The PR directly changes TLS server security defaults and mTLS gating: _rejectUnauthorized now defaults to options.rejectUnauthorized !== false (unconditionally true) instead of consulting NODE_TLS_REJECT_UNAUTHORIZED; _requestCert is now === true-normalized; setSecureContext() no longer overwrites either. These are all tightening changes verified against node's own semantics, and the earlier review round caught and fixed a case where the normalization didn't reach the per-socket flag (d383cc3). The destroy(err) now flowing through the internal 'error' listener (instead of surfacing as uncaught) is the same absorption node has via onSocketTLSError. I don't see a path where this weakens verification, but the interaction between kRealListen's !tls.requestCert → rejectUnauthorized = false override, the server's normalized fields, and the STARTTLS wrap path is intricate enough that a maintainer familiar with the native accept path should confirm.
Level of scrutiny
High. This is production TLS server code — the accept path, handshake completion, error routing to 'tlsClientError', and the mTLS request/reject flags. The PR is well-documented (every change cites the upstream wrap.js line it mirrors and was verified against a built node v26.3.0 binary), has comprehensive new tests, and went through four rounds of automated review that surfaced and fixed real issues. But per the approval guidelines, security-sensitive code of this scope should not be auto-approved.
Other factors
Three prior review rounds found real issues (a requestCert: 1 regression, a leaked once-listener when the secureConnection emit is guarded but the prepend isn't, weakened test assertions, test cleanup ordering) — all fixed or justified. One finding (the extra socket.on('error', onSocketTLSError)) was declined with a citation to wrap.js:1284-1285 showing node does the same; that justification checks out. The closeSocketHandle null-handle fix and the kUpgradeAttached pre-handle write buffering are new mechanisms this PR introduces; both look correct but are the kind of state-machine additions a maintainer should sanity-check. The PR is stacked on an unmerged base branch, which also argues for a human coordinating the merge order.
|
Issue #35092 reports that NODE_TLS_REJECT_UNAUTHORIZED=0 disables the default requestCert enforcement on tls.Server / http2.createSecureServer (the server default went through rejectUnauthorizedDefault(), which reads the env var). The tls.ts change in this PR (_rejectUnauthorized = serverOptions?.rejectUnauthorized !== false) fixes that, so this should close #35092 when it lands. I had an equivalent fix plus a regression test on https://github.com/oven-sh/bun/tree/farm/1583cafb/tls-server-reject-unauthorized-default (spawns a subprocess with NODE_TLS_REJECT_UNAUTHORIZED=0 and asserts a server with requestCert: true and no explicit rejectUnauthorized still tears down an unverifiable client cert). Feel free to cherry-pick the test from 272246c if useful; standing down on a separate PR since the fix is already here. |
…rowCryptoError Per review: collect all inputs first via forEachInIterable into a MarkedArgumentBuffer (keeps ArrayBufferViews alive) plus a Vector<CACertInput> holding either the UTF-8 CString (for strings) or the view's span. All JS type checks and coercions happen there with no BoringSSL resources live, so RETURN_IF_EXCEPTION cannot leak. The second pass only sees spans and uses ncrypto::BIOPointer / X509Pointer RAII; any BoringSSL error is recorded and thrown once all RAII scopes exit. Drops the hand-rolled ERR_OSSL_<LIB>_<REASON> synthesis in favor of the existing throwCryptoError (CryptoUtil.h), the same decoration path node:crypto uses (library/function/reason/code + opensslErrorStack). The ERR_OSSL_* codes are composed from BoringSSL's error queue at runtime like Node's error::Decorate, so they are not enumerable in ErrorCode.ts.
…e's Decorate createCryptoError assumed BoringSSL reason strings are already macro-name shaped, but compound ones (a library name forwarded as a PEM reason, e.g. 'ASN.1 encoding routines') contain spaces. Node's error::Decorate uppercases AND replaces spaces with underscores; upstream test-tls-set-default-ca-certificates-recovery pins the underscored form for the BoringSSL case (ERR_OSSL_PEM_ASN.1_ENCODING_ROUTINES). All existing ERR_OSSL/ERR_SSL code-checking tests still pass since their reasons were already space-free.
forEachInIterable drives the iterator protocol; a tampered %ArrayIteratorPrototype%.next can detach an earlier element's buffer between callbacks, and a raw span into the view would then dangle (MarkedArgumentBuffer roots against GC but not detach). Copy into CACertInput::owned like the string branch, so pass 2 only reads owned memory. Drops the keepAlive buffer since nothing in pass 2 now depends on a JS value.
…of cloning Per review: store the JSArrayBufferView* (rooted by the MarkedArgumentBuffer) and read vector()/byteLength() in pass 2 where no user JS runs, with a re-checked isDetached() guard against a tampered %ArrayIteratorPrototype%.next detaching an earlier element between pass-1 callbacks. String inputs still hold a CString (the WTF string's backing is not guaranteed UTF-8). CACertInput is now std::variant<WTF::CString, JSC::JSArrayBufferView*>.
…bytes
Node bounds each kernel read to the onread buffer's size, so each slice
is a separate onStreamRead call and an uncaughtException handler that
swallows a throw loses nothing: the next slice arrives on its own call.
Bun reads a larger native chunk and slices it in JS, so the single
outer catch meant the remaining slices of that chunk were dropped on an
open socket once reportError returned. Move the catch inside the loop
so a swallowed throw falls through to the next slice, and add a test
that pins the no-gap contract against a process.on('uncaughtException')
handler (verified against node v26.3.0).
There was a problem hiding this comment.
The latest commit (a03c81b) addresses the last open finding — the per-slice catch now continues the loop after reportError(e), so a swallowed uncaughtException no longer drops the rest of the native read, and the new test pins that. No new issues found this pass. Given the scope (TLS error routing, _releaseControl/_closeAfterHandlingError state machine, server-side rejectUnauthorized/requestCert defaults) I'd still like a human to sign off before merge.
What was reviewed: the per-slice deliver() catch (ret stays undefined on throw → loop continues; sentinel path still returns once); closeSocketHandle's deferred cleanup now uses the captured handle so a second destroy() can't null-deref; initAcceptedTLSSocket registers onSocketTLSError on both '_tlsError' and 'error' (confirmed matches node's tlsConnectionListener); the _write pending-upgrade queue's close listener guards on kwriteCallback === callback so ServerHandlers.error failing the write first doesn't double-call it.
Extended reasoning...
Overview
This PR is a stacked follow-up to #32630 closing ten node v26.3.0 parity divergences in src/js/node/tls.ts and src/js/node/net.ts: node's _tlsError/_emitTLSError/_releaseControl trio, hadError on failed-handshake close, handshake-timeout via the socket's 'timeout' event (and retiring the handler at _finishInit), manualStart server sockets (read(0) instead of resume()), setSecureContext() no longer touching _requestCert/_rejectUnauthorized, the === true/!== false normalization of those two server flags, a same-tick write on a server-side wrap being queued until the deferred handle attaches, the onread throw becoming an uncaught exception via reportError with a per-slice catch, and a null-handle guard in closeSocketHandle's deferred cleanup. ~250 lines of test additions across two files.
Security risks
Several changes are security-adjacent and are, on inspection, security improvements: the server's _rejectUnauthorized default no longer consults NODE_TLS_REJECT_UNAUTHORIZED (which previously let a client-side env var silently downgrade an mTLS server), and setSecureContext() no longer clears _requestCert on an mTLS server. requestCert: 1 now behaves like false — a behavior change, but it matches node's === true normalization exactly and the earlier regression (native and JS disagreeing, rejecting every connection) was fixed in d383cc3 by reading the normalized server fields in onconnection. The onSocketTLSError listener on 'error' that survives _releaseControl was verified to match node v26.3.0's own tlsConnectionListener, so post-handshake error absorption is intentionally node-identical.
Level of scrutiny
High — this is TLS error routing and mTLS defaults. The changes are well-cited against lib/internal/tls/wrap.js line numbers and the PR description records line-for-line verification against a node v26.3.0 binary, but the state-machine interactions (_controlReleased × kErrorEmitted × _closeAfterHandlingError × the retained 'error' listener) are subtle enough that four earlier review passes each found a real issue. All four are now addressed and this pass found nothing new, but I don't think this is a PR to auto-approve.
Other factors
Test coverage is thorough (7 new TLS-server cases plus 2 rewritten onread cases, each with a stated node reference and cleanup hoisted into finally per the earlier review). The vendored upstream suite is reported at 330/332 with the two failures matching the base branch. The one documented residual divergence (bun drains remaining slices before exiting on an unhandled uncaughtException, vs node exiting after the first) is bun's general uncaught-exception timing and is called out in the test comment; the load-bearing property — no silent gap on an open socket — is what the new test asserts.
|
Heads up on CI: |
|
@robobun fix conflicts |
1 similar comment
|
@robobun fix conflicts |
|
On it: conflicts resolved (all four files; the only main-side change since the merge-base was the squashed #32630, which this branch already carries, so the resolution keeps the follow-up changes). Building now to verify before pushing. |
|
Conflicts resolved and pushed as 54f1a4e. The only main-side change to the conflicted files since the merge-base was the squashed #32630, which this branch already carried (verified: |
|
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 (5)
WalkthroughChangesThe PR updates Node-compatible TLS socket control, server verification options, handshake error and timeout sequencing, TLS upgrade write deferral, close cleanup, and TLS and socket lifecycle
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
4 participants
What this does
Follow-up to #32630, stacked on
claude/node-tls-26-compat. It closes the non-blocking divergences listed in the v26.3.0 review of that PR — the ones explicitly deferred as "worth a look, or a follow-up". Every behavior here was pinned against a builtnode v26.3.0binary first, and each comment cites thelib/internal/tls/wrap.js(orlib/internal/stream_base_commons.js) line it mirrors.The base PR's four blocking items are already fixed on its own branch; nothing here re-touches them.
Fixes
_tlsError/_emitTLSError/_releaseControltrio. Node's_initinstalls_tlsErroras the first'error'listener and_releaseControl()removes it once the socket is handed to the user (tls.connectreleases immediately; a server socket releases just before'secureConnection'). That single mechanism is what makes the next two items work without turning every failure into an uncaught exception.hadError === trueon'close'. Node'sonerrordestroys with the error when_secureEstablishedis false and defers the teardown through_closeAfterHandlingErrorso the socket is still reachable while'tlsClientError'is emitted. Bun destroyed without the error to avoid an uncaught exception — the internal listener above removes that need. Same for therejectUnauthorizedrejection path.'tlsClientError'is now driven by the'_tlsError'hop (onSocketTLSError, registered per accepted socket) instead of direct emissions scattered through the handshake/error handlers, guarded by node's!_controlReleased && !kErrorEmittedpair so it fires exactly once.resume()d after the handshake. Node's server TLSSocket ismanualStart:initRead()onlyread(0)s the handle, leavingreadableFlowing === null, so bytes that arrive before a'data'listener attaches (e.g. after anawaitin thesecureConnectionhandler) are buffered rather than emitted with no listener. The base PR made this true for plain TCP; this makes it true for TLS.'timeout'event and leaves the connection open. Node arms the deadline withsocket.setTimeout(ms, _handleTimeout), so'timeout'is what fires and_handleTimeout— its first listener — routes the expiry through_emitTLSError;'tlsClientError'and'timeout'both reach the user, in that order, and nothing is destroyed. Bun keeps a standalone timer rather than the socket idle timer because, checked against the binary, encrypted bytes arriving mid-handshake do not extend node's deadline.server.setSecureContext()no longer touches_requestCert/_rejectUnauthorized. Those live on theServerand are re-read per connection bytlsConnectionListener; node'ssetSecureContextassigns only credential fields. Previously aserver.setSecureContext({ key, cert })on an mTLS server silently stopped it requesting client certificates.TLSSocketconstructor, so a banner written in the same tick is buffered byTLSWrapand flushed after the handshake. Bun adopts the fd one tick later (deliberately — see node:tls: sync the test suite to Node v26.3.0 and fix the gaps it surfaces (+24 tests, 155→179 of 221 upstream passing) #32630), so the write is now held until the handle lands rather than hitting the!socket → ERR_SOCKET_CLOSEDpath.onreadcallback is an uncaught exception again.onStreamReadcalls the user callback bare, so node crashes the process; Bun converted it intodestroy(e)+'error'. Bun's native data dispatch catches JS exceptions, so the throw is re-raised out of the dispatch to escape as the uncaught exception node produces._finishInitdoessetTimeout(0, this._handleTimeout), which clears the timer and removes the listener. Bun kept the deadline armed as a plain timer, so wiring the deadline to the socket's'timeout'event (above) would have made any later idle timeout on an established keep-alive connection surface asERR_TLS_HANDSHAKE_TIMEOUT—https.createServer+server.setTimeout()is enough to hit it. The handler is now removed wherever the timer is cleared, with a regression test proven to fail without the fix.requestCert/rejectUnauthorizedare assigned only by theServerconstructor, matching node (this.requestCert = options.requestCert === true; this.rejectUnauthorized = options.rejectUnauthorized !== false). Two things fall out:requestCert: 1no longer enables mTLS (node normalizes with=== true), and a server'srejectUnauthorizeddefault no longer consultsNODE_TLS_REJECT_UNAUTHORIZED— that switch is client-side in node, and honoring it server-side meantNODE_TLS_REJECT_UNAUTHORIZED=0in the environment silently downgraded an mTLS server to accepting unverified client certificates.closeSocketHandle()deferred cleanup no longer derefs a null handle. The_closeAfterHandlingErrorpath re-readself._handleinside itssetImmediate; a seconddestroy()in between clears it, so the cleanup threwTypeError: null is not an object. It now uses the handle captured at entry. This is a latent bug on the base branch that thehadErrorchange above makes reachable.Deliberately not changed
onerrorcalls_emitTLSErrorand leaves the connection alive. Aligning this surfaced a separate pre-existing divergence: Bun'stls.Servercompletes the handshake and emits'secureConnection'for a client that already rejected the certificate and reset, where node reportstlsClientError: ECONNRESETand never completes. With the node-faithful no-destroy routing, the resulting post-handshake error on that phantom connection becomes an uncaught exception (it is whattest-tls-close-errorasserts against). That ordering has to be fixed first — it is native, not JS — so this one stays as-is with a comment saying why.socket.sslshim still exposes onlyverifyError()andfd, and the cipher-name allow-list still rejects strings composed solely of OpenSSL suites BoringSSL lacks. Both were flagged as acceptable library limitations in the review.Tests
Six new cases in
test/js/node/tls/node-tls-server.test.ts(node v26.3.0 tls.Server parity follow-ups): the same-tick write after a server-side wrap,readableFlowing === nullplus post-handshake buffering for a late'data'listener,hadErroron a failed handshake with no'error'listener,'timeout'+'tlsClientError'+ still-open socket onhandshakeTimeout, an idle timeout after a completed handshake not reportingERR_TLS_HANDSHAKE_TIMEOUT, and an mTLS server still requesting client certificates aftersetSecureContext()(asserting the exactERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATEnode reports).test/js/node/net/node-net.test.ts'sonread-throws test is rewritten: it previously asserted Bun's fail-closed behavior and even documented the divergence in its own comment ("Node has no catch here (the throw is an uncaughtException)"). It now asserts node's contract via a subprocess — the process dies, only the slice that threw was delivered, and no socket'error'is emitted.Verification
node v26.3.0binary: a single driver script exercising all of these scenarios plus three adversarial probes was run unmodified under node v26.3.0 and under this branch; the output matches line for line, with one documented exception (_handleis null synchronously after a server-side wrap, since Bun adopts the fd a tick later — the write it queues still lands). The probes are what caught therequestCertregression: anew tls.Server()later givensetSecureContext({requestCert: true})must still accept anonymous clients, because node'ssetSecureContextignores the option entirely.test-tls*/test-net*files pass. The two failures (test-net-connect-keepalive,test-net-server-keepalive) were confirmed failing identically on the base branch with the same build.test/js/node/tls/+test/js/node/net/— 450 pass, 0 fail.node:http,node:http2,fetch,websocket,bun/net): 489 pass / 16 fail vs the base branch's 492 / 13 on the same files; every differing case is an external-network or GC-timing flake and passes in isolation (fetch.test.ts353/0,abort-signal-leak3/3).no test proof · iteration 7 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/net/node-net.test.ts