install: reject tarball, folder and git packages whose package.json name is invalid

[robobun]

Problem

• A file: tarball, file: folder, git or workspace dependency whose own package.json has a name such as "a:b" installs fine, and bun install writes "x": ["a:b@../evil.tgz", ...] into bun.lock.
• The bun.lock parser rejects that entry (error: Invalid package name, src/install/lockfile/bun.lock.rs line 2357), so every following bun install prints warn: Ignoring lockfile, re-resolves and writes the same lockfile again, bun install --frozen-lockfile always fails with lockfile had changes, but lockfile is frozen, and bun pm ls fails with error: Error loading lockfile: InvalidLockfile.
• Cause: Package::parse_with_json_impl (src/install/lockfile/Package.rs, the 'name: block near line 2064) copies any non-empty name out of the package's package.json. Nothing applies the check the lockfile parser applies on reload. Registry packages are not affected because their name is the requested, already validated name; this only concerns names read out of tarball, folder, git and workspace packages.
• In the same situation the isolated linker already refuses the package ("a:b" is not a valid install folder name, the name is a store folder there), so only the hoisted linker got as far as saving the lockfile.

Fix

• Package::parse_with_json_impl runs dependency::is_safe_install_folder_name on the name of every non-root package. On failure it logs Invalid package name "<name>" pointing at the name value in that package.json and returns InvalidPackageJSON, the error this function already uses for package.json contents it refuses, so the install fails before a lockfile is saved.
• This is the same predicate the bun.lock parser uses, so whatever bun saves it can load again; and since install: reject dependency names containing control characters #38615 extends that predicate to control characters, those get rejected here as well once it lands. The rejected name is echoed through bun_core::fmt::quote, which JSON-escapes control characters and quotes.
• Failing instead of inventing a name: these names are already rejected by the isolated linker and by the lockfile, and npm's own name rules reject them too (:/\, ./.. segments, empty segments), so no package produced by normal tooling is affected. A substituted name would also never match the installed package.json, so bun install would reinstall the package every run.
• The root package is exempt (FEATURES.is_main): its name is only written to the workspaces section, which accepts it, and a root named "a:b" installs and reloads fine today.
• The git and tarball arms of process_extracted_tarball_package (src/install/PackageManager/processDependencyList.rs) now exit through PackageManager::crash() instead of Global::crash(), so the logged reason is printed; without that, the new error (like any InvalidPackageJSON from a tarball today) would only show up as expected package.json in ../evil.tgz to be a JSON file: InvalidPackageJSON. The folder and workspace paths already printed the log. The npm arm of that match is not on this path and is unchanged.
• Out of scope: a tarball/folder package with no name at all ("x": ["@file:../dir", {}]) and names containing @ also produce a lockfile that does not load back, but that is a problem of the name@resolution encoding rather than of name validation, and is left for a separate change.
• Verified: test/cli/install/bun-install.test.ts, describe a dependency whose own package.json has an invalid name: local tarball, file: folder (asserts the error location), escaping of the echoed name, git dependency (local dumb-http repo), plus two controls that must keep passing: a scoped @scope/dep tarball still installs and the saved bun.lock loads under --frozen-lockfile, and an invalid root name is still accepted. The four rejection tests fail on the released binary and pass with this change; the controls pass on both.
• Also ran with the debug build: the rest of bun-install.test.ts (remaining failures are the bitbucket/gitlab network tests and two tests that fail identically without this change in this environment), bun-workspaces, bun-add, bun-patch, isolated-install, migration/migrate, and the folder/tarball/workspace/git subset of bun-install-registry; all green.

Background

• bun.lock stores each installed package as "<path>": ["<name>@<resolution>", ...]. On load, the name is split back out and checked with is_safe_install_folder_name, because package names from the lockfile become folder names: the npm cache folder for registry packages and the node_modules/.bun/<name>@... store folder for isolated installs. A name failing the check makes the whole lockfile unloadable, which bun reports as Ignoring lockfile and treats as "no lockfile".
• is_safe_install_folder_name (src/install/dependency.rs) rejects empty names, ./.. or empty /-separated segments, and segments containing \, : or NUL; @scope/name passes.
• Package::parse_with_json_impl is the one function that turns a package.json into a lockfile Package for everything that is not a registry manifest: the root (Features::MAIN, the only caller with is_main), workspace members, file: folders, link: targets, and the package.json extracted from tarball and git dependencies. That is why the check lives there.
• PackageManager::crash() prints the accumulated install log to stderr (unless --silent) and exits 1; Global::crash() just exits 1.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 26 seconds

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: 1b9a9ea0-2456-420e-8d8b-e48ae636e489

📥 Commits

Reviewing files that changed from the base of the PR and between 2f5c180 and 88372c2.

📒 Files selected for processing (3)

• src/install/PackageManager/processDependencyList.rs
• src/install/lockfile/Package.rs
• test/cli/install/bun-install.test.ts

---

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

[robobun]

Status: ready for review.

Reproduced on the released binary (1.4.0-canary.1) with a local tarball and with a file: folder whose package.json name is "a:b": the first bun install exits 0 and writes "x": ["a:b@../evil.tgz", ...], the second prints error: Invalid package name / warn: Ignoring lockfile, and bun pm ls fails with InvalidLockfile.

With this change the install fails with error: Invalid package name "a:b" pointing at the dependency's package.json and no lockfile is written. Tests: test/cli/install/bun-install.test.ts, describe a dependency whose own package.json has an invalid name (the four rejection cases fail on the released binary, the two controls pass on both).

[claude]

LGTM — a focused validation fix that reuses the lockfile parser's own predicate so bun never writes a lockfile it can't reload.

What was reviewed:

• The new check in parse_with_json_impl gates on !FEATURES.is_main; verified only Features::MAIN sets is_main: true, so workspace/folder/tarball/git all pass through the check while the root stays exempt.
• Global::crash() → self.crash() in the git and tarball arms: confirmed PackageManager::crash prints the accumulated log then calls Global::crash(), and the remaining Global::crash() in the npm arm keeps the Global import live.
• Error uses existing machinery (log.add_error_fmt, value_loc_of, bun_core::fmt::quote, InvalidPackageJSON) matching sibling error paths in the same file.
• Tests cover tarball/folder/git rejection, escaping of the echoed name, plus positive controls (scoped names still install and --frozen-lockfile reloads; invalid root name still accepted); test helpers createDumbHttpGitRepo/serveDirectory are reused from the same file.

Extended reasoning...

Overview

Three files touched. src/install/lockfile/Package.rs gains a 10-line guard inside the 'name: block of parse_with_json_impl: for any non-root package.json, the name field is now run through dependency::is_safe_install_folder_name, and on failure an Invalid package name "…" error is logged (pointing at the value's location via value_loc_of) and InvalidPackageJSON is returned. src/install/PackageManager/processDependencyList.rs swaps two Global::crash() calls for self.crash() in the git and tarball arms of process_extracted_tarball_package, so the newly logged error (and any other InvalidPackageJSON reason from a tarball) is actually printed before exit. test/cli/install/bun-install.test.ts adds a six-test describe.concurrent block.

