Conversation
|
Warning Review limit reached
This review includes 6 billable files and costs up to $1.50.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 10 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (6)
Comment |
|
Reproduced on the released build (1.4.0-canary, eabb96d) with a local tarball whose package.json declares Fix and tests are in this PR ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Review follow-ups: On the possible duplicate, #35461: that PR adds path-form |
There was a problem hiding this comment.
I re-reviewed after 477e53c and e87aa0e — the earlier nit and the comment-cop flag are both addressed, and this pass found no bugs. Since this is a security guard in the install path (deciding which declarers may point link: at directories outside the project) and it overlaps with the containment rule in #35461 — which draws the line at root/workspace only, whereas this PR also trusts file: directories — a maintainer should sign off on where that boundary sits.
Checked: bin_target_escapes_package_dir covers absolute paths, Windows drive-prefix and post-normalization .. (the x/../../ case); the !version_was_replaced gate correctly exempts root overrides/resolutions/catalog substitutions; get_parent_pkg_of_dependency returning None for auto-install root-appended edges lets them through. Name-form link:<name> never trips the escape check, so the documented bun link flow is untouched.
Extended reasoning...
Overview
Adds a guard in the Tag::Symlink arm of enqueue_dependency_with_main_and_success_fn (src/install/PackageManager/PackageManagerEnqueue.rs) that leaves a link: edge unresolved when its value climbs out with .. or is absolute AND the declaring package was installed from the cache (registry/git/tarball) rather than read from the project. Two small helpers support it: resolution::Tag::is_local_package (Root/Workspace/Folder) and Lockfile::get_parent_pkg_of_dependency (linear scan of package dependency ranges). ~260 lines of tests in bun-install.test.ts cover tarball/git/registry declarers × three escaping shapes, peer/optional behaviour, and five stays-allowed cases (root, workspace, overrides, resolutions, file: directory, name-form link).
Security risks
This IS the security-sensitive change — it closes a symlink-escape where a published package could get an arbitrary host directory linked into node_modules and its name written to bun.lock. The guard fails closed for required edges (error + exit 1, no lockfile) and silently skips optional ones. The escape predicate is the existing bin_target_escapes_package_dir, already used for the neighbouring file: directory containment, so the traversal detection itself is not new code. The one policy question is whether Tag::Folder belongs on the trusted side: this PR says yes (a file: directory's package.json is a file the project itself named, consistent with #38986/#33106); #35461 says no. That's a maintainer call, not a bug.
Level of scrutiny
High. Package-install path traversal is exactly the class of bug REVIEW.md's Security section calls out ("any string … that becomes a path rejects … .."). The Rust change is small (~40 lines) and reuses existing predicates, but the boundary it draws affects what third-party manifests can do on every user's machine, and there is an open PR (#35461) that rewrites the same arm with a different boundary. A human should confirm which lands and in what order.
Other factors
My previous nit (undrained stdout pipe in packDeclarer) was fixed in 477e53c by switching to stdout: "ignore"; the comment-cop flag on the long doc comment was fixed in e87aa0e. Test coverage is thorough — refused cases assert exact error lines, no lockfile, no node_modules, exit 1; allowed cases assert the resolved row appears in bun.lock. Tests use tempDir, isolated cache/global dirs, and the local dummy registry / dumb-http git server (no external network). describe.concurrent keeps the block fast.
|
Updated 4:58 AM PT - Oct 1st, 2026
✅ @robobun, your commit 6426ffc9a1f6f1c8fbc2d40f67dba33bf236dbb8 passed in 🧪 To try this PR locally: bunx bun-pr 39023That installs a local version of the PR into your bun-39023 --bun |
…ackages installed from the cache A link: target starting with "." is resolved against the project directory and linked from the global link dir, so a registry, git or tarball package declaring "x": "link:../dir" got <project>/../dir/package.json read and recorded in bun.lock, and with a path valid from both directories the directory symlinked into node_modules under a name of its choosing. The Symlink enqueue arm now leaves such an edge unresolved, with an error for required edges, unless the declaring package is read from the project (root, workspace, file: directory) or the specifier came from the root's overrides/resolutions. Name-form link:<name> is unchanged.
|
Rebased onto main bf42a52 (was e87aa0e, now bc214ff).
|
e87aa0e to
bc214ff
Compare
…he resolver and both linkers The resolve-time guard now uses Lockfile::is_trusted_folder_dependency, the rule of the file: folder arm, instead of a private copy. The hoisted and the isolated linker repeat the check for rows loaded from bun.lock, keyed on the package (is_trusted_folder_package) so that --production does not refuse a link the root declares.
robobun
changed the title
|
Reworked after the review on bc214ff (now 6426ffc):
The block |
| if dependency_tag == dependency::version::Tag::Symlink | ||
| && dependency::link_path_escapes_root(this.lockfile.str(version.symlink())) | ||
| && !this.lockfile.is_trusted_folder_dependency(id) | ||
| { | ||
| if dependency.behavior.is_required() { | ||
| reject_link_path_of_remote_package(this, id, dependency); | ||
| } | ||
| return Ok(()); |
There was a problem hiding this comment.
🔴 Projects whose root links a path via a catalog entry (or alongside an override of the same name) now fail to install after any catalog or override edit; the base installs them. When bun.lock exists and the root row slice is rebuilt, the old root rows stay in buffers.dependencies with no declaring package, and the catalogs/overrides-changed loops re-enqueue them. For such a row is_trusted_folder_dependency(id) is false at PackageManagerEnqueue.rs:1603, so the guard logs a refusal and the install exits 1. Fix: never refuse an edge that belongs to no package (the PR text says these are left alone), or skip orphaned rows in the re-enqueue loops at install_with_manager.rs:503 and :536. [also at: src/install/PackageManager/PackageManagerEnqueue.rs:1603 - Projects whose root links a path via a catalog entry, or that add or remove a scoped override naming a root link:../x dependency, now fail to install at the next catalog or override change.]
Why this was flagged
Root package.json has dependencies { lib: "catalog:" } and catalog { lib: "link:../lib", other: "1.0.0" }; first install succeeds. The user bumps other and runs bun install again. install_with_manager.rs:324-330 points packages[0].dependencies at a new slice appended at off = lf.dependencies.len(), leaving the old root rows at their old offsets, referenced by no package. Because summary.catalogs_changed is set, the loop at install_with_manager.rs:536-561 re-enqueues each Catalog-tagged row, including the stale old root row lib@ catalog:. The catalog substitutes link:../lib, link_path_escapes_root("../lib") is true, and is_trusted_folder_dependency(id) is false because get_parent_pkg_of_dependency (lockfile.rs:794) returns None for the orphaned row. reject_link_path_of_remote_package logs refusing to resolve "lib@ catalog:": only the root package.json, a workspace, or an override may link to a path outside the project; had_errors_before_cleaning_lockfile (install_with_manager.rs:658) then triggers Global::crash() at :922. On the base branch the stale row simply re-resolves to the existing package and clean_with_logger drops it.
Verification: Regression against the base, triggered when a project with an existing bun.lock has a root dependency whose catalog value is an escaping link: path and any catalog entry is then edited. install_with_manager.rs:324-330 leaves the old root rows referenced by no package; install_with_manager.rs:535-561 re-enqueues them, and the guard at PackageManagerEnqueue.rs:1600-1610 fires because get_parent_pkg_of_dependency returns None.
| if dependency_tag == dependency::version::Tag::Symlink | ||
| && dependency::link_path_escapes_root(this.lockfile.str(version.symlink())) | ||
| && !this.lockfile.is_trusted_folder_dependency(id) |
There was a problem hiding this comment.
🔴 Projects whose root package.json supplies an escaping link: path through a scoped or ranged override now fail to install, where the base branch resolved them. The guard at PackageManagerEnqueue.rs:1603 trusts an edge only via is_trusted_folder_dependency, whose contains_name check covers plain overrides only, so a root rule like "bar>outside": "link:../outside" or "outside@*": "link:../outside" is refused as if the registry package wrote it. Fix: treat every override-substituted edge as root-authored, e.g. gate on !version_was_replaced (already in scope) or make the predicate use overrides.get(lockfile, id, name_hash) so the install-time checks in both linkers agree too.
Why this was flagged
A root package.json has overrides: { "bar>outside": "link:../outside" } (or a ranged rule "outside@*"), and registry package bar declares outside. The rule is applied at PackageManagerEnqueue.rs:836 through OverrideMap::get, which handles scoped rules (OverrideMap.rs:154-157), and version_was_replaced stays true. The new guard at PackageManagerEnqueue.rs:1601-1603 then calls is_trusted_folder_dependency(id) (lockfile.rs:898-906): the declarer is a registry package so is_dependency_of_local_package is false, and overrides.contains_name (OverrideMap.rs:291-303) only consults the flat map, never scoped. The edge is refused, reject_link_path_of_remote_package logs refusing to resolve "outside@ link:../outside" declared by bar@..., and verify_resolutions fails the install with exit 1. The base branch resolved this edge against the project and installed it. The PR description says values substituted from the root's overrides/resolutions are unchanged, but the code never consults version_was_replaced, so only plain name-keyed overrides are exempt.
Verification: The new guard (PackageManagerEnqueue.rs:1600-1608) never looks at version_was_replaced; is_trusted_folder_dependency (lockfile.rs:898-906) returns true only for a local declarer or contains_name, which reads self.map and ignores self.scoped (OverrideMap.rs:290-303). On the base commit the Tag::Symlink arm had no check at all, so such a rule resolved and the install exited 0.
| /// Leaves the project root (`..` or absolute); same rule `file:` uses. | ||
| pub(crate) fn link_path_escapes_root(target: &[u8]) -> bool { | ||
| crate::bin::bin_target_escapes_package_dir(target) |
There was a problem hiding this comment.
🟡 nit (optional): maintainers get two names for one predicate; link_path_escapes_root only forwards to bin::bin_target_escapes_package_dir, and the sibling file: arm in the same resolver function still calls the original directly (PackageManagerEnqueue.rs:2973) while the new link: arm calls the wrapper (PackageManagerEnqueue.rs:1602). Fix: call bin_target_escapes_package_dir at the three new sites and drop the wrapper, or route the file: arm and both installers' Folder/Symlink checks through the one shared name so the trust rule has a single predicate. [also at: src/install/PackageInstaller.rs:1531 - nit: maintainers get the same refusal wording hand-copied at three sites that can drift apart. sweep:only the root package.json, a workspace, or an override may link to a path outside the project The sentence is a const REASON in PackageManagerEnqueue.rs:1871 but a literal again in PackageInstaller.rs:1531 and isolated_install.rs:2111.]
Why this was flagged
src/install/dependency.rs:554-556 adds link_path_escapes_root(target) whose whole body is crate::bin::bin_target_escapes_package_dir(target); no normalization or extra rule is added. The new Symlink guard at src/install/PackageManager/PackageManagerEnqueue.rs:1602 and the two installer checks (src/install/PackageInstaller.rs:1524, src/install/isolated_install.rs:2105) call the wrapper, while the Folder arm of the same resolver function at src/install/PackageManager/PackageManagerEnqueue.rs:2973 and the hoisted installer's Folder arm at src/install/PackageInstaller.rs:1456 call bin::bin_target_escapes_package_dir directly. No runtime behavior differs from the base branch; this is a code-shape nit only: a reader auditing the security predicate must now discover that two differently named functions are the same check, and a later change to one name will not reach the other sites.
Verification: nit. src/install/dependency.rs:554-556 adds link_path_escapes_root, which only forwards to crate::bin::bin_target_escapes_package_dir. The new link: arm calls the wrapper (PackageManagerEnqueue.rs:1602) while the sibling file: arm still calls the original directly (PackageManagerEnqueue.rs:2973). Nothing observably breaks since both names resolve to the same predicate.
| if crate::dependency::link_path_escapes_root(folder) | ||
| && !self.lockfile().is_trusted_folder_package(package_id) | ||
| { |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: a bun.lock row whose link: value is longer than the path buffer makes the hoisted linker panic instead of printing the refusal error. The new refusal at src/install/PackageInstaller.rs:1524-1526 only checks escaping, so a long name-form value reaches buf[len..len + folder.len()] at PackageInstaller.rs:1558 and overflows folder_path_buf. The sibling Folder arm refuses this case with folder.len() >= self.folder_path_buf.len() at PackageInstaller.rs:1455. Fix: refuse a Symlink row whose global_link_dir.len() + 1 + folder.len() + 1 exceeds folder_path_buf.len() with the same error path, so every over-long lockfile path is reported and counted in summary.fail rather than crashing.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A bun.lock loaded by bun install (hoisted linker) holds a package row whose link value is about 1000 bytes on macOS (MAX_PATH_BYTES is 1024 at src/bun_core/util.rs:685) or about 4050 bytes on Linux (4096 at util.rs:682); rows from bun.lock skip the resolver. The value has no .. and is not absolute, so link_path_escapes_root at PackageInstaller.rs:1524 is false and the new guard does nothing. Execution reaches PackageInstaller.rs:1549-1560, which copies global_link_dir, a separator and folder into self.folder_path_buf with unchecked slice ranges, so buf[len..len + folder.len()] panics with a range-end-out-of-bounds error and the whole install aborts with a crash report instead of an error: line and a non-zero summary.fail. The base branch panics the same way, so this is pre-existing; the Folder arm at PackageInstaller.rs:1455-1473 refuses the row when folder.len() >= self.folder_path_buf.len() and continues the install.
Verification: pre-existing. bun install with the hoisted linker loads a bun.lock whose package row has a link: resolution longer than MAX_PATH_BYTES. The new guard at src/install/PackageInstaller.rs:1524-1525 is link_path_escapes_root(folder), which a long name-form value does not trip; execution reaches buf[len..len + folder.len()] at :1558 and Rust panics. The base commit's Symlink arm has the identical unchecked write.
|
Results of a pre-merge run of this pull request at 6426ffc, on Linux x64, release and ASan builds, beside main 4b02e10 and 1.4.2. They confirm the two red review threads of 11:02Z by runs. I relay them here with the script. I did not re-run them. The security half is complete. A lockfile that main wrote with the row no longer installs the link: the decoy was linked in 372 of 1,012 cells on main and in 0 here. Every refusal exits 1. False refusal 1: the root's own link after an ordinary edit. The root links a directory outside the project through a catalog. After a catalog edit of another package the install is refused, and it stays refused until mkdir -p lib app && cd app && echo '{"name":"lib","version":"1.0.0"}' > ../lib/package.json
pj() { echo "{\"name\":\"app\",\"workspaces\":{\"packages\":[],\"catalog\":{\"lib\":\"link:../lib\",\"is-number\":\"$1\"}},\"dependencies\":{\"lib\":\"catalog:\",\"is-number\":\"catalog:\"}}" > package.json; }
pj 7.0.0; bun install --linker isolated; echo "first $?"
pj 6.0.0; bun install --linker isolated; echo "after a catalog edit $?"; bun install --linker isolated; echo "again $?"Main: 0, 0, 0. This head: 0, 1, 1, with False refusal 2: a root rule that is scoped or ranged. Equal on both builds: no edit, a dependency added, a plain override added or edited. A question from the same run. |
This branch has not been deployed
1 participant
Problem
"loot": "link:../secret". bun resolves the path against the project, writes"loot": ["secret-dir@link:../secret", {}]tobun.lock, and both linkers symlink the directory intonode_modules(exit 0). Probe in Notes.Tag::Symlinkarm ofenqueue_dependency_with_main_and_success_fn(src/install/PackageManager/PackageManagerEnqueue.rs) never asks who declared the edge. Thefile:folder arm next to it already refuses an escaping path unlessLockfile::is_trusted_folder_dependencyholds (install: only constrain transitive file: targets of remote packages #33106, install: bound isolated store entry names; tarball URL credentials; file: tarballs relative to their folder package #38867), and the hoisted linker repeats that check (PackageInstaller.rs:1456).Fix
link:arm now uses the same rule: a target that leaves the project (.., also after normalization, or absolute) is refused unless the root, a workspace, afile:folder they declare, or a rootoverrides/resolutionsentry declares the edge. Required edges logrefusing to resolve "loot@link:../secret" declared by tb@./tb-1.0.0.tgz: only the root package.json, a workspace, or an override may link to a path outside the projectand the install fails. Optional edges are skipped.bun.lockrow never passes the resolver, so the hoisted linker (PackageInstaller.rs,Tag::Symlinkarm) and the isolated linker (isolated_install.rs, before the first task) repeat the check. They key it on the package (Lockfile::is_trusted_folder_package, the helper install: refuse a registry package's escaping file: folder in the isolated linker #43101 adds), because--productiondrops the root's devDependency and leaves only a registry package's peer edge on the link.link:<name>for every declarer, and every escaping target a root, workspace,file:folder or override declares.test/cli/install/bun-install.test.ts, blocklink: paths with .. or an absolute path declared by a dependency. 7 of 14 fail on the released build. Also ran thefile:,link, workspace, overrides and isolated suites.Background
link:dependency becomes aResolution::Symlinkrow. Its value is read relative to the project when it starts with., and both linkers join it onto the global link dir ($BUN_INSTALL_GLOBAL_DIR/node_modules), so a target with enough..segments is valid from both places.is_trusted_folder_dependencyis the trust rule of thefile:folder form: the declarer was read from the project, or a root override named the dependency.is_trusted_folder_packageasks it about every lockfile edge that resolves to the package, installed or filtered out.link:(the first version of this PR: declarer tag check plusversion_was_replaced): it trusted everyfile:folder, including one shipped by a registry package, and had no install-time check. Sharing thefile:predicate removes both gaps. install: support path-stylelink:./dirdependencies (yarn/pnpm semantics) #40025 (path-formlink:support) draws the same line and replaces this guard when it lands.Downsides
bun.lockwritten by an older bun with such a row under a registry, git or tarball package now fails to install (exit 1) instead of linking the directory. The row only exists if a third-party manifest put it there.is_trusted_folder_packagescans every lockfile edge once per escapinglink:row at install time. It runs only for those rows. An install with no such row does no extra work.link:or for root-declared paths. Checked:bun-link,bun-add-filter,bun-add-catalog,bun-workspaces,isolated-install.Notes
Review follow-ups in this push
link:.from a remote package links the project root intonode_modules. That target stays inside the project, which is the line install: support path-stylelink:./dirdependencies (yarn/pnpm semantics) #40025 draws too (a target inside the project is allowed for every declarer).link:./xis normalized tox, a name-form lookup in the global link dir, so it never reached the project. Both stay allowed.bun.lockrow was not checked at install time. Both linkers now check it. Test:are not installed by the <linker> linker from a bun.lock that already holds one under a tarball dependency.Resolution::Tag::is_local_package, which trusts everyFolderpackage). It is nowis_trusted_folder_dependency, shared with thefile:arm.link:../sharedunder--productionwhen a registry package peer-depends on it (I ran that shape: the hoisted linker logged the refusal). Keyed on the package it links. Tests:are still linked by the <linker> linker when --production leaves only a registry package's peer dependency on a root-declared one.Relationship to other PRs
link:./dirdependencies (yarn/pnpm semantics) #40025 adds path-formlink:as a feature (re-bases the value onto the project root, changes whatbun.lockstores) with a containment rule at the same three sites. This PR is only the guard, built from helpers already on main. If install: support path-stylelink:./dirdependencies (yarn/pnpm semantics) #40025 merges first this one can be closed; if this merges first, install: support path-stylelink:./dirdependencies (yarn/pnpm semantics) #40025's rewrite of the same arms replaces it.is_trusted_folder_packagefor thefile:folder form in the isolated linker. The helper here has the same text, so the branches merge in either order.file:folder arm still keys its check on the placed dependency (PackageInstaller.rs:1456), so it has the--productiongap described above. install: refuse a registry package's escaping file: folder in the isolated linker #43101 notes it. Not changed here.Probe on the released build (1.4.0-canary, eabb96d)
tb.tgzcontains only a package.json with"loot": "link:../../../../../../../../../../../../../../../../tmp/linkrepro2/secret"; the project depends on"tb": "file:./tb.tgz";/tmp/linkrepro2/secret/package.jsonis{"name":"secret-dir","main":"index.js"}.BUN_INSTALL_GLOBAL_DIRpoints at an empty directory.Suites run with the debug build of this branch
bun-install.test.ts -t "link|file:|folder|tarball|workspace": 121 pass, 1 fail (should treat non-GitHub http(s) URLs as tarballs, needs the public internet).bun-link.test.ts,overrides.test.ts,nested-overrides.test.ts,bun-workspaces.test.ts,isolated-install.test.ts(link and symlink tests),bun-add-catalog.test.tsandbun-add-filter.test.ts(link tests): pass.should link dependency without crashinginbun-link.test.tsfails the same way with main'ssrc/installswapped in (a debug-only stack dump lands in stdout).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.test.ts