Skip to content

install: fix yarn.lock migration panic on tarball URLs with "/-/" right after the host #38803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
+82 −15
Open

install: fix yarn.lock migration panic on tarball URLs with "/-/" right after the host #38803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 14 additions & 15 deletions src/install/yarn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,18 @@
strings::without_trailing_slash(path)
}

/// `https://registry.npmjs.org/@scope/name/-/name-1.0.0.tgz` -> `@scope/name`
pub(crate) fn get_package_name_from_default_registry_url(url: &[u8]) -> Option<&[u8]> {
let host_and_path = url
.strip_prefix(b"https://")
.or_else(|| url.strip_prefix(b"http://"))?;
let path = host_and_path
.strip_prefix(b"registry.npmjs.org/")
.or_else(|| host_and_path.strip_prefix(b"registry.yarnpkg.com/"))?;
let name = &path[..strings::index_of(path, b"/-/")?];
(!name.is_empty()).then_some(name)

Check warning on line 171 in src/install/yarn.rs

View check run for this annotation

Claude / Claude Code Review

Scoped package named '-' extracts as bare '@scope' 

The scoped sibling of the `dash-package` row isn't handled: for `https://registry.npmjs.org/@scope/-/-/--0.0.1.tgz`, the host-stripped path is `@scope/-/-/--0.0.1.tgz` and `index_of(path, b"/-/")` matches at byte 6 (inside the name), so the extracted name is `@scope` instead of `@scope/-`. Fix: when `path` starts with `@`, skip past the first `/` before searching for `/-/`. Extreme edge case (npm permits `@scope/-` by the same rule that permits the unscoped `-` package this PR already tests), no
Comment on lines +170 to +171

@claude claude Bot Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The scoped sibling of the dash-package row isn't handled: for https://registry.npmjs.org/@scope/-/-/--0.0.1.tgz, the host-stripped path is @scope/-/-/--0.0.1.tgz and index_of(path, b"/-/") matches at byte 6 (inside the name), so the extracted name is @scope instead of @scope/-. Fix: when path starts with @, skip past the first / before searching for /-/. Extreme edge case (npm permits @scope/- by the same rule that permits the unscoped - package this PR already tests), no crash — just noting it since the PR explicitly enumerates this input class.

Extended reasoning...

What the bug is

Entry::get_package_name_from_default_registry_url (src/install/yarn.rs:170) strips the scheme and host, then takes everything before the first /-/ in the remaining path as the package name:

let name = &path[..strings::index_of(path, b"/-/")?];

For a URL dependency on a scoped package literally named -, the tarball URL is https://registry.npmjs.org/@scope/-/-/--0.0.1.tgz. After stripping https://registry.npmjs.org/, path = "@scope/-/-/--0.0.1.tgz". The bytes /-/ first appear at index 6 — the slash between @scope and its bare-name segment -, followed by that -, followed by the actual separator's leading slash — so name = path[..6] = "@scope", a bare scope with no package part.

Step-by-step trace

path = "@scope/-/-/--0.0.1.tgz":

index 0 1 2 3 4 5 6 7 8 9 10 11…
byte @ s c o p e / - / - / --0.0.1.tgz
  • strings::index_of(path, b"/-/") scans left-to-right and matches bytes 6–8 (/, -, /).
  • name = &path[..6] = b"@scope".
  • The correct separator is at index 8, which would give &path[..8] = b"@scope/-".

The unscoped - package (the dash-package test row) works because its path is -/-/--0.0.1.tgz, where the first /-/ at index 1 correctly yields - — there is no scope segment for the search to land inside.

Why nothing else prevents it

The caller at yarn.rs:962 uses the returned name directly (break 'blk name) with no further validation, so @scope becomes the package's recorded name in bun.lock ("@scope@https://…"). get_package_name_from_resolved_url (the sibling parser used for aliases) has the identical limitation for this input — it also searches for the first /-/ before splitting on the last slash — so nothing downstream corrects it.

Why it's the same input class this PR enumerates

The PR description explicitly calls out "the package literally named -" as "the one real package whose URL starts with /-/", adds it as the dash-package test row, and hardens the parser against it. npm's naming rules permit @scope/- by exactly the same rule that permits unscoped - (a name may not start with . or _; a lone - is fine). Per REVIEW.md's "Cover the variant matrix, not just the repro" and "Fix the whole class in the same PR", the scoped variant is the direct sibling of a case the PR chose to enumerate.

Impact

No crash — the slice bounds are ordered, so the PR's primary goal (never panic) is preserved. The migrated bun.lock records the entry as @scope@<url> instead of @scope/-@<url>. For a RemoteTarball resolution the name is largely a display label (installation fetches the URL directly), so the practical fallout is a wrong name in the lockfile rather than a failed install. Reachable only via a URL dependency on such a package or a hand-crafted yarn.lock — the same adversarial-input surface the rest of this PR hardens.

How to fix

When path starts with @, skip past the first / (the scope/name boundary) before searching for /-/, then include the scope prefix in the returned slice:

let sep_search_start = if path.first() == Some(&b'@') {
    strings::index_of(path, b"/")? + 1
} else {
    0
};
let sep = sep_search_start + strings::index_of(&path[sep_search_start..], b"/-/")?;
let name = &path[..sep];
(!name.is_empty()).then_some(name)

and add a "scoped-dash-package": "https://registry.npmjs.org/@scope/-/-/--0.0.1.tgz" row alongside dash-package in the new test, expecting "@scope/-@…".

}

Check failure on line 172 in src/install/yarn.rs

View check run for this annotation

Claude / Claude Code Review

PR description claims fixes and test coverage not present in the commit 

The PR description and the 06:53/09:52 timeline comments describe a rework at 12002d9 (modifying `get_package_name_from_resolved_url` to skip `<scheme>://<host>/`, adding `Entry::is_default_registry_url`, fixing the `registry.yarnpkg.com@1.0.0` alias case, and adding `pinned` + alias test rows), but ef0d36cc has none of that — it adds a separate `get_package_name_from_default_registry_url` helper called only from the `name_to_use` block, leaves `get_package_name_from_resolved_url` untouched, and
Comment on lines +162 to +172

@claude claude Bot Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The PR description and the 06:53/09:52 timeline comments describe a rework at 12002d9 (modifying get_package_name_from_resolved_url to skip <scheme>://<host>/, adding Entry::is_default_registry_url, fixing the registry.yarnpkg.com@1.0.0 alias case, and adding pinned + alias test rows), but ef0d36c has none of that — it adds a separate get_package_name_from_default_registry_url helper called only from the name_to_use block, leaves get_package_name_from_resolved_url untouched, and the test has only the nine URL-dependency rows. This looks like the reworked implementation was dropped in the rebase/squash to ef0d36c; either restore it, or rescope the description so it doesn't claim to fix the alias sibling or to test it (CLAUDE.md #11).

Extended reasoning...

What the mismatch is

The PR description (updated for 12002d9 per the 01:39 status comment) and the author's 06:53 timeline comment both describe a specific reworked shape:

  1. "get_package_name_from_resolved_url now skips <scheme>://<host>/ before looking for /-/" — so the alias call sites also stop returning the host as a package name.
  2. "behind a new Entry::is_default_registry_url check" — a separate predicate gating the name_to_use block.
  3. The alias bug is fixed — an npm: alias whose tarball URL has /-/ right after the host "previously registry.yarnpkg.com@1.0.0".
  4. The test has "a semver entry resolved to such a tarball" (the pinned row) and "an alias with /-/ after the host", checked "through a loopback registry, the name the alias was given".

The 09:52 comment even refers to "the pinned row of the new test" when discussing the #38795 rebase.

None of this is in ef0d36c — the commit CodeRabbit and the robobun build comment both name as the current head.

Step-by-step check against ef0d36c

(1) get_package_name_from_resolved_url is untouched. The diff has exactly two hunks in src/install/yarn.rs: adding the new helper get_package_name_from_default_registry_url at lines 162-172, and rewiring the name_to_use block to call it. The existing parser at lines 255-266 still starts with:

let path = &url[..strings::index_of(url, b"/-/")?];
let (prefix, name) = strings::rsplit_once_char(path, b'/')?;

For https://registry.yarnpkg.com/-/foo-1.0.0.tgz: index_of finds /-/ at 27, so path = b"https://registry.yarnpkg.com"; rsplit_once_char on / gives prefix = b"https:/", name = b"registry.yarnpkg.com". The function returns Some(b"registry.yarnpkg.com"). The three alias call sites (the is_npm_alias branch of the package-id loop, the base_name computation, and the trailing alias-registration loop) all still call this function, so an npm alias with that resolved URL is still named after the host — the third bullet in the PR description's Problem section is not fixed.

(2) No is_default_registry_url exists. grep -n is_default_registry_url src/install/yarn.rs returns nothing. The host check is folded into the new get_package_name_from_default_registry_url helper instead — the earlier "second helper" shape the 06:53 comment says was replaced ("Instead of adding a second URL-to-name helper next to get_package_name_from_resolved_url, the name_to_use block now calls that parser").

(3) The description says the name_to_use block "names the entry with Entry::get_package_name_from_resolved_url, the same parser npm: aliases use". It doesn't — it calls the new separate helper, so nothing is shared with the alias call sites.

(4) The test has exactly nine rows, all direct URL dependencies in the tarballs object. There is no pinned (semver) row, no npm: alias row, and the only registry assertion is expect(registryRequests).toStrictEqual([]) — nothing about "the name the alias was given". The description's Verified section explicitly claims all of these exist and were checked individually against main.

Why this matters

This is not a case of an imprecise description. The 06:53 comment is the author's own record of having reworked the implementation at 12002d9, and the description was updated to match. The 09:52 comment (posted after ef0d36c was already building at 8:52 per the robobun status) references a test row that doesn't exist. The commit at HEAD has the earlier shape the 06:53 comment says was replaced. This is the signature of a rebase/squash that dropped the reworked hunks.

If merged as-is:

  • The panic fix works, but the alias sibling — a same-class site with the identical /-/-after-host defect the PR title names, which the description explicitly commits to fixing — stays broken and untested (REVIEW.md, "Fix the whole class in the same PR").
  • The PR record permanently claims to fix and test something it doesn't (CLAUDE.md Convert all stored paths in .bun to project-relative #11: "never overstate what you got done").
  • Work the author believes is in the PR is silently discarded.

How to fix

One of:

  • Restore the dropped rework: modify get_package_name_from_resolved_url to strip <scheme>://<host>/ before the /-/ search, add Entry::is_default_registry_url, wire the name_to_use block to the shared parser behind that gate, delete get_package_name_from_default_registry_url, and add the pinned semver row and the alias row (with the loopback-registry manifest-name assertion) to the test — as the 06:53 comment and the description already describe.
  • Rescope: keep the current diff, but drop the third Problem bullet, the "same parser npm: aliases use" and is_default_registry_url sentences from Fix, and the pinned/alias claims from Verified. Note in the description that the alias sibling is intentionally left for install: name yarn.lock npm: alias packages after the alias spec, not the tarball URL #38948 (which per the description deletes get_package_name_from_resolved_url entirely).


pub(crate) fn parse_git_url(
_yarn_lock: &YarnLock<'a>,
version: &'a [u8],
Expand Down Expand Up @@ -947,23 +959,10 @@
|| Entry::is_remote_tarball(resolved)
|| resolved.ends_with(b".tgz")
{
// https://registry.npmjs.org/package/-/package-version.tgz
if strings::index_of(resolved, b"registry.npmjs.org/").is_some()
|| strings::index_of(resolved, b"registry.yarnpkg.com/").is_some()
if let Some(name) = Entry::get_package_name_from_default_registry_url(resolved)
{
if let Some(separator_idx) = strings::index_of(resolved, b"/-/") {
if let Some(registry_idx) = strings::index_of(resolved, b"registry.") {
let after_registry = &resolved[registry_idx..];
if let Some(domain_slash) = strings::index_of(after_registry, b"/")
{
let package_start = registry_idx + domain_slash + 1;
let extracted_name = &resolved[package_start..separator_idx];
break 'blk extracted_name;
}
}
}
break 'blk name;
}
break 'blk base_name;
}
}
break 'blk base_name;
Expand Down
68 changes: 68 additions & 0 deletions test/cli/install/migration/yarn-lock-migration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -903,6 +903,74 @@ needs-node-types@1.0.0:
expect([...requestedManifests].sort()).toStrictEqual(["@types/node", "needs-node-types"]);
});

