install: honor process umask when creating directories#29726
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
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:
WalkthroughAdds POSIX umask-aware directory creation and path-opening: exports Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.zig`:
- Around line 2400-2406: The POSIX fallback should call the
dangling-symlink-safe path creator instead of MakePath.makePath to avoid
infinite bouncing on intermediate dangling symlinks; replace the call to
MakePath.makePath(...) with the safe implementation (use self.makePath(sub_path)
or bun.makePath with the same parameters) so the openDir retry uses the
lstat/delete handling already implemented there, preserving the try/return error
flow; if you prefer to keep MakePath.makePath, port the lstat+delete logic from
bun.makePath into MakePath.makePath before invoking it.
In `@src/install/PackageManager/PackageManagerDirectories.zig`:
- Line 127: Replace all uses of std.fs.cwd() in this file with
FD.cwd().stdDir(); specifically update calls inside
bun.MakePath.makeOpenPath(...) (seen at the occurrences including the one on the
shown diff line) to call FD.cwd().stdDir(), replace assignments like var
cache_dir = std.fs.cwd(); to var cache_dir = FD.cwd().stdDir(); and update calls
such as std.fs.cwd().createFileZ(...) to FD.cwd().stdDir().createFileZ(...).
Ensure you change every instance (the seven reported occurrences) and keep
existing arguments and error handling intact.
In `@src/install/PackageManager/PackageManagerOptions.zig`:
- Around line 175-195: In openGlobalDir (and similarly in openGlobalBinDir)
replace all calls that pass std.fs.cwd() into bun.MakePath.makeOpenPath with
bun.FD.cwd().stdDir() so the directory handle uses Bun's filesystem abstraction;
update every occurrence of std.fs.cwd() used as the first argument to
bun.MakePath.makeOpenPath in those functions to bun.FD.cwd().stdDir().
In `@test/cli/install/bun-install.test.ts`:
- Line 8808: Remove the disallowed assertion
expect(errText).not.toContain("panic:") from the test; simply delete that
expect(...) line (the assertion referencing errText and "panic:") so the test no
longer checks for absence of "panic" in output, leaving the remaining assertions
intact.
🪄 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: 7d0efb8b-8c1b-4bf2-8745-70031dffd51e
📒 Files selected for processing (10)
src/bun.zigsrc/install/PackageInstall.zigsrc/install/PackageInstaller.zigsrc/install/PackageManager.zigsrc/install/PackageManager/PackageManagerDirectories.zigsrc/install/PackageManager/PackageManagerOptions.zigsrc/install/bin.zigsrc/install/hoisted_install.zigsrc/install/isolated_install.zigtest/cli/install/bun-install.test.ts
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
3 similar comments
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/cli/install/bun-install.test.ts (1)
8823-8823:⚠️ Potential issue | 🟠 MajorRemove the disallowed no-
panicassertionLine 8823 uses a forbidden pattern (
expect(errText).not.toContain("panic:")) that should not be used in this test suite.Suggested fix
- expect(errText).not.toContain("panic:"); expect(errText).not.toContain("error:"); expect(exitCode).toBe(0);As per coding guidelines
test/**/*.test.ts: “CRITICAL: Never write tests that check for no 'panic' or 'uncaught exception' or similar in the test output—these tests will never fail in CI”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli/install/bun-install.test.ts` at line 8823, Remove the forbidden assertion expect(errText).not.toContain("panic:") from the test (the disallowed pattern appears as expect(errText).not.toContain("panic:")); instead either drop the check entirely or replace it with a positive, deterministic assertion that validates the expected error output (e.g., assert the specific expected error text or status) so the test no longer relies on a "no panic" negative check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/bin.zig`:
- Around line 549-556: The ensureUmask() function currently has a
publication-order race: replace the unguarded if (!has_set_umask) check with a
proper once-initialization (e.g., atomic compare-and-exchange on has_set_umask
or a mutex) so that only one caller runs the bun.sys.umask(0)/restore sequence
and other threads block until umask is fully captured and stored; ensure you set
the umask variable (umask) and only then mark/commit has_set_umask so no
concurrent caller can observe a partially-initialized value, and protect the
process-global bun.sys.umask calls with the same serialization to avoid
concurrent umask races.
In `@test/cli/install/bun-install.test.ts`:
- Around line 8789-8840: The test author used a manual for loop to parameterize
the linker cases instead of the prescribed pattern; replace the `for (const
linker of ["hoisted", "isolated"] as const) { ...
it.skipIf(isWindows)(\`${linker} install respects process umask...\`, async ()
=> { ... }) }` construct with a describe.each-driven matrix (e.g.,
`describe.each([["hoisted"], ["isolated"]])` or similar) so each linker becomes
a named sub-suite and the inner `it.skipIf(isWindows)(...)` remains the same;
update the test title to use the parameter placeholder from describe.each and
retain all assertions and helper calls (pkgDir creation, Bun.spawn, statMode
checks) unchanged inside the test body.
---
Duplicate comments:
In `@test/cli/install/bun-install.test.ts`:
- Line 8823: Remove the forbidden assertion
expect(errText).not.toContain("panic:") from the test (the disallowed pattern
appears as expect(errText).not.toContain("panic:")); instead either drop the
check entirely or replace it with a positive, deterministic assertion that
validates the expected error output (e.g., assert the specific expected error
text or status) so the test no longer relies on a "no panic" negative check.
🪄 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: b8bbd9f0-b912-49b2-b7ab-c3e281258b3d
📒 Files selected for processing (2)
src/install/bin.zigtest/cli/install/bun-install.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/cli/install/bun-install.test.ts (1)
8789-8837: 🧹 Nitpick | 🔵 TrivialUse
describe.each()instead of a manualforloop for linker parameterization.At Line 8789, this parameterized test should follow the suite rule and use
describe.each()rather than iterating withfor (...).As per coding guidelines
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: “Use describe.each() for parameterized tests...”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli/install/bun-install.test.ts` around lines 8789 - 8837, Replace the manual for-loop parameterization over linker with a Jest-style describe.each table: move the current for (const linker of ["hoisted","isolated"] as const) { ... } block into a describe.each([["hoisted"],["isolated"]])("linker: %s", (linker) => { ... }) wrapper and keep the inner it.skipIf(isWindows)(`${linker} install respects process umask...`, async () => { ... }); ensure you reference the same symbols (it.skipIf, tempDirWithFiles, bunExe, statMode, Bin.Linker.createSymlink behavior) and pass the linker variable from describe.each into the test body so the test logic and expectations remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/cli/install/bun-install.test.ts`:
- Around line 8789-8837: Replace the manual for-loop parameterization over
linker with a Jest-style describe.each table: move the current for (const linker
of ["hoisted","isolated"] as const) { ... } block into a
describe.each([["hoisted"],["isolated"]])("linker: %s", (linker) => { ... })
wrapper and keep the inner it.skipIf(isWindows)(`${linker} install respects
process umask...`, async () => { ... }); ensure you reference the same symbols
(it.skipIf, tempDirWithFiles, bunExe, statMode, Bin.Linker.createSymlink
behavior) and pass the linker variable from describe.each into the test body so
the test logic and expectations remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 356af6e3-d0f6-4342-8123-2172dc60402c
📒 Files selected for processing (2)
src/bun.zigtest/cli/install/bun-install.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.zig (1)
2429-2452:⚠️ Potential issue | 🟠 Major
MakePath.makePath()still keeps the old dangling-symlink hang.Line 2442 still treats
PathAlreadyExistsas success without re-statting the component, so a direct POSIX caller can still bounce forever between a dangling symlink parent and its child.makeOpenPath()is fixed now, but this sibling API should delegate tomakePathSafe()(or share the file-levellstatat/delete logic) so both entry points behave the same.Possible fix
pub fn makePath(comptime T: type, self: std.fs.Dir, sub_path: []const T) !void { if (Environment.isWindows) { var dir = try makeOpenPath(self, sub_path, .{}); dir.close(); return; } - var it = try componentIterator(T, sub_path); - var component = it.last() orelse return; - while (true) { - std.posix.mkdirat(self.fd, component.path, bun.umask_mkdir_mode) catch |err| switch (err) { - error.PathAlreadyExists => { - // TODO stat the file and return an error if it's not a directory - // this is important because otherwise a dangling symlink - // could cause an infinite loop - }, - error.FileNotFound => |e| { - component = it.previous() orelse return e; - continue; - }, - else => |e| return e, - }; - component = it.next() orelse return; + comptime { + if (T != u8) `@compileError`("POSIX makePath expects UTF-8 paths"); } + return makePathSafe(self, sub_path); }As per coding guidelines, "Use
bun.makePath()for recursively creating directories in Zig code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.zig` around lines 2429 - 2452, The makePath function currently treats error.PathAlreadyExists as success and can hang on dangling-symlinks; update makePath to reuse the safe logic used by makeOpenPath by delegating to makePathSafe (or copy the same lstatat/delete-and-retry sequence): when std.posix.mkdirat returns PathAlreadyExists, perform an lstatat on the component path (using the same relative-at logic as makeOpenPath/makePathSafe), verify it is a directory and continue, or if it is a dangling symlink remove it and retry (or return the appropriate error) instead of silently accepting PathAlreadyExists; reference the functions makePath, makeOpenPath, makePathSafe and the lstatat logic to locate where to change the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.zig`:
- Around line 2241-2243: makeDirUmask currently calls std.posix.mkdirat directly
and bypasses Bun's bun.sys wrappers; change it to call bun.sys.mkdirat and
convert the std.fs.Dir to the bun.sys form using .fromStdDir(dir) so the
function uses Bun's tagged-error syscall plumbing consistently (preserve the
Windows path returning dir.makeDir(sub_path) and otherwise call
bun.sys.mkdirat(.fromStdDir(dir), sub_path, umask_mkdir_mode) and propagate its
tagged error).
---
Outside diff comments:
In `@src/bun.zig`:
- Around line 2429-2452: The makePath function currently treats
error.PathAlreadyExists as success and can hang on dangling-symlinks; update
makePath to reuse the safe logic used by makeOpenPath by delegating to
makePathSafe (or copy the same lstatat/delete-and-retry sequence): when
std.posix.mkdirat returns PathAlreadyExists, perform an lstatat on the component
path (using the same relative-at logic as makeOpenPath/makePathSafe), verify it
is a directory and continue, or if it is a dangling symlink remove it and retry
(or return the appropriate error) instead of silently accepting
PathAlreadyExists; reference the functions makePath, makeOpenPath, makePathSafe
and the lstatat logic to locate where to change the 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: 9a4f15a4-dede-42e8-88ff-5da40a06d9aa
📒 Files selected for processing (1)
src/bun.zig
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/bin.zig`:
- Around line 813-823: Add a defensive debug assertion before calling
bun.sys.chmod in Bin.Linker.link() to ensure the cached umask was primed by
ensureUmask() and we won't accidentally use an uninitialized value (which would
produce a fallback of 0o777). Implement this by checking whatever sentinel your
umask uses (e.g., an umask_initialized boolean or a sentinel value for umask)
immediately before the chmod line (_ = bun.sys.chmod(abs_target, 0o777 &
~umask)); the assert should only run in debug builds and must reference the
existing symbols ensureUmask(), umask (or umask_initialized), Bin.Linker.link(),
abs_target, and bun.sys.chmod so the check prevents accidental use of an
unprimed umask.
🪄 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: a8b21ec2-1a0c-41bc-aeeb-902ce6470640
📒 Files selected for processing (2)
src/install/bin.zigsrc/install/isolated_install.zig
claude[bot] noted that 'this PR fixed' in the link_command comment and the '#29726' reference in bun.zig's makePath both lose meaning after merge. Reword them to describe the invariant directly instead.
claude[bot] noted that 'this PR fixed' in the link_command comment and the '#29726' reference in bun.zig's makePath both lose meaning after merge. Reword them to describe the invariant directly instead.
Jarred-Sumner
force-pushed
the
farm/63375d77/umask-honor-install
branch
from
62080f7 to
4cccd65
Compare
There was a problem hiding this comment.
All prior review feedback has been addressed and CI is green; deferring to a human since this changes permission semantics and process-wide umask handling across the install path (including the shared bun.makePath helper), which is broad enough to warrant a maintainer sign-off.
Extended reasoning...
Overview
This PR makes bun install honor the process umask when creating directories and bin targets, fixing #29723. It touches 12 files: the core bun.makePath / MakePath.makeOpenPath helpers in src/bun.zig, the hoisted and isolated installers, Bin.Linker (umask capture + bin chmod formula), bun link/bun unlink, and several PackageManager directory-creation sites, plus a new POSIX-only regression test parameterized over both linkers.
Security risks
The change is permission-related. Directories move from a hardcoded 0o755 to 0o777 & ~umask (kernel-applied), and bin targets move from an unconditional 0o777 to 0o777 & ~umask. Under the default umask 0o022 directory perms are unchanged (still 0o755) and bin perms get tighter (0o755 vs the old 0o777), so the default posture improves. With umask 0 directories become world-writable, but that matches mkdir(1)/Node/npm/pnpm and is the user's explicit choice. ensureUmask() no longer zeroes the process-wide umask; the PR asserts the affected code already fchmods where exact modes matter, which looks correct from inspection but is the kind of assumption a maintainer should confirm.
Level of scrutiny
This is not a mechanical change. It alters process-global state handling (umask read-and-restore, now primed on the main thread for both linkers with a debug assert guarding the invariant), rewrites the POSIX branch of MakePath.makeOpenPath to route through bun.makePath (which now deleteTrees non-directory path components and was switched from lstat to lstatat), and changes the bin chmod formula. The bun.makePath/lstatat change affects callers beyond this PR's install scope. Given the blast radius (every POSIX bun install) and the security-adjacent subject matter, this warrants a human maintainer's review.
Other factors
I reviewed this PR across seven prior rounds and every issue raised (isolated-mode umask priming, thread-pool race on ensureUmask, lstat vs lstatat dirfd mismatch, CI BUN_INSTALL_CACHE_DIR override, missed link/unlink sites, dead PathAlreadyExists check, comment hygiene) was addressed; all threads are resolved and Build #47990 is green. The bug-hunting system found nothing on the current revision. I'm deferring rather than approving solely on scope/criticality grounds, not because of any outstanding concern with the implementation.
There was a problem hiding this comment.
All prior bot feedback is addressed and I found nothing new, but this changes permission semantics across the whole install path (0o755 → umask-derived, ensureUmask no longer zeroing the process umask) and reroutes MakePath.makeOpenPath on POSIX through bun.makePath — which will now lstatat+deleteTree non-directory path components for callers that previously just used std's makeOpenPath. Worth a human pass on those two behavior changes specifically.
Extended reasoning...
Overview
Twelve files across the install subsystem: src/bun.zig (new umask_mkdir_mode, makeDirUmask, makePath switched from lstat→lstatat, MakePath.makeOpenPath POSIX rewrite), bin.zig (ensureUmask now read-and-restore instead of zeroing; bin chmod umask | 0o777 → 0o777 & ~umask; debug assert), hoisted_install.zig / isolated_install.zig / PackageInstall.zig / PackageInstaller.zig / PackageManager*.zig (mkdir mode constants and makeOpenPath call-site conversions), link_command.zig / unlink_command.zig (global node_modules creation), plus a parametrized regression test.
Security risks
File permissions are security-adjacent. The change is in the loosening direction only when the user's umask is looser than 0o022 (e.g. 0o002 → group-writable trees), which matches npm/pnpm/Node and is the explicit goal. The earlier risk — ensureUmask() racing on worker threads and zeroing the process umask, yielding 0o777 dirs — was caught and fixed by priming on the main thread in both hoisted and isolated entry points, with a debug assert in createSymlink. I don't see a remaining path to unintended world-writable output, but the correctness depends on every Bin.Linker.link() caller having primed first.
Level of scrutiny
Medium-high. Two non-mechanical behavior changes deserve human eyes:
ensureUmask()no longer zeros the process umask. The PR asserts that everything which needed exact modes alreadyfchmods afterward (tryNormalizeShebang, the bin chmod). That's true for the call sites I traced, but it's a process-wide assumption.MakePath.makeOpenPathon POSIX now routesFileNotFoundthroughbun.makePath, which onPathAlreadyExistswilllstatatthe component anddeleteTreeit if it isn't a directory. Pre-PR, the converted callers (cache dir, temp dir, global dirs, package destination dirs) used std'smakeOpenPath, which would just error on a dangling symlink. Now they'll silently rm-rf and recreate. That's almost certainly fine for these specific paths, but it's a real semantic change applied to ~15 call sites.
Other factors
The PR went through seven rounds of bot review with substantive bugs found and fixed each time (isolated-mode umask not primed → thread race on ensureUmask → cwd-relative lstat → CI env-var override in the test → missed bun link/unlink sites → dead PathAlreadyExists check → comment hygiene). All threads are resolved and the current diff looks correct to me, but the iteration history itself suggests this is subtle enough to merit a human sign-off rather than bot-only approval.
|
Latest CI failure on
My PR only changes install/bin code paths — zero references to Nothing to retrigger on my end — CI queue pressure caused 18 of the 23 "failures" to simply expire without running, and the 3 that did run failed on pre-existing flakes. Ready for human merge sign-off. |
claude[bot] noted that 'this PR fixed' in the link_command comment and the '#29726' reference in bun.zig's makePath both lose meaning after merge. Reword them to describe the invariant directly instead.
robobun
force-pushed
the
farm/63375d77/umask-honor-install
branch
from
6021382 to
7019185
Compare
|
Rebased onto main (no merge conflicts — the big src/ restructure handled file renames automatically). The Zig→Rust install port has since landed, so the binary now runs Rust. Reapplied the umask fix in the Rust port:
The Zig fixes from the original commits are preserved but now unreachable — the .zig files still compile but install paths route through Rust. Verified manually:
Regression test in |
robobun
force-pushed
the
farm/63375d77/umask-honor-install
branch
from
0130089 to
45d6364
Compare
|
CI on afe8270: 4 failures, all on lanes unrelated to this PR's diff (install/ + sys/):
My diff touches only Re-roll budget exhausted. Not pushing another |
claude[bot] noted that 'this PR fixed' in the link_command comment and the '#29726' reference in bun.zig's makePath both lose meaning after merge. Reword them to describe the invariant directly instead.
robobun
force-pushed
the
farm/63375d77/umask-honor-install
branch
from
afe8270 to
959014a
Compare
|
CI status on 255a035 (latest): the remaining What this PR did fix along the way: under a permissive umask (0o002) bunx used to create its own cache root group-writable and then refuse it on the next run via
Re-roll budget spent; not pushing a retrigger. Diff is green on everything its code can affect — needs a maintainer to merge (and #31928 or equivalent to clear the bunx user-agent red that hits all PRs). |
bun install hard-coded 0o755 when creating cache/node_modules directories, and zeroed the process umask before installing packages. Under umask 0o002 users expected 0o775 (group-writable) directories like they get from npm and pnpm, but bun always produced 0o755 — blocking shared multi-user repos. Pass 0o777 to mkdirat instead and let the kernel subtract the caller's umask. With the default 0o022 umask the final mode is still 0o755, so behavior is unchanged for typical users. With umask 0o002 it becomes 0o775, matching Node, npm, pnpm, and /bin/mkdir. Also stop zeroing the process umask in Bin.Linker.ensureUmask. The places that relied on umask=0 already fchmod back to an explicit mode afterwards, so removing the umask(0) is safe, and keeping the user's umask means mkdir/openat see it. Fixes #29723
Bin.Linker.ensureUmask() was only called from hoisted_install.zig, link_command.zig, and unlink_command.zig. The isolated installer went straight to Bin.Linker.link() without priming, so Bin.Linker.umask stayed at its default 0 and the new chmod(target, 0o777 & ~umask) degenerated to 0o777 — bin targets ignored umask under --linker=isolated. Call ensureUmask() from createSymlink itself. The guard inside ensureUmask() makes redundant calls a no-op. Test expanded to cover both linker modes.
CodeRabbit correctly flagged that MakePath.makePath still has the old PathAlreadyExists TODO — a dangling intermediate symlink can make it loop forever between parent and child components. Route the POSIX makeOpenPath fallback through the file-level bun.makePath instead, which lstats and rm-rf's the dangling entry. Also drop the disallowed 'panic:' check in the test per test guidelines.
claude[bot] flagged that `bun.makePath`'s `PathAlreadyExists` branch calls `sys.lstat(component.path)`, which resolves against the process CWD — but the surrounding `mkdirat` and `deleteTree` both operate relative to the `dir` fd. Pre-fix to this PR the only caller was cwd so it happened to work; after e40726b rerouted `MakePath.makeOpenPath` through here, non-cwd callers (node_modules fds from PackageInstall, cache dir fds from PackageManagerDirectories, etc.) now risk statting the wrong location. Swap `sys.lstat` for `sys.lstatat(.fromStdDir(dir), path)` so the stat lines up with the mkdirat/deleteTree it's classifying.
claude[bot] flagged that calling ensureUmask() from createSymlink runs it on thread-pool workers under --linker=isolated. ensureUmask() is not thread-safe — plain bool + two non-atomic umask(2) syscalls — so concurrent workers can: - both see has_set_umask==false before either writes true - one calls umask(0) → reads previous 0o022, then umask(0o022) → restores - other calls umask(0) → reads 0 (just set by first), then umask(0) → process umask permanently 0 - subsequent mkdirat(..., 0o777) produces 0o777 dirs, defeating this PR Also, even without that worst-case interleaving, there's a window where umask is 0 while other workers mkdirat concurrently — those dirs come out 0o777. Prime ensureUmask() from installIsolatedPackages (main thread) the same way hoisted_install.zig:91 does, and drop the lazy call inside createSymlink.
Previous build hit HTTP 502 downloading the prebuilt WebKit tarball. No code change.
… self-import claude[bot] caught two issues on af45fc1/caf49d9: 1. The umask test set the cache dir via bunfig (`cache = "./.umask-cache"`), but CI's runner.node.mjs:1171 sets BUN_INSTALL_CACHE_DIR, and the env var wins over bunfig in fetchCacheDirectoryPath. So in CI the bunfig setting was ignored, the expected cache dir never got created, and stat(cacheDir) would throw ENOENT. Pass the cacheDir explicitly through env so bunfig and env agree. 2. The `makePathSafe = @import("bun").makePath` alias in MakePath was redundant — `const bun = @this();` at the top of bun.zig is already in scope here (other code inside the same struct uses bun.* already). Only the bare `makePath` identifier is shadowed; `bun.makePath` is a qualified lookup that resolves to the file-level function. Drop the alias and call bun.makePath directly.
coderabbit flagged this as a defensive nit. Add a debug-only bun.assertWithLocation(has_set_umask, @src()) guard before the chmod so a future caller that forgets to call ensureUmask() on the main thread trips immediately instead of silently producing 0o777 bins (what the pre-PR code always did, and what a miss would regress back to). No behavior change in release builds.
claude[bot] caught two parallel call sites this PR missed: link_command.zig:69 and unlink_command.zig:82 both created $BUN_INSTALL/install/global/node_modules via std's Dir.makeOpenPath (hardcoded 0o755) instead of going through the converted globalLinkDir() in PackageManagerDirectories.zig:389. link_command.zig also creates the `@scope` subdir via std's Dir.makeDir. Route them through bun.MakePath.makeOpenPath / bun.makePath so they honor umask like the rest of the install-path dirs fixed in this PR. Verified with `umask 002 && bun link` on both unscoped and @scope packages — 0o775 everywhere. Default `umask 022` still gives 0o755.
claude[bot] caught this: when 39810b6 swapped node_modules.makeDir() for bun.makePath(), the surrounding 'if (err == error.PathAlreadyExists) break :brk;' became unreachable. bun.makePath handles EEXIST internally (lstatat + deleteTree-or-continue, src/bun.zig:2209-2227) and never propagates error.PathAlreadyExists to callers. No behavior change — verified scoped link still works on both fresh global dir and one where @scope already exists.
claude[bot] noted that 'this PR fixed' in the link_command comment and the '#29726' reference in bun.zig's makePath both lose meaning after merge. Reword them to describe the invariant directly instead.
Build #51083 expired all 42+ jobs in the queue before they could pick up agents — no actual test or build failure, just queue pressure. Empty commit to schedule a fresh run.
The rebase onto main brought in the Zig→Rust port of install/. The
Zig-side umask fix in this PR was untouched by the rebase, but the
binary now runs the Rust port where:
- src/sys/lib.rs mkdir_recursive_at defaults to mode 0o755, which
stops the kernel's umask application from ever loosening perms
beyond 0o755.
- src/install/{hoisted,isolated}_install.rs and PackageInstall.rs
pass hard-coded 0o755 to sys::mkdir / sys::mkdirat.
- isolated_install.rs never primes Bin::Linker::ensure_umask on the
main thread — same gap claude[bot] flagged on the Zig side.
Port the fixes:
- New bun_sys::UMASK_MKDIR_MODE = 0o777 constant.
- mkdir_recursive_at and Dir::make_dir use it as default.
- hoisted_install.rs, isolated_install.rs, PackageInstall.rs pass it
where they had 0o755.
- install_isolated_packages primes bin::Linker::ensure_umask on the
main thread before scheduling work (ensure_umask is already atomic
via compare_exchange in the Rust port).
Verified: under umask 0o002 hoisted + isolated now produce 0o775 dirs
and 0o775 bin targets; default umask 0o022 still yields 0o755.
Regression test (test/cli/install/bun-install.test.ts) fails without
this commit and passes with it.
claude[bot] caught two nits after the rebase onto the Rust port: - src/sys/lib.rs: the UMASK_MKDIR_MODE doc said "install-owned directories" but the constant is the default for the general-purpose mkdir_recursive_at / Dir::make_dir helpers, which the bundler, DevServer, router, bunx, profilers, etc. all reach via make_path / make_open_path. The Zig sibling in the same PR already says "when creating directories" — match that wording. - src/install/bin.rs: the bun.assertWithLocation(has_set_umask) guard added in commit 1a9d44b only landed in bin.zig. After the rebase that code is unreachable; the live path is chmod_on_ok in bin.rs. Add the equivalent debug_assert!(HAS_SET_UMASK.load(..)) before the sys::chmod so a future caller that skips ensure_umask() trips loudly instead of silently widening bin perms to 0o777.
claude[bot] flagged one more rotation of the same comment-rot fixed in 65fe998: the parametrized-test block still referenced isolated_install.zig, isolated_install/Installer.zig, and hoisted_install.zig. Per src/CLAUDE.md those are uncompiled reference files — the live paths are the .rs siblings. Rewrite to match.
robobun
force-pushed
the
farm/63375d77/umask-honor-install
branch
from
959014a to
0955a41
Compare
There was a problem hiding this comment.
No bugs found in this revision, but the UMASK_MKDIR_MODE change in src/sys/{lib,dir}.rs retargets the general-purpose mkdir_recursive_at / Dir::make_dir helpers used across the whole codebase (bundler, router, bunx, etc.), not just install — combined with the ensureUmask no-longer-zeroing change, that's a permissions-semantics shift broad enough to want a human sign-off.
Extended reasoning...
Overview
This PR fixes #29723 by changing directory-creation mode from hardcoded 0o755 to 0o777 so the kernel applies the process umask. It touches 18 files across both the live Rust install path (src/sys/lib.rs, src/sys/dir.rs, src/install/*.rs) and the now-reference Zig siblings (src/bun.zig, src/install/*.zig, src/runtime/cli/{link,unlink}_command.zig), plus a regression test. The Rust UMASK_MKDIR_MODE constant becomes the default for mkdir_recursive_at and Dir::make_dir, and Bin.Linker.ensureUmask() switches from umask(0) (zero the process umask) to a read-and-restore probe.
Security risks
File permissions are security-adjacent. The change can only loosen directory perms relative to before (when umask < 0o022), which is the explicit user intent and matches mkdir(1)/Node/npm/pnpm. With the default umask 0o022 the result is still 0o755, so most users see no change. The ensureUmask change means the process umask is no longer zeroed for the duration of install — combined with the 0o755→0o777 switch, the net effect at default umask is identical, but the interaction is non-trivial and process-wide. No injection/auth/data-exposure surface.
Level of scrutiny
Medium-high. The core idea is simple, but the blast radius is large: mkdir_recursive_at and Dir::make_dir are general-purpose helpers reached by the bundler, router, DevServer, profilers, standalone graph, bunx, etc. — this is a global behavior change, not an install-scoped one. The MakePath.makeOpenPath POSIX rewrite also routes through bun.makePath's dangling-symlink deleteTree path, which is a behavior change beyond the umask fix. The PR went through ~7 review iterations (thread-pool race on ensureUmask, lstat vs lstatat dirfd mismatch, CI BUN_INSTALL_CACHE_DIR override, Windows unused_imports build break), which is normal for a change of this scope but underscores it isn't mechanical.
Other factors
All prior inline comments (mine and CodeRabbit's) are resolved; the bug-hunting system found nothing in the current revision. A regression test covers both hoisted and isolated linkers. The author has been waiting on maintainer sign-off through several CI-flake cycles. Given the permissions-semantics scope and the global-helper retargeting, I'm deferring rather than approving.
1 participant
Fixes #29723.
Repro (Linux)
Cause
Two things conspired to ignore the caller's umask:
0o755on everymkdirat— e.g.bun.sys.mkdir("node_modules", 0o755),sys.mkdirat(..., node_modules_path, 0o755),bun.sys.mkdirat(..., entry.path, 0o755).std.fs.Dir.makeDir/makeOpenPath(used viaMakePath.makeOpenPathetc.) hard-code the samedefault_mode = 0o755. Kernel umask is subtractive, so0o755 & ~0o002 == 0o755— umask can never loosen perms beyond 0o755.Bin.Linker.ensureUmask()callsbun.sys.umask(0)before the install loop. Even if mkdirat received0o777, the kernel would stop masking, so dirs would come out0o777instead of0o775.Fix
bun.makePath,bun.makePathW,bun.MakePath.makePath, andbun.MakePath.makeOpenPathnow pass0o777tomkdiraton POSIX (newbun.umask_mkdir_modeconstant). Windows paths are untouched.sys.mkdirat(..., 0o755)calls inhoisted_install.zig,isolated_install.zig,PackageInstall.zigswitch tobun.umask_mkdir_mode.std.fs.Dir.makeOpenPath(PackageManagerOptions,PackageManagerDirectories,PackageManager.zig,PackageInstaller.zig,PackageInstall.zig) now callbun.MakePath.makeOpenPath, so they pick up the umask-friendly path.Bin.Linker.ensureUmaskno longer zeros the process umask — it reads and restores. The code that needed an exact mode alreadyfchmods afterwards (tryNormalizeShebang, the bin symlink chmod), so dropping theumask(0)is a no-op for them.umask | 0o777which is always0o777; changed to0o777 & ~umaskso bin files respect umask too (0o755at default,0o775atumask 0o002).Verification
Default behavior (0o022) is unchanged — matches Node/npm/pnpm.
Test added in
test/cli/install/bun-install.test.ts(skips on Windows). Fails on main, passes here.bunx interaction
is_trusted_cache_root(added on main by #31339) refuses a bunx cache root in the shared temp dir that is group/other-writable. With umask-honoring mkdir, a permissive umask like0o002made bunx create its own cache root0o775, so the second invocation refused the cache the first one created (caught by CI on every platform). bunx now requests0o755explicitly for its cache root: the kernel can only subtract bits from it, so the root can never become group/other-writable while stricter umasks still tighten it. Covered by a second regression test intest/cli/install/bunx.test.tsthat runs bunx twice underumask 0002against the mock registry; it fails without the bunx change and passes with it.