stop named-pipe tls.connect({ socket }) re-emitting post-upgrade bytes on the original socket

[Sids15]

What

Fixes #32242 — on Windows, tls.connect({ socket }) over a named-pipe net.Socket re-emits post-upgrade (encrypted)
bytes on the original socket's pre-existing data listeners.

Why

The named-pipe upgrade takes the upgradeDuplexToTLS path. Unlike the TCP upgradeTLS path, it does not replace
connection._handle, so the original native pipe handle keeps firing SocketHandlers2.data with self = connection. The TLS layer was fed by subscribing to the connection's public data event (connection.on("data", events[0])), so post-upgrade ciphertext flowed to both the TLS feeder (intended) and any pre-existing user
data listener — the STARTTLS re-entry from #32239 — leaking encrypted bytes as cleartext.

The kupgradedToTLS flag from #32241 (TCP path) can't be reused here: suppressing self.push(buffer) would also
starve the feeder, which consumes that very data event, breaking TLS over named pipes entirely.

How

Store the data feeder on the connection (kupgradeDuplexFeeder) instead of subscribing to the public data event,
and have SocketHandlers2.data call it directly — bypassing connection.push. No data event is emitted
post-upgrade, so the original socket goes quiet while the TLS layer still receives every byte. end/drain/close
subscriptions are unchanged (they carry no payload). Windows named pipes only.

Test

Adds a regression test to test/js/node/tls/node-tls-namedpipes.test.ts (it.if(isWindows)): attaches a
pre-existing data listener on the original net.Socket, upgrades via tls.connect({ socket }), round-trips TLS
app data, and asserts the original listener received zero bytes. It fails on the unfixed build (listener receives
ciphertext) and passes after the fix. Runs on Windows CI.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5e8a778c-2929-406e-88a9-6602546f8ab4

📥 Commits

Reviewing files that changed from the base of the PR and between 1ebb308 and 629fd43.

📒 Files selected for processing (2)

• src/js/node/net.ts
• test/js/node/tls/node-tls-namedpipes.test.ts

---

Walkthrough

A new internal symbol kupgradeDuplexFeeder is added to net.ts to intercept named-pipe socket data before it reaches user-observable data listeners during TLS upgrades. The SocketHandlers2.data and onread handlers route buffers to the feeder when present, and both upgrade paths in Socket.prototype.connect use a new helper to assign the feeder directly instead of registering a "data" listener. A Windows-only regression test verifies zero bytes leak to pre-upgrade listeners.

Changes

Named-pipe TLS upgrade data leak fix

Layer / File(s)	Summary
kupgradeDuplexFeeder symbol and handler interception src/js/node/net.ts	Declares kupgradeDuplexFeeder symbol at module scope, initializes it to undefined in the Socket constructor, and adds early-return branches in both SocketHandlers2.data and the onread-overridden data() handler that forward buffers to self[kupgradeDuplexFeeder] when set, bypassing the normal push()/pause() flow and user callbacks.
attachUpgradeDuplexDataFeeder helper and connect wiring src/js/node/net.ts	Introduces attachUpgradeDuplexDataFeeder(connection, onData) to conditionally store the feeder on connection[kupgradeDuplexFeeder] for Socket instances or attach it as a "data" listener for non-Socket duplexes. Both the immediate and deferred Duplex-to-TLS upgrade paths in Socket.prototype.connect replace direct connection.on("data", events[0]) calls with this helper.
Windows named-pipe TLS upgrade regression test test/js/node/tls/node-tls-namedpipes.test.ts	Adds a Windows-only it.if(isWindows) test that attaches a data listener to a named-pipe net.Socket before calling tls.connect({ socket }), performs the TLS handshake and message exchange, then asserts that collected bytes on the pre-upgrade listener total zero length before tearing down sockets and servers.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main change: fixing tls.connect re-emitting post-upgrade bytes on named-pipe sockets.
Description check	✅ Passed	The description provides comprehensive coverage of both required template sections with clear explanations of what the PR does and how verification was performed.
Linked Issues check	✅ Passed	The PR fully addresses the objectives in #32242: preventing post-upgrade encrypted bytes from reaching pre-existing data listeners while ensuring TLS continues receiving all bytes.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the named-pipe TLS upgrade issue with no unrelated modifications or scope creep detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[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 `@src/js/node/net.ts`:
- Line 1003: The assignment of connection[kupgradeDuplexFeeder] at line 1003 and
line 1042 is incomplete for non-Socket Duplex upgrades. Currently these only set
the feeder reference, but non-Socket Duplex types never execute through
SocketHandlers2.data, leaving the TLS feeder uninitialized. Restore the generic
Duplex data wiring by adding the necessary feeder invocation logic at both
locations (line 1003 and line 1042) to handle cases where SocketHandlers2.data
will not be called, ensuring the TLS upgrade path works for all Duplex types,
not just Bun Socket named-pipe paths.

