From 7b24d415a8b129157c6ef09595dbac3a3e1ed154 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 18:22:48 +0000 Subject: [PATCH 1/2] install: reject tarball, folder and git packages whose package.json name is invalid Package::parse copied the name out of a non-root package's package.json verbatim, so a tarball, folder, git or workspace package named e.g. "a:b" was written to bun.lock as "a:b@". The bun.lock parser rejects such names ("Invalid package name"), so every following install ignored the lockfile and rewrote it, --frozen-lockfile always failed and bun pm ls failed with InvalidLockfile. Apply the lockfile parser's check (dependency::is_safe_install_folder_name) when the name is read, log an error pointing at the name in that package.json, and fail the install before anything is saved. The root package is exempt: its name is not a bun.lock packages entry. The tarball and git arms of process_extracted_tarball_package exit through PackageManager::crash so the logged reason is printed before exiting. --- .../PackageManager/processDependencyList.rs | 4 +- src/install/lockfile/Package.rs | 10 ++ test/cli/install/bun-install.test.ts | 143 ++++++++++++++++++ 3 files changed, 155 insertions(+), 2 deletions(-) diff --git a/src/install/PackageManager/processDependencyList.rs b/src/install/PackageManager/processDependencyList.rs index 7fdd1f654dac..492110ad87af 100644 --- a/src/install/PackageManager/processDependencyList.rs +++ b/src/install/PackageManager/processDependencyList.rs @@ -167,7 +167,7 @@ impl PackageManager { format_args!("{}", resolution.fmt_url(string_buf)), ); } - Global::crash(); + self.crash(); } let has_scripts = pkg.scripts.has_any() || { @@ -264,7 +264,7 @@ impl PackageManager { err.name(), ); } - Global::crash(); + self.crash(); } let has_scripts = package.scripts.has_any() || { diff --git a/src/install/lockfile/Package.rs b/src/install/lockfile/Package.rs index 43b9edf56896..3f4bd10c96ec 100644 --- a/src/install/lockfile/Package.rs +++ b/src/install/lockfile/Package.rs @@ -2064,6 +2064,16 @@ impl Package { if let Some(name_q) = json.as_property(b"name") { if let Some(name) = name_q.expr.as_utf8(&bump) { if !name.is_empty() { + // Non-root names become bun.lock `packages` entries, which the + // lockfile parser rejects with the same check (see bun.lock.rs). + if !FEATURES.is_main && !dependency::is_safe_install_folder_name(name) { + log.add_error_fmt( + source, + value_loc_of(source, name_q.loc), + format_args!("Invalid package name {}", bun_core::fmt::quote(name)), + ); + return Err(crate::Error::InvalidPackageJSON); + } string_builder.count(name); break 'name; } diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index 0602dec36d5c..b08b814cf775 100644 --- a/test/cli/install/bun-install.test.ts +++ b/test/cli/install/bun-install.test.ts @@ -9696,6 +9696,149 @@ it("does not install transitive file: dependencies that point outside their pack expect(exitCode).toBe(1); }); +describe.concurrent("a dependency whose own package.json has an invalid name", () => { + // The name inside a tarball, folder or git dependency's package.json is written + // to bun.lock as `name@resolution`, and the bun.lock parser rejects names that + // are not safe install folder names. Saving such a name used to succeed and + // leave behind a lockfile that every later `bun install` ignored, so the + // install has to fail before the lockfile is saved. + async function install(root: string, ...args: string[]) { + await using proc = spawn({ + cmd: [bunExe(), "install", ...args], + cwd: join(root, "project"), + env: { ...env, BUN_INSTALL_CACHE_DIR: join(root, "cache") }, + stdout: "pipe", + stderr: "pipe", + }); + const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { out, err, exitCode }; + } + + async function expectRejected(root: string, quotedName: string) { + const { out, err, exitCode } = await install(root); + expect(err).toContain(`error: Invalid package name ${quotedName}`); + expect(out).not.toContain("1 package installed"); + expect(await exists(join(root, "project", "bun.lock"))).toBe(false); + expect(await exists(join(root, "project", "node_modules", "dep"))).toBe(false); + expect(exitCode).toBe(1); + return err; + } + + it("fails to install a local tarball", async () => { + using dir = tempDir("invalid-name-tarball", { + "project/package.json": JSON.stringify({ + name: "foo", + version: "0.0.1", + dependencies: { dep: "file:../dep.tgz" }, + }), + }); + await Bun.Archive.write( + join(String(dir), "dep.tgz"), + { "package/package.json": JSON.stringify({ name: "a:b", version: "1.0.0" }) }, + { compress: "gzip" }, + ); + + await expectRejected(String(dir), '"a:b"'); + }); + + it("fails to install a file: folder and points at the offending package.json", async () => { + using dir = tempDir("invalid-name-folder", { + "project/package.json": JSON.stringify({ + name: "foo", + version: "0.0.1", + dependencies: { dep: "file:../lib" }, + }), + "lib/package.json": JSON.stringify({ name: "lib/..", version: "1.0.0" }), + }); + + const err = await expectRejected(String(dir), '"lib/.."'); + expect(err).toMatch(/ at .*lib[\\/]package\.json:1:9\s/); + }); + + it("escapes the rejected name in the error message", async () => { + using dir = tempDir("invalid-name-escaped", { + "project/package.json": JSON.stringify({ + name: "foo", + version: "0.0.1", + dependencies: { dep: "file:../lib" }, + }), + "lib/package.json": JSON.stringify({ name: "a\0b", version: "1.0.0" }), + }); + + await expectRejected(String(dir), '"a\\u0000b"'); + }); + + it("fails to install a git dependency", async () => { + using dir = tempDir("invalid-name-git", { + "work/package.json": JSON.stringify({ name: "a\\b", version: "1.0.0" }), + }); + await createDumbHttpGitRepo(String(dir), {}); + using server = serveDirectory(String(dir)); + await write( + join(String(dir), "project", "package.json"), + JSON.stringify({ + name: "foo", + version: "0.0.1", + dependencies: { dep: `git+http://localhost:${server.port}/repo.git` }, + }), + ); + + await expectRejected(String(dir), '"a\\b"'); + }); + + it("still installs scoped names and writes a lockfile the next install loads", async () => { + using dir = tempDir("valid-scoped-name-tarball", { + "project/package.json": JSON.stringify({ + name: "foo", + version: "0.0.1", + dependencies: { dep: "file:../dep.tgz" }, + }), + }); + await Bun.Archive.write( + join(String(dir), "dep.tgz"), + { "package/package.json": JSON.stringify({ name: "@scope/dep", version: "1.0.0" }) }, + { compress: "gzip" }, + ); + + const first = await install(String(dir)); + expect(first.err).not.toContain("error:"); + expect(first.out).toContain("1 package installed"); + expect(first.exitCode).toBe(0); + expect(await file(join(String(dir), "project", "bun.lock")).text()).toContain('"dep": ["@scope/dep@../dep.tgz"'); + + const second = await install(String(dir), "--frozen-lockfile"); + expect(second.err).not.toContain("Ignoring lockfile"); + expect(second.err).not.toContain("error:"); + expect(second.exitCode).toBe(0); + }); + + it("does not apply to the root package's own name", async () => { + // The root is not a bun.lock `packages` entry; its name only appears in the + // `workspaces` section, which accepts it as-is. + using dir = tempDir("invalid-name-root", { + "project/package.json": JSON.stringify({ + name: "a:b", + version: "0.0.1", + dependencies: { dep: "file:../lib" }, + }), + "lib/package.json": JSON.stringify({ name: "lib", version: "1.0.0" }), + }); + + const first = await install(String(dir)); + expect(first.err).not.toContain("error:"); + expect(first.out).toContain("1 package installed"); + expect(first.exitCode).toBe(0); + const lockfile = await file(join(String(dir), "project", "bun.lock")).text(); + expect(lockfile).toContain('"name": "a:b"'); + expect(lockfile).toContain('"dep": ["lib@file:../lib", {}]'); + + const second = await install(String(dir), "--frozen-lockfile"); + expect(second.err).not.toContain("Ignoring lockfile"); + expect(second.err).not.toContain("error:"); + expect(second.exitCode).toBe(0); + }); +}); + it("does not install transitive file: dependencies with overlong folder targets", async () => { const overlongTarget = "file:./" + Buffer.alloc(120000, "a").toString(); using dir = tempDir("transitive-file-dep-overlong", { From 0847a60a414ea8fea4ba803c46ecf533c3950829 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:30:01 +0000 Subject: [PATCH 2/2] install: name folder, tarball and git packages without a package.json name after their source bun.lock stores a package as "@" and splits it apart again at the first "@" after an optional scope, so an empty name ("@file:../dir") or a name containing another "@" is written out but cannot be loaded back: every following install prints "Ignoring lockfile" and --frozen-lockfile always fails. Replace the git-only new_name hook on ResolverContext with fallback_name(), implemented by the git, tarball and folder/link resolvers: a package whose package.json has no name is named after the last component of its repository, tarball URL or folder (dependency::fallback_package_name), or "unnamed-package" when that is not storable either. Git packages keep the name they were given before; the SHA-1 fallback for an empty repository name is replaced by the shared one. The parse-time name check now uses is_safe_lockfile_package_name, which also rejects names with an extra "@", so folder, tarball, git and workspace packages with such a name fail to install instead of saving a lockfile that never loads. --- .../PackageManager/processDependencyList.rs | 58 +++----- src/install/PackageManager/runTasks.rs | 2 - src/install/dependency.rs | 30 ++++ src/install/lockfile/Package.rs | 131 ++++-------------- src/install/repository.rs | 51 ++----- src/install/resolution.rs | 11 -- src/install/resolvers/folder_resolver.rs | 9 ++ test/cli/install/bun-add.test.ts | 45 ++++++ test/cli/install/bun-install-git-deps.test.ts | 57 +++++++- test/cli/install/bun-install.test.ts | 49 +++++++ test/cli/install/bun-lock.test.ts | 97 +++++++++++++ test/cli/install/isolated-install.test.ts | 47 +++++++ 12 files changed, 382 insertions(+), 205 deletions(-) diff --git a/src/install/PackageManager/processDependencyList.rs b/src/install/PackageManager/processDependencyList.rs index 492110ad87af..92ca76b219fb 100644 --- a/src/install/PackageManager/processDependencyList.rs +++ b/src/install/PackageManager/processDependencyList.rs @@ -15,11 +15,11 @@ use crate::package_manager_real::options::LogLevel; use crate::package_manager_real::{ PackageManager, TaskCallbackList, enqueue, resolution as pm_resolution, }; -use crate::repository_real::{Repository, RepositoryExt as _}; +use crate::repository_real::RepositoryExt as _; use crate::resolution::{ResolutionType, Tag as ResolutionTag, TaggedValue}; use crate::{ - DependencyID, ExtractData, Features, INVALID_PACKAGE_ID, PackageID, Resolution, - TaskCallbackContext, initialize_store, + ExtractData, Features, INVALID_PACKAGE_ID, PackageID, Resolution, TaskCallbackContext, + dependency, initialize_store, }; // ────────────────────────────────────────────────────────────────────────── @@ -29,16 +29,12 @@ use crate::{ pub struct GitResolver<'a> { pub(crate) resolved: &'a [u8], pub(crate) resolution: &'a Resolution, - pub(crate) dep_id: DependencyID, - /// Owned scratch buffer that - /// `Package::parse_with_json` may assign when the package.json `name` - /// field is missing (see `ResolverContext::set_new_name`). - pub(crate) new_name: Vec, + /// `Repository::fallback_package_name`, copied out before parsing starts + /// because it is sliced from the lockfile string buffer parsing appends to. + pub(crate) fallback_name: &'a [u8], } impl<'a> ResolverContext for GitResolver<'a> { - const IS_GIT_RESOLVER: bool = true; - fn check_bundled_dependencies() -> bool { true } @@ -67,20 +63,8 @@ impl<'a> ResolverContext for GitResolver<'a> { })) } - fn resolution(&self) -> &Resolution { - self.resolution - } - fn dep_id(&self) -> DependencyID { - self.dep_id - } - fn new_name(&self) -> &[u8] { - &self.new_name - } - fn set_new_name(&mut self, name: Vec) { - self.new_name = name; - } - fn take_new_name(&mut self) -> Vec { - core::mem::take(&mut self.new_name) + fn fallback_name(&self) -> Option> { + Some(self.fallback_name.to_vec()) } } @@ -117,6 +101,10 @@ impl<'a> ResolverContext for TarballResolver<'a> { _ => unreachable!(), })) } + + fn fallback_name(&self) -> Option> { + Some(dependency::fallback_package_name(self.url).to_vec()) + } } // ────────────────────────────────────────────────────────────────────────── @@ -128,7 +116,6 @@ impl PackageManager { pub(crate) fn process_extracted_tarball_package( &mut self, package_id: &mut PackageID, - dep_id: DependencyID, resolution: &Resolution, data: &ExtractData, log_level: LogLevel, @@ -136,11 +123,14 @@ impl PackageManager { match resolution.tag { ResolutionTag::Git | ResolutionTag::Github => { let mut package = 'package: { + let fallback_name: Vec = resolution + .repository() + .fallback_package_name(self.lockfile.buffers.string_bytes.as_slice()) + .to_vec(); let mut resolver = GitResolver { resolved: &data.resolved, resolution, - dep_id, - new_name: Vec::new(), + fallback_name: &fallback_name, }; let mut pkg = Package::default(); @@ -184,25 +174,15 @@ impl PackageManager { } // package.json doesn't exist, no dependencies to worry about but we need to decide on a name for the dependency - // tag is `.git` or `.github`; both store `Repository`. - let repo = *resolution.repository(); - - let new_name = Repository::create_dependency_name_from_version_literal( - &repo, - self.lockfile.buffers.string_bytes.as_slice(), - &self.lockfile.buffers.dependencies[dep_id as usize], - ); - // `defer manager.allocator.free(new_name)` — `new_name: Vec` drops at scope end. - { let mut builder = self.lockfile.string_builder(); - builder.count(&new_name); + builder.count(&fallback_name); resolver.count(&mut builder, &Expr::default()); bun_core::handle_oom(builder.allocate()); - let name = builder.append::(&new_name); + let name = builder.append::(&fallback_name); pkg.name = name.value; pkg.name_hash = name.hash; diff --git a/src/install/PackageManager/runTasks.rs b/src/install/PackageManager/runTasks.rs index 05c3c87d1002..2516888d37c5 100644 --- a/src/install/PackageManager/runTasks.rs +++ b/src/install/PackageManager/runTasks.rs @@ -1152,7 +1152,6 @@ pub fn run_tasks( } } else if let Some(pkg) = manager.process_extracted_tarball_package( &mut package_id, - dependency_id, resolution, // Tag-checked accessor (debug_asserts Extract|LocalTarball); // shared `&task` here coexists with the field-disjoint @@ -1481,7 +1480,6 @@ pub fn run_tasks( } } else if let Some(pkg) = manager.process_extracted_tarball_package( &mut package_id, - git_checkout.dependency_id, resolution, // Tag-checked accessor (debug_asserts GitCheckout); shared // `&task` here coexists with the field-disjoint diff --git a/src/install/dependency.rs b/src/install/dependency.rs index 3a2661f63ab6..baa6aa057e6e 100644 --- a/src/install/dependency.rs +++ b/src/install/dependency.rs @@ -586,6 +586,36 @@ pub(crate) fn is_safe_install_folder_name(name: &[u8]) -> bool { true } +/// bun.lock stores a package as `"@"` and takes it apart again with +/// `split_name_and_version`, then rejects names that fail `is_safe_install_folder_name`. +/// A name containing any `@` other than a scope marker (`a@b`, `@s/a@b`) or failing that +/// check would therefore be written out but could never be loaded back. +pub(crate) fn is_safe_lockfile_package_name(name: &[u8]) -> bool { + is_safe_install_folder_name(name) && !strings::contains_char(&name[1..], b'@') +} + +const FALLBACK_PACKAGE_NAME: &[u8] = b"unnamed-package"; + +/// Name for a package whose own package.json has no `name`: the last component of where it +/// came from, so `../pkgs/foo`, `https://host/foo.tgz?token=x` and `https://host/user/foo` +/// all become `foo`. Always `is_safe_lockfile_package_name`, since it ends up in bun.lock. +pub(crate) fn fallback_package_name(location: &[u8]) -> &[u8] { + let without_query = match strings::index_of_char(location, b'?') { + Some(query_start) => &location[..query_start as usize], + None => location, + }; + let basename = bun_paths::basename(without_query); + let name = strings::without_suffix_comptime( + strings::without_suffix_comptime(basename, b".tgz"), + b".tar.gz", + ); + if is_safe_lockfile_package_name(name) { + name + } else { + FALLBACK_PACKAGE_NAME + } +} + /// assumes version is valid pub fn without_build_tag(version: &[u8]) -> &[u8] { if let Some(plus) = strings::index_of_char(version, b'+') { diff --git a/src/install/lockfile/Package.rs b/src/install/lockfile/Package.rs index 3f4bd10c96ec..8d40ac40f3b5 100644 --- a/src/install/lockfile/Package.rs +++ b/src/install/lockfile/Package.rs @@ -12,12 +12,10 @@ use bun_semver::{self as semver, ExternalString, String, Version as SemverVersio use crate::bun_json::{E, Expr, ExprData}; use crate::dependency::{Behavior, DependencyExt as _, TagExt as _}; -use crate::repository::RepositoryExt as _; use crate::{ self as install, Aligner, Bin, Dependency, ExternalStringList, ExternalStringMap, Features, - Npm, PackageID, PackageManager, PackageNameHash, Repository, TruncatedPackageNameHash, - UpdateRequest, bin, default_trusted_dependencies, dependency, initialize_store, - invalid_package_id, + Npm, PackageID, PackageManager, PackageNameHash, TruncatedPackageNameHash, UpdateRequest, bin, + default_trusted_dependencies, dependency, initialize_store, invalid_package_id, }; // `Package.rs` is mounted as `crate::lockfile_real::package`; the parent module // (`super`) is the real `lockfile.rs`, distinct from the `crate::lockfile` @@ -183,7 +181,6 @@ pub(crate) type Resolution = ResolutionType; // override what they need. The `()` impl is the no-op resolver. pub trait ResolverContext { const IS_VOID: bool = false; - const IS_GIT_RESOLVER: bool = false; fn check_bundled_dependencies() -> bool { false @@ -207,34 +204,12 @@ pub trait ResolverContext { json: &Expr, ) -> crate::Result>; - // ── GitResolver-only surface ──────────────────────────────────────────── - // Trait methods so non-git - // resolvers don't need the fields; default impls are dead code (gated on - // `IS_GIT_RESOLVER`). The bodies here are never executed — calls are - // statically guarded by `if R::IS_GIT_RESOLVER` — so a debug assertion - // documents the invariant without panicking in release. - fn resolution(&self) -> &ResolutionType { - debug_assert!( - false, - "ResolverContext::resolution called on non-git resolver" - ); - // SAFETY: unreachable in practice; never dereferenced when the - // `IS_GIT_RESOLVER` gate is false. `ZEROED` is an associated const on a - // trait-bounded generic impl, which Rust refuses to evaluate in `const` - // position; a `static` (with `Sync` POD payload) sidesteps that. - static EMPTY: ResolutionType = ResolutionType::::ZEROED; - &EMPTY - } - fn dep_id(&self) -> install::DependencyID { - debug_assert!(false, "ResolverContext::dep_id called on non-git resolver"); - 0 - } - fn new_name(&self) -> &[u8] { - b"" - } - fn set_new_name(&mut self, _name: Vec) {} - fn take_new_name(&mut self) -> Vec { - Vec::new() + /// Name to store when the package.json has no `name`. Resolvers for + /// packages fetched from somewhere (git, tarballs, folders) derive one from + /// the source; `None` (the root, workspace members, the npm cache) leaves + /// the package unnamed. + fn fallback_name(&self) -> Option> { + None } } @@ -266,7 +241,6 @@ impl ResolverContext for () { // permitted on object-safe trait methods, only type generics are not. trait ResolverContextDyn { fn is_void(&self) -> bool; - fn is_git(&self) -> bool; fn check_bundled_dependencies(&self) -> bool; fn count(&mut self, builder: &mut StringBuilder<'_>, json: &Expr); @@ -276,11 +250,7 @@ trait ResolverContextDyn { json: &Expr, ) -> crate::Result>; - fn resolution(&self) -> &ResolutionType; - fn dep_id(&self) -> install::DependencyID; - fn new_name(&self) -> &[u8]; - fn set_new_name(&mut self, name: Vec); - fn take_new_name(&mut self) -> Vec; + fn fallback_name(&self) -> Option>; } impl ResolverContextDyn for R { @@ -289,10 +259,6 @@ impl ResolverContextDyn for R { R::IS_VOID } #[inline] - fn is_git(&self) -> bool { - R::IS_GIT_RESOLVER - } - #[inline] fn check_bundled_dependencies(&self) -> bool { R::check_bundled_dependencies() } @@ -311,24 +277,8 @@ impl ResolverContextDyn for R { } #[inline] - fn resolution(&self) -> &ResolutionType { - ResolverContext::resolution(self) - } - #[inline] - fn dep_id(&self) -> install::DependencyID { - ResolverContext::dep_id(self) - } - #[inline] - fn new_name(&self) -> &[u8] { - ResolverContext::new_name(self) - } - #[inline] - fn set_new_name(&mut self, name: Vec) { - ResolverContext::set_new_name(self, name) - } - #[inline] - fn take_new_name(&mut self) -> Vec { - ResolverContext::take_new_name(self) + fn fallback_name(&self) -> Option> { + ResolverContext::fallback_name(self) } } @@ -2060,13 +2010,13 @@ impl Package { self.name_hash = 0; // -- Count the sizes - 'name: { + let name: &[u8] = 'name: { if let Some(name_q) = json.as_property(b"name") { if let Some(name) = name_q.expr.as_utf8(&bump) { if !name.is_empty() { - // Non-root names become bun.lock `packages` entries, which the - // lockfile parser rejects with the same check (see bun.lock.rs). - if !FEATURES.is_main && !dependency::is_safe_install_folder_name(name) { + // Non-root names become bun.lock `packages` entries, which can + // only be read back if `is_safe_lockfile_package_name` holds. + if !FEATURES.is_main && !dependency::is_safe_lockfile_package_name(name) { log.add_error_fmt( source, value_loc_of(source, name_q.loc), @@ -2074,29 +2024,18 @@ impl Package { ); return Err(crate::Error::InvalidPackageJSON); } - string_builder.count(name); - break 'name; + break 'name name; } } } - // name is not validated by npm, so fallback to creating a new from the version literal - if resolver.is_git() { - let resolution: &Resolution = resolver.resolution(); - let repo = match resolution.tag { - ResolutionTag::Git => *resolution.git(), - ResolutionTag::Github => *resolution.github(), - _ => break 'name, - }; - - resolver.set_new_name(Repository::create_dependency_name_from_version_literal( - &repo, - string_builder.string_bytes.as_slice(), - &lockfile.buffers.dependencies[resolver.dep_id() as usize], - )); - - string_builder.count(resolver.new_name()); + match resolver.fallback_name() { + Some(fallback_name) => bump.alloc_slice_copy(&fallback_name), + None => b"", } + }; + if !name.is_empty() { + string_builder.count(name); } if let Some(patched_deps) = json.as_property(b"patchedDependencies") { @@ -2415,28 +2354,10 @@ impl Package { // was reserved above so it does not realloc. let mut package_dependencies: Vec = Vec::with_capacity(total_len - off); - 'name: { - if resolver.is_git() { - if !resolver.new_name().is_empty() { - let new_name = resolver.take_new_name(); - let external_string = string_builder.append::(&new_name); - self.name = external_string.value; - self.name_hash = external_string.hash; - break 'name; - } - } - - if let Some(name_q) = json.as_property(b"name") { - if let Some(name) = name_q.expr.as_utf8(&bump) { - if !name.is_empty() { - let external_string = string_builder.append::(name); - - self.name = external_string.value; - self.name_hash = external_string.hash; - break 'name; - } - } - } + if !name.is_empty() { + let external_string = string_builder.append::(name); + self.name = external_string.value; + self.name_hash = external_string.hash; } if !FEATURES.is_main { diff --git a/src/install/repository.rs b/src/install/repository.rs index 7d86db59cd51..ae1a2fc0716d 100644 --- a/src/install/repository.rs +++ b/src/install/repository.rs @@ -321,11 +321,9 @@ pub trait RepositoryExt: Sized { fn parse_append_git(input: &[u8], buf: &mut StringBuf<'_>) -> Result; fn parse_append_github(input: &[u8], buf: &mut StringBuf<'_>) -> Result; - fn create_dependency_name_from_version_literal( - repository: &Repository, - string_buf: &[u8], - dep: &Install::Dependency, - ) -> Vec; + /// `dependency::fallback_package_name` of the repository, for a checkout + /// whose package.json has no `name` (or no package.json at all). + fn fallback_package_name<'a>(&'a self, string_buf: &'a [u8]) -> &'a [u8]; fn format_as(&self, label: &str, buf: &[u8], writer: &mut impl fmt::Write) -> fmt::Result; fn fmt_store_path<'a>(&'a self, label: &'a str, string_buf: &'a [u8]) -> StorePathFormatter<'a>; @@ -541,45 +539,12 @@ impl RepositoryExt for Repository { Ok(result) } - fn create_dependency_name_from_version_literal( - repository: &Repository, - string_buf: &[u8], - dep: &Install::Dependency, - ) -> Vec { - // Callers (`parse_with_json`) hold a split `StringBuilder` - // borrow on `string_bytes`, so accept the two pieces directly. - let buf = string_buf; - let repo_name = repository.repo; - let repo_name_str = repo_name.slice(buf); - - let name = 'brk: { - let mut remain = repo_name_str; - - if let Some(hash_index) = strings::index_of_char(remain, b'#') { - remain = &remain[..hash_index as usize]; - } - - if remain.is_empty() { - break 'brk remain; - } - - if let Some(slash_index) = strings::last_index_of_char(remain, b'/') { - remain = &remain[slash_index + 1..]; - } - - remain - }; - - if name.is_empty() { - let version_literal = dep.version.literal.slice(buf); - let mut name_buf = [0u8; bun_sha::SHA1::DIGEST]; - let mut sha1 = bun_sha::SHA1::init(); - sha1.update(version_literal); - sha1.r#final(&mut name_buf); - return name_buf.to_vec(); + fn fallback_package_name<'a>(&'a self, string_buf: &'a [u8]) -> &'a [u8] { + let mut repo = self.repo.slice(string_buf); + if let Some(hash_index) = strings::index_of_char(repo, b'#') { + repo = &repo[..hash_index as usize]; } - - name.to_vec() + Dependency::fallback_package_name(repo) } fn format_as(&self, label: &str, buf: &[u8], writer: &mut impl fmt::Write) -> fmt::Result { diff --git a/src/install/resolution.rs b/src/install/resolution.rs index c3041672be89..66e3d328dc9c 100644 --- a/src/install/resolution.rs +++ b/src/install/resolution.rs @@ -94,17 +94,6 @@ pub enum TaggedValue { } impl ResolutionType { - /// Const-evaluable zeroed sentinel. Mirrors `Default::default()` but usable - /// in `const` / `static` position (e.g. dummy `&'static Resolution` returns). - /// Only the tag/padding are guaranteed zero — the union payload is the - /// `uninitialized` variant, which is the only field a `Tag::Uninitialized` - /// reader may legally access. - pub(crate) const ZEROED: Self = Self { - tag: Tag::Uninitialized, - _padding: [0; 7], - value: Value { uninitialized: () }, - }; - /// Construct from a tagged value, e.g. `Resolution::init(TaggedValue::Npm(...))`. #[inline] pub(crate) fn init(value: TaggedValue) -> Self { diff --git a/src/install/resolvers/folder_resolver.rs b/src/install/resolvers/folder_resolver.rs index d75d0c9c96df..fb071942157a 100644 --- a/src/install/resolvers/folder_resolver.rs +++ b/src/install/resolvers/folder_resolver.rs @@ -128,6 +128,15 @@ impl<'a, const TAG: ResolutionTag> ResolverContext for NewResolver<'a, TAG> { _ => unreachable!(), })) } + + fn fallback_name(&self) -> Option> { + // Workspace members were already registered under their package.json + // name by `WorkspaceMap` and are found by that name again later. + if matches!(TAG, ResolutionTag::Workspace) { + return None; + } + Some(dependency::fallback_package_name(self.folder_path).to_vec()) + } } type Resolver<'a> = NewResolver<'a, { ResolutionTag::Folder }>; diff --git a/test/cli/install/bun-add.test.ts b/test/cli/install/bun-add.test.ts index 2a0f346eb12b..3886052b1695 100644 --- a/test/cli/install/bun-add.test.ts +++ b/test/cli/install/bun-add.test.ts @@ -93,6 +93,51 @@ it("should add existing package", async () => { ); }); +it("should add a folder whose package.json has no name under the folder's name", async () => { + // Used to be added as `"": "file:..."` and then fail to install anywhere. + const pkg_dir = join(add_dir, "pkg-without-name"); + await mkdir(pkg_dir); + await writeFile(join(pkg_dir, "package.json"), JSON.stringify({ version: "0.0.1" })); + await writeFile( + join(package_dir, "package.json"), + JSON.stringify({ + name: "bar", + version: "0.0.2", + }), + ); + const add_path = relative(package_dir, pkg_dir).replace(/\\/g, "/"); + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "add", `file:${add_path}`], + cwd: package_dir, + stdout: "pipe", + stdin: "pipe", + stderr: "pipe", + env, + }); + const [err, out, exitCode] = await Promise.all([stderr.text(), stdout.text(), exited]); + expect(err).not.toContain("error:"); + expect(err).not.toContain("warn:"); + expect(err).toContain("Saved lockfile"); + expect(out.replace(/\s*\[[0-9\.]+m?s\]\s*$/, "").split(/\r?\n/)).toEqual([ + expect.stringContaining("bun add v1."), + "", + `installed pkg-without-name@${add_path}`, + "", + "1 package installed", + ]); + expect(exitCode).toBe(0); + expect(await file(join(package_dir, "package.json")).json()).toEqual({ + name: "bar", + version: "0.0.2", + dependencies: { + "pkg-without-name": `file:${add_path}`, + }, + }); + expect(await file(join(package_dir, "node_modules", "pkg-without-name", "package.json")).json()).toEqual({ + version: "0.0.1", + }); +}); + it("should reject missing package", async () => { await writeFile( join(package_dir, "package.json"), diff --git a/test/cli/install/bun-install-git-deps.test.ts b/test/cli/install/bun-install-git-deps.test.ts index 76997319f218..10eb7627219a 100644 --- a/test/cli/install/bun-install-git-deps.test.ts +++ b/test/cli/install/bun-install-git-deps.test.ts @@ -32,17 +32,21 @@ function git(cwd: string, ...args: string[]) { } interface BranchPackage { - name: string; + name?: string; branch: string; dependencies?: Record; } -// Creates `/shared-repo.git`, a bare repo with one orphan branch per +// Creates `/`, a bare repo with one orphan branch per // package, and prepares it for serving over dumb HTTP. -async function makeSharedRepo(root: string, packages: BranchPackage[]): Promise { - const bare = join(root, "shared-repo.git"); +async function makeSharedRepo( + root: string, + packages: BranchPackage[], + repoName: string = "shared-repo.git", +): Promise { + const bare = join(root, repoName); const work = join(root, "work"); - await git(root, "init", "-q", "--bare", "shared-repo.git"); + await git(root, "init", "-q", "--bare", repoName); mkdirSync(work); await git(work, "init", "-q"); for (const pkg of packages) { @@ -423,3 +427,46 @@ test.concurrent("installs a git+file:// dependency", async () => { expect(await installedVersionOf(project, "@scope/pkg-b")).toBe("pkg-b"); expect(exitCode).toBe(0); }); + +// A git package whose package.json has no name is named after its repository. bun.lock stores a +// package as "@", so when that repository name itself contains an "@" (or is +// otherwise unusable as a package name) the generic name is used instead; the repository name +// used to be taken as-is, producing an entry the next install could not parse ("Ignoring +// lockfile"). Folders and tarballs without a name are covered in bun-lock.test.ts. +test.concurrent("names a git dependency without a package.json name after its repository", async () => { + using dir = tempDir("git-dep-no-name", {}); + using oddDir = tempDir("git-dep-no-name-odd-repo", {}); + const root = String(dir); + const repo = `git+${pathToFileURL(await makeSharedRepo(root, [{ branch: "no-name" }]))}`; + const oddRepo = `git+${pathToFileURL(await makeSharedRepo(String(oddDir), [{ branch: "odd-repo" }], "odd@repo.git"))}`; + + const project = join(root, "project"); + mkdirSync(project); + writeFileSync( + join(project, "package.json"), + JSON.stringify({ + name: "project", + version: "1.0.0", + dependencies: { + "no-name-dep": `${repo}#no-name`, + "odd-repo-dep": `${oddRepo}#odd-repo`, + }, + }), + ); + + let { stderr, exitCode } = await runInstall(project, join(root, "cache"), {}); + expect(stderr).not.toContain("error:"); + expect(await installedVersionOf(project, "no-name-dep")).toBe("no-name"); + expect(await installedVersionOf(project, "odd-repo-dep")).toBe("odd-repo"); + expect(exitCode).toBe(0); + + const lockfile = await Bun.file(join(project, "bun.lock")).text(); + expect(lockfile).toContain(`"no-name-dep": ["shared-repo.git@${repo}#`); + expect(lockfile).toContain(`"odd-repo-dep": ["unnamed-package@${oddRepo}#`); + + ({ stderr, exitCode } = await runInstall(project, join(root, "cache"), {}, "--frozen-lockfile")); + expect(stderr).not.toContain("Ignoring lockfile"); + expect(stderr).not.toContain("error:"); + expect(await Bun.file(join(project, "bun.lock")).text()).toBe(lockfile); + expect(exitCode).toBe(0); +}); diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index b08b814cf775..1636d66bd304 100644 --- a/test/cli/install/bun-install.test.ts +++ b/test/cli/install/bun-install.test.ts @@ -9786,6 +9786,55 @@ describe.concurrent("a dependency whose own package.json has an invalid name", ( await expectRejected(String(dir), '"a\\b"'); }); + // The bun.lock parser splits `name@resolution` at the first "@" after an optional scope, so a + // name with any other "@" in it cannot be stored either, even though it is a safe folder name. + it("fails to install a file: folder whose name contains an @", async () => { + using dir = tempDir("invalid-name-at-folder", { + "project/package.json": JSON.stringify({ + name: "foo", + version: "0.0.1", + dependencies: { dep: "file:../lib" }, + }), + "lib/package.json": JSON.stringify({ name: "a@b", version: "1.0.0" }), + }); + + await expectRejected(String(dir), '"a@b"'); + }); + + it("fails to install a local tarball whose scoped name contains a second @", async () => { + using dir = tempDir("invalid-name-at-tarball", { + "project/package.json": JSON.stringify({ + name: "foo", + version: "0.0.1", + dependencies: { dep: "file:../dep.tgz" }, + }), + }); + await Bun.Archive.write( + join(String(dir), "dep.tgz"), + { "package/package.json": JSON.stringify({ name: "@scope/a@b", version: "1.0.0" }) }, + { compress: "gzip" }, + ); + + await expectRejected(String(dir), '"@scope/a@b"'); + }); + + it("fails to install a workspace whose member's name contains an @", async () => { + using dir = tempDir("invalid-name-at-workspace", { + "project/package.json": JSON.stringify({ + name: "foo", + version: "0.0.1", + workspaces: ["packages/*"], + }), + "project/packages/member/package.json": JSON.stringify({ name: "a@b", version: "1.0.0" }), + }); + + const { err, exitCode } = await install(String(dir)); + expect(err).toContain('error: Invalid package name "a@b"'); + expect(err).toMatch(/ at .*member[\\/]package\.json:1:9\s/); + expect(await exists(join(String(dir), "project", "bun.lock"))).toBe(false); + expect(exitCode).toBe(1); + }); + it("still installs scoped names and writes a lockfile the next install loads", async () => { using dir = tempDir("valid-scoped-name-tarball", { "project/package.json": JSON.stringify({ diff --git a/test/cli/install/bun-lock.test.ts b/test/cli/install/bun-lock.test.ts index 10e598240208..b80b46e27c63 100644 --- a/test/cli/install/bun-lock.test.ts +++ b/test/cli/install/bun-lock.test.ts @@ -813,6 +813,103 @@ it("escapes double quotes in npm registry tarball URLs when saving bun.lock", as expect(await exited).toBe(0); }); +// bun.lock stores every package as "@" and splits the name back off at the +// first "@" after an optional scope, so a package without a name cannot be stored. A folder or +// tarball package whose package.json had no name used to be written as e.g. "@file:../dir", which +// the next install could not parse: it printed "Ignoring lockfile", and --frozen-lockfile always +// failed. Such packages are now named after their folder or tarball. The git variant lives in +// bun-install-git-deps.test.ts, the isolated store layout in isolated-install.test.ts. +it("names folder and tarball packages without a package.json name after their folder or tarball", async () => { + const noName = JSON.stringify({ version: "1.0.0" }); + const { packageDir, packageJson } = await registry.createTestDir({ + bunfigOpts: { linker: "hoisted" }, + files: { + "pkgs/folder-pkg/package.json": noName, + "pkgs/empty-name-pkg/package.json": JSON.stringify({ name: "", version: "1.0.0" }), + // "odd@folder" cannot be stored as a name either, so this one gets the generic name. + "pkgs/odd@folder/package.json": noName, + }, + }); + await Bun.Archive.write( + join(packageDir, "tarball-pkg.tgz"), + { "package/package.json": noName }, + { compress: "gzip" }, + ); + // Serves that tarball under other file names: remote packages are named after the URL. + await using server = Bun.serve({ + port: 0, + fetch: () => new Response(file(join(packageDir, "tarball-pkg.tgz"))), + }); + const dependencies: Record = { + "folder-dep": "file:./pkgs/folder-pkg", + "empty-name-dep": "file:./pkgs/empty-name-pkg", + "odd-folder-dep": "file:./pkgs/odd@folder", + "tarball-dep": "file:./tarball-pkg.tgz", + "remote-dep": `http://localhost:${server.port}/remote-pkg.tgz`, + "signed-url-dep": `http://localhost:${server.port}/signed-pkg.tgz?token=abc/def`, + }; + await write(packageJson, JSON.stringify({ name: "deps-without-names", dependencies })); + + await runBunInstall(env, packageDir); + const lockfile = await file(join(packageDir, "bun.lock")).text(); + expect(lockfile.replaceAll(/localhost:\d+/g, "localhost:1234").replaceAll(/"sha512-[^"]*"/g, '""')) + .toMatchInlineSnapshot(` + "{ + "lockfileVersion": 2, + "configVersion": 1, + "workspaces": { + "": { + "name": "deps-without-names", + "dependencies": { + "empty-name-dep": "file:./pkgs/empty-name-pkg", + "folder-dep": "file:./pkgs/folder-pkg", + "odd-folder-dep": "file:./pkgs/odd@folder", + "remote-dep": "http://localhost:1234/remote-pkg.tgz", + "signed-url-dep": "http://localhost:1234/signed-pkg.tgz?token=abc/def", + "tarball-dep": "file:./tarball-pkg.tgz", + }, + }, + }, + "packages": { + "empty-name-dep": ["empty-name-pkg@file:pkgs/empty-name-pkg", {}], + + "folder-dep": ["folder-pkg@file:pkgs/folder-pkg", {}], + + "odd-folder-dep": ["unnamed-package@file:pkgs/odd@folder", {}], + + "remote-dep": ["remote-pkg@http://localhost:1234/remote-pkg.tgz", {}, ""], + + "signed-url-dep": ["signed-pkg@http://localhost:1234/signed-pkg.tgz?token=abc/def", {}, ""], + + "tarball-dep": ["tarball-pkg@./tarball-pkg.tgz", {}, ""], + } + } + " + `); + expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual([ + "empty-name-dep", + "folder-dep", + "odd-folder-dep", + "remote-dep", + "signed-url-dep", + "tarball-dep", + ]); + + // runBunInstall rejects any warning, which includes "warn: Ignoring lockfile". + await runBunInstall(env, packageDir, { savesLockfile: false }); + expect(await file(join(packageDir, "bun.lock")).text()).toBe(lockfile); + await runBunInstall(env, packageDir, { frozenLockfile: true }); + + // The name comes from the folder, not from the alias, so a second alias for the same folder + // resolves to the package already in the lockfile. + dependencies["folder-dep-again"] = "file:./pkgs/folder-pkg"; + await write(packageJson, JSON.stringify({ name: "deps-without-names", dependencies })); + await runBunInstall(env, packageDir); + expect(await file(join(packageDir, "bun.lock")).text()).toContain( + '"folder-dep-again": ["folder-pkg@file:pkgs/folder-pkg", {}]', + ); +}); + it("escapes quotes and newlines in requested version literals when writing yarn.lock", async () => { const { packageDir, packageJson } = await registry.createTestDir(); diff --git a/test/cli/install/isolated-install.test.ts b/test/cli/install/isolated-install.test.ts index a4ff2a70903f..52c30ad03eb1 100644 --- a/test/cli/install/isolated-install.test.ts +++ b/test/cli/install/isolated-install.test.ts @@ -327,6 +327,53 @@ test("can install folder dependencies", async () => { ).toBe("module.exports = 'hello from pkg-1';"); }); +test("a folder dependency without a package.json name is stored under its folder name", async () => { + const { packageJson, packageDir } = await registry.createTestDir({ bunfigOpts: { linker: "isolated" } }); + + await Promise.all([ + write( + packageJson, + JSON.stringify({ + name: "test-pkg-nameless-folder-dep", + dependencies: { + "folder-dep": "file:./pkg-1", + }, + }), + ), + write(join(packageDir, "pkg-1", "package.json"), JSON.stringify({ version: "1.0.0" })), + write(join(packageDir, "pkg-1", "index.js"), "module.exports = 'hello from pkg-1';"), + ]); + + await runBunInstall(bunEnv, packageDir); + + // Without a name the store entry used to be ".bun/@file+pkg-1/node_modules" with the package's + // files spilled directly into that node_modules directory. + expect(readlinkSync(join(packageDir, "node_modules", "folder-dep"))).toBe( + join(".bun", "pkg-1@file+pkg-1", "node_modules", "pkg-1"), + ); + expect( + await file( + join(packageDir, "node_modules", ".bun", "pkg-1@file+pkg-1", "node_modules", "pkg-1", "package.json"), + ).json(), + ).toEqual({ version: "1.0.0" }); + + await using proc = spawn({ + cmd: [bunExe(), "-e", "console.log(require('folder-dep'))"], + cwd: packageDir, + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toBe("hello from pkg-1\n"); + expect(stderr).toBe(""); + expect(exitCode).toBe(0); + + // The entry bun wrote loads again (runBunInstall fails on "warn: Ignoring lockfile"). + expect(await file(join(packageDir, "bun.lock")).text()).toContain('"folder-dep": ["pkg-1@file:pkg-1", {}]'); + await runBunInstall(bunEnv, packageDir, { savesLockfile: false }); +}); + test("can install folder dependencies on root package", async () => { const { packageDir, packageJson } = await registry.createTestDir({ bunfigOpts: { linker: "isolated" } });