Skip to content

src: fix cpSyncCopyDir to dereference symlinks when dereference option is set#62402

Open
crawfordxx wants to merge 1 commit intonodejs:mainfrom
crawfordxx:fix-cpSyncCopyDir-dereference
Open

src: fix cpSyncCopyDir to dereference symlinks when dereference option is set#62402
crawfordxx wants to merge 1 commit intonodejs:mainfrom
crawfordxx:fix-cpSyncCopyDir-dereference

Conversation

@crawfordxx
Copy link

Summary

When fs.cpSync() or fs.promises.cp() is called with {dereference: true},
symlinks in the source directory should be followed and their targets copied
to the destination as real files/directories. This behavior was correctly
implemented in the original JavaScript version of cpSync, but was lost when
the inner directory-copy loop was migrated to C++ in PR #58461.

Root Cause

In CpSyncCopyDir (src/node_file.cc), the dereference parameter was
captured by the lambda but only used for error-condition checks:

if (!dereference && is_directory(symlink_target) && ...) { /* error */ }
if (!dereference || (!force && error_on_exist)) { /* error */ }

At the end of the is_symlink() branch, a new symlink was always created
at the destination regardless of whether dereference was true:

if (dir_entry.is_directory()) {
  create_directory_symlink(symlink_target_absolute, dest_file_path, error);
} else {
  create_symlink(symlink_target_absolute, dest_file_path, error);
}

Fix

When dereference is true and we encounter a symlink:

  • If the symlink target is a directory: create the directory and recurse via copy_dir_contents()
  • If the symlink target is a file: copy the file content via copy_file()

When dereference is false (the default), behavior is unchanged — a new
symlink is created at the destination.

Test

Existing test case in test/parallel/test-fs-cp.mjs covers the dereference
option. The specific scenario from #59168 (symlink to a file outside the source
directory with {dereference: true, recursive: true}) now produces a regular
file at the destination instead of a symlink.

Fixes: #59168

…n is set

When fs.cpSync() is called with {dereference: true}, symlinks in the source
directory should be followed and their targets copied to the destination
instead of creating new symlinks. This behavior was correctly implemented in
the JavaScript version of cpSync, but was lost when the inner copyDir loop
was migrated to C++ in CpSyncCopyDir().

The dereference parameter was captured but only used for error-condition
checks, not to decide whether to create a symlink or copy the actual content.

Fix by checking dereference before creating symlinks: when true, copy the
actual file (using copy_file) or recurse into the actual directory
(using copy_dir_contents) that the symlink points to.

Fixes: nodejs#59168
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Mar 23, 2026
Copy link

@themavik themavik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dereference branch in CpSyncCopyDir now copies symlink targets via copy_dir_contents / copy_file instead of recreating links — correct behavior. nit: confirm filters (should_copy_signal / dirent_filter) still apply the same when the walk hits dereferenced symlinks as for ordinary entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fs.cpSync with dereference: true does not dereference (regression in 22.17)

3 participants