🪄 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: 9fd4d91b-61ac-48da-acb7-8d533e2b649d

📥 Commits

Reviewing files that changed from the base of the PR and between f764878 and a2398ce.

📒 Files selected for processing (2)

• src/js/node/net.ts
• test/js/node/tls/node-tls-namedpipes.test.ts

[coderabbitai]

Actionable comments posted: 3

🤖 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 `@src/js/node/net.ts`:
- Around line 897-898: The upgrade feeder routing at lines 897-898 does not
account for Socket instances created with custom `onread` handlers, which
replace `this[khandlers]` around lines 763-775. These custom handlers bypass the
`kupgradeDuplexFeeder` check entirely, causing TLS upgrade bytes to be routed to
the `onread.callback` instead of the intended `events[0]` handler. To fix this,
modify the custom handler logic in the lines 763-775 region to also check for
and route data through `kupgradeDuplexFeeder` before delegating to the custom
`onread.callback`, ensuring that socket upgrade bytes are always processed by
the feeder mechanism regardless of whether the socket was constructed with an
`onread` handler.

In `@test/js/node/tls/node-tls-namedpipes.test.ts`:
- Around line 141-150: The test awaits multiple conditions without racing them
against error events: the `once(client, "secureConnect")` call, the
`messageReceived` promise, and the `clientReceived` promise are all awaited
independently. If any error event fires before these conditions are met, the
test will hang until timeout instead of failing immediately. Use Promise.race()
to race each awaited condition against the `done` promise, ensuring that if
`done` rejects (due to error events wired to `rejectDone`), all awaited
operations fail fast at the actual error cause rather than hanging.
- Around line 98-100: Remove the verbose comments from the test body that
explain bug mechanics and issue context. Keep only a terse reference to the
issue number (like `#32242`) at the top of the test if needed, and remove inline
comments throughout the test that restate the bug context or mechanics. Let the
test assertions demonstrate the invariant being tested rather than having
comments explain the expected behavior.

🪄 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: fa91279f-7da0-45aa-b6c5-a70ec36d4124

📥 Commits

Reviewing files that changed from the base of the PR and between a2398ce and 1ebb308.

📒 Files selected for processing (2)

• src/js/node/net.ts
• test/js/node/tls/node-tls-namedpipes.test.ts

[robobun]

Thanks for the patch. #36534 adopts this (with Co-authored-by credit), rebased onto current main and extended to the server-side upgradeDuplexToTLS sites and the hasUnflushedWrites trigger added in #34598, with a platform-independent regression test.

[robobun]

Thanks again for this patch. It has been folded into #36534, which is now the single PR for this bug: it uses the same idea (feed the engine straight from the socket's native data handlers instead of its public data event), extended to the other upgrade sites that exist on current main, and your named-pipe round-trip test is carried there. You are credited as co-author on the commit. Closing this one so the fix lands in one place.