node:tls: make client-side new TLSSocket(socket) upgrade the socket it wraps

[robobun]

Problem

• Client-side STARTTLS, wrapping an already connected socket with new tls.TLSSocket(raw), is inert on Bun 1.4.0 and main: the first write throws TypeError: socket.@write is not a function, later writes return true and are dropped, and no handshake starts. Node handshakes and passes data.
• On the same wrap, _start() (what the mysql driver calls) throws ERR_MISSING_ARGS and destroy() fails with handle.close is not a function.
• Cause: the client branch of the constructor kept the wrapped JS socket as the TLS socket's native handle and nothing ever replaced it. Only the server wrap and tls.connect({ socket }) had an upgrade path.
• Two more gaps when the wrapped thing is a generic Duplex (also via tls.connect({ socket: duplex })): its 'error' had no listener, so it became an uncaught exception and 'close' never fired; and a throwing write() reached the error handler as an exception cell, not the thrown value (ASSERTION FAILED: isSymbol() on debug, TypeError: Cannot convert a symbol to a string on release).

Fix

• The client wrap now runs the same upgrade as tls.connect({ socket }), so the handshake starts in the constructor and _start() is a no-op. Check: after construction _handle is never the stream passed in, and _handle._parentWrap (read by http2-wrapper, so by got, at import) is still set.
• The wrap gets only Node's _finishInit half of completion: 'secure' fires and the verdict lands in authorized / ssl.verifyError(); no hostname check, no 'secureConnect'. Deliberate differences from Node: rejectUnauthorized: true is still enforced, and the ClientHello goes out at construction, so servername / session must be constructor options.
• Every upgrade path now turns the stream's 'error' into a destroy of the TLS socket, the native engine passes the thrown value (same hunk as node:tls: pass the thrown Error, not the internal exception wrapper, to 'error' over a Duplex transport #36909, byte-identical), and destroying a wrap before its socket connects destroys that socket too. Pre-handshake writes over a generic Duplex are still lost; pre-existing, tracked separately.
• Verification: 13 new tests. All but the http2-wrapper probe fail without the change (the throwing write() case aborts a debug build); all pass with it. Vendored Node tls / https / net / http-upgrade tests pass; two test/js/node/tls/ tests fail identically on main in this environment.

Background

• STARTTLS: a connection starts in plaintext and switches to TLS in place. In Node the client does this with new tls.TLSSocket(existingSocket), optionally followed by _start(); tls.connect() is the other entry point and adds client checks on top.
• Upgrade: in Bun's node:net a socket's _handle must be a native handle, since _write and _destroy call into it. Making an existing socket speak TLS means the native side takes the connection over (adopting the fd, or driving a generic Duplex through UpgradedDuplex) and hands back a TLS handle to install as _handle.
• Node splits client handshake completion in two: _finishInit (emit 'secure', record the verdict), which every TLSSocket gets, and onConnectSecure (hostname check, rejectUnauthorized, 'secureConnect'), which only tls.connect() installs.
• take_exception vs take_error in the JSC bindings: the first yields the engine's exception wrapper cell, the second the value JS threw. The JS error handlers expect the latter.
• http2-wrapper evaluates new TLSSocket(new PassThrough())._handle._parentWrap.constructor at import time, so a client wrap must expose _handle._parentWrap synchronously and then fail quietly.

Original description

Reproduction

The STARTTLS client shape: wrap a connected socket yourself instead of calling tls.connect({ socket }).

import net from "node:net"; import tls from "node:tls";
const srv = tls.createServer({ key, cert }, s => s.on("data", d => s.write("echo:" + d))).listen(0, "127.0.0.1", () => {
  const raw = net.connect(srv.address().port, "127.0.0.1", () => {
    const t = new tls.TLSSocket(raw, { isServer: false, rejectUnauthorized: false });
    t.on("secure", () => console.log("secure", t.getProtocol()));
    t.on("data", d => console.log("data", String(d)));
    setInterval(() => console.log("write ->", t.write("tick")), 100);
  });
});

Node v26.3.0: write -> true, secure TLSv1.3, data echo:tick per tick.

Bun 1.4.0 (and main): the first write throws

TypeError: socket.@write is not a function. (In 'socket.@write(chunk, encoding)', 'socket.@write' is undefined)

every later write returns true and is dropped, no handshake ever starts (raw.bytesWritten stays 0, no secure/error/close), t._start() (what the mysql driver calls after constructing the wrap) throws ERR_MISSING_ARGS, and t.destroy() fails with handle.close is not a function.

Cause

The client branch of the TLSSocket constructor stored the wrapped net.Socket itself as this._handle and nothing ever upgraded it, so net.Socket's _write/_destroy called native handle methods on a JS socket. Only the server wrap (isServer: true) and tls.connect({ socket }) have upgrade paths. _start() called connect() with no arguments, which throws.

