install: reject bun.lockb packages whose resolution tag no writer produces - #39720
install: reject bun.lockb packages whose resolution tag no writer produces#39720robobun wants to merge 5 commits into
Conversation
The binary lockfile loader accepted resolution tag 0 (Uninitialized) as a valid tag. A package with that tag has no source to install from, but the dependencies that point at it still count as resolved. bun install then skipped the package, printed "Saved lockfile", exited 0, and wrote the unresolved package back out. A debug build hit the unreachable arm in Resolution::eql while cloning the package. A package is appended to a lockfile only once it is resolved, so no saved package carries this tag. Treat it like any other unknown tag byte: fail the load with "invalid resolution tag", so the install ignores the lockfile and resolves again. The format v2 migration arm that turned unknown tags into Uninitialized now returns the same error.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 10 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: fix pushed, waiting for CI. Head is 502b388. Reproduced on the released bun 1.4.0 with the local test registry: install With this branch the same steps print The second part of the report (a |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — tightens existing tag validation to reject the one in-memory-only sentinel value.
What was reviewed:
- Confirmed
ResolutionTagconstants (0,1,2,4,8,16,32,64,72,80,100) inresolution.rs— the newmatches!covers every real tag and only dropsUninitialized. - The v2 migration
_arm is unreachable afterload_fieldsruns first; returning the error instead of silently mapping toUninitializedis defense-in-depth, not a behavior change for valid files. - Checked that the fallback on rejection is "Ignoring lockfile" + fresh resolve, so a false positive (none known) degrades to a re-resolve rather than a hard failure.
- Tests mirror the neighboring
packageScriptsFilledOffsets/ meta-corruption pattern; the v2 fixture already exists and is shared withmigrate-bun-lockb-v2.test.ts.
Extended reasoning...
Overview
Two files: src/install/lockfile/Package.rs drops 0 from the accepted resolution-tag bytes in load_fields and changes the format-v2 migration catch-all arm from Resolution::init(TaggedValue::Uninitialized) to returning LockfileValidationFailedInvalidResolutionTag. test/cli/install/bun-lockb.test.ts adds a helper (packageResolutionTagOffsets) and two tests covering v3 (registry-generated) and v2 (checked-in fixture) lockfiles.
Security risks
None. This is input validation on a local on-disk lockfile. The change tightens an existing raw-byte enum check, following the same shape as the adjacent Meta origin/has_install_script and Scripts.filled checks. No new parsing, no new allocation, no new external I/O.
Level of scrutiny
Low-to-moderate. The src change is a one-token removal from a matches! plus converting a silent-degrade arm to an explicit error. I verified the Tag constants in src/install/resolution.rs:974-990 — 1 | 2 | 4 | 8 | 16 | 32 | 64 | 72 | 80 | 100 is exactly the set of non-Uninitialized variants. The v2 migration _ arm sits after load_fields has already rejected every byte outside that set, so it is defensive only. The behavioral question — could a legitimate saved lockfile carry tag 0 — is answered convincingly in the PR description (packages are appended only after resolution; the text-lockfile writer already skips them; the only known producer is a migration bug fixed in #39001), and even if one existed, the outcome is "warn, ignore, re-resolve from package.json", which is strictly better than the current silent-skip-and-rewrite (release) or unreachable!() panic (debug).
Other factors
- Tests follow the exact pattern of the three neighboring lockb-corruption tests (same offset math, same sanity-assert-then-corrupt, same
Ignoring lockfilerecovery assertions), and additionally assert the negative contract (bun bun.lockbexits 1 with the right message; the re-saved lockfile no longer contains the bad tag). - The v2 fixture
test/cli/install/fixtures/bun.lockb.v2already exists and is used bymigrate-bun-lockb-v2.test.ts. - No CODEOWNERS entry covers
src/install/. - No outstanding reviewer comments; only a CodeRabbit rate-limit placeholder in the timeline.
|
One precision on the review summary above: the re-resolve fallback applies to a plain |
SingleFileModule is a placeholder tag. Nothing writes a package with it, the bun.lock writer skips it next to Uninitialized, and the installers skip it the same way, so a bun.lockb that carries it has the same effect as one with an Uninitialized package. The loader now accepts exactly the tags the writers produce, spelled with the tag constants instead of their byte values. The tests cover both bytes.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Tag::belongs_in_lockfile lists the tags a writer produces. The bun.lock writer and the bun.lockb loader both use it, so the two cannot drift apart, and the loader no longer needs a comment to explain its list.
robobun
changed the title
|
Three follow-up commits since the first review:
The PR description is updated for the current diff. |
There was a problem hiding this comment.
LGTM — tightens bun.lockb resolution tag validation and deduplicates the tag list into Tag::belongs_in_lockfile.
What was reviewed:
Tagis a#[repr(transparent)]u8 wrapper, soResolutionTag(raw[tag_at])on arbitrary file bytes is sound;offset_of!matches the neighboring Meta/Bin/Scripts checks.- The bun.lock.rs writer change is behavior-preserving (same nine tags proceed, same three skip); the v2 migration
_arm is guarded byload_fieldsand now fails closed instead of silently degrading toUninitialized. - Four new tests cover both tags × both format versions using the existing offset-helper pattern; the
bun.lockb.v2fixture already exists. - The comment-cop flags on earlier revisions were addressed in 9485310/502b388 — the remaining comments in the diff are one or two lines.
Extended reasoning...
Overview
The PR tightens bun.lockb loading to reject packages whose resolution tag byte is Uninitialized (0) or SingleFileModule (100) — tags no writer produces and no installer handles. It touches four files: a new Tag::belongs_in_lockfile helper in src/install/resolution.rs listing the nine valid tags; src/install/lockfile/Package.rs where load_fields now calls the helper via offset_of! (matching the adjacent Meta/Bin/Scripts checks) and the v2 migration _ arm returns LockfileValidationFailedInvalidResolutionTag instead of silently constructing an Uninitialized resolution; src/install/lockfile/bun.lock.rs where the writer's own copy of the same nine-tag list is replaced by the shared helper; and four new tests in test/cli/install/bun-lockb.test.ts.
Security risks
None introduced. This is input-validation hardening on a binary file format — it rejects more invalid states and fails closed to the existing Ignoring lockfile recovery path. Tag is #[repr(transparent)] struct Tag(pub u8), not a #[repr(u8)] enum, so constructing it from an arbitrary file byte and matching on it is well-defined; the change removes a case where release builds silently exit 0 with a dependency uninstalled.
Level of scrutiny
Medium-low. The net src change is small (~30 lines), mechanical, and follows the exact pattern of the three neighboring column validators in load_fields. The bun.lock.rs writer change is a pure dedup — the old match and belongs_in_lockfile accept the identical nine tags and continue on the identical three. The v2 migration arm change is guarded: load_fields runs first on the same bytes and already rejects everything the removed arms handled, so the new error return is defensive rather than a live behavior change. The failure path (Ignoring lockfile → re-resolve) is the established recovery for every other lockfile validation error.
Other factors
Tests are solid: it.each over both rejected tags, format v3 via the local Verdaccio registry and format v2 via the existing checked-in fixture, pipes drained concurrently, exact tag-byte assertions before and after mutation, and a check that the re-saved lockfile no longer carries the bad tag. They mirror the existing packageScriptsFilledOffsets pattern in the same file. The PR description confirms all four fail without the src change and lists the broader install suites run against the debug build. The comment-cop bot flagged long comments on intermediate revisions; the head commit's comments are one or two lines each and robobun confirmed the cleanup in 9485310/502b388.
|
Nothing left to change from this review. The head it covers, 502b388, is the one in CI now (build 101626: 160 jobs passed, 19 running). The only failing test so far is |
|
CI result for 502b388 (build 101626): one test failed on every attempt, |
There was a problem hiding this comment.
LGTM — tightens bun.lockb tag validation to reject the two tag values no writer produces, with the fallback path and tests to match.
What was reviewed:
Tag::belongs_in_lockfilelists exactly the nine tags with a producer; the loader and the bun.lock writer now share it (writer change is behavior-preserving — it already skipped 0/100/unknown).load_fieldsruns before the v2 migration match, so the migration's_arm returning the error is defensive and unreachable in practice.offset_of!(ResolutionType, tag)resolves to the first byte of the#[repr(C)]struct, matching the previousraw[0]and the neighboring Meta/Bin/Scripts checks.- Four new tests cover both tags across format v2 and v3, and the v3 case confirms the re-saved lockfile has no broken tag left; the referenced
fixtures/bun.lockb.v2exists.
Extended reasoning...
Overview
This PR narrows the set of resolution tag bytes the bun.lockb loader accepts. Previously load_fields accepted all eleven named Tag constants including Uninitialized (0) and SingleFileModule (100), neither of which any writer emits and neither of which any installer handles. A lockfile carrying one of these would silently skip the package in release builds (exit 0, "Saved lockfile", nothing installed) and panic in debug builds. The fix introduces Tag::belongs_in_lockfile() listing the nine tags a writer actually produces, uses it in the loader (src/install/lockfile/Package.rs), reuses it in the bun.lock text writer (src/install/lockfile/bun.lock.rs) which had its own copy of the list, and changes the v2→v3 migration wildcard arm to return the same error instead of silently producing an Uninitialized resolution. The loader also switches from hardcoded raw[0] to mem::offset_of!(ResolutionType, tag), matching the neighboring column checks.
Security risks
None. This is input validation on a file read from disk; the change fails closed (rejects more, accepts less). A rejected lockfile falls back to a fresh resolve from package.json on plain bun install, or a hard error under --frozen-lockfile — the same path every other lockfile validation error takes.
Level of scrutiny
Low-to-medium. The Rust change is small (~30 net lines across three source files) and mechanical: an allowlist tightened by two entries, a duplicated list deduplicated into a helper next to the tag constants, and a hardcoded offset replaced with offset_of!. The only user-observable behavior change is that lockfiles containing tag 0 or 100 — which no bun release writes, and whose only known producer is the yarn.lock migration bug tracked separately in #39001 — now trigger a re-resolve instead of a silent partial install. I confirmed load_fields::<u32> runs before the v2 migration match, so the migration's new error-returning _ arm is unreachable given the loader check; returning the error there rather than unreachable!() is the right defensive choice per the repo's "user-reachable failures are recoverable errors, never panics" rule. The bun.lock.rs writer change is behavior-preserving: the old inline match already skipped Uninitialized, SingleFileModule, and unknown tags via continue.
Other factors
Tests are thorough for the change size: four it.each cases covering both rejected tags in both format v3 (via the local registry, checking bun bun.lockb exits 1, bun install recovers with "Ignoring lockfile", both packages install, and the re-saved lockfile has no broken tag) and format v2 (via the checked-in fixture, which I verified exists). The tests follow the file's existing patterns (same offset-computation helpers, same spawn + Promise.all shape as the neighboring scripts-flag and meta-id corruption tests). The comment-cop bot flags were all resolved by moving the tag list into the named helper and replacing the layout comment with offset_of!. The PR description records that the wider install test suites still pass on the debug build. No bugs were found by the bug-hunting system.
1 participant
Problem
bun.lockbpackage whose resolution tag byte is 0 (Uninitialized) or 100 (SingleFileModule) loads.bun installprintsSaved lockfile, exits 0, installs nothing for it, and writes it back out. A debug build panics instead:unreachable!()inResolutionType::eql(src/install/resolution.rs:525). Install fuzzer ledger entry 16544.load_fields(src/install/lockfile/Package.rs) accepted every tag constant. Dependencies on such a package keep a valid package id, soverify_resolutionscounts them as resolved. No installer has a path for either tag.Fix
Tag::belongs_in_lockfilelists the nine tags a writer produces. The bun.lockb loader rejects every other byte withinvalid resolution tag, as it did for unknown bytes:bun installprintsIgnoring lockfileand resolves again,bun bun.lockbexits 1. The bun.lock writer, which skipped the same two tags with its own list, uses the helper too.SingleFileModuleis a placeholder with no producer. The one way to get such a file today is the yarn.lock migration bug that the open install: skip yarn.lock entries the migration cannot build a resolution for #39001 fixes, withsaveTextLockfile = false. That file misses a dependency on every install. Now the next install resolves it again from package.json.test/cli/install/bun-lockb.test.ts, four new cases (both tags, format v3 through the registry and format v2 through the checked in fixture). All four fail without the src change.Background
bun.lockbstores packages as columns. Eachresolutionentry is a tag byte plus a union that says where the package comes from (npm, folder, git, workspace, ...). Tag 0 is the state of a package that is not resolved yet.load_fieldscopies each column from the file and checks the enum bytes first, because the rest of the loader matches on them.bun installwarnsIgnoring lockfileand resolves from package.json.--frozen-lockfilefails withlockfile had changes.Notes
Debug build panic path for tag 0:
ResolutionType::eql<-Lockfile::get_package_id<- thedebug_assert!inLockfile::append_package_with_id<-Package::clone<-Lockfile::clean_with_logger. The release build skips the assertion and carries the package through. Tag 100 has aneqlarm and reaches the_arm ofPackageInstaller::install_package_with_name_and_resolutioninstead, which panics in debug builds and counts the package as installed in release builds.isolated_installtreats the two tags as one class as well.The format v2 migration arms for the two tags return the same error as the loader. History of this PR: the first version rejected tag 0 only. The self-review found that tag 100 reproduces the report one byte away. The comment that explained the loader's list was then flagged as too long, so the list moved next to the tag constants, the writer shares it, and the loader reads the tag offset with
offset_of!like theMeta,BinandScriptschecks next to it.Before this change
bun install --frozen-lockfilealready refused the mutated file (lockfile had changes). It accepted the file that the plain install saved afterwards, so the broken state was sticky. The new tests check that the saved file has no such tag left.Suites run with the debug build:
bun-lockb,bun-lock,bun-install,migrate-bun-lockb-v2,bun-workspaces,bun-link,bun-add,bun-update,bun-remove,catalogs,migration/migrate, and theport-era-markers,pre-port-identifiersandbyte-searchsource lints.bun-install.test.ts: 224 pass, 14 fail. The failures are the bitbucket, gitlab and external tarball URL tests plus--registry CLI flag. All of them fail the same way on the released build in this environment (no network).bun-link.test.tshas one failure,should link dependency without crashing. It expects exact stdout and gets the debug onlydebug_traceof an install failure. Both are unrelated to this change.The fuzz report had a second part: a
bun.lockwhose"packages"key names one package (xbin) while the tuple id names another (p13@1.1.0) installsp13atnode_modules/xbinwith exit 0. This PR does not change that, on purpose:npm:alias ("my-alias": ["no-deps@1.0.0", ...], checked with the released build) and with overrides. A check on the key name rejects those lockfiles.enqueue_dependency_with_main_and_success_fnredirects a plain dependency to a same namednpm:alias declared elsewhere in the tree (known_npm_aliases). When the alias is removed later, the redirected edge survivesclean_with_logger. So bun itself writes a lockfile in which a plainfooedge points at a package namedbar, with no alias or override left to explain it.--frozen-lockfilewould then fail on a lockfile bun wrote.bun.lockthat pinsno-deps@2.0.0under a package.json that says1.0.0installs 2.0.0 under--frozen-lockfile(checked with the released build). Locked edges are trusted as a whole. Validating them against package.json is a feature in the area of install: fail --frozen-lockfile on manifest drift and fix the spurious lockfile re-saves behind it #33632, not a loader bug.The report's third point, never exit 0 with a root dependency bound to a package that cannot be installed, has two producers: this loader, fixed here, and the yarn.lock migration, fixed by the open #39001. A release mode check at the
append_package*entry points would also cover a future producer. It is left out here because both known producers are covered.no test proof · iteration 1 · 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