test: add a harness isRoot and skip the chmod-based ls and Bun.file cases as root - #39232
test: add a harness isRoot and skip the chmod-based ls and Bun.file cases as root#39232robobun wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 59 seconds Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (17)
Comment |
|
Updated 7:39 PM PT - Aug 15th, 2026
✅ @robobun, your commit 17da809a84569ab4e24080ad96960833a59d0056 passed in 🧪 To try this PR locally: bunx bun-pr 39232That installs a local version of the PR into your bun-39232 --bun |
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only skip guard matching the existing pattern in neighboring shell tests.
What was reviewed:
- Confirmed
const isRoot = process.getuid?.() === 0andtest.if(isPosix && !isRoot)match the identical guards already inbunshell.test.tsandcommands/mv.test.ts. - The two gated tests rely on
chmod 000to provoke EACCES, which root bypasses — skipping under uid 0 is correct and CI (unprivileged) still exercises them. - No runtime code touched; no other
chmod 000-based cases in this file were left unguarded.
Extended reasoning...
Overview
This PR adds a single isRoot constant to test/js/bun/shell/commands/ls.test.ts and changes two test.if(isPosix) guards to test.if(isPosix && !isRoot). The two affected tests create a directory, chmod 000 it, and assert that the ls builtin reports "Permission denied". When run as root, the kernel bypasses DAC mode bits (CAP_DAC_OVERRIDE / CAP_DAC_READ_SEARCH), so the open succeeds and the tests fail spuriously. Test-only change; no runtime code modified.
Security risks
None. This only adds a skip condition to two test cases in the test suite.
Level of scrutiny
Low. This is a mechanical, 4-line test-gating change that copies an established pattern verbatim from sibling files. I verified via grep that bunshell.test.ts:57 and commands/mv.test.ts:23 define the same isRoot constant, and bunshell.test.ts:941 / :1168 use the identical test.if(isPosix && !isRoot) guard for the same reason (chmod-based EACCES fixtures). The repo review guidance explicitly allows skipping with a stated reason, and the added comment ("Root bypasses directory mode bits, so chmod 000 cannot produce EACCES for it.") documents why.
Other factors
- CI agents run unprivileged, so the EACCES error-path coverage is unchanged in CI — this only affects local runs as root.
- The
process.getuid?.()optional-chaining handles Windows (wheregetuidis undefined) correctly, yieldingundefined === 0→false, so!isRootistrueand the existingisPosixgate remains the effective condition there. - Checked that no other tests in this file depend on chmod-based permission denial; only these two do.
- No prior review comments to address; the bug-hunting system found nothing.
robobun
changed the title
|
Reshaped in aee03a8 after self-review: the uid 0 predicate now lives in |
There was a problem hiding this comment.
LGTM — test-only skip guard for chmod-based EACCES fixtures under uid 0, matching the pattern already used in neighbouring shell tests. The inline nit about migrating the pre-existing local isRoot copies is a fair follow-up but not blocking.
Extended reasoning...
Overview
Test-only change touching three files: exports isRoot = process.getuid?.() === 0 from test/harness.ts, gates two chmod 000 permission-denied cases in test/js/bun/shell/commands/ls.test.ts on isPosix && !isRoot, and applies the same guard to the bad permissions throws describe in test/js/web/fetch/fetch.test.ts. No runtime code touched.
Security risks
None. Test infrastructure only; the harness constant reads process.getuid() and is used purely to decide whether to register a test.
Level of scrutiny
Low. This is a mechanical skip guard for a well-understood POSIX property (root bypasses DAC via CAP_DAC_OVERRIDE / CAP_DAC_READ_SEARCH, so chmod 000 cannot provoke EACCES for uid 0). The identical pattern already exists in bunshell.test.ts and mv.test.ts. CI agents run unprivileged, so the EACCES error-path coverage is unchanged where it matters; this only removes false failures for developers running the suite as root locally.
Other factors
- Verified both directions per the description: 27 pass / 2 skip as root, both cases still run and pass as an unprivileged user.
- The one inline nit (migrate the six pre-existing local
const isRootcopies to the new harness export) is a consistency cleanup. REVIEW.md's "don't ride file-wide standardization on a focused bugfix" cuts the other way here, so leaving those for a follow-up is reasonable — the change as-is moves in the right direction by promoting to harness rather than adding a seventh local copy. - The fetch.test.ts hunk correctly extends the existing
skipIf(isWindows)toskipIf(isWindows || isRoot)for the same reason (chmod 0o000 on a file cannot deny read to root).
Root bypasses directory mode bits, so the chmod 000 setup in these two tests never produces EACCES and the builtin lists the directory instead of printing the expected error. Gate them on not running as uid 0, the same way bunshell.test.ts and mv.test.ts gate their chmod-based cases.
…ases on it too Move the uid 0 predicate into test/harness.ts next to the other is* helpers so ls.test.ts imports it instead of defining its own copy, and use it to gate the "bad permissions throws" block in fetch.test.ts, which chmods a file to 000 and fails the same way as root.
Replaces the file-local `const isRoot = process.getuid?.() === 0` copies and the inline getuid() comparisons used to gate tests with the harness export, so the uid 0 check has one spelling. No gating changes: every site keeps the same condition, only spelled through isRoot.
aee03a8 to
17da809
Compare
robobun
changed the title
robobun
changed the title
There was a problem hiding this comment.
LGTM — thanks for the follow-up sweep in 17da809; all the local isRoot copies and inline process.getuid?.() === 0 checks now use the harness export.
What was reviewed:
- Verified every migrated site is semantically identical to the harness definition (including
resolve.test.ts, whose old!isWindows &&guard is subsumed byprocess.getuidbeing undefined on Windows). - Confirmed the remaining
process.getuid() === 0hits are all in vendoredtest/js/node/test/parallel/upstream tests, correctly left untouched. - Checked the new skip gates on
ls.test.tsandfetch.test.tsonly narrow when running as uid 0; unprivileged CI coverage is unchanged.
Extended reasoning...
Overview
Test-only PR that (1) exports isRoot = process.getuid?.() === 0 from test/harness.ts, (2) gates six chmod-000-based test cases in ls.test.ts and fetch.test.ts on !isRoot so they skip instead of falsely failing when run as uid 0, and (3) — in response to my earlier review — sweeps every existing file-local const isRoot and inline process.getuid?.() === 0 check across 15 test files onto the new harness export. No src/ code is touched.
Security risks
None. This only changes which tests run under uid 0, a configuration CI does not use. No production code paths, no auth/crypto/permissions logic.
Level of scrutiny
Low. Every hunk is one of two mechanical shapes: add isRoot to an existing from "harness" import and delete a byte-identical local definition, or replace an inline process.getuid?.() === 0 with isRoot. The harness expression is character-for-character the same as the removed locals, so behaviour is provably identical. The two spots that differed in spelling (resolve.test.ts's !isWindows && prefix, env.test.ts's typeof process.getuid === "function" && prefix) are equivalent because process.getuid is undefined on Windows and the optional-chain returns undefined !== 0.
Other factors
- My prior review's only ask (migrate the local copies) is fully addressed and then some — the sweep covers all 14 non-vendored call sites.
- Grepped for stragglers: the only remaining
process.getuid()-based root checks are intest/js/node/test/parallel/, which are vendored upstream Node tests and should not import Bun's harness. - The
process.test.jshunk that looks large is just prettier reflowing anit.skipIf(...)back onto one line after its condition shortened; the test body is unchanged. - CI runs unprivileged, so the new
skipIf(... || isRoot)gates do not reduce CI coverage.
1 participant
Problem
test/js/bun/shell/commands/ls.test.ts:bunshell ls > errors > permission denied directoryand... permission denied directory recursive(27 pass / 2 fail;Expected to contain: "Permission denied",Received: ""). Bothchmod 000a directory and expect thelsbuiltin to report EACCES (ls.test.ts:306 and :322 on main).test/js/web/fetch/fetch.test.ts: the fourBun.file > bad permissions throwscases (fetch.test.ts:1115 on main), whichchmoda file to 000 and expect every read method to reject withpermission denied(as root the reads succeed; thejsoncase fails withFailed to parse JSONbecause it read the file).chmod 000fixture cannot produce EACCES for it. These tests run in the test process itself (the shell ones throughTestBuilder, which callsBun.$directly), so there is no child process to run as another user; the only correct outcome as root is to skip them.const isRoot = process.getuid?.() === 0, or the same comparison inline in askipIf), whiletest/harness.tsexports every other shared predicate (isWindows,isPosix,isMusl, ...) and has noisRoot. Four open shell PRs (test(shell): make TestBuilder fail on expectations that cannot assert anything #37737, shell: keep OutputTask boxed through the vtable and output queues #37688, shell: treat a lone-as an operand in the builtin option parsers #39214, test(shell): stop the ls/rm/bunshell tests from running bun install against the registry #39231) each list the twolsfailures as known local noise.Fix
test/harness.ts: exportisRoot = process.getuid?.() === 0next toisPosix/isWindows. It is always false on Windows:process.getuidis only installed under#if !OS(WINDOWS)(src/jsc/bindings/BunProcess.cpp:4919), so the optional call yieldsundefined.ls.test.ts: the two cases becometest.if(isPosix && !isRoot).fetch.test.ts:describe.skipIf(isWindows)onbad permissions throwsbecomesdescribe.skipIf(isWindows || isRoot).isRoot(getuid?.() === 0->isRoot,getuid?.() !== 0->!isRoot, env.test.ts'stypeof getuid === "function" && getuid() === 0->isRoot); about half of them are root-only tests (setuid/setgid, cgroups) rather than skips, which is why the harness comment describes both uses. The larger hunks inprocess.test.jsandbun-prune.test.tsare prettier reflowing the shortened conditions and the longer import line.test/js/bun/http/serve.test.ts:2534(binding port 1003 must fail): that depends on CAP_NET_BIND_SERVICE andnet.ipv4.ip_unprivileged_port_start, not on mode bits, soisRootwould be the wrong predicate for it.process.getuid()(cache directory names inbunx.test.ts/cc.test.ts, launchctl paths incron.test.ts, theprocess.getuidAPI tests themselves, a uid printed inside a child_process fixture).unprivilegedSpawnOptions()to the harness for tests that spawn a child and can therefore keep running as root by dropping privileges; that is the right tool for spawn-based tests and does not apply to these in-process ones. It touches a different part of harness.ts and none of the files here, so the two do not conflict. Of the open PRs touching the same test files, test(shell): make TestBuilder fail on expectations that cannot assert anything #37737 (ls.test.ts, bunshell.test.ts), shell: treat a lone-as an operand in the builtin option parsers #39214 (bunshell.test.ts, mv.test.ts) and shell: fail an empty operand with ENOENT instead of acting on the cwd #38002 (mv.test.ts) merge cleanly with this branch (checked with a three-way merge); test(shell): stop the ls/rm/bunshell tests from running bun install against the registry #39231 also rewrites theharnessimport line of ls.test.ts, so whichever of the two lands second needs a one-line import merge.bun bd test test/js/bun/shell/commands/ls.test.ts27 pass / 2 skip (was 27 pass / 2 fail);bun bd test test/js/web/fetch/fetch.test.ts -t "bad permissions throws"4 skip (was 4 fail); a full run of fetch.test.ts on this branch and on main differs only in those four cases going from fail to skip.runuser -u nobody, debug and release builds): bothlscases and all fourfetchcases still run and pass.Background
chmod 000removes every permission bit from a file or directory, and opening or listing it fails with EACCES for an ordinary user. The kernel does not apply these checks to uid 0 (on Linux this is the CAP_DAC_OVERRIDE capability root holds), so for root the same path opens normally. Any test that relies onchmodto provoke EACCES is therefore only meaningful when run as a non-root user; conversely, tests that setuid/setgid a child can only run as root, which is the other wayisRootis used.test/harness.tsis preloaded into every test file and is where the shared platform predicates live;test.if(cond)anddescribe.skipIf(cond)register the affected cases as skipped instead of running them.Per-site results as root (debug build, this branch; identical statuses on main)
canTriggerEACCEStests run via runuser and passglob over an unreadable directory,cd with EACCESskipunreadable directory across devicesskipuid/gid: 2 pass,throws EPERMskipuid/gid: 3 pass,throws EPERMskipuid/gid options: 3 pass, EPERM case skipseteuid under GC pressureruns and passes,initgroups ... unknown string userskipno permission to watchcases skip, the inotify_add_watch case runs and passesprocess.env is preserved when cwd lacks read permissionruns via runuser and passeskill(pid, 0)liveness check gets EPERM for thenobodygrandchild. Unrelated to this change and reported separately.Earlier revisions of this PR
The first push only added a file-local
const isRoottols.test.tsand gated the two shell cases. Self-review pointed out that this was one more copy of a predicate the harness should own and thatfetch.test.tshad the same class of failure, so the second revision added the export and the fetch gate. Review of that revision asked for the pre-existing local copies to move to the export as well, which the third revision does.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.