Skip to content

install: key npm cache entries by their integrity, derive URL-based cache names with SHA-256 - #37756

Closed
Jarred-Sumner wants to merge 10 commits into
mainfrom
claude/install-cache-entry-identity
Closed

install: key npm cache entries by their integrity, derive URL-based cache names with SHA-256#37756
Jarred-Sumner wants to merge 10 commits into
mainfrom
claude/install-cache-entry-identity

Conversation

@Jarred-Sumner

@Jarred-Sumner Jarred-Sumner commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

npm packages in the global install cache are now stored under a folder name that ends in a short fingerprint of the integrity value the registry manifest or the lockfile provides for them:

~/.bun/install/cache/lodash@4.17.21@@@2.0gwamgzgacdck
                                     └── '.' + 13 chars (lowercase base32, the alphabet
                                         bundler chunk hashes use): the first 64 bits of
                                         SHA-256(<cache-dir key> ‖ <hash algorithm> ‖ <digest>)

<cache-dir key> is 16 random bytes kept in <cache dir>/.id. It is created (O_EXCL) the first time a cache directory is used and read once per process when the cache directory is opened, so it adds no per-package work; because it lives inside the cache directory it travels with saved/restored CI caches. Keying the fingerprint means that which two digests would share a name cannot be worked out ahead of time, so 64 bits are enough and the name stays short.

An entry that was extracted for one digest is therefore never reused by an install whose lockfile pins a different digest for the same name@version; that install fetches and verifies its own copy, and both entries coexist. The cache-hit check is unchanged: it is still a single existence probe on the folder. Packages without an integrity value, and everything installed with --no-verify (where nothing vouches for the digest), use the plain name@version@@@2 name, so a later verifying install fetches its own copy rather than trusting them. In-flight download tasks are keyed by the digest as well, so two lockfile entries for the same name@version with different digests are fetched separately. For package names long enough that the fingerprinted component would not fit in 255 bytes, the fingerprint is left off.

The cache layout version is bumped from @@@1 to @@@2, and remote/local tarball entries (@T@…) and cached bare git clones (<hex>.git) now derive their folder names from a SHA-256 prefix of the URL rather than a Wyhash of it (same width as before).

Consequences worth calling out: every existing cache entry is fetched (git repositories: cloned) again once after upgrading, and the old @@@1 entries are left in place. A cache directory that was populated by an earlier Bun therefore does not satisfy installs on its own until it has been repopulated once — air-gapped setups and CI images with a baked-in cache need one online install (or a re-bake) with this version. This is deliberate (adopting the old entries would keep serving exactly the entries whose origin is unknown) and easy to reverse if preferred.

How did you verify your code works?

New tests in test/cli/install/bun-install-registry.test.ts (hoisted and isolated linkers, plus auto-install) pre-populate cache folders under the previous layout's name, under the digest-less name and under a name for a different digest, and check that bun install / bun --print "require(...)" fetch and use the registry contents instead, and that the matching entry is reused as-is on the next install. A new test in test/cli/install/bun-install.test.ts pre-populates a bare clone of an unrelated repository under the previously derived clone directory name and checks that the dependency is cloned from its own URL. Tests derive expected cache folder names through one helper in test/harness.ts (installCacheFolderName, which reads <cache>/.id); existing tests that assert exact cache folder / clone directory names were updated.

bun-install-registry.test.ts (232 pass), bun-install.test.ts (195), bun-add (54), isolated-install, bun-patch, bun-install-patch, bun-pm, npmrc, bun-lock, lockfile-only, lockfile-version-2, migrate-bun-lockb-v2, bun-run-dir and test/internal/source-lints pass locally with a debug build; the new tests fail against the current release.

Release-build numbers (bun run build:release of this branch vs the installed 1.4.0-canary.1 release; Linux x64, shared machine):

Syscalls per install (strace -f -c, 68-package express fixture, two runs each):

scenario 1.4.0-canary.1 this branch
warm cache, node_modules removed 2024 / 2027 2014 / 2017
warm cache, node_modules present (no-op) 640 618

