node:https: make req.socket a tls.TLSSocket with getPeerCertificate() - #37255
node:https: make req.socket a tls.TLSSocket with getPeerCertificate()#37255robobun wants to merge 4 commits into
Conversation
The socket node:https hands to request handlers was a plain Socket: no getPeerCertificate(), getCipher() or getProtocol(), and instanceof tls.TLSSocket was false, so mTLS servers could not read the client identity from req.socket. Expose getPeerCertificate/getCipher/getTLSVersion on the native NodeHTTP server socket handle (sharing the BoringSSL rendering with the tls socket binding), and swap encrypted NodeHTTPServerSocket instances onto a prototype that keeps every NodeHTTPServerSocket member but chains through TLSSocket.prototype, so instanceof and the inherited TLSSocket methods (getPeerX509Certificate, setServername guards, ...) behave like Node. Fixes #37251
|
Warning Review limit reached
Next review available in: 26 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 (4)
Comment |
node-tls-server.test.ts has an environment-sensitive SNI test (binds and connects via the localhost hostname) that fails in some containers regardless of this change; a dedicated file keeps this fix's coverage isolated from it.
|
Updated 6:47 AM PT - Aug 9th, 2026
✅ @robobun, your commit 501d89309ef0826d7fa5cbf176437a25d2fea982 passed in 🧪 To try this PR locally: bunx bun-pr 37255That installs a local version of the PR into your bun-37255 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- .unwrap_or(JSValue::ZERO) is banned by the empty-jsvalue-laundering source lint; route through HostReturn::or_pending_exception instead. - net.Socket's constructor stamps an own isServer = false that shadows the prototype, so set the own property in the encrypted branch and drop the dead prototype descriptor; cover isServer in the test.
There was a problem hiding this comment.
Both findings from the earlier pass are addressed in 501d893 (or_pending_exception() and the own-property isServer), and this run found nothing new. Given the scope — new C++↔Rust FFI surface for TLS peer-cert introspection plus a runtime Object.setPrototypeOf swap on every encrypted node:https connection — a maintainer sign-off on the prototype-swap design and the partial TLSSocket method coverage would still be worthwhile.
Checked: us_socket_get_native_handle dispatches on s->ssl internally, so the single-arg extern decl matches the C signature; tls_socket_functions.rs is mod-included once (socket_body.rs only), so the new #[no_mangle] externs do not collide; the refactored peer_certificate_to_js/cipher_to_js/tls_version_to_js are behavior-preserving extractions of the existing bodies.
Extended reasoning...
Overview
Four files: _http_server.ts (encrypted NodeHTTPServerSocket gets isServer = true and its prototype swapped to a lazily-built object that chains through TLSSocket.prototype, with three overridden methods reading the native NodeHTTP handle; connectionListener's native-socket guard is folded into isNodeHTTPServerSocket), JSNodeHTTPServerSocketPrototype.cpp (three new host functions + a sslHandleFor helper calling us_socket_get_native_handle), tls_socket_functions.rs (three #[unsafe(no_mangle)] extern "C" entry points and the bodies of get_peer_certificate/get_cipher/get_tls_version extracted into *_to_js helpers taking the raw SSL*), and a new mTLS test file.
Security risks
The change exposes TLS session introspection (peer certificate, cipher, protocol) through a new FFI path. It reuses the existing X509/cipher rendering code verbatim, and sslHandleFor guards on is_ssl && socket before dereferencing, with the Rust side null-checking again. I did not find a way for user input to reach the new FFI with an invalid pointer. No auth/verification logic is altered — this is read-only introspection of state BoringSSL already computed.
Level of scrutiny
High. This is TLS-adjacent Node compat with a new C++↔Rust FFI boundary and a per-connection Object.setPrototypeOf (which deoptimizes inline caches on the swapped instance). The runtime prototype-swap approach — vs. a static subclass or defining the methods directly on NodeHTTPServerSocket.prototype behind an encrypted guard — is an architectural choice a maintainer should weigh. The prototype chains through TLSSocket.prototype, so every non-overridden TLSSocket method (getSession, exportKeyingMaterial, getFinished, renegotiate, …) is now callable on req.socket and will read a null _handle; that partial-coverage boundary is worth a human look.
Other factors
Both of my earlier inline findings (source-lint on unwrap_or(JSValue::ZERO); dead isServer on the prototype) were fixed in 501d893 and the test now asserts isServer: true. I confirmed tls_socket_functions.rs is only included once (the in-file comment about dual inclusion is stale but pre-existing), so the new #[no_mangle] symbols are unique. The Rust refactor is a clean extraction — the pre-existing callers wrap the new helpers with Ok(...) and behavior is unchanged. Test coverage is solid for the primary fix (instanceof, getPeerCertificate, getPeerX509Certificate, getCipher, getProtocol, authorized) and includes a tls.createServer control.
|
Overlap check against #35535 (cirospaciari, opened 2026-07-25), which also makes encrypted Duplicated by #35535:
Not in #35535:
Sequencing: |
1 participant
Fixes #16834 (#37251, which this PR was opened for, was closed as a duplicate of it)
Repro
Node prints
TLSSocket true function; Bun printedSocket false undefined, so an mTLS server had no way to read the client certificate from the request handler (req.socket.getPeerCertificate()threwTypeError: s.getPeerCertificate is not a function).Cause
node:httpsrequest sockets areNodeHTTPServerSocketinstances (backed by the native NodeHTTP handle,class Socket extends net.Socket). The handle exposed the verification verdict (peerCertVerified,authorizationError) but not the certificate itself, and the JS class never chained throughtls.TLSSocket.prototype, so both the methods and theinstanceofcontract were missing. Onlytls.createServerproduced a realTLSSocket.Fix
getPeerCertificate(abbreviated),getCipher()andgetTLSVersion()to the NodeHTTP server socket handle (JSNodeHTTPServerSocketPrototype.cpp), rendered by the same Rust/BoringSSL code thetlssocket binding uses (tls_socket_functions.rs, refactored into shared helpers taking theSSL*).NodeHTTPServerSocketinstances get a prototype that keeps everyNodeHTTPServerSocketmember as own properties but whose chain goes throughTLSSocket.prototypebeforenet.Socket.prototype.instanceof tls.TLSSocketandinstanceof net.Socketare both true,getPeerCertificate/getCipher/getProtocolread the NodeHTTP handle, and inherited genericTLSSocketmethods (getPeerX509Certificate, thesetServernameserver guard, ...) work unchanged. Plain HTTP sockets are untouched.Verification
New test
test/js/node/http/node-https-req-socket-tls.test.ts("https req.socket is a TLSSocket that exposes the client certificate") coversinstanceof,constructor.name,authorized, leaf and detailedgetPeerCertificate().subject.CN,getPeerX509Certificate().subject,getCipher().nameandgetProtocol(). It fails on current bun with the TypeError above and passes with this change. The issue's repro script now prints the same line as Node v26:Also ran the full
node-tls-server,node-tls-connect,node-http,node-http-connect,node-http-with-ws,node-http-server-timeoutsandnode-https-checkServerIdentitysuites; the only failures are pre-existing on main (external-network proxy test, an SNI bind-hostname test, and a debug-build 5s-timeout flake innode-http-connect, all reproduced on an unmodified build).Note: the issue also mentions that the certificate object omits
modulus/exponentfor RSA keys on thetls.createServerpath; that is a separate gap in the shared X509 rendering and is not addressed here.Relationship to #35535
#35535 (opened earlier) also makes encrypted server sockets pass
instanceof tls.TLSSocket, so the JS prototype splice here duplicates that part of it. The rest of this PR is not in #35535:getPeerCertificate/getCipher/getTLSVersionaccessors on the NodeHTTP server socket handle.TLSSocket.prototype.getPeerCertificatereadsthis._handle, which isnullon these sockets, so with the splice alonereq.socket.getPeerCertificate()returnsnull(getCipher()undefined,getProtocol()null) and an mTLS server still cannot read the client certificate;isServer = trueon encrypted sockets;connectionListenerrecognizing the re-prototyped sockets. Itsinstanceof NodeHTTPServerSocketguard (http/http2: node v26.3.0 compat — HTTP/1 fallback + upgrade handoff, http2 session errors, perf_hooks and frame framing (+11 upstream tests) #34432) postdates node: tls/https v26 compat wave 2 — allowHalfOpen close_notify, fetch setDefaultCACertificates, https.Server setSecureContext/keylog/TLSSocket (+6 tests) #35535's base and is failed by any socket whose chain is built onTLSSocket.prototype.The two branches conflict in one place, the prototype construction after
Object.defineProperty(NodeHTTPServerSocket, "name", ...)in_http_server.ts. This PR should land after #35535: at that point the splice here gets dropped, the three methods move into #35535's descriptor object, and the native accessors,isServer, theconnectionListenerchange and the test stay as they are.[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
The root cause was that node:https built request sockets as plain net.Socket instances, never switching them to the TLSSocket prototype even when the connection was encrypted, so TLS methods like getPeerCertificate were unavailable to handlers. The fix swaps the socket's prototype to a TLS-aware server socket prototype when the connection is encrypted and wires the underlying TLS handle through to the native peer certificate, cipher, and protocol accessors, making req.socket an instanceof tls.TLSSocket with a working getPeerCertificate. It also sets isServer to true as an own property on en…