bunx: store the package cache under the per-user bun cache directory - #31447
bunx: store the package cache under the per-user bun cache directory#31447Jarred-Sumner wants to merge 11 commits into
Conversation
|
Updated 9:05 PM PT - Jun 3rd, 2026
✅ @autofix-ci[bot], your commit 947a889b8521dac3fe8f1be72ab95e1bbf173634 passed in 🧪 To try this PR locally: bunx bun-pr 31447That installs a local version of the PR into your bun-31447 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
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:
Walkthroughbunx now prefers a POSIX per-user install-cache root when resolving its cache directory, falling back to a temp-directory layout; initialization enforces 0700 permissions and ownership checks. cache rm UID handling was centralized and tests were added/updated for both per-user and fallback behaviors. Changesbunx per-user install cache
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/install/bunx.test.ts`:
- Around line 1383-1385: Replace the direct tmpdirSync() usage with the test
harness tempDir helper: import or use tempDir from 'harness', call tempDir() to
create the directory, apply chmodSync to the tempDir instance's path (e.g.,
chmodSync(tempDir.path, 0o500)), and set env.BUN_INSTALL_CACHE_DIR =
join(tempDir.path, "cache"); do the same replacement for the other occurrence
that used tmpdirSync() (the case around the later lines). Ensure you reference
the variables/functions tmpdirSync, tempDir, readOnlyParent, chmodSync, and
env.BUN_INSTALL_CACHE_DIR when making the change so the temp directory lifecycle
is managed by the harness.
🪄 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: 004757d8-7808-47ef-b9c8-0db590142afa
📒 Files selected for processing (3)
src/runtime/cli/bunx_command.rssrc/runtime/cli/package_manager_command.rstest/cli/install/bunx.test.ts
There was a problem hiding this comment.
All earlier feedback has been addressed and nothing new turned up on this revision — looks good to me, but since this relocates the bunx executable cache and touches the directory-ownership/permission trust checks, I'd like a human to give it the final sign-off.
Extended reasoning...
Overview
This PR moves the POSIX bunx package cache from $TMPDIR/bunx-<uid>-<pkg> to <install cache>/.bunx-<uid>/<pkg> (0700, per-user), with a fallback to the legacy temp-dir layout when the install cache can't be resolved or created. It also:
- adds a second
is_trusted_cache_rootcheck after creating/opening the cache dir, - updates
bun pm cache rmto count and delete.bunx-<uid>via the same env-onlyfetch_cache_directory_path(env, None)lookup bunx uses (so bunfiginstall.cache.dirdivergence doesn't leave bunx packages behind), - refreshes four doc comments that previously described the cache as unconditionally living in a world-writable temp dir,
- adds five new tests covering the new location/permissions, the no-env fallback, the unwritable-cache fallback, and the bunfig-divergence
pm cache rmcase.
Files: src/runtime/cli/bunx_command.rs, src/runtime/cli/package_manager_command.rs, test/cli/install/bunx.test.ts.
Security risks
The change is security-adjacent: it alters where bunx caches executable package contents and how the cache root is created/validated (0700 mode, uid ownership, not group/other-writable). The existing per-binary uid checks (is_trusted_cached_binary) and post-install re-verification are kept and still run unconditionally, and the new layout is strictly more private than the old shared-temp-dir layout. I don't see a new attack surface introduced — the temp-dir fallback path is the same shape as before and is still covered by the same hardening — but changes to this trust model are exactly the kind of thing a human should confirm.
Level of scrutiny
Medium-high. bunx is a widely-used entry point that resolves and executes binaries; the cache location and its permission/ownership checks are part of the local-attacker threat model documented in the file. The logic itself is straightforward and well-tested, but the category of change (filesystem trust boundary for executed code) is one where I default to human review rather than bot approval.
Other factors
- All five of my earlier inline comments (cache-rm count, stale doc comments, bunfig/
pm cache rmdivergence, unwritable-HOME fallback, scoped-package count) are resolved; the last one was intentionally left as-is with a reasonable justification (matches legacy semantics). - CodeRabbit's
tempDirsuggestion was applied in 9c5e8dd. - The bug-hunting system found no issues on the current revision.
- CI shows two failures (
serve-body-leak.test.ts, elysiastream.test.ts) that are unrelated to these files. - New tests exercise both the primary path and both fallback paths, plus the
pm cache rminteraction.
9c5e8dd to
e3f69ff
Compare
|
Rebased onto origin/main. The conflict was confined to the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/cli/bunx_command.rs`:
- Around line 1322-1343: The post-open cache validation can be bypassed by
rename/swap races; instead of re-checking the path string after calling
Fd::cwd().make_open_path(bunx_cache_dir), perform an fstat/fmetadata check on
the opened handle (the value returned by make_open_path / bunx_install_dir) to
verify ownership/permissions (the same checks done by is_trusted_cache_root),
and bind the child to that opened directory (use fchdir or openat relative to
the directory FD) so the child cannot be pointed at a different path;
alternatively, if any post-open lookup/lstat/fstat fails, fail closed by
re-opening and re-validating immediately before spawn. Use the opened directory
FD rather than re-validating bunx_cache_dir string (functions to touch:
Fd::cwd().make_open_path / bunx_install_dir usage, is_trusted_cache_root checks,
and the code path that spawns `bun add`) and ensure errors call
Output::err_generic/Global::exit as before.
🪄 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: 818bc6f4-8932-4339-88ab-c24412620461
📒 Files selected for processing (3)
src/runtime/cli/bunx_command.rssrc/runtime/cli/package_manager_command.rstest/cli/install/bunx.test.ts
There was a problem hiding this comment.
All my earlier feedback has been addressed — thanks. I'm deferring final sign-off to a human since this is a user-visible behavior change to bunx's on-disk cache layout with security-adjacent permission/ownership handling, and it's worth a maintainer confirming the new default location and fallback semantics are what's wanted.
Extended reasoning...
Overview
This PR relocates the bunx package cache on POSIX from $TMPDIR/bunx-<uid>-<pkg> to <install cache>/.bunx-<uid>/<pkg> (a 0700 per-user directory under the same root bun install uses), with a fallback to the legacy temp-dir layout when the install cache can't be resolved or created. It also updates bun pm cache rm to count and delete the new .bunx-<uid> subtree using the same env-only resolution bunx uses (so bunfig install.cache.dir divergence doesn't leave bunx packages behind), refreshes several doc comments describing the threat model, and adds four new tests covering the new location, both fallback paths, and the pm cache rm interaction. Net change is roughly +180 lines of Rust across bunx_command.rs / package_manager_command.rs and +170 lines of tests.
Security risks
The change is security-adjacent: bunx already carries hardening (uid-ownership checks on cached binaries, is_trusted_cache_root, post-install re-verification) precisely because the old cache lived in a world-writable temp dir. This PR moves the primary location to a private 0700 directory — a strict improvement — and keeps all existing checks running unconditionally for the temp-dir fallback and as defense-in-depth. A second is_trusted_cache_root call was added after the post-install make_open_path so a directory that becomes group/other-writable between the early probe and the install is still refused. I don't see a way the new code weakens the existing protections; the main residual risk is the usual TOCTOU surface around mkdir_recursive_at_mode + later make_open_path, which is no worse than before and is mitigated by the post-create trust check.
Level of scrutiny
This warrants a human look rather than bot-only approval. It's a user-visible product decision (changes where bunx stores files on every POSIX user's machine, implements XDG compliance per #1678, mirrors npx semantics), and over the review cycle it needed four substantive fixes — a stale-count message, a functional regression when bunfig install.cache.dir diverged from the env-resolved root, a robustness regression when the install cache root was unwritable, and stale doc comments — all of which the author addressed cleanly with added tests. The current diff looks correct to me and the bug hunter found nothing this round, but the iteration history and the cross-cutting interaction between bunx and pm cache rm make this the kind of change a maintainer should sign off on.
Other factors
No CODEOWNERS cover these paths. All five of my prior inline comments are resolved (the scoped-package count nit was intentionally left as-is to match the legacy temp-dir sweep semantics, which is reasonable). CodeRabbit's tempDir suggestion was applied. Test coverage for the new behavior is solid (new-location layout + 0700 mode, env-unset fallback, unwritable-root fallback, bunfig-divergence pm cache rm). CI build #58808 for the latest commit is in progress.
There was a problem hiding this comment.
All earlier feedback has been addressed and I have no remaining concerns, but since this reworks cache-directory ownership/permission validation and TOCTOU hardening for bunx, it's worth a human sign-off.
Extended reasoning...
Overview
This PR relocates the bunx package cache on POSIX from $TMPDIR/bunx-<uid>-<pkg> to <install cache>/.bunx-<uid>/<pkg> (a 0700 per-user directory under the same root bun install uses), with a temp-dir fallback when the install cache cannot be resolved or created. It touches src/runtime/cli/bunx_command.rs (~100 lines of new path-resolution / mkdir / fstat+lstat trust-check logic), src/runtime/cli/package_manager_command.rs (counting .bunx-<uid> entries before bun pm cache rm wipes the install cache), and adds five new tests in test/cli/install/bunx.test.ts.
Security risks
The change is security-adjacent by design: it modifies where bunx writes and execs cached binaries, adds a post-open dual fstat/lstat ownership+mode validation of the cache directory, refactors is_trusted_cache_root into a stat-predicate helper, and changes the threat model for the primary path from "predictable path under world-writable temp" to "0700 directory under the user's home". The temp-dir fallback retains the pre-existing exposure on non-sticky world-writable temp dirs (documented as out of scope). A CodeRabbit-flagged rename/swap race was tightened to fail-closed on both the opened fd and the path. None of this introduces new attack surface that I can see — it strictly narrows it on the primary path — but it is exactly the kind of permission/uid/TOCTOU logic that benefits from a maintainer's eyes.
Level of scrutiny
Medium-high. This is a user-visible behavioral change (cache location, persistence across temp-dir cleanup, XDG compliance) plus security-sensitive filesystem trust checks in a command that ultimately execs downloaded code. It went through seven rounds of inline feedback (count accuracy, bunfig divergence, unwritable-HOME fallback, redundant delete after rebase, eager-mkdir side effect, stale doc comments) — all addressed and resolved — and a rebase onto #31495 that required non-trivial conflict resolution in pm cache rm.
Other factors
The bug-hunting system found nothing on the current revision. CI shows one unrelated failure (bun-serve-html.test.ts segfault on x64). Test coverage for the new behavior is good (per-user location + 0700, no-env fallback, unwritable-cache fallback, pm cache rm count with bunfig divergence, refusal cases). I'm deferring rather than approving solely because of the security-adjacent nature of the directory-trust logic, not because of any outstanding concern with the implementation.
On POSIX, the bunx install cache now lives at <install cache>/.bunx-<uid>/<pkg>@<version> under the same install cache directory bun install uses (BUN_INSTALL_CACHE_DIR / BUN_INSTALL / XDG_CACHE_HOME / HOME), with the .bunx-<uid> subtree created with mode 0700. When no install cache directory can be resolved, bunx keeps the previous $TMPDIR/bunx-<uid>-<pkg>@<version> layout. Windows behavior is unchanged. After creating or opening the cache directory, bunx re-checks that it is a directory owned by the current user and not group/other writable before using it.
- bun pm cache rm resolves the bunx cache root the same way bunx does and removes it explicitly, so it is cleared even when the install cache is configured elsewhere. - bunx falls back to the temp directory layout when the per-user cache directory cannot be created, matching how bun install degrades when its cache directory is unavailable.
bun pm cache rm: drop the separate bunx cache delete now that the cache directory removal already covers it.
3b57f00 to
343e5dd
Compare
2 participants
What changed
On POSIX, the bunx package cache moves out of the shared temp directory and into the per-user bun install cache directory — the same root
bun installresolves (BUN_INSTALL_CACHE_DIR>BUN_INSTALL>XDG_CACHE_HOME>HOME, e.g.~/.bun/install/cache/):<install cache>/.bunx-<uid>/<pkg>@<version>/node_modules/.bin/<bin>. The.bunx-<uid>subtree is created with mode0700.$TMPDIR/bunx-<uid>-<pkg>@<version>layout.This matches what
npxdoes (it keys its ephemeral installs under the user's npm cache at~/.npm/_npx/<hash>rather than the temp dir), and makes the cache survive temp-dir cleanup.bun pm cache rmalready deletes the whole install cache (which now contains.bunx-<uid>) and still sweeps the temp directory for the legacy/fallbackbunx-<uid>-*entries, so cache clearing keeps working for both layouts.Behavior notes
$TMPDIR/bunx-*caches are no longer consulted on POSIX, so the firstbunx <pkg>after upgrading reinstalls; stale temp entries are removed bybun pm cache rmor normal temp cleanup.BUN_INSTALL_CACHE_DIR(env only); a bunfiginstall.cache.diris not consulted for the bunx root, same as before.Tests
test/cli/install/bunx.test.tsto point at the new location.BUN_INSTALL_CACHE_DIR/.bunx-<uid>with0700permissions, that nothing is created in$TMPDIR, and that--no-installreuses it across runs.HOME/XDG_CACHE_HOME/BUN_INSTALL/BUN_INSTALL_CACHE_DIR).bunx.test.tsrun: 33 pass, 2 pre-existing local failures unrelated to this change.bun-pm.test.ts: 12 pass. Cross-targetcargo check(linux x64, windows x64, darwin arm64, android x64) passes.