From 782049579d68f92e65b090261cbb97e57861f9b2 Mon Sep 17 00:00:00 2001 From: haramjeong <04harams77@gmail.com> Date: Thu, 20 Aug 2026 10:34:46 +0900 Subject: [PATCH] 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: https://github.com/nodejs/node/issues/65097 Signed-off-by: haramjeong <04harams77@gmail.com> --- lib/internal/fs/cp/cp-sync.js | 16 +++++-- lib/internal/fs/cp/cp.js | 12 ++++- src/node_file.cc | 43 +++++++++++++---- ...test-fs-cp-async-symlink-repeated-copy.mjs | 44 +++++++++++++++++ .../test-fs-cp-sync-symlink-repeated-copy.mjs | 48 +++++++++++++++++++ 5 files changed, 147 insertions(+), 16 deletions(-) create mode 100644 test/parallel/test-fs-cp-async-symlink-repeated-copy.mjs create mode 100644 test/parallel/test-fs-cp-sync-symlink-repeated-copy.mjs diff --git a/lib/internal/fs/cp/cp-sync.js b/lib/internal/fs/cp/cp-sync.js index 03fcae9b7cdb..33925a0ab48d 100644 --- a/lib/internal/fs/cp/cp-sync.js +++ b/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. @@ -186,7 +187,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); @@ -211,7 +212,13 @@ function onLink(destStat, src, dest, verbatimSymlinks) { } const srcIsDir = fsBinding.internalModuleStat(src) === 1; - 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}`, @@ -224,7 +231,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, diff --git a/lib/internal/fs/cp/cp.js b/lib/internal/fs/cp/cp.js index 10c52b114634..0c56cbb207d2 100644 --- a/lib/internal/fs/cp/cp.js +++ b/lib/internal/fs/cp/cp.js @@ -63,6 +63,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; @@ -357,7 +358,13 @@ async function onLink(destStat, src, dest, opts) { const srcIsDir = fsBinding.internalModuleStat(src) === 1; - 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}`, @@ -371,7 +378,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, diff --git a/src/node_file.cc b/src/node_file.cc index ae0d9f34f8e1..5c0938ff9633 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3755,6 +3755,7 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { force, error_on_exist, dereference, + dest_path, &isolate](std::filesystem::path src, std::filesystem::path dest) { std::error_code error; @@ -3778,6 +3779,15 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { return false; } + auto symlink_target_absolute = std::filesystem::weakly_canonical( + std::filesystem::absolute(src / symlink_target)); +#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)) { if (std::filesystem::is_symlink((dest_file_path.c_str()))) { auto current_dest_symlink_target = @@ -3787,9 +3797,29 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { return false; } + // 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. + auto current_dest_symlink_target_absolute = + std::filesystem::weakly_canonical( + std::filesystem::absolute(dest_file_path.parent_path() / + current_dest_symlink_target)); +#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 + bool same_target = + symlink_target_absolute == + current_dest_symlink_target_absolute && + !isInsideDir(symlink_target_absolute, dest_path); + if (!dereference && std::filesystem::is_directory(symlink_target) && - isInsideDir(symlink_target, current_dest_symlink_target)) { + isInsideDir(symlink_target, current_dest_symlink_target) && + !same_target) { static constexpr const char* message = "Cannot copy %s to a subdirectory of self %s"; THROW_ERR_FS_CP_EINVAL( @@ -3801,7 +3831,8 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { // 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) && - isInsideDir(current_dest_symlink_target, symlink_target)) { + isInsideDir(current_dest_symlink_target, symlink_target) && + !same_target) { static constexpr const char* message = "cannot overwrite %s with %s"; THROW_ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY( @@ -3828,14 +3859,6 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { } } } - auto symlink_target_absolute = std::filesystem::weakly_canonical( - std::filesystem::absolute(src / symlink_target)); -#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()) { std::filesystem::create_directory_symlink( symlink_target_absolute, dest_file_path, error); diff --git a/test/parallel/test-fs-cp-async-symlink-repeated-copy.mjs b/test/parallel/test-fs-cp-async-symlink-repeated-copy.mjs new file mode 100644 index 000000000000..547279352a7c --- /dev/null +++ b/test/parallel/test-fs-cp-async-symlink-repeated-copy.mjs @@ -0,0 +1,44 @@ +// 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)); + })); +})); + +// 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)); + })); + })); +} diff --git a/test/parallel/test-fs-cp-sync-symlink-repeated-copy.mjs b/test/parallel/test-fs-cp-sync-symlink-repeated-copy.mjs new file mode 100644 index 000000000000..def28f8e115f --- /dev/null +++ b/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)); +}