install: bump default lockfileVersion to 2, gate stricter parse checks behind it#31539
Conversation
…s behind it Two parse-time checks added after the Rust rewrite reject text lockfiles that earlier versions accepted: - an npm package resolved to a tarball URL outside the configured registry must carry a supported integrity hash (bun.lock.rs) - a git/github .bun-tag must be a safe path/checkout component Both break lockfiles written before the checks existed. Introduce lockfileVersion 2, make it the default, and gate both rejections behind it so v0/v1 lockfiles keep loading. The git tag is still re-validated in Repository::checkout before any cache path is built or git is run, so skipping the parse-time check for older versions does not reopen the checkout hole. Updates written-lockfile snapshots from version 1 to 2.
|
Warning Review limit reached
More reviews will be available in 7 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughThis PR introduces lockfile format version V2 as the new default, gates two security validations (npm tarball integrity and git tag safety) to enforce only on V2+, and maintains backward compatibility for V1 lockfiles. Version-gating is implemented via a new ChangesLockfile V2 Versioning and Validation
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
StatusDone — ready for review. Bumped default
Both broke lockfiles written before the checks existed; v0/v1 now skip them and keep loading. The git-tag guard is still enforced at checkout time ( New offline test |
|
Updated 1:05 AM PT - May 29th, 2026
❌ @robobun, your commit 272af67 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31539That installs a local version of the PR into your bun-31539 --bun |
|
Actionable comments posted: 0 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- git .bun-tag test: point at an unreachable local endpoint (127.0.0.1:1) so the v1 case, which now parses successfully and proceeds to git clone, fails fast offline instead of contacting github.com. - off-registry integrity test: restore the same-registry-URL-accepted assertion (at lockfileVersion 2) that exercises the npm_url_needs_integrity = false branch, instead of only pointing at the off-registry coverage.
Update — addressed review feedbackclaude[bot] flagged two test issues; both fixed in
Both review threads resolved. Offline test file ( |
…keep github tag check unconditional Addresses two issues in the v2 gating: 1. Re-save would defeat the backward-compat gate. The writer always stamped Version::CURRENT and round-trips npm integrity / git tags verbatim with no backfill, so a v0/v1 lockfile carrying an off-registry tarball without integrity (or an unsafe git .bun-tag) got rewritten as lockfileVersion 2 on the next save and then failed to re-parse. Add Stringifier::version_to_write: stamp v2 only when every package is already v2-valid (npm under the configured/default registry or carrying a supported integrity hash; git/github resolved tag safe), else stay at v1 so the file round-trips. 2. The github tarball path has no use-site re-validation. The git .bun-tag check was gated to v2 on the premise that Repository::checkout re-validates, but that only covers git; github dependencies resolve via the tarball-download path and feed the resolved tag straight into the cache folder name. Keep the safety check unconditional for github while leaving git gated to v2. Fix the one migration snapshot (yarn-stuff) whose off-registry, no-integrity npm entry now correctly keeps it at lockfileVersion 1.
- packages/bun-types/bun.d.ts: widen BunLockFile.lockfileVersion to 0 | 1 | 2 - docs/pm/catalogs.mdx: bump the bun.lock excerpt to lockfileVersion 2
Update 2 — addressed deeper review feedbackThree substantive findings fixed (commits
New tests: github-unconditional rejection + v1-off-registry round-trip (both verified fail-before/pass-after). All 6 review threads resolved. |
Since the github `.bun-tag` safety check is enforced at every lockfile version (its download path has no checkout-time re-validation), no version can round-trip an unsafe github tag — the v1 fallback in version_to_write only meaningfully applies to git. Drop the dead `Github` arm and correct the Version::V2 doc comment to list only what is actually gated to v2. No functional change.
CI status — failure is infra flake, diff is greenBuild #58840's top-level Key signals that this is a transient BuildKite hiccup, not a real failure:
No leaf touches this diff is red. I haven't burned a (Locally: all 6 offline tests in |
`--lockfile-only` re-serializes the lockfile unconditionally (via save_lockfile_only), so the omitted configVersion was an inert red herring — it is not what triggers the re-save. Reword the comment to describe the actual trigger and add configVersion: 1 to the input. No behavior change.
CI status (build #58859) — same transient aggregate flake, diff is greenBuild #58859 on
No leaf that touches this diff is red. This is a BuildKite aggregate-status artifact, not a real failure. I'm not pushing a (Locally: all 6 offline tests in |
|
@robobun fix the test failures in test/cli/install/migration/yarn-lock-migration.test.ts |
The version-selection pre-pass scanned the entire in-memory pkg_resolutions buffer, which during a migration (e.g. yarn.lock -> bun.lock) can contain pruned/unreferenced package entries that never reach the written `packages` object. Such a stale entry could force lockfileVersion 1 even when every package actually serialized was v2-valid — breaking the workspace-yarn-migration snapshot (expected 2, got 1). Walk the package tree the same way the writer does (tree::Iterator over the reachable dependencies, resolving each dep to its package via resolution_buf, skipping invalid_package_id) so version_to_write sees exactly the set of packages that get written.
|
@Jarred-Sumner Fixed in 4509e0a. Root cause: the Fix: (The |
Correction on the earlier CI analysisMy two prior comments called build #58840/#58859 pure flake based on the status-API snapshot showing no failing leaf — but the Linux test lanes were still pending at the time I looked, and one of them (the yarn migration lane) did go on to fail for a real reason: the Now fixed in 4509e0a; |
Only the first two tests use `file:` deps; the rest use loopback/unreachable endpoints. Reword the header to describe how the whole file stays offline.
| ResolutionTag::Npm => { | ||
| if pkg_metas[i].integrity.tag.is_supported() { | ||
| continue; | ||
| } | ||
| // No supported integrity: only v2-clean if the tarball | ||
| // URL is under the configured/default registry (mirrors | ||
| // the parser's `npm_url_needs_integrity` computation). | ||
| let url = res.npm().url.slice(buf); | ||
| let configured_registry = options | ||
| .scope_for_package_name(pkg_names[i].slice(buf)) | ||
| .url | ||
| .href(); | ||
| let under_registry = url_is_under_registry(url, configured_registry) | ||
| || url_is_under_registry(url, Npm::Registry::DEFAULT_URL.as_bytes()); | ||
| if !under_registry { | ||
| return Version::V1; | ||
| } |
There was a problem hiding this comment.
🔴 version_to_write's "v2-clean" check for an integrity-less npm URL accepts URLs under options.scope_for_package_name(...) — the writer's configured/scoped registry — but the parser's npm_url_needs_integrity (lines 2421-2431) uses the reader's config. So a developer with a per-machine scope (e.g. @myorg → http://internal.example.com via ~/.npmrc or env) re-saving a v1 lockfile containing such a URL with empty integrity will stamp it v2, and a teammate / CI without that scope config will then fail to parse it with "Missing integrity hash…" — defeating the round-trip guarantee this function exists for, just across config boundaries. The DEFAULT_URL half of the check is config-independent and safe; consider dropping the scope_for_package_name half so the v2 stamp is conservative and reader-config-independent.
Extended reasoning...
What the bug is
version_to_write() (added in 58bc805 to fix the "re-save stamps v2 and breaks the next parse" issue) decides whether to stamp lockfileVersion: 2 by mirroring the parser's npm_url_needs_integrity predicate: for each npm package without a supported integrity hash, it checks whether the URL is under either options.scope_for_package_name(pkg_name).url or Npm::Registry::DEFAULT_URL (lines 231-237). The parser does the same at lines 2421-2431. The problem is that both halves call scope_for_package_name(...) on the current process's PackageManagerOptions — so the writer evaluates the predicate against its configured scopes, and the parser evaluates it against its own. When those differ across machines, a lockfile the writer judges "v2-clean" can be "v2-invalid" for a reader.
The specific code path
- Writer URL serialization (lines 916-923) only blanks the URL to
""when it's underDEFAULT_URL. A URL under a configured-but-not-default registry (e.g.http://internal.example.com) is written verbatim. version_to_writeNpm arm (lines 224-240): for an entry with!integrity.tag.is_supported(), computesunder_registryagainst the writer'soptions.scope_for_package_name(...)∪DEFAULT_URL. If true, the entry is treated as v2-clean and the loop continues; if every package passes, the function returnsVersion::CURRENT(v2).- Parser (lines 2421-2431, 2646-2655): computes
npm_url_needs_integrityagainst the reader'smgr.scope_for_package_name(...)∪DEFAULT_URL. At v2, an unsupported integrity withnpm_url_needs_integrity == truefails the parse.
Why existing code doesn't prevent it
version_to_write's doc comment states the contract: "Only stamp v2 when every package already satisfies the v2 invariants; otherwise stay at v1 so the file round-trips (load → save → load) cleanly." The function correctly mirrors the parser's predicate — but the predicate itself is config-dependent for the configured-registry half, and lockfiles are explicitly meant to be committed and shared across machines with potentially different registry config (~/.npmrc, NPM_CONFIG_REGISTRY, uncommitted bunfig.toml, etc.). The DEFAULT_URL half is config-independent: the writer blanks those URLs to "" and an empty URL never sets npm_url_needs_integrity. The configured-registry half is not.
Step-by-step proof
- Dev A has
@myorg → http://internal.example.comconfigured locally (e.g. in~/.npmrc, not committed). - The repo has a v1
bun.lockcontaining"@myorg/foo": ["@myorg/foo@1.0.0", "http://internal.example.com/@myorg/foo/-/foo-1.0.0.tgz", {}, ""]— off-default-registry, empty integrity (e.g. from a yarn/pnpm migration or older bun). - Dev A runs
bun add bar(or--lockfile-only).version_to_writereaches the@myorg/fooentry: integrity unsupported;options.scope_for_package_name("@myorg/foo").url=http://internal.example.com/;url_is_under_registry(url, that)= true →under_registry = true→ continues. All packages pass → returnsV2. - Writer emits
"lockfileVersion": 2. At line 916, the URL is not underDEFAULT_URL(npmjs.org), so it's written verbatim. Integrity is written as"". - Dev A commits. Dev B (or CI), with no
@myorgscope configured, runsbun install. Parser readslockfileVersion: 2. At line 2421,mgr.scope_for_package_name("@myorg/foo")falls back to the default scope →configured_registry= npmjs.org.url_is_under_registry("http://internal.example.com/...", npmjs.org)= false; same forDEFAULT_URL→npm_url_needs_integrity = true. - At line 2646:
at_least(V2) && npm_url_needs_integrity && !is_supported()→ parse fails with "Missing integrity hash for npm package resolved to a tarball URL outside the configured registry".
So a v1 lockfile that (post-PR) loads fine for Dev B becomes unloadable after Dev A re-saves it — exactly the load→save→load failure mode version_to_write was added to prevent, just split across two machines' configs.
Impact
This is not a strict regression vs. the immediately-prior release — pre-PR, Dev B would have rejected the v1 lockfile too (the check was unconditional). But it is a gap in this PR's newly-added fix: this PR makes the v1 lockfile loadable for Dev B, and version_to_write exists precisely to keep it that way through a re-save; the configured-registry half undermines that for cross-config teams. The scenario (private scoped registry, per-developer config, legacy lockfile without integrity) is the bread-and-butter enterprise case that the off-registry-integrity backward-compat is for.
Fix
In version_to_write's Npm arm, drop the configured_registry check and only treat the entry as v2-clean if the URL is under DEFAULT_URL:
let url = res.npm().url.slice(buf);
if !url_is_under_registry(url, Npm::Registry::DEFAULT_URL.as_bytes()) {
return Version::V1;
}This makes the v2 stamp conservative and reader-config-independent, matching the only writer-side normalization that actually happens (line 916). Packages freshly resolved from a configured registry carry integrity anyway, so they pass the is_supported() early-continue and never reach this branch — the only cost is keeping legacy private-registry-no-integrity lockfiles at v1 a little longer for single-config teams.
There was a problem hiding this comment.
Follow-up in #31556 — dropped the scope_for_package_name half so the v2 stamp only trusts the default-registry normalization and is reader-config-independent. Added an offline writer→reader round-trip test across config boundaries.
|
✅ Follow-up opened → #31556
|
…istry config (#31556) Follow-up to #31539 (review [thread](#31539 (comment))). Jarred asked for this as a separate PR. ## What `version_to_write` picks whether a re-saved text lockfile can be stamped `lockfileVersion: 2`. For an npm package without a supported integrity hash, it treated the entry as "v2-clean" if the tarball URL was under **either** the writer's scope-configured registry (`scope_for_package_name(...)`) **or** the default registry. The `scope_for_package_name` half is config-dependent and undermines the cross-machine round-trip guarantee the function exists for. This PR drops it, keeping only the config-independent default-registry check. ## The problem A lockfile is committed and shared, so whether the **reader** accepts it must not depend on the **writer's** registry config. But: 1. A dev has `@myorg → http://internal.example.com` configured locally (`~/.npmrc` / `bunfig.toml`, not committed). 2. The repo has a v1 `bun.lock` with `"@myorg/foo": ["@myorg/foo@1.0.0", "http://internal.example.com/@myorg/foo/-/foo-1.0.0.tgz", {}, ""]` — off-default-registry, empty integrity (e.g. from a migration or older Bun). 3. The dev re-saves (`bun add …`, `--lockfile-only`, …). `version_to_write` sees the entry: integrity unsupported, but the URL **is** under `scope_for_package_name("@myorg/foo")` (their `@myorg` scope) → treated as v2-clean → whole file stamped **v2**. 4. The writer only blanks a URL to `""` when it's under the **default** registry, so this URL is written verbatim and integrity stays `""`. 5. A teammate / CI with **no** `@myorg` scope runs `bun install`. The parser's `npm_url_needs_integrity` is evaluated against the *reader's* config → the URL is under neither the reader's scope nor the default → at v2, parse **fails** with *"Missing integrity hash for npm package resolved to a tarball URL outside the configured registry"*. So a v1 lockfile that (post-#31539) loads fine becomes unloadable for a teammate after the first dev re-saves it — the exact load→save→load failure `version_to_write` was added to prevent, just split across two machines' configs. ## The fix In `version_to_write`'s `Npm` arm, drop the `scope_for_package_name` check and only treat an integrity-less entry as v2-clean when the URL is under `Npm::Registry::DEFAULT_URL`: ```rust let url = res.npm().url.slice(buf); if !url_is_under_registry(url, Npm::Registry::DEFAULT_URL.as_bytes()) { return Version::V1; } ``` The default-registry case is the only normalization the writer actually performs (it blanks those URLs to `""`, and an empty URL never sets `npm_url_needs_integrity`), so it round-trips for any reader regardless of config. Packages freshly resolved from a configured registry carry integrity and pass the `is_supported()` early-continue before this branch, so the only cost is keeping legacy private-registry-without-integrity lockfiles at v1 a little longer for single-config teams. `options` is no longer needed, so it's dropped from the signature. ## Verification New test in `test/cli/install/lockfile-version-2.test.ts` — *"re-saving keeps v1 for a tarball under a writer-only scoped registry"* — runs fully offline: - **Writer** has `@myorg` scoped to a loopback registry and re-saves (`--lockfile-only`) a v1 lockfile with an `@myorg/foo` tarball under that registry and empty integrity. The re-saved lockfile must stay `"lockfileVersion": 1`. - **Reader** (fresh dir, no `@myorg` scope) loads the re-saved lockfile and must **not** hit the integrity error — proving the round-trip holds across config boundaries. Fail-before/pass-after confirmed by building with the `src/` change stashed: the writer stamps `"lockfileVersion": 2` and the assertion fails; with the change applied it stays v1 and the reader loads it.
2 participants
What
Introduces text
lockfileVersion2, makes it the default, and gates two parse-time checks — added after the Rust rewrite — behind it so that older lockfiles keep loading.Background
Two checks landed in
src/install/lockfile/bun.lock.rsthat reject a text lockfile the earlier versions accepted:Missing integrity hash for npm package resolved to a tarball URL outside the configured registry..bun-tag— a git/github.bun-tagthat is not a safe single path component (is_safe_resolved_tag) is rejected at parse withInvalid git dependency tag.Both are breaking: a
bun.lockwritten before these checks existed (e.g. an npm entry with an off-registry tarball and no integrity hash) now fails to parse.Change
Version::V2and anat_least()helper; bumpVersion::CURRENTfrom V1 to V2. The writer always emits the current version, so fresh lockfiles are now"lockfileVersion": 2.lockfile_version.at_least(Version::V2). For v0/v1 lockfiles the checks are skipped (the pre-check behavior), so existing lockfiles keep loading.Security note
The git
.bun-tagcheck is a path-traversal / checkout-arg guard. It is re-validated at the point of use inRepository::checkout(src/install/repository.rs) with the sameis_safe_resolved_tag, before any cache directory path is built orgitis invoked — and a git dependency can only reach the cache throughcheckout. Skipping the parse-time rejection for older lockfile versions therefore does not reopen the vulnerability; an unsafe tag still cannot be checked out or turned into a traversal path.The off-registry integrity gate restores, for v0/v1 only, the behavior Bun shipped before the check was added. Lockfiles written at v2 always carry integrity for off-registry tarballs, so enforcement there is unaffected.
Verification
New offline test
test/cli/install/lockfile-version-2.test.ts(usesfile:deps, no registry):.bun-tagis rejected only at version 2Fail-before/pass-after confirmed by building with the
src/change stashed: the three version-gated tests fail (v2 is an unknown version / v1 still rejects), and pass with the change applied.Also updated the existing off-registry-integrity test in
test/cli/install/bun-lock.test.tsto assert the v2 behavior, and regenerated the written-lockfile snapshots (lockfileVersion1 → 2). Input-fixture lockfiles in tests are deliberately left at version 1 to keep exercising backward-compatible loading.