node:tls: ignore onread when the TLS socket wraps an existing socket

[robobun]

Problem

• tls.connect({ socket, onread }) hands the decrypted bytes to onread.callback. Node ignores onread when the TLS socket wraps an existing socket and emits 'data'. Node logs [ 'data 5' ], bun logs [ "onread 5" ] for the script in node:tls: tls.connect({ socket, onread }) uses onread, node ignores it for a wrapped socket #42419.
• The cause is the TLSSocket constructor (src/js/node/tls.ts:757). It forwards the caller's options object to net.Socket, so onread reaches net.Socket for a wrapped socket too. Node passes onread: !socket ? tlsOptions.onread : null (wrap.js#L596).

Fix

• Set onread: null in the two branches that already rebuild the options for a wrapped socket (new TLSSocket(socket, ...) and options.socket). The dialed path (tls.connect({ host, port, onread })) is unchanged.
• Correct because both branches match the one condition Node uses (a wrapped socket), and net.Socket treats onread: null as absent.
• Verified: test/js/node/tls/node-tls-upgrade.test.ts (one new test fails on stock bun with ["onread 5"], one covers the server-side new TLSSocket(socket, { isServer: true, onread }) form). Also ran node-tls-connect.test.ts, node-tls-server.test.ts, node-tls-context.test.ts.

Background

• tls.connect({ socket }) wraps an already-connected net.Socket. The TLS socket reads the ciphertext from that socket and exposes the plaintext itself.
• onread is a net.Socket option. It replaces 'data' events with a callback that fills a caller-owned buffer. Bun implements it in the net.Socket constructor (src/js/node/net.ts, the kOnreadDeliver handler).

Notes

• Fail-before: USE_SYSTEM_BUN=1 bun test test/js/node/tls/node-tls-upgrade.test.ts -t 42419 fails with ["onread 5"] on bun 1.4.3.
• The server-side test passes on stock bun too. The server upgrade path installs its own handlers. It is there to cover the first-argument branch of the change.
• Four tests in node-tls-connect.test.ts and node-tls-server.test.ts time out in this container both with and without the change (SNICallback runs even when..., destroySoon follows end, a client paused while connecting x2).
• Node also drops every other option from the net.Socket call and passes a fixed set. This change does not go that far. It fixes the one option the issue reports.

---

[human-review] gate passed · iteration 1 · 2 files touched

fails on main (without fix)

ASAN without fix: 1 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-upgrade.test.ts
bun test v1.4.3 (6a92015fc)

test/js/node/tls/node-tls-upgrade.test.ts:
(pass) should be able to upgrade a paused socket and also have backpressure on it #15438 [1511.93ms]
(pass) tls.connect({ socket }) over a net.Socket with readable: false keeps the TLS bytes off the wrapped socket [282.09ms]
(pass) tls.connect({ socket }) over a net.Socket with an onread buffer keeps the TLS bytes off the wrapped socket [110.95ms]
(pass) tls.connect({ socket }) over a net.Socket with no reader keeps the TLS bytes off the wrapped socket [105.57ms]
129 |       onread: { buffer: Buffer.alloc(16), callback: (n: number) => log.push(`onread ${n}`) },
130 |     });
131 |     tlsSocket.on("data", data => log.push(`data ${data.length}`));
132 |     tlsSocket.on("error", () => {});
133 |     await once(tlsSocket, "close");
134 |     expect(log).toEqual(["data 5"]);
                      ^
error: expect(received).toEqual(expected)

  [
-   "data 5",
+   "onread 5",
  ]

- Expected  - 1
+ Received  + 1

      at <anonym
... (truncated)

release without fix: all passed
bun test v1.4.3-canary.1 (1e0a6f351)

