Repository navigation
child_process: reject a tls.TLSSocket in stdio like node - #43722
Conversation
spawn() and spawnSync() took the descriptor of any stream that has `_handle.fd`. For a tls.TLSSocket that is the raw TCP descriptor under the TLS session: a child that reads it gets TLS records, and what a child writes to it goes into the TLS stream as plaintext. Node accepts only pipe, TCP, TTY and UDP handles here and throws ERR_INVALID_ARG_VALUE for a TLSSocket. Do the same, with the check that subprocess.send() already uses to refuse a TLS socket as an IPC handle.
|
Status: fix and tests are pushed. This PR is #43722. How I reproduced it (Linux x64, bun 1.4.3-canary.1 367d939, node v26.3.0 for comparison):
The new test |
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.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; 0 remain after this review. WalkthroughThe child-process stdio descriptor helper now rejects Bun TLS sockets with ChangesSocket stdio handling
Suggested reviewers: Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:04 PM PT - Sep 21st, 2026
✅ @robobun, your commit 1b70102523b2b31e0e92dfa759501095aefc3541 passed in 🧪 To try this PR locally: bunx bun-pr 43722That installs a local version of the PR into your bun-43722 --bun |
ERR_INVALID_ARG_VALUE inspects the value it receives into the message. For a tls.TLSSocket the inspected object reaches the key and the passphrase of its server or of its connect options, so the message carried them. Pass a depth -1 inspect of the socket, which is the class name in brackets, and pin the exact message in the test.
There was a problem hiding this comment.
The follow-up commit addresses the secret-exposure concern from the earlier review, and I found no bugs in the current diff; a human look is still worthwhile because the error message now deliberately departs from Node's text, which is a compat call a maintainer should make.
What was reviewed:
- Confirmed
inspect(item, { depth: -1 })hits the depth cutoff insrc/js/internal/util/inspect.js:1467and yields[TLSSocket], and thatnet.Socket/TLSSocketdefine no custom inspect hook that could bypass it. - The pre-rendered string is quoted by the shared
ERR_INVALID_ARG_VALUEbuilder, so the message readsReceived '[TLSSocket]'rather than Node's unquotedReceived TLSSocket {...(cut at 128 chars); this is intentional and the test pins it, but it is a visible deviation. - Checked the check's placement after the own-
fdlookup and before_handle.fd, matching Node's ordering; bothspawnandspawnSyncreach it throughnodeToBun. - The
toThrow(expect.objectContaining(...))shape is used elsewhere in the suite; no debug build was available here, so I did not execute the new tests locally.
Extended reasoning...
The change adds a four-line guard in streamFdOf in src/js/node/child_process.ts that throws ERR_INVALID_ARG_VALUE when a stdio entry is a tls.TLSSocket (detected by the ::buntls:: marker), plus a new describe block in test/js/node/child_process/child_process.test.ts covering both socket ends in stdio slots 0 and 1 for spawn and spawnSync, and a net.Socket control. It touches TLS-adjacent data exposure only in that the error message previously could inspect the socket graph; the second commit limits it to the class name. Deferring rather than approving because the message format intentionally differs from Node's and that trade-off is a maintainer decision, and because I could not run the tests locally to confirm they pass on this branch.
|
On the message text, for the maintainer who decides it:
I kept the class name because it does not depend on that order and does not need #38402. If you prefer node's shape, I will switch this line back to the socket once #38402 is merged. |
Problem
spawn()andspawnSync()accept atls.TLSSocketinstdioand give the child the raw TCP descriptor under the TLS session. The child reads TLS records (17 03 03 ...). A child that writes puts plaintext into the TLS stream. Node throwsERR_INVALID_ARG_VALUE.streamFdOf(src/js/node/child_process.ts:1752). It returns_handle.fdfor any stream. Node accepts a stream only when its handle is a pipe, TCP, TTY or UDP wrap (child_process.js#L1063-L1083).Fix
streamFdOfthrowsERR_INVALID_ARG_VALUEfor atls.TLSSocketbefore it reads_handle.fd. It reuses the check thatsubprocess.send()has for a TLS socket (src/js/builtins/Ipc.ts:31).Received '[TLSSocket]'. The error builder inspects its value in full, and an inspectedTLSSocketreaches its key and passphrase._handle.fddoes not carry the stream's own bytes. Anet.Socketis still accepted.test/js/node/child_process/child_process.test.ts(a socket as a stdio entry, the TLS test fails without the fix). Alsotest/js/node/child_process/and 18 vendoredtest-child-process-*scripts.Background
stdioshares its descriptor with the child: both processes hold the same kernel socket.tls.TLSSocketis anet.Socketwhose bytes pass through a TLS session in the parent. Its kernel socket carries only TLS records.socket._handleis the native socket object. NativeTCPSocketandTLSSocketexposefd.tls.TLSSocketand its subclasses have theSymbol.for("::buntls::")method. net.ts and Ipc.ts detect a TLS socket with it.Notes
Repro. A TLS server hands its accepted socket to a child as stdin. The peer sends 20,000 bytes of
0x41. The child counts what it reads (python3, so the reader does not depend on Bun).spawn() throws ERR_INVALID_ARG_VALUEchild read 1022 bytes; first 5: 17030303f9; parent socket.bytesRead=5000, thenchild read 0 bytes, thenchild read 1046 bytes; first 5: 17030303f9spawn() throws ERR_INVALID_ARG_VALUECompared with node v26.3.0, entry by entry (Linux x64,
spawn("true", [], { stdio: [entry, "ignore", "ignore"] })):TLSSocketthat atls.Serveraccepted, at index 0, 1, 2 or 3,spawnandspawnSyncERR_INVALID_ARG_VALUEERR_INVALID_ARG_VALUETLSSocketfromtls.connect()ERR_INVALID_ARG_VALUEERR_INVALID_ARG_VALUETLSSocketfromtls.connect({ socket })ERR_INVALID_ARG_VALUEERR_INVALID_ARG_VALUEnet.Socketunder that wrapsession.socketof a secure HTTP/2 sessionERR_INVALID_ARG_VALUEERR_INVALID_ARG_VALUEnew tls.TLSSocket(), a destroyedTLSSocket, a subclass instanceERR_INVALID_ARG_VALUEError("without an underlying file descriptor")ERR_INVALID_ARG_VALUETLSSocketwith an own numericfdpropertynet.SocketNode takes a numeric
fdproperty before it looks at handles, so the new check sits after the ownfdcheck.Why only TLS. Node has an allowlist of handle wraps. Bun has no wrap classes: a stream reaches the child through
_handle.fd, and the only handles with anfdare the nativeTCPSocket(TCP and unix sockets, the raw transport) and the nativeTLSSocket. fs, tty andprocess.std*streams carry their ownfd. The HTTP/2session.socketproxy forwards the::buntls::lookup to itsTLSSocket, so the same check covers it.Error message.
$ERR_INVALID_ARG_VALUE(name, value)inspectsvalueinto the message with no depth or length limit. The first commit passed the socket. With a PEM stringkeyand apassphrase, the message was 81,943 characters for an accepted socket and 16,431 for a connected one, and it contained the key body and the passphrase. Node's message is 173 characters (node cuts the inspected value at 128) and contains neither. The second commit keeps the socket away from the builder.inspect(item, { depth: -1 })is the shallow form thatnode:eventsuses for the MaxListeners warning. It reads the constructor name and no property value:'[TLSSocket]','[Sub]'for a subclass,'[ServerHttp2Session]'for the HTTP/2session.socketproxy. The quotes are there because the builder quotes a string value. The test pins the exact message. #38402 (open) adds node's 128 character cut to the builder for every caller. This PR does not depend on it.Windows. Checked on canary 1.4.3-canary.1+9cba9036a: any socket in
stdiofails withEBADF: bad file descriptor, uv_spawn(node:spawn ENOTSUP). The new check runs before that, so the TLS test is not platform specific. Thenet.Socketcontrol is POSIX only.Related open PRs on the same mapping: #35226 (error codes for entries with no fd), #39220 (entries that carry an
fd), #43707 (stop the parent's reads after a hand-off). None of them rejects a TLS socket. A trial merge with #43707 conflicts in one line, thenode:netimport ofchild_process.test.ts(keep #43707's line, it is a superset of this one). #35226 and #39220 conflict instreamFdOf, so whichever merges later needs a small rebase.Local runs of
test/js/node/child_process/(debug build). Three tests fail, none through this diff (a fourth, "should allow us to set env", takes 4.6 s to 5.1 s on my debug build and crosses the 5 s timeout in some runs):child_process.test.ts, "should allow us to spawn in the default shell": also fails on the release canary in my container.child_process.test.ts, "extra stdio pipes are not double-closed on GC": 5 s timeout. Its script printsOKafter 6.2 s on a debug build (test(child_process): keep the extra stdio GC test under the timeout on debug builds #39687).child-process-exec.test.ts, "stderr > exceeding maxBuffer should throw": fails the same way on a debug build of main without this change, passes on the release canary.child_process_ipc_handle.test.tshit the 5 s timeout in 2 tests on one run and passed 14 of 14 on the next. Of the 18 vendored scripts,test-child-process-emfile.jsfails only on a debug build: it loads built-in modules from disk and cannot underEMFILE. It passes on the release canary.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/child_process/child_process.test.ts