pack: look up a dependency of a bundled dependency only by package name

Closed
RO
1mo ago
+92-1

Problem

  • bun pm pack and bun publish copy the dependencies of each bundled dependency. add_bundled_dep (src/runtime/cli/pack_command.rs:1085) makes a path from each dependencies key and opens it below the package root, with no check of the key.
  • The key ../../outside adds package/node_modules/../../outside/secret.txt, a file outside the package, and the pack exits 0. "" and . add all of node_modules.

Fix

  • is_nested_dependency_name accepts a key only when it is name or @scope/name and the name does not start with .. It calls the check of the installers, alias_is_safe_install_target, which is now pub.
  • The pack skips a key that fails the check, as it skips a dependency that is not installed.
  • Verified: test/cli/install/bun-pack.test.ts, 14 new tests. 9 fail on main on Linux. 3 more have \ keys, for Windows.
  • Self-reviewed: 5 concerns raised, 3 addressed. Notes give the other 2.

Background

  • A bundled dependency is a package in node_modules that the pack copies into the tarball. Its package.json comes from the registry, so its keys are not trusted.
  • npm 11.16.0 ignores each of these keys. It finds nested dependencies in directory listings.
  • Considered the same listing design here. It rewrites three walkers. The check closes the same keys in 18 lines.

Downsides

  • A pack with bundled dependencies runs the check for each dependencies key: +943 instructions for three keys (39,917 to 40,860 in iterate_bundled_deps). It adds no syscall and no allocation. Release binary text: +512 bytes.
  • A key that is not name or @scope/name is not bundled. npm does not install a package with such a name.
Notes

Each key

One bundled dependency, dep1, has the key in its dependencies. A correct tarball has 2 entries: package.json and node_modules/dep1/package.json. "main" is a release build of the merge base, on Linux.

Keymainthis PR
../../outsideexit 0, 3 entries, 1 from outside the package2 entries
other/libexit 0, adds node_modules/other/lib2 entries
"", .exit 0, 8 entries: all of node_modules2 entries
..exit 0, adds the package root2 entries
@scopeexit 0, adds every package of the scope2 entries
.bin, @scope/.hiddenexit 0, adds the directory2 entries
other\0x (a NUL byte)exit 1, EISDIR: failed to read file2 entries
..\..\outside, x\..\..\..\outside, other\lib2 entries on Linux, where \ is a name byte2 entries
C:/Users/Public2 entries2 entries
JSONStream, @Scope/Namebundledbundled

The \ rows are for Windows, where \ is a separator. They were not run on a Windows build of main.

The check

alias_is_safe_install_target refuses an empty name, a . or .. part, \, :, a NUL byte, and any shape but one part or @scope/name. Two more clauses refuse a bare @scope and a name part that starts with ..

Other open PRs

  • #44439 adds is_valid_node_modules_entry_name and makes alias_is_safe_install_target call it. After it lands, the two extra clauses here are not necessary. #44439 and #43067 change lines next to that signature.
  • #41925 opens each path part without following links. From a read of its code, it also refuses ../../outside and .. at the open. The other keys need this check with or without it.
  • This is the first commit of #44873, as its own PR.

Self-review

  • Done: this check is its own PR, the \, NUL and : rows are in the test, and this text says what #41925 already refuses.
  • Not done: a call to the shared predicate of #44439. That PR is not merged. This PR calls the check that exists on main, and #44439 makes that check call its predicate.
  • Not done: a list of the other commands that read a name from a manifest. They are outside this change.

Cost, measured

  • Instructions: gdb single-step from the first instruction of iterate_bundled_deps to its return, on release builds of the merge base and of this PR. The tree has one bundled dependency with the keys dep2, dep3 and @scoped/dep4. Two runs each: 39,916 and 39,919 before, 40,863 and 40,857 after. A package with no bundled dependency: 35 before and after.
  • Size: size on the two release binaries (linux-x64), text 89,103,407 to 89,103,919. add_bundled_dep grows from 4,865 to 5,281 bytes.

Tests

  • bun-pack.test.ts on the debug build: 99 of 100 pass. lifecycle scripts execution order (not changed) hits the 5 s limit on this machine. It does the same with main's copy of the file, and passes alone.
  • On the release build of the merge base, 9 of the 14 new tests fail: the eight / keys and the NUL key.
  • cargo fmt --check, prettier, and bun check in test/ for the changed file.

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-pack.test.ts