test/js/node/tls/node-tls-upgrade.test.ts:
(pass) should be able to upgrade a paused socket and also have backpressure on it #15438 [39.34ms]
(pass) tls.connect({ socket }) over a net.Socket with readable: false keeps the TLS bytes off the wrapped socket [8.04ms]
(pass) tls.connect({ socket }) over a net.Socket with an onread buffer keeps the TLS bytes off the wrapped socket [4.22ms]
(pass) tls.connect({ socket }) over a net.Socket with no reader keeps the TLS bytes off the wrapped socket [4.02ms]
(pass) tls.connect({ socket, onread }) ignores onread and emits 'data' (#42419) [4.02ms]
(pass) new tls.TLSSocket(socket, { isServer: true, onread }) ignores onread and emits 'data' (#42419) [4.47ms]
(pass) a STARTTLS exchange hands no TLS bytes to the 'data' listeners of the wrapped sockets (#32239) [4.51ms]

 7 pass
 0 fail
 14 expect() calls
Ran 7 tests across 1 file. [173.00ms]
__F:0:S:0

passes on PR (with fix)

ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-upgrade.test.ts
bun test v1.4.3 (6a92015fc)

test/js/node/tls/node-tls-upgrade.test.ts:
(pass) should be able to upgrade a paused socket and also have backpressure on it #15438 [1314.16ms]
(pass) tls.connect({ socket }) over a net.Socket with readable: false keeps the TLS bytes off the wrapped socket [297.34ms]
(pass) tls.connect({ socket }) over a net.Socket with an onread buffer keeps the TLS bytes off the wrapped socket [117.43ms]
(pass) tls.connect({ socket }) over a net.Socket with no reader keeps the TLS bytes off the wrapped socket [101.39ms]
(pass) tls.connect({ socket, onread }) ignores onread and emits 'data' (#42419) [94.06ms]
(pass) new tls.TLSSocket(socket, { isServer: true, onread }) ignores onread and emits 'data' (#42419) [160.37ms]
(pass) a STARTTLS exchange hands no TLS bytes to the 'data' listeners of the wrapped sockets (#32239) [144.43ms]

 7 pass
 0 fail
 14 expect() calls
Ran 7 tests across 1 file. [5.62s]
__F:0:S:0

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 934ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/124] gen cpp.rs (cppbind)
[2/124] gen JS modules (bundle-modules)
Preprocess modules (10739ms)
Bundle modules (122ms)
Postprocesss modules (198ms)
Bundle Functions (738ms)
Generate Code (51ms)

[11.86s] Bundled "src/js" for production
  2600 kb
  197 internal modules
  13 native modules
  50 internal functions across 16 files
[2/123] cargo bun_runtime → libbun_runtime.a
�[1m�[92m   Compiling�[0m bun_runtime v0.0.0 (/workspace/bun/src/runtime)
�[1m�[92m    Finished�[0m `release` profile [optimized + debuginfo] target(s) in 8m 10s
[119/123] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-0.cpp.o
[120/123] link bun-profile
[122/123] strip bun
[122/123] bun-profile --revision
1.4.3-canary.1+3c0a16802
[build] done
bun test v1.4.3-canary.1 (3c0a16802)

test/js/node/tls/node-tls-upgrade.test.ts:
(pass) should be able to upgrade a paused socket and also have backpressure on it #15438 [77.72ms]
(pass) tls.connect({ socket }) over a net.Socket with readable: false keeps the TLS bytes off the wrapped sock
... (truncated)

diff hotspot

src/js/node/tls.ts                        |  6 +--
 test/js/node/tls/node-tls-upgrade.test.ts | 66 ++++++++++++++++++++++++++++++-
 2 files changed, 68 insertions(+), 4 deletions(-)

gate history · 1 passed · 1 rejected · iteration 1

evidence per changed file

file                                       reads  edits  tests
src/js/node/tls.ts                             2      3     14
test/js/node/tls/node-tls-upgrade.test.ts      5      7     14

root cause · written by the author bot

Bun's TLSSocket forwarded the caller's options object, including onread, to the net.Socket constructor even when wrapping an existing socket, so decrypted bytes were delivered to onread.callback instead of being emitted as 'data' events, whereas Node only honors onread when the TLS socket dials its own connection. The fix sets onread: null in the rebuilt options on both wrap paths (the positional socket argument and options.socket), matching Node's !socket ? tlsOptions.onread : null condition in lib/internal/tls/wrap.js, while leaving the dialed path untouched so `tls.co…

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 2b3f6fad-2524-4289-9020-d00e89ac3d8e

📥 Commits

Reviewing files that changed from the base of the PR and between 90f04ca and f3123d2.

📒 Files selected for processing (1)

• test/js/node/tls/node-tls-upgrade.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.

---

Walkthrough

TLSSocket now clears onread when wrapping a Duplex and preserves allowHalfOpen. Regression tests cover client and server upgrades and verify TLS payloads arrive through 'data' events.

Changes

TLS socket upgrade behavior

Layer / File(s)	Summary
Wrapped socket option normalization src/js/node/tls.ts	TLSSocket clears onread and preserves the wrapped socket’s allowHalfOpen setting.
Client and server regression coverage test/js/node/tls/node-tls-upgrade.test.ts	Tests verify that supplied onread callbacks are ignored and TLS payloads are emitted through 'data' events for both upgrade paths. The backpressure test uses 20 round trips with the updated timeout expectation.

Suggested reviewers: cirospaciari

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 3c0a1

The TLS upgrade behavior is covered for both client and server paths, with no remaining merge-blocking concerns.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the primary change: ignoring onread when TLSSocket wraps an existing socket.
Description check	✅ Passed	The description is detailed and on topic. It explains the problem, fix, scope, verification steps, regression coverage, and known test limitations. It does not use the exact template headings, but it …

---

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

[robobun]

The automated review raised no findings. No open review threads. Waiting on the Buildkite build (#114580).

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[robobun]

Addressed the review nit in 79c461b: the test now destroys the client in the finally block, so a failed assertion does not leave the connection open.

[robobun]

^{Updated 1:33 AM PT - Sep 12th, 2026}

✅ @robobun, your commit 3c0a168024d99eaa7ce8f3e5c4e5f065d90a0985 passed in Build #114616! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 42425

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

bun-42425 --bun

[robobun]

Pushed 1e0a6f3: the comment on the wrapped-socket branch is now one line. No open review threads. Buildkite build #114585 is running.

[claude]

LGTM — the client-cleanup nit is addressed (client?.destroy() now runs in finally), and the comment was tightened.

What was reviewed:

• Both TLSSocket constructor branches that rebuild options for a wrapped socket now set onread: null, matching Node's !socket ? tlsOptions.onread : null in wrap.js; the dialed-connection path is untouched so tls.connect({ host, port, onread }) still works.
• onread is a data-delivery hook, not a TLS/security flag — dropping it on wrap is strictly a compat fix with no auth/verification impact.
• New tests cover both entry points (options.socket and positional socket with isServer: true), use port: 0 / 127.0.0.1, wire error events to reject, and clean up in finally.

Extended reasoning...

Overview

The PR changes two lines in src/js/node/tls.ts (the TLSSocket constructor) to add onread: null to the options object when wrapping an existing net.Socket/Duplex, in both the positional-argument branch and the options.socket branch. It also updates the adjacent comment's Node source link to cover lines 592-596. Two tests are added to test/js/node/tls/node-tls-upgrade.test.ts exercising each branch and asserting that onread.callback is never invoked and the payload arrives via 'data'.

Security risks

None identified. onread is a net.Socket data-delivery option (buffer + callback in place of 'data' events), not a TLS verification, cipher, or certificate flag. Nulling it on the wrapped-socket path makes the TLS socket fall back to the standard 'data' event flow, which is what Node does. No security guard is weakened or removed; rejectUnauthorized, ca, servername, etc. are untouched.

Level of scrutiny

Low-to-moderate. The change is a two-line option override in a spot that already rebuilds the options object for the same reason (allowHalfOpen), and it is backed by a Node source permalink showing the exact conditional (onread: !socket ? tlsOptions.onread : null). The PR description correctly notes that the direct-dial path (tls.connect({ host, port, onread })) is left alone, and I confirmed neither modified branch is reachable without a wrapped socket. No CODEOWNERS entry covers these files.

Other factors

Since my prior review, commit 79c461b moved client.destroy() into the finally block (guarded with ?.), addressing the resource-leak-on-failure nit I raised. Commit 1e0a6f3 shortened the source comment. The tests follow harness conventions: port: 0, 127.0.0.1, once(...) for listening/connect, Promise.withResolvers with error handlers wired to reject, and cleanup in finally registered before assertions. The first test awaits the socket's 'close' event before asserting, so the connection is already torn down by the time the assertion runs. The bug-hunt exit reason was dry_streak with no findings.

[robobun]

Pushed 90f04ca: the existing #15438 backpressure test in this file takes 3.5 to 5 s under the debug ASAN build with or without this change, so it now has a 30 s timeout. No open review threads. Buildkite build #114603 is running.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[robobun]

Pushed f3123d2: the #15438 backpressure test now runs 20 round trips instead of 100 and has no explicit timeout. It takes about 1.5 s on a debug ASAN build. No open review threads.

[claude]

LGTM — both earlier nits are addressed (client cleanup moved into finally, and the backpressure test's workload was reduced to 20 iterations instead of raising the timeout).

What was reviewed:

• src/js/node/tls.ts: onread: null set on both wrap-socket branches, matching the cited Node wrap.js#L596 condition; the dialed path (no wrapped socket) still passes onread through unchanged.
• New tests use port: 0, await events (once(..., "close") / a resolved promise) rather than sleeping, wire error to reject, and clean up in finally.
• The 100→20 iteration reduction still moves ~2.5 MiB through a paused TLS socket, so the backpressure path the test guards is still exercised.

Extended reasoning...

Overview

Two-line runtime change in src/js/node/tls.ts: when TLSSocket wraps an existing net.Socket/Duplex (either as the positional first argument or as options.socket), the rebuilt options object now sets onread: null so a caller-supplied onread handler is dropped and 'data' events fire on the TLS wrapper. This mirrors Node's !socket ? tlsOptions.onread : null at lib/internal/tls/wrap.js#L596, and the comment is updated to cite that range. Two new tests in test/js/node/tls/node-tls-upgrade.test.ts cover both branches (client-side tls.connect({ socket, onread }) and server-side new tls.TLSSocket(socket, { isServer: true, onread })), and an existing backpressure test's loop count is cut from 100 to 20 with an explanatory comment.

Security risks

None. This drops an option in a wrap path rather than adding one; it does not touch certificate validation, rejectUnauthorized, SNI, or the secure-context setup. onread is a net.Socket data-delivery convenience, not a security boundary.

Level of scrutiny

Low-to-medium. The runtime change is two symmetric one-line edits to option objects that were already being rebuilt for the wrap case, with a Node source permalink justifying the behavior. The bug-class rule ("fix every sibling site") is satisfied — both wrap branches are updated. The dialed path (else branch when options.socket is not a Duplex) is untouched, so tls.connect({ host, port, onread }) still honors onread as before.

Other factors

This is my fifth pass on the PR. Earlier feedback (client socket not destroyed on the failure path; a 30 s per-test timeout added to absorb debug-ASAN latency) has been addressed in follow-up commits: client?.destroy() now runs in finally before server.close(), and the timeout arg was removed in favor of shrinking the workload to 20 iterations (still ~2.5 MiB over a paused TLS socket, enough to hit the backpressure path the original test guards). Tests use port: 0, local servers only, once()/Promise.withResolvers to await real events, and toEqual on the exact log array. No outstanding third-party CHANGES_REQUESTED reviews are visible in the timeline.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[Jarred-Sumner]

Superseded by #44618, which consolidates the open TLS pull requests. This fix and its tests are in there, either as written, rewritten smaller, or merged with the other PRs that patched the same cause (see the "By area" list in that PR). Closing in favor of it.