node:tls: report servername as false on server sockets when the client sent no SNI - #39088
node:tls: report servername as false on server sockets when the client sent no SNI#39088robobun wants to merge 1 commit into
Conversation
…t sent no SNI After the handshake, tls.Server sockets (and standalone server-side TLSSocket wraps) exposed socket.servername as undefined when the ClientHello carried no SNI extension, and the ALPNCallback received servername: undefined in the same case. Node reports false on both: TLSWrap::GetServername() returns false when SSL_get_servername() is NULL, _finishInit stores that on the socket and callALPNCallback passes it to the callback. Normalize the native "no name" result in the two ServerHandlers sites that feed those node-visible values. The handshake-failure path is left as is: node never reaches _finishInit there, so it only ever stores a name the ClientHello did carry.
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 12:13 PM PT - Aug 15th, 2026
🔄 @robobun, the build for your commit |
|
Status: reproduced and fixed.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a two-line ?? false normalization in the node: compat layer, matching Node's TLSWrap::GetServername().
What was reviewed:
- The two
servernameOrFalse()call sites (handshake-successself.servernameand theALPNCallbackargument) against the cited Node sources; the deliberately-unchanged failure-path assignment is correctly justified. - Downstream consumers of
socket.servername(http2.tsinitOriginSet,net.ts:311) — all use falsy checks (!/||), sofalsevsundefinedis invisible to them. - The four new tests: both SNI/no-SNI variants for
tls.Serverand standalone server wraps, error events wired to reject, cleanup infinally, reusing the block's existinglisten()helper.
Extended reasoning...
Overview
This PR normalizes undefined → false for the server-side socket.servername property and the ALPNCallback servername argument when the client's ClientHello carried no SNI extension, matching Node v26.3.0's TLSWrap::GetServername() semantics. The functional change is a one-line helper (servername ?? false) applied at exactly two sites in src/js/node/net.ts (ServerHandlers.alpnCallback and the success branch of ServerHandlers.handshake). The rest of the source diff is comments — including one explaining why the adjacent handshake-failure assignment is not changed (Node never reaches _finishInit there, so the property keeps its constructor default). Four test cases are added to the existing v26.3.0 parity block in test/js/node/tls/node-tls-server.test.ts.
Security risks
None. This is a value-normalization change that only affects what user code observes for a reporting property; it does not alter any verification, authorization, or handshake decision. The ?? operator maps only null/undefined to false — a non-empty SNI string passes through unchanged, and BoringSSL's extract_sni rejects empty host_name at ClientHello parse time, so the server-side native value cannot be "".
Level of scrutiny
Low-to-medium. The behavioral surface is two expressions changing from x to x ?? false. I checked every in-tree consumer of .servername on server-side sockets: http2.ts initOriginSet reads it via if (!hostName) and net.ts:311 via self.servername || ... — both treat false and undefined identically. The _http_server.ts getter already normalized to false independently, so this brings the tls.Server path in line. The change is correctly placed in the node: compat layer per REVIEW.md guidance (Bun-native Bun.Socket#getServername() still returns undefined).
Other factors
The PR description is unusually thorough: it cites the exact Node source lines for each decision (why false, why ?? not an empty-string check, why the failure path stays as-is, why client-side is out of scope via #33476). The tests cover both the fixed case and a with-SNI control, for both entry points that share ServerHandlers (natively-accepted tls.Server connections and standalone new TLSSocket(raw, { isServer: true }) wraps). Tests follow the file's existing conventions — the shared listen() helper, Promise.withResolvers with error events wired to reject, and try/finally cleanup. The author reports the no-SNI cases fail on the unfixed build and pass with the fix, and that the vendored Node test-tls-* / test-https-agent-sni.js suites all pass.
|
@robobun can you submit a PR to our boringssl fork with the patches changes instead? and then can you remove the patch from this PR and push up a bump |
|
This PR does not carry a BoringSSL patch. The diff is two The open robobun PRs that currently add a
main itself still carries Leave the same comment on the PR you meant (or name it here, "all of the above" works too) and it gets the #35433 / oven-sh/boringssl#10 treatment: PR against oven-sh/boringssl, patch removed, |
2 participants
Problem
tls.createServer()connection listener,socket.servernameisundefinedwhen the client sent no SNI extension. Node v26.3.0 reportsfalse. Same for a standalonenew tls.TLSSocket(raw, { isServer: true })after'secure'.ALPNCallbackcarriesservername: undefinedin the same case. Node passesfalsethere too.test/parallel/test-https-agent-sni.jsdoesif (req.socket.servername !== false) res.setHeader('x-sni', servername); on atls.Serversocket under Bun that branch is taken withundefinedandsetHeaderthrows.src/js/node/net.tsServerHandlers.handshakeassigns the nativesocket.getServername()result as is, andServerHandlers.alpnCallbackforwards the native SNI argument as is. The native side (src/runtime/socket/tls_socket_functions.rsget_servername,src/runtime/socket/socket_body.rsselect_alpn_callback) reports a NULLSSL_get_servername()asundefined.httpsserver was already right:_http_server.ts's socket getter normalizes tofalse. Only thetls.Server/ server-sideTLSSocketpaths were inconsistent.Fix
servernameOrFalse()innet.ts(name ?? false) and applies it at the two sites that produce the node-visible values: the handshake-success assignment ofself.servernameand theALPNCallbackargument object. The fixing lines are the twoservernameOrFalse(...)call sites; the rest is comments.falseand why these two sites: both values come from the same node primitive,TLSWrap::GetServername(), which returns the SNI name orfalsewhenSSL_get_servername()is NULL (crypto_tls.cc#L1359-L1373)._finishInitstores it on the socket (wrap.js#L1094-L1096) andcallALPNCallbackpasses it to the callback (wrap.js#L243).??and not an empty-string check: BoringSSL rejects an empty SNI host_name at ClientHello parse time (vendor/boringssl/ssl/handshake_server.cc,extract_sni), so on the server side the native value is either a non-empty name or undefined;??mirrors node's NULL check exactly.'tlsClientError'socket). Node never reaches_finishIniton that path; onlySelectSNIContextCallbackwrites the property, and only when the ClientHello carried a name (crypto_tls.cc#L1396-L1399), so a no-SNI failure keeps the property's initial value in node. That matches what Bun does there today; a comment now says why that line differs from the one below it. The client-side value after the handshake (node:falsewithout SNI, Bun:"") is the subject of node:tls: do not derive the SNI server_name from host in tls.connect #33476 and is not touched here.node:compat layer on purpose:Bun.Socket#getServername()is a public Bun API and keeps returningundefined.test/js/node/tls/node-tls-server.test.tsgains four cases in the v26.3.0 parity block (tls.Serversocket +ALPNCallback, and a standalone server-side wrap, each with and without SNI). On the unfixed build the two no-SNI cases fail (undefinedreceived,falseexpected) and the two with-SNI cases pass; all four pass with the fix.test/js/node/tls/directory (the only failures areSNICallback runs even when the requested servername matches the bind hostname, which is the host-dependent ECONNREFUSED that test(tls): dial bound IP in 'SNICallback matches bind hostname' (host-dependent ECONNREFUSED) #35160 fixes and fails the same way before this change, and the root-certs worker race test hitting its 5s budget on this debug+ASAN container, also unchanged), plus the vendoredtest-tls-sni-*.js,test-tls-alpn-server-client.js,test-tls-psk-alpn-callback-exception-handling.js,test-tls-add-context.js,test-tls-empty-sni-context.js,test-tls-snicallback-error.jsandtest-https-agent-sni.js, all exit 0.Background
SSL_get_servername()is the BoringSSL/OpenSSL accessor for that name on a connection; it returns NULL when the ClientHello had no such extension.TLSWrapis node's native per-connection TLS object;TLSWrap::GetServername()is the JS-callable wrapper aroundSSL_get_servername()and is where node'sfalseoriginates._finishInitis the node function that runs when a handshake completes;callALPNCallbackis the one that invokes a server'sALPNCallbackoption while the handshake is still in progress.ServerHandlers.handshakeandServerHandlers.alpnCallbackinnet.tsare Bun's counterparts, shared bytls.Serverconnections and standalone server-side wraps, which is why one change covers both.node v26.3.0 vs Bun on the affected paths
Probe: a
tls.createServerwith anALPNCallback, a server expecting TLS 1.3 dialed by a TLS 1.2-only client (to reach'tlsClientError'), and anew TLSSocket(raw, { isServer: true })wrap; each dialed once without SNI and once withservername: "sni.example".The remaining
undefinedvsnullbefore the handshake (and on the failure path) is theTLSSocketconstructor's initial value, shared with client sockets, and is outside this change.