fs: fix opts.filter issue in cp async

PR-URL: #44922 Fixes: #44720 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
nodejs · Oct 11, 2022 · bc00f3b · bc00f3b
1 parent 4bdef48
commit bc00f3b
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 19 deletions.
diff --git a/lib/internal/fs/cp/cp.js b/lib/internal/fs/cp/cp.js
@@ -63,15 +63,16 @@ async function cpFn(src, dest, opts) {
     process.emitWarning(warning, 'TimestampPrecisionWarning');
   }
   const stats = await checkPaths(src, dest, opts);
-  const { srcStat, destStat } = stats;
+  const { srcStat, destStat, skipped } = stats;
+  if (skipped) return;
   await checkParentPaths(src, srcStat, dest);
-  if (opts.filter) {
-    return handleFilter(checkParentDir, destStat, src, dest, opts);
-  }
   return checkParentDir(destStat, src, dest, opts);
 }
 
 async function checkPaths(src, dest, opts) {
+  if (opts.filter && !(await opts.filter(src, dest))) {
+    return { __proto__: null, skipped: true };
+  }
   const { 0: srcStat, 1: destStat } = await getStats(src, dest, opts);
   if (destStat) {
     if (areIdentical(srcStat, destStat)) {
@@ -114,7 +115,7 @@ async function checkPaths(src, dest, opts) {
       code: 'EINVAL',
     });
   }
-  return { srcStat, destStat };
+  return { srcStat, destStat, skipped: false };
 }
 
 function areIdentical(srcStat, destStat) {
@@ -190,18 +191,6 @@ function isSrcSubdir(src, dest) {
   return ArrayPrototypeEvery(srcArr, (cur, i) => destArr[i] === cur);
 }
 
-async function handleFilter(onInclude, destStat, src, dest, opts, cb) {
-  const include = await opts.filter(src, dest);
-  if (include) return onInclude(destStat, src, dest, opts, cb);
-}
-
-function startCopy(destStat, src, dest, opts) {
-  if (opts.filter) {
-    return handleFilter(getStatsForCopy, destStat, src, dest, opts);
-  }
-  return getStatsForCopy(destStat, src, dest, opts);
-}
-
 async function getStatsForCopy(destStat, src, dest, opts) {
   const statFn = opts.dereference ? stat : lstat;
   const srcStat = await statFn(src);
@@ -328,8 +317,8 @@ async function copyDir(src, dest, opts) {
   for await (const { name } of dir) {
     const srcItem = join(src, name);
     const destItem = join(dest, name);
-    const { destStat } = await checkPaths(srcItem, destItem, opts);
-    await startCopy(destStat, srcItem, destItem, opts);
+    const { destStat, skipped } = await checkPaths(srcItem, destItem, opts);
+    if (!skipped) await getStatsForCopy(destStat, srcItem, destItem, opts);
   }
 }
 

diff --git a/test/parallel/test-fs-cp.mjs b/test/parallel/test-fs-cp.mjs
@@ -746,6 +746,43 @@ if (!isWindows) {
      }));
 }
 
+// Copy async should not throw exception if child folder is filtered out.
+{
+  const src = nextdir();
+  mkdirSync(join(src, 'test-cp-async'), mustNotMutateObjectDeep({ recursive: true }));
+
+  const dest = nextdir();
+  mkdirSync(dest, mustNotMutateObjectDeep({ recursive: true }));
+  writeFileSync(join(dest, 'test-cp-async'), 'test-content', mustNotMutateObjectDeep({ mode: 0o444 }));
+
+  cp(src, dest, {
+    filter: (path) => !path.includes('test-cp-async'),
+    recursive: true,
+  }, mustCall((err) => {
+    assert.strictEqual(err, null);
+  }));
+}
+
+// Copy async should not throw exception if dest is invalid but filtered out.
+{
+  // Create dest as a file.
+  // Expect: cp skips the copy logic entirely and won't throw any exception in path validation process.
+  const src = join(nextdir(), 'bar');
+  mkdirSync(src, mustNotMutateObjectDeep({ recursive: true }));
+
+  const destParent = nextdir();
+  const dest = join(destParent, 'bar');
+  mkdirSync(destParent, mustNotMutateObjectDeep({ recursive: true }));
+  writeFileSync(dest, 'test-content', mustNotMutateObjectDeep({ mode: 0o444 }));
+
+  cp(src, dest, {
+    filter: (path) => !path.includes('bar'),
+    recursive: true,
+  }, mustCall((err) => {
+    assert.strictEqual(err, null);
+  }));
+}
+
 // It throws if options is not object.
 {
   assert.throws(