Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
56 changes: 32 additions & 24 deletions src/install/yarn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,15 @@
version.starts_with(b"https://") && version.ends_with(b".tgz")
}

/// yarn v1 writes tarball `resolved` fields as `<url>#<sha1 of the tarball>`.
/// The hash is yarn's integrity annotation, not part of the URL.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub(crate) fn url_without_hash(resolved: &[u8]) -> &[u8] {
match strings::index_of_char_usize(resolved, b'#') {
Some(hash_idx) => &resolved[..hash_idx],
None => resolved,
}
}

pub(crate) fn is_workspace_dependency(version: &[u8]) -> bool {
version.starts_with(b"workspace:") || version == b"*"
}
Expand Down Expand Up @@ -957,28 +966,21 @@
let name_to_use: &[u8] = 'blk: {
if entry.commit.is_some() && entry.git_repo_name.is_some() {
break 'blk entry.git_repo_name.as_deref().unwrap();
} else if let Some(resolved) = entry.resolved.as_deref() {
if is_direct_url_dep
|| Entry::is_remote_tarball(resolved)
|| resolved.ends_with(b".tgz")
} else if let (true, Some(resolved)) = (is_direct_url_dep, entry.resolved.as_deref()) {
// 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()
{
// 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(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;
}
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 base_name;
}
}
break 'blk base_name;
Expand Down Expand Up @@ -1049,16 +1051,12 @@
}
break 'blk Resolution::default();
} else if let Some(resolved) = entry.resolved.as_deref() {
if is_direct_url_dep {
break 'blk Resolution::init(ResolutionValue::RemoteTarball(
sbuf!().append(resolved)?,
));
}
let resolved = Entry::url_without_hash(resolved);

// Yarn v1 lockfiles legitimately contain entries without an integrity field
// (workspace deps, file:, codeload tarballs), so migration intentionally
// accepts off-registry tarball URLs without integrity instead of failing.
if Entry::is_remote_tarball(resolved) || resolved.ends_with(b".tgz") {
if is_direct_url_dep {

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

View check run for this annotation

Claude / Claude Code Review

Stale comment: 'codeload tarballs' rationale now attached to wrong block

This comment used to sit directly above the `Entry::is_remote_tarball(resolved) || resolved.ends_with(b".tgz")` fallback it justifies — the PR moved that fallback down into the `!result.valid` branch (line 1069) but left the comment attached to the new `if is_direct_url_dep` check, whose condition none of the cited examples (workspace deps, `file:`, codeload tarballs) reach. Move it down to the `.tgz` fallback it still describes, or drop it.
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
break 'blk Resolution::init(ResolutionValue::RemoteTarball(
sbuf!().append(resolved)?,
));
Expand All @@ -1068,9 +1066,19 @@
let result =
Semver::Version::parse(version.sliced(this.buffers.string_bytes.as_slice()));
if !result.valid {
if Entry::is_remote_tarball(resolved) || resolved.ends_with(b".tgz") {
break 'blk Resolution::init(ResolutionValue::RemoteTarball(
sbuf!().append(resolved)?,
));
}
break 'blk Resolution::default();
}

// A versioned entry is a registry package whose tarball is `resolved`,
// even when nothing follows `.tgz`: yarn only appends the `#sha1` when
// the registry reported a shasum. `has_trusted_dependency` compares this
// URL against the canonical registry tarball URL, so it must be stored
// the way a fresh install would have recorded it.
Comment thread
robobun marked this conversation as resolved.
Outdated
let is_default_registry = resolved.starts_with(b"https://registry.yarnpkg.com/")
|| resolved.starts_with(b"https://registry.npmjs.org/");

Expand Down
100 changes: 100 additions & 0 deletions test/cli/install/bun-install-lifecycle-scripts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,106 @@ test.concurrent("default trusted dependencies require the canonical registry tar
expect(await exited).toBe(0);
});