test("package names are only read from the path of default registry tarball URLs", async () => {
// Dependencies declared as tarball URLs. A tarball on registry.npmjs.org / registry.yarnpkg.com
// is named after the path segment(s) before "/-/"; every other URL keeps the dependency's name.
const tarballs = {
"host-after-separator": "https://evil.example/-/registry.npmjs.org/x.tgz",
"nothing-before-separator": "https://registry.npmjs.org/-/y.tgz",
"empty-segment-before-separator": "https://registry.npmjs.org//-/z.tgz",
"mirror-with-registry-in-path": "https://registry.mirror.example/registry.npmjs.org/other/-/other-1.0.0.tgz",
"dash-package": "https://registry.npmjs.org/-/-/--0.0.1.tgz",
"scoped-npmjs-https": "https://registry.npmjs.org/@scope/real/-/real-1.0.0.tgz",
"npmjs-http": "http://registry.npmjs.org/real-a/-/real-a-1.0.0.tgz",
"yarnpkg-https": "https://registry.yarnpkg.com/real-b/-/real-b-1.0.0.tgz",
"yarnpkg-http": "http://registry.yarnpkg.com/real-c/-/real-c-1.0.0.tgz",
};

await using tmpDir = tempDir("yarn-migration-tarball-names", {
"package.json": JSON.stringify({ name: "tarball-names-test", version: "1.0.0", dependencies: tarballs }, null, 2),
"yarn.lock":
"# yarn lockfile v1\n\n\n" +
Object.entries(tarballs)
.map(([name, url]) => `"${name}@${url}":\n version "1.0.0"\n resolved "${url}"\n`)
.join("\n"),
});

// Every entry is a tarball package, so the migration has no registry manifests to fetch.
const registryRequests: string[] = [];
await using registry = Bun.serve({
port: 0,
fetch(req) {
registryRequests.push(new URL(req.url).pathname);
return new Response("not found", { status: 404 });
},
});

await using proc = Bun.spawn({
cmd: [bunExe(), "pm", "migrate", "-f"],
cwd: tmpDir,
env: {
...bunEnv,
BUN_CONFIG_REGISTRY: registry.url.href,
BUN_INSTALL_CACHE_DIR: join(tmpDir, ".bun-cache"),
},
stdout: "pipe",
stderr: "pipe",
stdin: "ignore",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(exitCode, stdout + stderr).toBe(0);

const lock = Bun.JSONC.parse(await Bun.file(join(tmpDir, "bun.lock")).text()) as { packages: unknown };
expect(lock.packages).toStrictEqual({
"host-after-separator": [`host-after-separator@${tarballs["host-after-separator"]}`, {}],
"nothing-before-separator": [`nothing-before-separator@${tarballs["nothing-before-separator"]}`, {}],
"empty-segment-before-separator": [
`empty-segment-before-separator@${tarballs["empty-segment-before-separator"]}`,
{},
],
"mirror-with-registry-in-path": [`mirror-with-registry-in-path@${tarballs["mirror-with-registry-in-path"]}`, {}],
"dash-package": [`-@${tarballs["dash-package"]}`, {}],
"scoped-npmjs-https": [`@scope/real@${tarballs["scoped-npmjs-https"]}`, {}],
"npmjs-http": [`real-a@${tarballs["npmjs-http"]}`, {}],
"yarnpkg-https": [`real-b@${tarballs["yarnpkg-https"]}`, {}],
"yarnpkg-http": [`real-c@${tarballs["yarnpkg-http"]}`, {}],
});
expect(registryRequests).toStrictEqual([]);
});

test("yarn.lock with resolutions", async () => {
await using tmpDir = tempDir("yarn-migration-resolutions", {
"package.json": JSON.stringify(
Expand Down
Loading