Repository navigation
Conversation
|
Updated 10:11 AM PT - Jun 17th, 2026
✅ @robobun, your commit 8aaefe4ff49d6056c948ee69db93d56cec6ac6fa passed in 🧪 To try this PR locally: bunx bun-pr 32243That installs a local version of the PR into your bun-32243 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRoutes post-TLS-upgrade bytes for named-pipe sockets to a per-socket TLS feeder so upgraded TLS data is handled by TLS instead of being re-emitted as cleartext; updates multiple data paths, both upgrade branches, and socket reuse cleanup. Adds Windows STARTTLS regression tests. ChangesNamed-pipe TLS upgrade data routing
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Update: the two named-pipe TLS regression tests now pass on Windows CI (all The red on that build is unrelated flake on lanes this diff does not touch: Because that |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@test/js/node/tls/node-tls-namedpipes.test.ts`:
- Around line 190-203: The test currently treats any "close" event as a
successful terminal outcome, which masks cases where the socket closes before
TLS is actually completed; change the bare close handlers so they signal failure
instead of success: update the serverSocket.on("close") and
tlsSocket.on("close") callbacks to resolve with a non-empty message (e.g.
resolve({ upgrades, message: "closed before secureConnect" })) so only a real
TLS terminal event (tlsSocket.on("secureConnect") or an error) counts as success
and the final assertion can fail when post-upgrade bytes are dropped.
🪄 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: 3b31593e-2854-4484-934c-5db652bdaed5
📒 Files selected for processing (2)
src/js/node/net.tstest/js/node/tls/node-tls-namedpipes.test.ts
… tls.connect({ socket })
On Windows, tls.connect({ socket }) over a named pipe upgrades through the
Duplex TLS wrapper. Unlike the TCP path, it leaves connection._handle in
place, so the native pipe handle keeps firing SocketHandlers2.data after the
upgrade. The post-upgrade bytes were pushed onto the original socket and
re-emitted as cleartext data, reaching both the TLS feeder and any
pre-existing user data listener. In a STARTTLS pattern the re-entrant
listener upgrades the already-upgraded socket again.
Route the named-pipe handle's post-upgrade reads straight to the TLS data
feeder (kDuplexTLSData) instead of self.push, so the original socket goes
quiet after the upgrade. Pure Duplex transports are unchanged (they have no
native handle and match Node's behavior).
…lers
isNamedPipeSocket() is true for any pipe-variant handle, including a
server-accepted named-pipe socket whose active data handler is
ServerHandlers.data (not SocketHandlers2.data). connect() sets
kDuplexTLSData for those too, so the feeder has to be consulted there
as well, otherwise tls.connect({ socket: serverAcceptedPipeSocket })
would starve the TLS layer (no data listener, and ServerHandlers.data
ignored the feeder).
Mirror the SocketHandlers2.data check in ServerHandlers.data and
SocketHandlers.data (the fd-connect path), and add a server-accepted
named-pipe STARTTLS regression.
…arser Tag each terminal outcome and require the mock records to surface as a TLS `error` (outcome === "tls-error"). A bare socket close or a completed handshake now fails the assertion instead of passing silently, so a feeder that dropped post-upgrade bytes could not masquerade as success.
… bytes On Windows the upgraded TLS socket emits secureConnect (the handshake completes) rather than a parser error, so requiring outcome === "tls-error" failed in CI. Both a completed handshake and a parser error prove the post-upgrade bytes reached the TLS layer via the feeder; only the raw socket closing with the TLS layer idle indicates a dropped feeder. Assert the outcome is not that raw-close sentinel instead of a specific TLS result. The re-emission guard (upgrades === 1) is unchanged.
Keep the durable setup and assertion explanations (mock STARTTLS protocol, why the mock records reach the TLS parser, what the outcome/upgrades assertions guard); drop the 'before the fix' narrative, the #32239 cross-reference, and the SocketHandlers2/ServerHandlers internal names that a refactor could rename. Per root CLAUDE.md, that context lives in the PR description.
2ec05db to
8aaefe4
Compare
|
#36534 covers this on current |
|
Closing in favor of #36534, which covers the named-pipe path on current main (including the server-side and unflushed-writes stream-level sites added since this was opened) and carries the accepted-named-pipe test from this PR. |
Follow-up to #32241 (fix for #32239), carved out as #32242.
Summary
tls.connect({ socket })over a Windows named pipe re-emits post-upgrade bytes as cleartextdataon the originalnet.Socket. #32241 fixed this for the TCP path; this PR covers the sibling named-pipe path, which #32241 could not reuse.Root cause
Socket.prototype.connect(src/js/node/net.ts) has two code paths for an existing socket:socket.upgradeTLS): swaps the native handle withconnection._handle = raw.upgradeDuplexToTLS): does not replaceconnection._handle.A named-pipe
net.Socketreuses the Duplex wrapper (isNamedPipeSocket(socket)is true only on Windows). Because its native pipe handle is left in place, it keeps firingSocketHandlers2.datawithself = connectionafter the upgrade. That handler runsself.push(buffer), which emits a publicdataevent. The TLS layer was fed through that same public event (connection.on("data", events[0])), so post-upgrade ciphertext reached both the TLS feeder (intended) and any pre-existing userdatalistener.In a STARTTLS pattern the re-entrant listener calls
tls.connect({ socket })again on the already-upgraded socket (the #32239 "Invalid socket" re-entry).The #32241
kupgradedToTLSflag cannot be reused here: suppressingself.push(buffer)is exactly what would starve the only source of thedataevents the feeder consumes, breaking TLS over named pipes entirely.Fix
Route the named-pipe handle's post-upgrade reads straight to the TLS data feeder instead of through the public
dataevent:events[0]on the connection asconnection[kDuplexTLSData](only on the named-pipe branch, not for a plain Duplex).self.pushnever runs and the original socket goes quiet after the upgrade, matching Node and the net: stop re-emitting post-upgrade bytes on the original socket after tls.connect({ socket }) #32241 TCP contract.The fixing lines are the
connection[kDuplexTLSData] = events[0]assignments in the twoupgradeDuplexbranches plus the feeder check in every raw data handler.isNamedPipeSocketis true for any pipe-variant handle, so the feeder can be set on a client named-pipe socket (SocketHandlers2.data, or theonreadvariant) or a server-accepted named-pipe socket (ServerHandlers.data); thefd-connect path (SocketHandlers.data) is covered defensively. A plainDuplextransport keepsconnection.on("data", events[0])(it has no native handle to feed from, and its behavior already matches Node).Scope
isNamedPipeSocketis always false on POSIX, so the named-pipe branch is never taken on Linux/macOS and this change is a no-op there.main; does not depend on net: stop re-emitting post-upgrade bytes on the original socket after tls.connect({ socket }) #32241'skupgradedToTLS.Testing
test/js/node/tls/node-tls-namedpipes.test.tsgains two STARTTLS regressions driven over a named pipe (it.if(isWindows)), modeled on #32241's proven TCP test: a mock peer greets, repliesPROCEED, then sends mock TLS records. Without the fix those bytes re-enter the upgrader'sdatahandler and attempt a second upgrade (upgrades > 1); with the fix exactly one upgrade happens and it never fails withInvalid socket. One test upgrades a clientnet.connectsocket (SocketHandlers2.data), the other upgrades a server-accepted socket (ServerHandlers.data).This path is reachable only on Windows, so the fail-before/after check on a Linux runner cannot exercise it (the test is skipped there). Verification done on Linux:
node-tls-upgrade.test.ts,node-tls-duplex-close-throw-uaf.test.ts,node-tls-socket-allow-half-open-option.test.ts.kDuplexTLSDatais only set whenisNamedPipeSocketis true, and thenamedPipecomputation is equivalent to the previousupgradeDuplex = isNamedPipeSocket(socket)on every other input.The regression test runs on Windows CI.