describe("default trusted dependencies after yarn.lock migration", () => {
const electronIntegrity =
"sha512-GkuwCdn6o8Krsxb3DIIqYP+TAi8Y5jYUadmseZ6nR2op2k5ssdKRYo4JjYDGopa1ACrGAcQuWViz/+vX/WjYnA==";

// Installs `electron@1.0.0` (on the default trusted list) from a yarn.lock
// that resolves it to `resolved`, and returns the migrated bun.lock.
async function installFromYarnLock(ctx: TestCtx, resolved: string, integrity: string) {
const { packageDir, packageJson, env } = ctx;
await Promise.all([
writeFile(
packageJson,
JSON.stringify({
name: "foo",
version: "1.0.0",
dependencies: {
"electron": "1.0.0",
},
}),
),
writeFile(
join(packageDir, "yarn.lock"),
`# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1


electron@1.0.0:
version "1.0.0"
resolved "${resolved}"
integrity ${integrity}
`,
),
]);

await using proc = spawn({
cmd: [bunExe(), "install"],
cwd: packageDir,
stdout: "pipe",
stdin: "ignore",
stderr: "pipe",
env,
});

const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(err).toContain("migrated lockfile from yarn.lock");
expect(err).not.toContain("error:");
expect(await exists(join(packageDir, "node_modules", "electron", "package.json"))).toBeTrue();
expect(exitCode).toBe(0);
return { out, lockfile: await file(join(packageDir, "bun.lock")).text() };
}

// yarn v1 writes the tarball's sha1 as a URL fragment after registry tarball
// URLs. Other producers of the format (bun's own `--yarn` output, registries
// without a shasum) omit it. Both shapes describe the canonical registry
// tarball for electron@1.0.0, so both must keep the default-trusted grant
// after migration, exactly like an install that never had a yarn.lock.
test.concurrent.each([
["with a sha1 fragment", "#f1b8bc2c23cd7e4f1500669dfaf8757578d2e391"],
["without a fragment", ""],
])("scripts run for the canonical registry tarball URL %s", async (_, fragment) => {
using ctx = await setupTest();
const canonicalUrl = `http://localhost:${verdaccio.port}/electron/-/electron-1.0.0.tgz`;

const { out, lockfile } = await installFromYarnLock(ctx, `${canonicalUrl}${fragment}`, electronIntegrity);

expect(out).not.toContain("Blocked");
expect(await exists(join(ctx.packageDir, "node_modules", "electron", "preinstall.txt"))).toBeTrue();
// The migrated entry is an npm package whose tarball URL is the canonical
// one for the configured registry, with yarn's fragment dropped.
expect(lockfile).toContain(`"electron": ["electron@1.0.0", "${canonicalUrl}", {}, "${electronIntegrity}"]`);
});

test.concurrent("scripts stay blocked when the yarn.lock points the name at another tarball", async () => {
using ctx = await setupTest();
// Same registry, but the tarball of a different package (which has
// preinstall/install/postinstall scripts of its own) recorded under the
// default-trusted name. Migration must keep that URL, so the canonical URL
// check still denies the default grant.
const otherUrl = `http://localhost:${verdaccio.port}/all-lifecycle-scripts/-/all-lifecycle-scripts-1.0.0.tgz`;
const otherIntegrity =
"sha512-hgU56juWYnFOQ3byQuydEgugxd+iWvaWfaoGvly4k/AxehC3dhM6IhXoDc3K7b/n1mP/II8hGIjI+LmxXFNMlw==";

const { lockfile } = await installFromYarnLock(
ctx,
`${otherUrl}#91cd0bd6a450b21db0078b9118c54bc0a27fccb7`,
otherIntegrity,
);

expect(lockfile).toContain(`"electron": ["electron@1.0.0", "${otherUrl}", {}, "${otherIntegrity}"]`);
const electronDir = join(ctx.packageDir, "node_modules", "electron");
expect(
await Promise.all([
exists(join(electronDir, "install.js")),
exists(join(electronDir, "preinstall.txt")),
exists(join(electronDir, "install.txt")),
exists(join(electronDir, "postinstall.txt")),
]),
).toEqual([true, false, false, false]);
});
});

