install: reject dependency names containing control characters - #38615
install: reject dependency names containing control characters#38615robobun wants to merge 3 commits into
Conversation
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/<name> 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.
|
Status: reproduced on 1.4.0-canary with a loopback registry whose CI on the current head (59cf72e, build 96362): 177 of 179 jobs passed with no test failures; the remaining two are the |
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and the automated bug-hunting pass found no issues. Because it adds security-sensitive input validation on the critical dependency-resolution path and changes the semantics of a shared validator (is_safe_install_folder_name) that several installers, the tree builder, and the lockfile parser all depend on, a human look would still be worthwhile.
What was reviewed
contains_control_charactercorrectly distinguishes UTF-8-encoded C1 (C2 80..9F) from ordinary continuation bytes; the dropped\0incontains_anyis now covered by the control-char check.alias_is_safe_install_targetrefactor preserves the prior rules (empty /./../\\/:/ NUL / component count) via delegation.- The
alias == version.literalplaceholder exemption and theis_required()error-vs-warn split in the enqueue check; a candidate concern that optional peers would error instead of warn was examined and ruled out. - The
pkg_id != invalid_package_idguard inTree.rs— unresolved deps skip the folder-name check there because enqueue already reported them, avoiding a duplicate error.
Extended reasoning...
Overview
This PR hardens bun install against dependency names containing terminal control characters (C0/DEL/C1), which a hostile registry manifest or lockfile could otherwise use to inject escape sequences into progress lines, error messages, and node_modules/ folder names. It touches 14 files across src/install/ and src/bun_core/: a new escape_control_chars formatter, an extended is_safe_install_folder_name (now also rejects control characters), a new pre-resolution validation gate in enqueue_dependency_with_main_and_success_fn, a refactor of alias_is_safe_install_target to delegate to the shared validator, a new Error::InvalidDependencyName variant, escaping applied to ~8 error-message sites, and 7 new tests plus 3 updated tests.
Security risks
This is itself a security fix (terminal escape injection / defense-in-depth for path traversal via control chars). The main risk to review for is whether the tightened validator now rejects legitimate inputs it previously accepted — the PR description addresses this (non-ASCII names with continuation bytes still pass; the placeholder alias for bun add <specifier> is exempted from folder-name rules), and the C1 detection is sequence-based rather than byte-based. No new attack surface is introduced.
Level of scrutiny
High. The enqueue function is on the hot path for every dependency of every install, and is_safe_install_folder_name is called from the tree builder, both installers, the lockfile parser, tarball extraction, and bin-name validation — a false positive here would break installs of legitimate packages. The refactor of alias_is_safe_install_target changes how it derives its rules. The Tree.rs guard change alters when the second-line-of-defense check fires. These are all well-reasoned in the PR body, but the blast radius warrants a maintainer confirming the exemption logic (placeholder alias, empty name, optional/peer behavior) matches intent.
Other factors
Test coverage is strong: 7 new tests covering manifest-declared names, unnamed git deps, the bun add placeholder exemption, optional-dependency warn-not-error, alias vs npm: target, and hostile lockfiles via both bun pm ls and bun install; each asserts the escaped message appears, the raw bytes never leak to either stream or the request log, nothing is installed, and the exit code. Three existing tests were updated to observe the earlier rejection layer. The PR description documents extensive test-suite runs and clippy. No prior reviewer comments to address.
|
Not a duplicate of any of the four, but they are related, so for whoever triages:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Because it adds a new validation gate at the central enqueue_dependency seam, tightens the shared is_safe_install_folder_name path validator used by the tree builder / installers / lockfile parser, and needs landing-order coordination with #38525 / #38536 / #38557 / #34737, a human look is still worthwhile.
What was reviewed:
alias_is_safe_install_targetrefactor is behavior-preserving plus the new control-char rule (NUL moved from the\\:byte set intocontains_control_character, still rejected).contains_control_character's C1 check is UTF-8-sequence-based (0xC2 0x80..0x9F), so ordinary non-ASCII names whose continuation bytes fall in0x80..0x9Fare not rejected.- The new
pkg_id != invalid_package_idguard inTree.rsonly skips the name check for unresolved deps, which never get a folder (filtered inclean()/continue 'dep). - Optional/peer handling: optional-peer deps return before the new check; optional deps warn and skip, matching how the same function reports other unresolvable optionals.
Extended reasoning...
Overview
This PR hardens bun install against dependency names containing terminal control characters (C0, DEL, UTF-8 C1) supplied by an untrusted registry manifest or a hand-edited bun.lock. It adds bun_core::fmt::escape_control_chars (a Display wrapper that spells out control characters), extends dependency::is_safe_install_folder_name to reject control characters, inserts a name-validation gate in enqueue_dependency_with_main_and_success_fn before any request/progress-line/package creation, refactors alias_is_safe_install_target to delegate to the shared validator, adds Error::InvalidDependencyName, and routes ~9 error/warning messages that echo a name through the new escaper. Three existing tests were updated because the rejection now surfaces at an earlier layer, and 7 new tests cover manifest-declared names, git placeholders, optional deps, npm: aliases (both directions), and hostile lockfiles.
Security risks
The change is itself a security hardening: it closes a terminal-escape-injection and directory-name-injection vector from registry-controlled data. The risk to weigh is whether the new gate over-rejects (breaking a legitimate name) or under-rejects (a bypass). The C1 check is byte-sequence-aware so multi-byte non-ASCII names are preserved; NUL moved from the explicit byte set into is_ascii_control() so it is still rejected. The bun add <specifier> placeholder alias (alias == literal) is exempted from the folder rules but still checked for control characters, which the tests exercise. No validation was removed — the tree builder / installers / lockfile parser keep their own checks as a second line.
Level of scrutiny
High. This is security-sensitive code (untrusted-input validation that becomes filesystem paths and terminal output) at the single seam every dependency of every source flows through. The repo's own review guidance calls out "security checks fail closed and cover every path to the protected effect" and "when tightening validation, enumerate every legitimate input class" — both apply directly here and merit a maintainer's eyes.
Other factors
- Four open PRs (#38525, #38536, #38557, #34737) add the same or an adjacent helper at the same locations; a maintainer should pick the landing order.
- Three existing tests were edited to observe the earlier layer's message; one (
bun-install-registry.test.ts) now accepts either of two messages, which is a small assertion loosening a reviewer should confirm is intended. - The comment-cop bot flagged several comments; the author tightened them and justified the remainder as doc comments / non-local invariants — those threads are resolved.
- No CI results are visible in the timeline yet.
|
Heads-up on the shared struct EscapeControlCharsWriter<'a, 'f>(&'a mut Formatter<'f>);
impl fmt::Write for EscapeControlCharsWriter<'_, '_> {
fn write_str(&mut self, s: &str) -> fmt::Result {
let bytes = s.as_bytes();
let mut start = 0;
let mut cursor = 0;
// `\` doubles as the quote char so the scan stops at nothing else extra.
while let Some(offset) =
strings::index_of_needs_escape_for_java_script_string(&bytes[cursor..], b'\\')
{
let i = cursor + offset as usize;
let (code_point, len) = match bytes[i] {
byte @ (0x00..=0x1F | 0x7F) => (byte as u32, 1),
0xC2 if matches!(bytes.get(i + 1), Some(0x80..=0x9F)) => (bytes[i + 1] as u32, 2),
byte => {
let char_len = strings::wtf8_byte_sequence_length(byte) as usize;
cursor = (i + char_len).min(bytes.len());
continue;
}
};
self.0.write_str(&s[start..i])?;
match code_point {
0x0A => self.0.write_str("\\n")?,
0x0D => self.0.write_str("\\r")?,
0x09 => self.0.write_str("\\t")?,
0x00..=0x7F => write!(self.0, "\\x{:02x}", code_point)?,
_ => write!(self.0, "\\u{:04x}", code_point)?,
}
start = i + len;
cursor = start;
}
self.0.write_str(&s[start..])
}
} |
|
The The middle line is the one this PR removes. It also echoes credentials embedded in the URL ( This path is not covered by the tests here (the No separate PR for this; the fix is the one already in this diff. |
3 participants
Problem
ev+ OSC 52 "write to the clipboard" + CSI 2J "clear the screen" +il).bun installaccepts it: the name is requested from the registry, written raw onto the progress line (ev<ESC>]52;c;aGkK<BEL><ESC>[2Jil [9/8]), printed raw inerror: InvalidURL downloading tarball <name>@1.0.0and in the<name>@1.0.0 failed to resolvesummary, stored in bun.lock, andnode_modules/ev<ESC>]52;...il/is created;bun pm lsandbun whythen print the raw bytes again on every run. A hand-edited bun.lock with such a package name behaves the same.enqueue_dependency_with_main_and_success_fn(src/install/PackageManager/PackageManagerEnqueue.rs) turns the name into the manifest request and later the package name without looking at it;is_safe_install_folder_name(src/install/dependency.rs), which the tree builder, the installers and the bun.lock parser apply to folder names, only knows about path rules (..,\,:); and the messages that report a bad or unresolvable name (Tree.rs,isolated_install.rs,PackageInstaller.rs,extract_tarball.rs,TarballStream.rs,PackageManagerResolution.rs::verify_resolutions,bun.lock.rs::dependency_resolution_failure) all wrote the name throughBStr/fmt::s, i.e. raw.Fix
is_safe_install_folder_nameadditionally rejects names containing C0 controls, DEL, or a UTF-8 encoded C1 control (C2 80..C2 9F, which some terminals also execute). This covers every place that already used it: the hoisted tree builder, the hoisted and isolated installers (alias_is_safe_install_targetnow delegates to it instead of duplicating its rules), package names read from bun.lock (sobun pm ls/bun installrefuse a hostile lockfile instead of printing or installing from it), bin names, and tarball extraction.enqueue_dependency_with_main_and_success_fnvalidates the dependency before resolving it, which is before anything is requested, shown on the progress line or turned into a package: the alias (the futurenode_modules/folder) with the full validator, and for npm / dist-tag dependencies also the resolved registry name (npm:alias targets, overrides and catalog replacements included). The placeholder aliasbun add <specifier>creates (alias == version literal, replaced byassign_resolutiononce the package is known) is only checked for control characters, since it is printed by the git tasks but never becomes a folder. A required dependency is reported aserror: Invalid dependency name "..."(the install fails, as the tree builder's check already did for..names), an optional one as a warning and skipped, matching how an unresolvable dependency is reported today. Auto-install callers get a newError::InvalidDependencyNamethrough theirfail_fn.bun_core::fmt::escape_control_charsrenders text with C0/DEL/C1 spelled out (\x1b,\n, and a four-digit backslash-u spelling for C1 code points), everything else verbatim; all the rejection messages above and the two "failed to resolve" summaries use it, so reporting a rejected name is byte-identical to before for ordinary names and cannot replay a hostile one. The same helper (same name, table and shape) is being added by install: escape control characters in the bun pm untrusted/trust script listing #38525, pm view: escape control characters coming from the registry #38536 and audit: fail closed on malformed registry responses and escape control characters in the report #38557 for other commands, and install: escape control characters in resolutions, specifiers, bin names and registry error text #38631 (which escapes the non-name strings: resolutions, specifiers, bin names, registry error text) reuses it and this PR'sPackageManagerResolution.rshunk verbatim; whichever lands first, the others reduce to dropping their copy.does not extract a tarball for a dependency alias containing '..'(the tarball is no longer even requested), the isolatedrejects dependency aliases that traverse outside node_modules(the next test still exercises the installer's own check), and the registry host-pinning test, whose assertion now accepts either layer and still checks that no request reaches the other host.test/cli/install/bun-install.test.ts,describe("dependency names containing terminal control characters"): manifest-declared name (withBUN_INSTALL_PROGRESS=1, so the progress line is captured), unnamed git dependency in a manifest,bun add <tarball url>placeholder still allowed, optional dependency only warns, alias of a dependency,npm:alias target, package name in bun.lock (bun pm lsandbun install). Each asserts the escaped message, that the raw name appears in neither stream, that nothing with that name was requested or installed, and the exit code. All 7 fail on a build without thesrc/changes (the progress line shows the raw bytes, the package installs,bun pm lsprints it) and pass with it.0x80continuation byte (okĀname) still passes and prints verbatim.bun-install.test.ts(remaining failures need network),bun-install-registry.test.ts,isolated-install.test.ts,bun-add.test.ts,bun-workspaces.test.ts,overrides.test.ts,catalogs.test.ts,bun-lock.test.ts,bun-update.test.ts,bun-pm*.test.ts,migration/migrate.test.ts,bunx.test.ts,test/regression/issue/31652.test.ts(empty-name optional dependency stays tolerated),cargo clippy -p bun_core -p bun_install,test/internal/source-lints.nameis invalid, where bun currently writes a bun.lock entry that the parser then refuses (pre-existing for:/\names, now also for control characters).Background
dependenciesand becomes thenode_modules/<alias>folder. The registry name is what is fetched and becomes the package's name in the lockfile; it equals the alias except fornpm:aliases, overrides and catalogs, where it comes out of the version string. Both come out of whatever manifest declared the dependency, so a transitive package controls both.enqueue_dependency_with_main_and_success_fnfor every dependency of every source (package.json, fetched manifests, lockfiles); a dependency left unresolved there is reported byverify_resolutionsand skipped by the tree builder. Folder creation happens later from the lockfile, which is why the folder validator stays in the tree builder, both installers and the bun.lock parser as the second line of defense for lockfiles that never go through resolution.bun add ./x.tgz/bun add github:o/rstore the specifier itself as the alias until the package is known (assign_resolutionswaps in the package name when alias == version literal); that is why such an alias must not be held to the folder rules.C2 80..C2 9F, and e.g. U+009B is an alternative spelling ofESC [. The check is sequence-based because bytes0x80..0x9Falso occur as continuation bytes of ordinary non-ASCII names.Before / after output
Unfixed build, manifest-declared name, stderr bytes (
BUN_INSTALL_PROGRESS=1):Fixed build:
Fixed build,
bun pm lson a bun.lock carrying such a package name:no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-registry.test.ts test/cli/install/bun-install.test.ts test/cli/install/isolated-install.test.ts