Conversation
|
Updated 1:08 AM PT - Sep 1st, 2026
✅ @robobun, your commit 77846daffdb9399317f3cf7be310249842ab8b17 passed in 🧪 To try this PR locally: bunx bun-pr 33253That installs a local version of the PR into your bun-33253 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. WalkthroughSNI-selected SSL contexts now install the ALPN selector before registration or use during an in-flight handshake. Tests cover synchronous and asynchronous ChangesTLS SNI and ALPN integration
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The PR fixes ALPN negotiation for SNI-selected TLS contexts, and the supplied current-head ASAN and release tests pass. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The PR addresses the ALPN portion of issue Full details: Description checkExplanation The description clearly explains the ALPN/SNI fix, affected paths, implementation, issue linkage, and verification results. It uses a different heading from the template for the change summary and does not include the exact verification heading, but it provides the required information in detail. Comment |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Verified all three against this branch:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the await using fix — no further issues on my end, but since this touches the TLS handshake path and installs the ALPN callback onto user-supplied SSL_CTXs, I'd like a human reviewer to sign off.
Extended reasoning...
Overview
This PR fixes ALPN negotiation being silently dropped whenever SNI selects a non-default SecureContext. The native change is small and surgical: a new install_sni_alpn_selector() helper in src/runtime/socket/socket_body.rs that registers the existing select_alpn_callback on a given SSL_CTX, called from the three SNI selection paths (us_dispatch_server_name for sync SNICallback, resume_sni for async SNICallback, and Listener::add_server_name for addContext). The callback itself is unchanged — it already dispatches through the per-SSL ex_data slot and returns NOACK when unset. Five new tests in test/js/node/tls/node-tls-context.test.ts cover all three paths plus ALPNCallback and the RFC 7301 no_application_protocol fatal alert.
Security risks
This is TLS handshake code interfacing directly with BoringSSL. The change adds a callback registration to SSL_CTX objects that previously had none, and those contexts can be user-created (tls.createSecureContext(...)) and potentially shared across servers or reused as client contexts. The PR argues (and select_alpn_callback at socket_body.rs:79 confirms) that the selector is inert when the per-SSL ex_data slot is unset (returns NOACK), and BoringSSL only invokes the ALPN select callback server-side. I don't see a concrete vulnerability, but mutating a user-owned shared SSL_CTX in a security-critical path is exactly the kind of change that benefits from a domain expert's eye — e.g., confirming there's no scenario where a shared context's newly-installed callback overrides an intentionally different ALPN configuration on another listener, or where ex_data slot 0 collides with something else.
Level of scrutiny
High. TLS/ALPN negotiation is security-sensitive and production-critical (every multi-domain HTTPS/h2/gRPC server hits this path). The fix is well-reasoned, the PR description traces the root cause through BoringSSL's ssl_negotiate_alpn, and test coverage is thorough — but per the approval guidelines I should not auto-approve changes to crypto/TLS paths regardless of apparent quality.
Other factors
- My prior nit (test cleanup ordering) was addressed in 1dda998 with
await usingand the thread is resolved. - The bug-hunting system found no issues.
- No CODEOWNERS entry covers these files.
- Tests demonstrate before/after behavior against
USE_SYSTEM_BUN=1and pass locally per the description; the widertest/js/node/tls/suite and relevant Node parallel tests were also run.
|
To save the human reviewer the dig on the two specific concerns raised above: Shared context overriding another listener's ALPN configuration. It can't. ex_data slot 0 collision. BoringSSL reserves index 0 of the |
There was a problem hiding this comment.
Thanks for addressing the cleanup nit — no further issues from my side, but since this installs an ALPN selector on user-supplied SSL_CTXs across three TLS handshake paths, I'd like a human familiar with the BoringSSL/SNI layer to confirm the shared-context reasoning before merge.
Extended reasoning...
Overview
The PR fixes ALPN negotiation being silently dropped whenever SNI (via SNICallback, resumeSNI, or addContext) swaps in a fresh SSL_CTX that lacks the ALPN selection callback. The native change is small: a new install_sni_alpn_selector() helper in src/runtime/socket/socket_body.rs that wraps SSL_CTX_set_alpn_select_cb, plus three call sites in Listener.rs / socket_body.rs covering every path a context reaches the SNI machinery. Five new tests in test/js/node/tls/node-tls-context.test.ts cover sync/async SNICallback, addContext, ALPNCallback-with-SNI (#17932), and the RFC 7301 fatal-alert path.
My earlier nit about resource cleanup on assertion failure was addressed in 1dda998 (await using for servers/clients, with a documented try/finally exception for the intentionally-erroring client). The bug-hunting pass found nothing further.
Security risks
This is a TLS handshake code path. The change registers Bun's select_alpn_callback on SSL_CTX objects that may be user-created and/or shared across connections/listeners. The author's argument that this is inert — the callback dispatches entirely through per-SSL ex_data slot 0 (returning SSL_TLSEXT_ERR_NOACK when unset) and reads no state from the SSL_CTX it's registered on — checks out against the source (select_alpn_callback at socket_body.rs:79-226), and the same registration already happens on the listener's default context in on_open. Still, mutating a user-owned SecureContext's underlying SSL_CTX and the cross-listener sharing scenario are exactly the kind of subtle TLS-layer side effects a maintainer should sign off on.
Level of scrutiny
Medium-high. The diff is small and well-reasoned with thorough test coverage, but it sits in production-critical crypto/TLS handshake code (src/runtime/socket/, BoringSSL FFI). Per repo guidelines, security-sensitive changes should get human review regardless of size.
Other factors
- No CODEOWNERS entry for these paths.
- The PR description pre-emptively addresses the two non-obvious concerns (shared-context cross-contamination, ex_data slot 0 collision) with source citations, which should make the human review quick.
- Tests verified to fail on system Bun and pass on the debug build per the description.
|
Ready to merge: CI is green on the rebased head (build 109014 passed at 77846da, base e2eac5f). Rebased onto current main per the request above. Conflicts were main's move to the Verified on the rebased build: all 22 tests in The two questions the automated reviewer raised (whether installing the ALPN selector on a shared Earlier CI red on this PR (builds 67976, 67990, 70802, all pre-rebase) was infra or pre-existing flake on lanes unrelated to the diff: a darwin-26-aarch64 agent's artifact-download timeout, a MySQL docker container health-check timeout on alpine, Bake dev-server and |
39f515b to
f8fd1f1
Compare
There was a problem hiding this comment.
No issues found and my earlier nit is addressed — deferring only because this touches the TLS handshake / BoringSSL ALPN path, which warrants a human sign-off.
Extended reasoning...
Overview
This PR adds a small helper, install_sni_alpn_selector, that registers the existing select_alpn_callback on any SSL_CTX that the SNI machinery may install via SSL_set_SSL_CTX, and calls it from the three SNI selection paths (us_dispatch_server_name for the synchronous SNICallback, resume_sni for the asynchronous one, and Listener::add_server_name for addContext/addServerName). Five new tests in test/js/node/tls/node-tls-context.test.ts cover each path plus the ALPNCallback interaction and the RFC 7301 no_application_protocol alert on a genuine mismatch.
Security risks
The change lives in the TLS handshake path: it mutates user-owned/shared SSL_CTX instances by installing a server-side ALPN selection callback, and it affects whether a mismatched ALPN offer produces a fatal alert. The author's analysis (the selector reads only per-SSL ex_data set in on_open and returns SSL_TLSEXT_ERR_NOACK when unset, so installing it on a shared context is inert for other connections; ex_data slot 0 is BoringSSL's reserved app-data slot and not otherwise written) looks sound and the callback itself is unchanged, but this is exactly the class of change where a subtle assumption error weakens or breaks TLS negotiation.
Level of scrutiny
High. This is production-critical, security-sensitive native code (BoringSSL callback registration inside the SNI/ALPN handshake sequence) rather than a mechanical or config change. The native diff is small and well-argued, but per policy I don't auto-approve changes to crypto/TLS paths.
Other factors
The bug-hunting pass found nothing. My earlier nit about test resource cleanup on the failure path was addressed with await using (and a documented try/finally for the intentionally-erroring client). Tests are hermetic (port: 0, local loopback, rejectUnauthorized: false), assert both client- and server-side alpnProtocol, and were shown to fail against an unfixed binary. The PR description enumerates the sibling SSL_set_SSL_CTX sites and explains why the two untouched ones (bind-hostname SNI tree entry, setKeyCert) don't need the same treatment. Given all that, this looks correct to me — I'm deferring purely because TLS handshake changes should get a human reviewer's sign-off.
|
Issue #41061 is a new report of this bug, reproduced on 1.4.0 and 1.4.1: both the ALPNProtocols + SNICallback and the ALPNCallback + SNICallback shapes negotiate no ALPN, and it blocks mockttp as an HTTP/2 proxy under Bun. The branch currently conflicts with main and needs a rebase. |
|
@robobun fix conflicts rebase |
BoringSSL resolves the server-side ALPN selection callback off the current ssl->ctx. Bun only registered that callback on the listener's default context (in on_open), so whenever the SNI machinery installed a different SSL_CTX with SSL_set_SSL_CTX (an SNICallback result, an asynchronous resumeSNI result, or an addContext/addServerName tree entry), BoringSSL found no callback at ALPN-negotiation time and silently skipped ALPN. Every SNI/multi-domain TLS server lost its ALPNProtocols and ALPNCallback the moment SNI selected a per-domain certificate (alpnProtocol === false on both ends), and a genuine mismatch completed the handshake instead of sending the RFC 7301 no_application_protocol alert. Register the selector on every context the SNI machinery can install. The selector dispatches through the per-SSL ex_data slot on_open sets and returns NOACK when it is unset, so installing it on a shared or user-owned SecureContext is inert for connections that do not negotiate ALPN.
A failing assertion left the listening server and client socket alive. Register both for disposal at declaration (await using), and destroy the expected-to-error client in a finally since a Readable's Symbol.asyncDispose rejects with the stream's terminal error.
f8fd1f1 to
bd2233f
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased onto current main (e2eac5f, #39848) and force-pushed; the branch is now conflict-free at 77846da. What the conflicts were, in order:
Verified on the rebased build: all 22 tests in The two follow-up commits trim the comments the comment-cop flagged: the three call-site comments are gone, and the helper's doc comment is down to the one BoringSSL fact the code depends on. |
There was a problem hiding this comment.
Re-reviewed after the latest push and didn't find any bugs — the earlier cleanup nit is now addressed (tests use await using on the server/client, and the mismatch case wraps the expected-to-error client in try/finally { client.destroy() }). Since this touches the TLS handshake path across three SNI entry points, a human sign-off is still worthwhile.
What was reviewed:
install_sni_alpn_selector— null-guarded, registers the existingselect_alpn_callback(which already dispatches via per-SSL ex_data and returnsNOACKwhen unset), so installing it on a shared/user-owned ctx is inert for connections that never set the ex_data.- Refcount at
decode_sni_result:sc.ctx.clone().into_raw()+1 is unchanged and still freed by the C dispatcher afterSSL_set_SSL_CTX; the new helper only sets a callback and takes no reference. - New tests:
port: 0, event-driven waits (once/withResolvers), error events wired to reject, one.toEqualon{client, server}, and the RFC 7301 case asserts the specificERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOLcode.
Extended reasoning...
Overview
The PR fixes ALPN negotiation being silently dropped when a node:tls server selects a per-domain SecureContext via SNI. It adds a small helper install_sni_alpn_selector(ctx) in src/runtime/socket/socket_body.rs that registers the pre-existing select_alpn_callback on any SNI-selected SSL_CTX, and calls it from three sites: Listener::add_server_name (addContext), decode_sni_result (synchronous SNICallback), and the async resume_sni path in NewSocket. Five new tests in test/js/node/tls/node-tls-context.test.ts cover sync/async SNICallback, addContext, ALPNCallback still firing after SNI (issue #17932), and the RFC 7301 fatal no_application_protocol alert on a genuine mismatch.
Security risks
This is TLS handshake code. The change does not weaken any security check — it restores ALPN negotiation (and with it, the fatal-alert-on-mismatch behaviour) that was being skipped. The callback being installed is the same one already registered on the listener's default context; it reads the TLSSocket back from per-SSL ex_data and returns SSL_TLSEXT_ERR_NOACK when that slot is unset, so registering it on a user-owned or shared SSL_CTX is a no-op for connections that never had the ex_data set (including client-side use of the same context, where BoringSSL doesn't invoke the server ALPN callback anyway). No new trust decisions, no flag removals, no relaxed validation. That said, any change to how BoringSSL callbacks are wired across SSL_set_SSL_CTX deserves a maintainer's eyes.
Level of scrutiny
Medium-high. The native diff is tiny (~15 lines) and mechanical — it reuses an existing callback and existing FFI wrapper — but it sits in the TLS handshake path and touches three distinct SNI entry points, one of which involves an into_raw() refcount hand-off to C. The FFI/refcount shape is unchanged (the helper only calls SSL_CTX_set_alpn_select_cb, which stores a function pointer and takes no reference), and the null guard covers the async path where ctx_ptr can legitimately be null. Per REVIEW.md, changes to TLS/crypto paths should not be auto-approved.
Other factors
An earlier run of this review left a nit about the new tests leaking the server/client on assertion failure. Commit bd2233fb addressed it: the first four tests now use await using on both tls.createServer and tls.connect, and the fifth (which expects the client to error, so Symbol.asyncDispose would reject) wraps the client in try/finally { client.destroy() } with a comment explaining why. The tests follow the harness conventions in test/CLAUDE.md — port: 0, once(emitter, event) and Promise.withResolvers instead of sleeps, error/tlsClientError wired to reject the awaited promise, a single .toEqual on the combined client/server result, and a specific error-code assertion rather than a bare toThrow. The PR description's before/after evidence shows all five new tests fail on USE_SYSTEM_BUN=1 and pass on the debug build. No outstanding third-party CHANGES_REQUESTED reviews are visible in the timeline; a maintainer commented today, so surfacing a fresh clean-review status is useful rather than noise.
…'s TLS context (#43015) ### Problem - `tls.Server#setSecureContext()` on a listening server changes nothing. Later handshakes keep the original certificate, key and client-CA store. A CA that the operator removed keeps authorizing client certificates (`authorized=true`). Node applies the new context to the next connection. - Cause: `Listener::listen` (`src/runtime/socket/Listener.rs`) builds the `SSL_CTX` once. `setSecureContext` (`src/js/node/tls.ts`) only reassigned the option fields that `listen()` reads. ### Fix - `setSecureContext()` on a listening server builds an `SSL_CTX` from the staged options. `us_listen_socket_set_default_ssl_ctx()` makes it the default for later accepts. It runs before any field is assigned, so rejected material throws and nothing changes. - First commit: `listen()` no longer registers the default context under the bind hostname in the SNI tree. That entry shadowed an `addContext()` wildcard and would pin the name to the old context. - As in Node, `setSecureContext()` no longer reads `ALPNProtocols` (constructor only). A cluster worker reads its TLS options when the primary answers, so a call made before `'listening'` counts. - Verified on Linux and Windows: `node-tls-server.test.ts` (17 new tests, 11 fail on bun 1.4.2), `node-tls-namedpipes.test.ts`. On Linux: `test/js/node/tls/`, `node-http2.test.js`, `node-net-server.test.ts`. Self-reviewed: one regression found and fixed (Notes). ### Background - An `SSL_CTX` is BoringSSL's TLS configuration: certificate chain, key, CA store, verify mode. Each accepted socket creates its `SSL` from the listen socket's default and holds a reference. - The SNI tree maps server names to other contexts (`addContext()`). A matching ClientHello switches that connection's context. - Session tickets belong to one `SSL_CTX`, so a new context never resumes an old session. <details><summary>Notes</summary> **Repro** (two CAs, server trusts `caA`, then `setSecureContext({ ..., ca: caB })`): ``` bun 1.4.2 / main this branch = node v26.3.0 after clientA (caA): hello clientA authorized=true refused after clientB (caB): refused hello clientB authorized=true resume clientA session: authorized=true reused=true refused ``` The same holds for an `Http2SecureServer`, which is the class `@grpc/grpc-js` reloads credentials on. **Scope.** `tls.Server` and `Http2SecureServer` only. Not covered, on purpose: - `https.Server` and `Bun.serve().reload({ tls })` (#8872). They go through `Bun.serve`, not `Listener.rs`. #42576 and the draft #35535 add `https.Server#setSecureContext()`. No open PR covers `Bun.serve().reload({ tls })`. - `setSecureContext(secureContextInstance)`. Node takes an options object only. node v26.3.0 reads the instance as options with no key and no certificate, and every later handshake fails. Bun 1.4.2 and this branch both leave the listening socket on its context for that form. A context from `tls.createSecureContext()` carries no `requestCert` verify mode, so this PR does not install it. - ALPN on a connection whose SNI selects an `addContext()` or `SNICallback` context is still not negotiated. That is #33253. The wildcard test here asserts the certificate only. **Cluster workers.** A worker's `listen()` completes when the primary answers. `net.ts` built the native TLS options when `listen()` was called, so a `setSecureContext()` or `addContext()` made before `'listening'` was lost. `listenOnPrimaryHandle` now reads them again. For `tls-cluster-set-secure-context-fixture.mjs`, node v26.3.0 prints `default: agent3, viaAddContext: agent2` and bun 1.4.2 prints `agent1` for both. **ALPNProtocols.** This is a behavior change outside a listening server too. `setSecureContext({ ALPNProtocols })` no longer sets the list, also before `listen()`. node v26.3.0 reads the option in the `Server` constructor only (`lib/internal/tls/wrap.js` L1381-L1382). Probe: a server created with `['h2']` and given `['http/1.1']` through `setSecureContext()` negotiates `h2` in node, also after `close()` + `listen()`. Bun 1.4.2 negotiates `http/1.1` after the re-listen. Before this change a listening server reported the new list on `server.ALPNProtocols` while its listener kept the old one. **requestCert clamp.** A server that does not request a client certificate must not reject for one. `net.ts` applied that to the options after `[buntls]` built them. The rule now lives in `[buntls]`, so `listen()`, the cluster reply and the context swap share it. **Related PRs.** The C primitive and its header comment are taken as they are from the drafts #35535 and #41400, so the C hunks merge without a conflict. The Rust wrapper takes an `&OwnedSslCtx` where the drafts take a raw pointer, so safe code cannot pass a dead pointer. Neither draft calls it from `Listener.rs`. #42576 has the same function under the name `us_listen_socket_set_ssl_ctx`. #42050 builds `_sharedCreds` eagerly so that bad material throws before `listen()`. Its notes say it does not rotate the listener. It edits the same function in `tls.ts`, so one of the two needs a rebase. #32435 adds the `tls.Server` methods to `https.Server`. #37896 changes what a cluster TLS worker's `_handle` is. Here a `_handle` that is not a `Listener` is a no-op, and the worker's connections are wrapped in JS from the server's own fields. #33365 was the first version of this fix. A stale-PR sweep closed it. #42355 and #42998 edit `Listener.rs` and `openssl.c` in other places. **Self-review.** A review of the first version of this diff found one regression, and it is fixed. That version moved the bind-hostname SNI entry to the new context. A connection accepted before the swap whose ClientHello arrived after it was switched to the new context, which had no ALPN selector yet, so an h2 server answered `unknownProtocol`. The first commit removes the entry, and two tests pin the case for `tls.Server` and `Http2SecureServer`. The review also asked for h2 coverage, for the `ALPNProtocols` fix, for the requestCert/rejectUnauthorized matrix tests and for the maintainer's symbol name. All are in. **Reference counts.** `secure_ctx` is an `OwnedSslCtx`. The swap is `set_default_ssl_ctx` (C takes its own reference and drops the one on the old default) and `secure_ctx.set(Some(ctx))` (drops the listener's reference on the old one). The test `frees the context it replaces` asserts with `sslCtxLiveCount()` that 20 swaps and one rejected swap add no live `SSL_CTX`, and that `close()` releases the last one. **Windows.** A named-pipe TLS server keeps its context in `WindowsNamedPipeListeningContext.ctx`, now a `JsCell`. Each accept clones the context out of the cell before it can run JS, because `setSecureContext()` can replace the slot. At ddd655c I built the debug binary on Windows x64 and ran `node-tls-namedpipes.test.ts` (7 pass) and the new tests in `node-tls-server.test.ts` (18 pass, the cluster test included). The canary on that machine (1.4.3) fails the named-pipe test: it serves `agent1` after the swap. `bun run rust:check-all` passes for all 12 targets at the same commit. **Test hygiene.** `SNICallback runs even when the requested servername matches the bind hostname` now dials the address `listen()` bound. `localhost` resolves to `::1` and `127.0.0.1` on a dual-stack host, and `connect()` need not pick the one `listen()` did. #35160 makes the same change on its own. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 13 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/tls/node-tls-server.test.ts, test/js/node/tls/node-tls-namedpipes.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
|
Any plans on merging this? It has become a blocker for us |
The tests of #33253: the server's ALPNProtocols and ALPNCallback hold after a synchronous SNICallback, an asynchronous SNICallback and addContext(), and a protocol mismatch gets the no_application_protocol alert.
This branch has not been deployed
3 participants
What
Any
SecureContextselected through SNI silently drops the server's ALPN configuration. An h2/gRPC client that negotiatesh2against the default context gets no ALPN at all (alpnProtocol === falseon both ends) onceSNICallbackoraddContextpicks a per-domain certificate, and nothing logs why. A genuine ALPN mismatch also completes the handshake instead of sending the RFC 7301no_application_protocolalert. This affects every SNI/multi-domaintls/https/http2server, which is exactly the deployment shape that needs ALPN.All three paths that hand a context to the SNI machinery reproduce it: a synchronous
SNICallback, an asynchronousSNICallback(theresumeSNIresume), andserver.addContext(). The same loss also silences a serverALPNCallbackwhenever anSNICallbackis present, which is the remaining half of #17932 (its exactSNICallback+ALPNCallbackrepro givesalpnCalled: 0, alpnProtocol: falsebefore this change and matches Node after it). It is invisible in single-cert tests because without SNI the default context never gets replaced.Fixes #17932
Why
BoringSSL resolves the server ALPN selection callback off the current
ssl->ctx(ssl_negotiate_alpn,vendor/boringssl/ssl/extensions.cc), and resolves it after the SNI callbacks have run. Bun registered its selector (select_alpn_callbackinsrc/runtime/socket/socket_body.rs) only on the listener's defaultSSL_CTX, inon_openat accept time. Every SNI selection path installs a different context withSSL_set_SSL_CTX(us_select_cert_cb/sni_cbinpackages/bun-usockets/src/crypto/openssl.c). The freshly createdSecureContexthas noalpn_select_cb, so BoringSSL's "ignore ALPN if not configured" branch fires and ALPN is skipped without an error.Node never hits this because it copies only the certificate/key/chain from the SNI-selected
SecureContextonto the connection; its connection-level ALPN handling is independent of which context SNI selected.Fix
The selector already dispatches entirely through per-SSL state (it reads the
TLSSocketback from the ex_data sloton_opensets, never from theSSL_CTX), so the context it is registered on does not matter as long as it is the one BoringSSL looks at. Add a helper that installs it on a givenSSL_CTX, and call it from the three places a context reaches the native SNI machinery:decode_sni_result(src/runtime/socket/Listener.rs), whichus_dispatch_server_namecalls to turn the synchronousSNICallbackresult into theSSL_CTX*handed back to C.resume_sni(src/runtime/socket/socket_body.rs): the asynchronousSNICallbackresult.Listener::add_server_name(src/runtime/socket/Listener.rs):addContext/addServerNamestatic SNI tree entries.The selector returns
SSL_TLSEXT_ERR_NOACKwhen the per-SSL ex_data is unset, so installing it on a shared or user-owned context is inert for connections that do not negotiate ALPN, including clients (BoringSSL only invokes it server-side).Not changed: the bind-hostname SNI tree entry points at the listener's own default context, which
on_openalready registers.setKeyCertalso callsSSL_set_SSL_CTX, but Node restricts it toALPNCallback, which runs from inside BoringSSL's ALPN negotiation after the callback has already been resolved, so it cannot drop ALPN.Tests
test/js/node/tls/node-tls-context.test.ts, "ALPN survives an SNI-selected SecureContext": five cases covering the synchronousSNICallback, the asynchronousSNICallback,addContext, theALPNCallback(the other consumer of the same selector), and the fatalno_application_protocolalert on a genuine mismatch.Before / after
Also green with the fix:
test/js/node/tls/(the three remaining failures there reproduce with this diff stashed),test-tls-sni-option.js,test-tls-empty-sni-context.js,test-tls-snicallback-error.js,test-tls-psk-alpn-callback-exception-handling.js,test-tls-secure-context-usage-order.js.[human-review] gate passed · iteration 9 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 9
evidence per changed file