test.concurrent("binary lockfile trusted dependency entries require an exact name match", async () => {
using ctx = await setupTest();
const { packageDir, packageJson, env } = ctx;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2935,7 +2935,7 @@ exports[`bun pm migrate for existing yarn.lock yarn-cli-repo: yarn-cli-repo 1`]

"class-utils/define-property/is-descriptor/is-data-descriptor/kind-of": ["kind-of@3.2.2", "", { "dependencies": { "is-buffer": "^1.1.5" } }, "sha1-MeohpzS6ubuw8yRm2JOupR5KPGQ="],

"commitizen/inquirer/cli-cursor/restore-cursor/onetime": ["onetime@1.1.0", "http://registry.npmjs.org/onetime/-/onetime-1.1.0.tgz#a1f7838f8314c516f05ecefcbc4ccfe04b4ed789", {}, "sha1-ofeDj4MUxRbwXs78vEzP4EtO14k="],
"commitizen/inquirer/cli-cursor/restore-cursor/onetime": ["onetime@1.1.0", "http://registry.npmjs.org/onetime/-/onetime-1.1.0.tgz", {}, "sha1-ofeDj4MUxRbwXs78vEzP4EtO14k="],

"eslint/inquirer/chalk/supports-color/has-flag": ["has-flag@3.0.0", "", {}, "sha1-tdRU3CGZriJWmfNGfloH87lVuv0="],

Expand Down Expand Up @@ -3154,7 +3154,7 @@ exports[`bun pm migrate for existing yarn.lock yarn-stuff: yarn-stuff 1`] = `

"reg": ["abbrev@1.1.1", "", {}, "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q=="],

"remote": ["abbrev@https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz#f8f2c887ad10bf67f634f005b6987fed3179aac8", {}],
"remote": ["abbrev@https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz", {}],

"symlink": ["symlink@file:abbrev-link-target", {}],

Expand Down
100 changes: 100 additions & 0 deletions test/cli/install/migration/yarn-lock-migration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1547,3 +1547,103 @@ fsevents@^2.3.2:
expect(bunLockContent).toContain("@esbuild/darwin-arm64");
});
});

describe.concurrent("yarn.lock migration of registry tarball URLs", () => {
const integrity = {
parent: "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==",
leaf: "sha512-LEyx4aLEC3x6T0UguF6YILf+ntvmOaWsVfENmIW0E9H09vKlLDGelMjjSm0jkDHALj8A8quZ/HapKNigzwge+Q==",
};

async function migrate(dependencies: Record<string, string>, yarnLockEntries: string) {
// After migrating, bun fetches manifests from the configured registry to
// fill in bin/os/cpu. Point it at a local server that has nothing so the
// test stays off the network; the migrated resolutions don't depend on it.
using registry = Bun.serve({
port: 0,
fetch: () => new Response("not found", { status: 404 }),
});
await using dir = tempDir("yarn-migration-registry-urls", {
"package.json": JSON.stringify({ name: "registry-urls", version: "1.0.0", dependencies }),
"yarn.lock": `# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1


${yarnLockEntries}`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "pm", "migrate", "-f"],
cwd: String(dir),
env: { ...bunEnv, BUN_CONFIG_REGISTRY: registry.url.href },
stdout: "pipe",
stderr: "pipe",
stdin: "ignore",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stdout + stderr).toContain("migrated lockfile from yarn.lock");
expect(exitCode).toBe(0);
return fs.readFileSync(join(String(dir), "bun.lock"), "utf8");
}

test("private registry: yarn's #sha1 suffix is dropped and suffix-less URLs stay npm packages", async () => {
const registry = "https://npm.example.com";
const bunLock = await migrate(
{ parent: "^1.0.0" },
`parent@^1.0.0:
version "1.2.3"
resolved "${registry}/parent/-/parent-1.2.3.tgz#f8f2c887ad10bf67f634f005b6987fed3179aac8"
integrity ${integrity.parent}
dependencies:
"@scope/leaf" "~2.0.0"

"@scope/leaf@~2.0.0":
version "2.0.1"
resolved "${registry}/@scope/leaf/-/leaf-2.0.1.tgz"
integrity ${integrity.leaf}
`,
);

// Both entries are npm packages whose tarball URL is exactly what the
// registry would hand a fresh install: no `#sha1`, and no remote tarball
// entry (`parent@https://...`) just because yarn omitted the suffix.
expect(bunLock).toContain(
`"parent": ["parent@1.2.3", "${registry}/parent/-/parent-1.2.3.tgz", { "dependencies": { "@scope/leaf": "~2.0.0" } }, "${integrity.parent}"]`,
);
expect(bunLock).toContain(
`"@scope/leaf": ["@scope/leaf@2.0.1", "${registry}/@scope/leaf/-/leaf-2.0.1.tgz", {}, "${integrity.leaf}"]`,
);
});

test("default registry: a URL without yarn's #sha1 suffix is still the default registry", async () => {
// This is the shape `bun install --yarn` itself writes.
const bunLock = await migrate(
{ "@scope/leaf": "~2.0.0", "leaf-alias": "npm:leaf@^1.0.0" },
`"@scope/leaf@~2.0.0":
version "2.0.1"
resolved "https://registry.yarnpkg.com/@scope/leaf/-/leaf-2.0.1.tgz"
integrity ${integrity.leaf}

"leaf-alias@npm:leaf@^1.0.0":
version "1.0.0"
resolved "https://registry.npmjs.org/leaf/-/leaf-1.0.0.tgz"
integrity ${integrity.parent}
`,
);

expect(bunLock).toContain(`"@scope/leaf": ["@scope/leaf@2.0.1", "", {}, "${integrity.leaf}"]`);
expect(bunLock).toContain(`"leaf-alias": ["leaf@1.0.0", "", {}, "${integrity.parent}"]`);
});

test("a dependency declared as a tarball URL stays a remote tarball, without yarn's #sha1 suffix", async () => {
const url = "https://npm.example.com/leaf/-/leaf-2.0.1.tgz";
const bunLock = await migrate(
{ leaf: url },
`"leaf@${url}":
version "2.0.1"
resolved "${url}#f8f2c887ad10bf67f634f005b6987fed3179aac8"
`,
);

expect(bunLock).toContain(`"leaf": ["leaf@${url}", {}]`);
});
});