Repository navigation
Conversation
`bun pm view` and `bun info` take the `versions` entry that matches the
requested version and call `Expr::set` on it. `Expr::set` is
`unreachable!()` for a value that is not an object, so a registry
response such as `"versions": {"1.0.0": "x"}` aborted the command with
`panic: internal error: entered unreachable code`.
Check the entry before use. Print an error and exit 1 when it is not an
object.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 3 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 3 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 1 minute for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: closed as a duplicate of #43019. #43019 makes the same check at the same place in // registry.js
const packument = { name: "pkg", "dist-tags": { latest: "1.0.0" }, versions: { "1.0.0": "x" } };
const server = Bun.serve({ port: 0, fetch: () => Response.json(packument) });
console.log(`[install]\nregistry = "http://localhost:${server.port}/"`); // put this in bunfig.toml |
There was a problem hiding this comment.
LGTM — small, self-contained fix that turns a user-reachable unreachable!() panic into a clean error exit.
What was reviewed:
- Confirmed the crash mechanism:
Expr::set(src/ast/expr.rs:496-499) hitsunreachable!()on a non-object, and the new check at src/runtime/cli/pm_view_command.rs:227 runs before themanifest.setcall at line 299 on every code path. - Checked the
--jsonpath printing a plain-text stderr error: consistent with the siblingfailed to parse response body as JSON/failed to parse package manifestexits in the same function, so not a new inconsistency. - Checked the test: hermetic local
Bun.serve({ port: 0 })registry, pipes drained concurrently, combined{stdout, stderr, exitCode}assertion,describe.concurrent; the expected stderr string matcheserr_generic'serror:prefix with<b>/<r>stripped andbun_fmt::quotequoting. - Considered whether root-level lookups like
bun pm view pkg dist-tagsshould still succeed on a non-object entry: they previously panicked atmanifest.setanyway, so the new exit 1 is strictly an improvement, not a regression.
Extended reasoning...
Overview
The PR touches one function in src/runtime/cli/pm_view_command.rs (the view command backing bun info / bun pm view) and appends six concurrent tests to test/cli/install/bun-info.test.ts. The code change is eight lines: after the loop finds the versions entry matching the resolved semver, it checks that the entry's ExprData is EObject and, if not, prints an error via Output::err_generic and exits with code 1. Otherwise behavior is unchanged.
Security risks
None introduced. The change strictly narrows what a registry response can do: previously a malformed manifest (string/null/array under versions[<v>]) reached Expr::set, which is unreachable!() for non-objects — a panic on network-controlled input. The new check happens before any use of the value. No new allocation, parsing, or path handling is added.
Level of scrutiny
Low. The diff is small, the invariant being enforced (every later step reads fields of the matched entry, and manifest.set at line 299 requires an object) is verifiable by reading the surrounding ~80 lines, and the .expect("infallible: prop has value") it retains is the pre-existing one, relying on the JSON parser producing values for object properties. The error message follows repo voice (error: prefix, bun_fmt::quote, <b>/<r> markup, stderr). The --json mode emitting plain-text stderr matches the two adjacent parse-failure exits in the same function, so this is not a new inconsistency. No changed file is covered by .github/CODEOWNERS.
Other factors
The bug hunt exited on dry_streak with zero findings; the four ruled-out candidates (JSON-mode error shape, root-level property lookups, the retained expect, non-object document bodies) were each examined and do not represent regressions from the pre-PR state. The tests are hermetic (local Bun.serve, port: 0, proxy env cleared), use describe.concurrent, drain pipes with Promise.all, and assert a combined {stdout, stderr, exitCode} object, matching test/CLAUDE.md conventions. I did not run a debug build in this session; the PR author's claim that the tests fail on the unfixed canary is consistent with the unreachable!() I confirmed in Expr::set.
|
Closing: this is covered by #43019. It makes the same check at the same place in |
Problem
bun pm viewandbun infoabort withpanic: internal error: entered unreachable codewhen the matchedversionsentry of the registry response is not an object, for example"versions": {"1.0.0": "x"}. A registry, or a host on the path of an http registry, can send that response.view()(src/runtime/cli/pm_view_command.rs:291). It callsExpr::seton the matched entry.Expr::set(src/ast/expr.rs:496) isunreachable!()for a value that is not an object.Fix
view()checks the matched entry before it uses it. If the entry is not an object, it printserror: registry manifest for "pkg" has a "1.0.0" version that is not an objectand exits with code 1.view()already exits with code 1 for a response that does not parse.test/cli/install/bun-info.test.ts(6 new tests, canary c6b7fcb fails 6 of 6 with SIGABRT). The whole file passes on the debug build.Background
versionsobject maps each version string to thepackage.jsonof that version.bun pm viewparses the response two times.PackageManifest::parseresolves the requested tag or range, and it accepts an entry that is not an object. A generic JSON parse gives theExprtree that the command prints.Expris the AST node type of the JS parser.Expr::setwrites one property of an objectExpr.Notes
Repro (loopback registry, release canary 1.4.3-canary.1+c6b7fcb5b, Linux x64):
A string, a number,
null,true,[]and[{...}]all abort.bun pm view pkg,bun pm view pkg --json,bun pm view pkg versionandbun info pkgall abort, because theExpr::setcall runs before the output mode is read.Shape matrix: 240 malformed manifests (each top-level field, each field of the version,
dist,dist-tags,dependencies,maintainers,keywords,time, and the whole body, each replaced by 10 values of the wrong type) times 5 argument forms, 1,200 runs.PackageManifest::parse(src/install/npm.rs:2127for a version key that is not semver,src/install/npm.rs:2697for a dependency value that is not a string). Release builds skip both cases, andbun installshares them. install: don't debug_assert on untrusted registry packument version keys #34651 covered them and was closed as stale, not on its merits.#42052 rewrites
view()for npm parity and removes theExpr::setcall as a side effect. It is a larger, separate change.Expr::sethas other callers inbun publish. They pass the localpackage.jsonafter they validate it.The error goes to stderr as plain text with
--jsontoo. The other malformed-response exits inview()(failed to parse response body as JSON,failed to parse package manifest) do the same.