From 95f25ad1e2e9670d7172a09f9bb46675eb852104 Mon Sep 17 00:00:00 2001 From: haramjeong <04harams77@gmail.com> Date: Thu, 20 Aug 2026 10:34:46 +0900 Subject: [PATCH 1/2] 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> Assisted-by: Codex --- lib/internal/fs/cp/cp-sync.js | 16 +++- lib/internal/fs/cp/cp.js | 12 ++- src/node_file.cc | 78 ++++++++++++------- ...test-fs-cp-async-symlink-repeated-copy.mjs | 62 +++++++++++++++ .../test-fs-cp-sync-symlink-repeated-copy.mjs | 48 ++++++++++++ 5 files changed, 184 insertions(+), 32 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 cdbec3e6796e..ecfc6c14f8ab 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. @@ -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, diff --git a/lib/internal/fs/cp/cp.js b/lib/internal/fs/cp/cp.js index 1ec5a2d89cac..31422f650094 100644 --- a/lib/internal/fs/cp/cp.js +++ b/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, diff --git a/src/node_file.cc b/src/node_file.cc index b624e9da41bd..9a43bcc1bab8 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -4838,9 +4838,9 @@ CpError CopyDirRecursive(const std::filesystem::path& src_path, std::function copy_dir_contents; - copy_dir_contents = [&options, ©_dir_contents, file_copy_opts]( - std::filesystem::path src, - std::filesystem::path dest) -> CpError { + copy_dir_contents = + [&options, ©_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); 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..54c079a9e5ce --- /dev/null +++ b/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)); + })); + })); + })); + })); +} 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)); +} From 239d414f8f2322d2411c61cb82c6c7020b1ca6e6 Mon Sep 17 00:00:00 2001 From: haramjeong <04harams77@gmail.com> Date: Mon, 28 Sep 2026 09:45:43 +0900 Subject: [PATCH 2/2] fs: preserve symlink copy guards for aliased paths Canonicalize the destination root before allowing identical symlink replacements, and use real paths for the corresponding JavaScript guard. Share Windows namespace normalization and simplify the async tests. Cover descendant, equal, and ancestor targets through direct and aliased destinations with absolute and relative links, with and without filters. Signed-off-by: haramjeong <04harams77@gmail.com> Assisted-by: Codex --- lib/internal/fs/cp/cp-sync.js | 5 +- lib/internal/fs/cp/cp.js | 5 +- src/node_file.cc | 58 +++++---- ...test-fs-cp-async-symlink-repeated-copy.mjs | 111 ++++++++++-------- .../test-fs-cp-sync-symlink-repeated-copy.mjs | 91 ++++++++------ 5 files changed, 156 insertions(+), 114 deletions(-) diff --git a/lib/internal/fs/cp/cp-sync.js b/lib/internal/fs/cp/cp-sync.js index ecfc6c14f8ab..3bb828320d75 100644 --- a/lib/internal/fs/cp/cp-sync.js +++ b/lib/internal/fs/cp/cp-sync.js @@ -25,6 +25,7 @@ const { mkdirSync, opendirSync, readlinkSync, + realpathSync, statSync, symlinkSync, unlinkSync, @@ -228,8 +229,8 @@ function onLink(destStat, src, dest, verbatimSymlinks, destRoot) { // 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)); + const sameTarget = srcIsDir && resolvedSrc === resolvedDest && + !isSrcSubdir(realpathSync(src), realpathSync(destRoot)); if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest) && !sameTarget) { throw new ERR_FS_CP_EINVAL({ diff --git a/lib/internal/fs/cp/cp.js b/lib/internal/fs/cp/cp.js index 31422f650094..d718708a9e19 100644 --- a/lib/internal/fs/cp/cp.js +++ b/lib/internal/fs/cp/cp.js @@ -43,6 +43,7 @@ const { mkdir, opendir, readlink, + realpath, stat, symlink, unlink, @@ -389,8 +390,8 @@ async function onLink(destStat, src, dest, opts) { // 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)); + 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({ diff --git a/src/node_file.cc b/src/node_file.cc index 9a43bcc1bab8..fd1e62a30173 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -4643,18 +4643,23 @@ static void CpSyncCheckPaths(const FunctionCallbackInfo& 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 normalizePathToArray( const std::filesystem::path& path) { std::vector 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()); } @@ -4903,12 +4908,8 @@ CpError CopyDirRecursive(const std::filesystem::path& src_path, 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 + symlink_target_absolute = + StripWindowsNamespace(symlink_target_absolute); if (std::filesystem::exists(dest_file_path, error)) { if (std::filesystem::is_symlink(dest_file_path, error)) { @@ -4927,23 +4928,27 @@ CpError CopyDirRecursive(const std::filesystem::path& src_path, 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 + 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 = - symlink_target_absolute == - current_dest_symlink_target_absolute && - !isInsideDir(symlink_target_absolute, dest_path); + 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, @@ -4958,7 +4963,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_absolute, + symlink_target_absolute) && !same_target) { return {CpError::kSymlinkToSubdirectory, 0, diff --git a/test/parallel/test-fs-cp-async-symlink-repeated-copy.mjs b/test/parallel/test-fs-cp-async-symlink-repeated-copy.mjs index 54c079a9e5ce..687ac74b7890 100644 --- a/test/parallel/test-fs-cp-async-symlink-repeated-copy.mjs +++ b/test/parallel/test-fs-cp-async-symlink-repeated-copy.mjs @@ -1,62 +1,73 @@ -// 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'; +// 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 { cp, mkdirSync, realpathSync, symlinkSync } from 'node:fs'; -import { join } from 'node:path'; - +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(); - -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)); - })); - })); - })); -})); +tmpdir.refresh(); -// A symlink with a relative target pointing to an unrelated directory. -{ +// 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, mustNotMutateObjectDeep({ recursive: true })); + mkdirSync(src, { 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)); + 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'); + } - // 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)); - })); - })); - })); - })); + 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); + } + } + } } diff --git a/test/parallel/test-fs-cp-sync-symlink-repeated-copy.mjs b/test/parallel/test-fs-cp-sync-symlink-repeated-copy.mjs index def28f8e115f..61355b921d93 100644 --- a/test/parallel/test-fs-cp-sync-symlink-repeated-copy.mjs +++ b/test/parallel/test-fs-cp-sync-symlink-repeated-copy.mjs @@ -1,48 +1,71 @@ -// This tests that repeatedly copying a directory containing a symlink -// to an unrelated directory succeeds. -// See https://github.com/nodejs/node/issues/65097. +// 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, realpathSync, symlinkSync } from 'node:fs'; -import { join } from 'node:path'; - +import { cpSync, mkdirSync, readlinkSync, realpathSync, symlinkSync } from 'node:fs'; +import { join, relative } 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)); +tmpdir.refresh(); -// A symlink with a relative target pointing to an unrelated directory. -{ +// 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, mustNotMutateObjectDeep({ recursive: true })); + mkdirSync(src, { recursive: true }); mkdirSync(target); - symlinkSync('../target', join(src, 'link')); - cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true })); - cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true })); + 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)); +} - // 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)); +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); + } + } + } }