Fix

• tls.ts: the client branch now drives the same upgrade tls.connect({ socket }) uses, through a new net.Socket.prototype[kUpgradeClientTLS] (the counterpart of the existing server-wrap method), so the handshake starts in the constructor and _handle is a TLS handle. The _handle._parentWrap that http2-wrapper (and therefore got) reads at import time is kept. _start() is a no-op: the handshake is already running. The socket: this._handle field of [buntls] and the tls.socket fallbacks in the three connect paths existed only for the old _start() and are removed.
• net.ts onClientHandshakeComplete: a constructor wrap gets Node's _finishInit semantics, since Node installs onConnectSecure from tls.connect() only: 'secure' is emitted, the chain verdict lands in authorized/authorizationError/ssl.verifyError(), there is no hostname check against a connect() host the wrap does not have, and no 'secureConnect'. rejectUnauthorized: true still refuses an unverifiable peer, reported once through the destroy (the standalone server wrap already behaves this way). Node itself verifies nothing on a constructor wrap; the mysql driver's STARTTLS, the main user of this API, reads ssl.verifyError() from its 'secure' listener and works with either.
• Wrapping a generic stream reached two gaps in the shared duplex path (also reachable through tls.connect({ socket: duplex }) before this change):
  • the stream's own 'error' had no listener, so it surfaced as an uncaught exception and the stream's 'close' was never emitted. Both upgrade paths now attach the four engine listeners plus an error forwarder (listenToUpgradedDuplex), destroying the TLS socket with the stream's error, which is where Node routes it too.
  • UpgradedDuplex::call_write_or_end passed take_exception(), the JSC::Exception cell, to the error handler; JS then read properties off it (ASSERTION FAILED: isSymbol() in JSValue::synthesizePrototype in debug builds, TypeError: Cannot convert a symbol to a string in release). It now passes the thrown value with take_error(), like the other handler sites. This is the same three-line hunk as node:tls: pass the thrown Error, not the internal exception wrapper, to 'error' over a Duplex transport #36909 (kept byte-identical so either lands first without conflict); it is carried here because the client wrap makes the vendored test-tls-socket-allow-half-open-option reach it. node:tls: pass the thrown Error, not the internal exception wrapper, to 'error' over a Duplex transport #36909 additionally fixes the on_data conversion site and has its own test.
• Destroying a wrap whose socket has not connected yet destroys that socket as well, as closing Node's JSStreamSocket does; the hook is removed once the upgrade takes over.

Deliberate differences from Node, for review: the ClientHello is sent at construction rather than on _start()/first write, so setServername()/setSession() between construction and _start() are too late (pass servername/session in the constructor options instead); and rejectUnauthorized: true is enforced on the wrap. Writes before the handshake are delivered on the normal (fd adopting) path; over a generic Duplex they are lost, which is pre-existing behaviour of tls.connect({ socket: duplex }) and tracked separately, so the duplex test here writes after 'secure'.

Overlaps: #32824 makes _start() perform the upgrade lazily (this change starts it in the constructor instead, so _start() and a write without _start() both work); #36909 carries the identical UpgradedDuplex hunk (above); #35842 adds a closeSocketHandle fallback for the same root cause (with this change _handle is never a plain stream, and its two scenarios are covered by the destroy tests here).

Verification

13 new tests in test/js/node/tls/node-tls-connect.test.ts (write after wrap over TCP, _start(), rejectUnauthorized rejection, chain-but-not-hostname versus tls.connect({ socket }), wrap over an in-memory Duplex pair, the http2-wrapper probe exiting quietly, destroy before connect for net.Socket and Duplex, and stream errors / throwing write() for the client wrap, the server wrap and tls.connect({ socket })). Without the change every one of them except the probe test (which guards existing behaviour) fails, and the throwing write() case aborts a debug build on the assertion above; all pass with the change.

test/js/node/tls/ passes apart from two tests that fail identically on main in this environment (SNICallback ... bind hostname, which needs localhost to resolve consistently, and the root-certificate Worker race hitting its 5s timeout on a debug build). The vendored test-tls-*, test-https-*, test-net-* and test-http-upgrade* parallel tests pass, including test-tls-socket-allow-half-open-option (which wraps a write-less Duplex on the client side and previously only passed because the wrap was inert), test-http-upgrade-reconsume-stream and test-tls-transport-destroy-after-own-gc.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 21338a86-2a93-43fa-a0db-c2f23b859489

📥 Commits

Reviewing files that changed from the base of the PR and between 0e5d9df and 0b3d4b9.

📒 Files selected for processing (5)

• src/js/internal/net/symbols.ts
• src/js/node/net.ts
• src/js/node/tls.ts
• src/runtime/socket/UpgradedDuplex.rs
• test/js/node/tls/node-tls-connect.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 4:50 PM PT - Aug 11th, 2026}

