Skip to content

install: refuse untrusted extraction cache and make --force re-verify - #35105

Open
robobun wants to merge 9 commits into
mainfrom
farm/4ec62df0/install-cache-trust
Open

install: refuse untrusted extraction cache and make --force re-verify#35105
robobun wants to merge 9 commits into
mainfrom
farm/4ec62df0/install-cache-trust

Conversation

@robobun

@robobun robobun commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Problem

The install extraction cache is keyed on name@version[@@host]@@@N and trusted on directory-name match alone. determine_preinstall_state does is_folder_in_cache = directory_exists_at, and the installer's package_missing_from_cache probes for <entry>/package.json through any symlink. The lockfile's integrity is compared to bytes exactly once, at tarball download time, so whatever sits in a matching cache directory is linked into node_modules and executed without further checks:

  • a rewritten file inside a cached extraction survives bun install --frozen-lockfile, --force, and --no-cache (manifest-only flag) with zero registry requests
  • a pre-planted directory for a never-fetched version skips the tarball GET entirely and the fresh bun.lock records the registry's genuine integrity for bytes that never touched the wire
  • a symlink at the entry name is followed into an attacker directory
  • the cache root itself has no owner/permission check (bunx already has is_trusted_cache_root; the install cache did not)
bun install                         # lockfile pins sha512, extraction lands in shared cache
C=$(ls -d ~/.bun/install/cache/victim-pkg@1.0.0*)
echo "module.exports = 'POISONED';" > "$C/index.js"
rm -rf node_modules
bun install --frozen-lockfile       # 0 registry requests
bun -p 'require("victim-pkg")'      # => POISONED

Cause

  • ensure_cache_directory opens the configured cache root with make_open_path and returns it unconditionally; there is no fstat for owner or mode.
  • is_folder_in_cache and the installer cache probes use fstatat(.., 0) / exists_at, which follow symlinks.
  • Enable::FORCE_INSTALL is read only by the node_modules re-link path (hoisted_install, isolated_install); neither determine_preinstall_state nor package_missing_from_cache consulted it, so --force never re-downloaded a cache hit.

Fix

  • ensure_cache_directory now fstats the opened shared cache and, on POSIX, rejects it unless it is a real directory owned by the effective uid with no group/other write bits (the same check bunx applies to its cache root). A rejected root warns and falls through to the existing per-project node_modules/.cache path. make_open_path always creates the directory 0o755, so this only fires on externally created or chmod'd cache dirs.
  • Cache-hit probes use lstatat and require S_ISDIR, so a symlink at the entry name is treated as absent and re-fetched (and renameat_concurrently then replaces the symlink with the fresh extraction).
  • --force bypasses the on-disk cache hit in determine_preinstall_state and on the first installer pass (hoisted and isolated), enqueuing a download whose integrity is verified against the lockfile. The post-download callback re-enters with the preinstall state set to Done (by runTasks) and links the fresh extraction instead of re-enqueuing.

