shell(cp): classify the target operand through symlinks - #37956
Conversation
The cp builtin lstat()ed the target, so a symlink to a directory was taken for a file: `cp file linkdir` tried to write to the link itself and failed with EISDIR, and `cp a b linkdir` reported that linkdir is not a directory. cp(1) stat()s the target operand; do the same, so the copy lands inside the directory the link points at. The source operand is still classified without following links.
|
Warning Review limit reached
Next review available in: 4 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 (2)
Comment |
|
Updated 4:01 AM PT - Aug 13th, 2026
✅ @robobun, your commit d42d963e82bdfc8399b08f202039979ea3a32845 passed in 🧪 To try this PR locally: bunx bun-pr 37956That installs a local version of the PR into your bun-37956 --bun |
|
Status: fix and tests pushed (d42d963), self-review done, waiting on CI. Reproduced on bun 1.4.0 (Linux, builtin enabled with
|
There was a problem hiding this comment.
LGTM — small, well-reasoned fix swapping lstat for stat when classifying the cp target operand, matching cp(1)/coreutils/BSD.
What was reviewed:
target_is_dirusesbun_sys::stat(cross-platform via libuv on Windows);S::ISDIR(st.st_mode as _)on abun_sys::statresult is an established pattern used elsewhere (e.g.run_command.rs,node_fs.rs).- ENOENT from
staton a dangling link lands in the existing ENOENT arm →tgt_exists = false, so the 3rd-synopsis "is not a directory" path fires as intended; non-ENOENT errors propagate unchanged. - Source classification correctly stays on
lstatsince the copy recreates links. - Tests cover the variant matrix (single/multi/-R file/-R dir/junction/link-to-file/dangling), spawn child bun with the builtin env var, drain pipes concurrently, and use
tempDirwithusing.
Extended reasoning...
Overview
This PR fixes the shell cp builtin so that a symlink-to-directory used as the target operand is treated as a directory (as cp(1) does), rather than as the link itself. The native change is minimal: a new 3-line target_is_dir helper that calls bun_sys::stat (follows links) instead of lstat/GetFileAttributesW, plus one call-site swap in run_from_thread_pool_impl. The source-operand classification is unchanged. ~120 lines of new tests exercise the fix across POSIX and Windows.
Security risks
None identified. Following symlinks when classifying the target of a user-invoked cp is the standard POSIX/coreutils/BSD behaviour the user is asking for; there's no privilege boundary here. The change doesn't affect source handling or introduce any new path-traversal surface.
Level of scrutiny
Low-to-medium. The cp builtin is experimental on POSIX (env-gated) and default only on Windows. The native change is a one-line call swap plus a tiny helper that reuses an existing cross-platform bun_sys::stat abstraction — the same S::ISDIR(st.st_mode as _) pattern appears throughout the codebase on stat results. The ENOENT arm already existed and now correctly absorbs dangling links. The PR description is thorough, cites POSIX and both reference implementations, and explains the Windows behaviour delta.
Other factors
The tests are well-constructed per repo conventions: describe.concurrent for independent subprocess tests, tempDir with using, {...bunEnv, ...} spread, concurrent pipe draining via Promise.all, exact-value assertions on {exitCode, stdout, stderr}, and a Windows-only junction case. The PR notes both Linux and Windows debug-build test runs pass and that the new tests fail against the current release/canary. The bug-hunting system found nothing. This is a focused bug fix with clear intent and no design decisions requiring human judgment.
-R files into a dangling link used to exit 0 without copying; a single file onto one addresses the link itself; a link that cannot be followed (a loop, or a Windows file-type link to a directory) reports the stat error.
There was a problem hiding this comment.
LGTM — swaps the target classifier from lstat to bun_sys::stat so cp file linkdir copies into the linked directory, matching cp(1).
What was reviewed:
target_is_diruses the samebun_sys::stat+S::ISDIR(st.st_mode)pattern as the shell's[[ -d ]](CondExpr.rs:207), so it's cross-platform; the existing ENOENT arm now correctly treats dangling links as "target does not exist".- Source classifier (
is_dir) intentionally left onlstat— the builtin copies link operands as links; #37954 handles that side. - Multi-source error assertions use
.repeat(2)on identical per-task lines, so concurrent task ordering can't flake them. - Tests spawn a child bun with the builtin env var (read once per process), use
tempDir+using, drain pipes concurrently, and pin the negative contract (link unchanged,missingnot created).
Extended reasoning...
Overview
The shell cp builtin classified its target operand with lstat/GetFileAttributesW, which describes the symlink itself rather than what it points at. That made cp file linkdir (where linkdir → realdir) fail with EISDIR on POSIX and, on Windows, made a dangling directory-type link look like an existing directory. The fix adds a 4-line target_is_dir that calls bun_sys::stat (follows links) and swaps the one call site in run_from_thread_pool_impl. ~190 lines of new tests cover all three POSIX cp synopses against directory links, junctions, file links, dangling links, and a symlink loop.
Security risks
None. This is a stat-vs-lstat classification change in a shell builtin that only runs on Windows by default (opt-in via env var on POSIX). No user input parsing, no path validation is weakened — if anything, the Windows dangling-link case now fails closed instead of silently creating the missing directory.
Level of scrutiny
Low-to-medium. The Rust change is 7 lines using an established in-tree helper (bun_sys::stat + S::ISDIR) with the exact same pattern as [[ -d ]] in CondExpr.rs and several other cross-platform call sites. The ENOENT match arm is unchanged and correctly absorbs the new dangling-link case. The source classifier is deliberately left alone (documented in the PR; #37954 handles it). I traced each of the three synopses through run_from_thread_pool_impl with the new (tgt_is_dir, tgt_exists) values for link-to-dir, dangling, and loop targets and they land where the PR says.
Other factors
- Tests follow harness conventions:
tempDirwithusing,bunEnvspread,Promise.allon stdout/stderr/exited, exact-objecttoEqualassertions,describe.concurrentfor independent subprocess tests,skipIf(!isWindows)for junction/file-type-link cases. - Multi-source tests assert
stderr: line.repeat(2)where both concurrentShellCpTasks emit the identical target-keyed line, so ordering can't flake. - The comment-cop bot flagged verbose doc comments; both were trimmed in d42d963 and the threads are resolved.
- PR description confirms the new tests fail on 1.4.0 (Linux) and canary (Windows dangling-link), and pass on a debug build of this branch on both platforms.
Problem
cpbuiltin (the defaultcpon Windows,BUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1on POSIX) cannot copy into a directory that is named through a symlink. Withlinkdir -> realdir, on Linux (bun 1.4.0 and main):cp file.txt linkdir/already worked: the trailing slash makes the kernel follow the link.)cp -R a.txt b.txt danglingexits 0 without copying anything on every platform (the native copy swallows the EEXIST it gets creatingdangling/), and on Windows, where a directory-type link carries the directory attribute itself,cp a.txt b.txt danglingexits 0 and creates the missing directory with the copies in it. cp(1) fails both: the target is not a directory.ShellCpTask::is_dir(src/runtime/shell/builtin/cp.rs) classifies the target operand the same way as the source, withlstat(GetFileAttributesWon Windows), so it describes the link rather than what it points at.run_from_thread_pool_implthen picks the wrong synopsis: file -> file for a link to a directory (the native copy opens the directory for writing, EISDIR), and "existing target" for a dangling link. cp(1) classifies the target operand withstat.Fix
target_is_dirclassifies the target withbun_sys::stat(follows links) on every platform and replaces theis_dir(tgt)call. The source keepsis_dir: the builtin copies a link operand as a link, so it has to look at the link itself there (shell(cp): copy the file a symlink operand points at unless -R is given #37954 changes that separately for the non--Rcase; the two compose, with a few-line conflict next tois_dir).cpsynopsis by whether the target names an existing directory, and coreutils and BSDcpboth decide that withstat(), so a link to a directory is a directory target and a dangling link is a target that does not exist. The dangling case lands in the existing ENOENT arm (tgt_exists = false), which yields cp's answers: "is not a directory" for several sources, "directory X does not exist" for-Rwith several sources, and the file -> file synopsis for one source. Themvbuiltin already classifies its target by following links (ShellMvCheckTargetTask,openatwithO_DIRECTORY), somv file linkdirandcp file linkdirnow agree.-R files... dangling: exit 1 "directory dangling does not exist" everywhere (was exit 0 without copying; on Windows it created the directory).files... dangling: unchanged on POSIX; on Windows exit 1 "is not a directory" instead of exit 0 plus creating the directory.file dangling: the file -> file synopsis on both builds, so unchanged on POSIX, where the open() writes through the link as BSD cp does (coreutils refuses); on WindowsCopyFileWnow fails with "Operation not permitted" where the old classification created the directory.statcannot follow (a loop; on Windows also a file-type link that points at a directory) now fails with the stat error for every form, as coreutils does ("target 'loop': Too many levels of symbolic links"); before, the several-sources form said "is not a directory" and the one-source form hit the same error later in the copy.-R dir dangling(one directory source) is now the "create the target directory" synopsis, as in cp. On POSIX it still exits 1 (mkdir fails with "File exists" on the link; before, ENOENT under it). On Windows the native directory copy accepts the existing link as the new directory and creates the link's target when it writes the entries, so this one input now exits 0 with the tree at the link's target where it used to fail with ENOENT; coreutils refuses it. That is the native copy's handling of a dangling link as a destination (the single-file form of which also produced the old exit 0 above), reported separately rather than special-cased in the shell, and it is not pinned by a test here.bun_sys::staton Windows is libuv'suv_fs_stat, the same callfs.statSyncand the shell's[[ -d ]]use; directory symlinks and junctions still classify as directories, and an exclusively locked target file still classifies (attribute-only open), so shell(cp): exit 1 when a held-back Windows error turns out to be real #37943's EBUSY handling is unaffected.test/js/bun/shell/commands/cp.test.ts, new block "bunshell cp with a symlink as the target" (each cp runs in a child bun with the builtin enabled): one file, several files and-R fileinto a link to a directory;-R dirinto one; a junction (Windows); and, as targets that are not directories, a link to a file, a dangling link in all three forms, a link to itself, and a file-type link to a directory (Windows). Linux: 5 of the 9 fail on bun 1.4.0 (the three linkdir copies,-Rinto the dangling link, the loop) and all pass withbun bd test. Windows x64, debug build of this branch: all 41 tests in the file pass, including the TestBuilder block that is the builtin's default-platform coverage; against the current canary the three dangling-link tests, the loop and the file-type link fail with the old outputs listed above.catbuiltin, shell(cat): read regular files synchronously in the builtin on POSIX #35337);cargo fmtand clippy are clean.Background
cpbuiltin:Cp::nextschedules oneShellCpTaskper source operand. Each task resolves its source and the shared target to absolute paths, picks one of the three POSIXcpsynopses (file -> file;-Rsources -> target; files -> existing directory) and hands the final pair to the node:fs async copy (ShellAsyncCpTask). Whether the target "is a directory" and whether it "exists" are the two answers that pick the synopsis, so they decide between copying onto the target path, copying totarget/<basename>inside it, and refusing.statvslstat:statfollows a symlink and describes the file it points at, failing with ENOENT when it points nowhere (or ELOOP for a loop);lstatdescribes the link itself, so it succeeds for any link.GetFileAttributesWbehaves likelstat, and a Windows directory-type link carriesFILE_ATTRIBUTE_DIRECTORYwhether or not its target exists, which is where the Windows "dangling link is a directory" behaviour came from. Windows links have a type: a file-type link whose target is a directory cannot be followed at all.cpis on the shell's POSIX-disabled list, so on Linux and macOS it only runs as a builtin whenBUN_ENABLE_EXPERIMENTAL_SHELL_BUILTINS=1is set; the flag is read once per process, which is why the tests spawn a child bun per case and why the file's TestBuilder block is skipped on POSIX.Repro on Linux (bun 1.4.0) and Windows canary output
Windows canary (1.4.0-canary),
danglingcreated withfs.symlinkSync("missing", "dangling", "dir"), each in a fresh directory:no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/shell/commands/cp.test.ts