Skip to content

install: fix default-trusted lifecycle scripts being blocked after yarn.lock migration - #38795

Open
robobun wants to merge 7 commits into
mainfrom
farm/3e678c3b/yarn-migration-registry-url-hash
Open

install: fix default-trusted lifecycle scripts being blocked after yarn.lock migration#38795
robobun wants to merge 7 commits into
mainfrom
farm/3e678c3b/yarn-migration-registry-url-hash

Conversation

@robobun

@robobun robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Found while looking at lifecycle scripts after lockfile migration (the trustedDependencies side of that is #38773); this part is about the URLs the yarn.lock migrator records and is independent of it. Touches the same name_to_use block as #38803 (a crash fix in the name extraction); see "Relation to #38803" below.

Problem

  • After migrating a yarn.lock whose entries resolve to any registry other than registry.yarnpkg.com / registry.npmjs.org (a private registry, or the test Verdaccio), packages on bun's default trusted list (electron, esbuild, ...) have their lifecycle scripts blocked on the install that follows. The same bun install without the yarn.lock runs them.
  • yarn v1 writes tarballs as resolved "<url>#<sha1>". The migrator (src/install/yarn.rs, resolution block) stored that string verbatim as the package's npm tarball URL, so bun.lock ends up with
    "electron": ["electron@1.0.0", "http://localhost:PORT/electron/-/electron-1.0.0.tgz#f1b8bc2c...", {}, "sha512-..."].
    Lockfile::has_trusted_dependency (src/install/lockfile.rs:3105) only grants default-list trust when the stored URL equals the canonical <registry>/<name>/-/<name>-<version>.tgz, so the suffix fails the comparison.
  • Entries whose URL has no suffix (what bun install --yarn itself writes, and what registries without a shasum produce) took a different wrong turn: the resolved.ends_with(".tgz") check turned them into remote tarball packages ("electron": ["electron@http://.../electron-1.0.0.tgz", ...]), which are never default-trusted and also skip the post-migration bin/os/cpu manifest fill-in. package-lock.json and pnpm-lock.yaml migration already store the bare canonical URL for both shapes.
  • The same suffix on a GitHub archive download (resolved "https://github.com/o/r/archive/v1.tar.gz#<sha1>") was read as a git commit, because Entry::is_git_dependency matched every https://github.com/ URL. The entry migrated to ["r/archive/v1.tar.gz@github:o/r/archive/v1.tar.gz#<sha1 prefix>", {}, ""], which the next bun install --frozen-lockfile rejects as an invalid git tag.
  • The canonical-URL check landed in Hardening: input validation and bounds tightening across 36 subsystems (round 4) #31339 and is not in a release yet (LATEST is still 1.3.14). Every bun.lock the yarn migrator has written so far on a non-default registry carries the suffix, and a plain bun install keeps the recorded URL, so without a reader-side fix the first release with Hardening: input validation and bounds tightening across 36 subsystems (round 4) #31339 would start blocking those projects' default-trusted scripts even though this PR fixes new migrations.

Fix

  • Entry::url_without_hash strips the suffix; the migrator computes the stripped URL once per entry and uses it both to name the package and to build the resolution, so the npm URL and the remote tarball URL of direct URL dependencies are both stored without it.
  • A non-URL dependency with a semver version is always stored as an npm package whose tarball URL is resolved (empty for the two default registries, as before). The .tgz to remote-tarball fallback now only applies to entries whose version is not semver, the one case where it was not misclassifying registry packages. Direct URL dependencies (foo@https://... specs) still become remote tarballs.
  • is_git_dependency classifies https://github.com/... with the dependency parser's existing is_github_tarball_path rule (applied to the URL without the suffix), so archive downloads become remote tarballs like any other URL dependency and repositories are still git.
  • has_trusted_dependency compares the stored URL without its fragment. This is what keeps already-migrated lockfiles working once Hardening: input validation and bounds tightening across 36 subsystems (round 4) #31339 ships. It is exactly as strict as before: the request path bun sends is URL.pathname, which bun_url cuts at # (src/url/lib.rs:649-668, used by build_request in src/http/lib.rs), so <canonical>#anything downloads the canonical tarball and nothing else; a URL whose path differs still fails, fragment or not.
  • The name block now only reads a package name out of the URL for direct URL dependencies; registry entries take the spec's name, which is what every suffixed entry already did. With the reclassification above, a default-registry entry's name is what the download URL is built from, so it should come from the spec rather than from however the URL happened to spell it.
  • Verified (each new test fails on the build without src/ changes and passes with them, except the two guards noted):
    • test/cli/install/bun-install-lifecycle-scripts.test.ts, describe("default trusted dependencies after yarn.lock migration"): electron@1.0.0 via yarn.lock with and without the suffix runs its preinstall and records the canonical URL; a yarn.lock pointing electron at all-lifecycle-scripts's tarball keeps the scripts blocked; a hand-written bun.lock in the shape the migrator produced until now (...electron-1.0.0.tgz#<sha1>) runs the scripts, and the same lockfile pointing at the other package's tarball (also suffixed) stays blocked (guard, passes before and after). The pre-existing "require the canonical registry tarball URL" test (wrong path, wrong origin) still passes.
    • test/cli/install/migration/yarn-lock-migration.test.ts, describe("yarn.lock migration of registry tarball URLs"): bun pm migrate against a local 404 registry checks the migrated bun.lock for a private registry (suffixed and suffix-less, with a dependency edge between them), the default registry without a suffix (plain and npm: alias), a direct URL dependency on a registry host and on a GitHub archive, and a github.com repository URL still migrating as git (guard).
    • Existing snapshots: only two lines change, both losing a #sha1 suffix (onetime in yarn's own lockfile, an http://registry.npmjs.org/ URL, and the remote direct URL dependency in yarn-stuff). The other ~1000 entries of the yarn-cli-repo snapshot, including its git and codeload entries, are unchanged.
    • Also run: the whole yarn-lock-migration.test.ts file, nested-overrides.test.ts "yarn.lock resolutions paths become nested rules" (suffix-less local-registry URLs, migrate + install), the trust tests in bun-pm.test.ts and bun-install-registry.test.ts, and the full lifecycle scripts file (124 pass; the 3 failures need node on PATH and fail on main in this environment too).

Relation to #38803

Background

  • Default trusted dependencies: when package.json has no trustedDependencies, bun runs lifecycle scripts only for packages on a built-in list (src/install/default-trusted-dependencies.txt). Because the grant is keyed by name, has_trusted_dependency (since Hardening: input validation and bounds tightening across 36 subsystems (round 4) #31339) additionally requires the package to be an npm package whose tarball URL is either empty (default registry) or exactly the canonical tarball URL on the configured registry, so a lockfile cannot obtain the grant by recording a different tarball under a trusted name.
  • npm resolution URL: an npm package in the lockfile carries the tarball URL the registry reported (dist.tarball); bun.lock prints it as "" when it is under the default registry and verbatim otherwise. A remote tarball resolution is a different package kind used for dependencies declared as a URL; it has no version and no registry, so it never qualifies for the default list.
  • yarn v1 resolved field: <tarball url>#<hex sha1 of the tarball> for registry tarballs, URL dependencies and GitHub archives alike; the suffix is omitted when the registry did not report a shasum. For git dependencies the part after # is the commit instead. bun's own bun install --yarn printer writes URLs without a suffix.
  • URL fragment: the part of a URL after #. HTTP clients never transmit it; bun's URL parser puts it in hash and excludes it from pathname, which is what becomes the request path.
Earlier shape of this PR

The first revision only stripped the suffix in the migrator and reclassified suffix-less registry entries. Self-review turned up the GitHub archive case (same suffix, misread as a commit one layer up) and the population of lockfiles that were already migrated with the suffix, which the unreleased canonical-URL check would otherwise start blocking; both were folded in, the strip was hoisted so the name block sees the same URL as the resolution block, and the tests above were extended to cover them.


no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-lifecycle-scripts.test.ts

… registry entries as npm packages

yarn v1 writes registry and remote tarball URLs as <url>#<sha1>. The
yarn.lock migrator stored that string verbatim as the npm tarball URL of
packages on a non-default registry, so Lockfile::has_trusted_dependency,
which compares the URL against the canonical registry tarball URL, denied
default-trusted packages (electron, esbuild, ...) their lifecycle scripts
after migrating. When the suffix was absent (bun's own --yarn output,
registries without a shasum) the same entries were turned into remote
tarball packages instead, which are never default-trusted either.

Strip the suffix once, and treat every non-URL dependency with a semver
version as a registry package whose tarball is the resolved URL, the same
way the package-lock.json migrator does. Direct URL dependencies still
become remote tarballs (now without the suffix), and the package name is
only taken from the URL for those.
@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

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

Next review available in: 5 minutes

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: 5ca05315-3eaf-4897-b92e-25d790ec5edc

📥 Commits

Reviewing files that changed from the base of the PR and between 2c2ef7c and ec15522.

⛔ Files ignored due to path filters (1)
  • test/cli/install/migration/__snapshots__/yarn-lock-migration.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (5)
  • src/install/dependency.rs
  • src/install/lockfile.rs
  • src/install/yarn.rs
  • test/cli/install/bun-install-lifecycle-scripts.test.ts
  • test/cli/install/migration/yarn-lock-migration.test.ts

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

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Status: fix complete (code unchanged since 30251b7). Build 97776 passed all 177 lanes that ran and only failed because its two darwin aarch64 lanes expired waiting for an agent; the listed test failures all passed on retry and are in unrelated files. Re-ran CI once (ec15522, empty commit) to give those two lanes another attempt. Ready for review, in particular the has_trusted_dependency change described below and in the PR description.

Reproduced on the released build with the Verdaccio harness: package.json depending on electron@1.0.0 plus a yarn.lock resolving it to http://localhost:<port>/electron/-/electron-1.0.0.tgz#f1b8bc2c... (and, separately, the same URL without the suffix). bun install reports migrated lockfile from yarn.lock and exits 0, but node_modules/electron/preinstall.txt is never written; deleting the yarn.lock and installing again writes it. The migrated bun.lock shows why: the suffixed entry keeps ...tgz#f1b8bc2c... as its npm URL, the suffix-less one becomes electron@http://.../electron-1.0.0.tgz (a remote tarball entry). A bun.lock already written in the suffixed shape reproduces the blocked preinstall directly, and a GitHub archive resolved URL with the suffix migrates to an invalid github: entry.

With this branch all of those run the preinstall (or, for the archive, migrate to a plain tarball entry), while a lockfile pointing the electron name at another package's tarball stays blocked. The new tests in test/cli/install/bun-install-lifecycle-scripts.test.ts and test/cli/install/migration/yarn-lock-migration.test.ts fail on the build without the src/ changes and pass with them; the two snapshot changes are both a dropped #sha1 suffix. Overlaps with #38803 in one block, see the description.

Comment thread src/install/yarn.rs Outdated
Comment thread src/install/yarn.rs Outdated
Comment thread src/install/yarn.rs
Comment thread src/install/yarn.rs Outdated
Comment thread src/install/yarn.rs

@claude claude Bot left a comment

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.

I reviewed this PR and didn't find any bugs; the earlier stale-comment note has been addressed. Because the change widens the default-trusted-dependency grant for yarn-migrated lockfiles (from "always blocked" to "same as a fresh install"), a human sign-off on the trust-model reasoning would still be worthwhile.

What was reviewed:

  • url_without_hash and the reordered resolution block — verified the .tgz→RemoteTarball fallback now only fires when version isn't semver, and direct-URL deps still become RemoteTarball.
  • The name_to_use restriction to is_direct_url_dep — checked that suffixed and suffix-less registry entries now name identically from the spec, and the ~1000-entry yarn-cli-repo snapshot only changes two lines (both dropped fragments).
  • The negative lifecycle test (electron pointed at all-lifecycle-scripts's tarball) — confirms stripping the fragment doesn't let a non-canonical URL past has_trusted_dependency.
Extended reasoning...

Overview

The PR fixes yarn.lock migration in src/install/yarn.rs so that (a) yarn v1's #<sha1> fragment is stripped from stored tarball URLs via a new Entry::url_without_hash, and (b) registry entries with a valid semver version are always stored as npm packages rather than falling into the .tgz→RemoteTarball heuristic. The name-from-URL extraction in the name_to_use block is now gated on is_direct_url_dep so suffix-less and suffixed entries name the same way. Tests are added to bun-install-lifecycle-scripts.test.ts (Verdaccio, positive + negative trust cases) and yarn-lock-migration.test.ts (bun pm migrate shapes for private/default registries and direct-URL deps), plus two snapshot lines lose their #sha1 suffix.

Security risks

This is security-adjacent: has_trusted_dependency gates lifecycle-script execution for packages on the built-in trusted list by comparing the stored tarball URL against the canonical <registry>/<name>/-/<name>-<version>.tgz. Before this change, yarn-migrated entries either kept the #sha1 suffix (comparison fails → blocked) or became RemoteTarball (never trusted → blocked) — overly restrictive but safe. After this change, they store the bare canonical URL and pass the check, so scripts run. That widens the grant for migrated lockfiles to match what a fresh install already grants. The stored URL is still the one from yarn.lock, only the fragment is dropped, so a lockfile that records a non-canonical tarball under a trusted name still fails the comparison — the new "scripts stay blocked" test covers this. I don't see a bypass, but the direction of the change (less restrictive) on a security gate is why I'm deferring rather than approving.

Level of scrutiny

Medium-high. The Rust change itself is small and mechanical (strip a fragment, reorder two branches, tighten a condition), and the ~1000-entry yarn-cli-repo snapshot only moves two lines, which is strong evidence the classification change is narrow. But the effect is on the lifecycle-script trust path, and the name_to_use gating change also subtly alters package identity for suffix-less registry entries. A maintainer who owns the has_trusted_dependency model should confirm the reasoning.

Other factors

My earlier inline comment (misplaced code comment after the branch reshuffle) was addressed in 6a82230, and the comment-cop bot's length complaints were addressed in 0f4989b; all threads are resolved. Test coverage is thorough — both yarn.lock shapes (with/without fragment), private and default registries, an npm: alias, a direct-URL dep, a dependency edge between migrated entries, and a negative trust case. The PR description states all new tests fail on the released build and pass here, and the snapshot diff is minimal.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

On the trust question: the check in has_trusted_dependency is unchanged by this PR. The behavior change is that a yarn-migrated lockfile now records the same tarball URL a fresh bun install (or a package-lock.json migration) records, so it gets the same answer from that check instead of always failing it on a non-default registry. The stored URL is still whatever the yarn.lock said minus the #sha1, which is why the "scripts stay blocked" case (trusted name pointed at another package's tarball on the same registry) still fails the canonical-URL comparison. The Fix section of the description now spells this out for whoever signs off on it.

dylan-conway pushed a commit that referenced this pull request Aug 15, 2026
…d package (#38806)

### Problem
- Migrating a yarn v1 lockfile whose `npm:` alias points at a scoped
package records the unscoped tail of the name. For
`"my-node-types@npm:@types/node@20.11.5"` resolved from
`https://registry.yarnpkg.com/@types/node/-/node-20.11.5.tgz#...`,
`bun.lock` gets `"my-node-types": ["node@20.11.5", ...]` instead of
`["@types/node@20.11.5", ...]`, and the post-migration manifest pass
asks the registry for `node`. The same migration runs inside `bun
install` when only a `yarn.lock` exists, which is the `GET
https://registry.npmjs.org/monaco-editor-treemended/-/monaco-editor-treemended-1.83.16.tgz
- 404` reported in #27781. Reproduces on the released canary (b7a0431)
and on main.
- Cause: `Entry::get_package_name_from_resolved_url`
(`src/install/yarn.rs:239`), which names alias entries after their
tarball URL, walks back from `/-/` and checks the last path segment for
a leading `@`. In a scoped tarball URL the last segment is the unscoped
name and the scope is the segment before it, so the check never fires
and only the last segment is returned. The Zig original had the same
bug; #27782 and #27889 fixed it there and were closed when the Zig
sources went away.

### Fix
- `get_package_name_from_resolved_url` now splits the path before `/-/`
into its last segment and the one before it, and returns both when the
earlier one starts with `@`, otherwise just the last one. It returns
`None` (callers then fall back to the spec name) when there is no name
segment, where the old code returned an off-by-one slice or an empty
name.
- Why this is correct: the registry tarball layout is
`<registry>/<name>/-/<basename>-<version>.tgz`, and `<name>` is one
segment for plain packages and exactly two (`@scope/name`) for scoped
ones, so the segment before the name starting with `@` is the only thing
that distinguishes the two shapes; nothing else in the URL encodes it.
Plain packages take the same path as before, so their output is
unchanged.
- All three callers benefit: the package name written to the lockfile
(`yarn.rs:941`), the `(name, version)` key used to give the alias and a
plain entry of the same package one package id (`yarn.rs:843`), and the
alias detection that registers an entry under its spec name only when it
differs from the real name (`yarn.rs:1603`), which previously fired for
every scoped package.
- Left alone on purpose: `Entry::parse_npm_alias` also mis-splits
`npm:@scope/name@range` at the scope's `@`, but its only consumers are
the first dependency pass, whose slices are overwritten by the second
pass at `yarn.rs:1695` onward, and a `version` field spelled `npm:...`,
which yarn v1 never writes. Nothing observable depends on it.
- Test: `test/cli/install/migration/yarn-lock-migration.test.ts`,
"yarn.lock with npm aliases of a scoped package keep the scope". One
migration covers an unscoped alias, a scoped alias and a transitive
alias of `@types/node@20.11.5` (sharing one yarn entry, as yarn writes
patterns that resolve to the same tarball) next to a plain
`@types/node@18.19.0`, asserts the full `packages` object of the
resulting `bun.lock`, and runs against a loopback registry that records
which manifests the migration asks for (`@types/node` and the consumer,
no `node`). Fails on the released build with the three alias entries
reading `node@20.11.5`, passes with this change.
- The rest of `yarn-lock-migration.test.ts` (scoped-package and
real-world fixtures, 14 snapshots), `migrate.test.ts` and
`lockfile-only.test.ts` pass unchanged with the debug build; the
`yarn-cli-repo` output is byte-identical to its snapshot. That case
still brushes its 5s timeout under debug+ASAN, which is pre-existing and
tracked by #35377.
- No overlap with the other open yarn migration PRs: #38795 and #38803
rework the `name_to_use` block for tarball entries and leave this
function as is (after #38795 it becomes the only source of alias package
names).

Fixes #27781

### Background
- A yarn v1 `npm:` alias entry is keyed by the alias spec
(`alias@npm:real-name@range`) and carries the real package's `version`,
`resolved` tarball URL and `integrity`. The migrator stores the package
under its real name and hangs it in the tree under the alias, so it
needs the real name, and it currently recovers it from the tarball URL.
- `bun.lock` `packages` entries are `["<real name>@<version>",
"<registry, empty for the default>", {meta}, "<integrity>"]` keyed by
the node_modules path. The first element is what `bun install` later
downloads, which is why a wrong name there turns into a 404 or a
different package.
- After migrating,
`fetch_necessary_package_metadata_after_yarn_or_pnpm_migration` fetches
the manifest of every migrated npm package by the name recorded in the
lockfile to fill in bin/os/cpu. The test points that pass at a loopback
server and uses the requested names as a second, external view of which
package each alias was resolved to.
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:13 PM PT - Aug 15th, 2026

🔄 @robobun, the build for your commit ec155226 (Build #98304) was cancelled — waiting for the next build...

… tarball downloads as git dependencies

The name block and the resolution block now share one hash-stripped URL.
A resolved field like https://github.com/o/r/archive/v1.tar.gz#<sha1> was
matched by the bare https://github.com/ prefix and its sha1 taken as a
commit, producing a github resolution with a bogus tag; classify those
with the same github tarball path rule the dependency parser uses.
…ackage's tarball URL

bun.lock files written by the yarn.lock migrator so far carry the suffix
on every non-default-registry URL. The fragment never reaches the
registry, so comparing without it keeps the canonical-URL check as
strict as before while letting those lockfiles keep running the scripts
of default-trusted packages.
Comment thread src/install/lockfile.rs Outdated
Comment thread src/install/yarn.rs Outdated
Comment thread src/install/lockfile.rs
@robobun robobun changed the title install: drop yarn's #sha1 suffix from migrated tarball URLs and keep registry entries as npm packages install: fix default-trusted lifecycle scripts being blocked after yarn.lock migration Aug 15, 2026

@claude claude Bot left a comment

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.

I reviewed this PR and didn't find any bugs. Because it relaxes the canonical-URL comparison in has_trusted_dependency (the gate that decides whether default-trusted lifecycle scripts run) and reorders the yarn migrator's resolution classification, a human sign-off is still warranted.

What was reviewed:

  • has_trusted_dependency: fragment-stripping is sound — bun_url excludes #… from pathname/request path, so <canonical>#anything fetches the same bytes; a differing path still fails. The negative test (trusted name → other package's tarball, with a fragment) confirms blocking is preserved.
  • is_git_dependency now defers to is_github_tarball_path on the hash-stripped path; .git repo URLs and codeload.github.com still classify as before (guard test + unchanged yarn-cli-repo snapshot entries).
  • The .tgz → RemoteTarball fallback moving behind !result.valid — checked that direct-URL specs and non-semver version entries still reach it; the yarn-stuff remote snapshot row only loses its #sha1.
Extended reasoning...

Overview

The PR fixes default-trusted lifecycle scripts being blocked after migrating a yarn.lock that resolved packages against a non-default registry. It touches three source files: src/install/yarn.rs (adds Entry::url_without_hash, computes the stripped URL once per entry, gates the URL-name-extraction on is_direct_url_dep, moves the .tgz → RemoteTarball fallback behind the invalid-semver branch, and makes is_git_dependency treat GitHub archive/tarball URLs as non-git via the existing is_github_tarball_path classifier), src/install/lockfile.rs (has_trusted_dependency now strips a URL fragment before comparing against the canonical registry tarball URL, so already-migrated lockfiles keep working once the unreleased #31339 check ships), and src/install/dependency.rs (visibility widening only). Tests cover both migration output shape and end-to-end lifecycle-script behavior, including negative guards; two snapshot lines change, both dropping a #sha1 suffix.

Security risks

has_trusted_dependency is the security gate that decides whether packages on the built-in default-trusted list may run install scripts without an explicit trustedDependencies opt-in. This PR relaxes its URL comparison by ignoring the fragment. The argument — that a URL fragment is never transmitted (bun's own URL parser cuts pathname at #, and build_request uses pathname) so <canonical>#x downloads exactly the canonical tarball — is correct, and the integrity check is unchanged. The PR includes a negative test showing a fragment-suffixed URL pointing at a different package's tarball is still blocked. I don't see a bypass here, but any relaxation of a lifecycle-script trust gate deserves a maintainer's explicit sign-off; the author's own comment ("spells this out for whoever signs off on it") anticipates that.

Level of scrutiny

High. The migrator classification changes are behavioral (registry entries with valid semver now always become npm packages rather than remote tarballs; https://github.com/... URLs are now split by the tarball-path predicate rather than blanket-treated as git), and the reader-side change touches a security check. The ~1000-line yarn-cli-repo snapshot being unchanged apart from one onetime row is good evidence the reclassification doesn't disturb the common shapes, but the interaction with #38803 (same name_to_use block) and the trust-gate change both call for a human look.

Other factors

  • CI passed on 0f4989b; later commits (fc7f7f9, 107a7bd, 30251b7) added the reader-side fragment handling and tightened comments.
  • My earlier inline comment (stale comment placement) was addressed in 6a82230. Remaining unresolved inline comments are from the automated comment-cop bot flagging 2–3-line comments; the author already justified keeping the cross-file-invariant ones and pushed 30251b7 to tighten them. I don't consider those blocking.
  • Tests are well-constructed: hermetic (Verdaccio / local 404 registry), concurrent, drain all pipes, include both positive and guard cases, and assert exact bun.lock entry shapes rather than substrings of the whole file.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Summary of what changed since the first review round, for whoever signs off: (1) has_trusted_dependency now ignores a URL fragment when comparing against the canonical tarball URL, so the lockfiles the migrator has already written (suffixed URLs) keep running default-trusted scripts once the #31339 check ships; the argument for why that is exactly as strict as before is in the Fix section, and the new lifecycle tests cover both the grant and the still-blocked case. (2) is_git_dependency no longer treats github.com archive downloads as repositories. (3) The hash strip happens once per entry so the name block and the resolution block see the same URL; that is also the only overlap with #38803, described at the bottom of the description. All review threads are resolved.

Build 97776 passed all 177 lanes that ran; its two darwin 14 aarch64 test
lanes expired waiting for an agent.

@claude claude Bot left a comment

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.

I reviewed this PR and didn't find any bugs; my earlier note about the stale comment on the .tgz fallback was addressed. Because the has_trusted_dependency change relaxes the canonical-URL gate that decides whether default-trusted lifecycle scripts run — and the author explicitly flagged that change for sign-off — a human look at that comparison would still be worthwhile.

What was reviewed:

  • url_without_hash in has_trusted_dependency: the fragment is dropped from the request path by bun's URL parser, so <canonical>#anything fetches the canonical tarball; a differing path still fails. The negative test (electron pointed at another package's tarball, with a fragment) confirms the deny case.
  • is_git_dependency now excludes github.com archive/tarball paths via the existing is_github_tarball_path; the guard test confirms .git repo URLs still classify as git.
  • Resolution-block reordering: the .tgz→RemoteTarball fallback now only fires when version isn't valid semver, so registry entries stay npm packages; direct-URL specs still become remote tarballs. The two snapshot changes are exactly the two dropped #sha1 suffixes.
Extended reasoning...

Overview

The PR fixes yarn.lock migration writing tarball URLs that fail has_trusted_dependency's canonical-URL check (from #31339), causing default-trusted packages' lifecycle scripts to be blocked after migration on non-default registries. It touches three source files: src/install/yarn.rs (strip yarn's #sha1 suffix once per entry, reorder the resolution block so semver-versioned entries always become npm packages, gate the URL-name-extraction on direct-URL deps only), src/install/lockfile.rs (compare the stored URL without its fragment in has_trusted_dependency), and src/install/dependency.rs (widen is_github_tarball_path to pub(crate)). Tests cover migration output for suffixed/suffix-less private-registry URLs, default-registry URLs, direct-URL deps, github archive downloads, github git repos, and end-to-end lifecycle-script behavior including a hand-written bun.lock in the shape older migrations produced.

Security risks

has_trusted_dependency is the gate that grants default-trusted lifecycle-script execution by package name. The change from url == canonical to url_without_hash(url) == canonical is a relaxation of a security check. The PR's argument — that bun's HTTP client never transmits the fragment, so <canonical>#anything requests exactly the canonical path — is sound and cited to src/url/lib.rs / src/http/lib.rs, and the test suite includes both a grant case and a still-blocked case (trusted name pointed at another package's tarball with a fragment). I did not find a way for the fragment strip to admit a URL whose request path differs from the canonical one. That said, this is exactly the kind of change REVIEW.md calls out under "Security checks fail closed" as needing explicit sign-off, and the author has asked for it.

Level of scrutiny

Medium-high. The migrator itself is not on a hot path and its output is validated by bun install, but the reader-side has_trusted_dependency change affects every install that consults the default-trusted list, not just migrated lockfiles. The resolution-block reordering also changes classification for a class of entries (suffix-less .tgz URLs with a semver version) from RemoteTarball to Npm; the ~1000-entry yarn-cli-repo snapshot changing by exactly the two expected lines is good evidence this doesn't regress other shapes.

Other factors

My prior inline comment (stale comment left on the wrong branch after the reorder) was addressed. All comment-cop threads are resolved. CI is green on 177 lanes. The PR description documents the one-block conflict with #38803 and how to resolve it. No CODEOWNERS file is present for these paths as far as I can tell. The remaining reason to defer is the security-gate relaxation, which the author themselves flagged for human sign-off.

Jarred-Sumner added a commit that referenced this pull request Aug 17, 2026
… expectations for combined behaviour in catalogs, pnpm migration and redacted logs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant