tls: hold the handshake's final flight until on_handshake decides by robobun · Pull Request #35378 · oven-sh/bun

robobun · 2026-07-24T04:18:34Z

Problem

A tls.connect() client whose server-certificate verification fails (rejectUnauthorized: true with an untrusted cert) tears the connection down from its on_handshake callback, but the server still sees a completed handshake and emits 'secureConnection' for a socket that is about to close. Node's server emits 'tlsClientError' (ECONNRESET) and never 'secureConnection'.

// node: ["client-error","UNABLE_TO_VERIFY_LEAF_SIGNATURE"],["client-close",true],["tlsClientError","ECONNRESET"]
// bun:  ["client-error","UNABLE_TO_VERIFY_LEAF_SIGNATURE"],["client-close",true],["secureConnection",false]
const tls = require('tls');
const s = tls.createServer({key, cert});
s.on('secureConnection', () => ev.push('secureConnection'));
s.on('tlsClientError', e => ev.push(['tlsClientError', e.code]));
s.listen(0, () => tls.connect(s.address().port) /* no ca, rejectUnauthorized defaults to true */);

This is a client-side bug: a Bun server talking to a Node client already behaves correctly (tlsClientError), and a Node server talking to a Bun client sees the spurious secureConnection. It is TLS-1.3-specific; under TLS 1.2 the server's handshake completes before the client's by protocol design and both runtimes emit secureConnection there.

It surfaces as test/js/node/test/parallel/test-tls-close-error.js passing only because its common.mustNotCall() secureConnection handler's assertion throw is silently routed through the native socket error handler. That blocks wrapping server.emit('secureConnection', ...) for uncaughtException routing (the TLS sibling of the 'connection' wrap in #35347, deferred there and in #29762).

Cause

Bun's SSL BIO (BIO_s_custom_write) writes handshake records straight to the socket fd via us_socket_raw_write. When SSL_read processes the server's flight and seals the client's Finished, those bytes are already in the kernel send buffer by the time on_handshake dispatches. self.destroy(verifyError) in SocketHandlers.handshake cannot recall them; the server receives Finished and its handshake completes.

Node's TLSWrap writes handshake records into a memory BIO (enc_out_) and runs JS from the SSL_CB_HANDSHAKE_DONE info callback inside SSL_do_handshake, before EncOut() copies enc_out_ to the underlying stream. destroy() there drops the buffer and the server never sees Finished.

Fix

packages/bun-usockets/src/crypto/openssl.c: batch the BIO output around SSL_do_handshake and SSL_read while the handshake is pending. When the handshake completes, ssl_trigger_handshake_success parks the batched flight in the loop's spill slot with a new ssl_spill_discard_on_close flag set, then dispatches on_handshake(success=1):

A close from inside the callback frees the spill without the last-chance drain (and marks the SSL fatal so no close_notify is sent under keys the peer has not installed). The peer sees the TCP close mid-handshake and reports ECONNRESET, matching Node.
A us_internal_ssl_write from inside the callback drains the spill first, so the held flight precedes any application data when the connection is kept.
If the callback returns without closing, the flag is cleared and the spill drains normally.

When the spill slot is already occupied by another socket's partial write, the batch is flushed immediately (the pre-change behaviour) rather than held.

Verification

New test in test/js/node/tls/node-tls-server.test.ts records the exact server-side event sequence for both rejectUnauthorized: true (must see ['tlsClientError','ECONNRESET'], no secureConnection) and rejectUnauthorized: false (must see ['secureConnection', false] and the client's secureConnect). Output matches Node v26.3.0 exactly; the reject case fails on main with ['secureConnection', false] in place of tlsClientError.

Regression sweeps, failures match main:

test/js/node/test/parallel/test-tls-*.js: 172/175 pass, no diff vs main
test/js/node/test/parallel/test-https-*.js: 58/59, no diff
test/js/node/test/parallel/test-http2-*.js (first 30): no diff
test/js/node/http2/node-http2.test.js: 307 pass / 0 fail
test/js/node/tls/*.test.ts, test/js/bun/net/socket.test.ts: only pre-existing failures
test/js/node/test/parallel/test-tls-close-error.js: continues to pass (now for the right reason)

Bun's SSL BIO writes handshake records straight to the socket fd, so by the time the on_handshake callback runs the client's Finished is already in the kernel send buffer. A tls.connect() whose rejectUnauthorized check then destroys the socket cannot recall those bytes, and the server's handshake completes: it emits 'secureConnection' for a connection that is about to be torn down. Node's TLSWrap seals records into a memory BIO and runs JS from the SSL_CB_HANDSHAKE_DONE info callback before EncOut copies enc_out_ to the TCP stream, so destroy() there drops the flight and the server reports tlsClientError ECONNRESET instead. Batch the BIO output around SSL_do_handshake / SSL_read while the handshake is pending, and when it completes park the batched flight in the spill slot with a discard-on-close flag before dispatching on_handshake(success=1). A close from inside the callback frees the spill without draining (and marks the SSL fatal so no close_notify leaks under keys the peer has not installed); any write the callback issues drains it first so wire order is kept when the connection is accepted. Unblocks wrapping the server 'secureConnection' emit for uncaughtException routing (test-tls-close-error.js's mustNotCall handler was being hit and its assertion swallowed by the native error path).

robobun · 2026-07-24T04:18:45Z

^{Updated 2:45 AM PT - Jul 24th, 2026}

✅ @robobun, your commit 7beee20b03f83d5185027806d34839ad749b222d passed in Build #79417! 🎉

🧪 To try this PR locally:

bunx bun-pr 35378

That installs a local version of the PR into your bun-35378 executable, so you can run:

bun-35378 --bun

robobun · 2026-07-24T04:19:36Z

Reproduced on main with tls.connect(port) (no ca) against tls.createServer: server emits secureConnection instead of tlsClientError. Fixed by holding the client's final handshake flight until the on_handshake callback decides; node server now sees ECONNRESET like with a Node client. New test in test/js/node/tls/node-tls-server.test.ts fails on main, passes here.

Self-review findings addressed:

f89a705: handshake batching gated on !ssl_spill_owner (match us_internal_ssl_write), so a partial flush cannot overwrite another socket's pending spill.
4b07675: us_internal_ssl_loop_state_save saves+zeros ssl_write_batching across SNI/ALPN in-handshake JS, so a close on a different socket from inside the callback writes through per-record instead of into the handshaking socket's batch.
c165dba: test awaits the server terminal event rather than tick-counting.
7beee20: ssl_flush_write_batch refuses to overwrite a live spill (defensive sink-side check for the TOCTOU between the entry-time gate and a re-entrant JS region producing one).

coderabbitai · 2026-07-24T04:25:02Z

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 17 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 084ed6df-8bbd-4901-aae4-fbc211f573ca

📥 Commits

Reviewing files that changed from the base of the PR and between dc9b5c0 and 7beee20.

📒 Files selected for processing (5)

packages/bun-usockets/src/crypto/openssl.c
packages/bun-usockets/src/internal/internal.h
src/runtime/socket/socket_body.rs
src/runtime/socket/tls_socket_functions.rs
test/js/node/tls/node-tls-server.test.ts

Walkthrough

Changes

The TLS OpenSSL path now batches handshake-final ciphertext through callback and close handling, preserving correct on_handshake success sequencing. Spill ownership and release behavior were updated, and a regression test covers certificate verification rejection and acceptance.

TLS handshake batching

Layer / File(s)	Summary
Spill ownership and discard state `packages/bun-usockets/src/crypto/openssl.c`	Adds discard-on-close tracking and updates spill flush, drain, and release behavior.
Handshake callback and close coordination `packages/bun-usockets/src/crypto/openssl.c`	Parks the final handshake flight during success callbacks and prevents matching close paths from draining it.
Handshake batching integration and regression test `packages/bun-usockets/src/crypto/openssl.c`, `test/js/node/tls/node-tls-server.test.ts`	Integrates handshake-success batching into handshake and read processing, with coverage for rejected and accepted client verification.

Possibly related issues

Bun.listen() does not enforce rejectUnauthorized, and socket.authorized reports true for a certificate that failed verification #33754 — Covers TLS certificate-verification rejection and server-side handshake behavior addressed by this change.

Possibly related PRs

oven-sh/bun#33755 — Changes TLS on_handshake authorization and rejection handling related to this callback path.
oven-sh/bun#34510 — Updates the same OpenSSL spill and deferred-close handling paths.
oven-sh/bun#34598 — Adjusts OpenSSL handshake verification rejection and final-flight spill semantics.

Suggested reviewers: cirospaciari, jarred-sumner

🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR does not address linked issue `#39` about Node.js build output; it instead changes TLS handshake behavior.	Either retarget the PR to the TLS bug or add the build-output changes required by `#39`, including require, node:* externals, parallelization, or CommonJS output.
Out of Scope Changes check	⚠️ Warning	The TLS handshake changes and test are unrelated to the linked Node.js build-output objective, so they are out of scope.	Remove the TLS changes from this PR or update the issue scope so it matches the handshake fix and test.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the TLS handshake batching fix and is specific to the main change.
Description check	✅ Passed	The description covers the problem, fix, and verification, so it is mostly complete despite different section headings.

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

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 `@packages/bun-usockets/src/crypto/openssl.c`:
- Around line 1619-1626: Update ssl_trigger_handshake_success so
ssl_flush_write_batch is only called when loop_ssl_data exists, has a nonzero
ssl_write_batch_len, and has no ssl_spill_owner. When another socket owns the
spill slot, leave the write batch pending and avoid triggering a flush that
could overwrite it.

🪄 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: 9c4132d0-2f29-48a9-b53b-a125264663f3

📥 Commits

Reviewing files that changed from the base of the PR and between d978f40 and dc9b5c0.

📒 Files selected for processing (2)

packages/bun-usockets/src/crypto/openssl.c
test/js/node/tls/node-tls-server.test.ts

github-actions · 2026-07-24T04:26:03Z

This PR may be a duplicate of:

tls: close_notify on end(), injected-socket upgrades, reject-handshake wire fix, duplex data-loss, SNI, ALPN (+14 tests, tls 81%→86%) #34598 - Both modify openssl.c handshake functions to suppress the client's TLS Finished flight when certificate verification fails, preventing the server from emitting secureConnection instead of tlsClientError. tls: close_notify on end(), injected-socket upgrades, reject-handshake wire fix, duplex data-loss, SNI, ALPN (+14 tests, tls 81%→86%) #34598 uses an inline-reject BIO suppression mechanism while this PR uses ssl_spill hold-and-discard.

🤖 Generated with Claude Code

Matches us_internal_ssl_write: when another socket's partial write occupies the spill, a later ssl_flush_write_batch short-write would overwrite it. With the gate the hold is best-effort (writes through per record when spill is busy, the pre-change behaviour) and the spill cannot be clobbered.

robobun · 2026-07-24T04:29:32Z

Re the duplicate check for #34598: that PR addresses the same test-tls-close-error.js symptom as part of a much larger TLS overhaul, via a BIO-level swallow keyed on the native X509 verify result (us_ssl_inline_reject_tripped). The approach here instead holds the final flight across the JS on_handshake callback, so it also covers rejections decided in JS after the native verify succeeds, notably checkServerIdentity (hostname mismatch on a trusted cert). With this branch:

// ca trusted, servername: 'not-agent1'
node: ['client-error','ERR_TLS_CERT_ALTNAME_INVALID'],['client-close',true],['tlsClientError','ECONNRESET']
bun:  ['client-error','ERR_TLS_CERT_ALTNAME_INVALID'],['client-close',true],['tlsClientError','ECONNRESET']

The two PRs will conflict in openssl.c; happy to rebase whichever way makes sense.

The handshake batching flag is now on while the SNI/ALPN select-cert callbacks dispatch user JS. us_internal_ssl_loop_state_save already saves ssl_socket and the read window for this re-entrancy; extend it to also save ssl_write_batching and zero it for the callback's duration, so a close_notify or write on a different socket from inside the callback reaches that socket's fd via the per-record BIO path instead of being appended to the interrupted handshake's batch buffer and later flushed to the wrong peer. Widens the save array to 6 slots at both call sites.

The reject/allow subprocess now resolves each dial on both the client close and the server's tlsClientError/secureConnection, so the positive tlsClientError assertion is gated on the event itself rather than two setImmediate ticks. Moves bunExe/bunEnv to the existing top-level harness import.

The entry-time !ssl_spill_owner gate on handshake batching is a snapshot: SNI/ALPN JS (now running with batching zeroed) can issue a write on another socket that short-writes into the spill slot, and the outer flush would then overwrite that spill on its own short-write. Guard at the sink: when the slot is already taken, mark this socket fatal (its sealed remainder is undeliverable) rather than clobbering the other socket's pending ciphertext. Same disposition as the existing out-of-memory branch immediately below.

claude

All three inline findings from earlier passes are addressed (f89a705, 4b07675, 7beee20) and this run found nothing new. Deferring to a human anyway: this reworks the TLS handshake write-ordering in openssl.c — per-loop batch/spill state, re-entrancy across SNI/ALPN JS, and a new discard-on-close mode — and it took three iterations to close the cross-socket spill-clobber gaps, so a maintainer should sign off on the final shape. There's also the overlap with #34598 to resolve.

What was reviewed:

ssl_flush_write_batch now refuses to overwrite a live spill (7beee20) — verified this covers the ssl_trigger_handshake_success fallback and the hs_batching flushes in us_internal_ssl_on_data.
us_internal_ssl_loop_state_save/restore widened to 6 slots on both call sites (openssl.c us_select_cert_cb and Rust ALPN in socket_body.rs); ssl_write_batching is saved+zeroed so cross-socket close_notify from inside SNI/ALPN goes per-record.
ssl_release_spill discard-on-close path sets ssl_fatal_error so no close_notify goes out under uninstalled keys; us_internal_ssl_close FAST_SHUTDOWN skips the deferred-close-after-spill when the spill is a held flight.
New test awaits the server terminal event (secureConnection/tlsClientError) and client close, no sleeps.

Extended reasoning...

Overview

The PR changes how Bun's TLS client emits its final handshake flight: instead of writing handshake records straight to the socket fd via BIO_s_custom_write, it batches them around SSL_do_handshake/SSL_read and parks the batched flight in the loop's spill slot with a new ssl_spill_discard_on_close flag before dispatching on_handshake(success=1). If the JS callback destroys the socket (verify-reject), the spill is freed without draining and ssl_fatal_error is set so no close_notify follows; otherwise the spill drains normally after the callback returns. Files touched: packages/bun-usockets/src/crypto/openssl.c (core change), internal.h + socket_body.rs + tls_socket_functions.rs (widen the SNI/ALPN save/restore array from 5→6 slots), and a new subprocess test in node-tls-server.test.ts.

Security risks

This is TLS handshake wire-ordering and per-loop shared mutable state (loop_ssl_data->ssl_write_batch*, ssl_spill*) in C. The three earlier review rounds each found a real memory-safety-class bug in exactly this area — spill overwrite/leak when another socket owns the slot, cross-socket batch contamination via SNI/ALPN re-entrancy, and a TOCTOU between the entry-time gate and re-entrant JS producing a spill. All are now closed (the last defensively at the sink), and I don't see a fourth, but the interaction surface is large enough that I'm not confident a bot review is sufficient.

Level of scrutiny

High. openssl.c is the shared BIO/write path for every TLS socket on the loop; the new state (ssl_spill_discard_on_close) and the batching-around-handshake behaviour are visible to every SSL write, close, and shutdown path. REVIEW.md flags native memory safety as the most-blocked category and specifically calls out "anything that can run user JS can synchronously free your state — re-validate liveness guards after every callback," which is precisely the class of bug the follow-up commits fixed.

Other factors

The duplicate-PR bot flagged #34598, which addresses the same symptom via a different mechanism (BIO-level swallow keyed on X509 verify result). The author noted this PR also covers JS-side rejections like checkServerIdentity and offered to rebase either way — a maintainer should decide which approach lands.
Regression sweeps in the description look thorough (test-tls-, test-https-, test-http2-*, node-http2.test.js) with no new failures vs main.
The new test asserts the exact event sequence for both rejectUnauthorized: true and false, awaits real terminal events, and would fail on main.

…ect write in one segment (#40655) ### Problem - A TLS 1.3 `tls.connect()` client whose certificate the server rejects reports `error: read ECONNRESET` and `close(true)`. A Node client in the same exchange reports a clean `end` and `close(false)` (#40653, reproduced 5/5 on Linux, 20/20 by the reporter on macOS). - The client sends its final handshake flight and the write from `secureConnect` as two TCP segments: the custom BIO (`BIO_s_custom_write`, `packages/bun-usockets/src/crypto/openssl.c`) raw-writes the flight from inside `SSL_read`, before the handshake event reaches JS. The server rejects the certificate after reading segment one and destroys the socket with segment two unread. Linux then sends an RST instead of a FIN, and the RST is a real ECONNRESET at the client. ### Fix - Batch the records `SSL_read` seals while the handshake is pending. When the handshake completes in that read, hold the batch across the `on_handshake` dispatch. A write the callback issues appends to the held flight, and the existing flush in `us_internal_ssl_write` sends flight plus first application data in one kernel write. This is the segment shape Node produces through its memory BIO. - Any other outcome of the read flushes the batch at once. The close and shutdown paths flush a held batch before their close_notify/FIN. An error teardown or a destroy from inside the handshake drops it. A new `ssl_write_batch_owner` field pins the shared per-loop batch to one socket, and the loop-state save/zero keeps re-entrant JS (SNI/ALPN callbacks) from mixing another socket's records into a held flight. The snapshot size is now exported and debug-asserted at the Rust call site. - Verified: four new tests in `test/js/node/tls/node-tls-connect.test.ts`. The segment-count test fails on main (3 chunks, expected 2). The cross-socket-write and end-from-handshake tests cover the shared-slot hard paths. The issue's script prints `secureConnect -> write -> end -> close:false` 10/10 with the debug build. Also ran `test/js/node/tls/`, `node-http2.test.js`, `fetch.tls.test.ts`, `bun-serve-ssl.test.ts`, and the upstream `test-tls-*`/`test-https-*` suites. ### Background - usockets gives every TLS socket on a loop the same BIO pair. `loop_ssl_data->ssl_socket` routes `BIO_s_custom_write` output to the right fd. `ssl_write_batching` already existed for `us_internal_ssl_write`: it collects sealed records and writes them once. - The spill slot holds ciphertext a partial batch flush could not deliver. SSL counts those bytes as written, so they must reach that exact socket, in order. Batching is gated on a free spill slot for that reason. - In TLS 1.3 the client finishes first: it sees `secureConnect` one round trip before the server decides on the client certificate. That is why the rejection lands after the client already wrote application data. <details><summary>Notes</summary> - Diagnosis: an `LD_PRELOAD` shim logging `send`/`recv` shows Node's client writes flight + app data as one `write(1253)` while Bun issues `send(1152)` then `send(57)`. The server (Node in both runs) reads 1253 in one chunk from the Node client, but only 1152 from the Bun client before it destroys. - The RST path needs the second segment to arrive inside the window between the server reading the flight and destroying the socket. Release builds hit it almost always. Debug builds are slow enough that the server's FIN usually wins, and Linux masks the late RST (`SOCK_DONE` is checked before `sk_err` in `tcp_recvmsg`). The gate-facing test therefore counts the client's segments through a raw in-test TCP proxy. The count discriminates reliably on an unloaded machine; under heavy CPU load the two unfixed sends can coalesce in the kernel, so a green run on a loaded machine is weaker evidence than the idle fail-before recorded here (20/20 fail on the unfixed build when idle). The end-to-end Node-server repro is kept as a second test, `skipIf(!nodeExe())`. - Related, not the same bug: #40412 covers the send side of a destroyed TLS socket (bare FIN, no close_notify). #35378 holds the flight so a verify-rejecting client's destroy can recall it; it parks the flight in the spill slot and drains it before the callback's write, so it still produces two segments and does not fix #40653. This PR's batch-owner hold subsumes most of that machinery. #35378 should be restacked on top of this as a flush-vs-release policy decision at `us_internal_ssl_close` rather than rebased mechanically. - Follow-up direction for this file: a per-socket outbound-ciphertext buffer would delete the shared batch/spill slots, both owner fields, the loop-state save slots for routing, and the cross-socket double-spill fatal path. Worth doing before more features extend the shared-slot surface. - Pre-existing failures seen during the sweep, identical on an unmodified build: `node-tls-server.test.ts` "SNICallback runs even when the requested servername matches the bind hostname" (ECONNREFUSED in this environment), `test/js/bun/net/` hostname-resolution failures (13, same set on system bun), `test-tls-client-allow-partial-trust-chain.js` (node:test runner), and `test-https-timeout.js` (hangs on a main debug build too). </details>  --- **[review]** gate passed · iteration 0 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 failed, 18 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/tls/node-tls-connect.test.ts bun test v1.4.1 (731aa92) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [3.82ms] (pass) should thow ECONNRESET if FIN is received before handshake [346.19ms] (pass) initializes authorizationError to null in the TLSSocket constructor [7.78ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [106.76ms] (pass) should be able to grab the JSStreamSocket constructor [12.46ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [82.50ms] (pass) tls.connect > should have peer certificate when using self asign certificate [77.93ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port a ... (truncated) release without fix: 2 failed, 18 skipped bun test v1.4.1-canary.1 (731aa92) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [0.03ms] (pass) should thow ECONNRESET if FIN is received before handshake [6.30ms] (pass) initializes authorizationError to null in the TLSSocket constructor [0.15ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [3.27ms] (pass) should be able to grab the JSStreamSocket constructor [0.22ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [4.43ms] (pass) tls.connect > should have peer certificate when using self asign certificate [3.11ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port and host correctly (skip) tls.connect > should process port, host, and callback correctly (skip) tls.connect > should handle the absence of a callback gracefully (skip) tls.c ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 18 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/tls/node-tls-connect.test.ts bun test v1.4.1 (731aa92) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [2.71ms] (pass) should thow ECONNRESET if FIN is received before handshake [362.37ms] (pass) initializes authorizationError to null in the TLSSocket constructor [8.06ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [115.84ms] (pass) should be able to grab the JSStreamSocket constructor [13.31ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [87.82ms] (pass) tls.connect > should have peer certificate when using self asign certificate [85.23ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port a ... (truncated) release with fix: 18 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 863ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/25] cc obj/packages/bun-usockets/src/bsd.c.o [2/25] cc obj/packages/bun-usockets/src/context.c.o [3/25] cc obj/packages/bun-usockets/src/crypto/openssl.c.o [4/25] cxx obj/unified/UnifiedSource-packages_bun_usockets_src_crypto-0.cpp.o [5/25] cc obj/packages/bun-usockets/src/eventing/libuv.c.o [6/25] cxx obj/unified/UnifiedSource-src_uws_sys-0.cpp.o [7/25] cc obj/packages/bun-usockets/src/eventing/epoll_kqueue.c.o [8/25] cc obj/packages/bun-usockets/src/fault_inject.c.o [9/25] cxx obj/src/jsc/bindings/bindings.cpp.o [10/25] cxx obj/unified/UnifiedSource-src_jsc_bindings-5.cpp.o [11/25] cxx obj/unified/UnifiedSource-src_jsc_bindings_node-0.cpp.o [12/25] cc obj/packages/bun-usockets/src/loop.c.o [13/25] cc obj/packages/bun-usockets/src/quic.c.o [14/25] cc obj/packages/bun-usockets/src/node_quic_shim.c.o [15/25] cc obj/packages/bun-usockets/src/udp.c.o [16/25] cc obj/packages/bun-usockets/src/socket.c.o [17/25] cxx obj/unified/UnifiedSource-src_jsc_bindings-4.cpp.o [18/25] cxx obj/unified/UnifiedSource-src_j ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` packages/bun-usockets/src/crypto/openssl.c | 149 +++++++++++++- packages/bun-usockets/src/internal/internal.h | 11 +- src/runtime/socket/socket_body.rs | 9 +- src/runtime/socket/tls_socket_functions.rs | 9 +- test/js/node/tls/node-tls-connect.test.ts | 276 +++++++++++++++++++++++++- 5 files changed, 436 insertions(+), 18 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests packages/bun-usockets/src/crypto/openssl.c 9 21 0 packages/bun-usockets/src/internal/internal.h 2 3 0 src/runtime/socket/socket_body.rs 2 2 0 src/runtime/socket/tls_socket_functions.rs 2 3 0 test/js/node/tls/node-tls-connect.test.ts 2 9 0 ``` </details>  --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

robobun · 2026-09-22T19:22:04Z

Closing this PR. Main now covers the scenario that it tests, and its mechanism no longer fits main.

The tested scenario is a client that fails the native verification (untrusted certificate with rejectUnauthorized: true, as in test-tls-close-error.js). tls: close_notify on end(), injected-socket upgrades, reject-handshake wire fix, duplex data-loss, SNI, ALPN (+14 tests, tls 81%→86%) #34598 fixed it on main: the BIO write drops the client's final flight when the X509 verification failed (us_ssl_inline_reject_tripped).
tls: send the TLS 1.3 final handshake flight and the first secureConnect write in one segment #40655 reworked and merged the final-flight hold that this PR introduced. It holds the flight in the write batch across the on_handshake dispatch (ssl_write_batch_owner). The spill-slot parking in this diff (ssl_spill_discard_on_close) does not fit that design.
I ran only the test from this PR on main at bf80d21 (debug build, no other change). It passes in 4 of 4 runs. The reject case prints [["client-error","UNABLE_TO_VERIFY_LEAF_SIGNATURE"],["client-close",true],["tlsClientError","ECONNRESET"]], the same as Node v26.3.0. test/js/node/test/parallel/test-tls-close-error.js exits 0 in 3 of 3 runs.
One case from the discussion above is still open on main: a rejection that JS decides after the native verification succeeds (checkServerIdentity). node:tls: a client that rejects the server in checkServerIdentity still sends its TLS 1.3 Finished, so the server emits 'secureConnection' where Node emits 'tlsClientError' #43807 tracks it with a repro and the place for the fix on top of tls: send the TLS 1.3 final handshake flight and the first secureConnect write in one segment #40655.

If the scenario of this PR still reproduces on a current build, please reopen it or comment on #43807.

robobun assigned cirospaciari Jul 24, 2026

github-actions Bot added the claude label Jul 24, 2026

coderabbitai Bot reviewed Jul 24, 2026

View reviewed changes

Comment thread packages/bun-usockets/src/crypto/openssl.c Resolved

[autofix.ci] apply automated fixes

a70b907

claude Bot reviewed Jul 24, 2026

View reviewed changes

Comment thread packages/bun-usockets/src/crypto/openssl.c Resolved

Comment thread packages/bun-usockets/src/crypto/openssl.c Resolved

robobun added 2 commits July 24, 2026 05:04

claude Bot reviewed Jul 24, 2026

View reviewed changes

Comment thread packages/bun-usockets/src/crypto/openssl.c Resolved

claude Bot reviewed Jul 24, 2026

View reviewed changes

robobun mentioned this pull request Aug 27, 2026

tls: send the TLS 1.3 final handshake flight and the first secureConnect write in one segment #40655

Merged

VenkatKwest mentioned this pull request Sep 21, 2026

fetch, Bun.SQL (postgres + mysql), Bun.RedisClient and WebSocket send the mTLS client certificate to servers that fail verification #43684

Closed

robobun mentioned this pull request Sep 22, 2026

node:tls: a client that rejects the server in checkServerIdentity still sends its TLS 1.3 Finished, so the server emits 'secureConnection' where Node emits 'tlsClientError' #43807

Closed

robobun closed this Sep 22, 2026

robobun mentioned this pull request Sep 25, 2026

node:tls: destroy() in the handshake callback releases the held final flight #43946

Closed

Navigation Menu

Repository navigation

tls: hold the handshake's final flight until on_handshake decides - #35378

tls: hold the handshake's final flight until on_handshake decides#35378
robobun wants to merge 6 commits into
mainfrom
farm/f0fb88d3/tls-client-hold-final-flight

robobun commented Jul 24, 2026

Uh oh!

robobun commented Jul 24, 2026 •

edited

Loading

Uh oh!

robobun commented Jul 24, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 24, 2026 •

edited

Loading

Review limit reached

❌ Failed checks (2 warnings)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 24, 2026

Uh oh!

robobun commented Jul 24, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Sep 22, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

robobun commented Jul 24, 2026

Problem

Cause

Fix

Verification

Uh oh!

robobun commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

❌ Failed checks (2 warnings)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 24, 2026

Uh oh!

robobun commented Jul 24, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Sep 22, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Jul 24, 2026 •

edited

Loading

robobun commented Jul 24, 2026 •

edited

Loading

coderabbitai Bot commented Jul 24, 2026 •

edited

Loading