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

install: keep cache .id handling out of the raw projection, tolerate …

8b34bc3
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Aug 12, 2026 in 17m 9s

Code review found 3 potential issues

Found 2 candidates, confirmed 3. See review comments for details.

Details

Severity Count
🔴 Important 0
🟡 Nit 3
🟣 Pre-existing 0
Severity File:Line Issue
🟡 Nit src/install/PackageManager/PackageManagerDirectories.rs:402-407 .id created with mode 0o644; PR description and harness say 0o600
🟡 Nit src/install/PackageManager/PackageManagerDirectories.rs:412-414 loop-exhaustion fallback returns random bytes, doc says zeroes

Annotations

Check warning on line 407 in src/install/PackageManager/PackageManagerDirectories.rs

See this annotation in the file changed.

@claude claude / Claude Code Review

.id created with mode 0o644; PR description and harness say 0o600

The `.id` file is opened with mode `0o644` here, but the PR description says "created (`O_EXCL`, mode 0600)" and the test-harness twin `installCacheFolderName` in test/harness.ts writes it with `{ flag: "wx", mode: 0o600 }`. The security impact is negligible (the id is only a keying salt), but the three sources should agree — either change this to `0o600` or update the description and harness.

Check warning on line 414 in src/install/PackageManager/PackageManagerDirectories.rs

See this annotation in the file changed.

@claude claude / Claude Code Review

loop-exhaustion fallback returns random bytes, doc says zeroes

The doc comment says "if the file cannot be read or created at all the key falls back to zeroes so names stay stable", and the `Err(_) => return [0; 16]` arms honour that — but the loop-exhaustion fallthrough here calls `rand_bytes(&mut id)` and returns fresh random bytes instead. Reachable via 3× `Ok(0) => continue` or 3× ENOENT-then-EEXIST races; two processes that both hit it get different ids and orphan each other's fingerprinted entries, which is what the zero fallback is meant to prevent.