From 95102c183d5a93e097aa75366c18e7f28025890d Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:25:46 +0000 Subject: [PATCH 1/3] install: reject dependency names containing control characters A registry manifest (or a lockfile) can declare a dependency whose name contains terminal control characters. bun requested it, put the raw name on the progress line and in error output, wrote it into bun.lock and created node_modules/ from it, and bun pm ls / bun why printed it raw again later. Validate the alias and, for registry dependencies, the resolved name when a dependency is enqueued, before anything is requested or printed, using the same is_safe_install_folder_name the tree builder, the installers and the bun.lock parser already apply to folder names; that validator now also rejects C0 controls, DEL and UTF-8 encoded C1 controls. A required dependency with such a name is an error, an optional one a warning, like an unresolvable dependency. The messages that report a rejected or unresolved name render it through a new bun_core::fmt::escape_control_chars so the report cannot replay the characters it is complaining about. --- src/bun_core/fmt.rs | 47 ++++ src/install/PackageInstaller.rs | 21 +- .../PackageManager/PackageManagerEnqueue.rs | 53 +++++ .../PackageManagerResolution.rs | 12 +- src/install/TarballStream.rs | 2 +- src/install/dependency.rs | 25 +- src/install/error.rs | 3 + src/install/extract_tarball.rs | 4 +- src/install/isolated_install.rs | 2 +- src/install/lockfile/Tree.rs | 13 +- src/install/lockfile/bun.lock.rs | 6 +- test/cli/install/bun-install-registry.test.ts | 6 +- test/cli/install/bun-install.test.ts | 222 +++++++++++++++++- test/cli/install/isolated-install.test.ts | 5 +- 14 files changed, 381 insertions(+), 40 deletions(-) diff --git a/src/bun_core/fmt.rs b/src/bun_core/fmt.rs index 2dc33cbfe4fc..32ff0333cd5b 100644 --- a/src/bun_core/fmt.rs +++ b/src/bun_core/fmt.rs @@ -3325,6 +3325,53 @@ fn escape_powershell_impl(str: &[u8], writer: &mut impl fmt::Write) -> fmt::Resu write_bytes(writer, remain) } +// ─────────────────────────────────────────────────────────────────────────── +// escapeControlChars +// ─────────────────────────────────────────────────────────────────────────── + +/// Renders the wrapped `Display` with C0 controls, DEL and C1 controls +/// spelled out (`\n`, `\r`, `\t`, `\x1b`, `\x7f`, `\u009b`) instead of +/// written raw. For text somebody else authored (a registry manifest, a +/// dependency's `package.json`): printed raw, an ESC/CR/C1 sequence can erase +/// or repaint the line it is shown on and a newline can forge further lines +/// of our output. Everything else passes through unchanged. +pub struct EscapeControlChars(pub T); + +/// [`EscapeControlChars`] over raw bytes; invalid UTF-8 renders as U+FFFD. +pub fn escape_control_chars(text: &[u8]) -> EscapeControlChars<&bstr::BStr> { + EscapeControlChars(bstr::BStr::new(text)) +} + +impl Display for EscapeControlChars { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + let mut writer = EscapeControlCharsWriter(f); + write!(writer, "{}", self.0) + } +} + +struct EscapeControlCharsWriter<'a, 'f>(&'a mut Formatter<'f>); + +impl fmt::Write for EscapeControlCharsWriter<'_, '_> { + fn write_str(&mut self, s: &str) -> fmt::Result { + let mut start = 0; + for (i, c) in s.char_indices() { + if !matches!(c, '\0'..='\x1f' | '\x7f' | '\u{80}'..='\u{9f}') { + continue; + } + self.0.write_str(&s[start..i])?; + match c { + '\n' => self.0.write_str("\\n")?, + '\r' => self.0.write_str("\\r")?, + '\t' => self.0.write_str("\\t")?, + c if c.is_ascii() => write!(self.0, "\\x{:02x}", c as u32)?, + c => write!(self.0, "\\u{:04x}", c as u32)?, + } + start = i + c.len_utf8(); + } + self.0.write_str(&s[start..]) + } +} + // js_bindings (fmtString for highlighter.test.ts) lives in src/jsc/fmt_jsc.rs // alongside fmt_jsc.bind.ts; bun_core/ stays JSC-free. diff --git a/src/install/PackageInstaller.rs b/src/install/PackageInstaller.rs index 6c96867f593b..476501a4c636 100644 --- a/src/install/PackageInstaller.rs +++ b/src/install/PackageInstaller.rs @@ -404,23 +404,16 @@ impl<'a> LazyPackageDestinationDir<'a> { } /// A dependency alias becomes the install destination inside `node_modules` -/// (the existing entry is renamed aside, deleted, and re-created). Reject -/// anything that could escape `node_modules`: empty names, `.`/`..` -/// components, absolute paths, drive letters, backslashes, NUL bytes, and any -/// separator other than the single `/` in a scoped name (`@scope/name`). +/// (the existing entry is renamed aside, deleted, and re-created). On top of +/// `is_safe_install_folder_name` (empty names, `.`/`..` components, drive +/// letters, backslashes, control characters), reject any separator other than +/// the single `/` in a scoped name (`@scope/name`). pub(crate) fn alias_is_safe_install_target(alias: &[u8]) -> bool { - if alias.is_empty() || alias.len() >= MAX_PATH_BYTES || strings::contains_any(alias, b"\\:\0") { + if alias.len() >= MAX_PATH_BYTES || !crate::dependency::is_safe_install_folder_name(alias) { return false; } - let mut component_count = 0usize; - for component in strings::split(alias, b"/") { - component_count += 1; - if component.is_empty() || component == b"." || component == b".." { - return false; - } - } - + let component_count = strings::split(alias, b"/").count(); component_count == 1 || (component_count == 2 && alias[0] == b'@') } @@ -1295,7 +1288,7 @@ impl<'a> PackageInstaller<'a> { if log_level != Options::LogLevel::Silent { bun_core::pretty_errorln!( "error: refusing to install dependency with unsafe name {}", - bstr::BStr::new(alias.slice(string_buf!())), + bun_core::fmt::escape_control_chars(alias.slice(string_buf!())), ); } self.summary.fail += 1; diff --git a/src/install/PackageManager/PackageManagerEnqueue.rs b/src/install/PackageManager/PackageManagerEnqueue.rs index ca23f272fd55..131342977946 100644 --- a/src/install/PackageManager/PackageManagerEnqueue.rs +++ b/src/install/PackageManager/PackageManagerEnqueue.rs @@ -770,6 +770,59 @@ pub fn enqueue_dependency_with_main_and_success_fn( version_was_replaced = false; break 'version dependency.version.clone(); }; + + // The alias becomes the `node_modules/` folder and, for registry + // dependencies, `name` becomes the request, the progress line and the + // package name; both come from untrusted manifests. Refuse to resolve (and + // so to fetch or print) either when it is unsafe, reporting it the way an + // unresolvable dependency is reported. Empty names are tolerated like in + // the tree builder. + let invalid_name = { + let alias = this.lockfile.str(&dependency.name); + let alias_is_safe = if alias == this.lockfile.str(&dependency.version.literal) { + // `bun add ` stores the specifier as the alias until + // `assign_resolution` replaces it with the resolved package's + // name, so it never becomes a folder, but it is still printed. + !dependency::contains_control_character(alias) + } else { + alias.is_empty() || dependency::is_safe_install_folder_name(alias) + }; + if !alias_is_safe { + Some(alias) + } else { + match version.tag { + dependency::version::Tag::Npm | dependency::version::Tag::DistTag => { + let registry_name = this.lockfile.str(&name); + (!registry_name.is_empty() + && !dependency::is_safe_install_folder_name(registry_name)) + .then_some(registry_name) + } + _ => None, + } + } + }; + if let Some(invalid_name) = invalid_name { + if let Some(fail) = fail_fn { + fail(this, dependency, id, crate::Error::InvalidDependencyName); + return Ok(()); + } + let name = bun_fmt::escape_control_chars(invalid_name); + if dependency.behavior.is_required() { + this.log_mut().add_error_fmt( + None, + bun_ast::Loc::EMPTY, + format_args!("Invalid dependency name \"{name}\""), + ); + } else { + this.log_mut().add_warning_fmt( + None, + bun_ast::Loc::EMPTY, + format_args!("Invalid dependency name \"{name}\""), + ); + } + return Ok(()); + } + let mut loaded_manifest: Option = None; match version.tag { diff --git a/src/install/PackageManager/PackageManagerResolution.rs b/src/install/PackageManager/PackageManagerResolution.rs index 04093280799e..f2ec57c7bad9 100644 --- a/src/install/PackageManager/PackageManagerResolution.rs +++ b/src/install/PackageManager/PackageManagerResolution.rs @@ -352,14 +352,20 @@ impl PackageManager { { Output::err_generic( "{} failed to resolve", - (failed_dep.version.literal.fmt(string_buf),), + (bun_core::fmt::escape_control_chars( + failed_dep.version.literal.slice(string_buf), + ),), ); } else { Output::err_generic( "{}@{} failed to resolve", ( - bstr::BStr::new(failed_dep.name.slice(string_buf)), - failed_dep.version.literal.fmt(string_buf), + bun_core::fmt::escape_control_chars( + failed_dep.name.slice(string_buf), + ), + bun_core::fmt::escape_control_chars( + failed_dep.version.literal.slice(string_buf), + ), ), ); } diff --git a/src/install/TarballStream.rs b/src/install/TarballStream.rs index 7ae9f698469b..77e9bd906cb7 100644 --- a/src/install/TarballStream.rs +++ b/src/install/TarballStream.rs @@ -1089,7 +1089,7 @@ impl TarballStream { bun_ast::Loc::EMPTY, format_args!( "Refusing to install package with invalid name \"{}\"", - bun_fmt::s(tarball.name_and_basename().0), + bun_fmt::escape_control_chars(tarball.name_and_basename().0), ), ); } else { diff --git a/src/install/dependency.rs b/src/install/dependency.rs index 3a2661f63ab6..06993b24680c 100644 --- a/src/install/dependency.rs +++ b/src/install/dependency.rs @@ -566,11 +566,13 @@ pub fn is_scoped_package_name(name: &[u8]) -> Result { Err(PackageNameError::InvalidPackageName) } -/// A dependency name/alias becomes a directory under `node_modules/`. Names -/// come from untrusted `package.json` / manifest keys, so reject anything that -/// could resolve outside that directory. `@scope/name` stays valid. +/// A dependency name/alias becomes a directory under `node_modules/` and is +/// echoed in progress and error output. Names come from untrusted +/// `package.json` / manifest keys, so reject anything that could resolve +/// outside that directory or carry terminal control sequences into the +/// output. `@scope/name` stays valid. pub(crate) fn is_safe_install_folder_name(name: &[u8]) -> bool { - if name.is_empty() { + if name.is_empty() || contains_control_character(name) { return false; } @@ -578,7 +580,7 @@ pub(crate) fn is_safe_install_folder_name(name: &[u8]) -> bool { if component.is_empty() || component == b"." || component == b".." { return false; } - if strings::contains_any(component, b"\\:\0") { + if strings::contains_any(component, b"\\:") { return false; } } @@ -586,6 +588,19 @@ pub(crate) fn is_safe_install_folder_name(name: &[u8]) -> bool { true } +/// The characters `bun_core::fmt::escape_control_chars` would have to escape: +/// C0 controls and DEL, plus the UTF-8 encoding of the C1 controls +/// (U+0080..=U+009F, `C2 80`..`C2 9F`), which terminals interpret as well. +pub(crate) fn contains_control_character(name: &[u8]) -> bool { + name.iter().enumerate().any(|(i, &byte)| { + byte.is_ascii_control() + || (byte == 0xC2 + && name + .get(i + 1) + .is_some_and(|next| (0x80..=0x9F).contains(next))) + }) +} + /// 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/error.rs b/src/install/error.rs index ab1c59a3a839..104a2952cc56 100644 --- a/src/install/error.rs +++ b/src/install/error.rs @@ -70,6 +70,8 @@ pub enum Error { Failed, #[error("UnrecognizedDependencyFormat")] UnrecognizedDependencyFormat, + #[error("InvalidDependencyName")] + InvalidDependencyName, #[error("No global directory found")] NoGlobalDirectoryFound, #[error("InvalidPackageID")] @@ -292,6 +294,7 @@ impl Error { Self::HTTPError => "HTTPError", Self::Failed => "Failed", Self::UnrecognizedDependencyFormat => "UnrecognizedDependencyFormat", + Self::InvalidDependencyName => "InvalidDependencyName", Self::NoGlobalDirectoryFound => "No global directory found", Self::InvalidPackageID => "InvalidPackageID", Self::PartialInstallFailed => "PartialInstallFailed", diff --git a/src/install/extract_tarball.rs b/src/install/extract_tarball.rs index b8d3a8b091b5..cdb719ae36ed 100644 --- a/src/install/extract_tarball.rs +++ b/src/install/extract_tarball.rs @@ -247,7 +247,7 @@ impl ExtractTarball { bun_ast::Loc::EMPTY, format_args!( "Refusing to install package with invalid name \"{}\"", - bun_fmt::s(name), + bun_fmt::escape_control_chars(name), ), ); return Err(crate::Error::InstallFailed); @@ -466,7 +466,7 @@ impl ExtractTarball { bun_ast::Loc::EMPTY, format_args!( "Refusing to install package with invalid name \"{}\"", - bun_fmt::s(name), + bun_fmt::escape_control_chars(name), ), ); return Err(crate::Error::InstallFailed); diff --git a/src/install/isolated_install.rs b/src/install/isolated_install.rs index 9b94cdda2d30..af8b2a9d153e 100644 --- a/src/install/isolated_install.rs +++ b/src/install/isolated_install.rs @@ -2123,7 +2123,7 @@ pub(crate) fn install_isolated_packages( if let Some(name) = unsafe_folder_name { Output::err_generic( "\"{}\" is not a valid install folder name", - (BStr::new(name),), + (bun_core::fmt::escape_control_chars(name),), ); Output::flush(); Global::exit(1); diff --git a/src/install/lockfile/Tree.rs b/src/install/lockfile/Tree.rs index 66d7c3c067ce..e5e942735bb1 100644 --- a/src/install/lockfile/Tree.rs +++ b/src/install/lockfile/Tree.rs @@ -372,7 +372,7 @@ pub(crate) fn relative_path_and_depth<'b, const PATH_STYLE: IteratorPathStyle>( if !folder_name_is_safe(name) { Output::err_generic( "Lockfile is malformed (dependency name \"{}\" is not a valid folder name)", - (bstr::BStr::new(name),), + (bun_core::fmt::escape_control_chars(name),), ); bun_core::Global::crash(); } @@ -791,17 +791,20 @@ impl Tree { // don't treat it as unsafe — match the lockfile parser and isolated // installer (`bun.lock.rs`, `isolated_install.rs`) which guard // `!name.is_empty()` here rather than failing the whole install. + // Neither does an unresolved dependency (it is skipped below, or as + // an optional peer bound to an already checked one), and + // `enqueue_dependency_with_main_and_success_fn` already reported + // its name if that is why it did not resolve. let dependency_name = dependency .name .slice(lockfile.buffers.string_bytes.as_slice()); - if !dependency_name.is_empty() + if pkg_id != invalid_package_id + && !dependency_name.is_empty() && !crate::dependency::is_safe_install_folder_name(dependency_name) { builder.maybe_report_error(format_args!( "Invalid dependency name \"{}\"", - dependency - .name - .fmt(lockfile.buffers.string_bytes.as_slice()), + bun_core::fmt::escape_control_chars(dependency_name), )); continue 'dep; } diff --git a/src/install/lockfile/bun.lock.rs b/src/install/lockfile/bun.lock.rs index 379de751af9b..425c2aa77480 100644 --- a/src/install/lockfile/bun.lock.rs +++ b/src/install/lockfile/bun.lock.rs @@ -3228,8 +3228,8 @@ fn dependency_resolution_failure( format_args!( "Failed to resolve {} dependency '{}' for package '{}'", behavior_str, - bstr::BStr::new(dep.name.slice(buf)), - bstr::BStr::new(path), + bun_core::fmt::escape_control_chars(dep.name.slice(buf)), + bun_core::fmt::escape_control_chars(path), ), ); } else { @@ -3239,7 +3239,7 @@ fn dependency_resolution_failure( format_args!( "Failed to resolve root {} dependency '{}'", behavior_str, - bstr::BStr::new(dep.name.slice(buf)), + bun_core::fmt::escape_control_chars(dep.name.slice(buf)), ), ); } diff --git a/test/cli/install/bun-install-registry.test.ts b/test/cli/install/bun-install-registry.test.ts index e40b14dcaf7a..2f755ac01d18 100644 --- a/test/cli/install/bun-install-registry.test.ts +++ b/test/cli/install/bun-install-registry.test.ts @@ -9133,8 +9133,10 @@ test("rejects npm aliases whose manifest URL resolves to a different host than t const err = await stderr.text(); await stdout.text(); - // The manifest request must be refused with a clear error... - expect(err).toContain("is not on registry"); + // The manifest request must be refused with a clear error (today the name is + // already refused while resolving, before a URL is built; the URL check stays + // behind it as a second line of defense)... + expect(err).toMatch(/Invalid dependency name|is not on registry/); // ...and no request (carrying the registry Authorization header) may reach // a host other than the configured registry. expect(received).toEqual([]); diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index 0602dec36d5c..e54b4edb41c8 100644 --- a/test/cli/install/bun-install.test.ts +++ b/test/cli/install/bun-install.test.ts @@ -9637,13 +9637,231 @@ it("does not extract a tarball for a dependency alias containing '..' path segme expect(await readdirSorted(zone)).toEqual(["a"]); expect(await readdirSorted(join(zone, "a"))).toEqual(["b"]); expect(await readdirSorted(join(zone, "a", "b"))).toEqual(["c"]); - // The unsafe alias is reported as an error and nothing is installed. - expect(err).toContain("Refusing to install package with invalid name"); + // The unsafe alias is reported as an error before the tarball is even + // requested, and nothing is installed. + expect(err).toContain('Invalid dependency name "x/../../../.."'); + expect(urls).toEqual([]); expect(out).not.toContain("1 package installed"); expect(exitCode).not.toBe(0); }); }); +describe("dependency names containing terminal control characters", () => { + // OSC 52 (write to the clipboard) followed by CSI 2J (clear the screen). A + // registry serves it as an ordinary JSON string (`"ev\u001b]52;..."`), so it + // reaches bun like any other dependency name. + const controlName = "ev\x1b]52;c;aGkK\x07\x1b[2Jil"; + // How bun reports it: with the control characters spelled out. + const escapedName = String.raw`ev\x1b]52;c;aGkK\x07\x1b[2Jil`; + // Forces the progress line, which echoes the name of every manifest being + // fetched, even though stderr is a pipe here. + const progressEnv = { ...env, BUN_INSTALL_PROGRESS: "1" }; + + /** + * The dummy registry, which has a manifest for every name asked of it, plus: + * the control-character package really is installable (its tarball is + * bar's) and `bar@0.0.2` declares `barDeps`. + */ + function registry(ctx: TestContext, urls: string[], barDeps: object = {}) { + const dummy = dummyRegistryForContext(ctx, urls); + return async (req: Request) => { + const name = decodeURIComponent(new URL(req.url).pathname.slice(`/${ctx.id}/`.length)); + if (name.endsWith(".tgz") && name.includes(controlName)) { + urls.push(req.url); + return new Response(file(join(import.meta.dir, "bar-0.0.2.tgz"))); + } + const res = await dummy(req); + if (name !== "bar") return res; + const manifest = await res.json(); + Object.assign(manifest.versions["0.0.2"], barDeps); + return Response.json(manifest); + }; + } + + function assertNameNeverLeaked(out: string, err: string, urls: string[]) { + expect(out).not.toContain(controlName); + expect(err).not.toContain(controlName); + expect(err).not.toContain("\x1b]52;"); + expect(urls.filter(url => url.includes("%1B") || url.includes("\x1b"))).toEqual([]); + } + + /** Entries of node_modules other than the cache this registry setup (`cache = false`) puts there. */ + async function installed(ctx: TestContext) { + const entries = await readdirSorted(join(ctx.package_dir, "node_modules")).catch(() => []); + return entries.filter(entry => entry !== ".cache"); + } + + // A dependency whose key repeats its specifier is how `bun add ` + // records a dependency it has not named yet, so such a key is exempt from the + // folder name rules, but not from this one: git tasks put it on the progress + // line as-is. + const gitSpecifier = `git+https://127.0.0.1:9/${controlName}.git`; + const escapedGitSpecifier = `git+https://127.0.0.1:9/${escapedName}.git`; + const manifestCases: [string, object, string][] = [ + ["a dependency name", { dependencies: { [controlName]: "0.0.2" } }, escapedName], + ["an unnamed git dependency", { dependencies: { [gitSpecifier]: gitSpecifier } }, escapedGitSpecifier], + ]; + for (const [description, barDeps, expectedName] of manifestCases) { + it(`rejects ${description} declared by a package manifest before requesting or printing it`, async () => { + await withContext(defaultOpts, async ctx => { + const urls: string[] = []; + setContextHandler(ctx, registry(ctx, urls, barDeps)); + await writeFile( + join(ctx.package_dir, "package.json"), + JSON.stringify({ name: "foo", version: "0.0.1", dependencies: { bar: "0.0.2" } }), + ); + + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: ctx.package_dir, + stdout: "pipe", + stderr: "pipe", + env: progressEnv, + }); + const [out, err, exitCode] = await Promise.all([stdout.text(), stderr.text(), exited]); + + expect(err).toContain(`error: Invalid dependency name "${expectedName}"`); + assertNameNeverLeaked(out, err, urls); + expect(urls).toContain(`${ctx.registry_url}bar`); + expect(await installed(ctx)).toEqual([]); + expect(exitCode).toBe(1); + }); + }); + } + + it("keeps the unnamed form for specifiers without control characters", async () => { + await withContext(defaultOpts, async ctx => { + const urls: string[] = []; + setContextHandler(ctx, registry(ctx, urls)); + await writeFile(join(ctx.package_dir, "package.json"), JSON.stringify({ name: "foo", version: "0.0.1" })); + + // The alias of this dependency is the specifier until the tarball has + // been resolved: full of characters a folder name may not contain. + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "add", `${ctx.registry_url}bar-0.0.2.tgz`], + cwd: ctx.package_dir, + stdout: "pipe", + stderr: "pipe", + env, + }); + const [out, err, exitCode] = await Promise.all([stdout.text(), stderr.text(), exited]); + + expect(err).not.toContain("Invalid dependency name"); + expect(out).toContain("installed bar@"); + expect(await installed(ctx)).toEqual(["bar"]); + expect(exitCode).toBe(0); + }); + }); + + it("only warns when the name is declared as an optional dependency", async () => { + await withContext(defaultOpts, async ctx => { + const urls: string[] = []; + setContextHandler(ctx, registry(ctx, urls, { optionalDependencies: { [controlName]: "0.0.2" } })); + await writeFile( + join(ctx.package_dir, "package.json"), + JSON.stringify({ name: "foo", version: "0.0.1", dependencies: { bar: "0.0.2" } }), + ); + + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: ctx.package_dir, + stdout: "pipe", + stderr: "pipe", + env: progressEnv, + }); + const [out, err, exitCode] = await Promise.all([stdout.text(), stderr.text(), exited]); + + expect(err).toContain(`warn: Invalid dependency name "${escapedName}"`); + assertNameNeverLeaked(out, err, urls); + expect(await installed(ctx)).toEqual(["bar"]); + expect(out).toContain("1 package installed"); + expect(exitCode).toBe(0); + }); + }); + + // [description, root dependencies, how the unresolved dependency is summarized] + const aliasCases: [string, Record, string][] = [ + ["as the alias of a dependency", { [controlName]: "npm:bar@0.0.2" }, `${escapedName}@npm:bar@0.0.2`], + [ + "as the target of an npm: alias", + { "safe-alias": `npm:${controlName}@0.0.2` }, + `safe-alias@npm:${escapedName}@0.0.2`, + ], + ]; + for (const [description, dependencies, summary] of aliasCases) { + it(`rejects a name used ${description}`, async () => { + await withContext(defaultOpts, async ctx => { + const urls: string[] = []; + setContextHandler(ctx, registry(ctx, urls)); + await writeFile( + join(ctx.package_dir, "package.json"), + JSON.stringify({ name: "foo", version: "0.0.1", dependencies }), + ); + + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), "install"], + cwd: ctx.package_dir, + stdout: "pipe", + stderr: "pipe", + env, + }); + const [out, err, exitCode] = await Promise.all([stdout.text(), stderr.text(), exited]); + + expect(err).toContain(`error: Invalid dependency name "${escapedName}"`); + expect(err).toContain(`error: ${summary} failed to resolve`); + assertNameNeverLeaked(out, err, urls); + expect(await installed(ctx)).toEqual([]); + expect(exitCode).toBe(1); + }); + }); + } + + it("rejects a package name found in bun.lock instead of installing or listing it", async () => { + await withContext(defaultOpts, async ctx => { + const urls: string[] = []; + setContextHandler(ctx, registry(ctx, urls)); + await Promise.all([ + writeFile( + join(ctx.package_dir, "package.json"), + JSON.stringify({ name: "foo", version: "0.0.1", dependencies: { [controlName]: "0.0.2" } }), + ), + writeFile( + join(ctx.package_dir, "bun.lock"), + textLockfile(1, { + workspaces: { "": { name: "foo", dependencies: { [controlName]: "0.0.2" } } }, + packages: { [controlName]: [`${controlName}@0.0.2`, `${ctx.registry_url}bar-0.0.2.tgz`, {}, ""] }, + }), + ), + ]); + + // `bun pm ls` prints straight from the lockfile, so it runs first, before + // `bun install` gets a chance to touch it. + const commands: [string[], string][] = [ + [["pm", "ls", "--all"], "Error loading lockfile: InvalidLockfile"], + [["install"], "Invalid package name"], + ]; + for (const [args, expectedError] of commands) { + const { stdout, stderr, exited } = spawn({ + cmd: [bunExe(), ...args], + cwd: ctx.package_dir, + stdout: "pipe", + stderr: "pipe", + env, + }); + const [out, err, exitCode] = await Promise.all([stdout.text(), stderr.text(), exited]); + + expect(err).toContain(expectedError); + assertNameNeverLeaked(out, err, urls); + expect(exitCode).toBe(1); + } + // Ignoring the rejected lockfile must not make `bun install` fall back to + // resolving the same name from package.json either. + expect(urls).toEqual([]); + expect(await installed(ctx)).toEqual([]); + }); + }); +}); + it("does not install transitive file: dependencies that point outside their package", async () => { // A dependency declared by a non-workspace package (here: a folder dependency // of the project) uses a file: specifier pointing at an absolute path outside diff --git a/test/cli/install/isolated-install.test.ts b/test/cli/install/isolated-install.test.ts index a4ff2a70903f..e99cbbccff35 100644 --- a/test/cli/install/isolated-install.test.ts +++ b/test/cli/install/isolated-install.test.ts @@ -2768,7 +2768,8 @@ test("rejects dependency aliases that traverse outside node_modules", async () = // A (transitively) malicious package.json can use an arbitrary string as a // dependency alias. The alias becomes a `node_modules/` path // component in the isolated store layout, so a `..` segment lets it plant - // symlinks outside of node_modules. + // symlinks outside of node_modules. Such an alias is refused while resolving + // (the installer has its own check as well, see the next test). await write( packageJson, JSON.stringify({ @@ -2788,7 +2789,7 @@ test("rejects dependency aliases that traverse outside node_modules", async () = }); const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); - expect(stderr).toContain("is not a valid install folder name"); + expect(stderr).toContain('Invalid dependency name "../pwned-by-alias"'); // Nothing may be created outside of node_modules. `lstatSync` instead of // `existsSync` because the escaped artifact would be a dangling symlink. expect(() => lstatSync(join(packageDir, "pwned-by-alias"))).toThrow(); From 599b097f5e1a10e9e188c0b4d148160fd4b39207 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:50:48 +0000 Subject: [PATCH 2/3] ci: retrigger From 59cf72eeee978c0bc084d9948ec18c7c43c621ed Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:56:06 +0000 Subject: [PATCH 3/3] install: tighten comments around the name checks --- src/bun_core/fmt.rs | 9 +++------ src/install/PackageInstaller.rs | 8 +++----- .../PackageManager/PackageManagerEnqueue.rs | 14 +++++--------- src/install/dependency.rs | 13 +++++-------- src/install/lockfile/Tree.rs | 6 ++---- 5 files changed, 18 insertions(+), 32 deletions(-) diff --git a/src/bun_core/fmt.rs b/src/bun_core/fmt.rs index 32ff0333cd5b..528fd29ae191 100644 --- a/src/bun_core/fmt.rs +++ b/src/bun_core/fmt.rs @@ -3329,12 +3329,9 @@ fn escape_powershell_impl(str: &[u8], writer: &mut impl fmt::Write) -> fmt::Resu // escapeControlChars // ─────────────────────────────────────────────────────────────────────────── -/// Renders the wrapped `Display` with C0 controls, DEL and C1 controls -/// spelled out (`\n`, `\r`, `\t`, `\x1b`, `\x7f`, `\u009b`) instead of -/// written raw. For text somebody else authored (a registry manifest, a -/// dependency's `package.json`): printed raw, an ESC/CR/C1 sequence can erase -/// or repaint the line it is shown on and a newline can forge further lines -/// of our output. Everything else passes through unchanged. +/// Renders the wrapped `Display` with C0 controls, DEL and C1 controls spelled +/// out (`\n`, `\x1b`, `\x7f`, `\u009b`) instead of written raw, for text a +/// registry or a package authored; everything else passes through unchanged. pub struct EscapeControlChars(pub T); /// [`EscapeControlChars`] over raw bytes; invalid UTF-8 renders as U+FFFD. diff --git a/src/install/PackageInstaller.rs b/src/install/PackageInstaller.rs index 476501a4c636..ac8b18d445c0 100644 --- a/src/install/PackageInstaller.rs +++ b/src/install/PackageInstaller.rs @@ -403,11 +403,9 @@ impl<'a> LazyPackageDestinationDir<'a> { } } -/// A dependency alias becomes the install destination inside `node_modules` -/// (the existing entry is renamed aside, deleted, and re-created). On top of -/// `is_safe_install_folder_name` (empty names, `.`/`..` components, drive -/// letters, backslashes, control characters), reject any separator other than -/// the single `/` in a scoped name (`@scope/name`). +/// The alias is the install destination inside `node_modules` (renamed aside, +/// deleted and re-created), so on top of `is_safe_install_folder_name` it must +/// be a single path component, or two for a scoped name. pub(crate) fn alias_is_safe_install_target(alias: &[u8]) -> bool { if alias.len() >= MAX_PATH_BYTES || !crate::dependency::is_safe_install_folder_name(alias) { return false; diff --git a/src/install/PackageManager/PackageManagerEnqueue.rs b/src/install/PackageManager/PackageManagerEnqueue.rs index 131342977946..f3bc05ae29ed 100644 --- a/src/install/PackageManager/PackageManagerEnqueue.rs +++ b/src/install/PackageManager/PackageManagerEnqueue.rs @@ -771,18 +771,14 @@ pub fn enqueue_dependency_with_main_and_success_fn( break 'version dependency.version.clone(); }; - // The alias becomes the `node_modules/` folder and, for registry - // dependencies, `name` becomes the request, the progress line and the - // package name; both come from untrusted manifests. Refuse to resolve (and - // so to fetch or print) either when it is unsafe, reporting it the way an - // unresolvable dependency is reported. Empty names are tolerated like in - // the tree builder. + // Refuse an unsafe alias (the future `node_modules/` folder) or registry + // name (the request and the package name) here, before either is fetched + // or printed. Empty names are tolerated, as in the tree builder. let invalid_name = { let alias = this.lockfile.str(&dependency.name); let alias_is_safe = if alias == this.lockfile.str(&dependency.version.literal) { - // `bun add ` stores the specifier as the alias until - // `assign_resolution` replaces it with the resolved package's - // name, so it never becomes a folder, but it is still printed. + // `bun add ` uses the specifier as the alias until + // `assign_resolution` names it: never a folder, but still printed. !dependency::contains_control_character(alias) } else { alias.is_empty() || dependency::is_safe_install_folder_name(alias) diff --git a/src/install/dependency.rs b/src/install/dependency.rs index 06993b24680c..8e3732297153 100644 --- a/src/install/dependency.rs +++ b/src/install/dependency.rs @@ -566,11 +566,9 @@ pub fn is_scoped_package_name(name: &[u8]) -> Result { Err(PackageNameError::InvalidPackageName) } -/// A dependency name/alias becomes a directory under `node_modules/` and is -/// echoed in progress and error output. Names come from untrusted -/// `package.json` / manifest keys, so reject anything that could resolve -/// outside that directory or carry terminal control sequences into the -/// output. `@scope/name` stays valid. +/// Names come from untrusted `package.json` / manifest keys and end up as +/// `node_modules/` directories and in progress and error output, so reject path +/// escapes and terminal control characters. `@scope/name` stays valid. pub(crate) fn is_safe_install_folder_name(name: &[u8]) -> bool { if name.is_empty() || contains_control_character(name) { return false; @@ -588,9 +586,8 @@ pub(crate) fn is_safe_install_folder_name(name: &[u8]) -> bool { true } -/// The characters `bun_core::fmt::escape_control_chars` would have to escape: -/// C0 controls and DEL, plus the UTF-8 encoding of the C1 controls -/// (U+0080..=U+009F, `C2 80`..`C2 9F`), which terminals interpret as well. +/// C0 controls and DEL, plus UTF-8 encoded C1 controls (`C2 80`..`C2 9F`, +/// U+0080..=U+009F), which terminals interpret too. pub(crate) fn contains_control_character(name: &[u8]) -> bool { name.iter().enumerate().any(|(i, &byte)| { byte.is_ascii_control() diff --git a/src/install/lockfile/Tree.rs b/src/install/lockfile/Tree.rs index e5e942735bb1..840c3df70994 100644 --- a/src/install/lockfile/Tree.rs +++ b/src/install/lockfile/Tree.rs @@ -791,10 +791,8 @@ impl Tree { // don't treat it as unsafe — match the lockfile parser and isolated // installer (`bun.lock.rs`, `isolated_install.rs`) which guard // `!name.is_empty()` here rather than failing the whole install. - // Neither does an unresolved dependency (it is skipped below, or as - // an optional peer bound to an already checked one), and - // `enqueue_dependency_with_main_and_success_fn` already reported - // its name if that is why it did not resolve. + // Neither does an unresolved dependency, and if its name is why it + // did not resolve, enqueue already reported it. let dependency_name = dependency .name .slice(lockfile.buffers.string_bytes.as_slice());