CI check: fs: respect force: false for symlinks in cp (rebased)

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

@@ -71,7 +71,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);
   }
 
   // It is not possible to get here because all possible cases are handled above.
@@ -198,16 +198,29 @@ 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, opts) {
   let resolvedSrc = readlinkSync(src);
-  if (!verbatimSymlinks && !isAbsolute(resolvedSrc)) {
+  if (!opts.verbatimSymlinks && !isAbsolute(resolvedSrc)) {
     resolvedSrc = resolve(dirname(src), resolvedSrc);
   }
   const srcIsDir = fsBinding.internalModuleStat(src) === 1;
   const symlinkType = srcIsDir ? 'dir' : 'file';
   if (!destStat) {
     return symlinkSync(resolvedSrc, dest, symlinkType);
   }
+  // Like a file, an existing destination is only replaced when force is set.
+  if (!opts.force) {
+    if (opts.errorOnExist) {
+      throw new ERR_FS_CP_EEXIST({
+        message: `${dest} already exists`,
+        path: dest,
+        syscall: 'cp',
+        errno: EEXIST,
+        code: 'EEXIST',
+      });
+    }
+    return;
+  }
   let resolvedDest;
   try {
     resolvedDest = readlinkSync(dest);

lib/internal/fs/cp/cp.js

@@ -369,6 +369,19 @@ async function onLink(destStat, src, dest, opts) {
   if (!destStat) {
     return symlink(resolvedSrc, dest, symlinkType);
   }
+  // Like a file, an existing destination is only replaced when force is set.
+  if (!opts.force) {
+    if (opts.errorOnExist) {
+      throw new ERR_FS_CP_EEXIST({
+        message: `${dest} already exists`,
+        path: dest,
+        syscall: 'cp',
+        errno: EEXIST,
+        code: 'EEXIST',
+      });
+    }
+    return;
+  }
   let resolvedDest;
   try {
     resolvedDest = await readlink(dest);

src/node_file.cc

@@ -4963,6 +4963,26 @@ CpError CopyDirRecursive(const std::filesystem::path& src_path,
       }
 
       if (is_symlink && !options.dereference) {
+        // Like a file, an existing destination is only replaced when force is
+        // set. symlink_status() does not follow a link already there, so a
+        // dangling one counts as existing, as with lstat() in the JS walk.
+        if (!options.force) {
+          std::error_code dest_error;
+          if (std::filesystem::exists(std::filesystem::symlink_status(
+                  dest_file_path, dest_error))) {
+            if (options.error_on_exist) {
+              return {CpError::kEexist,
+                      0,
+                      "cp",
+                      SPrintF("[ERR_FS_CP_EEXIST]: Target already exists: "
+                              "cp returned EEXIST (%s already exists)",
+                              dest_file_path),
+                      {}};
+            }
+            continue;
+          }
+        }
+
         if (options.verbatim_symlinks) {
           std::filesystem::copy_symlink(
               dir_entry.path(), dest_file_path, error);
@@ -5010,9 +5030,6 @@ CpError CopyDirRecursive(const std::filesystem::path& src_path,
                         {}};
               }
 
-              // symlinks get overridden by cp even if force: false, this is
-              // being applied here for backward compatibility, but is it
-              // correct? or is it a bug?
               std::filesystem::remove(dest_file_path, error);
               if (error) {
                 return CpError::Std(error, dest_str);

test/parallel/test-fs-cp-async-symlink-existing-dest-force-false.mjs

@@ -0,0 +1,131 @@
+// This tests that cp() and fsPromises.cp() leave an existing destination in
+// place when copying a symlink with force set to false, and fail with
+// ERR_FS_CP_EEXIST when errorOnExist is also set, as they do for files.
+// Refs: https://github.com/nodejs/node/issues/66774
+
+import {
+  canCreateSymLink,
+  mustCall,
+  mustNotMutateObjectDeep,
+  skip,
+} from '../common/index.mjs';
+import { nextdir } from '../common/fs.js';
+import assert from 'node:assert';
+import {
+  cp,
+  mkdirSync,
+  promises,
+  readFileSync,
+  readlinkSync,
+  realpathSync,
+  symlinkSync,
+  writeFileSync,
+} from 'node:fs';
+import { join } from 'node:path';
+import tmpdir from '../common/tmpdir.js';
+
+if (!canCreateSymLink())
+  skip('insufficient privileges to create symbolic links');
+
+tmpdir.refresh();
+
+const targets = nextdir();
+mkdirSync(targets);
+const srcTarget = join(targets, 'src.txt');
+const destTarget = join(targets, 'dest.txt');
+const missingTarget = join(targets, 'missing');
+writeFileSync(srcTarget, 'src');
+writeFileSync(destTarget, 'dest');
+
+// What may already be at the destination, how to read it back, and what that
+// gives as long as it is left in place.
+const existing = {
+  'symlink': {
+    create: (path) => symlinkSync(destTarget, path),
+    read: readlinkSync,
+    expected: destTarget,
+  },
+  'dangling symlink': {
+    create: (path) => symlinkSync(missingTarget, path),
+    read: readlinkSync,
+    expected: missingTarget,
+  },
+  'file': {
+    create: (path) => writeFileSync(path, 'existing'),
+    read: (path) => readFileSync(path, 'utf8'),
+    expected: 'existing',
+  },
+};
+
+for (const { create, read, expected } of Object.values(existing)) {
+  {
+    const src = nextdir();
+    const dest = nextdir();
+    symlinkSync(srcTarget, src);
+    create(dest);
+    cp(src, dest, mustNotMutateObjectDeep({ force: false }), mustCall((err) => {
+      assert.ifError(err);
+      assert.strictEqual(read(dest), expected);
+    }));
+  }
+
+  {
+    const src = nextdir();
+    const dest = nextdir();
+    symlinkSync(srcTarget, src);
+    create(dest);
+    const opts = { force: false, errorOnExist: true };
+    cp(src, dest, mustNotMutateObjectDeep(opts), mustCall((err) => {
+      assert.strictEqual(err.code, 'ERR_FS_CP_EEXIST');
+      assert.strictEqual(read(dest), expected);
+    }));
+  }
+
+  {
+    const src = nextdir();
+    const dest = nextdir();
+    symlinkSync(srcTarget, src);
+    create(dest);
+    await promises.cp(src, dest, mustNotMutateObjectDeep({ force: false }));
+    assert.strictEqual(read(dest), expected);
+    await assert.rejects(
+      promises.cp(src, dest, mustNotMutateObjectDeep({ force: false, errorOnExist: true })),
+      { code: 'ERR_FS_CP_EEXIST' },
+    );
+    assert.strictEqual(read(dest), expected);
+  }
+
+  // A symlink inside a directory that is copied into an existing one.
+  for (const verbatimSymlinks of [false, true]) {
+    const src = nextdir();
+    const dest = nextdir();
+    mkdirSync(src);
+    mkdirSync(dest);
+    symlinkSync(srcTarget, join(src, 'link'));
+    create(join(dest, 'link'));
+    const opts = { force: false, recursive: true, verbatimSymlinks };
+    await promises.cp(src, dest, mustNotMutateObjectDeep(opts));
+    assert.strictEqual(read(join(dest, 'link')), expected);
+  }
+}
+
+// With force (the default), an existing destination symlink is still replaced.
+{
+  const src = nextdir();
+  const dest = nextdir();
+  symlinkSync(srcTarget, src);
+  symlinkSync(destTarget, dest);
+  await promises.cp(src, dest);
+  assert.strictEqual(realpathSync(dest), realpathSync(srcTarget));
+}
+
+{
+  const src = nextdir();
+  const dest = nextdir();
+  mkdirSync(src);
+  mkdirSync(dest);
+  symlinkSync(srcTarget, join(src, 'link'));
+  symlinkSync(destTarget, join(dest, 'link'));
+  await promises.cp(src, dest, mustNotMutateObjectDeep({ recursive: true }));
+  assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(srcTarget));
+}

test/parallel/test-fs-cp-sync-symlink-existing-dest-force-false.mjs

@@ -0,0 +1,124 @@
+// This tests that cpSync() leaves an existing destination in place when copying
+// a symlink with force set to false, and throws ERR_FS_CP_EEXIST when
+// errorOnExist is also set, as it does for files.
+// Refs: https://github.com/nodejs/node/issues/66774
+
+import { canCreateSymLink, mustNotMutateObjectDeep, skip } from '../common/index.mjs';
+import { nextdir } from '../common/fs.js';
+import assert from 'node:assert';
+import {
+  cpSync,
+  mkdirSync,
+  readFileSync,
+  readlinkSync,
+  realpathSync,
+  symlinkSync,
+  writeFileSync,
+} from 'node:fs';
+import { join } from 'node:path';
+import tmpdir from '../common/tmpdir.js';
+
+if (!canCreateSymLink())
+  skip('insufficient privileges to create symbolic links');
+
+tmpdir.refresh();
+
+const targets = nextdir();
+mkdirSync(targets);
+const srcTarget = join(targets, 'src.txt');
+const destTarget = join(targets, 'dest.txt');
+const missingTarget = join(targets, 'missing');
+writeFileSync(srcTarget, 'src');
+writeFileSync(destTarget, 'dest');
+
+// What may already be at the destination, how to read it back, and what that
+// gives as long as it is left in place.
+const existing = {
+  'symlink': {
+    create: (path) => symlinkSync(destTarget, path),
+    read: readlinkSync,
+    expected: destTarget,
+  },
+  'dangling symlink': {
+    create: (path) => symlinkSync(missingTarget, path),
+    read: readlinkSync,
+    expected: missingTarget,
+  },
+  'file': {
+    create: (path) => writeFileSync(path, 'existing'),
+    read: (path) => readFileSync(path, 'utf8'),
+    expected: 'existing',
+  },
+};
+
+// A source directory with a symlink in it, and an existing destination
+// directory with something already where the symlink would be copied.
+function makeDirs(create) {
+  const src = nextdir();
+  const dest = nextdir();
+  mkdirSync(src);
+  mkdirSync(dest);
+  symlinkSync(srcTarget, join(src, 'link'));
+  create(join(dest, 'link'));
+  return { src, dest, destLink: join(dest, 'link') };
+}
+
+for (const { create, read, expected } of Object.values(existing)) {
+  {
+    const src = nextdir();
+    const dest = nextdir();
+    symlinkSync(srcTarget, src);
+    create(dest);
+    cpSync(src, dest, mustNotMutateObjectDeep({ force: false }));
+    assert.strictEqual(read(dest), expected);
+    assert.throws(
+      () => cpSync(src, dest, mustNotMutateObjectDeep({ force: false, errorOnExist: true })),
+      { code: 'ERR_FS_CP_EEXIST' },
+    );
+    assert.strictEqual(read(dest), expected);
+  }
+
+  // With a filter, cpSync() walks the directory in JavaScript.
+  for (const verbatimSymlinks of [false, true]) {
+    const { src, dest, destLink } = makeDirs(create);
+    cpSync(src, dest, { force: false, recursive: true, verbatimSymlinks, filter: () => true });
+    assert.strictEqual(read(destLink), expected);
+  }
+}
+
+// With force (the default), an existing destination symlink is still replaced.
+{
+  const src = nextdir();
+  const dest = nextdir();
+  symlinkSync(srcTarget, src);
+  symlinkSync(destTarget, dest);
+  cpSync(src, dest);
+  assert.strictEqual(realpathSync(dest), realpathSync(srcTarget));
+}
+
+{
+  const { src, dest, destLink } = makeDirs((path) => symlinkSync(destTarget, path));
+  cpSync(src, dest, { recursive: true, filter: () => true });
+  assert.strictEqual(realpathSync(destLink), realpathSync(srcTarget));
+}
+
+// Without a filter, cpSync() copies the directory in C++.
+for (const { create, read, expected } of Object.values(existing)) {
+  for (const verbatimSymlinks of [false, true]) {
+    const { src, dest, destLink } = makeDirs(create);
+    const opts = { force: false, recursive: true, verbatimSymlinks };
+    cpSync(src, dest, mustNotMutateObjectDeep(opts));
+    assert.strictEqual(read(destLink), expected);
+    assert.throws(
+      () => cpSync(src, dest, mustNotMutateObjectDeep({ ...opts, errorOnExist: true })),
+      { code: 'ERR_FS_CP_EEXIST' },
+    );
+    assert.strictEqual(read(destLink), expected);
+  }
+}
+
+{
+  const { src, dest, destLink } = makeDirs((path) => symlinkSync(destTarget, path));
+  cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true }));
+  assert.strictEqual(realpathSync(destLink), realpathSync(srcTarget));
+}