Conversation
https.createServer({ SNICallback }) was accepted but never dispatched: the
node:https server path listens via Bun.serve, whose uWS listen socket had no
dynamic SNI resolver hooked up, so every handshake served the default context
and the callback was never reached. tls.createServer already worked because it
listens via the Bun.listen path, which registers us_listen_socket_on_server_name.
Wire the same select-certificate hook into the Bun.serve listen socket:
- us_listen_socket_on_server_name gains a user-data pointer so the dispatch can
recover its owning server (group->ext belongs to the uWS HttpContext here, not
the server).
- NewServer carries an on_server_name JSValue (GC-rooted via the wrapper's
m_onServerName WriteBarrier), set through setServerCustomOptions alongside the
existing onClientError / onConnection handlers.
- _http_server.ts validates and stores options.SNICallback, and passes a
serverName handler to setServerCustomOptions that calls the user callback and
returns the selected native SecureContext / undefined / Error, matching the
net.ts ServerHandlers.serverName contract so Listener.rs's decode_sni_result
is reused for the return-value decoding.
Fixes #14395.
WalkthroughChangesThe change adds HTTPS HTTPS SNI callback support
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:05 AM PT - Aug 4th, 2026
❌ @robobun, your commit 75d5af3 has some failures in 🧪 To try this PR locally: bunx bun-pr 36707That installs a local version of the PR into your bun-36707 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Match Node's context.context || context so cb(null, secureContext.context) works on the node:https path the same as node:tls. decode_sni_result on the native side already validates SecureContext and aborts on anything else, so the JS handler hands the unwrapped value through and lets that check reject an invalid context.
Narrow to __attribute__((nonnull(1, 2))) instead of dropping the attribute when the nullable data parameter was added, matching the surrounding declarations in this header.
node-http.test.ts has a host-dependent ECONNREFUSED in 'request via http proxy, issue#4295' (proxy listens on localhost, client dials 127.0.0.1); keep the SNICallback coverage in a dedicated file alongside the other node-https-* tests so it stays green independent of that.
cb(null, true) was evaluating to the suspend sentinel via the context.context || context fallthrough and parking the handshake with no resume handle on this path. Require the unwrapped value to be an object so primitives hit 'Invalid SNI context' (matching net.ts consumeSNIResult and its sentinel-collision guard). Emit tlsClientError on the server before returning a failure so the SNI error is observable like it is on tls.createServer.
Match on_client_error_callback/on_connection_callback's guard before reading the WriteBarrier-shadow JSValue; the listen socket is closed before the wrapper can finalize so the window is not reachable today, but the shadow slots share the same storage model and the file documents the guard as the contract for reading them.
The cb() result is now decoded via a single internal/tls helper that
instanceof-checks against the native SecureContext, so cb(null, {}) and
cb(null, true) both surface 'Invalid SNI context' through tlsClientError on
node:https the same as on node:tls. Wrap the error-test loop body in
try/finally so a failing assertion does not leak a listening server.
process.nextTick the tlsClientError emission so a throwing listener surfaces as uncaughtException instead of its thrown value being fed back into decode_sni_result as the select-certificate result; matches net.ts where the emit happens after us_select_cert_cb returns. Validate options.SNICallback only inside the isTlsSymbol branch and gate on truthiness so plain http.createServer ignores the option the way Node's http.Server does.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/tls.ts`:
- Around line 143-149: Update unwrapSNIContext so the context.context access is
wrapped in exception handling and any getter or Proxy failure is returned as the
caught Error rather than escaping; preserve the existing valid-context and
invalid-context behavior. Add coverage for a callback result containing a
throwing getter or Proxy and verify the failure follows the intended SNI
error/tlsClientError path.
In `@test/js/node/http/node-https-snicallback.test.ts`:
- Around line 77-87: Update the test’s event synchronization around the
tlsConnect call: register once(server, "tlsClientError") before creating the
client, then await both the client error and server tlsClientError events before
making assertions. Use the awaited server error for the expectedMessage check,
while preserving the existing client termination assertion and tlsClientError
occurrence validation.
- Around line 22-27: Update the SNICallback test to defer cb(null, altContext)
for the agent1 server name until a later turn, and adjust the listener resume
path to select that deferred context. Assert the client receives the agent1
certificate, while preserving the existing raw-handle and default-context cases.
🪄 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: be6b97ca-485c-494b-b423-c1bfe61b4261
📒 Files selected for processing (15)
packages/bun-usockets/src/context.cpackages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/libusockets.hpackages/bun-uws/src/App.hsrc/js/internal/http.tssrc/js/internal/tls.tssrc/js/node/_http_server.tssrc/jsc/bindings/NodeHTTP.cppsrc/runtime/server/mod.rssrc/runtime/server/server.classes.tssrc/runtime/server/server_body.rssrc/runtime/socket/Listener.rssrc/uws_sys/ListenSocket.rstest/js/node/http/node-https-snicallback.test.ts
Register once(server, 'tlsClientError') before connecting and await both so the assertion does not depend on nextTick running before the client's error event.
…h for the error matrix
Match net.ts toSNIError so cb("boom") / throw 42 carry the original value
as .reason on the tlsClientError payload, and move the per-variant loop into
it.each so a failure names the case and later cases still run.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/http/node-https-snicallback.test.ts`:
- Around line 62-65: The “cb(string)” SNI error test only checks the normalized
message and must also verify preservation of the original callback value in
Error.reason. Update the assertions in the sniErrorCases tests and the related
assertions at the referenced locations to check that the produced error’s reason
equals "boom", while retaining the existing message and error-case coverage.
🪄 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: ccf61e41-9a61-4084-b63f-f1242e475efa
📒 Files selected for processing (2)
src/js/node/_http_server.tstest/js/node/http/node-https-snicallback.test.ts
There was a problem hiding this comment.
No new issues found in this pass — the earlier inline findings (SNICallback validation gate, tlsClientError emit timing, unwrapSNIContext decode contract, .reason attachment, it.each matrix) have all been addressed through 658fe85. Deferring to a human because this threads a TLS certificate-selection hook through native code across the C/C++/Rust FFI boundary (raw *mut Self stashed on the listen socket's SNI data slot, a new GC-rooted on_server_name slot, and a dispatch extern fn that recovers the server from that pointer mid-handshake), which is security-adjacent and lifetime-sensitive enough to warrant a maintainer look.
What was reviewed:
server_body.rsdispatch: null/shutdown guards,decode_sni_resultreuse, and the "listen socket cannot outlive the server" lifetime claim againstdo_stop/deinitordering.onServerSNIin_http_server.ts: fail-closed on invalid context / non-Error / throw; deferred-cb fall-through whensocketHandleis undefined.us_listen_socket_on_server_namesignature change: all call sites updated (App.h ×2, Listener.rs),on_server_name_datazero-init incontext.c.m_onServerNameWriteBarrier slot wiring viaserver.classes.ts+wrap_handler_slot.
Extended reasoning...
Overview
This PR wires https.createServer({ SNICallback }) through to the underlying TLS listener. It touches 15 files across four languages: bun-usockets C (context.c, openssl.c, internal.h, libusockets.h) to add an opaque on_server_name_data pointer alongside the existing SNI resolver hook; bun-uws C++ (App.h) and ListenSocket.rs to update call sites for the widened signature; NodeHTTP.cpp to plumb an onServerName callback through setServerCustomOptions; server_body.rs (~90 new lines) for Server__setOnServerName and its per-handshake dispatch extern fn; server.classes.ts / mod.rs for the m_onServerName WriteBarrier slot; _http_server.ts for the onServerSNI JS handler and SNICallback option validation; internal/tls.ts for the shared unwrapSNIContext helper; Listener.rs to expose decode_sni_result and pass null for the new data arg; and a new 97-line test file.
Security risks
SNI certificate selection is on the TLS handshake path. The failure modes that matter — an invalid/absent SNICallback result must not leave the connection serving the wrong certificate, and a callback error must not leave the handshake in a half-open state — appear to be handled fail-closed: unwrapSNIContext instanceof-checks both the wrapper and the raw handle against NativeSecureContext and returns an Error for anything else, which decode_sni_result maps to abort_handshake = 1; cb(null, undefined) returns undefined → default context (matching Node). The deferred-cb case (async SNICallback) falls through to the default context because there is no resume handle on the uWS path yet — acknowledged in the PR description and by CodeRabbit as out of scope. Nothing here changes the default-cert path for servers that don't pass SNICallback.
Level of scrutiny
High. This is not a mechanical change: it introduces a new native→JS re-entry point mid-TLS-handshake (dispatch in server_body.rs calls callback.call(...) from inside BoringSSL's select-certificate callback), adds a raw *mut NewServer stashed on the C listen socket with a lifetime justified by a comment ("the listen socket cannot outlive the server that owns it"), and adds a GC-visited JSValue slot. Each of these is the kind of thing REVIEW.md's memory-safety section calls out (re-entrancy under a raw pointer, GC rooting, cross-FFI ownership). The dispatch fn does guard vm.is_shutting_down() and js_value_for_dispatch().is_none() before calling into JS, and the pattern mirrors the existing us_dispatch_server_name in Listener.rs, but the ownership chain (uWS HttpContext owns the group, server owns the listener via this.listener, on_server_name_data points back at the server) is novel enough that a maintainer familiar with the Bun.serve teardown ordering should confirm it.
Other factors
The PR has been through ~10 rounds of bot review; every prior inline finding (from both the bug-hunting system and CodeRabbit) is marked resolved, and the author folded each into follow-up commits (7538071, 05d8c2e, 05abc55, 3fdc0d1, 658fe85). Test coverage is solid for the synchronous path: certificate selection by servername, raw-handle acceptance, fall-through on cb(null, undefined), ERR_INVALID_ARG_TYPE for a non-function option, and a 5-case it.each error matrix asserting both tlsClientError.message and .reason. The one outstanding sibling-parity nit (migrating net.ts consumeSNIResult to the shared unwrapSNIContext) was explicitly deferred to a follow-up and does not change any existing behavior. Given the cross-language surface and TLS-handshake sensitivity, I'm deferring rather than approving.
|
The diff is ready for review. The fail-before/pass-after gate passed on both ASAN and release builds, all GitHub lints are green, and the automated review pass on 75d5af3 reports no remaining issues. Buildkite #88708 for 75d5af3: 190/196 jobs passed including (Builds #87329/#87368/#87404 earlier all expired with every One deferred follow-up (noted in the resolved review threads): migrating |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Do not add unsafe.
Store any_server_packed (not a typed raw pointer) in the listen socket's SNI
data slot and unpack it via a new AnyServer::from_packed (the inverse of
to_packed, moved next to it from NodeHTTPResponse.rs). The dispatch then reads
vm()/global_this()/js_value_for_dispatch()/on_server_name() through AnyServer's
existing safe accessors, dropping the raw '&*data.cast::<$T>()' deref and
pulling the extern fn out of the per-type macro so there is one copy instead
of two.
The remaining 'unsafe { &mut *this }' in the setter and the CStr::from_ptr for
the callback's hostname arg are byte-identical to the sibling
server_set_on_connection/on_client_error setters and us_dispatch_server_name
respectively.
|
73aea6a reworks the dispatch so it does not dereference a typed raw pointer. The listen socket's SNI data slot now carries the server's The remaining
If the intent is to eliminate those too (which would mean diverging from the surrounding setters' pattern or adding new safe helpers for them), happy to go further with a pointer on the preferred abstraction. |
The listen socket's SNI data slot now holds the server's encoded JSValue (the JS wrapper cell). The dispatch decodes it with JSValue::from_encoded and downcasts via the safe as_class_ref, reading vm()/global_this() from the resulting &NewServer and the callback from the m_onServerName WriteBarrier slot via the codegen'd on_server_name_get_cached. The setter uses the same safe as_class_ref for the listener read and writes the callback via js_gc_on_server_name_set, so no NewServer field is mutated and the on_server_name shadow field and the AnyServer::from_packed/on_server_name additions are dropped. That leaves two 'unsafe' in the Rust diff: the Rust 2024 #[unsafe(export_name)] attribute on the shim (same as every sibling shim), and CStr::from_ptr for the C callback's hostname arg (same as us_dispatch_server_name in Listener.rs for this C callback).
|
75d5af3 redoes this per the Slack note: the listen socket's SNI data slot holds the server's JS wrapper cell (encoded Remaining |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/server/server_body.rs`:
- Around line 3936-3941: Update the SNICallback invocation around callback.call
so it does not pass undefined as the callback argument while the uWS handshake
cannot resume. Reject the unsupported asynchronous form during registration or
route deferred completion to tlsClientError; do not allow decode_sni_result to
silently return the default certificate when the callback returns undefined.
Preserve synchronous SNICallback behavior and ensure callback(err, ctx) receives
a valid callable callback when supported.
- Around line 3968-3974: Update the listener lifecycle around the on_server_name
registration and the associated us_select_cert_cb SNI-resolution flow so
listeners remain alive until every pending TLS handshake completes, including
resolutions returning no context after stop(). Alternatively, ensure stop()
detaches and aborts all pending handshakes before closing the listener,
preventing stale us_listen_socket_t dereferences.
🪄 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: ac08b5a0-df36-4804-8cb1-9a446b2cc800
📒 Files selected for processing (3)
src/runtime/server/mod.rssrc/runtime/server/server_body.rstest/js/node/http/node-https-snicallback.test.ts
There was a problem hiding this comment.
No issues found on the 75d5af3 rework, but this spans TLS handshake dispatch across C/C++/Rust/TS with GC-lifetime reasoning (encoded JSValue stashed in the listen socket's void* data slot), so it warrants a maintainer look.
What was reviewed on 75d5af3:
us_dispatch_serve_server_name: server recovery viaas_class_refon the stashed encoded JSValue,is_shutting_downguard, callback read from them_onServerNameWriteBarrier slot — no raw shadow field remains.EventLoop::enter_scopeomission checked againstus_dispatch_server_namein Listener.rs (same select-certificate caller insideSSL_do_handshake) — matches the sibling, not a regression.decode_sni_resultreturn contract shared with the Bun.listen path;onServerSNI'sunwrapSNIContextrejects non-SecureContext values beforetruecan reach the suspend arm.
Extended reasoning...
Overview
Wires SNICallback from node:https through to the uWS listen socket's select-certificate hook. 15 files, ~300 LOC: adds a void* on_server_name_data slot to us_listen_socket_t (bun-usockets C), a Rust dispatch (us_dispatch_serve_server_name in server_body.rs) that recovers the HTTPSServer from that slot and calls the JS handler, a new onServerName WriteBarrier value slot on the server class, and the _http_server.ts glue (onServerSNI, unwrapSNIContext, toSNIError). The dispatch was reworked twice on 2026-08-04 (73aea6a → 75d5af3) based on offline maintainer direction, landing on stashing the server's encoded JSValue in the C data slot and downcasting via as_class_ref.
Security risks
This is TLS certificate selection during the handshake — a security-relevant path. The fail-closed contract looks correct: invalid contexts (unwrapSNIContext returns an Error), callback errors, and non-SecureContext returns all set *abort_handshake = 1 and drop the connection before any cert is served; cb(null, undefined) falls through to the default context. The suspend sentinel (true) cannot leak from user input because unwrapSNIContext instanceof-checks before onServerSNI returns. No trust-store or verification-mode changes.
Level of scrutiny
High. The change spans four languages with FFI boundaries, adds a GC-rooted callback slot, and stashes an encoded JSValue in a C void* that must stay live for the listen socket's lifetime. The lifetime argument (server wrapper is strongly held while listening; listener close precedes wrapper finalize) was traced in earlier review rounds and looks sound, but the 75d5af3 approach is fresh and should be confirmed by someone who owns src/runtime/server/.
Other factors
All prior inline findings from earlier revisions are resolved. The enter_scope omission was re-examined this run and matches the Listener.rs sibling for the same C caller. Buildkite #88708 for 75d5af3 was still building at review time. The author noted one deferred follow-up (migrating net.ts's consumeSNIResult to the shared unwrapSNIContext).
This branch has not been deployed
2 participants
What does this PR do?
Fixes the remaining half of #14395:
https.createServer({ SNICallback })accepted the option but never dispatched it, so every handshake served the default certificate and the callback was never reached.tls.createServer({ SNICallback })already works because it listens via theBun.listensocket path, which registers a dynamic SNI resolver on the listen socket. Thenode:httpsserver listens viaBun.serve, whose uWS listen socket had no such hook.Repro
Cause
_http_server.tsbuilt atlsconfig forBun.servefromkey/cert/ca/ciphers/minVersion/etc but droppedSNICallbackon the floor, andBun.servehad no dynamic SNI dispatch to receive it anyway: the uWS listen socket'sgroup->extbelongs to the uWSHttpContext, so the existingBun.listendispatch (which recovers its owner vials->group->ext) could not be reused as-is.Fix
Wire the same
SSL_CTX_set_select_certificate_cbhook theBun.listenpath uses into theBun.servelisten socket:us_listen_socket_on_server_namegains a user-data pointer, stored alongside the resolver on the listen socket, so a dispatch whosegroup->extbelongs to someone else (the uWSHttpContext) can still recover its owning server. The existingBun.listencaller passesNULLand keeps usinggroup->ext.NewServercarries anon_server_name: JSValue(GC-rooted via the generatedm_onServerNameWriteBarrier slot), set throughsetServerCustomOptionsalongside the existingonClientError/onConnectionhandlers. When set on an HTTPS server the setter registers the select-certificate dispatch on the live listen socket with the server as user data.decode_sni_result(nativeSecureContext/ fall-through / Error / suspend), so the return-value contract is shared with theBun.listenpath._http_server.tsvalidatesoptions.SNICallback(ERR_INVALID_ARG_TYPE when not a function), stores it asserver._SNICallback, and passes aserverNamehandler tosetServerCustomOptionsthat invokes the user callback and returns the selected context.cb(err)andcb(null, <not a SecureContext>)drop the connection before the handshake completes (no TLS alert), andcb(null, undefined)falls through to the default context, matching Node andtls.createServer.An
SNICallbackthat defers its completion callback (does not callcbsynchronously) currently falls through to the default context on thenode:httpspath: there is no resume handle for the in-flight uWS socket yet, so the handler takes the same!socketHandlefall-through the net.tsserverNamehandler already has. A synchronouscb(null, ctx)(the issue's case) is fully supported.How did you verify your code works?
New tests in
test/js/node/http/node-http.test.tsunder "node https server":dispatches SNICallback and serves the selected certificate: two handshakes against an HTTPS server whoseSNICallbackselects an alternate context foragent1andundefinedotherwise; asserts the peer CN isagent1/server-bunrespectively and that the callback was invoked with both servernames.rejects a non-function SNICallback: asserts ERR_INVALID_ARG_TYPE.Both fail on
main(CN is the default cert,callsstays[], and the non-function option was accepted) and pass with this change.test/js/node/tls/node-tls-server.test.ts -t SNICallbackandtest/js/node/test/parallel/test-tls-sni-option.jscontinue to pass.[review] gate passed · iteration 3 · 15 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 7 passed · 1 rejected · iteration 3
evidence per changed file