The install-path syscalls (faccessat2, mkdirat, linkat, getdents64, readlink) are identical in count; reading .id is one openat/read/close per process, and the remaining differences are startup noise (mmap, futex, …).

hyperfine on a 552-package project (--warmup 3 --runs 30, both orders):

scenario 1.4.0-canary.1 this branch
warm cache, rm -rf node_modules before each run 136.8 ms ± 7.0 / 141.1 ms ± 12.8 142.0 ms ± 8.1 / 139.5 ms ± 9.6
warm cache and node_modules present 19.8 ms ± 2.7 / 22.0 ms ± 3.7 23.8 ms ± 6.1 / 22.5 ms ± 4.9

(minimums: 125.9/125.4 ms vs 130.0/127.0 ms and 16.4/17.3 ms vs 16.9/16.3 ms — within run-to-run noise on this machine.)

…ache names with SHA-256

npm packages in the install cache are now stored under
`<name>@<version>@@@1_integrity=<fingerprint>` whenever the registry
manifest or the lockfile provides an integrity value, where the
fingerprint is the leading bytes of that digest. A cache entry that was
extracted for one digest is therefore never picked up by an install whose
lockfile expects a different one, and the cache hit check remains a single
directory probe. Packages without a known integrity keep their previous
folder name. Download tasks are keyed the same way.

Remote/local tarball entries (`@T@...`) and cached bare git clones now
derive their folder names from a SHA-256 prefix of the URL instead of
Wyhash.

Existing cache entries for these packages are fetched again once after
upgrading.
@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator
Updated 4:48 AM PT - Aug 12th, 2026

@Jarred-Sumner, your commit ea44477e65602995fc9af016e22d19e52cd9534c passed in Build #93102! 🎉


🧪   To try this PR locally:

bunx bun-pr 37756

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

bun-37756 --bun

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Npm cache directory names and package task IDs now include package integrity fingerprints. Installer and patch flows pass integrity metadata through cache resolution. Git cache hashes use SHA-256 prefixes. Tests and documentation reflect the new cache paths.

Changes

Npm cache integrity

