fs: fix repeated copy of directory with symlinks

Repeatedly copying a directory that contains a symlink to an unrelated
directory fails on the second copy with ERR_FS_CP_EINVAL because the
symlink target is mistaken for a self-referential copy.

Compare symlink targets using canonicalized paths, treating identical
targets as self-copies only when the target is the destination root or
one of its ancestors.

Fixes: #65097
Signed-off-by: haramjeong <04harams77@gmail.com>
Assisted-by: Codex

Author: haramj

lib/internal/fs/cp/cp-sync.js

@@ -53,6 +53,7 @@ function cpSyncFn(src, dest, opts) {
     if (!shouldCopy) return;
   }
 
+  opts = { ...opts, destRoot: dest };
   fsBinding.cpSyncCheckPaths(src, dest, opts.dereference, opts.recursive);
 
   return getStats(src, dest, opts);
@@ -71,7 +72,7 @@ function getStats(src, dest, opts) {
            srcStat.isBlockDevice()) {
     return onFile(srcStat, destStat, src, dest, opts);
   } else if (srcStat.isSymbolicLink()) {
-    return onLink(destStat, src, dest, opts.verbatimSymlinks);
+    return onLink(destStat, src, dest, opts.verbatimSymlinks, opts.destRoot);
   }
 
   // It is not possible to get here because all possible cases are handled above.
@@ -198,7 +199,7 @@ function copyDir(src, dest, opts, mkDir, srcMode) {
 }
 
 // TODO(@anonrig): Move this function to C++.
