Skip to content

install: honor bun.lockb trustedDependencies sentinel in bun pm untrusted/trust - #34327

Open
robobun wants to merge 1 commit into
mainfrom
claude/farm-e38217c7-lockb-trusted-sentinel
Open

install: honor bun.lockb trustedDependencies sentinel in bun pm untrusted/trust#34327
robobun wants to merge 1 commit into
mainfrom
claude/farm-e38217c7-lockb-trusted-sentinel

Conversation

@robobun

@robobun robobun commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

What

In projects whose lockfile is the legacy binary bun.lockb, bun pm untrusted listed every package with lifecycle scripts as blocked even when it was in trustedDependencies and its postinstall had already run during install. bun pm trust <pkg> then re-executed the postinstall a second time. The same project with a text bun.lock correctly reported 0 untrusted.

Repro

D=$(mktemp -d); cd "$D"; mkdir -p dep
echo '{"name":"root","version":"1.0.0","dependencies":{"dep":"file:./dep"},"trustedDependencies":["dep"]}' > package.json
echo '{"name":"dep","version":"1.0.0","scripts":{"postinstall":"echo RAN >> ../../ran.txt"}}' > dep/package.json
printf '[install]\nsaveTextLockfile = false\n' > bunfig.toml
bun install --no-summary
bun pm untrusted
# before: ./node_modules/dep @dep  » [postinstall]: ...  "blocked during install"
# after:  Found 0 untrusted dependencies with scripts.

Cause

The binary lockfile format stores only truncated u32 name hashes for trustedDependencies, not the names. The loader (src/install/lockfile/bun.lockb.rs) inserts an empty Box<[u8]> as the documented "name unknown, hash-only match" sentinel (see the TrustedDependenciesSet doc comment).

Lockfile::has_trusted_dependency compared the stored name against the candidate with !name.is_empty() && **name == *trusted_name, so an empty stored name never matched. Every trustedDependencies entry loaded from a bun.lockb therefore failed. bun install was unaffected because it repopulates the map from package.json with real names before consulting it; bun pm untrusted / bun pm trust read the lockfile as loaded.

Regressed in #31339, which flipped the original name.is_empty() || **name == *alias check.

Fix

Restore the sentinel semantics in has_trusted_dependency: an empty stored name means "hash-only match" and accepts. Entries that do carry a name still compare it to guard against truncated-hash collisions.

Verification

New test in test/cli/install/bun-lockb.test.ts creates a project with a file: dependency that has a postinstall appending to a counter file, a trustedDependencies entry for it, and saveTextLockfile = false. After install it asserts bun pm untrusted reports 0 untrusted, bun pm trust dep refuses with "0 scripts ran" / "already trusted", and the postinstall ran exactly once. Fails on main at the Found 0 untrusted assertion, passes with this change.


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

…dependency

The binary lockfile format stores only truncated u32 name hashes for
trustedDependencies, not the names themselves. The loader inserts an
empty Box<[u8]> as the documented "name unknown, hash-only match"
sentinel (TrustedDependenciesSet doc comment and bun.lockb.rs loader).

has_trusted_dependency rejected any entry with an empty stored name,
so every trustedDependencies entry loaded from a bun.lockb failed to
match. `bun pm untrusted` then listed every script-bearing package as
blocked even though its postinstall had already run during install, and
`bun pm trust <pkg>` re-executed the lifecycle scripts a second time.
`bun install` itself was unaffected because it repopulates the map from
package.json with real names before consulting it.

Restore the sentinel semantics: an empty stored name is a hash-only
match (the 1.3.x behavior). Entries that do carry a name still compare
it to guard against truncated-hash collisions.
@robobun

robobun commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

Reproduced with the file: repro and also with a real npm package; bun pm trust re-ran the postinstall before this change and refuses ("already trusted") after. Test fails on main at the Found 0 untrusted dependencies assertion and passes on this branch.

CI (build 73744): the new bun-lockb.test.ts case passed on every lane that ran it. Remaining red is unrelated to this diff: test-worker-message-port-transfer-terminate.js (SIGABRT on x64-asan) and test-net-connect-memleak.js (alpine x64) both also fail on main, plus a Windows x64-baseline lane that never ran because Azure agent provisioning failed. Ready for review.

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6abdfd58-3bd6-4ba3-b987-0755ae60b34b

📥 Commits

Reviewing files that changed from the base of the PR and between 4bbe075 and fbf8912.

📒 Files selected for processing (2)
  • src/install/lockfile.rs
  • test/cli/install/bun-lockb.test.ts

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

@robobun

robobun commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 3:06 AM PT - Jul 16th, 2026

@robobun, your commit fbf8912 has 3 failures in Build #73744 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 34327

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

bun-34327 --bun

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

LGTM — one-line restoration of the documented bun.lockb empty-name sentinel semantics.

What was reviewed:

  • Confirmed the TrustedDependenciesSet doc comment (lockfile.rs:109-111) and the bun.lockb loader (bun.lockb.rs:536-540) both explicitly define empty as the "hash-only match" sentinel; the fix aligns has_trusted_dependency with that contract.
  • Checked all four other callers of has_trusted_dependency (install paths) — they repopulate from package.json first so behavior is unchanged there, matching the PR's claim.
  • Ruled out the sibling bun.lock.rs serializer comparison at line 521 — separate path, out of scope for this fix.
  • Test follows existing patterns in the file and asserts the postinstall ran exactly once.
Extended reasoning...

Overview

One-line change in src/install/lockfile.rs: has_trusted_dependency now treats an empty stored name in TrustedDependenciesSet as a match (name.is_empty() || **name == *trusted_name) instead of a non-match (!name.is_empty() && **name == *trusted_name). Adds a regression test in test/cli/install/bun-lockb.test.ts covering bun pm untrusted and bun pm trust against a binary lockfile with a trustedDependencies entry.

Security risks

trustedDependencies gates lifecycle-script execution, so widening a match here is nominally security-adjacent. However: (1) the empty sentinel is only ever inserted by the bun.lockb loader, which stores only truncated u32 hashes and physically cannot supply a name; (2) the type's own doc comment and the loader's inline comment both define empty as "hash-only match, accept"; (3) bun install already runs these scripts because it repopulates the map from package.json before consulting it — this change only fixes the reporting/re-run behavior of bun pm untrusted/trust; (4) the PR restores behavior that shipped before #31339 flipped the condition. No new attack surface is introduced.

Level of scrutiny

Low-medium. The change is a single boolean expression that brings a lookup into agreement with its type's documented contract, verified against two independent in-tree comments describing the sentinel. The test is well-targeted (asserts postinstall ran exactly once, untrusted reports 0, trust refuses with exit 1) and follows the file's established harness patterns (VerdaccioRegistry, stderrForInstall, Promise.all on pipes, exit code asserted last).

Other factors

No CODEOWNERS coverage on src/install/. No prior human review comments to address. The bug hunting system found no issues; the one candidate raised by finders (the bun.lock.rs serializer at line 521 not honoring the sentinel) was examined and is a separate serialization path outside this PR's scope.

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.

1 participant