From e8d792db92c1fc650a97de9beaf09adc21ba5832 Mon Sep 17 00:00:00 2001 From: kuldeepsinh19 Date: Wed, 4 Nov 2026 04:45:00 +0000 Subject: [PATCH 1/3] fs: respect force when cp() copies a symlink When cp() or fsPromises.cp() copied a symlink with force set to false, an existing destination symlink was still replaced, and an existing file failed with EEXIST from symlink(2). errorOnExist was ignored in both cases. This is a breaking change: with force set to false, an existing destination is now left in place, as it already is when copying a file, or the copy fails with ERR_FS_CP_EEXIST when errorOnExist is set. This is the behavior the documentation describes for both options. Copies with force set to true, the default, are unchanged. Refs: https://github.com/nodejs/node/issues/66774 Signed-off-by: kuldeepsinh19 --- lib/internal/fs/cp/cp.js | 13 ++ ...sync-symlink-existing-dest-force-false.mjs | 131 ++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 test/parallel/test-fs-cp-async-symlink-existing-dest-force-false.mjs diff --git a/lib/internal/fs/cp/cp.js b/lib/internal/fs/cp/cp.js index 1ec5a2d89cac..53b5b19bb408 100644 --- a/lib/internal/fs/cp/cp.js +++ b/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); diff --git a/test/parallel/test-fs-cp-async-symlink-existing-dest-force-false.mjs b/test/parallel/test-fs-cp-async-symlink-existing-dest-force-false.mjs new file mode 100644 index 000000000000..0720acc1798d --- /dev/null +++ b/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)); +} From 7cf034b0809c20c329f25df73eb7a1f9911760dc Mon Sep 17 00:00:00 2001 From: kuldeepsinh19 Date: Wed, 4 Nov 2026 04:45:03 +0000 Subject: [PATCH 2/3] fs: respect force when cpSync() copies a symlink cpSync() had the same problem as cp() when it copied a symlink, on its own or inside a directory copied with a filter function: with force set to false, an existing destination symlink was replaced, an existing file failed with EEXIST from symlink(2), and a dangling destination symlink failed with ENOENT. errorOnExist was ignored. This is a breaking change: with force set to false, an existing destination is now left in place, or the copy fails with ERR_FS_CP_EEXIST when errorOnExist is set, as with cp(). onLink() now takes the options to check them. Copies with force set to true, the default, are unchanged. Refs: https://github.com/nodejs/node/issues/66774 Signed-off-by: kuldeepsinh19 --- lib/internal/fs/cp/cp-sync.js | 19 +++- ...sync-symlink-existing-dest-force-false.mjs | 103 ++++++++++++++++++ 2 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-fs-cp-sync-symlink-existing-dest-force-false.mjs diff --git a/lib/internal/fs/cp/cp-sync.js b/lib/internal/fs/cp/cp-sync.js index cdbec3e6796e..64938d12c83c 100644 --- a/lib/internal/fs/cp/cp-sync.js +++ b/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,9 +198,9 @@ 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; @@ -208,6 +208,19 @@ function onLink(destStat, src, dest, verbatimSymlinks) { 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); diff --git a/test/parallel/test-fs-cp-sync-symlink-existing-dest-force-false.mjs b/test/parallel/test-fs-cp-sync-symlink-existing-dest-force-false.mjs new file mode 100644 index 000000000000..961fe97db64a --- /dev/null +++ b/test/parallel/test-fs-cp-sync-symlink-existing-dest-force-false.mjs @@ -0,0 +1,103 @@ +// 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)); +} From 09beec0981ab9181429ffe2a6498003d21f2cc19 Mon Sep 17 00:00:00 2001 From: kuldeepsinh19 Date: Wed, 4 Nov 2026 04:45:06 +0000 Subject: [PATCH 3/3] fs: respect force for symlinks in the native cpSync() walk Without a filter function, a recursive cpSync() copies the directory in C++. With force set to false, that walk still replaced an existing destination symlink, as a comment there pointed out, and failed with EEXIST for an existing file, for a dangling symlink, and for anything already there when verbatimSymlinks was set. This is a breaking change: with force set to false, an existing destination is now left in place, as in the JavaScript walk. It is looked up with symlink_status(), so that a dangling symlink counts as existing, as it does for lstat() there. Copies with force set to true, the default, are unchanged. Fixes: https://github.com/nodejs/node/issues/66774 Signed-off-by: kuldeepsinh19 --- src/node_file.cc | 23 ++++++++++++++++--- ...sync-symlink-existing-dest-force-false.mjs | 21 +++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index e9f441715565..a1ce0272ac8a 100644 --- a/src/node_file.cc +++ b/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); diff --git a/test/parallel/test-fs-cp-sync-symlink-existing-dest-force-false.mjs b/test/parallel/test-fs-cp-sync-symlink-existing-dest-force-false.mjs index 961fe97db64a..6d3df98fd51e 100644 --- a/test/parallel/test-fs-cp-sync-symlink-existing-dest-force-false.mjs +++ b/test/parallel/test-fs-cp-sync-symlink-existing-dest-force-false.mjs @@ -101,3 +101,24 @@ for (const { create, read, expected } of Object.values(existing)) { 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)); +}