fs: fix repeated copy of directory with symlinks

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

@@ -25,6 +25,7 @@ const {
   mkdirSync,
   opendirSync,
   readlinkSync,
+  realpathSync,
   statSync,
   symlinkSync,
   unlinkSync,
@@ -53,6 +54,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 +73,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 +200,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 +226,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 = srcIsDir && resolvedSrc === resolvedDest &&
+    !isSrcSubdir(realpathSync(src), realpathSync(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 +245,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

@@ -43,6 +43,7 @@ const {
   mkdir,
   opendir,
   readlink,
+  realpath,
   stat,
   symlink,
   unlink,
@@ -66,6 +67,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 +387,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 = srcIsDir && resolvedSrc === resolvedDest &&
+    !isSrcSubdir(await realpath(src), await realpath(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 +407,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

@@ -4643,18 +4643,23 @@ static void CpSyncCheckPaths(const FunctionCallbackInfo<Value>& args) {
   }
 }
 
+std::filesystem::path StripWindowsNamespace(std::filesystem::path path) {
+#ifdef _WIN32
+  auto wstr = path.wstring();
+  if (wstr.starts_with(L"\\\\?\\")) {
+    return std::filesystem::path(wstr.substr(4));
+  }
+#endif
+  return path;
+}
+
 std::vector<std::string> normalizePathToArray(
     const std::filesystem::path& path) {
   std::vector<std::string> parts;
   std::error_code error;
   std::filesystem::path absPath = std::filesystem::absolute(path, error);
   if (error) absPath = path;
-#ifdef _WIN32
-  auto wstr = absPath.wstring();
-  if (wstr.starts_with(L"\\\\?\\")) {
-    absPath = std::filesystem::path(wstr.substr(4));
-  }
-#endif
+  absPath = StripWindowsNamespace(absPath);
   for (const auto& part : absPath) {
     if (!part.empty()) parts.push_back(part.string());
   }
@@ -4838,9 +4843,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 +4893,24 @@ 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);
+          }
+          symlink_target_absolute =
+              StripWindowsNamespace(symlink_target_absolute);
+
           if (std::filesystem::exists(dest_file_path, error)) {
             if (std::filesystem::is_symlink(dest_file_path, error)) {
               auto current_dest_symlink_target =
@@ -4896,9 +4919,37 @@ 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);
+              }
+              current_dest_symlink_target_absolute =
+                  StripWindowsNamespace(current_dest_symlink_target_absolute);
+              // Equal targets are safe unless they point to the destination
+              // root or one of its ancestors.
+              bool same_target = false;
+              if (symlink_target_absolute ==
+                  current_dest_symlink_target_absolute) {
+                auto canonical_dest =
+                    std::filesystem::weakly_canonical(dest_path, error);
+                if (error) {
+                  return CpError::Std(error, dest_str);
+                }
+                same_target =
+                    !isInsideDir(symlink_target_absolute, canonical_dest);
+              }
+
               if (!options.dereference &&
-                  std::filesystem::is_directory(symlink_target, error) &&
-                  isInsideDir(symlink_target, current_dest_symlink_target)) {
+                  std::filesystem::is_directory(symlink_target_absolute,
+                                                error) &&
+                  isInsideDir(symlink_target_absolute,
+                              current_dest_symlink_target_absolute) &&
+                  !same_target) {
                 return {CpError::kEinval,
                         0,
                         "cp",
@@ -4912,7 +4963,9 @@ 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_absolute,
+                              symlink_target_absolute) &&
+                  !same_target) {
                 return {CpError::kSymlinkToSubdirectory,
                         0,
                         "cp",
@@ -4939,27 +4992,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,73 @@
+// Repeated copies may replace identical links to unrelated directories or
+// children of dest, but must still reject links to dest or its ancestors.
+// Refs: 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 { mkdirSync, readlinkSync, realpathSync, symlinkSync } from 'node:fs';
+import { cp } from 'node:fs/promises';
+import { join, relative } from 'node:path';
+import tmpdir from '../common/tmpdir.js';
+
+tmpdir.refresh();
+
+// The first unfiltered copy to a missing destination also exercises the
+// native asynchronous directory-copy path.
+for (const relativeTarget of [false, true]) {
+  const root = nextdir();
+  const src = join(root, 'src');
+  const dest = join(root, 'dest');
+  const target = join(root, 'target');
+  mkdirSync(src, { recursive: true });
+  mkdirSync(target);
+  symlinkSync(relativeTarget ? '../target' : target, join(src, 'link'), 'dir');
+  const opts = mustNotMutateObjectDeep({ recursive: true });
+  await cp(src, dest, opts);
+  await cp(src, dest, opts);
+  assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+}
+
+for (const filtered of [false, true]) {
+  for (const aliased of [false, true]) {
+    for (const relativeTarget of [false, true]) {
+      for (const location of ['outside', 'inside', 'self', 'ancestor']) {
+        const root = nextdir();
+        const src = join(root, 'src');
+        const actual = join(root, 'actual');
+        mkdirSync(src, { recursive: true });
+        mkdirSync(join(actual, 'dest'), { recursive: true });
+        let dest = join(actual, 'dest');
+        if (aliased) {
+          const alias = join(root, 'alias');
+          symlinkSync(actual, alias, 'junction');
+          dest = join(alias, 'dest');
+        }
+
+        const target = {
+          outside: join(root, 'target'),
+          inside: join(actual, 'dest', 'sub'),
+          self: join(actual, 'dest'),
+          ancestor: actual,
+        }[location];
+        mkdirSync(target, { recursive: true });
+        const canonicalTarget = realpathSync(target);
+        symlinkSync(relativeTarget ? relative(src, canonicalTarget) : canonicalTarget,
+                    join(src, 'link'), 'dir');
+        const opts = mustNotMutateObjectDeep({
+          recursive: true,
+          ...(filtered ? { filter: () => true } : {}),
+        });
+        await cp(src, dest, opts);
+        const link = join(dest, 'link');
+        const originalTarget = readlinkSync(link);
+        if (location === 'self' || location === 'ancestor') {
+          await assert.rejects(cp(src, dest, opts), { code: 'ERR_FS_CP_EINVAL' });
+          assert.strictEqual(readlinkSync(link), originalTarget);
+        } else {
+          await cp(src, dest, opts);
+        }
+        assert.strictEqual(realpathSync(link), canonicalTarget);
+      }
+    }
+  }
+}

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

@@ -0,0 +1,71 @@
+// Repeated copies may replace identical links to unrelated directories or
+// children of dest, but must still reject links to dest or its ancestors.
+// Refs: 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, readlinkSync, realpathSync, symlinkSync } from 'node:fs';
+import { join, relative } from 'node:path';
+import tmpdir from '../common/tmpdir.js';
+
+tmpdir.refresh();
+
+// Preserve coverage for initially missing destinations and relative targets.
+for (const relativeTarget of [false, true]) {
+  const root = nextdir();
+  const src = join(root, 'src');
+  const dest = join(root, 'dest');
+  const target = join(root, 'target');
+  mkdirSync(src, { recursive: true });
+  mkdirSync(target);
+  symlinkSync(relativeTarget ? '../target' : target, join(src, 'link'), 'dir');
+  const opts = mustNotMutateObjectDeep({ recursive: true });
+  cpSync(src, dest, opts);
+  cpSync(src, dest, opts);
+  assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+}
+
+for (const filtered of [false, true]) {
+  for (const aliased of [false, true]) {
+    for (const relativeTarget of [false, true]) {
+      for (const location of ['outside', 'inside', 'self', 'ancestor']) {
+        const root = nextdir();
+        const src = join(root, 'src');
+        const actual = join(root, 'actual');
+        mkdirSync(src, { recursive: true });
+        mkdirSync(join(actual, 'dest'), { recursive: true });
+        let dest = join(actual, 'dest');
+        if (aliased) {
+          const alias = join(root, 'alias');
+          symlinkSync(actual, alias, 'junction');
+          dest = join(alias, 'dest');
+        }
+
+        const target = {
+          outside: join(root, 'target'),
+          inside: join(actual, 'dest', 'sub'),
+          self: join(actual, 'dest'),
+          ancestor: actual,
+        }[location];
+        mkdirSync(target, { recursive: true });
+        const canonicalTarget = realpathSync(target);
+        symlinkSync(relativeTarget ? relative(src, canonicalTarget) : canonicalTarget,
+                    join(src, 'link'), 'dir');
+        const opts = mustNotMutateObjectDeep({
+          recursive: true,
+          ...(filtered ? { filter: () => true } : {}),
+        });
+        cpSync(src, dest, opts);
+        const link = join(dest, 'link');
+        const originalTarget = readlinkSync(link);
+        if (location === 'self' || location === 'ancestor') {
+          assert.throws(() => cpSync(src, dest, opts), { code: 'ERR_FS_CP_EINVAL' });
+          assert.strictEqual(readlinkSync(link), originalTarget);
+        } else {
+          cpSync(src, dest, opts);
+        }
+        assert.strictEqual(realpathSync(link), canonicalTarget);
+      }
+    }
+  }
+}