-function onLink(destStat, src, dest, verbatimSymlinks) {
+function onLink(destStat, src, dest, verbatimSymlinks, destRoot) {
   let resolvedSrc = readlinkSync(src);
   if (!verbatimSymlinks && !isAbsolute(resolvedSrc)) {
     resolvedSrc = resolve(dirname(src), resolvedSrc);
@@ -224,7 +225,13 @@ function onLink(destStat, src, dest, verbatimSymlinks) {
     resolvedDest = resolve(dirname(dest), resolvedDest);
   }
 
-  if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest)) {
+  // A symlink that resolves to the same target as the destination symlink
+  // is not a self-copy, unless the target is the destination directory
+  // itself or one of its ancestors.
+  const sameTarget = resolvedSrc === resolvedDest &&
+    !isSrcSubdir(resolvedSrc, resolve(destRoot));
+
+  if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest) && !sameTarget) {
     throw new ERR_FS_CP_EINVAL({
       message: `cannot copy ${resolvedSrc} to a subdirectory of self ` +
           `${resolvedDest}`,
@@ -237,7 +244,8 @@ function onLink(destStat, src, dest, verbatimSymlinks) {
   // Prevent copy if src is a subdir of dest since unlinking
   // dest in this case would result in removing src contents
   // and therefore a broken symlink would be created.
-  if (statSync(dest).isDirectory() && isSrcSubdir(resolvedDest, resolvedSrc)) {
+  if (statSync(dest).isDirectory() && isSrcSubdir(resolvedDest, resolvedSrc) &&
+      !sameTarget) {
     throw new ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY({
       message: `cannot overwrite ${resolvedDest} with ${resolvedSrc}`,
       path: dest,

lib/internal/fs/cp/cp.js

@@ -66,6 +66,7 @@ async function cpFn(src, dest, opts) {
       'node is not recommended';
     process.emitWarning(warning, 'TimestampPrecisionWarning');
   }
+  opts = { ...opts, destRoot: dest };
   const stats = await checkPaths(src, dest, opts);
   const { srcStat, destStat, skipped } = stats;
   if (skipped) return;
@@ -385,7 +386,13 @@ async function onLink(destStat, src, dest, opts) {
     resolvedDest = resolve(dirname(dest), resolvedDest);
   }
 
-  if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest)) {
+  // A symlink that resolves to the same target as the destination symlink
+  // is not a self-copy, unless the target is the destination directory
+  // itself or one of its ancestors.
+  const sameTarget = resolvedSrc === resolvedDest &&
+    !isSrcSubdir(resolvedSrc, resolve(opts.destRoot));
+
+  if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest) && !sameTarget) {
     throw new ERR_FS_CP_EINVAL({
       message: `cannot copy ${resolvedSrc} to a subdirectory of self ` +
             `${resolvedDest}`,
@@ -399,7 +406,8 @@ async function onLink(destStat, src, dest, opts) {
   // dest in this case would result in removing src contents
   // and therefore a broken symlink would be created.
   const srcStat = await stat(src);
-  if (srcStat.isDirectory() && isSrcSubdir(resolvedDest, resolvedSrc)) {
+  if (srcStat.isDirectory() && isSrcSubdir(resolvedDest, resolvedSrc) &&
+      !sameTarget) {
     throw new ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY({
       message: `cannot overwrite ${resolvedDest} with ${resolvedSrc}`,
       path: dest,

src/node_file.cc

@@ -4838,9 +4838,9 @@ CpError CopyDirRecursive(const std::filesystem::path& src_path,
 
   std::function<CpError(std::filesystem::path, std::filesystem::path)>
       copy_dir_contents;
-  copy_dir_contents = [&options, &copy_dir_contents, file_copy_opts](
-                          std::filesystem::path src,
-                          std::filesystem::path dest) -> CpError {
+  copy_dir_contents =
+      [&options, &copy_dir_contents, &dest_path, file_copy_opts](
+          std::filesystem::path src, std::filesystem::path dest) -> CpError {
     std::error_code error;
     // Only the error_code overloads are used from here on: this runs on a
     // thread pool thread and exceptions are disabled.
@@ -4888,6 +4888,28 @@ CpError CopyDirRecursive(const std::filesystem::path& src_path,
             return CpError::Std(error, dest_str);
           }
 
+          std::filesystem::path symlink_target_absolute;
+          if (options.fresh_destination) {
+            // As path.resolve() does: lexical only, absolute targets verbatim.
+            symlink_target_absolute =
+                symlink_target.is_absolute()
+                    ? symlink_target
+                    : std::filesystem::absolute(src / symlink_target, error)
+                          .lexically_normal();
+          } else {
+            symlink_target_absolute = std::filesystem::weakly_canonical(
+                std::filesystem::absolute(src / symlink_target, error), error);
+          }
+          if (error) {
+            return CpError::Std(error, dest_str);
+          }
+#ifdef _WIN32
+          auto wstr = symlink_target_absolute.wstring();
+          if (wstr.starts_with(L"\\\\?\\")) {
+            symlink_target_absolute = std::filesystem::path(wstr.substr(4));
+          }
+#endif
+
           if (std::filesystem::exists(dest_file_path, error)) {
             if (std::filesystem::is_symlink(dest_file_path, error)) {
               auto current_dest_symlink_target =
@@ -4896,9 +4918,33 @@ CpError CopyDirRecursive(const std::filesystem::path& src_path,
                 return CpError::Std(error, dest_str);
               }
 
+              auto current_dest_symlink_target_absolute =
+                  std::filesystem::weakly_canonical(
+                      std::filesystem::absolute(dest_file_path.parent_path() /
+                                                    current_dest_symlink_target,
+                                                error),
+                      error);
+              if (error) {
+                return CpError::Std(error, dest_str);
+              }
+#ifdef _WIN32
+              auto wstr2 = current_dest_symlink_target_absolute.wstring();
+              if (wstr2.starts_with(L"\\\\?\\")) {
+                current_dest_symlink_target_absolute =
+                    std::filesystem::path(wstr2.substr(4));
+              }
+#endif
+              // Equal targets are safe unless they point to the destination
+              // root or one of its ancestors.
+              bool same_target =
+                  symlink_target_absolute ==
+                      current_dest_symlink_target_absolute &&
+                  !isInsideDir(symlink_target_absolute, dest_path);
+
               if (!options.dereference &&
                   std::filesystem::is_directory(symlink_target, error) &&
-                  isInsideDir(symlink_target, current_dest_symlink_target)) {
+                  isInsideDir(symlink_target, current_dest_symlink_target) &&
+                  !same_target) {
                 return {CpError::kEinval,
                         0,
                         "cp",
@@ -4912,7 +4958,8 @@ CpError CopyDirRecursive(const std::filesystem::path& src_path,
               // dest in this case would result in removing src contents
               // and therefore a broken symlink would be created.
               if (std::filesystem::is_directory(dest_file_path, error) &&
-                  isInsideDir(current_dest_symlink_target, symlink_target)) {
+                  isInsideDir(current_dest_symlink_target, symlink_target) &&
+                  !same_target) {
                 return {CpError::kSymlinkToSubdirectory,
                         0,
                         "cp",
@@ -4939,27 +4986,6 @@ CpError CopyDirRecursive(const std::filesystem::path& src_path,
               }
             }
           }
-          std::filesystem::path symlink_target_absolute;
-          if (options.fresh_destination) {
-            // As path.resolve() does: lexical only, absolute targets verbatim.
-            symlink_target_absolute =
-                symlink_target.is_absolute()
-                    ? symlink_target
-                    : std::filesystem::absolute(src / symlink_target, error)
-                          .lexically_normal();
-          } else {
-            symlink_target_absolute = std::filesystem::weakly_canonical(
-                std::filesystem::absolute(src / symlink_target, error), error);
-          }
-          if (error) {
-            return CpError::Std(error, dest_str);
-          }
-#ifdef _WIN32
-          auto wstr = symlink_target_absolute.wstring();
-          if (wstr.starts_with(L"\\\\?\\")) {
-            symlink_target_absolute = std::filesystem::path(wstr.substr(4));
-          }
-#endif
           if (dir_entry.is_directory(error)) {
             std::filesystem::create_directory_symlink(
                 symlink_target_absolute, dest_file_path, error);

test/parallel/test-fs-cp-async-symlink-repeated-copy.mjs

@@ -0,0 +1,62 @@
+// This tests that repeatedly copying a directory containing a symlink
+// to an unrelated directory succeeds.
+// See https://github.com/nodejs/node/issues/65097.
+import { mustCall, mustNotMutateObjectDeep } from '../common/index.mjs';
+import { nextdir } from '../common/fs.js';
+import assert from 'node:assert';
+import { cp, mkdirSync, realpathSync, symlinkSync } from 'node:fs';
+import { join } from 'node:path';
+
+import tmpdir from '../common/tmpdir.js';
+tmpdir.refresh();
+
+const root = nextdir();
+const src = join(root, 'src');
+const dest = join(root, 'dest');
+const target = join(root, 'target');
+mkdirSync(src, mustNotMutateObjectDeep({ recursive: true }));
+mkdirSync(target);
+symlinkSync(target, join(src, 'link'));
+cp(src, dest, mustNotMutateObjectDeep({ recursive: true }), mustCall((err) => {
+  assert.ifError(err);
+  cp(src, dest, mustNotMutateObjectDeep({ recursive: true }), mustCall((err) => {
+    assert.ifError(err);
+    assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+
+    // Also exercise the JavaScript (filter) path.
+    cp(src, dest, mustNotMutateObjectDeep({ recursive: true, filter: () => true }), mustCall((err) => {
+      assert.ifError(err);
+      cp(src, dest, mustNotMutateObjectDeep({ recursive: true, filter: () => true }), mustCall((err) => {
+        assert.ifError(err);
+        assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+      }));
+    }));
+  }));
+}));
+
+// A symlink with a relative target pointing to an unrelated directory.
+{
+  const root = nextdir();
+  const src = join(root, 'src');
+  const dest = join(root, 'dest');
+  const target = join(root, 'target');
+  mkdirSync(src, mustNotMutateObjectDeep({ recursive: true }));
+  mkdirSync(target);
+  symlinkSync('../target', join(src, 'link'));
+  cp(src, dest, mustNotMutateObjectDeep({ recursive: true }), mustCall((err) => {
+    assert.ifError(err);
+    cp(src, dest, mustNotMutateObjectDeep({ recursive: true }), mustCall((err) => {
+      assert.ifError(err);
+      assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+
+      // Same as above, exercising the JavaScript (filter) path.
+      cp(src, dest, mustNotMutateObjectDeep({ recursive: true, filter: () => true }), mustCall((err) => {
+        assert.ifError(err);
+        cp(src, dest, mustNotMutateObjectDeep({ recursive: true, filter: () => true }), mustCall((err) => {
+          assert.ifError(err);
+          assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+        }));
+      }));
+    }));
+  }));
+}

test/parallel/test-fs-cp-sync-symlink-repeated-copy.mjs

@@ -0,0 +1,48 @@
+// This tests that repeatedly copying a directory containing a symlink
+// to an unrelated directory succeeds.
+// See https://github.com/nodejs/node/issues/65097.
+import { mustNotMutateObjectDeep } from '../common/index.mjs';
+import { nextdir } from '../common/fs.js';
+import assert from 'node:assert';
+import { cpSync, mkdirSync, realpathSync, symlinkSync } from 'node:fs';
+import { join } from 'node:path';
+
+import tmpdir from '../common/tmpdir.js';
+tmpdir.refresh();
+
+const root = nextdir();
+const src = join(root, 'src');
+const dest = join(root, 'dest');
+const target = join(root, 'target');
+mkdirSync(src, mustNotMutateObjectDeep({ recursive: true }));
+mkdirSync(target);
+symlinkSync(target, join(src, 'link'));
+cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true }));
+cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true }));
+assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+
+// Also exercise the JavaScript (filter) path. The destination symlink
+// already exists at this point, so this covers the repeated-copy case on
+// the filtered code path as well.
+cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true, filter: () => true }));
+cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true, filter: () => true }));
+assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+
+// A symlink with a relative target pointing to an unrelated directory.
+{
+  const root = nextdir();
+  const src = join(root, 'src');
+  const dest = join(root, 'dest');
+  const target = join(root, 'target');
+  mkdirSync(src, mustNotMutateObjectDeep({ recursive: true }));
+  mkdirSync(target);
+  symlinkSync('../target', join(src, 'link'));
+  cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true }));
+  cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true }));
+  assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+
+  // Same as above, exercising the JavaScript (filter) path.
+  cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true, filter: () => true }));
+  cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true, filter: () => true }));
+  assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+}