Repository navigation
Conversation
…has ended The raw half of a TLS pair still sees the connection's bytes. Since readable: false is honored, pushing them into a raw socket built that way was a push after EOF: it destroyed the raw socket and took the TLS socket down before its data arrived. Node's TLSWrap reads from the handle, so the raw stream's flags do not matter there.
|
Updated 8:10 PM PT - Sep 10th, 2026
❌ @robobun, your commit c53e913 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 42262That installs a local version of the PR into your bun-42262 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe change prevents ChangesTLS readable socket handling
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The TLS regression is addressed without changing byte accounting or idle-timer handling. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: closed in favor of #42265. #42265 fixes the same regression ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a two-line guard in the shared pushDataToSocket helper, with a targeted regression test.
What was reviewed:
- Verified
readable: falsesynchronously sets_readableState.endEmitted = trueinduplex.ts, so thereadableEndedguard is live immediately (no nextTick gap). - Confirmed the guard sits in the shared helper reached by all three native
datahandlers, and thatbytesRead/_unrefTimer()remain in the callers as the PR describes. - Checked
self.readableEndedmatches the file's existing convention (same accessor used at three other sites innet.ts). - Test:
listen(0)on 127.0.0.1,it.eachover both connect states, error events wired into the asserted array,await once(client, "close")with try/finally cleanup — no timing waits.
Extended reasoning...
Overview
The PR adds a single early-return (if (self.readableEnded) return;) to pushDataToSocket in src/js/node/net.ts, the shared helper that feeds native socket bytes into the JS Readable side of a net.Socket. This fixes a regression where tls.connect({ socket }) over a net.Socket constructed with readable: false would push() into an already-ended readable, triggering ERR_STREAM_PUSH_AFTER_EOF and destroying the raw socket (and the TLS socket riding on it). A new it.each case in test/js/node/tls/node-tls-connect.test.ts exercises both the "already connected" and "still connecting" raw-socket states against a local TLS server and asserts the exact event sequence.
Security risks
None identified. The change does not touch certificate verification, hostname checking, or any TLS security decision — it only stops feeding decrypted-side-irrelevant ciphertext bytes into a JS stream whose readable side the user explicitly disabled. Reading self.readableEnded (a prototype getter) matches the file's existing convention (used at three other sites in net.ts), so no new prototype-tampering surface is introduced relative to the surrounding code.
Level of scrutiny
Low-to-moderate. The production change is two lines in a well-understood helper, placed at the layer that owns the invariant (the shared push routine) rather than at one call site — so all three callers (plain socket, TLS raw half, and the third data handler) are covered in one place. I cross-checked src/js/internal/streams/duplex.ts to confirm readable: false sets endEmitted = true synchronously at construction, so the readableEnded getter is true from the first byte with no race window. The PR's claim that bytesRead and the idle-timer refresh should stay outside the guard checks out: both are updated in the callers before pushDataToSocket is invoked, matching Node's handle-level accounting.
Other factors
The test follows harness conventions cleanly: ephemeral port via listen(0), local 127.0.0.1 server, once() on real events (no sleeps), error listeners on both raw and client funneled into the asserted events array so a regression surfaces as a precise toEqual mismatch, and server.close() in finally. it.each covers the connected/connecting variant matrix. The bug hunt ran to a dry streak with no findings, no CODEOWNERS entries cover these paths, and there are no outstanding reviewer objections in the timeline.
|
@cirospaciari checked. #42265 covers this and more: it drops the TLS stream on the adopted raw half (so it also holds for a wrapped socket with an open readable side or an |
… socket (#42265) Fixes #32239. ### Problem - Regression from #42213. `tls.connect({ socket })` over a `net.Socket` built with `readable: false` reports `ERR_STREAM_PUSH_AFTER_EOF` on the wrapped socket during the handshake. That destroys the connection before any data arrives. - Cause: after TLS adopts the fd, the raw half still hands the ciphertext to the wrapped socket. `pushDataToSocket` (`src/js/node/net.ts:443`) pushes it into the ended Readable. The same leak calls a wrapped `onread`, re-enters `'data'` listeners left from STARTTLS (`Invalid socket`, #32239), and fills a buffer that nothing reads. - `net.connect({ readable: false, onread })` never calls `onread`. The `readableEnded` checks from #42213 keep the handle stopped. Node starts it whenever an `onread` buffer is set ([net.js#L830-L845](https://github.com/nodejs/node/blob/v26.3.0/lib/net.js#L830-L845)). ### Fix - Data on the adopted raw half (`kAdoptedTLSRaw`) is dropped before the push and before the `onread` callback. Node's `TLSWrap` takes over the handle's reads ([wrap.js#L723-L727](https://github.com/nodejs/node/blob/v26.3.0/lib/internal/tls/wrap.js#L723-L727)), so the wrapped socket never sees the TLS stream. Its idle timer is still refreshed. - The `readableEnded` checks in `resume()`, `read()` and `afterConnect()` exempt a socket with an `onread` buffer. - Verified: `node-tls-upgrade.test.ts` (`readable: false`, `onread`, no reader, STARTTLS on both peers) and `node-net.test.ts` (`onread` + `readable: false`). A build of main's `src/` fails all five. ### Background - `tls.connect({ socket })` and `new tls.TLSSocket(socket, { isServer: true })` adopt the connected fd. The native call returns a TLS handle and a raw handle. The raw handle stays on the original `net.Socket` (`kAdoptedTLSRaw`) and receives each ciphertext chunk. - A Duplex built with `readable: false` starts with `endEmitted` set, so `push()` reports `ERR_STREAM_PUSH_AFTER_EOF`. - `onread` makes a `net.Socket` read into the caller's buffer and skip the Readable. <details><summary>Notes</summary> **Repro 1, TLS over a `readable: false` socket** ```js const raw = net.connect({ port, host: "127.0.0.1", readable: false }); raw.on("connect", () => { const s = tls.connect({ socket: raw, ca, servername: "localhost" }); s.on("secureConnect", () => s.write("hi")); s.on("data", d => console.log("data", String(d))); }); ``` - node v26.3.0: `secureConnect | data "banner" | data "echo:hi"` - bun 1.4.2: `secureConnect | data "bannerecho:hi"` - main: `raw error ERR_STREAM_PUSH_AFTER_EOF | secureConnect | tls end | raw close | tls close` **Repro 2, `onread` + `readable: false`.** Node's `read()` / `resume()` start the handle whenever an `onread` buffer is set, whatever the Readable state, and `afterConnect` calls `read(0)`. Main reads nothing. **Results against node v26.3.0** | | node v26.3.0 | main | this branch | |---|---|---|---| | TLS over `readable: false` | `banner`, `echo:hi` | `ERR_STREAM_PUSH_AFTER_EOF`, no data | `bannerecho:hi` | | `onread` + `readable: false` | `"banner"` | `""` | `"banner"` | | wrapped `onread` socket, TLS bytes seen | 0 | 2790 | 0 | | STARTTLS, `'data'` calls on the wrapped sockets after the wrap | 0 | 4 (client and server) | 0 | | wrapped socket `readableLength` after a 16 MiB TLS transfer | 0 | 16,802,694 | 0 | | repro script of #32239 | upgrade succeeds | `Invalid socket` on the third `'data'` call | upgrade succeeds | The last three rows also fail on the released 1.4.3. They come from the same leak and are older than #42213. **More checks on this branch** - `bytesRead` of the wrapped socket still counts the TLS bytes on the push path, like node (the TCP handle counts every byte it reads). - A 300 ms `setTimeout()` on the wrapped socket does not fire while TLS traffic arrives every 100 ms, like node (`_unrefTimer()` still runs before the drop). - The event order on the wrapped socket and on the TLS socket is the same as on 1.4.3 in these cases: the peer ends first, the client calls `end()` first, each with a plain, a `readable: false` and an `allowHalfOpen` wrapped socket. **Suites** - `test/js/node/tls/node-tls-upgrade.test.ts` (5), `node-tls-connect.test.ts` (55), `node-tls-server.test.ts`, `test/js/node/net/node-net.test.ts`, `test/js/node/http2/node-http2-upgrade.test.mts` (15), `test/js/bun/net/socket-retention.test.ts` (5), `test/regression/issue/{12117,40401}.test.ts`. - The 30 vendored `test-tls-*` / `test-https-*` files that wrap a socket in TLS. - The author ran `node-http2.test.js` (380 pass) on the first commit. The second commit changes only tests. **Related PRs** - #42262, opened in parallel, returns early in `pushDataToSocket` when `readableEnded`. That fixes the `ERR_STREAM_PUSH_AFTER_EOF` case. It does not fix `onread` + `readable: false`, which comes from the `readableEnded` checks in `resume()` / `read()` / `afterConnect()`. It also still hands the TLS stream to a wrapped socket whose readable side is open or that has an `onread` buffer. Both PRs touch the top of `pushDataToSocket`, so the one that lands second needs a one-line rebase. - #36534 reworks the upgrade in native code so that the ciphertext is never dispatched to JS. This PR drops it in the JS handler, which is enough for the wrapped socket to see nothing. **Not addressed here** - `bytesRead` stays 0 on every socket built with `onread`, wrapped or not. The `onread` data handler never counted. This is also the case on 1.4.3. It has its own report. - A wrapped socket that goes through the stream-level TLS engine (a named pipe on Windows, or a socket with pending writes) feeds that engine from its `'data'` events. With `readable: false` it emits none. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/net/node-net.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
Problem
tls.connect({ socket: raw })over anet.Socketbuilt withreadable: falseemitsERR_STREAM_PUSH_AFTER_EOF("stream.push() after EOF") onraw. That destroysraw, which takes the TLS socket down before its data arrives. 1.4.3 and node v26.3.0 deliver the data.pushDataToSocket(src/js/node/net.ts:437) pushes them into the rawnet.Socket. Since node:net: honor readable/writable in the Socket constructor so a write-only adopted fd stays open #42213 areadable: falsesocket starts with its readable side ended, so that push is a push after EOF.Fix
pushDataToSocketreturns early whenself.readableEnded. An ended readable side takes no data.TLSWrapreads from the handle, so the raw stream's flags do not matter.bytesReadand the idle-timer refresh stay in the callers, which matches node (bytesReadis the handle's count, and TLS reads refresh the parent's timer).test/js/node/tls/node-tls-connect.test.ts(new case for a connected and a connecting raw socket, both fail on main withraw error ERR_STREAM_PUSH_AFTER_EOF). Also all of that file,node-tls-server.test.ts,node-net-server.test.tsand thereadable/ fd adoption tests innode-net.test.ts.Background
tls.connect({ socket })upgrades the connected handle in place (upgradeTLSDeferred). It returns a pair: a TLS handle for the newTLSSocketand a raw handle that stays on the originalnet.Socketand keeps feeding that socket'sdatahandler.readable: falsestarts withendEmittedset.push()on it reportsERR_STREAM_PUSH_AFTER_EOFthrougherrorOrDestroy, which destroys the stream.Notes
Repro (any TLS server that writes after the handshake):
node v26.3.0 and bun 1.4.3:
secureConnect/data hello. main before this change:raw error ERR_STREAM_PUSH_AFTER_EOF/secureConnect, no data.The released 1.4.3 does not have node:net: honor readable/writable in the Socket constructor so a write-only adopted fd stays open #42213, so the new test passes there. It fails on a build of main.
net: stop surfacing TLS traffic on the socket that tls.connect({ socket }) / new TLSSocket(socket) took over #36534 (open) stops surfacing TLS traffic on the raw socket at all. With it the raw handler would see nothing and this guard would be a no-op for the TLS case. The guard is still right on its own: the native layer can deliver data whatever the JS stream state is, for example
net.connect({ fd, readable: false }).The
onreadvariant of the handler does not push into the Readable, so it is not affected.[auto-merge] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file