node:tls: honor setServername() called before connect()

[robobun]

Problem

• setServername(name) on a client tls.TLSSocket that is not connected yet has no effect: the handshake carries no SNI and socket.servername is "" afterwards. Node v26.3.0 sends name and reports it as socket.servername once the handshake is done.
• TLSSocket.prototype.setServername (src/js/node/tls.ts:1032) has no handle to apply the name to at that point, so all it can do is store it in this.servername.
• Socket.prototype.connect (src/js/node/net.ts:1905) begins with this.servername = options.servername (undefined unless the caller passed one) and only then calls TLSSocket.prototype[buntls] (tls.ts:1111), which reads this.servername to build the native connect options. The stored name is gone by then and [buntls] falls back to the secure context's servername or the host-derived default.
• Every connect form loses it (host/port, autoSelectFamily, connect({ socket })). tls.connect({ servername }) is unaffected because it passes the name through the connect options.

Fix

• setServername() also records the name in a per-socket slot (kservername, the counterpart of the ksession slot setSession() uses), and [buntls] reads that slot when this.servername is empty, ahead of the secure context's servername and the host-derived default. net.ts is unchanged.
• Why a second slot rather than not resetting this.servername: this.servername is also the working value of the connect() in progress. net.ts:1905 seeds it from the options, net.ts:1962 stores the name [buntls] picked (possibly derived from the host), and internalConnect calls [buntls] again and must get the same answer. It therefore has to be reset on every connect(), otherwise a name derived from one host would be sent on a reconnect to a different host. The name the caller set explicitly is the one thing that should outlive that reset, so it gets its own field.
• Precedence is unchanged, the most recent instruction wins: a servername given to connect() itself is the later call and still takes priority (net.ts:1905 puts it in this.servername), and a setServername() made after tls.connect() returned still overrides the options as before, since it writes both fields. (Node's net.Socket#connect ignores a servername option entirely; honoring it is bun's own extension, which bun's tls.connect() is built on.)
• socket.servername ends up as the set name through the existing net.ts:1962 assignment, which is what Node reports from getServername() after the handshake.
• Tests: test/js/node/tls/node-tls-connect.test.ts, describe("setServername() before connect()"). A local server records the SNI of each connection and the client reports socket.servername. Three cases fail without the fix: host/port connect and connect({ socket }) (received "" on the client and no SNI on the server) and an autoSelectFamily connect, which goes through the per-address attempt path and used to send the host-derived name. Two controls pass before and after and pin the constraints above: a servername in the connect options takes precedence, and a host-derived name is recomputed on a reconnect to another host.
• Also run with the fix: the rest of test/js/node/tls (its two failures, node-tls-server.test.ts "SNICallback runs even when the requested servername matches the bind hostname" and node-tls-root-certs-concurrent-init.test.ts, fail identically without this change in this environment: localhost resolution, and a 5s timeout under the debug build), plus 23 vendored node tests (test-tls-sni-*, test-tls-error-servername, test-tls-ip-servername-forbidden, test-https-agent-servername, test-tls-connect-*, test-tls-client-resume*, test-tls-socket-*, test-http2-connect*), all passing. The script from the report prints the same as node.

Background

• SNI (server name indication) is the host name a TLS client puts in its ClientHello so that a server holding several certificates can pick one. tls.Server exposes the received name as socket.servername in its connection listener, which is how the tests observe what the client sent.
• In bun a client TLSSocket gets its native handle only when connect() runs. Socket.prototype.connect (net.ts) calls the [buntls] method TLSSocket defines (tls.ts) to turn the socket's TLS state (secure context, ALPN, session, servername, ...) into the options object handed to the native connect or upgrade. Node's client TLSSocket creates its handle in the constructor instead, so setServername() there applies to the SSL object on the spot and nothing needs to be carried over to connect time.
• ksession is the existing per-socket slot that setSession() and the constructor store the TLS session in; [buntls] copies it into the options the same way it now consults kservername.

Repro (from the report) before and after

const tls = require("node:tls");
const server = tls.createServer({ cert, key }, s => { console.log("server saw:", s.servername); s.end(); });
server.listen(0, "127.0.0.1", () => {
  const t = new tls.TLSSocket(undefined, { rejectUnauthorized: false });
  t.setServername("set.before.connect");
  t.connect(server.address().port, "127.0.0.1");
  t.on("secure", () => console.log("client:", JSON.stringify(t.servername)));
  t.on("close", () => server.close());
});

node v26.3.0:   server saw: set.before.connect   client: "set.before.connect"
bun (main):     server saw: undefined            client: ""
bun (this PR):  server saw: set.before.connect   client: "set.before.connect"

[robobun]

^{Updated 11:55 AM PT - Aug 15th, 2026}

❌ @robobun, your commit 9ac01ec has some failures in Build #98075 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 39040

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

bun-39040 --bun

[robobun]

Status: fix pushed, waiting for CI.

Reproduced on the release build (USE_SYSTEM_BUN=1 bun test test/js/node/tls/node-tls-connect.test.ts -t "setServername() before connect()"): the host/port, autoSelectFamily and connect({ socket }) cases fail (client reports "" or the host-derived name, server sees no SNI); the same three pass with this branch's debug build, as does the whole file.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 7 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: cab47bb3-fc2d-4ae3-b653-9ed184df7da1

📥 Commits

Reviewing files that changed from the base of the PR and between 88a6398 and 9ac01ec.

📒 Files selected for processing (2)

• src/js/node/tls.ts
• test/js/node/tls/node-tls-connect.test.ts

---

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

[claude]

LGTM — small Node-compat fix that mirrors the existing ksession pattern.

What was reviewed:

• Precedence chain in [buntls]: connect({servername}) still wins (via this.servername at net.ts:1905), setServername() now falls in ahead of ctx?.servername and the host-derived default — pinned by the two control tests.
• Traced the three bunTLS.$call sites in net.ts (1955, 3095, 3243): the later autoSelectFamily calls read this.servername after net.ts:1962 stored the first call's result, so they stay consistent.
• Checked that setServername("") behavior is unchanged (falsy in both old and new fallback chains) and that no verification/rejectUnauthorized path is touched.

Extended reasoning...

Overview

Adds a private kservername symbol slot on TLSSocket that setServername() writes alongside this.servername, and inserts it into the [buntls] servername fallback chain (this.servername || this[kservername] || ctx?.servername). Four substantive lines in src/js/node/tls.ts plus a five-case describe.concurrent block in test/js/node/tls/node-tls-connect.test.ts. net.ts is untouched.

Security risks

SNI selects which certificate a server presents, but this change only preserves a name the caller explicitly set via setServername(); it never weakens or bypasses verification, rejectUnauthorized, or checkServerIdentity. The precedence order keeps connect({servername}) winning (it's the later instruction) and keeps the host-derived default recomputed per connect (the reconnect control test pins this). No security-relevant regression.

Level of scrutiny

Low-to-medium. The fix is a mechanical carry-over of state that Node applies to the handle at setServername() time but Bun can't (no handle yet). It's an exact copy of the ksession slot pattern already used for setSession() two lines above. The PR description explains why a second slot is needed rather than not resetting this.servername (host-derived names must not stick across reconnects), and a test enforces that constraint.

Other factors

• Tests are hermetic (local tls.createServer on 127.0.0.1, custom lookup for the hostname cases), use await using for cleanup, once() for events, and describe.concurrent.
• Three tests cover the fixed paths (host/port, autoSelectFamily, connect({socket}) upgrade); two control tests pin precedence and reconnect behavior so the fix can't regress into the alternative designs the description rules out.
• Author verified with USE_SYSTEM_BUN=1 that the three fix tests fail on the release build and pass on the debug build, and ran the wider tls suite plus 23 vendored Node tests.
• No CODEOWNERS on this path; no prior human review comments to address.

[Jarred-Sumner]

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.