install: compare trusted dependency names, not just truncated hashes#31218
Conversation
|
Updated 1:13 AM PT - May 23rd, 2026
❌ @Jarred-Sumner, your commit a5bff0a has 1 failures in 🧪 To try this PR locally: bunx bun-pr 31218That installs a local version of the PR into your bun-31218 --bun |
|
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:
WalkthroughThis PR makes trusted dependency tracking collision-safe by storing exact alias bytes instead of only hash presence, preventing unrelated packages from gaining trust through truncated-hash collisions. The change propagates through discovery APIs, validation checkpoints, diff tracking, and lockfile persistence. ChangesHash-collision-safe trusted dependency tracking
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/lockfile/Package.rs (1)
1297-1319:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat default trusted dependencies as hash-only in the explicit→default path.
Line 1298 and Line 1315 only check the truncated hash, so a user-added trusted dependency that collides with a default entry is treated as if it were the default package. In that transition the old custom entry is never removed, which can leave a package trusted after it has been deleted from
trustedDependencies.🩹 Suggested direction
for entry in default_trusted_dependencies::entries() { - if !from_trusted_dependencies - .contains(&(entry.hash as TruncatedPackageNameHash)) - { + let hash = entry.hash as TruncatedPackageNameHash; + let same_entry = from_trusted_dependencies + .get(&hash) + .is_some_and(|from_name| { + from_name.is_empty() || &**from_name == entry.key + }); + if !same_entry { summary.added_trusted_dependencies.put( - entry.hash as TruncatedPackageNameHash, + hash, AddedTrustedDependency { add_to_lockfile: false, name: Box::from(entry.key), }, )?; } } // removed for (&from_trusted, from_name) in from_trusted_dependencies.iter() { - if !default_trusted_dependencies::has_with_hash(u64::from(from_trusted)) { + let is_default = default_trusted_dependencies::entries().iter().any(|entry| { + entry.hash as TruncatedPackageNameHash == from_trusted + && entry.key == &*from_name + }); + if !is_default { summary .removed_trusted_dependencies .put(from_trusted, from_name.clone())?; } }🤖 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/lockfile/Package.rs` around lines 1297 - 1319, The code is only comparing truncated hashes (TruncatedPackageNameHash) against default_trusted_dependencies, causing user-added entries that collide on truncated hash with a default to be treated as defaults and never removed; update the logic in the loops around default_trusted_dependencies::entries(), from_trusted_dependencies and default_trusted_dependencies::has_with_hash so you verify full identity (compare the default entry.key/name against the stored from_name) rather than only the truncated hash: when iterating default_trusted_dependencies::entries() check if a matching truncated hash exists in from_trusted_dependencies and also that the names match before skipping add_to_lockfile, and when computing removals use a lookup that confirms the default entry name differs (or use the full package identifier instead of truncated hash) so user-added trusted dependencies that collide on hash are still removed and recorded via summary.removed_trusted_dependencies.put.
🤖 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/Installer.rs`:
- Around line 111-116: The current trusted_dependencies_from_update_requests map
uses ArrayHashMap<TruncatedPackageNameHash, Box<[u8]>> which overwrites one
alias when two different exact names share the same truncated hash; change the
value to a collection of exact names (e.g., Vec<Box<[u8]>> or
HashSet<Box<[u8]>>) keyed by TruncatedPackageNameHash so inserts append rather
than replace, update all code that inserts into
trusted_dependencies_from_update_requests to push/insert into the collection,
and update lookup logic in RunPreinstall (and any place that writes back to
bun.lock / package.json) to iterate the stored exact names and perform the
byte-equality check against each stored alias before trusting or writing names
back.
In `@src/install/lockfile.rs`:
- Around line 120-121: The current TrustedDependenciesSet type stores a single
Box<[u8]> per TruncatedPackageNameHash which silently drops legitimate
trusted-name collisions — update the representation so each truncated hash can
map to multiple names (e.g., change the value from Box<[u8]> to a collection
type such as Vec<Box<[u8]>> or a small linked bucket/list) or switch to
full-name keys instead of truncated hashes; adjust all use sites that insert,
iterate, and lookup in TrustedDependenciesSet (and any helpers operating on
TruncatedPackageNameHash / ArrayHashMap) to handle multiple entries per hash and
perform exact byte-match checks against the per-hash collection when validating
trust.
In `@src/install/lockfile/bun.lock.rs`:
- Around line 1560-1571: The current logic inserts a single name per
TruncatedPackageNameHash into trusted_dependencies, which allows later inserts
to overwrite earlier ones on hash collisions; change the map value to a
collision bucket (e.g., HashMap<TruncatedPackageNameHash, Vec<Box<str>>> or
similar) and instead of trusted_dependencies.insert(...) push the name into the
vector bucket for both the s.is_utf8() and non-UTF8 branches; also update any
lookup code that checks trustedDependencies to iterate the bucket and perform
exact-name comparison (using the stored Box<str> names) so multiple distinct
names with the same truncated hash are all honored.
In `@src/install/lockfile/Package.rs`:
- Around line 1267-1285: When diffing explicit trusted-dependency sets, you
currently only test membership by hash (to_trusted_dependencies.contains(&hash)
/ from_trusted_dependencies.contains(&hash)), which misses replacements where a
different stored name collides to the same hash; change the membership check to
compare the stored name bytes as well (i.e., treat a match only if the
stored-name bytes are equal OR if the stored-name is the legacy empty-name
sentinel, treat that as a hash-only match for bun.lockb compatibility). Update
the loops that call summary.added_trusted_dependencies.put(...,
AddedTrustedDependency { ... }) and
summary.removed_trusted_dependencies.put(...) to use this name-aware comparison
so a replaced trusted package is emitted as a remove + add rather than ignored.
---
Outside diff comments:
In `@src/install/lockfile/Package.rs`:
- Around line 1297-1319: The code is only comparing truncated hashes
(TruncatedPackageNameHash) against default_trusted_dependencies, causing
user-added entries that collide on truncated hash with a default to be treated
as defaults and never removed; update the logic in the loops around
default_trusted_dependencies::entries(), from_trusted_dependencies and
default_trusted_dependencies::has_with_hash so you verify full identity (compare
the default entry.key/name against the stored from_name) rather than only the
truncated hash: when iterating default_trusted_dependencies::entries() check if
a matching truncated hash exists in from_trusted_dependencies and also that the
names match before skipping add_to_lockfile, and when computing removals use a
lookup that confirms the default entry name differs (or use the full package
identifier instead of truncated hash) so user-added trusted dependencies that
collide on hash are still removed and recorded via
summary.removed_trusted_dependencies.put.
🪄 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: f268e24d-4c25-401e-8731-9df8d0034640
📒 Files selected for processing (10)
src/install/PackageInstaller.rssrc/install/PackageManager/PackageManagerLifecycle.rssrc/install/isolated_install.rssrc/install/isolated_install/Installer.rssrc/install/lockfile.rssrc/install/lockfile/Package.rssrc/install/lockfile/bun.lock.rssrc/install/lockfile/bun.lockb.rssrc/runtime/cli/pm_trusted_command.rstest/cli/install/bun-install-lifecycle-scripts.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/lockfile/Package.rs (1)
1309-1337:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the explicit→default trusted-dependency diff name-aware too.
Line 1316 and Line 1333 still decide membership by truncated hash alone. If an explicit trusted dependency collides with a default trusted dependency, removing
trustedDependenciesfrompackage.jsoncan emit neither the removal of the old name nor the addition of the default one. That leaves the old colliding name looking unchanged to downstream trust/lockfile update logic.Suggested direction
if let (Some(from_trusted_dependencies), None) = ( from_lockfile.trusted_dependencies.as_ref(), to_lockfile.trusted_dependencies.as_ref(), ) { // added for entry in default_trusted_dependencies::entries() { - if !from_trusted_dependencies - .contains(&(entry.hash as TruncatedPackageNameHash)) - { + let hash = entry.hash as TruncatedPackageNameHash; + let already_trusted = from_trusted_dependencies + .get(&hash) + .is_some_and(|from_name| { + from_name.is_empty() || &**from_name == entry.key + }); + if !already_trusted { // although this is a new trusted dependency, it is from the default // list so it shouldn't be added to the lockfile summary.added_trusted_dependencies.put( - entry.hash as TruncatedPackageNameHash, + hash, AddedTrustedDependency { add_to_lockfile: false, name: Box::from(entry.key), }, )?; } } // removed for (&from_trusted, from_name) in from_trusted_dependencies.iter() { - if !default_trusted_dependencies::has_with_hash(u64::from(from_trusted)) { + let still_trusted = default_trusted_dependencies::entries().iter().any(|entry| { + entry.hash as TruncatedPackageNameHash == from_trusted + && (from_name.is_empty() || &**from_name == entry.key) + }); + if !still_trusted { summary .removed_trusted_dependencies .put(from_trusted, from_name.clone())?; } }🤖 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/lockfile/Package.rs` around lines 1309 - 1337, The current diff only compares truncated hashes (from_trusted_dependencies and default_trusted_dependencies::has_with_hash) and misses cases where an explicit trusted dependency hash collides with a default entry but has a different name; update both the "added" and "removed" branches to be name-aware: when iterating default_trusted_dependencies::entries() in the added branch, check whether from_trusted_dependencies contains an entry with the same hash AND the same name (compare entry.key) before creating AddedTrustedDependency; likewise, when iterating from_trusted_dependencies in the removed branch, if default_trusted_dependencies contains the same hash but a different name, treat it as a rename: put the old name into summary.removed_trusted_dependencies and also ensure you add the default name into summary.added_trusted_dependencies with add_to_lockfile = false (or if the hash is absent entirely, behave as before); use default_trusted_dependencies::entries() or a lookup by hash to get the default name for these comparisons and updates (affecting summary.added_trusted_dependencies.put, summary.removed_trusted_dependencies.put, and the logic around default_trusted_dependencies::has_with_hash).
🤖 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/lockfile/Package.rs`:
- Around line 1309-1337: The current diff only compares truncated hashes
(from_trusted_dependencies and default_trusted_dependencies::has_with_hash) and
misses cases where an explicit trusted dependency hash collides with a default
entry but has a different name; update both the "added" and "removed" branches
to be name-aware: when iterating default_trusted_dependencies::entries() in the
added branch, check whether from_trusted_dependencies contains an entry with the
same hash AND the same name (compare entry.key) before creating
AddedTrustedDependency; likewise, when iterating from_trusted_dependencies in
the removed branch, if default_trusted_dependencies contains the same hash but a
different name, treat it as a rename: put the old name into
summary.removed_trusted_dependencies and also ensure you add the default name
into summary.added_trusted_dependencies with add_to_lockfile = false (or if the
hash is absent entirely, behave as before); use
default_trusted_dependencies::entries() or a lookup by hash to get the default
name for these comparisons and updates (affecting
summary.added_trusted_dependencies.put,
summary.removed_trusted_dependencies.put, and the logic around
default_trusted_dependencies::has_with_hash).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f7a59d89-a1ad-4e62-8b8c-5cc4cc87c9ba
📒 Files selected for processing (1)
src/install/lockfile/Package.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this hardens the lifecycle-script trust gate (what decides whether postinstall scripts run) and threads a new name-equality invariant through both installers, both lockfile formats, and the diff path — worth a human pass, especially with CI currently red on the build-rust jobs.
Extended reasoning...
Overview
This PR closes a fail-open hash-collision in trustedDependencies / --trust: membership was previously decided by a truncated 32-bit name hash alone, so a crafted package name colliding with a trusted entry would have its lifecycle scripts run. The fix changes TrustedDependenciesSet and the --trust subtree map from ArrayHashMap<TruncatedPackageNameHash, ()> to ArrayHashMap<TruncatedPackageNameHash, Box<[u8]>> and requires the stored name bytes to match at every lookup. It touches the hoisted installer (PackageInstaller.rs), isolated installer (isolated_install.rs, isolated_install/Installer.rs), lockfile text parser/stringifier (bun.lock.rs), binary lockfile loader (bun.lockb.rs), diff summary (lockfile/Package.rs), has_trusted_dependency (lockfile.rs), and bun pm trust (pm_trusted_command.rs). Legacy bun.lockb entries get an empty-bytes sentinel that keeps hash-only matching for back-compat. Two regression tests use precomputed wyhash-truncation collisions to verify the colliding package's scripts stay blocked.
Security risks
This is the gate that decides whether arbitrary install-time scripts execute, so it is directly security-sensitive. The change moves the failure mode from fail-open to fail-closed, which is the right direction. The residual risks a reviewer should sanity-check are: (1) every lookup site now does the byte-equality check (I see it applied in has_trusted_dependency, both installers' --trust checks, the global-store eligibility check, the diff, and the bun.lock stringifier — looks complete); (2) the empty-sentinel back-compat path in has_trusted_dependency and bun.lock.rs only ever matches entries produced by the binary-lockfile loader; and (3) the bun.lock.rs parser's new UTF-8/UTF-16 branch agrees with the hash that was previously computed via as_string_hash_utf8.
Level of scrutiny
High. This is a deliberate security boundary change in production install code, threading a new invariant through ~10 files and two lockfile formats. It's well-scoped and well-tested, but it is exactly the category (security-sensitive, multi-site invariant) where a second pair of human eyes is appropriate rather than bot-only approval.
Other factors
- CodeRabbit's four concerns were all about the pre-existing single-slot-per-hash layout; Jarred explained the fail-closed tradeoff and they're resolved. The diff-comparison concern was additionally addressed in bb7db2f.
- My earlier inline note on
lockfile.rs:3374flags a pre-existing unsynchronized read in the isolated installer'sTask::run; it's not introduced by this PR and is marked as a separate follow-up. - CI (
Build #56957) is currently red on severalbuild-rust/build-cppjobs for bb7db2f — likely infra/toolchain (the failure is a-no-pieclang warning and genericci.tsbuild failures rather than test failures), but worth confirming green before merge. - The bug-hunting system found no issues.
|
🔧 Status — both review items are handled:
CI on a5bff0a (#57123): every test lane that has run is green — the earlier |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Delete your slop comments.
Store the name each trusted-dependency entry was registered under and require it to match at lookup time, instead of relying on the truncated 32-bit name hash alone. Applies to trustedDependencies from package.json and bun.lock, the --trust subtree set, and the install diff summary. Entries loaded from a legacy binary lockfile have no stored name and keep matching by hash, preserving existing behavior for those projects.
Jarred-Sumner
force-pushed
the
claude/hardening-fix-lockfile-rs-3359-27
branch
from
613d511 to
a5bff0a
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/shell/states/Expansion.rs:390— Wrapping every interpolated!as{!}means N consecutive!bytes produce N sequential brace groups, but the glob matcher'sBraceStackisBoundedArray<Brace, 10>and sequential groups consume one slot each (push happens before the recursive call returns), so the 11th{!}overflows the stack and the whole pattern returns no-match. For something likeecho prefix${'!'.repeat(11)}*against a file namedprefix!!!!!!!!!!!xthis is a regression — pre-PR the non-leading!bytes matched literally. Sinceglob_match_implalready treats a non-leading!as a literal, only the first!(whenpattern.is_empty()) actually needs the{!}wrap; subsequent ones can be emitted bare.Extended reasoning...
What the bug is
neutralize_glob_metachars(Expansion.rs:390) wraps every non-meta!byte from interpolated/quoted data as a one-branch brace group{!}. The rationale in the comment is correct as far as it goes —[!]would parse as a negated class, and a bare leading!would flip the matcher's negation loop inr#match(matcher.rs:143-146). But it doesn't account for the matcher's brace-stack capacity: N sequential{!}groups in a single path component require brace-stack depth N, and the stack is capped at 10.The code path
In
glob_match_impl(matcher.rs:358-366), encountering a{whoseopen_brace_idxis not already onbrace_stackdoesreturn match_brace(...).match_bracefinds the branch and callsmatch_brace_branch(line 458).match_brace_branchthen:- Pushes a
Brace { open_brace_idx, branch_idx }ontobrace_stack(line 522), returningfalseif theBoundedArray<Brace, 10>push fails (lines 525-527). - Recursively calls
glob_match_impl(line 534). - Only pops at line 543, after the recursive call returns.
Inside that recursive
glob_match_impl, after matching the literal!andskip_branch()-ing past}, the next byte is the next sequential{, at a differentglob_index. It's not onbrace_stack, so we againreturn match_brace(...)→ another push. The pop for group k doesn't happen until group k+1's recursion has fully returned, so N sequential{!}groups require brace-stack depth N.BunGlobWalkerZ::init_with_cwd→build_pattern_componentsonly splits the pattern on path separators (no brace pre-expansion), so the whole{!}{!}…{!}*component reachesmatch_pattern_slow→crate::r#match(GlobWalker.rs:1755) intact.Step-by-step proof
Take
await Bun.$echo prefix${'!'.repeat(11)}*`` in a directory containingprefix!!!!!!!!!!!x:current_out=b"prefix!!!!!!!!!!!*";meta_offsetsrecords only the trailing literal*(offset 17).neutralize_glob_metacharsemitsprefix{!}{!}{!}{!}{!}{!}{!}{!}{!}{!}{!}*— eleven sequential one-branch groups.- The glob walker hands this single component to
r#match.glob_match_implmatchesprefixliterally, then hits the first{at glob_index 6 and recurses throughmatch_brace_branch, pushing brace_stack[0]. - After matching
!and skipping past}, glob_index is at the second{; recurse again, push brace_stack[1]. Repeat. - At the 11th
{,brace_stack.push(...)on theBoundedArray<Brace, 10>fails;match_brace_branchreturnsfalse(lines 525-527), which propagates up as a no-match for the whole pattern. - The shell prints
no matches found: prefix!!!!!!!!!!!*even thoughprefix!!!!!!!!!!!xexists.
Before this PR, step 2 didn't happen — the raw
!bytes reachedglob_match_impland, being non-leading, fell through the_ => break to_elsearm (matcher.rs:384) and matched literally. So for non-leading!runs this is a regression. (For leading!runs, e.g.${'!'.repeat(11)}*, pre-PR was also broken differently — the negation loop at matcher.rs:143 consumed them — so that exact case is not a regression, just differently wrong.)Why existing code doesn't prevent it
The matcher's
BRACE_BRANCH_BUDGET(10000) limits alternatives, not nesting depth, so it doesn't help here. The new test "injected leading ! does not negate" only uses a single!, so it doesn't exercise this. The doc comment's claim that the matcher "provably treats [the wrapped form] as that literal character" is correct for any individual group but doesn't hold once 11+ groups must be on the stack simultaneously.Impact and fix
Edge case (11+
!bytes from interpolation in a single path component adjacent to a literal glob), and the failure is benign fail-closed (no-match error showing the original word, no crash, no security regression). But it's a correctness flaw in newly-added neutralization code with a one-line fix: only wrap!whenpattern.is_empty(); otherwise emit it bare, sinceglob_match_implalready treats a non-index-0!as a literal character. Something like:b'!' if pattern.is_empty() => pattern.extend_from_slice(b"{!}"), b'!' => pattern.push(b'!'),
- Pushes a
There was a problem hiding this comment.
I didn't find any issues beyond what's already resolved, but since this hardens the lifecycle-script trust boundary across both installers and both lockfile formats (with the bun.lockb empty-name sentinel as a compat path), it's worth a human sign-off rather than a bot approval.
Extended reasoning...
Overview
This PR closes a fail-open in trusted-dependency checks: membership was previously decided by a truncated 32-bit name hash alone, so a crafted package name that collided with a trusted entry's hash would have its lifecycle scripts run. The fix changes TrustedDependenciesSet from ArrayHashMap<TruncatedPackageNameHash, ()> to ArrayHashMap<TruncatedPackageNameHash, Box<[u8]>> and requires the stored name bytes to match at every lookup. The change is threaded through PackageInstaller, the isolated installer, find_trusted_dependencies_from_update_requests, the diff summary (AddedTrustedDependency), bun.lock parse/stringify, bun.lockb load (empty-name sentinel for hash-only legacy entries), and bun pm trust. Two regression tests use precomputed wyhash-truncation collisions to verify the colliding package's scripts stay blocked and that only the intended name is persisted.
Security risks
The code being changed is the security boundary — it gates whether postinstall/preinstall scripts execute. The direction is strictly hardening (fail-open → fail-closed on hash collision). The bun.lockb sentinel keeps legacy hash-only matching for entries loaded from the binary format, which is intentional for compatibility but means projects still on bun.lockb don't get the hardening until they migrate. The remaining single-slot-per-hash limitation (two legitimately trusted colliding names → one fails closed) was discussed and accepted as pre-existing and ~2⁻³²-per-pair. I don't see a way for this change to grant trust where it wasn't granted before.
Level of scrutiny
High. This is production-critical install-path code that decides whether arbitrary package scripts run on a user's machine, touches 10 files across both linker modes and both lockfile formats, and introduces a sentinel value with special-case semantics in several places (name.is_empty() checks in has_trusted_dependency, the bun.lock stringifier, and the diff). The implementation looks correct and consistent, but the blast radius of a mistake here is "runs untrusted scripts" or "silently stops running trusted ones", so it deserves a human reviewer rather than bot approval.
Other factors
All four CodeRabbit threads and Jarred's own "dead arena" note are resolved (the bun.lock parser now transcodes straight into the owned Box<[u8]>, and the diff now compares stored names per 50684b3). My earlier note on the pre-existing unsynchronized trusted_dependencies read in the isolated installer is being handled in #31245. Robobun reported all executed CI lanes green on 7b0c89f (macOS lanes expired waiting for agents); there have been four small follow-up commits since. The bug-hunting system found nothing. Net: I have no blocking concerns, but I'm deferring per the security-sensitive-code guideline.
2 participants
Trusted-dependency membership checks (
trustedDependencies, the--trustsubtree, and the install diff summary) keyed only on a truncated 32-bit name hash. This change stores the exact name each entry was registered under as the map value and requires it to match at every lookup, mirroring what the default trusted-dependencies list already does. Entries loaded from a legacy binarybun.lockbhave no stored name and keep matching by hash alone, so existing projects are unaffected.Adds two tests to
bun-install-lifecycle-scripts.test.tsusing precomputed truncated-hash collisions: one for atrustedDependenciesentry whose hash collides with a different dependency's name, and one for a--trustinstall with a colliding sibling dependency. Both verify the colliding package's scripts stay blocked and that only the intended name is written back to package.json and bun.lock.