Bun.listen: report socket.authorized from the client-cert verify result#33755
Bun.listen: report socket.authorized from the client-cert verify result#33755robobun wants to merge 5 commits into
Conversation
On a TLS server accepting a connection, socket.authorized was computed purely from whether the TLS handshake completed (success == 1). The native verify callback always continues the handshake and defers the decision, so a client certificate signed by an untrusted CA still reported socket.authorized === true while getAuthorizationError() returned a non-null error at the same time. Fold the BoringSSL verify result into the AUTHORIZED flag for server sockets so the getter matches the verification outcome. The success value handed to the handshake callback is left unchanged: node:tls's server handler treats a false second argument as "TLS session never established".
|
Updated 10:26 AM PT - Jul 8th, 2026
❌ @robobun, your commit 02a2433 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 33755That installs a local version of the PR into your bun-33755 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Thanks, but I'm going to hold off on the
So this change shouldn't alter either of those paths. They may be worth fixing, but I'd rather verify each against its own layer than auto-close them from here. |
|
Warning Review limit reached
Next review available in: 19 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 (1)
WalkthroughThis PR updates TLS handshake authorization so server-side ChangesTLS client-certificate authorization fix
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Broaden the fix from server sockets only to every TLS socket: the
AUTHORIZED flag now requires both a completed handshake and a zero
verify error. This also covers upgradeTLS({ isServer: true }) server
sockets (whose Handlers mode is deliberately Client) and Bun.connect
clients, which previously reported socket.authorized === true while
getAuthorizationError() returned a non-null X509 error. A plain TLS
server that never requests a client certificate now reports
authorized === false, matching node:tls's TLSSocket#authorized. The
handshake callback's success argument is unchanged.
Add tests for a plain (no requestCert) server and assert the callback
success argument stays true on a verify failure.
Add a client-side test: a Bun.connect TLS client to a server whose certificate does not chain to any trusted CA (but whose hostname matches) reports socket.authorized === false. This pins the client branch of the verify-result fold so a future narrowing back to server-only sockets does not pass green.
There was a problem hiding this comment.
Thanks for addressing all the review feedback — the change looks correct and well-tested now. Deferring to a human because this is TLS peer-authorization logic and it intentionally flips socket.authorized on the two most common configs (plain Bun.listen servers without requestCert, and Bun.connect clients to untrusted-but-hostname-matching servers), which is worth a maintainer sign-off.
Extended reasoning...
Overview
Two files changed. The native change is 9 lines in NewSocket::on_handshake (src/runtime/socket/socket_body.rs:1688-1698): a new local authorized_flag = authorized && ssl_error.error_no == 0 (for SSL sockets) drives Flags::AUTHORIZED, while the callback's second argument still receives the raw authorized derived from success == 1. The rest is ~250 lines of new tests in test/js/bun/net/socket.test.ts covering four cases: untrusted client cert, trusted client cert, plain server without requestCert, and a Bun.connect client to an untrusted server.
All five of my earlier inline comments were addressed across e700c4b and c0fc674: the gate was widened from handlers.mode.is_server() to all SSL sockets (covering upgradeTLS({isServer:true}) and Bun.connect clients), the no-requestCert behavior flip is now tested and called out in the PR description, server close/error handlers reject the awaited promise, result.success is asserted to lock in the callback-arg invariant, and a Bun.connect client test pins the client-side flip.
Security risks
This is a security fix — it can only turn a spurious authorized === true into false, never the reverse. The node:tls/node:https layers compute their own .authorized in JS from the callback's verifyError and are unaffected. The callback's success argument is deliberately unchanged so ServerHandlers.handshake in src/js/node/net.ts continues to distinguish "handshake failed" from "verify failed". I don't see a way this weakens any check.
Level of scrutiny
High. This is the native TLS peer-certificate authorization flag — exactly the kind of auth/crypto path the approval guidelines exclude from bot approval. Beyond that, the PR intentionally changes user-observable behavior on the two most common Bun.listen/Bun.connect TLS configurations: (1) a plain Bun.listen({tls:{key,cert}}) server now reports socket.authorized === false on every accepted connection (previously true), and (2) a Bun.connect client to a server whose cert doesn't chain to a trusted CA but whose hostname matches now reports authorized === false (previously true). Both flips are correct and match Node.js semantics, but they're default-config behavior changes that a maintainer should consciously accept.
Other factors
The 9-line native diff is mechanically simple and I'm confident it's correct. Test coverage is thorough (positive, negative, no-request, client-side). CI build #70505 was still building when I last checked the timeline. No CODEOWNERS entry covers this path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/bun/net/socket.test.ts (1)
1944-1952: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject the awaited promise on pre-handshake closes.
Both promises wait for a handshake callback, but a clean close before that callback can leave the test hanging. Wire
closetorejecton the client paths, and reject from the helper’s Node TLS client too.Proposed fix
const client = nodeTls.connect({ host: "127.0.0.1", port: server.port, ...(clientCert ? { key: clientCert.key, cert: clientCert.cert } : {}), ca: CA_CRT, servername: "localhost", rejectUnauthorized: false, }); client.on("error", reject); + client.on("close", () => reject(new Error("client closed before server handshake")));- socket: { open() {}, data() {}, close() {}, error() {} }, + socket: { + open() {}, + data() {}, + close() { + reject(new Error("server socket closed before client handshake")); + }, + error(_socket, err) { + reject(err); + }, + },- close() {}, + close() { + reject(new Error("client socket closed before handshake")); + },As per coding guidelines, “Wire EVERY failure event (
error,close,abort, process exit) to reject the awaited promise.”Also applies to: 1995-2017
🤖 Prompt for 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. In `@test/js/bun/net/socket.test.ts` around lines 1944 - 1952, The TLS client promise can hang if the socket closes before the handshake callback fires, because only the error path currently rejects. Update the client setup around nodeTls.connect to reject on close as well as error, and apply the same rejection wiring in the helper used by the Node TLS client and the other affected client paths referenced by the test block so every pre-handshake failure settles the awaited promise.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/bun/net/socket.test.ts`:
- Around line 1944-1952: The TLS client promise can hang if the socket closes
before the handshake callback fires, because only the error path currently
rejects. Update the client setup around nodeTls.connect to reject on close as
well as error, and apply the same rejection wiring in the helper used by the
Node TLS client and the other affected client paths referenced by the test block
so every pre-handshake failure settles the awaited promise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0947af7-6f14-4cca-976b-8694a0299702
📒 Files selected for processing (2)
src/runtime/socket/socket_body.rstest/js/bun/net/socket.test.ts
|
On the out-of-scope warning: the broadening from Bun.listen-only to all TLS sockets was a deliberate response to review feedback on this PR. The initial commit gated the fix on It is a net security improvement only: the flag can flip from a spurious |
Wire the server/client close and error handlers in the new TLS authorization tests to reject the awaited promise, so a clean close before the handshake callback fires surfaces as a failure instead of hanging to the runner timeout.
|
CI status: the failing jobs are all on the Windows 2019 x64 lane and unrelated to this diff:
This PR only touches |
There was a problem hiding this comment.
All prior feedback has been addressed and the change looks correct to me — deferring to a human since this shifts socket.authorized semantics on every TLS socket (including the documented flip to false for plain servers and Bun.connect clients), which is worth a maintainer's explicit sign-off.
Extended reasoning...
Overview
The PR changes NewSocket::on_handshake in src/runtime/socket/socket_body.rs to fold the BoringSSL peer-certificate verify result (ssl_error.error_no == 0) into the Flags::AUTHORIZED bit that backs socket.authorized, for all TLS sockets. Previously the flag reflected only whether the TLS handshake completed. The callback's second argument is deliberately left unchanged so node:tls's server handler is unaffected. Four new tests in test/js/bun/net/socket.test.ts cover: untrusted-CA client cert → authorized=false, trusted-CA client cert → authorized=true, plain server without requestCert → authorized=false, and a Bun.connect client to an untrusted server → authorized=false.
Over two prior review rounds I raised (a) the handlers.mode.is_server() gate missing upgradeTLS({isServer:true}) and Bun.connect clients, (b) the undocumented flip for plain servers, (c) wiring server close/error to reject, (d) asserting result.success, and (e) locking in the client-side flip with a test. All five were addressed in e700c4b, c0fc674, and 02a2433; every thread is resolved and the bug hunter found nothing this run.
Security risks
This is squarely security-sensitive: it changes what socket.authorized reports on a TLS socket, which is the value user code checks to decide whether to trust a peer. The change moves the getter in the safe direction (it now reports false where it previously misreported true for unverified peers), so it does not introduce a bypass. The node:tls compat layer computes its own .authorized in JS and is unaffected. The residual risk is compat: existing Bun.listen/Bun.connect user code that read socket.authorized on a plain server or on a client to a self-signed server will now see false instead of true.
Level of scrutiny
High. The native diff is only 9 lines and I've read the surrounding on_handshake body in full — the split between authorized (passed to the callback at line 1765) and authorized_flag (drives the getter) is clean and correct. But per the approval guidelines, changes to TLS/auth semantics warrant human review regardless of size, and this one carries a documented user-visible behavior flip.
Other factors
Test coverage is solid: positive/negative server-side cases, the plain-server flip, the client-side flip, and the success === true invariant are all pinned. No CODEOWNERS entry matches these paths. I'm not approving solely because this is a security-relevant behavior change that a maintainer should explicitly ack; on the code itself I have no remaining concerns.
1 participant
What
A
Bun.listenTLS server created withrequestCert: truereportssocket.authorized === truefor a client certificate signed by a CA it never trusted, whilegetAuthorizationError()returns a non-null error (unable to verify the first certificate) at the same time. That is the exact combination thegetAuthorizationError()doc says cannot happen, and it leaves a server handing application data to a client that never proved its identity.Fixes #33754.
Repro
Cause
In
NewSocket::on_handshake(src/runtime/socket/socket_body.rs), theAUTHORIZEDflag was set purely fromsuccess == 1, i.e. whether the TLS handshake completed. The native verify callback (us_verify_callback) always continues the handshake and defers the verdict to the layer above, so the certificate verification result was never folded intosocket.authorized. The only adjustment below that point is a hostname check gated on!is_server(), which never ran for server sockets.Fix
Fold the BoringSSL verify result into the
AUTHORIZEDflag for every TLS socket, so thesocket.authorizedgetter reflects whether the peer certificate actually verified. The flag now requires both a completed handshake and a zero verify error.This covers the whole class, not just the
Bun.listenserver in the report:upgradeTLS({ isServer: true })server sockets, whoseHandlersmode is deliberatelyClient, so a server-only gate would have missed them.Bun.connectTLS clients, which had the sameauthorized === true+ non-nullgetAuthorizationError()contradiction for an untrusted/self-signed server cert with a matching hostname.The
successvalue passed to thehandshakecallback is intentionally left unchanged:node:tls's server handler (ServerHandlers.handshakeinsrc/js/node/net.ts) treats a false second argument as "the TLS session was never established" and would otherwise misreport a verify failure as a handshake failure (and drop a connection thatrejectUnauthorized: falseis supposed to admit).node:tls/node:httpsare unaffected either way: they compute their own.authorizedin JS from the verify error.Behavior change
A plain
Bun.listenTLS server that never requests a client certificate ({ key, cert }, noca, norequestCert) now reportssocket.authorized === false(it wastruebefore). No client certificate is verified, so the connection is not authorized even though the handshake completes. This matches Node'stls.TLSSocket#authorized, and resolves the pre-existing inconsistency whereauthorizedwastruewhilegetAuthorizationError()was already non-null.Verification
New tests in
test/js/bun/net/socket.test.tsstart a realBun.listenTLS server and connect withnode:tls:socket.authorized === false,getAuthorizationError()non-null, and the callback'ssuccessarg staystruesocket.authorized === true, no errorrequestCert=> handshake completes (success === true) butsocket.authorized === falseFails before the change (
authorized === truefor the rogue cert), passes after.