pack: skip bins reached through symlinks; publish: do not read the readme through a symlink

[robobun]

Problem

• bun pm pack / bun publish never pack symlinks (the tree walk only takes regular files and directories, same as npm-packlist), but bin / directories.bin entries bypass the walk: pack() adds each bin path to the pack queue directly (src/runtime/cli/pack_command.rs, the for bin in &bins loop) and later openat()s it, which follows symlinks.
• So a bin that is a symlink, a bin under a symlinked directory ("bin": "sub/cli.js" with sub -> ../elsewhere), or a symlinked directories.bin is packed with the contents of the link target, which can be anything outside the package. npm 11 (npm-packlist 10) omits all three; its onstat drops every lstat result that is not a file or directory, bins included.
• A bin entry that names a directory (or is "", which normalizes to .) failed the whole pack with EISDIR: failed to read file: "lib" and left a truncated .tgz behind. npm ignores it.
• bun publish has the same hole for the readme: find_workspace_readme (src/runtime/cli/publish_command.rs) only skipped directory entries, so a README.md symlink was read through and its target sent to the registry as the readme / readmeFilename metadata, while the tarball itself (correctly) omitted the README.

Fix

• bin_kind_without_following_symlinks() lstats every component of a bin path; a bin is only force-added when it is a regular file (or, for directories.bin, a real directory) reached through real directories. Anything else is skipped and the normal tree walk decides what gets packed, which gives the npm behavior for the symlink cases and for a bin that names a directory.
• The components are stat'ed by absolute path (join(abs_workspace_path, prefix) + bun_sys::lstat) rather than lstatat(root_dir, ..): on Windows lstatat reports a reparse point as its target's kind (the same thing install/prune.rs works around), while lstat reports symlinks and junctions as links, so the check works on every platform. This part is folded in from the independent branch linked in the comments below.
• The trailing slash that directories.bin values commonly carry is stripped before the lstat, since lstat("bins/") would follow a symlinked directory.
• find_workspace_readme now requires a regular file entry, the same test the tarball applies, so the published readme is always the README that is actually in the tarball. Publishing from an existing tarball already did this. (pack, publish: include entries on filesystems whose readdir reports DT_UNKNOWN #38716 additionally resolves Unknown entry kinds in all of these loops; whichever lands second adapts this one line.)
• Verified:
  • test/cli/install/bun-pack.test.ts (bins block): symlinked bin (relative and absolute target, and a link inside the package), bin behind a symlinked directory, the same through --dry-run, a junction/symlink as directories.bin (spelled ./bins/), and bins naming a directory or "". The four new tests fail on the current release and pass with this change; the whole file passes (80 tests).
  • test/cli/install/bun-publish.test.ts (readme block): symlinked README is not sent as readme. Fails before, passes after; whole file passes (40 tests).
  • Windows x64 debug build: the four pack tests fail with the test changes alone and pass with the fix; the readme tests pass.
  • The same fixtures run through npm pack --dry-run (npm 11.16.0) produce the file lists the tests expect.
  • cargo check -p bun_runtime for x86_64-pc-windows-msvc and aarch64-apple-darwin.

Background

• bin (string or object of name -> path) and directories.bin (a directory whose files all become bins) in package.json name executables that must end up in the tarball even when files or .npmignore would exclude them. That is why pack handles them separately from the tree walk: bin files are pushed onto the pack queue before the walk, and the walk skips them to avoid duplicates; a directories.bin directory is walked on its own. The files: ["index.js"] in the test fixture is there so that only this bin handling can add the bins.
• lstat returns the kind of the path itself rather than of what a symlink points at; checking each prefix of the path is what makes a symlink in the middle of the path (sub -> ../elsewhere) visible, since an lstat of the full path would only look at the last component. Bin paths are normalized to / separators by get_package_bins on every platform, so splitting on / is enough.
• readme / readmeFilename are fields npm publish puts in the version metadata it sends to the registry; registries render them as the package page. Bun fills them from the first README it finds in the package directory when publishing from a directory, or from the README inside the archive when publishing an existing tarball.

Repro on the current release (1.4.0-canary)

mkdir -p outside pkg && echo 'SECRET' > outside/secret.js && mkdir outside/bins && echo x > outside/bins/a.js
cd pkg
printf '{"name":"symbin","version":"1.0.0","bin":{"cli":"cli.js","cli2":"sub/cli2.js"},"files":["index.js"]}' > package.json
echo 'module.exports=1' > index.js
ln -s ../outside/secret.js cli.js
ln -s ../outside sub
bun pm pack && tar -tzf symbin-1.0.0.tgz
# package/package.json
# package/cli.js          <- contents of ../outside/secret.js
# package/index.js
# package/sub/cli2.js     <- if ../outside/cli2.js exists
npm pack --dry-run       # index.js, package.json only

directories.bin pointing at a symlink to ../outside/bins packs bins/a.js the same way. For the publish side, a README.md -> ../outside/README.md link produces a PUT whose versions[v].readme is the target's contents although the tarball has no README entry.

Earlier revision of this PR: the bin check used lstatat relative to the package root, which is correct on POSIX but not on Windows until lstatat there reports reparse points (#38365), so the symlink tests were skipped on Windows. Superseded by the absolute-path lstat above.

---

no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-publish.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 34 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8d2f6f33-0331-4a22-9865-6f849073c1d8

📥 Commits

Reviewing files that changed from the base of the PR and between ada2a67 and 5a0921e.

📒 Files selected for processing (4)

• src/runtime/cli/pack_command.rs
• src/runtime/cli/publish_command.rs
• test/cli/install/bun-pack.test.ts
• test/cli/install/bun-publish.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: ready for review; CI is green on every lane that exercised this change.

Reproduced on 1.4.0-canary with a package whose bin entry is a symlink to a file outside the package (tar -tzf of the result shows the target's contents under package/cli.js), the same through a symlinked parent directory and a symlinked directories.bin, and a README.md symlink whose target shows up as readme in the publish PUT body captured by a loopback registry. npm pack --dry-run (npm 11.16.0) omits all of them.

Tests: the new cases in test/cli/install/bun-pack.test.ts (bins) and test/cli/install/bun-publish.test.ts (readme) fail on the current release and pass with this branch, on Linux and on a Windows x64 debug build; both files pass in full locally and on the Linux, ASan, Windows x64 and Windows aarch64 lanes of the latest build (176 of 179 shards green). The one red shard is test/js/node/test/parallel/test-cluster-shared-leak.js timing out on Windows 2019 (it passed on retry on the other Windows lane), unrelated to this change and reported separately; the two macOS aarch64 shards expired in the queue without running.

[claude]

I reviewed this PR and didn't find bugs. Since it adds new symlink-checking path logic in pack (security-adjacent — deciding what leaves the package) and the Windows path is acknowledged as not yet effective, a human look would still be worthwhile.

Checked: the component-by-component lstatat loop terminates on empty/leading-slash/double-slash inputs; bin paths are already Posix-normalized by get_package_bins before reaching the new helper; is_package_bin only sets the exec bit and does not cause the tree walk to drop a skipped bin; the find_workspace_readme DT_UNKNOWN concern was examined and ruled out as consistent with tarball behavior.

Extended reasoning...

Overview

Two changes to bun pm pack / bun publish:

• pack_command.rs: new bin_kind_without_following_symlinks() lstats each /-separated component of a bin path relative to the package root, and the for bin in &bins loop now skips any bin that isn't a real file (or real directory for directories.bin) reached through real directories. This closes a hole where a bin that is a symlink, or sits behind a symlinked directory, would be packed with the contents of the link target (possibly outside the package). It also fixes bin naming a directory, which previously failed pack with EISDIR.
• publish_command.rs: find_workspace_readme now requires entry.kind == File instead of != Directory, so a symlinked README isn't read through and sent as registry metadata when the tarball itself omits it.
• Four new tests in bun-pack.test.ts and one in bun-publish.test.ts, symlink cases skipped on Windows.

Security risks

This is a hardening change in security-adjacent territory (deciding which on-disk files end up in a published tarball / registry payload). The change is strictly more restrictive than before, so the risk profile is "might skip a legitimate bin" rather than "might leak more". I traced the loop for edge inputs (empty after trailing-slash strip → lstat("") fails → Unknown; leading slash → lstat("") fails → Unknown; double slash → extra harmless lstat; . → returns Directory). Bin paths reach this helper already normalized to Posix by resolve_path::normalize_buf::<Posix> in get_package_bins, and absolute/.. paths are already filtered by bin_path_escapes_root.

Level of scrutiny

Moderate-to-high. The diff is small (~35 native lines) but it's new path-component iteration + symlink logic, which is exactly the kind of code that's easy to get subtly wrong and where npm-compat details matter. The PR author verified against npm pack --dry-run on npm 11 and cross-checked cargo check on Windows/macOS targets. The Windows caveat (lstatat reports reparse points as target kind until #38365) is called out and the tests are appropriately skipped there.

Other factors

• is_package_bin (the other consumer of bins) only sets the +x permission bit on files that are already being written; it doesn't gate what the tree walk packs, so skipping a bin from the force-add queue correctly falls through to whatever the tree walk decides.
• The DT_UNKNOWN concern for find_workspace_readme (some filesystems return d_type = DT_UNKNOWN, which would now be skipped) was raised by a finder and refuted by verifiers — the tarball packing applies the same file-only rule, so the metadata stays consistent with the tarball.
• No prior reviews from me or humans on this PR.

[robobun]

Same fix, independently arrived at on branch farm/278dd26b/pack-bin-symlinks (775f10a), standing down in favor of this PR. Two differences there that may be worth folding in:

• It stats each prefix by absolute path (join(abs_workspace_path, prefix) + bun_sys::lstat, i.e. libuv's lstat on Windows) instead of lstatat(root_dir, ..), so the check is already effective on Windows without waiting for Remove get_fd_path: derive paths from cwd and what was opened, not from fds #38365, and the new tests run on Windows too (they create a file symlink, a "dir" symlink and a junction; verified on a Windows x64 box: the release canary packs all of them, the branch build skips all of them, and the 4 new tests pass there with bun bd test). Checking every prefix that ends at a separator also covers the trailing-slash case without stripping it, and splitting on Platform::AUTO.is_separator closes the "bin": "linkdir\\tool" spelling on Windows.
• It adds a --dry-run variant (the dry-run printer consumes the same queue) and a "bin": "" case (normalizes to ., which also failed with EISDIR before).

Unrelated to symlinks but in the same loop, for whoever touches this next: two bin names mapping to the same file pack that file twice, and a directories.bin value with a trailing slash packs its files twice (tools//x.js and tools/x.js). Also, if #36266 (force-including main/browser) lands, those entries bypass the tree walk the same way and need this check too.

[robobun]

Heads up on the find_workspace_readme change here: #38716 adds pack::entry_kind() (resolves FileKind::Unknown from filesystems that do not fill in d_type via lstat) and uses it in this loop. With entry.kind != File as written in this PR, a README on such a filesystem would never be sent; whichever of the two lands second should compare pack::entry_kind(workspace_dir, &entry) instead of entry.kind here.

[robobun]

^{Updated 9:05 PM PT - Aug 14th, 2026}

@robobun, your commit 5a0921e is building: #97064

[claude]

I re-checked the two points I raised earlier and both hold up: normalize_buf::<Posix> does route through normalize_string_loose_buf_t → is_sep_any_t (resolve_path.rs:1358→1964), so backslash-spelled bins are normalized to / before the per-component lstat, and 5a0921e now covers that; and keeping != File in find_workspace_readme is consistent with the tarball walk pending #38716. No new issues found this run.

Since this is symlink/path-containment logic in the packer driven by user-controlled bin / directories.bin values and exercised on both POSIX and Windows reparse points, a human sign-off would still be worthwhile.

What was reviewed

• bin_kind_without_following_symlinks: trailing-slash stripping (handles both separators), the end-advance loop terminates for empty/. inputs, and join_z_spill::<Auto> + lstat per prefix on Windows.
• get_package_bins normalization guarantees the /-only split contract.
• The != File readme guard's interaction with EntryKind::Unknown (deliberate, matches the tarball filter; #38716 resolves both).

Extended reasoning...

Overview

Two files changed in src/runtime/cli/: pack_command.rs gains bin_kind_without_following_symlinks() (per-component lstat of a bin path, joined absolute so Windows reparse points are seen) and a guard in the for bin in &bins loop that skips any bin whose final kind isn't File (or Directory for directories.bin). publish_command.rs tightens find_workspace_readme from == Directory to != File so a symlinked README isn't read through and sent as registry metadata. Four new pack tests and one new publish test cover symlinked bins (relative/absolute/inside-package targets), a bin behind a symlinked directory (including the backslash spelling), a symlinked/junction directories.bin, bins that name a directory or are empty, the --dry-run path, and a symlinked README.

Security risks

The change is a security tightening: it closes a hole where bun pm pack / bun publish would follow a symlinked bin (or a bin under a symlinked directory, or a symlinked directories.bin) and pack the target's bytes into the tarball — potentially content outside the package. The fix matches npm-packlist's onstat filtering. Because it is path-containment logic driven by attacker-controllable package.json fields and must behave identically on Windows (junctions, directory symlinks, lstatat vs lstat), a mistake here could either leave the hole open on one platform or over-exclude legitimate bins. I verified the two specific concerns I'd raised earlier: the \-separator case is handled because normalize_buf::<Posix> dispatches to normalize_string_loose_buf_t which splits on is_sep_any_t and emits / (resolve_path.rs:1358–1359, 1963–1964), and the EntryKind::Unknown change is deliberate and internally consistent with the tarball walk's existing != File && != Directory filter.

Level of scrutiny

High. This is exactly the class of code REVIEW.md calls out under Security ("lexical containment is defeated by symlinks", per-component checks, Windows reparse-point semantics). The implementation is small and looks correct — without_trailing_slash strips both separators, the loop handles the "." / empty-after-normalize case (returns Directory, which mismatches File and skips), and lstat on the absolute join is the documented Windows-safe path — but symlink-containment fixes in a packer warrant a human reviewer confirming the Windows behavior and the npm-parity claim.

Other factors

Test coverage is thorough and follows the harness conventions (per-variant, includes the negative assertion that outside content never appears in any entry, exercises --dry-run, uses a junction on Windows). The author verified the tests fail on the current release and pass with the change on both Linux and a Windows x64 debug build, and cross-checked against npm 11's --dry-run output. The two comment-cop nags were addressed (comments trimmed to one line). The one intentional behavior nuance — Unknown d_type on NFS/FUSE now skips the README metadata too — is acknowledged in the PR description and the timeline as coordinated with #38716.