WebSocket: honor tls.serverName and call tls.checkServerIdentity on the client handshake
Fixes #43635
Problem
new WebSocket(url, { tls })ignorestls.serverNameand never callstls.checkServerIdentity. The SNI and the verified name are always the URL host, and a pin that returns an Error still opens.- Cause:
HTTPClient::connect(WebSocketUpgradeClient.rs) takes the name from the URL only, andJSWebSocket.cppnever reads the callback.
Fix
serverNamesets the SNI and the verified name, also inside a proxy tunnel.- The callback replaces the built-in name check. A truthy verdict or a throw closes with 1015 before the upgrade request is sent, as in
tls.connect(). CppWebSocketgives the one verdict for every TLS handshake (direct, proxy hop, tunnel, renegotiation):server_identityinside the handshake,accepts_tls_peerafter it.- Verified:
websocket-tls-server-identity.test.ts(37 tests) and theBun.ModuleGraphcallbacks test. Each fix was checked against a build without it.
Background
- The upgrade client (
HTTPClient<SSL>) calls JS from its handshake handler under aRefPtrguard, because the callback may free it. - The callback sits in a
JSValueInWrappedObject, a weak slot that the wrapper'svisitChildrenmarks. AJSC::Strong, the first design, leaks aWebSocketthat the callback captures.
Downsides
- Ignored options now take effect: a
serverNamethe certificate lacks fails with 1015, a non-function callback throws, and anasynccallback fails every connection. - The callback gets the leaf certificate only, as in
fetch(). A Node callback that walkscert.issuerCertificatethrows, and the connection fails. - A connection without either option pays 2 FFI field reads per
wss://handshake, 0 allocations and 16 bytes perWebSocket. Binary: +4,608 bytes of text on linux-x64 (0.006%, atfbcd516).
Notes
Downsides, in detail. The certificate object is the one that fetch() passes (Bun__X509__toJSLegacyEncoding on the leaf). It has no issuerCertificate. tls.connect() passes the chain: each certificate links to its issuer, and a self-signed root links to itself. The certificate pinning example in the Node https documentation walks that chain with cert = cert.issuerCertificate. Here that loop throws a TypeError, the connection closes with 1015, and the exception is reported. It fails closed. The chain builder of node:tls is in src/runtime/socket/tls_socket_functions.rs, in a crate above the WebSocket client. Open #42054 moves it to bun_jsc (tls_server_identity::peer_certificate_chain). The WebSocket client has the live SSL when the callback runs, so it can pass the chain with that helper once #42054 is on main. This PR does not copy the builder. fetch() runs its callback on the JS thread from a DER copy of the leaf, so it needs more than that helper. The JSDoc of both options names the limit. A NUL in tls.serverName is no longer a downside. #43040 is on main and in this branch, and the constructor throws ERR_INVALID_ARG_TYPE ("serverName" must not contain null bytes). An earlier version of this body said that the name is cut short, which was true before #43040. The per-connection counts are from the handshake path in the diff. The upgrade client moves the hostname out of its cell while the verdict borrows it, and moves it back after, so no path copies it. A callback can close the WebSocket, and that clears the cell. The two new FFI calls read the callback slot, once inside the handshake and once after it. A renegotiation adds one more, which reads the connection type. JSValueInWrappedObject is a JSValue and a Weak, 8 bytes each. ContextId is a u32. The upgrade client grows by those 4 bytes and is freed after the upgrade. The PR exports 2 C functions (WebSocket__checkServerIdentity, WebSocket__isProxyTLS) and no host function. Binary size was measured with two release builds on the same linux-x64 machine and toolchain: main at 6d504dd, and main plus the nine source files of this PR. That was at fbcd516. The later commits (the one-line bracket change, and the single verdict, which moves code and removes a little) were not measured again. size reports text 80,656,785 and 80,661,393 bytes, a difference of 4,608. Data (110,300) and bss (1,822,960) are unchanged, and the stripped file grows by 4,096 bytes.
One verdict. Review asked for one place that decides which name is checked, whether the callback or the built-in check decides, and whether the result is enforced. That decision was made in five places: twice in the upgrade client's handle_handshake, in verify_peer_identity, in the tunnel's on_handshake, and in the connected client's renegotiation handler. CppWebSocket::accepts_tls_peer (src/http_jsc/websocket_client/CppWebSocket.rs) now makes it once, on the Rust handle of the C++ WebSocket that owns rejectUnauthorized and the callback. The rule: a chain that did not verify is accepted only without rejectUnauthorized, and the callback never sees it. Otherwise the callback decides for the target and the built-in check decides for an HTTPS proxy. Without rejectUnauthorized the callback still runs and its verdict is dropped. On a renegotiation the callback does not run again. The upgrade client and the tunnel pass the peer of their first handshake. For a renegotiation the WebSocket names the peer itself from its connection type, because the connected client owns a socket to the target or, for a ws:// target, to the HTTPS proxy. check_server_identity() is now private to the verdict, so no other code can ask whether a callback is present. #43863 landed on main during this review. It asks the owner of a TLS client for the name check inside the handshake, before a client certificate goes out. For the WebSocket client that question now goes to the same decision: CppWebSocket::server_identity runs the built-in check when the built-in check decides, and answers Unchecked when the callback decides, so the callback still owns the name. The upgrade client and the tunnel only pass the peer and the name. One behaviour change: under rejectUnauthorized the tunnel now rejects a handshake that has no SSL handle or no name. It accepted before, and no path reaches that state today. A renegotiation inside the tunnel is not checked, as on main: the tunnel returns once the upgrade client is detached.
Mechanism details. With a proxy, the outer socket keeps the proxy host for its own SNI and name check. JSWebSocket.cpp reads a callable checkServerIdentity and sets it on the wrapper right after the wrapper is created (the AbortSignal::m_reason pattern). The upgrade client reads it from C++ at handshake time. After the callback returns, handle_handshake re-checks cpp_websocket() and the tunnel re-reads upgrade_client, because the callback may have closed the WebSocket. The wrapper is alive during the handshake through the WebSocket's pending activity. The HTTPS proxy rule matches fetch (src/http/lib.rs, is_proxy_certificate).
Report: bun 1.4.2 and canary 1.4.3. Peer observed SNI: via.fetch for fetch() and SNI: None for the WebSocket with the same tls object, and a checkServerIdentity that returns an Error never ran.
The test server is a node:tls server with SNICallback that answers one WebSocket upgrade by hand, so a test can assert the SNI the peer saw. The proxy tests clear NO_PROXY because an explicit proxy: is still subject to it.
Rejection before the upgrade. The issue asks that a rejected connection never reaches the WebSocket upgrade. The identity check runs inside the TLS handshake callback, and a rejection closes the socket there. The test server records the application data it decrypts. Each rejection test reads that record after the server saw the connection end, and expects nothing. An approving test is the control: it shows the record does hold the request (GET / HTTP/1.1) when one is sent. A separate probe on the released 1.4.3 binary covered the built-in name check: a dial to 127.0.0.2, which is loopback but not in the certificate, with an Authorization header. The peer received 0 bytes under TLS 1.2 and TLS 1.3, in 6 of 6 runs. These runs are Linux only. CI runs the same assertions on macOS and Windows.
The callback receives the same certificate object as fetch() (Bun__X509__toJSLegacyEncoding), and the callback test checks fingerprint256 and raw on it. A callback that throws rejects the connection, and the exception is reported as uncaught, as Node does for this hook. An earlier version of this PR took the exception and dropped it. That hid the user's error, mishandled a worker's termination exception, and failed the jsresult-swallow source lint on this branch from the first push. The helper now propagates with ?, and the handshake callback folds the error with report_error_or_terminate. A returned Error still surfaces only as the 1015 close and the error event, which is how the client reports every verification failure today.
The verdict. Any truthy return value rejects the server, which is how Bun's tls.connect() (src/js/node/net.ts) and Node read it. undefined, null and false approve. An async callback returns a Promise, which is never an Error, so under an "only an Error rejects" reading an async pin always approved and the upgrade request went out. The hook is Node's and its contract is Error | undefined, so other values are out of contract and fail closed. The callback never ran for a WebSocket before this PR, so nothing depends on a lax reading. fetch() had the lax reading when this rule was written. #42987 changed it on main to the same rule, so the two clients agree now. They report a rejection differently: fetch() rejects with the returned object, or with ERR_INVALID_RETURN_VALUE for a Promise or a primitive. The WebSocket closes with 1015 and the fixed message TLS handshake failed.
Other edge cases follow fetch() as it is on main today (src/runtime/webcore/fetch/FetchSession.rs, src/http/lib.rs wants_server_identity_check). A checkServerIdentity that is not a function throws ERR_INVALID_ARG_TYPE, and undefined and null are ignored. It was dropped silently before, so the built-in check ran in place of the pin the caller thought was installed. The property is read through the prototype chain, up to Object.prototype, like the other tls options. With rejectUnauthorized: false the callback still runs when the chain verified, and its verdict is not enforced. An earlier version of these notes said that fetch() skips the callback in that case. That was true when written and is not true on main now.
When the callback cannot run. Once script may not run, JSValue::call is a silent no-op that returns undefined, which is also how the callback approves. A handshake that completed while the VM was stopping (worker.terminate()) therefore approved the peer with no identity check. The verdict now counts only if script was allowed, the VM did not forbid execution, and the call reached somebody. A probe that races terminate() against handshakes whose callback always returns an Error saw the rejected peer receive the upgrade request 4, 5 and 2 times in three runs of 120 attempts, and 0 across four runs with the check. There is no unit test: dispose() closes what a graph opened, so that path already fails closed at the socket, and VM stopping is a cross-thread race.
Context. The callback runs in the context of the script that made the WebSocket, like every other callback into script. The upgrade client records the ContextId at connect(), because the context pointer behind WebSocket__bunContext is only valid there, and enters it by id at handshake time. For a Bun.ModuleGraph this means a throw reaches the graph's onError instead of the host's handler. test/js/bun/module-graph/module-graph-callbacks.test.ts gains an entry for this callback, which is how that test tracks every API that calls back into a graph.
Renegotiation. After the upgrade, the connected client (src/http_jsc/websocket_client.rs, handle_handshake) owns the socket, so that handler runs only for a renegotiation. It applied the built-in name check against the SNI. A connection the callback approved under another name, or to an IP URL (no SNI), closed with 1006 Failed to connect when a TLS 1.2 server renegotiated. The verdict now accepts a renegotiation when the callback decided for that peer. The callback does not run again: BoringSSL rejects a renegotiation that changes the certificate (vendor/boringssl/ssl/handshake.cc, ssl_verify_peer_cert, SSL_R_SERVER_CERT_CHANGED) and skips re-verifying it for that reason, so the verdict still covers the peer. The tests use a real node TLS 1.2 server, because a BoringSSL server cannot renegotiate. The same handler also drops a wss://<IP> connection that has no callback at all. That predates this PR. #43672 corrected it on main, and this branch includes it: the verdict runs that hostname check only when the built-in check decides. test/js/node/tls/renegotiation.test.ts from #43672 covers a renegotiation with no callback, also by an HTTPS proxy in front of a ws:// target, and passes on this branch.
Client certificates. One test row uses a server that asks for a client certificate, and a callback that approves a name the certificate does not have. The connection opens and the server receives the certificate, so the callback owns the name verdict there too. #43863 (it replaces #43841) added a built-in name check inside the handshake, and it landed first. With that check left as main has it, which is what a plain merge gives, 5 tests of the file fail: this row, replaces the built-in hostname check, like fetch, the HTTPS proxy row with a name that only the callback accepts, one renegotiation row and the GC row. With CppWebSocket::server_identity the whole file passes, and test/js/bun/net/tls-reject-before-client-cert.test.ts from #43863 passes unchanged (110 pass, 9 skip).
Tests that review asked for. Each was run against a build with the guarded code removed. (1) An HTTPS proxy plus tls.serverName, in two rows: the proxy sees no SNI, the target sees serverName, and the proxy's certificate is still checked against the proxy host. With the !using_proxy guard in HTTPClient::connect deleted, one row fails on the SNI the proxy saw and the other closes with 1015. startRecordingProxy now records that SNI. (2) The callback survives Bun.gc(true) before the handshake ends when only the WebSocket refers to it. The server holds the handshake at the ClientHello until the GC ran. With the visitAdditionalChildrenInGCThread marking removed, the callback is collected and the connection closes with 1015. (3) 16 WebSockets whose callback captures the WebSocket are collected after they close (at most 2 may remain). With a JSC::Strong holding the callback, all 16 stay alive. The full file passed 6 of 6 runs on the debug build.
The ws package. The shim (src/js/thirdparty/ws.js) passes its tls object to the native client unchanged, so tls.serverName and tls.checkServerIdentity apply through it. A row in websocket-tls-server-identity.test.ts covers that, with the SNI that the peer saw. The shim drops the options that ws users write at the top level, as in new WebSocket(url, { ca, checkServerIdentity }). #31397 forwards the top-level keys that SSLConfig parses. That list has servername, and it does not have checkServerIdentity. An earlier version of these notes said that #31397 covers the shim. That was wrong for the callback: with #31397 a top-level ca is forwarded, the connection opens, and a top-level checkServerIdentity is still dropped. This PR does not change the shim.
Types. checkServerIdentity and servername are declared on the WebSocket tls option, and test/integration/bun-types/fixture/websocket.ts type-checks both. servername is not added to the shared TLSOptions: the shared parser accepts it, but this PR tests only the WebSocket client, and not every TLSOptions consumer was confirmed to honor the lowercase spelling.
Other suites run on the debug build: websocket-proxy.test.ts, websocket-unix.test.ts, websocket.tls.ipv6.test.ts, websocket-client.test.ts, websocket-syscall-fault.test.ts, test-ws-bidir-proxy.test.ts, the proxy leak and reentrancy tests, ws.test.ts, ws-proxy.test.ts, test/js/node/tls/renegotiation.test.ts, test/js/bun/net/tls-reject-before-client-cert.test.ts, test/internal/source-lints/, and the types test. After the last merge of main (a0a71e2cd7, which has #42987, #43873, #43924 and #43979 in the same TLS code) these passed again on a debug build: the identity file (37), fetch.tls.test.ts (61), tls-reject-before-client-cert.test.ts (121 pass, 9 skip), renegotiation.test.ts (21), and the proxy suites. In websocket.test.js two tests need the public internet and fail in my environment, with and without this PR. should connect many times over https takes 5.0 to 5.1 s there on a debug build inside the concurrent file run, with this PR and without it (2.6 s alone), which is at the 5 s default.
Self-review. Addressed: the HTTPS proxy rule, the leaf-only JSDoc, a stale comment. Rejected: a two-PR split (both gaps are one report and share one fixture), surfacing the callback's Error message (the client reports every verify failure as 1015 today, a separate change), moving the Bun__X509__toJSLegacyEncoding extern into bun_jsc (one declaration, same pattern as parseSSLConfig in this file), and forwarding top-level options from the ws shim (see the ws paragraph above). The latest self-review of the single verdict raised 9 concerns, and 6 survived. All 6 are addressed: the renegotiation peer for a ws:// target behind an HTTPS proxy (three reports of the same point), a wait in the GC test that could hang in place of failing, one unclear comment, and multi-line comments.
Rebased on main after #30674 (IPv6 bracket stripping for SNI) and #42590 (WebSocket__bunContext). An IPv6 literal in brackets loses the brackets in the dialed host and in tls.serverName, as get_tls_hostname does for fetch(): serverName: "[::1]" verifies against the IP:::1 SAN, sends no SNI, and the callback is given "::1". Review found that the direct path used the name as given, which closed with 1015.
[human-review] gate passed · iteration 4 · 14 files touched
fails on main (without fix)
ASAN without fix: 34 failed, 13 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/integration/bun-types/bun-types.test.ts test/js/bun/module-graph/module-graph-callbacks.test.ts test/js/web/websocket/websocket-tls-server-identity.test.ts
bun test v1.4.3 (367d939d9)
test/integration/bun-types/bun-types.test.ts:
(pass) @types/bun integration test > building and packing bun-types leaves packages/bun-types untouched [4.94ms]
(pass) @types/bun integration test > packed bun-types includes CLAUDE.md [13.41ms]
(skip) @types/bun integration test > basic type checks > checks without lib.dom.d.ts
(skip) @types/bun integration test > TypeScript latest > checks without lib.dom.d.ts
(skip) @types/bun integration test > TypeScript 7.1 > checks the fixture and import attributes through ts7.1/index.d.ts
(pass) @types/bun integration test > Bun.mmap > MMapOptions accepts offset and size [2529.10ms]
(pass) @types/bun integration test > TextDecoder > accepts the encoding labels the runtime supports [1702.72ms]
(pass) @types/bun integration test > TextDecoder > the fixture label table matches the runtime [242.04ms]
(pass) @types/bu
... (truncated)
release without fix: 10 FAILED
bun test v1.4.3-canary.1 (b76cca0df)
test/integration/bun-types/bun-types.test.ts:
(pass) @types/bun integration test > building and packing bun-types leaves packages/bun-types untouched [0.09ms]
(pass) @types/bun integration test > packed bun-types includes CLAUDE.md [0.31ms]
131 | expect(emptyInterfaces).toEqual(config.emptyInterfaces);
132 |
133 | if (typeof config.diagnostics === "function") {
134 | config.diagnostics(diagnostics);
135 | } else {
136 | expect(diagnostics).toEqual(config.diagnostics);
^
error: expect(received).toEqual(expected)
- []
+ [
+ {
+ "code": 2353,
+ "line": "websocket.ts:64:7",
+ "message": "Object literal may only specify known properties, and 'checkServerIdentity' does not exist in type 'TLSOptions'.",
+ },
+ {
+ "code": 7006,
+ "line": "websocket.ts:64:27",
+ "message": "Parameter 'hostname' implicitly has an 'any' type.",
+ },
+ {
+ "code": 7006,
+ "line": "websocket.ts:64:37",
+ "message": "Parameter 'cert' implicitly has an 'any' type.",
+ },
+ {
+ "code": 2561,
+ "line": "websocket.ts:289:28",
+ "message": "Obje
... (truncated)
passes on PR (with fix)
ASAN with fix: 13 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/integration/bun-types/bun-types.test.ts test/js/bun/module-graph/module-graph-callbacks.test.ts test/js/web/websocket/websocket-tls-server-identity.test.ts
bun test v1.4.3 (367d939d9)
test/integration/bun-types/bun-types.test.ts:
(pass) @types/bun integration test > building and packing bun-types leaves packages/bun-types untouched [4.60ms]
(pass) @types/bun integration test > packed bun-types includes CLAUDE.md [48.35ms]
(skip) @types/bun integration test > basic type checks > checks without lib.dom.d.ts
(skip) @types/bun integration test > TypeScript latest > checks without lib.dom.d.ts
(skip) @types/bun integration test > TypeScript 7.1 > checks the fixture and import attributes through ts7.1/index.d.ts
(pass) @types/bun integration test > Bun.mmap > MMapOptions accepts offset and size [4041.61ms]
(pass) @types/bun integration test > TextDecoder > accepts the encoding labels the runtime supports [2843.27ms]
(pass) @types/bun integration test > TextDecoder > the fixture label table matches the runtime [75.31ms]
(pass) @types/bun
... (truncated)
release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 3278ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/118] cxx obj/unified/UnifiedSource-src_jsc_bindings-1.cpp.o
[2/118] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-3.cpp.o
[3/118] cxx obj/src/jsc/bindings/ZigGlobalObject.cpp.o
[4/118] cxx obj/unified/UnifiedSource-src_jsc_bindings-4.cpp.o
[5/118] cxx obj/unified/UnifiedSource-src_runtime_webview-0.cpp.o
[6/118] cxx obj/unified/UnifiedSource-src_uws_sys-0.cpp.o
[7/118] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-2.cpp.o
[8/118] cxx obj/unified/UnifiedSource-src_jsc_bindings-0.cpp.o
[9/118] cc obj/packages/bun-usockets/src/crypto/openssl.c.o
[10/118] cc obj/packages/bun-usockets/src/eventing/epoll_kqueue.c.o
[11/118] cxx obj/unified/UnifiedSource-src_jsc_bindings_node-0.cpp.o
[12/118] cc obj/packages/bun-usockets/src/bsd.c.o
[13/118] cc obj/packages/bun-usockets/src/socket.c.o
[14/118] gen cpp.rs (cppbind)
[15/117] cxx obj/unified/UnifiedSource-packages_bun_usockets_src_crypto-0.cpp.o
[16/117] cc obj/packages/bun-usockets/src/context.c.o
[17/117] cc obj/packages/bun-usockets/src/node
... (truncated)
diff hotspot
packages/bun-types/bun.d.ts | 21 +-
src/http_jsc/websocket_client.rs | 29 +-
src/http_jsc/websocket_client/CppWebSocket.rs | 154 +++++
.../websocket_client/WebSocketProxyTunnel.rs | 96 ++-
.../websocket_client/WebSocketUpgradeClient.rs | 177 +++--
src/jsc/bindings/webcore/JSWebSocket.cpp | 46 ++
src/jsc/bindings/webcore/JSWebSocket.h | 3 +
src/jsc/bindings/webcore/WebSocket.cpp | 10 +
src/jsc/bindings/webcore/WebSocket.h | 11 +
test/integration/bun-types/bun-types.test.ts | 22 +-
test/integration/bun-types/fixture/websocket.ts | 20 +
.../module-graph/module-graph-callbacks.test.ts | 30 +-
test/js/web/websocket/proxy-test-utils.ts | 6 +
.../websocket-tls-server-identity.test.ts | 727 +++++++++++++++++++++
14 files changed, 1220 insertions(+), 132 deletions(-)
gate history · 9 passed · 1 rejected · iteration 4
evidence per changed file
file reads edits tests
packages/bun-types/bun.d.ts 3 5 128
src/http_jsc/websocket_client.rs 1 0 127
src/http_jsc/websocket_client/CppWebSocket.rs 1 0 127
src/http_jsc/websocket_client/WebSocketProxyTunnel.rs 2 4 125
src/http_jsc/websocket_client/WebSocketUpgradeClient.rs 15 20 126
src/jsc/bindings/webcore/JSWebSocket.cpp 1 0 126
src/jsc/bindings/webcore/JSWebSocket.h 0 0 125
src/jsc/bindings/webcore/WebSocket.cpp 0 0 126
src/jsc/bindings/webcore/WebSocket.h 0 0 126
test/integration/bun-types/bun-types.test.ts 0 0 15
test/integration/bun-types/fixture/websocket.ts 0 0 127
test/js/bun/module-graph/module-graph-callbacks.test.ts 0 0 35
test/js/web/websocket/proxy-test-utils.ts 1 0 128
…/js/web/websocket/websocket-tls-server-identity.test.ts 6 10 101