Layer / File(s) Summary
Integrity fingerprints and cache naming
src/install/integrity.rs, src/install/PackageManager/PackageManagerDirectories.rs, src/install/PackageManager.rs
Adds cache-directory IDs and integrity fingerprints. Npm cache paths now include versioned integrity suffixes when supported. Tarball cache hashes use SHA-256 URL prefixes.
Task identity propagation
src/install/PackageManagerTask.rs, src/install/PackageManager/PackageManagerEnqueue.rs, src/install/patch_install.rs
Includes integrity in npm task IDs and changes Git cache hashing to use SHA-256 URL prefixes.
Installer and patch cache integration
src/install/PackageInstaller.rs, src/install/PackageManager/*, src/install/extract_tarball.rs, src/install/isolated_install*
Passes package integrity through install, extraction, isolated-install, resolution, lifecycle, and patch cache flows.
Validation and documentation
test/cli/install/*, test/harness.ts, docs/pm/*
Tests matching, missing, mismatched, and stale cache entries. Updates Git cache expectations and documents integrity-qualified paths.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main cache identity and SHA-256 URL naming changes.
Description check ✅ Passed The description directly explains the cache changes, behavior, compatibility impact, tests, and benchmark results.
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.

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

@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: 2

Caution

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

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

204-217: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Preserve integrity-qualified cache entry names during disk-cache resolution.

get_installed_versions_from_disk_cache rejects entries containing _integrity= because it parses the complete entry name as a semver. Entries with supported integrity are therefore excluded before lookup. If such entries were parsed, resolve_from_disk_cache would still reconstruct an unsuffixed path with Integrity::default().

Retain the raw entry name for lookup and parse only its version portion.

🤖 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/PackageManager/PackageManagerResolution.rs` around lines 204 -
217, Update get_installed_versions_from_disk_cache to retain each raw cache
entry name while parsing only its version portion, including supported
_integrity= suffixes. Pass the raw entry name through resolve_from_disk_cache
and ensure its path lookup preserves the integrity-qualified filename instead of
reconstructing it with Integrity::default().
🤖 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 `@docs/pm/global-cache.mdx`:
- Line 6: Update the cache identity description to distinguish version-based and
integrity-based uniqueness: `${name}@${version}` allows multiple package
versions, while the tarball integrity fingerprint allows separate cache entries
for the same name and version when their digests differ.

In `@test/cli/install/bun-install-registry.test.ts`:
- Around line 140-143: Update the fallback-cache test’s seeded otherEntry
package metadata to use version 2.0.0, and make its index.js expose a different
observable version. Keep the expected process output at the registry version so
the test verifies integrity-qualified cache identity, rather than name/version
mismatch, prevents reuse.

---

Outside diff comments:
In `@src/install/PackageManager/PackageManagerResolution.rs`:
- Around line 204-217: Update get_installed_versions_from_disk_cache to retain
each raw cache entry name while parsing only its version portion, including
supported _integrity= suffixes. Pass the raw entry name through
resolve_from_disk_cache and ensure its path lookup preserves the
integrity-qualified filename instead of reconstructing it with
Integrity::default().
🪄 Autofix

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: f4fd5bea-1d16-46ae-9e7e-6ddfbb3e2e9a

📥 Commits

Reviewing files that changed from the base of the PR and between 2ffb8d4 and 5e15624.

📒 Files selected for processing (19)
  • docs/pm/cli/install.mdx
  • docs/pm/global-cache.mdx
  • docs/pm/global-store.mdx
  • src/install/PackageInstaller.rs
  • src/install/PackageManager/PackageManagerDirectories.rs
  • src/install/PackageManager/PackageManagerEnqueue.rs
  • src/install/PackageManager/PackageManagerLifecycle.rs
  • src/install/PackageManager/PackageManagerResolution.rs
  • src/install/PackageManager/patchPackage.rs
  • src/install/PackageManagerTask.rs
  • src/install/extract_tarball.rs
  • src/install/integrity.rs
  • src/install/isolated_install.rs
  • src/install/isolated_install/Installer.rs
  • src/install/patch_install.rs
  • test/cli/install/bun-add.test.ts
  • test/cli/install/bun-install-registry.test.ts
  • test/cli/install/bun-install.test.ts
  • test/cli/install/bun-run-dir.test.ts

Comment thread docs/pm/global-cache.mdx Outdated
Comment thread test/cli/install/bun-install-registry.test.ts Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. Security: Fixed-seed non-cryptographic hashes (Wyhash11 / std.hash.Wyhash) are algebraically collidable, enabling trustedDependencies RCE, scoped-registry token leak, and cache poisoning #32741 - Replaces the collidable Wyhash11(url) used to name tarball cache entries (@T@…) and cached bare git clones with a SHA-256 prefix, addressing the persistent-cache half of that issue's still-open PoC5 (two distinct tarball URLs colliding so one entry's content is silently substituted).

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

Fixes #32741

Note: #32741 tracks several PoCs. PoC1/1b, PoC2, PoC3, PoC4 and PoC7 were already fixed by #31218/#32745/#32749; PoC6 (RuntimeTranspilerCache), PoC9 (PackageNameHash) and PoC10 (YAML HashDoS) remain, and Task::Id::for_tarball still uses Wyhash11 for in-flight network dedupe — so this PR partially addresses it rather than closing it outright.

🤖 Generated with Claude Code

Comment thread src/install/PackageManager/PackageManagerResolution.rs
… auto-install test

No-Verification-Needed: docs and test fixture only
Comment thread src/install/PackageManager/PackageManagerDirectories.rs
@Jarred-Sumner

Copy link
Copy Markdown
Collaborator Author

File names are too long this way. We are not added _integrity as a suffix. And do a benchmark in the relaese buidl.

…ory, bump cache layout to @@@2

npm cache entries are now `<name>@<version>@@@2.<13 chars>`: the 13
characters are the first 64 bits, in lowercase base32, of SHA-256 over a
random 16-byte key stored in `<cache>/.id`, the hash algorithm and the
digest. The key is created once per cache directory (O_EXCL, 0600) and
read when the cache directory is opened. Entries without an integrity
value, and entries written with --no-verify, use `<name>@<version>@@@2`.
The fingerprint is omitted when the folder name would exceed NAME_MAX.
Download task ids hash the algorithm and digest directly.

The cache layout version moves from 1 to 2 so entries written by earlier
versions are uniformly left unused.
@Jarred-Sumner

Copy link
Copy Markdown
Collaborator Author

Reworked the name: no label any more. An entry is now

lodash@4.17.21@@@2.0gwamgzgacdck

i.e. the existing name plus . and 13 lowercase base32 characters (the same alphabet bundler chunk hashes use, so nothing case-sensitive and no =/+//). The 13 characters are the first 64 bits of SHA-256 over a random 16-byte key kept in <cache dir>/.id, the hash algorithm and the digest; the key is read once per process when the cache directory is opened, so nothing is added per package. Packages without an integrity value (and anything installed with --no-verify) keep the plain name@version@@@2 form, and the cache layout version moves from @@@1 to @@@2 so old entries are uniformly retired instead of half-trusted. If 14 extra characters is still more than you want, the fallback I'd suggest is folding it into the version slot (@@@2@@@2<8 chars>), but that drops the fingerprint to 40 bits.

Release-build numbers (this branch built with bun run build:release, compared with the installed 1.4.0-canary.1) are in the description now. Short version: syscalls per install are the same or marginally fewer (2014–2017 vs 2024–2027 with node_modules removed on a 68-package project, 618 vs 640 for a no-op install; the install-path syscalls are identical in count), and hyperfine on a 552-package project is within noise in both orders (≈137–142 ms either binary with node_modules removed, ≈20–24 ms no-op).

@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: 5

🤖 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 `@docs/pm/cli/install.mdx`:
- Line 357: Update the npm cache path description in the documentation to
include the required “@@@2” layout marker after ${name}@${version}, followed by
the optional .<fingerprint> suffix. Preserve the existing explanation of
integrity fingerprints and semver build/pre tags.

In `@docs/pm/global-store.mdx`:
- Line 89: Update the package cache tree entry in the documentation to remove
the stale “unchanged” wording, replacing it with the accurate “Package cache”
comment while preserving the @@@2.<fingerprint> layout.

In `@src/install/PackageManager/PackageManagerDirectories.rs`:
- Around line 704-717: Update the suffix-length handling around the integrity
fingerprint and patch hash writes so the final path component never exceeds
NAME_MAX. In the flow containing put_fingerprint and put_patch_hash,
independently verify that the component remains within 255 bytes before calling
put_patch_hash, or calculate the bound from the suffixes that will actually be
emitted; preserve the existing behavior of omitting only suffixes that cannot
fit.

In `@test/harness.ts`:
- Around line 2059-2069: Update the harness helper to import createHash and
randomBytes from node:crypto at module scope, then replace the inline
require("node:crypto") calls with those imported symbols. Keep the existing ID
generation and digest computation behavior unchanged.
- Around line 2049-2077: Update installCacheFolderName so it omits the 14-byte
fingerprint when the component name length plus the reserved 42-byte suffix
exceeds NAME_MAX (255), matching production behavior; otherwise retain the
existing fingerprint generation. Add a boundary assertion using a valid
integrity value to verify the fallback at the maximum-length condition.
🪄 Autofix

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: e1483bd8-0a83-42cb-b868-5841659f765a

📥 Commits

Reviewing files that changed from the base of the PR and between 5e15624 and 2f15886.

📒 Files selected for processing (15)
  • docs/pm/cli/install.mdx
  • docs/pm/global-cache.mdx
  • docs/pm/global-store.mdx
  • src/install/PackageInstaller.rs
  • src/install/PackageManager.rs
  • src/install/PackageManager/PackageManagerDirectories.rs
  • src/install/PackageManager/PackageManagerLifecycle.rs
  • src/install/PackageManagerTask.rs
  • src/install/integrity.rs
  • src/install/isolated_install.rs
  • test/cli/install/bun-add.test.ts
  • test/cli/install/bun-install-registry.test.ts
  • test/cli/install/bun-install.test.ts
  • test/cli/install/bun-run-dir.test.ts
  • test/harness.ts

Comment thread docs/pm/cli/install.mdx Outdated
Comment thread docs/pm/global-store.mdx Outdated
Comment thread src/install/PackageManager/PackageManagerDirectories.rs
Comment thread test/harness.ts
Comment thread test/harness.ts Outdated
Comment thread src/install/PackageManager/PackageManagerDirectories.rs Outdated
…AX rule and imports node:crypto at module scope

No-Verification-Needed: docs and test helper only

@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: 2

Caution

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

⚠️ Outside diff range comments (2)
test/harness.ts (2)

2056-2069: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Do not hash a corrupt cache-directory ID.

Line 2060 throws "short" inside the outer catch. If .id already exists with the wrong length, the wx write fails and Line 2067 reads the same invalid file without revalidating it. The helper then derives a fingerprint from malformed state. The inner catch also treats every write error as a concurrent creation and discards the original I/O error. Catch only ENOENT and EEXIST, validate the final buffer, and preserve other errors with the cache ID path.

Validate the final ID and preserve unexpected errors
   try {
     id = fs.readFileSync(idPath);
-    if (id.length !== 16) throw new Error("short");
-  } catch {
+  } catch (error) {
+    if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error;
     fs.mkdirSync(cacheDir, { recursive: true });
     id = randomBytes(16);
     try {
       fs.writeFileSync(idPath, id, { flag: "wx", mode: 0o600 });
-    } catch {
+    } catch (error) {
+      if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error;
       id = fs.readFileSync(idPath);
     }
   }
+  if (id.length !== 16) {
+    throw new Error(`Invalid cache directory ID at ${idPath}: expected 16 bytes`);
+  }

As per coding guidelines, propagate actual I/O errors, preserve original errors, and do not swallow failures.

🤖 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/harness.ts` around lines 2056 - 2069, Update the cache ID loading logic
around idPath to catch only ENOENT when the initial read is missing, and treat
the write fallback as concurrent creation only for EEXIST; propagate all other
I/O errors with idPath context. After either reading or generating the ID,
revalidate that the final Buffer is exactly 16 bytes before fingerprinting,
including after the EEXIST read, and reject corrupt existing files rather than
hashing them.

Source: Coding guidelines


2072-2081: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Validate the integrity digest before computing the fingerprint. Canonical digests match production, but inputs such as sha256-AAAA and sha256-A=== are accepted and produce different fingerprints or fingerprints that production would reject. Require strict base64 decoding and an algorithm-specific digest length before hashing.

🤖 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/harness.ts` around lines 2072 - 2081, Update the fingerprint computation
around the digest hashing to strictly decode the input digest and validate it
against the expected length for its algorithm before calling createHash. Reject
malformed base64, including invalid padding, and unsupported digest lengths such
as sha256-AAAA or sha256-A===, preserving production’s validation behavior
before generating folder.
🤖 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 `@docs/pm/cli/install.mdx`:
- Line 357: Update the package-installation path documentation to state that the
fingerprint suffix is appended only when verification is enabled and the
complete component fits the 255-byte limit; otherwise, including with
--no-verify or an overlong component, the path ends at @@@2.

In `@test/harness.ts`:
- Around line 2051-2055: Update the integrity handling around the visible
validation and fingerprint derivation logic to distinguish the existing
absent-integrity sentinel from malformed non-empty values. Reject malformed
algorithm/base64 strings and validate that the decoded digest length matches the
selected algorithm before deriving or returning a fingerprint; do not fall back
to the unsuffixed folder for invalid input, while preserving the current
no-integrity behavior.

---

Outside diff comments:
In `@test/harness.ts`:
- Around line 2056-2069: Update the cache ID loading logic around idPath to
catch only ENOENT when the initial read is missing, and treat the write fallback
as concurrent creation only for EEXIST; propagate all other I/O errors with
idPath context. After either reading or generating the ID, revalidate that the
final Buffer is exactly 16 bytes before fingerprinting, including after the
EEXIST read, and reject corrupt existing files rather than hashing them.
- Around line 2072-2081: Update the fingerprint computation around the digest
hashing to strictly decode the input digest and validate it against the expected
length for its algorithm before calling createHash. Reject malformed base64,
including invalid padding, and unsupported digest lengths such as sha256-AAAA or
sha256-A===, preserving production’s validation behavior before generating
folder.
🪄 Autofix

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: 45518ac1-394b-42fc-a202-a656b3f69b04

📥 Commits

Reviewing files that changed from the base of the PR and between 2f15886 and a485567.

📒 Files selected for processing (3)
  • docs/pm/cli/install.mdx
  • docs/pm/global-store.mdx
  • test/harness.ts

Comment thread docs/pm/cli/install.mdx Outdated
Comment thread test/harness.ts Outdated
Comment thread src/install/PackageManager/PackageManagerDirectories.rs Outdated
…an unreadable .id, harness/doc follow-ups

- read/create `.id` next to where the cache directory handle is stored and
  list `cache_directory_id` in the raw accessor's contract
- create `.id` 0644; an `.id` that cannot be read or created falls back to a
  fixed key instead of a per-process one; an empty file (being written by a
  concurrent install) is re-read rather than replaced
- docs: say when the fingerprint is present
- test harness: reject malformed integrity strings in installCacheFolderName

@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: 2

🤖 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 381-415: Update read_or_create_cache_directory_id so an empty or
interrupted .id file is completed or replaced safely after retries, using atomic
publication or coordination that does not delete another process’s live file;
never fall back to a new unpersisted ID when the file remains recoverable. Add a
regression test that starts with an empty .id and verifies subsequent managers
converge on the same persisted cache directory ID.

In `@test/harness.ts`:
- Around line 2055-2056: Update the integrity validation in
installCacheFolderName to distinguish an invalid integrity format from a decoded
digest with an unsupported length. For format failures, report the rejected
integrity value and expected format; for length failures, report the actual and
required byte lengths, while preserving rejection of both cases.
🪄 Autofix

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: aaca604b-6a4a-4f8c-9ad0-500daee2fa65

📥 Commits

Reviewing files that changed from the base of the PR and between a485567 and 89a7a79.

📒 Files selected for processing (3)
  • docs/pm/cli/install.mdx
  • src/install/PackageManager/PackageManagerDirectories.rs
  • test/harness.ts

Comment thread src/install/PackageManager/PackageManagerDirectories.rs Outdated
Comment thread test/harness.ts Outdated
Comment thread src/install/PackageManager/PackageManagerDirectories.rs Outdated
Comment thread src/install/PackageManager/PackageManagerDirectories.rs Outdated
…sh key in place; clearer harness helper errors
Comment thread src/install/PackageManager/PackageManagerDirectories.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

🤖 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 395-408: Update the cache initialization path around the `.id`
repair branch to propagate failures from `File::openat` or `write_all` instead
of breaking and returning a random `CacheDirId`. Add error context identifying
the `.id` resource and failed repair operation, and add a regression test that
forces the repair write to fail and verifies initialization returns the error.
🪄 Autofix

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: 06918283-29a6-4d29-9033-6da31b524ca0

📥 Commits

Reviewing files that changed from the base of the PR and between 89a7a79 and d85458d.

📒 Files selected for processing (2)
  • src/install/PackageManager/PackageManagerDirectories.rs
  • test/harness.ts

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.

I reviewed this revision and didn't find any new issues. My earlier inline comment on PackageManagerDirectories.rs:415 was posted against a stale snapshot — at HEAD the Ok(0) if attempt < 2 guard already falls through to the in-place O_WRONLY|O_TRUNC write on the last attempt, so a persistently empty .id is repaired; that thread can be resolved. Given the scope (cache-layout version bump, integrity-keyed entry names, Wyhash→SHA-256 for persisted git/tarball names, .id create/recover concurrency), a human look is still worthwhile.

Also checked: the version-index symlink under <cache>/<name>/… is derived from the fingerprinted folder name in extract_tarball.rs, and path_for_cached_npm_path reads it back with the lockfile integrity threaded through, so cache-hit lookups line up; PackageManagerResolution.rs passes Integrity::default() for the offline disk-cache probe, which now maps to the digest-less @@@2 name — that path never had an integrity to key on, so the miss just falls through to a fetch as before.

Extended reasoning...

Overview

This PR reworks how bun install names npm cache entries: it bumps the cache layout version from @@@1 to @@@2, appends a 13-char base32 fingerprint of SHA-256(cache-dir-id ‖ tag ‖ digest) to npm entry folder names when integrity is available and verification is on, introduces a per-cache-directory 16-byte random key stored in <cache>/.id, switches git-clone and tarball cache folder names from Wyhash to a SHA-256 prefix, and folds the integrity value into in-flight download task IDs. It touches 21 files across the install core (PackageManager*.rs, integrity.rs, extract_tarball.rs, isolated_install*.rs, patch_install.rs), three docs pages, four install test suites, and adds an installCacheFolderName helper to test/harness.ts.

Security risks

The change is security-adjacent by intent: it hardens the cache against reuse of an entry that was extracted for a different integrity digest. The new surface is the .id file (16 random bytes, mode 0600, O_EXCL-created) and a keyed SHA-256 fingerprint. The key is a collision-space salt, not a secret whose disclosure grants anything; an attacker who can write the cache directory can already replace entries directly. The Wyhash→SHA-256 change for persisted git/tarball names is a strict improvement. I don't see new injection or path-traversal surface — the fingerprint is fixed-width base32 into a bounded ByteCursor, and the NAME_MAX gate reserves room for the patch-hash suffix.

Level of scrutiny

High. This is a design-bearing change to the package manager's on-disk cache format with a one-way version bump (every existing entry re-fetched once), threads integrity through ~10 call sites, adds a small concurrent-create/recover state machine for .id, and changes filesystem-persisted hash derivation. The PR has already gone through five follow-up commits addressing review feedback on the .id concurrency edge cases (unlink race, empty/truncated recovery, mode, fallback semantics), which is exactly the kind of thing that benefits from a maintainer sign-off on the final shape.

Other factors

All prior inline findings (mine and CodeRabbit's) are addressed at HEAD; my last inline comment on line 415 quotes pre-d85458d code and is stale. Test coverage is thorough for the new behavior (hoisted + isolated linkers, auto-install, git-clone name derivation, harness helper mirrors the Rust encoding including the NAME_MAX rule). The debug_assert!(this.cache_directory.is_some()) before reading cache_directory_id and the reordered get_cache_directory-before-cached_npm_package_folder_name calls at every site keep the id initialized before it's read. Nothing new to flag from this run.

Comment thread src/install/PackageManager/PackageManagerDirectories.rs
Comment thread src/install/PackageManager/PackageManagerDirectories.rs Outdated
…ns.log_level in the accessor contract

No-Verification-Needed: message text and doc comment only

@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.

I reviewed this PR again after ea44477 and found no issues — both prior nits (the "saving" → "accessing" warning wording and adding options.log_level to the # Safety projection list) are addressed. Given the scope — a cache-layout version bump that keys npm entries by integrity digest, a new <cache>/.id key file with concurrent-creation/repair handling, and Integrity threaded through the hoisted/isolated/patch/auto-install paths — a human look is still worthwhile.

What was reviewed:

  • read_or_create_cache_directory_id: bounded re-read of an empty .id, in-place repair of truncated files, per-process fallback with a warn on unrecoverable errors — all exit paths look consistent now.
  • cached_npm_package_folder_name_print: fingerprint gated on VERIFY_INTEGRITY and NAME_MAX; ordering vs. get_cache_directory fixed at every call site so cache_directory_id is populated first.
  • Task::Id::for_npm_package / for_git_clone: integrity now folded into the dedupe key; SHA-256 prefix replaces Wyhash for persisted names.
  • Test harness installCacheFolderName mirrors the Rust derivation (tag byte, base32 alphabet, NAME_MAX rule, .id create mode 0600).
Extended reasoning...

Overview

This PR changes how bun install names entries in the global package cache: npm packages now get a 13-char base32 fingerprint of SHA-256(<cache-dir key> ‖ <hash algorithm> ‖ <digest>) appended to their folder name, so two lockfiles pinning the same name@version to different integrity digests get separate cache entries. The cache-layout version is bumped @@@1@@@2, and URL-derived names (remote/local tarballs, bare git clones) switch from Wyhash to a SHA-256 prefix. A 16-byte random key stored in <cache>/.id salts the fingerprint so collisions cannot be precomputed off-machine. Integrity is threaded through ~10 call sites (PackageInstaller, PackageManagerLifecycle, PackageManagerEnqueue, isolated_install, patch_install, extract_tarball, PackageManagerResolution, patchPackage) plus the download-task dedupe key. Docs and ~15 existing tests are updated for the new names; new tests cover the "different integrity → separate entry" property for hoisted, isolated, and auto-install, plus a git-clone-name test.

Security risks

The change is itself a security hardening (an entry extracted for one digest is never reused by an install pinning a different digest). The .id key is not a secret in the confidentiality sense — it only makes the 64-bit fingerprint space unpredictable — so the 0600 mode and per-process-random fallback are appropriate. I did not find any path where an unverified entry could be adopted under a fingerprinted name: --no-verify and missing-integrity packages use the digest-less name, and a later verifying install fetches its own copy. The SHA-256 prefix for git-clone/tarball names is not integrity-checked (same as before), but is now collision-resistant against a chosen-URL attacker.

Level of scrutiny

High. This is production-critical package-manager code with a persisted on-disk format change, concurrent file-creation logic, and a design decision (keyed 64-bit fingerprint vs. full digest, per-process fallback vs. hard error, cache-version bump invalidating all existing entries) that a maintainer should sign off on. The PR description explicitly flags the air-gapped/baked-cache consequence as "easy to reverse if preferred".

Other factors

The PR has been through several review rounds; every prior finding (mine and CodeRabbit's) is marked resolved and the fixes are visible in the current diff. Test coverage is thorough (both linkers, auto-install, git, and the harness helper mirrors the Rust derivation byte-for-byte). The remaining reason to defer is purely scope and design sign-off, not any open concern about correctness.

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator Author

Closing after thinking this through; writing down the reasoning so it isn't re-derived later.

What the change actually enforces is "a cache hit implies the entry was extracted from bytes that verified against the digest this lockfile pins". That's a real property, but look at where it matters:

  • On the default registry (did_override_default_scope is literally "URL != registry.npmjs.org"), name@version → tarball is immutable — you cannot publish over a version and unpublish doesn't reopen it — so the digest is already a function of the folder name. Appending it adds no information there, and the default registry is nearly all of the cache. What it would still do on npmjs is split --no-verify entries and sha1-vs-sha512 manifests of the same bytes into separate entries, which is a cost, not a protection.
  • Non-default registries can genuinely republish a version, so a suffix there is a correctness fix — but by the same logic the entries whose content most obviously drifts under a fixed key are @T@<url-hash> tarballs (the lockfile carries their integrity too), and this PR doesn't touch those. Suffixing some of the ways an entry can diverge from the lockfile and not others isn't a model, it's a subset. The actual model of the cache is: keyed by resolution identity, bytes verified once at fetch, the directory itself is the trust boundary. Changing that means verifying on hit, which is a different (much slower) cache and nobody is asking for it.
  • The security framing doesn't hold for the shared-cache case: anyone who can get a hostile lockfile installed against a cache dir has code execution or write access to it, <cache>/.id sits in the same directory, and they'd just write the correctly-named entry directly. It only helps against remote/registry-level divergence, where an unkeyed hash of the digest would do the same job — so .id (the extra file, the create/read/retry state machine, the warn path, and "restored cache without .id silently refetches everything") is machinery without a threat behind it.
  • Cost side: @@@1 → @@@2 invalidates every existing cache and every baked CI image / air-gapped setup once, and makes cache paths non-derivable from name@version (.2.0gwamgzgacdck) for people who look at them.

The one piece with an independent argument is wyhash → SHA-256 for @T@ / bare-clone names (a dependency-supplied URL choosing its own cache folder name is cheap to collide with wyhash); if anyone wants that it's a self-contained two-line change and doesn't need this PR. The related robustness work that did survive is in #38269: git cache folders are now built in a staging dir and renamed on success, and cache hits for git checkouts require the .bun-tag written last (npm: package.json) — the "don't trust a half-built entry" property this PR was partly reaching for, without changing the naming.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants