tls: fix tls.connect skipping hostname verification when servername omitted#29096
tls: fix tls.connect skipping hostname verification when servername omitted#29096Jarred-Sumner merged 3 commits intomainfrom
Conversation
…mitted
TLSSocket[buntls] returned the host fallback under camelCase 'serverName' but net.ts read lowercase 'servername', so checkServerIdentity was never called for tls.connect({host, port}) without an explicit servername. Any CA-trusted cert was accepted regardless of hostname.
|
Updated 3:03 PM PT - Apr 10th, 2026
❌ @Jarred-Sumner, your commit 63033b2 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29096That installs a local version of the PR into your bun-29096 --bun |
WalkthroughTLS servername selection and validation were tightened: secure-context Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/tls/node-tls-connect-hostname-verification.test.ts`:
- Around line 6-7: The test file is importing the Node test runner; replace the
import of describe and test from "node:test" with "bun:test" so the file uses
Bun's test runner (update the import statement that currently reads import {
describe, test } from "node:test" to import from "bun:test"); keep the tls
import as-is and run the same describe/test usage with Bun.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60277047-3fee-4ddd-b3e1-c296af07c4cf
📒 Files selected for processing (5)
src/js/node/tls.tstest/js/node/tls/node-tls-cert-extra-ca.fixture.tstest/js/node/tls/node-tls-cert.test.tstest/js/node/tls/node-tls-connect-hostname-verification.test.tstest/js/node/tls/node-tls-connect.test.ts
| import { describe, test } from "node:test"; | ||
| import tls from "node:tls"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file structure and content
cat -n test/js/node/tls/node-tls-connect-hostname-verification.test.ts | head -30Repository: oven-sh/bun
Length of output: 1540
🏁 Script executed:
# Check other test files in test/js/node/ to see patterns
fd -t f "\.test\.(ts|js)" test/js/node/ | head -20 | xargs -I {} sh -c 'echo "=== {} ===" && head -10 {} | grep -E "import.*from.*test|import.*from.*bun:test|import.*from.*node:test"'Repository: oven-sh/bun
Length of output: 1951
🏁 Script executed:
# Check if there's any configuration or documentation about test/js/node/
find test/js/node -name "*.md" -o -name "README*" | xargs cat 2>/dev/null || echo "No docs found"Repository: oven-sh/bun
Length of output: 50369
🏁 Script executed:
# Check how many test files in test/js/node/ use node:test vs bun:test
echo "=== Using node:test ===" && \
rg "import.*from.*['\"]node:test['\"]" test/js/node/ --type ts --type js | wc -l && \
echo "=== Using bun:test ===" && \
rg "import.*from.*['\"]bun:test['\"]" test/js/node/ --type ts --type js | wc -lRepository: oven-sh/bun
Length of output: 107
🏁 Script executed:
# Check the buffer-indexOf-detach.test.ts file to see context
head -20 test/js/node/buffer-indexOf-detach.test.tsRepository: oven-sh/bun
Length of output: 933
🏁 Script executed:
# Look for any CLAUDE.md or documentation in test/js/node/tls/
find test/js/node/tls -type f \( -name "*.md" -o -name "CLAUDE*" \) 2>/dev/nullRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check what other TLS tests use
fd -t f "\.test\.(ts|js)" test/js/node/tls/ | xargs -I {} sh -c 'echo "=== {} ===" && head -10 {} | grep -E "import.*from.*test|import.*from.*bun:test|import.*from.*node:test"'Repository: oven-sh/bun
Length of output: 1743
🏁 Script executed:
# Check if there's a pattern - look at other files in test/js/node/ that use node:test
fd -t f "\.test\.(ts|js)" test/js/node/ -exec grep -l "from.*['\"]node:test['\"]" {} \;Repository: oven-sh/bun
Length of output: 272
🏁 Script executed:
# Let me check the full content of the file to understand if there's a compatibility verification reason
cat test/js/node/tls/node-tls-connect-hostname-verification.test.tsRepository: oven-sh/bun
Length of output: 3625
Use bun:test in this *.test.ts file.
All test files matching test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} must use bun:test as the test runner. This applies to this file in test/js/node/tls/.
♻️ Proposed fix
-import { describe, test } from "node:test";
+import { describe, test } from "bun:test";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { describe, test } from "node:test"; | |
| import tls from "node:tls"; | |
| import { describe, test } from "bun:test"; | |
| import tls from "node:tls"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/tls/node-tls-connect-hostname-verification.test.ts` around lines
6 - 7, The test file is importing the Node test runner; replace the import of
describe and test from "node:test" with "bun:test" so the file uses Bun's test
runner (update the import statement that currently reads import { describe, test
} from "node:test" to import from "bun:test"); keep the tls import as-is and run
the same describe/test usage with Bun.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
This fixes a genuine TLS security vulnerability and is well-explained and tested, but given it touches security-sensitive hostname verification code, a human should confirm the fix is complete and that no camelCase serverName consumers remain.
Extended reasoning...
Overview
This PR fixes a security bug in TLSSocket.prototype[buntls] (src/js/node/tls.ts) where the host fallback was emitted under the camelCase key serverName but consumed as lowercase servername in net.ts, causing checkServerIdentity to be skipped and authorized to be incorrectly set to true. The fix is minimal: extract the SecureContext into ctx, spread it, then emit servername (lowercase) with the fallback chain this.servername || ctx?.servername || host || "localhost". Tests are updated to reflect the corrected behavior.
Security risks
The PR addresses a high-severity vulnerability: any tls.connect call without an explicit servername option bypassed hostname verification entirely. The fix sits directly in the code path that controls whether TLS connections verify the server hostname. One pre-existing gap remains (stale servername from a reused SecureContext) but is unchanged by this PR and noted in the description.
Level of scrutiny
High. The change is small and mechanically correct, but it controls certificate identity validation across all TLS connections. A human reviewer should confirm that no other consumers of the old camelCase serverName key remain that would now be silently broken, and verify the fallback chain is semantically equivalent to Node.js behavior.
Other factors
The two bug reports are both non-blocking: one is pre-existing and the other is a minor test cleanup nit. The new test file cross-validates against Node.js, which is a strong correctness signal. No CODEOWNERS rules apply to this path.
| const ctx = this[ksecureContext]; | ||
| return { | ||
| socket: this._handle, | ||
| ALPNProtocols: this.ALPNProtocols, | ||
| serverName: this.servername || host || "localhost", | ||
| checkServerIdentity: this[kcheckServerIdentity], | ||
| session: this[ksession], | ||
| rejectUnauthorized: this._rejectUnauthorized, | ||
| requestCert: this._requestCert, | ||
| ciphers: this.ciphers, | ||
| ...this[ksecureContext], | ||
| ...ctx, | ||
| servername: this.servername || ctx?.servername || host || "localhost", | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🟣 The ctx?.servername fallback can resolve to a stale servername from a pre-created SecureContext passed via options.secureContext — e.g. tls.connect({ host: 'new.example.com', secureContext: sharedCtx }) where sharedCtx was built with servername: 'old.example.com' will use 'old.example.com' for both SNI and checkServerIdentity instead of the intended host. This is a pre-existing issue: the old code's ...this[ksecureContext] spread already placed ctx.servername into the returned object, and net.ts consumed it identically, so this PR neither introduces nor worsens the behaviour.
Extended reasoning...
What the bug is: InternalSecureContext stores whatever servername is passed to createSecureContext(). If a caller builds a shared context explicitly — const sharedCtx = tls.createSecureContext({ servername: 'old.example.com', ca: ... }) — then passes it via options.secureContext to tls.connect({ host: 'new.example.com', port: 443, secureContext: sharedCtx }), the new explicit line servername: this.servername || ctx?.servername || host || 'localhost' resolves to 'old.example.com'. The TLSSocket constructor never assigns options.servername to this.servername (it stays undefined), so the ctx?.servername fallback wins over the host parameter. Both SNI and checkServerIdentity then operate on the wrong hostname, silently bypassing certificate identity verification for 'new.example.com'.
The specific code path: TLSSocket constructor sets this[ksecureContext] = options.secureContext || createSecureContext(options). When options.secureContext is provided, the user's shared context is used verbatim — including any servername it carries. In buntls, this.servername is undefined (never set from options), ctx.servername is 'old.example.com', and host is 'new.example.com', so the fallback chain resolves to 'old.example.com'.
Why existing code does not prevent it: The ctx?.servername fallback is intentionally necessary for the normal case: tls.connect({ host, servername: 'explicit' }) stores 'explicit' inside the SecureContext via createSecureContext(options), making ctx.servername the only path to recover the caller's explicit servername. There is no separate this.servername = options.servername assignment in the constructor. Node.js avoids this entirely by never supporting servername inside createSecureContext and using the strict formula options.servername || host.
Why this is pre-existing and not introduced by this PR: The pre-PR code returned ...this[ksecureContext] as a spread at the end of the object literal, which already placed ctx.servername (lowercase) into the result. net.ts reads tls.servername (lowercase) from the returned object, so it already received 'old.example.com' in the shared-context scenario before this change. The new explicit servername: this.servername || ctx?.servername || host || 'localhost' is semantically faithful to what the spread was already doing; the PR adds a meaningful host fallback for the case where servername is absent entirely — which is the actual fix being made. The problematic scenario is unchanged before and after.
Step-by-step proof: (1) const sharedCtx = tls.createSecureContext({ servername: 'old.example.com', ca: caPem }) — InternalSecureContext stores this.servername = 'old.example.com'. (2) tls.connect({ host: 'new.example.com', port: 443, secureContext: sharedCtx }) — TLSSocket constructor sets this[ksecureContext] = sharedCtx (options.secureContext is truthy) and this.servername = undefined. (3) buntls is called: this.servername is undefined, ctx.servername is 'old.example.com', host is 'new.example.com'. (4) The fallback chain this.servername || ctx?.servername || host resolves to 'old.example.com'. (5) net.ts uses self.servername = 'old.example.com' for both SSL_set_tlsext_host_name (SNI) and checkServerIdentity, so the certificate for 'old.example.com' passes even though we connected to 'new.example.com'.
Impact and fix: Any code that constructs a SecureContext with a servername for one host and reuses it for connections to other hosts will get wrong SNI and certificate identity checks. The proper fix is to assign this.servername = options.servername in the TLSSocket constructor and remove the ctx?.servername fallback, matching Node.js behaviour. This predates the PR and should be tracked as a separate follow-up.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/node/tls/node-tls-connect-hostname-verification.test.ts (1)
6-7:⚠️ Potential issue | 🟠 MajorSwitch this
*.test.tsfile tobun:test.Line 6 imports the Node test runner, but this path is covered by the repo rule requiring
bun:test.♻️ Proposed fix
-import { describe, test } from "node:test"; +import { describe, test } from "bun:test";#!/bin/bash # Verify this file uses Bun's test runner import rg -nP 'from\s+["'\'']node:test["'\'']|from\s+["'\'']bun:test["'\'']' test/js/node/tls/node-tls-connect-hostname-verification.test.tsAs per coding guidelines: "Use
bun:testfor test files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/node/tls/node-tls-connect-hostname-verification.test.ts` around lines 6 - 7, The test file imports the Node test runner; replace the import of describe and test from "node:test" with Bun's test runner by changing the import source to "bun:test" in node-tls-connect-hostname-verification.test.ts (keep the same named imports describe and test), ensuring the rest of the file (including tls usage) remains unchanged so it complies with the repo rule requiring bun:test for *.test.ts files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/js/node/tls/node-tls-connect-hostname-verification.test.ts`:
- Around line 6-7: The test file imports the Node test runner; replace the
import of describe and test from "node:test" with Bun's test runner by changing
the import source to "bun:test" in
node-tls-connect-hostname-verification.test.ts (keep the same named imports
describe and test), ensuring the rest of the file (including tls usage) remains
unchanged so it complies with the repo rule requiring bun:test for *.test.ts
files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dafc4b47-8452-4b0c-bcc5-e4f66a53a33c
📒 Files selected for processing (1)
test/js/node/tls/node-tls-connect-hostname-verification.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/net.ts`:
- Around line 1523-1524: The preconnected/upgraded TLS path fails to set
self._host/self._port so tls.connect({ socket, host }) and the upgrade-to-TLS
flow never populate these fields, causing self.servername to become "" and
checkServerIdentity() to validate against "localhost"; ensure _host and _port
are assigned for those paths by setting self._host = host and self._port = port
at the start of the TLS connection/upgrade code paths (in addition to or instead
of only in lookupAndConnect()), e.g., wherever tls.connect is invoked or the
upgrade-to-TLS routine runs so that self.servername is computed from the actual
host for checkServerIdentity().
In `@src/js/node/tls.ts`:
- Around line 692-695: The servername precedence incorrectly uses the `||`
operator which treats an explicit empty string as falsy and loses the caller's
intent to disable SNI; update the assignment of `servername` (currently using
`this.servername || ctx?.servername`) to check for undefined explicitly (e.g.,
prefer `this.servername` if it is !== undefined, else use `ctx?.servername` if
it is !== undefined, else fall back to deriving from `host`/`net.isIP(host)`),
referencing the `servername`, `this.servername`, `ctx?.servername`, `host`, and
`net.isIP` symbols so an explicit `""` is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 585a252f-10ed-41d6-a1a6-04fd98501584
⛔ Files ignored due to path filters (2)
test/js/third_party/grpc-js/fixtures/ca.pemis excluded by!**/*.pemtest/js/third_party/grpc-js/fixtures/server1.pemis excluded by!**/*.pem
📒 Files selected for processing (5)
src/js/node/net.tssrc/js/node/tls.tstest/js/node/test/parallel/test-http2-origin.jstest/js/node/tls/node-tls-connect-hostname-verification.test.tstest/js/third_party/grpc-js/fixtures/server1.key
| self._host = host; | ||
| self._port = port; |
There was a problem hiding this comment.
Populate _host for the preconnected/upgraded TLS path too.
Moving _host/_port initialization into lookupAndConnect() fixes the normal dial path, but tls.connect({ socket, host }) and the upgrade-to-TLS flow never call lookupAndConnect(). In those cases, an IP host now produces self.servername = "", so Line 255 / Line 568 fall through to "localhost" and checkServerIdentity() runs against the wrong hostname.
🐛 Proposed fix
{
const [options, connectListener] =
$isArray(args[0]) && args[0][normalizedArgsSymbol] ? args[0] : normalizeArgs(args);
let connection = this[ksocket];
let upgradeDuplex = false;
let { port, host, path, socket, rejectUnauthorized, checkServerIdentity, session, fd, pauseOnConnect } = options;
+ if (host !== undefined) this._host = host;
+ if (port !== undefined) this._port = port;
this.servername = options.servername;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/net.ts` around lines 1523 - 1524, The preconnected/upgraded TLS
path fails to set self._host/self._port so tls.connect({ socket, host }) and the
upgrade-to-TLS flow never populate these fields, causing self.servername to
become "" and checkServerIdentity() to validate against "localhost"; ensure
_host and _port are assigned for those paths by setting self._host = host and
self._port = port at the start of the TLS connection/upgrade code paths (in
addition to or instead of only in lookupAndConnect()), e.g., wherever
tls.connect is invoked or the upgrade-to-TLS routine runs so that
self.servername is computed from the actual host for checkServerIdentity().
| let servername = this.servername || ctx?.servername; | ||
| if (servername === undefined) { | ||
| servername = host && !net.isIP(host) ? host : ""; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js TLS, does passing servername: ""totls.connect() disable SNI and preserve the empty string instead of defaulting back to the host name?
💡 Result:
Yes, passing servername: "" to tls.connect disables SNI and preserves the empty string (no fallback to host), preventing the SNI extension from being sent in the TLS ClientHello.
Citations:
- 1: nodejs/node@f882c9b09b
- 2: http:
servername === falseshould disable SNI nodejs/node#27316 - 3: https://nodejs.org/api/https.html
- 4: https://nodejs.org/api/tls.html
🏁 Script executed:
# Read the TLSSocket.prototype[buntls] segment and connect function to see the exact code
cat -n src/js/node/tls.ts | sed -n '685,710p'Repository: oven-sh/bun
Length of output: 1873
🏁 Script executed:
# Also check the connect function around lines 881-889
cat -n src/js/node/tls.ts | sed -n '878,895p'Repository: oven-sh/bun
Length of output: 1873
Fix servername precedence to preserve explicit empty string for disabling SNI.
The || operator on line 692 treats servername: "" (which disables SNI) as falsy, falling through to the fallback on lines 693–694, which repopulates it from host. This breaks Node.js compatibility: an explicit empty string must remain empty and not revert to the hostname.
Use explicit undefined checks instead:
Proposed fix
- let servername = this.servername || ctx?.servername;
- if (servername === undefined) {
+ let servername = this.servername;
+ if (servername === undefined) {
+ servername = ctx?.servername;
+ }
+ if (servername === undefined) {
servername = host && !net.isIP(host) ? host : "";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let servername = this.servername || ctx?.servername; | |
| if (servername === undefined) { | |
| servername = host && !net.isIP(host) ? host : ""; | |
| } | |
| let servername = this.servername; | |
| if (servername === undefined) { | |
| servername = ctx?.servername; | |
| } | |
| if (servername === undefined) { | |
| servername = host && !net.isIP(host) ? host : ""; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/tls.ts` around lines 692 - 695, The servername precedence
incorrectly uses the `||` operator which treats an explicit empty string as
falsy and loses the caller's intent to disable SNI; update the assignment of
`servername` (currently using `this.servername || ctx?.servername`) to check for
undefined explicitly (e.g., prefer `this.servername` if it is !== undefined,
else use `ctx?.servername` if it is !== undefined, else fall back to deriving
from `host`/`net.isIP(host)`), referencing the `servername`, `this.servername`,
`ctx?.servername`, `host`, and `net.isIP` symbols so an explicit `""` is
preserved.
tls.connect({ socket, host: <ip>, ca }) was verifying the server
certificate against "localhost" instead of the IP passed in
options.host.
The upgrade-over-existing-socket path returns before lookupAndConnect,
so self._host stays undefined; and self.servername is "" for IP hosts
(no SNI per RFC 6066). The handshake handler's
`self.servername || self._host || "localhost"` therefore resolved to
"localhost".
Match Node's onConnectSecure and read from kConnectOptions:
options.servername || options.host || options.socket?._host ||
"localhost". options is already stored at connect time for this path.
This affected STARTTLS-style clients (pg, mysql2, ioredis, nodemailer)
connecting to servers by IP with IP-SAN certificates after #29096
started enforcing hostname verification on this path.
…mitted (oven-sh#29096) ## Summary `tls.connect({host, port})` without an explicit `servername` option was skipping hostname verification entirely and reporting `authorized: true` for any certificate that passed chain-of-trust. An on-path attacker presenting any CA-signed cert (e.g. a free Let's Encrypt cert for `attacker.com`) could MITM connections made by libraries like `pg`, `ioredis`, `mysql2`, and `nodemailer` that call `tls.connect` without setting `servername`. ## Root cause `TLSSocket.prototype[buntls]` (src/js/node/tls.ts) returned the host fallback under the key `serverName` (camelCase), but the consumer in src/js/node/net.ts read `tls.servername` (lowercase) — so `self.servername` stayed `undefined` and the guard `&& self.servername` at the handshake callback skipped `checkServerIdentity`, falling through to `authorized = true`. The lowercase key only existed (via the `...this[ksecureContext]` spread) when the user passed `servername` explicitly. The native layer provided no backstop: `SSL_set1_host` is declared but never called; only SNI is set. ## Fix Emit a single lowercase `servername` after the secureContext spread, computed as `this.servername || ctx?.servername || host || "localhost"`. The native `SSLConfig.fromJS` already accepts lowercase via `altNames: ["servername"]` in `SSLConfig.bindv2.ts`, so SNI continues to work. This also fixes a secondary issue where native SNI was reading the camelCase key (= host) instead of the user's explicit `servername` option. ## Tests - **New**: `test/js/node/tls/node-tls-connect-hostname-verification.test.ts` — written with `node:test` + `node:assert` so it runs identically under Node.js and Bun. Verified to pass in Node v25.2.1, fail on bun 1.3.11 (`authorized: true`, `checkServerIdentity` never called), and pass with this fix. - **Updated** existing tests that were silently relying on the bug: - `node-tls-cert.test.ts` — the shared `checkServerIdentity` override asserted `hostname === "localhost"` while the connect helper used `host: "127.0.0.1"`; the assertion was dead code before and is now corrected. Cipher tests now pass the override since their cert is for `agent10.example.com`. - `node-tls-cert-extra-ca.fixture.ts` — adds a no-op `checkServerIdentity` (fixture tests CA loading, not hostname). - `node-tls-connect.test.ts` — 4 assertions on the internal `[symbolConnectOptions].serverName` updated to `.servername`. ## Verification ``` node --experimental-strip-types --test test/js/node/tls/node-tls-connect-hostname-verification.test.ts # 3 pass USE_SYSTEM_BUN=1 bun test test/js/node/tls/node-tls-connect-hostname-verification.test.ts # 3 fail (bug repro) bun bd test test/js/node/tls/node-tls-{connect-hostname-verification,connect,cert}.test.ts # 52 pass / 0 fail bun bd test test/js/node/tls/node-tls-{server,context}.test.ts # 28 pass / 0 fail ``` ## Unrelated pre-existing gap While probing, noticed server-side `SNICallback` does not receive the client's SNI value in Bun (same on 1.3.11). Not touched by this PR.
Summary
tls.connect({host, port})without an explicitservernameoption was skipping hostname verification entirely and reportingauthorized: truefor any certificate that passed chain-of-trust. An on-path attacker presenting any CA-signed cert (e.g. a free Let's Encrypt cert forattacker.com) could MITM connections made by libraries likepg,ioredis,mysql2, andnodemailerthat calltls.connectwithout settingservername.Root cause
TLSSocket.prototype[buntls](src/js/node/tls.ts) returned the host fallback under the keyserverName(camelCase), but the consumer in src/js/node/net.ts readtls.servername(lowercase) — soself.servernamestayedundefinedand the guard&& self.servernameat the handshake callback skippedcheckServerIdentity, falling through toauthorized = true. The lowercase key only existed (via the...this[ksecureContext]spread) when the user passedservernameexplicitly.The native layer provided no backstop:
SSL_set1_hostis declared but never called; only SNI is set.Fix
Emit a single lowercase
servernameafter the secureContext spread, computed asthis.servername || ctx?.servername || host || "localhost". The nativeSSLConfig.fromJSalready accepts lowercase viaaltNames: ["servername"]inSSLConfig.bindv2.ts, so SNI continues to work. This also fixes a secondary issue where native SNI was reading the camelCase key (= host) instead of the user's explicitservernameoption.Tests
test/js/node/tls/node-tls-connect-hostname-verification.test.ts— written withnode:test+node:assertso it runs identically under Node.js and Bun. Verified to pass in Node v25.2.1, fail on bun 1.3.11 (authorized: true,checkServerIdentitynever called), and pass with this fix.node-tls-cert.test.ts— the sharedcheckServerIdentityoverride assertedhostname === "localhost"while the connect helper usedhost: "127.0.0.1"; the assertion was dead code before and is now corrected. Cipher tests now pass the override since their cert is foragent10.example.com.node-tls-cert-extra-ca.fixture.ts— adds a no-opcheckServerIdentity(fixture tests CA loading, not hostname).node-tls-connect.test.ts— 4 assertions on the internal[symbolConnectOptions].serverNameupdated to.servername.Verification
Unrelated pre-existing gap
While probing, noticed server-side
SNICallbackdoes not receive the client's SNI value in Bun (same on 1.3.11). Not touched by this PR.