Conversation
|
Status: superseded by #43035, which merged on 2026-09-17. #43035 contains the three changes from this PR:
It also removes two more debug-only checks on registry data, and it has a test for each shape. This PR was closed as stale on 2026-09-13, not on merit. No work remains here. |
WalkthroughChangesBun now logs and skips invalid semver registry keys, continues processing valid versions, and permits duplicate canonical versions during sorting. Regression tests cover both registry packument cases. Registry version handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
robobun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/cli/install/bun-install.test.ts`:
- Around line 2500-2507: Reorder both install-test assertion blocks so the
expected exit status is asserted before reading generated artifacts; at
test/cli/install/bun-install.test.ts#L2500-L2507 guard exit code 1, then verify
the lockfile exists or has expected content and the installed package, and at
test/cli/install/bun-install.test.ts#L2547-L2553 guard exit code 0 before the
equivalent lockfile and package checks.
🪄 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: 32ac90ba-3c78-403e-8dae-c43703e544ec
📒 Files selected for processing (2)
src/install/npm.rstest/cli/install/bun-install.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — debug-only asserts on untrusted packument data removed to match release behavior; my earlier feedback on the sibling unreachable!() / .expect() sites is addressed in 6925c7d.
Checked: first-pass at npm.rs:2084-2093 already skips invalid versions before string_builder.count, so the hoisted second-pass continue keeps count/append balanced. The count == items.len() gate on the debug cross-check is sound — for peers, any offsetting meta-only synthesis is already excluded by the existing optional_peer_dep_names.is_empty() clause, so the .expect() at :2944 is now unreachable from registry input. Release behavior is unchanged; the three new mock-registry tests follow the neighboring withContext pattern and drain pipes concurrently.
Extended reasoning...
Overview
Removes three debug-only assertions in PackageManifest::parse (src/install/npm.rs) that fire on malformed registry packuments where release builds already recover gracefully: two debug_assert!(parsed_version.valid) on versions keys, one unreachable!() on non-string dependency values, and relaxes the post-sort adjacency check from == Greater to != Less to permit duplicate canonical versions (issue #20371). The debug-only items[j] cross-check is gated on count == items.len() so its .expect() is only reached when nothing was skipped. Three new mock-registry tests in bun-install.test.ts cover each trigger.
Security risks
None. The change makes debug builds more tolerant of adversarial registry input, aligning with existing release-build handling. No new parsing, no new trust boundaries. The relaxed sort assert only affects a debug sanity check; find_best_version's backward iteration is correct with equal adjacent entries.
Level of scrutiny
Moderate — PackageManifest::parse is a large two-pass function on the install hot path, but the edits are mechanical: delete a debug_assert, hoist an existing continue above the tag-copy block (matching the first-pass skip point so string_builder.append is never called on an uncounted key), collapse a cfg!(debug_assertions) unreachable!() / else continue to plain continue, and relax one ordering comparison. No release-build behavior change beyond the hoist, which is strictly a narrowing (skip earlier).
Other factors
I traced the new count == items.len() guard through the peer-synthesis path: when a skipped non-string entry could be offset by a synthesized meta-only peer (making count == items.len() coincidentally true), optional_peer_dep_names is necessarily non-empty, so the retained (!is_peer || optional_peer_dep_names.is_empty()) clause still excludes the block. First-pass counting at :2187-2196 uses .unwrap_or(b"") for non-string values, so the second-pass skip only over-reserves (harmless). My prior inline comment on the sibling sites is fully addressed and resolved; the CodeRabbit ordering suggestion was correctly declined per CLAUDE.md convention. Tests use the established withContext/setContextHandler harness, drain stdout/stderr/exited concurrently, and verify on-disk node_modules/bar/package.json.
…versions Registry packuments are untrusted input. A corrupt mirror, proxy, or hostile registry can serve a versions object containing keys that are not valid semver (e.g. "not-a-version"). PackageManifest::parse already logs an error and skips such keys, but the debug_assert that preceded the check aborted debug builds on first contact with such data. Remove the two debug_assert!(parsed_version.valid) checks on the raw version key. In the second pass, hoist the validity check above the pre/build tag copy so string_builder.append is never called on a key that was not counted in the first pass. The remaining assert on the re-parsed copy is a genuine internal consistency check and is kept.
…rsions
A private registry can serve distinct JSON keys in the packument versions
object that parse to the same canonical semver ("1.0.0" and "01.0.0").
After sorting, adjacent entries compare Equal, which tripped the strict
Greater assertion. The invariant find_best_version relies on is only
non-decreasing order, so assert != Less instead.
Fixes #20371
…reachable!()
Same bug class as the other two fixes in this PR: a registry packument
version entry with a non-string dependency value (e.g. {"baz": null})
hit unreachable!() in debug and continued in release. The first-pass
counting loop already treats such values as empty via .unwrap_or(b""),
so the second-pass unreachable!() was never a JSON-parser invariant.
Also gate the debug-only items[j] cross-check on count == items.len() so
it is skipped when any entry was filtered out (it already skips when
meta-only peers break the 1:1 correspondence).
6925c7d to
2ab438d
Compare
There was a problem hiding this comment.
LGTM — debug-only assertion removals on untrusted packument input, release behavior unchanged. My earlier comment on the sibling unreachable!() / .expect() sites was addressed in 6925c7d with a covering test.
What was reviewed:
- First-pass and second-pass
parsed_version.validhandling — the hoisted check at npm.rs:2365 now skips beforestring_builder.append, matching the first-pass count skip at :2084. - The
count == items.len()guard on the debugitems[j]cross-check — verifiedcountis the post-loopi(npm.rs:2795), so any skipped non-string entry makes the guard false and the.expect()is unreachable. - Post-sort
!= Lessrelaxation —find_best_versioniterates backwards and only needs non-decreasing order; duplicate canonical versions are benign. - Three new tests follow the neighboring
withContext/setContextHandlerpattern, drain pipes concurrently, and assert on-disknode_modules/bar/package.json.
Extended reasoning...
Overview
Removes three debug-only aborts in PackageManifest::parse (src/install/npm.rs) that fire on malformed-but-reachable registry packument data: (1) debug_assert!(parsed_version.valid) in both counting passes on non-semver versions keys, (2) debug_assert!(order == Greater) on duplicate canonical versions after sorting, and (3) unreachable!() on non-string dependency-map values plus its downstream .expect() in the debug verification block. Each site already had a graceful release-build path (continue / log-and-skip) immediately adjacent; the assert was the only divergence. Three regression tests in test/cli/install/bun-install.test.ts serve the triggering packuments from the mock registry.
Security risks
None introduced. Packument data is already treated as untrusted; this change makes debug builds match the existing release-build handling rather than aborting. No new parsing, no new allocation sizing, no new trust boundary crossed.
Level of scrutiny
Low-medium. All source changes are debug-assertion removals/relaxations or a validity-check hoist; release semantics are unchanged. The one non-assertion code movement — hoisting if !parsed_version.valid { continue; } above the pre/build tag copy — is a strict tightening that keeps second-pass string_builder.append calls aligned with first-pass string_builder.count calls (which already skipped invalid keys at line 2084). I traced count back to the post-loop i at line 2795 to confirm the new count == items.len() gate correctly disables the items[j] index-correspondence check whenever any entry was skipped, and confirmed the retained debug_assert!(parsed_version.valid) at line 2373 is a re-parse of an already-validated string (true internal invariant).
Other factors
- My prior inline review flagged the sibling
unreachable!()and.expect()sites as required scope; the author addressed both in 6925c7d with a third test covering{baz:null, qux:123}. Thread resolved. - CodeRabbit's assertion-ordering suggestion was declined by the author citing CLAUDE.md and neighboring-test convention; CodeRabbit accepted. Thread resolved.
- PR description states the three new tests fail on main with the exact panic messages and pass with the fix; robobun confirms CI green on all lanes for these tests, with unrelated flakes elsewhere.
- No CODEOWNERS entry for
src/install/.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-21, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
…#43035) ### Problem - A registry response aborts `bun install` (also `add`, `update`, `pm view`) on a debug or ASAN build, not on a release build. The panics: `non-value Expr from JSON parser`, `tarball_url is empty for package evil@1.0.0`, `assertion failed: parsed_version.valid`, `assertion failed: order == core::cmp::Ordering::Greater`, `assertion failed: dependencies_list.name.off as usize + ...`. - The cause is debug-only checks on remote data in `PackageManifest::parse` (`src/install/npm.rs:2127`, `2389`, `2697`, `2887`, `3164`) and `Package::from_npm` (`src/install/lockfile/Package.rs:874`). The release path next to each check already handles the case. ### Fix - Debug builds take the release path: skip a non-string dependency value, skip a `versions` key that is not a version, accept equal neighbours after the version sort, use the default URL when a version has no tarball URL. - The four strict `<` bounds checks on the dependency lists go away. `ExternalSlice::get`, on the next line, asserts the correct bound. The debug cross-check against the source JSON now compares with the items the build loop kept. - Correct because release builds have none of these checks and already run this path on the same input. - Verified: 8 new tests in `test/cli/install/bun-install.test.ts` fail on a debug build before and pass after. Nine more install suites pass (Notes). ### Background - A packument is the registry document for a package. Its `versions` field maps a version string to that version's fields. `PackageManifest::parse` reads it in two passes: count, then fill. - `Package::from_npm` turns a resolved version into a lockfile package. An empty tarball URL means the default URL, `<registry>/<name>/-/<name>-<version>.tgz`. `bun.lock` writes it as `""`. - `debug_assert!` code runs only in debug and ASAN builds (`bun bd`, CI's ASAN lane). <details><summary>Notes</summary> **Shapes, all against a local `Bun.serve` registry.** Before this change the debug build ends with SIGABRT. The release build (1.4.3-canary c6b7fcb) and this branch give the result in the last column. | Packument | Panic on a debug build | Result | | --- | --- | --- | | `"dependencies": {"good": null}` (also a number, boolean or object, in any of the three groups) | `non-value Expr from JSON parser` | the entry is skipped, exit 0 | | `"dist": {}`, no `dist`, `dist` not an object, `dist.tarball` not a string or `""`, a `versions` entry that is not an object | `tarball_url is empty for package evil@1.0.0` | downloads `<registry>/evil/-/evil-1.0.0.tgz`, exit 0 | | a `versions` key `"not-a-version"` | `assertion failed: parsed_version.valid` | prints `error: Failed to parse dependency not-a-version`, installs the valid version, exit 1 | | `versions` keys `"1.0.0"` and `"01.0.0"` (also `"1.0"`, `"1.0.0-"`, `"v1.0.0"`) | `assertion failed: order == core::cmp::Ordering::Greater` | exit 0 | | no `dist-tags`, no tarball URL, one version with `dependencies` or `peerDependenciesMeta` | `assertion failed: dependencies_list.name.off as usize + (dependencies_list.name.len as usize) <` or `(dependencies_list.name.off as usize) < all_extern_strings.len()` | exit 0 | The last row is new. I found it with the probe below after the first four were gone. With no dist-tags and no tarball URL, the last dependency list ends exactly at the end of `all_extern_strings`, and the strict `<` fails. The deleted checks also compared the value list with `all_extern_strings`, but that list lives in `version_extern_strings`. `ExternalSlice::get` asserts `off + len <= len` against the buffer it reads. **Release builds.** The compiled release code changes in one place: in the second pass the `if !parsed_version.valid { continue; }` moves above the copy of the pre and build tags. The result is the same, because `Version::parse` sets `valid = false` only before it parses a tag, so an invalid key never has a tag to copy. The `unreachable!` arm of the `group_idx` match had the message `non-value Expr from JSON parser`. It now says what it guards (`DEPENDENCY_GROUPS` has 3 entries). **Why delete the `tarball_url is empty` panic and not warn.** An empty URL is a state the release path handles on purpose (`NetworkTask::for_tarball`, and `bun.lock` writes `""` for the default URL). `PackageInstaller` has a `debug_warn!` for the same field with the comment that old lockfiles make an assertion impossible. The registry makes it impossible here in the same way. A second debug-only message would make debug and release output differ on the same input, which is the thing this change removes. **The invalid `versions` key exits with 1.** That is the release behavior today: the parse error goes to the log, the install completes, and the exit code is 1. The test pins it as it is. This change does not decide if it must be a warning. **Probe.** 2,121 hostile packuments (20 values for each version, `dist` and root field, odd `versions` keys, dist-tags, manifests with no dist-tags and no tarball), each with `bun install` on the debug build, for an exact version, `latest` and a range. Before: 377 end with SIGABRT. After: 12. Open PRs cover the 12: #41735 (`assertion failed: !actual.is_empty()` for an empty dependency name) and #38954 (`range end index N out of range for slice of length 4095` for a long `bin` value, which also aborts release builds). I also ran the five shapes two times with the manifest disk cache on. The second run reads the cached manifest and exits the same way. **History.** #34651 removed three of these assertions and was closed as stale, not on merit. #20371 is the user report for the `Ordering::Greater` one. **Tests.** The 8 tests pass on the release build before and after, because only builds with debug assertions have the defect. `bun bd test test/cli/install/bun-install.test.ts -t "unexpected shape"` runs them alone. In the whole file, 13 other tests clone from gitlab.com and bitbucket.org. They fail in a sandbox with no internet access, with and without this change. Suites run on the debug build with this change: `bun-install.test.ts` (243 pass, the 13 above fail), `bun-install-registry.test.ts`, `bun-add.test.ts`, `bun-update.test.ts`, `bun-pm.test.ts`, `bun-info.test.ts`, `bun-install-cpu-os.test.ts`, `bun-install-tarball-integrity.test.ts`, `minimum-release-age.test.ts`, `bun-install-retry.test.ts`. </details> <!-- robobun:evidence:begin --> --- **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-install.test.ts <!-- robobun:evidence:end -->
1 participant
Fixes #20371
What
Debug builds abort in
PackageManifest::parse(src/install/npm.rs) on registry packuments containing malformed version data. Release builds already handle each case gracefully; the asserts sit immediately before or after the existing graceful path.Three triggers, all from untrusted registry data:
versionskey that does not parse as semver at all ("not-a-version","__proto__",""):versionskeys that parse to the same canonical version ("1.0.0"and"01.0.0", or"0.0.2"and"00.0.2"), reported in Bun Crash: Internal assertion failure in npm.zig due to inconsistent semver versions (i.e.1.1instead of1.1.1) #20371:dependencies/optionalDependencies/peerDependenciesmap ({"dependencies":{"baz":null}}):All three are reachable from every manifest fetch (
bun install,bun add,bun update,--install=auto) against any registry, proxy, or mirror.Fix
debug_assert!(parsed_version.valid)checks on the rawversionskey. The surrounding code already logsFailed to parse dependency ...and skips the entry. In the second pass, hoist the validity check above the pre/build tag copy sostring_builder.appendis never called on a key that was not counted in the first pass.== Greaterto!= Less.find_best_versiononly requires non-decreasing order for backwards iteration; duplicate canonical versions sort asEqualand work correctly.if cfg!(debug_assertions) { unreachable!(...) } else { continue }on non-string dependency values with a plaincontinue(the first-pass counting loop already treats them asb""via.unwrap_or). Gate the debug-onlyitems[j]cross-check oncount == items.len()so it is skipped when any entry was filtered.Verification
Three new tests in
test/cli/install/bun-install.test.tsserve packuments from the mock registry.Without the fix (
bun bdon main):With the fix: all three install
bar@0.0.2tonode_modules, matching release-build behavior.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install.test.ts