✅ @robobun, your commit 0b3d4b900139ca4da05e9dc761979cda34941f05 passed in Build #92587! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37664

That installs a local version of the PR into your bun-37664 executable, so you can run:

bun-37664 --bun

[claude]

I reviewed this PR and the bug hunting system found no issues. Because it reworks a security-sensitive TLS path (client-side new TLSSocket(socket) upgrade, handshake-complete verification semantics), explicitly flags deliberate Node divergences for review, and overlaps with #32824 / #35842, a human should sign off on the design choices.

What was reviewed:

• The new kUpgradeClientTLS path reuses the existing connect({ socket }) upgrade; kStandaloneWrap correctly gates checkServerIdentity/'secureConnect' while keeping the rejectUnauthorized destroy.
• Removed tls.socket / connection fallbacks in internalConnect/internalConnectMultiple were dead once _start() became a no-op — no other reader.
• take_exception → take_error in UpgradedDuplex.rs matches the other on_error sites in the file.
• listenToUpgradedDuplex replaces four identical listener blocks; the added 'error' forwarder and pre-connect destroyConnection hook are removed/superseded on the paths that take ownership.

Extended reasoning...

Overview

This PR makes client-side new tls.TLSSocket(socket) actually upgrade the wrapped socket (previously it stored the raw net.Socket as _handle and never handshaked). It touches src/js/node/tls.ts (constructor branch, _start(), [buntls]), src/js/node/net.ts (new kUpgradeClientTLS prototype method, kStandaloneWrap flag in onClientHandshakeComplete, listenToUpgradedDuplex helper, pre-connect destroy hook, dead-code removal in three connect paths), src/runtime/socket/UpgradedDuplex.rs (three take_exception → take_error), a new symbol, and 13 new tests.

Security risks

This is squarely in the TLS verification path. The kStandaloneWrap flag suppresses checkServerIdentity and 'secureConnect' for constructor wraps — the PR argues this matches Node's _finishInit-only semantics (no onConnectSecure) and cites the source lines, and it still enforces rejectUnauthorized on the chain verdict. The author also notes Bun is stricter than Node here (Node verifies nothing on a constructor wrap). That reasoning looks sound and is well-tested, but changing which verification steps run for a TLS client API is exactly the kind of decision a maintainer should confirm.

Level of scrutiny

High. TLS handshake/verification semantics, a Node-compat API used by real drivers (mysql, http2-wrapper/got), explicit "deliberate differences from Node, for review" in the description, and overlap with two other open PRs that a maintainer needs to reconcile.

Other factors

The change is thoroughly tested (13 new cases covering the fd-adopting wrap, duplex wrap, _start(), rejectUnauthorized, chain-vs-hostname, http2-wrapper probe, destroy-before-connect, and stream-error/throwing-write for all three wrap flavours), the dead-code removals are justified in the description, and the Rust change is a straightforward correctness fix matching sibling call sites. Nothing blocks on correctness from what I can see; the deferral is for the design/compat trade-offs the author explicitly surfaced.

[github-actions]

This PR may be a duplicate of:

1. node:tls: fix getCipher().version, getEphemeralKeyInfo(), client STARTTLS, and checkServerIdentity exception handling #32824 - Fixes the same client STARTTLS bug (new tls.TLSSocket(socket) + _start() throwing ERR_MISSING_ARGS) by making _start() lazily call connect({ socket }), where this PR performs the upgrade in the constructor instead — competing fixes to the same code path in tls.ts/net.ts.
2. node:tls: pass the thrown Error, not the internal exception wrapper, to 'error' over a Duplex transport #36909 - Contains the identical take_exception() → take_error() change at the same three UpgradedDuplex::call_write_or_end sites, fixing the same Cannot convert a symbol to a string / ASSERTION FAILED: isSymbol() failure when a wrapped Duplex's write() throws.
3. node:tls: handle destroy() on a TLSSocket wrapping an unconnected stream #35842 - Addresses the same root cause from the other end, adding a closeSocketHandle fallback for handle.close is not a function when destroying a TLSSocket whose _handle is the wrapped stream — a case this PR removes entirely by never storing a plain stream as _handle.

🤖 Generated with Claude Code

[robobun]

On the three PRs the duplicate check lists:

• node:tls: fix getCipher().version, getEphemeralKeyInfo(), client STARTTLS, and checkServerIdentity exception handling #32824 fixes one of the symptoms (_start() throwing) by making _start() run the upgrade lazily. This PR starts the upgrade in the constructor instead, which also covers the case that PR leaves broken: a wrap that is written to without calling _start(). The two _start() hunks conflict; the rest of node:tls: fix getCipher().version, getEphemeralKeyInfo(), client STARTTLS, and checkServerIdentity exception handling #32824 is unrelated to this change.
• node:tls: pass the thrown Error, not the internal exception wrapper, to 'error' over a Duplex transport #36909 is the same UpgradedDuplex.rs fix. The hunk here is now byte-identical to it (ba7858c), so whichever lands first the other merges cleanly; it is carried here because the client wrap makes the vendored test-tls-socket-allow-half-open-option reach that crash. node:tls: pass the thrown Error, not the internal exception wrapper, to 'error' over a Duplex transport #36909 additionally fixes the on_data site and adds a dedicated test, so it is still worth landing on its own.
• node:tls: handle destroy() on a TLSSocket wrapping an unconnected stream #35842 works around the same root cause in closeSocketHandle; with this change _handle is never a plain stream, and its two scenarios are covered by the destroy tests added here.

The comment-length review threads are addressed in 0b3d4b9 (each new comment is now a single line, plus a Node source link where the neighbouring code does the same).

[claude]

I reviewed this PR and the automated bug-hunt found no issues. This still needs a human look: it reworks the client-side TLS upgrade path and handshake-complete semantics (skipping hostname verification for constructor wraps), and the description explicitly flags deliberate Node divergences and three overlapping PRs (#32824, #36909, #35842) for a maintainer to reconcile.

What was reviewed:

• kStandaloneWrap gating in onClientHandshakeComplete — chain verification and rejectUnauthorized still run; only checkServerIdentity and 'secureConnect' are skipped, matching Node's _finishInit-without-onConnectSecure.
• The removed tls.socket / connection fallbacks in internalConnect / internalConnectMultiple — the locals were write-only after the [buntls] change, so removal is dead-code cleanup.
• listenToUpgradedDuplex applied to all four duplex-attach sites; the new error forwarder guards on !this.destroyed.

Extended reasoning...

Overview

This PR makes client-side new tls.TLSSocket(socket) actually upgrade the wrapped socket by routing it through the same connect({ socket }) path tls.connect uses, via a new Socket.prototype[kUpgradeClientTLS]. It touches src/js/node/tls.ts (constructor branch, _start() → no-op, [buntls] no longer returns socket), src/js/node/net.ts (onClientHandshakeComplete gains a kStandaloneWrap branch, four duplex-listener attach sites factored into listenToUpgradedDuplex with an added 'error' forwarder, a destroy-before-connect hook, and dead connection/tls.socket locals removed from internalConnect/internalConnectMultiple), a three-line take_exception → take_error fix in UpgradedDuplex.rs, and 13 new tests.

Security risks

The change is squarely in TLS verification territory. When kStandaloneWrap is set, onClientHandshakeComplete skips checkServerIdentity and the 'secureConnect' emit. The PR argues (with Node source citations) that this matches Node's behaviour for a constructor wrap, which never installs onConnectSecure, and it keeps rejectUnauthorized enforcement (stricter than Node). The chain verdict still lands in authorized/authorizationError/ssl.verifyError(). That reasoning looks correct against the cited Node source, but skipping hostname verification on a client TLS path is exactly the kind of decision REVIEW.md says a maintainer should sign off on.

Level of scrutiny

High. This is production TLS handshake/verification logic with an intentional Node divergence (ClientHello at construction rather than _start(), so setServername()/setSession() between construction and _start() are too late) that the author explicitly flags "for review". There are also three competing/overlapping open PRs whose reconciliation is a maintainer call.

Other factors

The bug-hunting system raised and ruled out one candidate (whether kStandaloneWrap needs a constructor-time default — it does not, the symbol read is undefined-safe). Test coverage is thorough (13 new cases including the negative contract for 'secureConnect', the duplex error/throw paths, and the http2-wrapper probe). All comment-cop threads are resolved. Given the security surface and the design questions the author raises for review, deferring to a human is the right call.

[robobun]

Cross-reference: the fifth upgradeDuplexToTLS site, Http2SecureServer#emit('connection', rawSocket) in _http2_upgrade.ts, has the same missing 'error' listener (rawSocket.destroy(err) after a request is an uncaught exception; node reports it as the server's 'sessionError'). Since listenToUpgradedDuplex here covers the four net.ts sites, #38124 only adds it at that site, so the two do not overlap.

Two more variants of the stream-error case that might be worth covering here, checked against node v26.3.0 with duplexPair(): a transport destroyed with an error after the handshake (node: tls.connect({ socket }) emits 'error' then 'close'; a standalone server wrap emits '_tlsError' then 'close'), and TLS inside TLS (tls.connect({ socket: outerTlsSocket }) and new TLSSocket(acceptedTlsSocket, { isServer: true })), where the outer socket's error has to land on the inner socket. On current main all of these are uncaught exceptions.