Security risks

None introduced. The change tightens validation of a name string that comes from an untrusted package.json before it is written into the lockfile and later used as a filesystem path. It reuses is_safe_install_folder_name, the same predicate the lockfile loader and isolated linker already enforce (rejects empty, ., .., empty segments, and any \\, : or NUL). The rejected name is echoed via bun_core::fmt::quote, which JSON-escapes control characters, so the error message can't be used for terminal injection. Failing closed here is strictly safer than the previous behavior of writing an unloadable lockfile.

Level of scrutiny

Low-to-medium. This is package-manager code, but the change is a small, guarded early-return that reuses an existing, well-exercised predicate rather than introducing new parsing logic. The is_main gate is the established discriminator for root vs. dependency in this function (it already drives meta.origin a few lines above and several later branches). The Global::crash() → self.crash() swap is a strict superset: PackageManager::crash prints the log (unless --silent) and then calls Global::crash(). I checked that the string_buf shared borrow in each arm is scoped inside the if log_level != Silent block, so self.crash()'s &mut self receiver is unencumbered.

Other factors

Test coverage is thorough for the size of the change: four rejection cases (local tarball, file: folder with location assertion, NUL-byte escaping, git via a local dumb-http repo — no external network) and two positive controls proving scoped names still install and round-trip through --frozen-lockfile, and that an invalid root name remains accepted. The tests reuse existing helpers from the same file and follow the harness conventions (tempDir, bunEnv spread, drain stdout/stderr/exited concurrently, assert output before exit code). The PR description explicitly scopes out the empty-name / @-in-name lockfile encoding issue as a separate concern, which is the right call — that's a serialization format problem, not a name-validity one.