child_process: honor uid and gid spawn options - #33060
Merged
Merged
Conversation
node:child_process validated `uid`/`gid` but never forwarded them, so the child always ran with the parent's ids. Plumb them from `Bun.spawn` / `Bun.spawnSync` (and the node:child_process wrappers) through to the spawned child, following libuv's ordering and error semantics: - POSIX: after chdir and the fd actions, drop supplementary groups (best-effort), then setgid, then setuid; a failure surfaces through the existing child-errno channel as a synchronous EPERM, matching Node's dispatch (EPERM is not in Node's deferred-to-'error'-event list). On Linux the child is a vfork child sharing the parent's address space, so the id changes use raw syscalls instead of the glibc wrappers (which broadcast SIGSETXID across the parent's thread list). - macOS: route uid/gid spawns through the fork-based posix_spawn_bun path, since Darwin's posix_spawn cannot change ids (same fallback libuv makes). - Windows: set uv_process_options_t uid/gid + UV_PROCESS_SETUID/SETGID so libuv returns ENOTSUP, like Node. Synchronously thrown spawn errors now carry `syscall: "spawn"` like Node.
Collaborator
|
Updated 8:36 AM PT - Jun 29th, 2026
❌ @Jarred-Sumner, your commit 8f0f36e has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33060That installs a local version of the PR into your bun-33060 --bun |
Contributor
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…able flag On Linux the kernel clears PR_SET_PDEATHSIG when the child's effective uid/gid changes, so re-arm it after the setgid/setuid calls. The vfork child also shares the parent's mm, so commit_creds resets the mm-wide dumpable flag; save it before vfork and restore it in the parent. Also delete the now-stale uid/gid TODO in child_process.ts.
Contributor
WalkthroughAdds Changesuid/gid spawn support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/jsc/bindings/bun-spawn.cpp`:
- Around line 161-165: The dumpable-flag comment near the saved_dumpable
initialization is too long and needs to be condensed to fit the 3-line comment
limit. Rewrite the existing explanatory block around saved_dumpable in
bun-spawn.cpp so it keeps only the essential rationale about vfork/setuid/setgid
resetting dumpable and restoring it after vfork returns, while preserving the
reference to the parent restore behavior in the relevant spawn path.
In `@src/spawn_sys/posix_spawn.rs`:
- Around line 611-621: Condense the comment above use_bun_spawn in
posix_spawn.rs to 3 lines max and make it match the actual condition. Keep only
the essential spawn-path rationale, explicitly note FreeBSD alongside Linux, and
trim the macOS explanation so it stays concise while still covering the PTY and
uid/gid fallback cases.
In `@test/cli/run/no-orphans.test.ts`:
- Around line 55-68: The child-nonbun-uid.js fixture only verifies shell
readiness, so it may not prove the uid/gid drop path is actually exercised.
Update the Bun.spawn setup in child-nonbun-uid.js to have the grandchild emit or
assert its numeric uid/gid before signaling readiness, then make the parent test
validate those values before continuing. This ensures the repro in
no-orphans.test.ts depends on the credential change behavior and not just on
/bin/sh starting successfully.
In `@test/js/bun/spawn/spawn.test.ts`:
- Around line 1234-1245: The async Bun.spawn() coverage around the existing
uid/gid cases does not actually verify gid, so update the tests in the spawn
test block to observe gid explicitly using the existing spawn() setup and
proc/exited flow. Adjust the first case so it checks both ids for the child
process, and update the second case to assert the unchanged gid as well as uid,
using the same isPosix && isRoot guards and the current id-based helpers to keep
the coverage strong.
In `@test/js/bun/spawn/spawnSync.test.ts`:
- Around line 81-84: Update the assertion in the spawnSync test so it compares
the child’s after value against its before value instead of hardcoding both to
1; this test should verify that the dumpable flag is preserved across the spawn.
Locate the expectation around JSON.parse(stdout) in the spawnSync test and keep
the exitCode check, but change the result assertion to validate the relationship
between before and after rather than assuming a specific initial state.
In `@test/js/node/child_process/child_process.test.ts`:
- Around line 755-764: The async spawn() test in child_process.test.ts only
validates uid because it runs id -u, so it never proves that gid forwarding
works. Update the existing spawn applies uid/gid (async) case to assert gid as
well, using the spawn() call and its stdout parsing so the test checks both
NOBODY uid and NOBODY gid and fails for the right reason if gid forwarding
breaks.
🪄 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: d072e92c-6550-4a70-a6e6-d7bfeed1930b
📒 Files selected for processing (12)
packages/bun-types/bun.d.tssrc/bun_core/util.rssrc/js/node/child_process.tssrc/jsc/bindings/bun-spawn.cppsrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/spawn/process.rssrc/spawn_sys/posix_spawn.rssrc/spawn_sys/spawn_process.rstest/cli/run/no-orphans.test.tstest/js/bun/spawn/spawn.test.tstest/js/bun/spawn/spawnSync.test.tstest/js/node/child_process/child_process.test.ts
Comment on lines
+161
to
+165
| // The vfork child shares this mm, and set*id in the child resets the | ||
| // mm-wide "dumpable" flag to /proc/sys/fs/suid_dumpable (commit_creds). | ||
| // Save it so the parent can restore it once vfork returns, like Go's | ||
| // forkAndExecInChild1 and systemd's safe_fork_full do. | ||
| int saved_dumpable = (request->set_uid || request->set_gid) ? prctl(PR_GET_DUMPABLE, 0, 0, 0, 0) : -1; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Keep the new dumpable rationale within 3 comment lines.
Suggested rewrite
- // The vfork child shares this mm, and set*id in the child resets the
- // mm-wide "dumpable" flag to /proc/sys/fs/suid_dumpable (commit_creds).
- // Save it so the parent can restore it once vfork returns, like Go's
- // forkAndExecInChild1 and systemd's safe_fork_full do.
+ // set*id in the vfork child resets the shared mm's dumpable flag.
+ // Save it so the parent can restore it after vfork returns, matching
+ // Go's forkAndExecInChild1 and systemd's safe_fork_full.As per coding guidelines, "Keep code comments to 3 lines max."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The vfork child shares this mm, and set*id in the child resets the | |
| // mm-wide "dumpable" flag to /proc/sys/fs/suid_dumpable (commit_creds). | |
| // Save it so the parent can restore it once vfork returns, like Go's | |
| // forkAndExecInChild1 and systemd's safe_fork_full do. | |
| int saved_dumpable = (request->set_uid || request->set_gid) ? prctl(PR_GET_DUMPABLE, 0, 0, 0, 0) : -1; | |
| // set*id in the vfork child resets the shared mm's dumpable flag. | |
| // Save it so the parent can restore it after vfork returns, matching | |
| // Go's forkAndExecInChild1 and systemd's safe_fork_full. | |
| int saved_dumpable = (request->set_uid || request->set_gid) ? prctl(PR_GET_DUMPABLE, 0, 0, 0, 0) : -1; |
🤖 Prompt for 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.
In `@src/jsc/bindings/bun-spawn.cpp` around lines 161 - 165, The dumpable-flag
comment near the saved_dumpable initialization is too long and needs to be
condensed to fit the 3-line comment limit. Rewrite the existing explanatory
block around saved_dumpable in bun-spawn.cpp so it keeps only the essential
rationale about vfork/setuid/setgid resetting dumpable and restoring it after
vfork returns, while preserving the reference to the parent restore behavior in
the relevant spawn path.
Source: Coding guidelines
Comment on lines
611
to
+621
| // Use posix_spawn_bun when: | ||
| // - Linux: always (uses vfork which is fast and safe) | ||
| // - macOS: only for PTY spawns (pty_slave_fd >= 0) because PTY setup requires | ||
| // setsid() + ioctl(TIOCSCTTY) before exec, which system posix_spawn can't do. | ||
| // For non-PTY spawns on macOS, we use system posix_spawn which is safer | ||
| // - macOS: for PTY spawns (pty_slave_fd >= 0) because PTY setup requires | ||
| // setsid() + ioctl(TIOCSCTTY) before exec, which system posix_spawn can't do, | ||
| // and for uid/gid spawns because Darwin's posix_spawn cannot change ids | ||
| // (libuv makes the same fork() fallback for UV_PROCESS_SETUID/SETGID). | ||
| // For other spawns on macOS, we use system posix_spawn which is safer | ||
| // (Apple's posix_spawn uses a kernel fast-path that avoids fork() entirely). | ||
| let use_bun_spawn = cfg!(any(target_os = "linux", target_os = "android")) | ||
| || cfg!(target_os = "freebsd") | ||
| || (cfg!(target_os = "macos") && pty_slave_fd >= 0); | ||
| || (cfg!(target_os = "macos") && (pty_slave_fd >= 0 || uid.is_some() || gid.is_some())); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Condense the spawn-path comment and include FreeBSD.
This new comment is longer than the repo’s comment guideline and describes Linux/macOS while the condition also enables FreeBSD.
Suggested rewrite
- // Use posix_spawn_bun when:
- // - Linux: always (uses vfork which is fast and safe)
- // - macOS: for PTY spawns (pty_slave_fd >= 0) because PTY setup requires
- // setsid() + ioctl(TIOCSCTTY) before exec, which system posix_spawn can't do,
- // and for uid/gid spawns because Darwin's posix_spawn cannot change ids
- // (libuv makes the same fork() fallback for UV_PROCESS_SETUID/SETGID).
- // For other spawns on macOS, we use system posix_spawn which is safer
- // (Apple's posix_spawn uses a kernel fast-path that avoids fork() entirely).
+ // Use posix_spawn_bun on Linux/Android/FreeBSD, and on macOS when PTY
+ // or uid/gid setup needs Bun's fork path instead of Darwin posix_spawn.
+ // Plain macOS spawns stay on the system fast path.As per coding guidelines, "Keep code comments to 3 lines max."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Use posix_spawn_bun when: | |
| // - Linux: always (uses vfork which is fast and safe) | |
| // - macOS: only for PTY spawns (pty_slave_fd >= 0) because PTY setup requires | |
| // setsid() + ioctl(TIOCSCTTY) before exec, which system posix_spawn can't do. | |
| // For non-PTY spawns on macOS, we use system posix_spawn which is safer | |
| // - macOS: for PTY spawns (pty_slave_fd >= 0) because PTY setup requires | |
| // setsid() + ioctl(TIOCSCTTY) before exec, which system posix_spawn can't do, | |
| // and for uid/gid spawns because Darwin's posix_spawn cannot change ids | |
| // (libuv makes the same fork() fallback for UV_PROCESS_SETUID/SETGID). | |
| // For other spawns on macOS, we use system posix_spawn which is safer | |
| // (Apple's posix_spawn uses a kernel fast-path that avoids fork() entirely). | |
| let use_bun_spawn = cfg!(any(target_os = "linux", target_os = "android")) | |
| || cfg!(target_os = "freebsd") | |
| || (cfg!(target_os = "macos") && pty_slave_fd >= 0); | |
| || (cfg!(target_os = "macos") && (pty_slave_fd >= 0 || uid.is_some() || gid.is_some())); | |
| // Use posix_spawn_bun on Linux/Android/FreeBSD, and on macOS when PTY | |
| // or uid/gid setup needs Bun's fork path instead of Darwin posix_spawn. | |
| // Plain macOS spawns stay on the system fast path. | |
| let use_bun_spawn = cfg!(any(target_os = "linux", target_os = "android")) | |
| || cfg!(target_os = "freebsd") | |
| || (cfg!(target_os = "macos") && (pty_slave_fd >= 0 || uid.is_some() || gid.is_some())); |
🤖 Prompt for 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.
In `@src/spawn_sys/posix_spawn.rs` around lines 611 - 621, Condense the comment
above use_bun_spawn in posix_spawn.rs to 3 lines max and make it match the
actual condition. Keep only the essential spawn-path rationale, explicitly note
FreeBSD alongside Linux, and trim the macOS explanation so it stays concise
while still covering the PTY and uid/gid fallback cases.
Source: Coding guidelines
Comment on lines
+55
to
+68
| // Same as child-nonbun.js but the grandchild drops to nobody. On Linux the | ||
| // kernel clears PR_SET_PDEATHSIG when the child's effective ids change | ||
| // (prctl(2)), so the spawn must re-arm it after setgid/setuid. | ||
| "child-nonbun-uid.js": ` | ||
| const gc = Bun.spawn({ | ||
| cmd: ["/bin/sh", "-c", "echo r; while :; do sleep 1; done"], | ||
| uid: 65534, | ||
| gid: 65534, | ||
| stdio: ["ignore", "pipe", "ignore"], | ||
| }); | ||
| await gc.stdout.getReader().read(); | ||
| console.log(process.pid, process.ppid, gc.pid); | ||
| setInterval(()=>{}, 1000); | ||
| `, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify the grandchild actually dropped credentials before using it as the repro.
Right now this fixture only waits for /bin/sh to print a readiness byte. If uid/gid stop applying here, the grandchild stays root, the credential-change-specific path is never exercised, and the outer reap test still passes.
Have the grandchild emit/assert its numeric uid/gid before the readiness signal, then validate those values in the parent test. As per coding guidelines, tests should "prove the test fails for the RIGHT reason" and "assert that setup created the precondition."
🤖 Prompt for 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.
In `@test/cli/run/no-orphans.test.ts` around lines 55 - 68, The
child-nonbun-uid.js fixture only verifies shell readiness, so it may not prove
the uid/gid drop path is actually exercised. Update the Bun.spawn setup in
child-nonbun-uid.js to have the grandchild emit or assert its numeric uid/gid
before signaling readiness, then make the parent test validate those values
before continuing. This ensures the repro in no-orphans.test.ts depends on the
credential change behavior and not just on /bin/sh starting successfully.
Source: Coding guidelines
Comment on lines
+1234
to
+1245
| it.if(isPosix && isRoot)("applies uid and gid to the child", async () => { | ||
| await using proc = spawn({ cmd: ["id", "-u"], uid: 65534, gid: 65534, stdout: "pipe" }); | ||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
| expect(stdout.trim()).toBe("65534"); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| it.if(isPosix && isRoot)("omitting uid/gid leaves the child's ids unchanged", async () => { | ||
| await using proc = spawn({ cmd: ["id", "-u"], stdout: "pipe" }); | ||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
| expect(stdout.trim()).toBe("0"); | ||
| expect(exitCode).toBe(0); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert gid in the async Bun.spawn() coverage.
These cases never observe gid: the first one runs id -u, and the second only checks that uid stays 0. If async gid forwarding regresses, this suite still passes.
Suggested fix
- it.if(isPosix && isRoot)("applies uid and gid to the child", async () => {
- await using proc = spawn({ cmd: ["id", "-u"], uid: 65534, gid: 65534, stdout: "pipe" });
+ it.if(isPosix && isRoot)("applies uid and gid to the child", async () => {
+ await using proc = spawn({ cmd: ["id"], uid: 65534, gid: 65534, stdout: "pipe" });
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
- expect(stdout.trim()).toBe("65534");
+ expect(stdout).toContain("uid=65534");
+ expect(stdout).toContain("gid=65534");
expect(exitCode).toBe(0);
});
it.if(isPosix && isRoot)("omitting uid/gid leaves the child's ids unchanged", async () => {
- await using proc = spawn({ cmd: ["id", "-u"], stdout: "pipe" });
+ await using proc = spawn({ cmd: ["id"], stdout: "pipe" });
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
- expect(stdout.trim()).toBe("0");
+ expect(stdout).toContain(`uid=${process.getuid?.()}`);
+ expect(stdout).toContain(`gid=${process.getgid?.()}`);
expect(exitCode).toBe(0);
});As per coding guidelines, tests should "prove the test fails for the RIGHT reason" and "assert the strongest invariant."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it.if(isPosix && isRoot)("applies uid and gid to the child", async () => { | |
| await using proc = spawn({ cmd: ["id", "-u"], uid: 65534, gid: 65534, stdout: "pipe" }); | |
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | |
| expect(stdout.trim()).toBe("65534"); | |
| expect(exitCode).toBe(0); | |
| }); | |
| it.if(isPosix && isRoot)("omitting uid/gid leaves the child's ids unchanged", async () => { | |
| await using proc = spawn({ cmd: ["id", "-u"], stdout: "pipe" }); | |
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | |
| expect(stdout.trim()).toBe("0"); | |
| expect(exitCode).toBe(0); | |
| it.if(isPosix && isRoot)("applies uid and gid to the child", async () => { | |
| await using proc = spawn({ cmd: ["id"], uid: 65534, gid: 65534, stdout: "pipe" }); | |
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | |
| expect(stdout).toContain("uid=65534"); | |
| expect(stdout).toContain("gid=65534"); | |
| expect(exitCode).toBe(0); | |
| }); | |
| it.if(isPosix && isRoot)("omitting uid/gid leaves the child's ids unchanged", async () => { | |
| await using proc = spawn({ cmd: ["id"], stdout: "pipe" }); | |
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | |
| expect(stdout).toContain(`uid=${process.getuid?.()}`); | |
| expect(stdout).toContain(`gid=${process.getgid?.()}`); | |
| expect(exitCode).toBe(0); |
🤖 Prompt for 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.
In `@test/js/bun/spawn/spawn.test.ts` around lines 1234 - 1245, The async
Bun.spawn() coverage around the existing uid/gid cases does not actually verify
gid, so update the tests in the spawn test block to observe gid explicitly using
the existing spawn() setup and proc/exited flow. Adjust the first case so it
checks both ids for the child process, and update the second case to assert the
unchanged gid as well as uid, using the same isPosix && isRoot guards and the
current id-based helpers to keep the coverage strong.
Source: Coding guidelines
Comment on lines
+81
to
+84
| expect({ result: JSON.parse(stdout), exitCode }).toEqual({ | ||
| result: { before: 1, after: 1, childUid: "65534" }, | ||
| exitCode: 0, | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Compare after to before, not to a hardcoded dumpable state.
This test is meant to prove the flag is preserved. Requiring both values to be 1 will fail on any environment that legitimately starts non-dumpable.
Suggested fix
- expect({ result: JSON.parse(stdout), exitCode }).toEqual({
- result: { before: 1, after: 1, childUid: "65534" },
- exitCode: 0,
- });
+ const result = JSON.parse(stdout);
+ expect(result.childUid).toBe("65534");
+ expect(result.before).toBeGreaterThanOrEqual(0);
+ expect(result.after).toBe(result.before);
+ expect(exitCode).toBe(0);As per coding guidelines, tests should "assert the strongest invariant."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect({ result: JSON.parse(stdout), exitCode }).toEqual({ | |
| result: { before: 1, after: 1, childUid: "65534" }, | |
| exitCode: 0, | |
| }); | |
| const result = JSON.parse(stdout); | |
| expect(result.childUid).toBe("65534"); | |
| expect(result.before).toBeGreaterThanOrEqual(0); | |
| expect(result.after).toBe(result.before); | |
| expect(exitCode).toBe(0); |
🤖 Prompt for 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.
In `@test/js/bun/spawn/spawnSync.test.ts` around lines 81 - 84, Update the
assertion in the spawnSync test so it compares the child’s after value against
its before value instead of hardcoding both to 1; this test should verify that
the dumpable flag is preserved across the spawn. Locate the expectation around
JSON.parse(stdout) in the spawnSync test and keep the exitCode check, but change
the result assertion to validate the relationship between before and after
rather than assuming a specific initial state.
Source: Coding guidelines
Comment on lines
+755
to
+764
| it.skipIf(isWindows || !isRoot)("spawn applies uid/gid (async)", async () => { | ||
| const { promise, resolve, reject } = Promise.withResolvers<{ out: string; code: number | null }>(); | ||
| const child = spawn("id", ["-u"], { uid: NOBODY, gid: NOBODY }); | ||
| let out = ""; | ||
| child.stdout.on("data", d => (out += d)); | ||
| child.on("error", reject); | ||
| child.on("close", code => resolve({ out, code })); | ||
| const { out: stdout, code } = await promise; | ||
| expect(stdout.trim()).toBe(`${NOBODY}`); | ||
| expect(code).toBe(0); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The async spawn() test doesn't exercise gid.
This case runs id -u, so the new gid forwarding can break without failing the test.
Suggested fix
- const child = spawn("id", ["-u"], { uid: NOBODY, gid: NOBODY });
+ const child = spawn("id", [], { uid: NOBODY, gid: NOBODY });
@@
- expect(stdout.trim()).toBe(`${NOBODY}`);
+ expect(stdout).toContain(`uid=${NOBODY}`);
+ expect(stdout).toContain(`gid=${NOBODY}`);
expect(code).toBe(0);As per coding guidelines, tests should "prove the test fails for the RIGHT reason" and "assert the strongest invariant."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it.skipIf(isWindows || !isRoot)("spawn applies uid/gid (async)", async () => { | |
| const { promise, resolve, reject } = Promise.withResolvers<{ out: string; code: number | null }>(); | |
| const child = spawn("id", ["-u"], { uid: NOBODY, gid: NOBODY }); | |
| let out = ""; | |
| child.stdout.on("data", d => (out += d)); | |
| child.on("error", reject); | |
| child.on("close", code => resolve({ out, code })); | |
| const { out: stdout, code } = await promise; | |
| expect(stdout.trim()).toBe(`${NOBODY}`); | |
| expect(code).toBe(0); | |
| it.skipIf(isWindows || !isRoot)("spawn applies uid/gid (async)", async () => { | |
| const { promise, resolve, reject } = Promise.withResolvers<{ out: string; code: number | null }>(); | |
| const child = spawn("id", [], { uid: NOBODY, gid: NOBODY }); | |
| let out = ""; | |
| child.stdout.on("data", d => (out += d)); | |
| child.on("error", reject); | |
| child.on("close", code => resolve({ out, code })); | |
| const { out: stdout, code } = await promise; | |
| expect(stdout).toContain(`uid=${NOBODY}`); | |
| expect(stdout).toContain(`gid=${NOBODY}`); | |
| expect(code).toBe(0); |
🤖 Prompt for 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.
In `@test/js/node/child_process/child_process.test.ts` around lines 755 - 764, The
async spawn() test in child_process.test.ts only validates uid because it runs
id -u, so it never proves that gid forwarding works. Update the existing spawn
applies uid/gid (async) case to assert gid as well, using the spawn() call and
its stdout parsing so the test checks both NOBODY uid and NOBODY gid and fails
for the right reason if gid forwarding breaks.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Thanks for addressing all three earlier findings in 8f0f36e — the PDEATHSIG re-arm, dumpable save/restore, and TODO cleanup all look correct with good regression coverage. No further issues found, but since this is privilege-dropping code (setuid/setgid in a vfork child) across three platforms, it's worth a human pass before merge.
Extended reasoning...
Overview
This PR implements uid/gid options end-to-end for Bun.spawn, Bun.spawnSync, and node:child_process. It touches 12 files: the native vfork/fork spawn path in bun-spawn.cpp (adding setgroups/setgid/setuid calls in the child, with raw syscalls on Linux to avoid NPTL SIGSETXID broadcast), the Rust FFI struct (BunSpawnRequest), option plumbing through spawn_process.rs/posix_spawn.rs/process.rs, JS-side validation and forwarding in js_bun_spawn_bindings.rs and child_process.ts, libuv flag passthrough on Windows, and TypeScript types. ~250 lines of new tests across four test files including root-gated success-path tests, EPERM error-path tests, and regression tests for the two vfork-credential kernel interactions I flagged earlier.
Security risks
This is privilege-management code. The implementation follows libuv's order (best-effort setgroups(0, NULL) → setgid → setuid), uses raw syscalls on Linux to avoid corrupting the parent's thread credentials under vfork, routes macOS through the fork path since Darwin's posix_spawn can't change ids, and surfaces failures via the existing child-errno channel. The two vfork+setuid kernel side-effects I raised on the first revision (PDEATHSIG cleared on credential change; parent's mm-wide dumpable flag reset) are now handled with the same approach Go and systemd use, and both have root-gated regression tests. I don't see remaining correctness issues, but the consequence of a bug here is a privilege boundary, so it merits human review.
Level of scrutiny
High. This is new feature code in the process-spawn hot path that performs credential changes inside a vfork child sharing the parent's address space — an area with well-documented footguns (several of which the author has now explicitly handled). It also adds a macOS-specific routing decision (uid/gid forces the fork-based posix_spawn_bun path instead of the system posix_spawn fast path) and Windows ENOTSUP behavior via libuv flags. Each platform path is distinct.
Other factors
All three of my earlier inline findings were confirmed by the author and fixed in 8f0f36e with targeted regression tests (no-orphans.test.ts for the PDEATHSIG re-arm, spawnSync.test.ts for the dumpable-flag restore via bun:ffi prctl). The PR description documents side-by-side verification against Node v26.3.0. Test coverage is thorough, including a root-gated fixture that drops to uid 65534 to exercise the EPERM path on root CI runners. Given the security-sensitive surface and cross-platform native changes, I'm deferring rather than approving.
Jarred-Sumner
added a commit
that referenced
this pull request
Jul 4, 2026
## What
A hardening and robustness pass across the runtime: input validation,
bounds checking, protocol-state handling, and object-lifetime
correctness in ~200 files. It contains 122 individual fixes and ~190 new
tests (171 new `test`/`it` blocks, several parameterized, across 79
existing test files). No new API is introduced; every behavioral change
below has a test unless explicitly noted, and each is aligned with Node,
the relevant RFC/spec, or the upstream reference implementation.
## Potentially breaking / behavior-visible changes
Read this section first. Everything else in the PR preserves behavior
for valid inputs.
- **`Bun.serve` `request.url` is only synthesized from a structurally
valid `Host`.** For every HTTP/1.x request (`Bun.serve` and node-compat
servers alike), a `Host` value that is empty or contains bytes outside
`uri-host [":" port]` (RFC 3986 authority: alphanumerics, `.-:_~%[]` and
sub-delims) is never used as the authority of the synthesized
`request.url`; `request.url` falls back to the request target (e.g.
`/path`) and the request is still served. No request is rejected on the
basis of the `Host` field value, and a valid `Host` still round-trips
into `request.url` exactly. Why: `request.url` should never carry an
authority that cannot come back out of `new URL()`.
- **`fetch` rejects request lines it cannot legally serialize.** A URL
path/host (or, for proxied requests, the full href) containing a control
character, space, or DEL now fails with `InvalidURL` before any bytes
are written (RFC 9112 request-line grammar; normal `fetch()` input is
percent-encoded by the URL parser and unaffected). Also: a redirect
whose `Location` resolves to a non-http(s) scheme now fails with
`UnsupportedRedirectProtocol` (Fetch spec, matches undici); a `101`
arriving on the pre-tunnel leg of a proxied request is treated as an
unrequested upgrade; connections whose identity was accepted by a
per-request `checkServerIdentity` callback are never entered into or
taken from the keep-alive pool.
- **An own `__proto__` key from data files and macros is printed as a
computed key.** The `json`, `jsonc`, `json5`, `toml`, `yaml`, and
CSS-module loaders — and objects returned from Bun macros — now emit
`["__proto__"]: ...` so importing such data yields an own `__proto__`
property (like `JSON.parse`) instead of a prototype assignment. Who
notices: only code importing data with a `__proto__` key; matches
esbuild's JSON loader and Node semantics.
- **`node:url` legacy `url.parse` lookup tables no longer inherit from
`Object.prototype`** (both `node:url` and the browser fallback), the
hostless/slashed lookups use the lowercased protocol, and `url.parse(s,
true).query` is a null-prototype object (empty query included). All
three match Node's `lib/url.js` exactly. Who notices: code doing
`query.hasOwnProperty(...)` or parsing schemes named like `toString:`.
- **WebSocket client: missing negotiated subprotocol fails the
handshake.** Per RFC 6455 §4.1, if `new WebSocket(url, ["a"])` requested
subprotocols and the server's 101 omits `Sec-WebSocket-Protocol`, the
connection now closes with 1002 instead of opening with `ws.protocol ===
""`. Matches browsers, `ws`, and undici. Connections that request no
subprotocol are unaffected.
- **HTTP/2 (client and server) enforces RFC 9113 message framing.**
Trailer blocks must carry END_STREAM and no pseudo-headers;
`content-length` must be `1*DIGIT`, non-duplicated, and equal to the
DATA actually received (CONNECT exempt) — violations get
RST_STREAM(PROTOCOL_ERROR) instead of being delivered. With
`maxSessionMemory` exceeded, new peer streams are refused with
REFUSED_STREAM (retryable), and reset streams promptly release their
native state — Node/nghttp2 parity throughout. The all-streams teardown
helper now throws a `TypeError` for a non-numeric error code instead of
coercing it per stream.
- **`node:http2` HTTP/1 fallback (`allowHTTP1`) frames responses like
Node.** Header-name matching is case-insensitive; HEAD and
close-delimited responses don't get an auto `Transfer-Encoding:
chunked`/terminating chunk; `writeHead` now throws
`ERR_HTTP_INVALID_STATUS_CODE` / `ERR_INVALID_CHAR` like Node's
`ServerResponse`. Re-entrant `sendTrailers()` raises
`ERR_HTTP2_TRAILERS_ALREADY_SENT` in the same order Node does.
- **`node:http(s)` proxy `CONNECT` endpoint is validated with
`validateHeaderValue` in release builds** (previously a debug-only
assertion), so an invalid host/port surfaces as the same error Node
throws.
- **Glob: walking through a self-referential directory symlink
completes.** With `followSymlinks`, a link that resolves to one of its
own live ancestors is descended exactly once (like `find -L`, glibc
`fts`, node-glob); sibling/cousin links to the same target are still all
visited. One pre-existing test changed: it previously asserted the walk
failed with `ENAMETOOLONG` after the path grew past the limit; it now
asserts the scan completes.
- **Resolver: an `exports`/`imports` target whose expansion would exceed
the OS path limit is a normal resolution error** (`Invalid module
specifier` / `Invalid package target`, as Node models it) instead of a
hard failure.
- **Shell:** template arrays nested deeper than 100 levels throw a clear
error instead of recursing without bound; an interpolated string equal
to `if`/`then`/`elif`/`else`/`fi` is treated as data, never as a
reserved word (POSIX: reserved words are only recognized literally);
`$.escape` now quotes strings containing tab, CR, or `?` (word
delimiters / glob metacharacters).
- **`bun pack` / `bun publish` include/exclude matches npm-packlist.**
With a `"files"` field, the non-overridable defaults (`.git`, `.npmrc`,
`node_modules`, lockfiles) are now applied inside the `files` traversal
too; conversely `.hg` moved to the *overridable* default-ignore list, so
`"files"` can re-include it — exactly npm's split.
- **`bun upgrade` verifies the downloaded artifact against the digest
the GitHub Releases API reports** for that asset, and fails with a
retryable error on mismatch. If the API reports no (or an unrecognized)
digest, behavior is unchanged.
- **install:** an `integrity` string carrying several space-separated
digests (legal SSRI) is now parsed correctly and verified against the
strongest algorithm present (see Deviations); a stored `bun.lockb` with
a non-0/1 byte in a boolean slot fails validation instead of being
reinterpreted; lifecycle scripts for *registry* packages always come
from the installed `package.json` (never from lockfile bytes), matching
what Bun writes and what npm does; isolated installs apply the same
name/alias shape validation as hoisted installs; bin links reached
through a subdirectory get the same resolved-containment check the
dotted forms already had (npm only links files inside the package
folder).
- **`node:fs`:** `mode` arguments are no longer masked to `0o777`
(setuid/setgid/sticky pass through to the syscall, like Node);
`copyFile`/`cp` create the destination with the source's permission bits
(libuv parity); on Windows, `cp` copies directory junctions/symlinks via
the unprivileged-create + junction-fallback helpers and rewrites
`\\?\UNC\` targets to `\\server\share` form (libuv parity), so copying a
tree with junctions works without elevation; on macOS the
`clonefile`/`openat` paths use `NOFOLLOW` so the copy matches the
`lstat` classification (`dereference:false`).
- **Web plumbing observable from JS:** record conversion (`new
Headers(obj)`, fetch init, `URLSearchParams`, …) snapshots the key list
once and re-resolves keys mutated by a converter, exactly as Web IDL
specifies (deleted keys skipped, replaced values re-read) — released
Bun/Node/WebKit order preserved; `TextDecoder.decode` over a
`SharedArrayBuffer` or resizable buffer view snapshots the bytes first;
consuming a `Blob`/`Response` body no longer empties *other* objects
sharing the same byte store (transfer only when sole owner); deeply
nested serialized arrays in `structuredClone` data hit the same
recursion cap objects already had; the SIMD `decodeURIComponent` fast
path decodes non-ASCII input as UTF-8 (with U+FFFD for ill-formed
sequences) instead of throwing/garbling.
- **N-API / V8 API:** `napi_create_arraybuffer` returns zeroed memory
(Node contract); `napi_get_typedarray_info`/`napi_get_dataview_info`
report the view's real `byte_offset`; `v8::String::Utf8Length` returns
the exact byte count `WriteUtf8` will produce for ill-formed UTF-16;
`v8::Number::New` canonicalizes NaN payloads.
- **Dev-only endpoints check `Host`/`Origin`:** the inspector (`bun
--inspect`) HTTP/WebSocket endpoint applies its Host/Origin checks
before the `/json`* discovery routes and rejects non-matching DNS-name
`Host` values with 400 (Node inspector semantics); the bake/dev-server
internal routes require an allowed Host and same-origin for the
error-report/sourcemap endpoints; internal HMR pub/sub topics are
namespaced so user `publish`/`subscribe` topic strings can never collide
with them.
- **markdown:** reference-link expansion is charged against md4c's exact
output budget (`16 × min(input, 64 KiB)` scale); once exhausted, further
references degrade to literal bracketed text — no error — exactly as
md4c does.
- **Misc small behavior corrections:** `checkPrime` validates the
candidate before the options (Node's order); HKDF rejects non-secret
`KeyObject`s with `ERR_CRYPTO_INVALID_KEY_OBJECT_TYPE` (current Node);
Ed25519 sign/verify with a wrong-length key errors/returns false; X25519
JWK import honors `kty`/`crv`/`use`/`key_ops`/`ext`; SPKAC helpers
return false/empty for empty or whitespace-only input (Node);
`CookieMap.delete` of a `__Host-`/`__Secure-` cookie emits `Secure` so
browsers accept the expiry; postgres `escapeIdentifier` rejects embedded
NUL (`pg` parity); valkey/redis pending commands reject with "Connection
closed" on disconnect and out-of-band push frames never consume an
unrelated command's promise (RESP3); semver strings consisting only of
`v`/`=`/whitespace parse as `*` (node-semver); `Bun.wrapAnsi` measures
rows whose seam joins grapheme clusters (combining marks/ZWJ/VS16) the
way npm `wrap-ansi` does; bash/zsh completions handle script names
containing `:` and other special characters; the Docker images verify
(not just decode) the release checksum signature.
## Changes by area
- **HTTP server (uWS / `Bun.serve` / `node:http`)** —
`packages/bun-uws/HttpParser.h`, `packages/bun-usockets`,
`src/runtime/webcore/Request.rs`, `src/runtime/server`: the
`request.url` `Host` handling above (URL synthesis in `Request.rs` only
— the HTTP parser's `Host` handling is unchanged and every request is
served); CONNECT requests are framed as an opaque tunnel regardless of
`Transfer-Encoding`/`Content-Length` (RFC 9110 §9.3.6); TLS socket
relocation updates the loop's spill/last-error owner pointers so
bookkeeping never points at a moved socket; the bake-only route
additionally requires an allowed `Host`.
- **fetch / HTTP client / webcore** — `src/http/lib.rs`,
`src/http/ssl_config.rs`,
`src/runtime/webcore/{Request,Blob,TextDecoder}.rs`,
`src/jsc/bindings/webcore/*`,
`src/jsc/bindings/decodeURIComponentSIMD.cpp`, `src/url/lib.rs`,
`src/runtime/api/BunObject.rs`: everything in the highlights, plus:
`write_request` failures propagate their real error instead of being
reported as out-of-memory; partial-header/1xx short reads no longer
re-feed already-consumed bytes to the parser; `SSLConfig` now actually
applies `secureOptions` and the client-renegotiation limit/window it was
already accepting; `URLSearchParams` no longer drops a pair whose value
has a malformed percent sequence (WHATWG: never drop, decode lazily);
`AbortSignal` native listeners deregistered by an earlier abort callback
are not invoked with a stale context; the compression helpers
(`Bun.gzipSync` et al.) read the options object before coercing the
input buffer (so a getter can't invalidate the captured slice) and no
longer register a deallocator for an empty result's dangling sentinel
pointer (an invalid free at GC time under debug allocators).
- **node compat** — `src/runtime/node/*`, `src/js/node/*`,
`src/node-fallbacks/url.js`, `src/jsc/ipc.rs`, `src/runtime/socket`:
everything in the highlights, plus: `fs` path arguments from typed
arrays are always pinned for the call's duration; `BlockList`
structured-clone carries only an opaque per-instance nonce resolved
through a live table (round-trip unchanged); IPC advanced-mode frame
lengths are range-checked before span arithmetic, serialization failures
leave no partial frame in the send queue, and a received fd is closed if
its message fails to parse; TLS-upgrade `initialData` is copied to an
owned buffer before use; `node:wasi` interprets rights bitfields as
unsigned u64 (per the ABI) and no longer reports success for a
`path_open` that threw internally; the inspector/debugger endpoint
changes above.
- **HTTP/2 & WebSocket client** —
`src/runtime/api/bun/h2/connection.rs`, `h2_frame_parser.rs`,
`src/http_jsc/*`: everything in the highlights, plus: 1xx interim
responses are not misclassified as trailers; refused streams still
HPACK-decode the discarded block (RFC 9113 §4.3) and advance
`last_stream_id` so pipelined RST_STREAMs don't become connection
errors; the frame parser no longer holds an exclusive stream reference
across calls back into JS (`options`/header getters, `toString`
coercions) — engine-side stream eviction is deferred until the dispatch
unwinds; the trailers failure path still ends the stream with
FRAME_SIZE_ERROR + graceful GOAWAY so in-flight streams stay retryable;
the deflate plumbing gains a per-VM slot (no behavior change yet).
- **install / pack / bunx / upgrade / create** — `src/install/*`,
`src/runtime/cli/{pack,create,upgrade}_command.rs`, `src/semver`,
`packages/bun-release`, `packages/bun-vscode`: everything in the
highlights, plus: SSRI option suffixes (`?...`) are stripped from digest
payloads; a GitHub dependency whose resolved ref would not form a single
well-formed folder name is refused with a clear error (real refs/SHAs
always pass); the trusted-dependency lookup moved off the extraction
worker thread (no user-visible change); `bun create`'s `package.json`
rewrite uses the CLI arena so the parsed AST outlives its uses; the npm
installer package validates archive entry paths stay inside the
destination; the VS Code lockfile preview escapes interpolated text and
the debug adapter's session id comes from `crypto.randomBytes`.
- **resolver / bundler / parsers / macros / sourcemap / markdown** —
`src/resolver`, `src/bundler`, `src/parsers/{json,json5,yaml}.rs`,
`src/ast/e.rs`, `src/js_parser/lexer.rs`, `src/js_parser_jsc/Macro.rs`,
`src/jsc/RuntimeTranspilerStore.rs`, `src/jsc/bindings/BunPlugin.cpp`,
`src/sourcemap`, `src/md`, `src/paths`, `src/standalone_graph`: the
`__proto__`, resolver-limit, and markdown items above, plus: a
native-plugin `onLoad` source buffer now has exactly one owner (its free
callback was registered twice); `onResolve` callback lists are
snapshotted (GC-visible) before user callbacks run, so a callback
registering more plugins can't perturb the in-progress dispatch;
barrel-import scheduling copies its alias seeds instead of holding
references into a map the BFS mutates; embedded bytecode caches are
handed to `ResolvedSource` as a genuinely owned allocation; the lazy
sourcemap decompression cache became `OnceLock`-based (shared-reference
safe); the lexer's SIMD long-string fast path advances past scanned
bytes (removes a quadratic re-scan on unterminated literals);
`is_parent_or_equal` uses a true prefix check instead of substring
containment.
- **shell / glob / CLI / wrapAnsi** — `src/shell_parser`,
`src/runtime/shell`, `src/glob/GlobWalker.rs`,
`completions/bun.{bash,zsh}`, `src/jsc/bindings/wrapAnsi.cpp`: the
highlights above; the glob walker's followed-link tracking is scoped to
the live ancestor chain (DAG revisits still enumerate); `wrapAnsi` also
caches row widths so the seam fix comes with fewer full-row rescans.
- **crypto** —
`src/jsc/bindings/{ncrypto.cpp,node/crypto/*,webcrypto/*}`: the
highlights above, plus: the sign/verify job copies signature bytes out
of the JS view before the async job runs; `ECDH.convertKey` and
`prepareAsymmetricKey` capture buffer spans only after argument
coercions that can run user JS; deserialized `CryptoKey`s re-validate
that the algorithm matches the key class (and an empty key payload is
rejected); an OOM while encoding returns after throwing.
- **sql / valkey / s3** — `src/sql`, `src/sql_jsc`,
`src/js/internal/sql`, `src/runtime/valkey_jsc`, `src/valkey`,
`src/s3_signing`: the highlights above, plus: postgres `CopyData`
payload length is computed per the wire protocol (was one byte short)
and `PortalSuspended`/`Copy*` messages are consumed instead of
desynchronizing the stream; MySQL zero-length
`AuthSwitchRequest`/`LocalInfileRequest` packets are clean protocol
errors instead of a length underflow; prepared-statement caches are
keyed by the full statement name, not a 64-bit hash (a hash hit is
verified by equality); the distributed-transaction name is type-checked;
the S3 region used to synthesize a host must be host-safe and endpoint
parsing is index-safe on odd endpoint strings.
- **JSC bindings / N-API / V8 / sqlite / misc** —
`src/jsc/bindings/{napi.cpp,v8/*,ZigException.cpp,ZigGlobalObject.cpp,CookieMap.cpp,sqlite/JSSQLStatement.cpp}`,
`src/jsc/rare_data.rs`, `src/runtime/webview`: the N-API/V8 items above;
stack-trace population skips out-of-range frame indices instead of
asserting; the native microtask trampoline is hidden from stack traces;
`bun:sqlite` detects a database closed re-entrantly from inside a bind
coercion and throws "Database has closed"; the macOS webview bridge
type-checks the objects a page posts to its internal message handler.
- **dev server / bake / build & CI** — `src/runtime/bake/*`,
`dockerhub/*`, `.github/workflows/update-vendor.yml`: the dev-endpoint
gating and HMR topic namespacing above; the dev-server terminal error
report blanks non-UTF-8 bytes (not just encoded C1); the React SSR
flight-data inliner escapes the fully decoded string instead of
per-chunk (a boundary-split escape character could previously produce a
malformed inline script); Docker images use `gpg --verify`; the
vendor-update workflow passes matrix values through `env:`.
## Deviations / decisions
- **Integrity: strongest-single-digest verification.** When an
`integrity` field carries several digests, Bun verifies the strongest
supported algorithm present; when several digests of that same algorithm
are present, the first is the one verified. npm/ssri accepts a match on
*any* digest of the chosen algorithm — keeping the full set needs
plumbing through the manifest cache/lockfile types and is left for a
follow-up.
- **No response-decompression size cap in `fetch`.** A per-response
decompressed-body limit was implemented and then deliberately removed
from this PR ("Keep fetch response decompression unbounded"): Node
imposes none, and any cap is a behavior change for legitimate large
responses. The net diff has no decompression change.
- **markdown reference expansion degrades instead of erroring**,
matching md4c exactly (see highlights). No error is ever surfaced.
- **Record conversion keeps the specification's per-property order** —
`[[GetOwnProperty]]`/`Get` interleaved with value conversion, so a
`toString` that mutates a sibling property is observed and a deleted one
skipped, the same as released Bun, Node, and WebKit. The property table
is never held across user code.
- Dead code removed because these changes made it unreachable:
`cache::Entry.external_free_function` (plus `Entry::new` and the free
branch of `Entry::deinit`) and `AlreadyBundled::bytecode_slice`; the
`bun_wyhash` dependency of `sql_jsc` is dropped.
- Deeper, behavior-visible sibling work in the same areas was split into
its own PRs so it can be reviewed on its own terms: #33054 (`node:https`
TLS option set), #33055 (websocket dispatch re-entrancy), #33056
(FileSystemRouter/resolver entry locking), #33060 (`child_process`
uid/gid), #33061 (`node:http` server headers/request timeouts). Nothing
from those PRs is claimed here.
- The changes to CI workflow files, shell completion scripts,
Dockerfiles, and the VS Code extension have no automated-test harness; a
few lifetime/ordering corrections have no deterministic observation from
JS (they are covered by the existing suites and the sanitizer jobs) and
are noted as such instead of shipping a non-asserting test.
## How it was verified
- `bun bd` (full debug build) and `cargo check` clean with zero
warnings; `bun run rust:check-all` passes 10/10 targets (the change set
includes Windows- and macOS-gated code); clippy lints raised on the
touched files were addressed.
- ~190 new regression tests in existing test files (171 `test`/`it`
blocks across 79 files, several parameterized). Each was verified to
fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test
<file>` (except the handful noted above with no JS-observable
assertion); the touched suites were run in full to confirm no
regressions.
- HTTP/2 changes were exercised against a dedicated h2 conformance suite
(`test/js/node/http2/h2-conformance.test.ts`) and Node's own http2
tests; the `node:http` Host behavior was checked against Node's
conformance test for accepted host values; the record-conversion
ordering was checked against WebKit/Node observable order.
- rustfmt / clang-format / prettier / oxlint clean over the changed
files.
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This was referenced Aug 12, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
node:child_processvalidates theuid/gidoptions but never forwarded them to the underlying spawn, so the child always ran with the parent's user/group ids. This implements them end-to-end forBun.spawn,Bun.spawnSync, andnode:child_process(spawn,spawnSync, and theexec*/forkwrappers that build on them), following libuv's behavior so it matches Node:chdirand the fd actions and immediately beforeexec: best-effortsetgroups(0, NULL), thensetgid, thensetuid. An id-set failure surfaces through the existing child-errno channel, socp.spawnthrowsEPERMsynchronously andcp.spawnSyncreturns it onresult.error— the same dispatch as Node (EPERM is not in Node's deferred-to-'error'-event list).vforkchild sharing the parent's address space, so the id changes use rawsyscall(SYS_setgroups/SYS_setgid/SYS_setuid, ...)rather than the glibc wrappers, which run the NPTLSIGSETXIDbroadcast over the (shared) parent thread list. glibc's ownposix_spawndoes the same forPOSIX_SPAWN_RESETIDS.posix_spawn_bunpath (already used for PTY spawns), because Darwin's systemposix_spawncannot change ids — the same fallback libuv makes.uv_process_options_t.uid/.gid+UV_PROCESS_SETUID/SETGIDare passed touv_spawn, which returnsUV_ENOTSUP— exactly what Node reports on Windows.Also: errors that
cp.spawnthrows synchronously (i.e. not in the deferred list) now getsyscall: "spawn", matching Node.packages/bun-types/bun.d.tsdocuments the newuid/gidoptions onBun.spawn.Behavior verified against real Node (v26.3.0, Linux)
Outputs compared side by side with
node:{uid: 65534, gid: 65534}→ child reportsuid=65534 gid=65534 groups=65534(supplementary group 0 dropped);{gid: 65534}alone →uid=0 gid=65534;id -Gprints only65534.spawn/spawnSync/execFileall match.{uid: 0}→spawnSync(...).erroris{code: "EPERM", errno: -1, syscall: "spawnSync id", spawnargs: []}andspawn()throws synchronously with{code: "EPERM", errno: -1, syscall: "spawn"}— identical fields to Node.test-child-process-spawnsync-validation-errors.jspasses both as root and as a non-root user.One intentional deviation: the EPERM error's
messagestring keeps Bun's existing native wording ("EPERM: operation not permitted, posix_spawn 'id'") instead of Node's"spawn EPERM", consistent with how Bun already reportsENOENTfrom spawn.code/errno/syscall/throw-vs-event dispatch all match Node; the new tests assert those, not the message text.Tests
Added to the existing
test/js/node/child_process/child_process.test.ts,test/js/bun/spawn/spawn.test.ts, andtest/js/bun/spawn/spawnSync.test.ts:test.skipIf(!isRoot)) success-path tests: uid+gid applied, supplementary groups dropped, gid-only leaves uid unchanged, asyncspawnvariant.spawn,result.errorfromspawnSync,Bun.spawn/Bun.spawnSync).ENOTSUP.uid: 1.5rejected) that runs everywhere regardless of privileges.All of the new tests (except the deliberate negative-contract one) fail with
USE_SYSTEM_BUN=1and pass withbun bd test. Full runs ofchild_process.test.ts,spawn.test.ts,spawnSync.test.ts, the othertest/js/node/child_process/*suites, and a sweep oftest/js/node/test/parallel/test-child-process-*show no new failures.