This does not address the hardlink write-back vector (a write into any project's node_modules mutating the shared cache inode) or same-user direct tampering; those need a content-addressed or read-only store and are left for a follow-up.

Related: #31868 scopes --force re-download to URL-keyed tarballs for the stale-content case; this change applies it to every extraction-cache hit as the integrity recovery path.

Verification

test/cli/install/bun-install-cache-trust.test.ts spins up a local registry serving a known tarball, installs once to populate the cache and lockfile, then:

  • tampers with the cached index.js, removes node_modules, runs bun install --force, and asserts the tarball was re-fetched and node_modules/baz/index.js matches the registry bytes (hoisted and isolated linkers)
  • replaces the cache entry with a symlink to an attacker directory, runs bun install --frozen-lockfile, and asserts the tarball was re-fetched and the clean bytes were installed
  • points BUN_INSTALL_CACHE_DIR at a 0o777 directory, runs install, and asserts the warning is printed, the shared cache is not populated, and node_modules/.cache is used instead

All four fail on the released build (one tarball GET, poisoned node_modules, no warning) and pass with this change. rust:check-all is clean on all ten targets; bun-install-patch, bun-install-tarball-integrity, and bun-pm pass unchanged.


no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-cache-trust.test.ts

@robobun

robobun commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 7:33 AM PT - Jul 29th, 2026

@robobun, your commit 4fff8e8 has 2 failures in Build #85048 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 35105

That installs a local version of the PR into your bun-35105 executable, so you can run:

bun-35105 --bun

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

The install extraction cache now validates configured roots and cache entries, rejects symlinked or untrusted directories, bypasses cached extraction state during forced installs, and refetches packages when verification requires it. Integration tests cover poisoning, symlinks, writable roots, and fallback behavior.

Install Cache Trust

Layer / File(s) Summary
Trusted cache root selection
src/install/PackageManager/PackageManagerDirectories.rs, src/sys/lib.rs, src/install/lib.rs, src/runtime/cli/bunx_command.rs, src/install/extract_tarball.rs
Configured cache roots and related directories are checked for ownership, directory type, and write permissions; invalid roots fall back to a project-local cache, and Windows move handling includes NOTDIR.
Symlink-safe cache checks
src/install/PackageManager/PackageManagerDirectories.rs, src/install/PackageManager/PackageManagerLifecycle.rs, src/install/PackageInstall.rs, src/install/isolated_install.rs, src/install/isolated_install/Installer.rs
Cache probes require real directories, with package metadata checks retained for npm resolutions and patched paths updated to use the symlink-safe helper.
Forced cache refetch flow
src/install/PackageManager/PackageManagerLifecycle.rs, src/install/PackageInstaller.rs, src/install/isolated_install.rs
Forced installs no longer trust existing cache-hit states and enqueue refetches when verification and preinstall conditions require them, removing stale derived patch entries.
Cache trust integration coverage
test/cli/install/bun-install-cache-trust.test.ts
Tests cover poisoned caches, symlinked entries, writable shared roots, linker modes, refetches, and project-cache fallback.

Possibly related PRs

  • oven-sh/bun#36229: Related cache-handling changes in the install subsystem, including Windows filesystem behavior and rename-collision handling.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the core change: rejecting untrusted extraction caches and making --force re-verify cache hits.
Description check ✅ Passed The description is detailed and covers the change, rationale, and verification, even though it uses custom headings instead of the template.

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Found 3 issues this PR may fix:

  1. bun install --force does not re-extract local .tgz dependencies after file is replaced #29372 - PR makes --force bypass the extraction cache and re-download/re-verify, directly fixing --force not re-extracting stale cached .tgz dependencies
  2. Security: Fixed-seed non-cryptographic hashes (Wyhash11 / std.hash.Wyhash) are algebraically collidable, enabling trustedDependencies RCE, scoped-registry token leak, and cache poisoning #32741 - PR mitigates the cache-poisoning attack vector (symlink rejection, ownership/permission checks, integrity re-verification) described in this security issue
  3. patchedDependencies silently not applied on CI installs with a warm global cache (Expo EAS builders) #33520 - PR's cache trust hardening and --force re-verification fix warm global cache causing patches to be silently skipped on CI

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #29372
Fixes #32741
Fixes #33520

🤖 Generated with Claude Code

@robobun

robobun commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

Not linking the suggested issues:

Comment thread test/cli/install/bun-install-cache-trust.test.ts Outdated
Comment thread src/install/PackageInstall.rs Outdated
Comment thread src/install/PackageManager/PackageManagerDirectories.rs
@robobun

robobun commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

CI status on 4fff8e8 (build #85048, rebased onto main @ 59242d6): bun-install-cache-trust.test.ts passes on every lane including Windows (junction rejection verified). Remaining red is unrelated to this diff:

  • test/js/bun/http/proxy-stress-protocol.test.ts and test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts: HTTP-proxy and worker_threads stress tests, unrelated to install; reported for main-break triage.
  • The rest are [flaky] (passed alone or on retry): file-watcher, svelte integration, timers, net-server, bun-serve-html, grpc-js, bun-audit, fastutf8stream, 13316.

None touch the code paths this PR changes (cache-root trust check, symlink/junction rejection, --force refetch, Windows rename-over-reparse-point retry). Ready for review.

Comment thread src/install/PackageManager/PackageManagerDirectories.rs
Comment thread src/install/PackageInstaller.rs
Comment thread src/install/isolated_install.rs Outdated
Comment thread test/cli/install/bun-install-cache-trust.test.ts Outdated
Comment thread src/install/PackageManager/PackageManagerDirectories.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/install/isolated_install.rs (1)

2345-2387: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Clear the derived patch cache on --force.
force_install() here only refreshes the base cache entry. If <entry>_patch_hash=<h> already exists, apply_package_patch() returns early and reuses it, so a forced reinstall can still serve stale patched contents. Mirror the hoisted linker’s delete_tree(...) cleanup before refetching.

🤖 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/install/isolated_install.rs` around lines 2345 - 2387, Update the
force_install branch in the preinstall-state match to remove the derived patch
cache directory before refreshing the base cache entry. Mirror the hoisted
linker’s delete_tree cleanup using the package’s patch-cache path, ensuring
apply_package_patch cannot reuse stale patched contents during a forced
reinstall.
🤖 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.

Outside diff comments:
In `@src/install/isolated_install.rs`:
- Around line 2345-2387: Update the force_install branch in the preinstall-state
match to remove the derived patch cache directory before refreshing the base
cache entry. Mirror the hoisted linker’s delete_tree cleanup using the package’s
patch-cache path, ensuring apply_package_patch cannot reuse stale patched
contents during a forced reinstall.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0088b5b3-f755-4e08-9fc2-6b5cfc54496d

📥 Commits

Reviewing files that changed from the base of the PR and between ffe42b5 and 3ca63af.

📒 Files selected for processing (9)
  • src/install/PackageInstall.rs
  • src/install/PackageInstaller.rs
  • src/install/PackageManager/PackageManagerDirectories.rs
  • src/install/isolated_install.rs
  • src/install/isolated_install/Installer.rs
  • src/install/lib.rs
  • src/runtime/cli/bunx_command.rs
  • src/sys/lib.rs
  • test/cli/install/bun-install-cache-trust.test.ts

Comment thread src/install/isolated_install.rs Outdated
Comment thread test/cli/install/bun-install-cache-trust.test.ts Outdated
Comment thread src/sys/lib.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (3)
test/cli/install/bun-install-cache-trust.test.ts (2)

198-198: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Do not convert a cache-directory read failure into success.

catch(() => []) lets this test pass if the shared cache becomes unreadable or disappears. Read it directly so that condition fails the test.

As per coding guidelines, assertions must fail for the intended reason.

🤖 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/install/bun-install-cache-trust.test.ts` at line 198, Update the
cache directory read in the test around readdir(cacheDir) to remove the catch(()
=> []) fallback. Let readdir failures propagate so unreadable or missing shared
caches fail the test rather than being treated as empty.

Source: Coding guidelines


100-129: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Add a mismatched-integrity forced-refetch case.

This only proves re-download: the registry still serves bytes matching its metadata, so it passes if --force skips lockfile-integrity validation. After the first install, serve altered tarball bytes with the original lockfile integrity and assert that bun install --force fails without installing them.

As per coding guidelines, every behavioral change needs an automated regression test that proves the strongest exact invariant.

🤖 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/install/bun-install-cache-trust.test.ts` around lines 100 - 129,
Extend the test named “--force re-downloads and re-verifies on a cache hit” with
a mismatched-integrity forced-refetch scenario: after the initial install, make
the registry serve altered tarball bytes while retaining the original metadata
integrity, then run `runInstall` with `--force`. Assert installation fails and
the altered package is not installed, while preserving the existing successful
clean-cache verification case.

Source: Coding guidelines

src/sys/lib.rs (1)

1270-1273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Keep added code comments to three lines or fewer.

  • src/sys/lib.rs#L1270-L1273: condense the cache-root predicate documentation.
  • src/sys/lib.rs#L7305-L7308: shorten the helper description.
  • src/install/isolated_install.rs#L2349-L2353: condense the forced patch-cache rationale.
  • test/cli/install/bun-install-cache-trust.test.ts#L149-L152: shorten the junction rationale.

As per coding guidelines, “Keep code comments to three lines or fewer.”

🤖 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/sys/lib.rs` around lines 1270 - 1273, Condense the comments at
src/sys/lib.rs lines 1270-1273 and 7305-7308, src/install/isolated_install.rs
lines 2349-2353, and test/cli/install/bun-install-cache-trust.test.ts lines
149-152 so each is no more than three lines while preserving its essential cache
predicate, helper, forced patch-cache, or junction rationale.

Source: Coding guidelines

🤖 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/install/isolated_install.rs`:
- Around line 2354-2357: Update the stale patched-cache cleanup in the
PatchInfo::Patch branch to handle the Result from delete_tree instead of
discarding it. Propagate the deletion error so the install task fails
immediately when removal of pkg_cache_dir_subpath is unsuccessful, while
preserving the existing behavior on successful deletion.

---

Outside diff comments:
In `@src/sys/lib.rs`:
- Around line 1270-1273: Condense the comments at src/sys/lib.rs lines 1270-1273
and 7305-7308, src/install/isolated_install.rs lines 2349-2353, and
test/cli/install/bun-install-cache-trust.test.ts lines 149-152 so each is no
more than three lines while preserving its essential cache predicate, helper,
forced patch-cache, or junction rationale.

In `@test/cli/install/bun-install-cache-trust.test.ts`:
- Line 198: Update the cache directory read in the test around readdir(cacheDir)
to remove the catch(() => []) fallback. Let readdir failures propagate so
unreadable or missing shared caches fail the test rather than being treated as
empty.
- Around line 100-129: Extend the test named “--force re-downloads and
re-verifies on a cache hit” with a mismatched-integrity forced-refetch scenario:
after the initial install, make the registry serve altered tarball bytes while
retaining the original metadata integrity, then run `runInstall` with `--force`.
Assert installation fails and the altered package is not installed, while
preserving the existing successful clean-cache verification case.
🪄 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: 92b160d2-76fe-48a0-b825-2237da6c775b

📥 Commits

Reviewing files that changed from the base of the PR and between 3ca63af and 306a896.

📒 Files selected for processing (3)
  • src/install/isolated_install.rs
  • src/sys/lib.rs
  • test/cli/install/bun-install-cache-trust.test.ts

Comment thread src/install/isolated_install.rs
Comment thread src/install/PackageInstaller.rs
Comment thread src/install/PackageManager/PackageManagerDirectories.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new issues found on this pass, but this is a security-hardening change to the install cache trust model (cache-root ownership check, symlink/junction rejection, --force re-download semantics) touching both linkers plus new Windows NT FFI in src/sys/lib.rs, so it warrants a human sign-off.

What was reviewed:

  • --force re-entry: post-download callback re-enters with PreinstallState::Done (set by runTasks), so force_cache_refetch is false on the second pass and does not re-enqueue in a loop (both linkers).
  • cache_entry_is_dir call sites: all six former directory_exists_at cache probes now route through it; the isolated apply_package_patch early-return uses it too.
  • nt_query_basic_attrs_at refactor preserves exists_at_type_nt's behavior (same OBJECT_ATTRIBUTES setup, same NTSTATUS→errno mapping); get_file_attributes_at now shares the body.
  • Acknowledged follow-ups left open: @scope intermediate-component symlink, and forced rebuild through PatchTask::apply when the best-effort delete_tree of the derived patched entry fails.
Extended reasoning...

Overview

The PR hardens bun install's extraction cache against tampering: (1) ensure_cache_directory now fstats the shared cache root and rejects it on POSIX unless it is a real directory owned by the effective uid with no group/other write bits, falling back to node_modules/.cache; (2) all cache-hit probes route through a new cache_entry_is_dir that uses lstatat/NtQueryAttributesFile so a symlink or junction planted at the entry name is treated as absent; (3) --force now bypasses the on-disk cache hit in determine_preinstall_state and on the first installer pass (hoisted and isolated), enqueuing a fresh download whose integrity is re-verified. Supporting changes: a shared stat_is_owner_only_writable_dir predicate consolidating three copies of the trust check (bunx, install cache, bun --bun node-symlink dir), a shared nt_query_basic_attrs_at NT syscall body, and an ENOTDIR retry arm in the Windows rename-into-cache path so a junction at the destination is moved aside like any other occupied slot. Tests cover --force re-download on both linkers, symlink/junction rejection on both platforms, and the writable-cache-root fallback.

Security risks

This is the security-sensitive code. The change tightens trust rather than loosening it, but the new policy surface is what a human should own: the POSIX-only is_trusted_cache_root check will start rejecting existing shared caches that are group-writable or root-owned (falls back with a warning, does not fail the install); --force now re-downloads every cache hit, changing its cost profile; the Windows trust check is a no-op (is_trusted_cache_root returns true). The PR body explicitly scopes out the hardlink write-back vector and same-user tampering; two follow-ups are noted (scoped-package @scope intermediate symlink, and force-rebuild through PatchTask::apply when the best-effort delete_tree of the derived patched entry fails).

Level of scrutiny

High. This is production package-manager code on the path that decides whether to trust bytes on disk over a registry-verified download, with new hand-written Windows NT FFI (nt_query_basic_attrs_at / get_file_attributes_at) and behavior changes to --force that every user of bun install --force will hit. It is well outside the "simple, mechanical, or obvious" bar for auto-approval.

Other factors

The PR has been through four review rounds; all prior 🔴/🟡 findings are marked addressed or explicitly deferred with rationale. The bug-hunting pass on the current head found nothing new. The --force re-entry does not loop because the post-download callback runs after runTasks sets PreinstallState::Done, making force_cache_refetch false on the second pass. The exists_at_type_nt refactor is behavior-preserving. Remaining acknowledged gaps (patched-entry force-rebuild, @scope intermediate symlink) are defense-in-depth on top of the cache-root ownership gate and are reasonable follow-ups, but a maintainer should confirm that scoping decision.

robobun and others added 8 commits July 29, 2026 08:52
The extraction cache is keyed on name@version[@@host]@@@n only, and a
directory that matches was trusted unconditionally: no owner/permission
check on the cache root, no symlink rejection on the entry, and no way
to make --force re-download a hit. A cache-dir writer (another user on a
shared host, or a symlink planted under the predictable name) could
supply the bytes that end up in node_modules without ever being compared
to the lockfile's integrity.

- ensure_cache_directory now fstat's the opened shared cache and, on
  unix, rejects it when it is not a real directory owned by the current
  user, or is group/other-writable (matching bunx's is_trusted_cache_root).
  Rejection warns and falls through to the per-project node_modules/.cache.
- is_folder_in_cache and the per-linker cache-hit probes use lstatat so
  a symlink at the cache-entry name is treated as absent and re-fetched.
- --force now bypasses the on-disk cache hit in determine_preinstall_state
  and on the first installer pass (hoisted and isolated), so the tarball
  is re-downloaded and its integrity re-verified against the lockfile.
  The post-download callback re-enters with the preinstall state set to
  Done and links the fresh extraction instead of re-enqueuing.
…ce, share trust predicate

- cache_entry_is_dir on Windows now queries NtQueryAttributesFile and
  refuses FILE_ATTRIBUTE_REPARSE_POINT, since lstatat->fstat there maps
  junctions to S_IFDIR and never sets S_IFLNK. New
  bun_sys::get_file_attributes_at exposes the attributes fd-relative.
- When the hoisted --force refetch fires for a patched dependency, remove
  the stale <entry>_patch_hash=<h> cache directory before enqueuing the
  download so the re-entry enqueues ApplyPatch against the fresh base
  (the npm extract task does not re-apply the patch itself).
- The isolated linker's apply_package_patch now uses cache_entry_is_dir
  for its patched-entry probe, matching the hoisted twin.
- bun_sys::stat_is_owner_only_writable_dir centralises the
  ISDIR && st_uid==uid && !(mode&(IWGRP|IWOTH)) check; bunx, the install
  cache root, and the per-sha --bun node dir now call it.
- describe.concurrent for the new test file; the four tests share no state.
…body; test junctions on Windows

- The isolated linker's --force branch now deletes the derived
  <entry>_patch_hash=<h> cache directory before enqueuing the base
  refetch, mirroring the hoisted linker, so apply_package_patch
  regenerates it from the fresh base instead of reusing stale contents.
- nt_query_basic_attrs_at factors the shared NtQueryAttributesFile body
  out of exists_at_type_nt; get_file_attributes_at is now a thin mapping
  over it.
- The link-rejection test now runs on Windows using a junction (no admin
  required) to exercise the new FILE_ATTRIBUTE_REPARSE_POINT check.
…parse point

cache_entry_is_dir now rejects a junction at the entry name, so the
re-extract tries to publish over it. NtSetInformationFile with
FileRenameInformation returns STATUS_NOT_A_DIRECTORY for that target
even with ReplaceIfExists; treat it like the other occupied-destination
errnos and move the existing entry aside before retrying.
@robobun
robobun force-pushed the farm/4ec62df0/install-cache-trust branch from fff94a5 to 30d8d9e Compare July 29, 2026 09:11
Comment thread src/install/PackageInstall.rs Outdated
Comment thread src/install/PackageInstaller.rs Outdated
Comment thread src/install/PackageInstaller.rs Outdated
Comment thread src/install/PackageManager/PackageManagerDirectories.rs Outdated
Comment thread src/install/PackageManager/PackageManagerDirectories.rs Outdated
Comment thread src/install/PackageManager/PackageManagerLifecycle.rs Outdated
Comment thread src/install/extract_tarball.rs Outdated
Comment thread src/install/isolated_install.rs Outdated
Comment thread src/install/isolated_install.rs Outdated
Comment thread src/install/isolated_install.rs
Comment thread src/sys/lib.rs Outdated
Comment thread src/sys/lib.rs
Comment thread src/sys/lib.rs
Comment thread src/sys/lib.rs
Comment thread src/install/PackageInstaller.rs
Comment thread src/install/PackageInstaller.rs
Comment thread src/install/PackageManager/PackageManagerDirectories.rs
Comment thread src/install/PackageManager/PackageManagerDirectories.rs
Comment thread src/install/PackageManager/PackageManagerLifecycle.rs
Comment thread src/install/isolated_install.rs
Comment thread src/sys/lib.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/install/PackageManager/PackageManagerDirectories.rs`:
- Around line 379-393: Update the Windows/non-Unix implementation of
is_trusted_cache_root to validate the cache directory’s owner and writability
using Windows ACL/security metadata, matching the Unix trust gate; do not leave
it unconditionally returning true. If equivalent validation is unavailable,
explicitly document the Windows platform exception and its security implications
at the implementation site.
🪄 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: c1ca6c2d-ca4a-4460-a362-8195cae62f19

📥 Commits

Reviewing files that changed from the base of the PR and between 306a896 and 4fff8e8.

📒 Files selected for processing (8)
  • src/install/PackageInstall.rs
  • src/install/PackageInstaller.rs
  • src/install/PackageManager/PackageManagerDirectories.rs
  • src/install/PackageManager/PackageManagerLifecycle.rs
  • src/install/extract_tarball.rs
  • src/install/isolated_install.rs
  • src/install/isolated_install/Installer.rs
  • src/install/lib.rs

Comment thread src/install/PackageManager/PackageManagerDirectories.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new findings on this pass, but this is security-sensitive install-cache-trust code (permission checks on the shared cache root, symlink/junction rejection, --force re-verify semantics) touching both linkers and Windows NT syscall plumbing — a maintainer should sign off.

What was reviewed:

  • --force re-entry gating in PackageInstaller.rs / isolated_install.rs — the NEEDS_VERIFY + PreinstallState::Done guard prevents re-enqueue loops on the post-download pass.
  • cache_entry_is_dir on POSIX (lstatat + S_ISDIR) and Windows (NtQueryAttributesFile reparse-point bit) — the exists_at_type_nt refactor preserves the prior FILE_ATTRIBUTE_DIRECTORY / errno mapping.
  • stat_is_owner_only_writable_dir consolidation — the three call sites (bunx, install cache root, bun-node shim dir) previously hand-rolled the same predicate; semantics unchanged.
  • ensure_cache_directory fallback — on rejection it clears Enable::CACHE and cache_directory_path before continue, so the loop lands on the node_modules/.cache branch without re-probing the rejected root.
Extended reasoning...

Overview

This PR hardens the bun install extraction cache against poisoning: it adds a POSIX owner/mode check on the shared cache root (falling back to node_modules/.cache with a warning), replaces symlink-following directory_exists_at cache probes with a no-follow cache_entry_is_dir (POSIX lstatat, Windows NtQueryAttributesFile + FILE_ATTRIBUTE_REPARSE_POINT), and makes --force bypass extraction-cache hits so the tarball is re-fetched and integrity re-verified. It threads the same probe through the hoisted and isolated linkers, the patched-entry path, and the isolated Installer::apply_package_patch guard. src/sys/lib.rs gains a shared stat_is_owner_only_writable_dir predicate (deduplicating three hand-rolled checks in bunx / install / the bun-node shim dir) and a get_file_attributes_at helper built by extracting the existing NtQueryAttributesFile body. extract_tarball.rs adds ENOTDIR to the Windows rename-into-cache retry set so a junction at the destination is replaced. A new test file covers --force re-fetch on both linkers, symlink/junction rejection, and the 0o777 cache-root warning.

Security risks

The change is itself a security hardening, but the surface it modifies is exactly where the risk lives: the cache-root trust predicate decides whether ~/.bun/install/cache is used or bypassed, and getting it wrong in either direction has consequences (false-negative → cache poisoning remains; false-positive → every install falls back to a per-project cache and re-downloads everything). The Windows arm of is_trusted_cache_root is intentionally a no-op (matching bunx precedent) — acknowledged as a follow-up. The @scope intermediate-component symlink gap and same-user tampering are explicitly scoped out. The --force change alters user-visible behavior (every --force install now re-downloads all tarballs, not just re-links), which is the intended integrity-recovery semantics but is a behavioral change a maintainer should confirm.

Level of scrutiny

High. This is the package manager's install path — the code that decides which bytes end up in node_modules and get executed by lifecycle scripts and require(). It spans both linkers, the preinstall state machine, and platform-specific syscall wrappers. REVIEW.md's Security section ("security checks fail closed and cover every path to the protected effect") applies directly, and the PR itself enumerates paths it does not cover (hardlink write-back, same-user tampering, Windows ACL). A maintainer should confirm the scope boundary and the --force behavior change are acceptable.

Other factors

Four rounds of prior inline review from this bot have been addressed or explicitly deferred as follow-ups (isolated --force patched-entry delete_tree, @scope intermediate-symlink check, Windows ACL check, patched-cache test coverage). CodeRabbit and the comment-cop linter have also weighed in; all threads are resolved. The test file uses a local Bun.serve registry, tempDir, describe.concurrent, and asserts tarball GET counts + installed bytes; the 0o777 root test is correctly skipIf(isWindows). The Windows junction test relies on fs.symlink(..., 'junction') and the new ENOTDIR retry arm — CI will exercise that. No bugs were surfaced by the automated hunt this run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants