Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
44 changes: 44 additions & 0 deletions src/bun_core/fmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3325,6 +3325,50 @@ fn escape_powershell_impl(str: &[u8], writer: &mut impl fmt::Write) -> fmt::Resu
write_bytes(writer, remain)
}

// ───────────────────────────────────────────────────────────────────────────
// escapeControlChars
// ───────────────────────────────────────────────────────────────────────────
Comment thread
robobun marked this conversation as resolved.

/// 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.
Comment thread
robobun marked this conversation as resolved.
pub struct EscapeControlChars<T>(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<T: Display> Display for EscapeControlChars<T> {
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.

Expand Down
21 changes: 6 additions & 15 deletions src/install/PackageInstaller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,24 +403,15 @@ 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 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.
Comment thread
robobun marked this conversation as resolved.
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'@')
}

Expand Down Expand Up @@ -1295,7 +1286,7 @@ impl<'a> PackageInstaller<'a> {
if log_level != Options::LogLevel::Silent {
bun_core::pretty_errorln!(
"<r><red>error<r>: refusing to install dependency with unsafe name <b>{}<r>",
bstr::BStr::new(alias.slice(string_buf!())),
bun_core::fmt::escape_control_chars(alias.slice(string_buf!())),
);
}
self.summary.fail += 1;
Expand Down
49 changes: 49 additions & 0 deletions src/install/PackageManager/PackageManagerEnqueue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,55 @@ pub fn enqueue_dependency_with_main_and_success_fn(
version_was_replaced = false;
break 'version dependency.version.clone();
};

// 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.
Comment thread
robobun marked this conversation as resolved.
let invalid_name = {
let alias = this.lockfile.str(&dependency.name);
let alias_is_safe = if alias == this.lockfile.str(&dependency.version.literal) {
// `bun add <specifier>` uses the specifier as the alias until
// `assign_resolution` names it: never a folder, but still printed.
Comment thread
robobun marked this conversation as resolved.
!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<Npm::PackageManifest> = None;

match version.tag {
Expand Down
12 changes: 9 additions & 3 deletions src/install/PackageManager/PackageManagerResolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,14 +352,20 @@ impl PackageManager {
{
Output::err_generic(
"<b>{}<r><d> failed to resolve<r>",
(failed_dep.version.literal.fmt(string_buf),),
(bun_core::fmt::escape_control_chars(
failed_dep.version.literal.slice(string_buf),
),),
);
} else {
Output::err_generic(
"<b>{}<r><d>@<b>{}<r><d> failed to resolve<r>",
(
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),
),
),
);
}
Expand Down
2 changes: 1 addition & 1 deletion src/install/TarballStream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
22 changes: 17 additions & 5 deletions src/install/dependency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -566,26 +566,38 @@ pub fn is_scoped_package_name(name: &[u8]) -> Result<bool, PackageNameError> {
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.
/// 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.
Comment thread
robobun marked this conversation as resolved.
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;
}

for component in strings::split(name, b"/") {
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;
}
}

true
}

/// C0 controls and DEL, plus UTF-8 encoded C1 controls (`C2 80`..`C2 9F`,
/// U+0080..=U+009F), which terminals interpret too.
Comment thread
robobun marked this conversation as resolved.
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'+') {
Expand Down
3 changes: 3 additions & 0 deletions src/install/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ pub enum Error {
Failed,
#[error("UnrecognizedDependencyFormat")]
UnrecognizedDependencyFormat,
#[error("InvalidDependencyName")]
InvalidDependencyName,
#[error("No global directory found")]
NoGlobalDirectoryFound,
#[error("InvalidPackageID")]
Expand Down Expand Up @@ -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",
Expand Down
4 changes: 2 additions & 2 deletions src/install/extract_tarball.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/install/isolated_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
11 changes: 6 additions & 5 deletions src/install/lockfile/Tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -791,17 +791,18 @@ 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, and if its name is why it
// did not resolve, enqueue already reported it.
Comment thread
robobun marked this conversation as resolved.
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;
}
Expand Down
6 changes: 3 additions & 3 deletions src/install/lockfile/bun.lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)),
),
);
}
Expand Down
6 changes: 4 additions & 2 deletions test/cli/install/bun-install-registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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([]);
Expand Down
Loading
Loading