copy*(): fix copying bind-mounted directories (#618)

* copy*(): fix copying bind-mounted dirs * copy*(): fix case-insensitive-paths tests * copy*(): refactor to check paths more efficiently * destructure stats object after checking err
jprichardson · Sep 2, 2018 · e0093f5 · e0093f5
1 parent 1657c61
commit e0093f5
Show file tree

Hide file tree

Showing 18 changed files with 222 additions and 104 deletions.
diff --git a/lib/copy-sync/__tests__/broken-symlink.test.js b/lib/copy-sync/__tests__/broken-symlink.test.js
@@ -2,7 +2,7 @@
 
 const fs = require('fs')
 const os = require('os')
-const fse = require(process.cwd())
+const fse = require('../../')
 const path = require('path')
 const assert = require('assert')
 const copySync = require('../copy-sync')

diff --git a/lib/copy-sync/__tests__/copy-sync-case-insensitive-paths.test.js b/lib/copy-sync/__tests__/copy-sync-case-insensitive-paths.test.js
@@ -3,7 +3,8 @@
 const assert = require('assert')
 const os = require('os')
 const path = require('path')
-const fs = require(process.cwd())
+const fs = require('../../')
+const platform = os.platform()
 
 /* global beforeEach, afterEach, describe, it */
 
@@ -17,7 +18,7 @@ describe('+ copySync() - case insensitive paths', () => {
     fs.emptyDir(TEST_DIR, done)
   })
 
-  afterEach(done => fs.remove(TEST_DIR, done))
+  afterEach(() => fs.removeSync(TEST_DIR))
 
   describe('> when src is a directory', () => {
     it('should behave correctly based on the OS', () => {
@@ -29,15 +30,13 @@ describe('+ copySync() - case insensitive paths', () => {
       try {
         fs.copySync(src, dest)
       } catch (err) {
-        if (os === 'darwin' || os === 'win32') {
+        if (platform === 'darwin' || platform === 'win32') {
           assert.strictEqual(err.message, 'Source and destination must not be the same.')
-          assert(fs.existsSync(src))
-          assert(!fs.existsSync(dest))
           errThrown = true
         }
       }
-      if (os === 'darwin' || os === 'win32') assert(errThrown)
-      if (os === 'linux') {
+      if (platform === 'darwin' || platform === 'win32') assert(errThrown)
+      if (platform === 'linux') {
         assert(fs.existsSync(dest))
         assert.strictEqual(fs.readFileSync(path.join(dest, 'subdir', 'file.txt'), 'utf8'), 'some data')
         assert(!errThrown)
@@ -55,15 +54,13 @@ describe('+ copySync() - case insensitive paths', () => {
       try {
         fs.copySync(src, dest)
       } catch (err) {
-        if (os === 'darwin' || os === 'win32') {
+        if (platform === 'darwin' || platform === 'win32') {
           assert.strictEqual(err.message, 'Source and destination must not be the same.')
-          assert(fs.existsSync(src))
-          assert(!fs.existsSync(dest))
           errThrown = true
         }
       }
-      if (os === 'darwin' || os === 'win32') assert(errThrown)
-      if (os === 'linux') {
+      if (platform === 'darwin' || platform === 'win32') assert(errThrown)
+      if (platform === 'linux') {
         assert(fs.existsSync(dest))
         assert.strictEqual(fs.readFileSync(dest, 'utf8'), 'some data')
         assert(!errThrown)
@@ -77,25 +74,23 @@ describe('+ copySync() - case insensitive paths', () => {
       fs.outputFileSync(path.join(src, 'subdir', 'file.txt'), 'some data')
       const srcLink = path.join(TEST_DIR, 'src-symlink')
       fs.symlinkSync(src, srcLink, 'dir')
-      dest = path.join(TEST_DIR, 'srcDir')
+      dest = path.join(TEST_DIR, 'src-Symlink')
       let errThrown = false
 
       try {
-        fs.copySync(src, dest)
+        fs.copySync(srcLink, dest)
       } catch (err) {
-        if (os === 'darwin' || os === 'win32') {
+        if (platform === 'darwin' || platform === 'win32') {
           assert.strictEqual(err.message, 'Source and destination must not be the same.')
-          assert(fs.existsSync(src))
-          assert(!fs.existsSync(dest))
           errThrown = true
         }
       }
-      if (os === 'darwin' || os === 'win32') assert(errThrown)
-      if (os === 'linux') {
+      if (platform === 'darwin' || platform === 'win32') assert(errThrown)
+      if (platform === 'linux') {
         assert(fs.existsSync(dest))
         assert.strictEqual(fs.readFileSync(path.join(dest, 'subdir', 'file.txt'), 'utf8'), 'some data')
-        const link = fs.readlinkSync(srcLink)
-        assert.strictEqual(link, dest)
+        const destLink = fs.readlinkSync(dest)
+        assert.strictEqual(destLink, src)
         assert(!errThrown)
       }
     })
@@ -105,25 +100,23 @@ describe('+ copySync() - case insensitive paths', () => {
       fs.outputFileSync(src, 'some data')
       const srcLink = path.join(TEST_DIR, 'src-symlink')
       fs.symlinkSync(src, srcLink, 'file')
-      dest = path.join(TEST_DIR, 'srcFile')
+      dest = path.join(TEST_DIR, 'src-Symlink')
       let errThrown = false
 
       try {
-        fs.copySync(src, dest)
+        fs.copySync(srcLink, dest)
       } catch (err) {
-        if (os === 'darwin' || os === 'win32') {
+        if (platform === 'darwin' || platform === 'win32') {
           assert.strictEqual(err.message, 'Source and destination must not be the same.')
-          assert(fs.existsSync(src))
-          assert(!fs.existsSync(dest))
           errThrown = true
         }
       }
-      if (os === 'darwin' || os === 'win32') assert(errThrown)
-      if (os === 'linux') {
+      if (platform === 'darwin' || platform === 'win32') assert(errThrown)
+      if (platform === 'linux') {
         assert(fs.existsSync(dest))
         assert.strictEqual(fs.readFileSync(dest, 'utf8'), 'some data')
-        const link = fs.readlinkSync(srcLink)
-        assert.strictEqual(link, dest)
+        const destLink = fs.readlinkSync(dest)
+        assert.strictEqual(destLink, src)
         assert(!errThrown)
       }
     })

diff --git a/lib/copy-sync/__tests__/copy-sync-dir.test.js b/lib/copy-sync/__tests__/copy-sync-dir.test.js
@@ -1,6 +1,6 @@
 'use strict'
 
-const fs = require(process.cwd())
+const fs = require('../../')
 const os = require('os')
 const path = require('path')
 const assert = require('assert')
@@ -75,7 +75,7 @@ describe('+ copySync() / dir', () => {
       const srcTarget = path.join(TEST_DIR, 'destination')
       fs.mkdirSync(src)
       fs.mkdirSync(srcTarget)
-      fs.symlinkSync(srcTarget, path.join(src, 'symlink'))
+      fs.symlinkSync(srcTarget, path.join(src, 'symlink'), 'dir')
 
       fs.copySync(src, dest)
 

diff --git a/lib/copy-sync/__tests__/copy-sync-file.test.js b/lib/copy-sync/__tests__/copy-sync-file.test.js
@@ -1,6 +1,6 @@
 'use strict'
 
-const fs = require(process.cwd())
+const fs = require('../../')
 const os = require('os')
 const path = require('path')
 const assert = require('assert')

diff --git a/lib/copy-sync/__tests__/copy-sync-preserve-time.test.js b/lib/copy-sync/__tests__/copy-sync-preserve-time.test.js
@@ -1,6 +1,6 @@
 'use strict'
 
-const fs = require(process.cwd())
+const fs = require('../../')
 const os = require('os')
 const path = require('path')
 const utimes = require('../../util/utimes')

diff --git a/lib/copy-sync/__tests__/copy-sync-prevent-copying-identical.test.js b/lib/copy-sync/__tests__/copy-sync-prevent-copying-identical.test.js
@@ -3,7 +3,7 @@
 const assert = require('assert')
 const os = require('os')
 const path = require('path')
-const fs = require(process.cwd())
+const fs = require('../../')
 const klawSync = require('klaw-sync')
 
 /* global beforeEach, afterEach, describe, it */

diff --git a/lib/copy-sync/__tests__/copy-sync-prevent-copying-into-itself.test.js b/lib/copy-sync/__tests__/copy-sync-prevent-copying-into-itself.test.js
@@ -3,7 +3,7 @@
 const assert = require('assert')
 const os = require('os')
 const path = require('path')
-const fs = require(process.cwd())
+const fs = require('../../')
 const klawSync = require('klaw-sync')
 
 /* global beforeEach, afterEach, describe, it */
@@ -166,6 +166,56 @@ describe('+ copySync() - prevent copying into itself', () => {
         assert.strictEqual(link, src)
       })
 
+      it('should error when dest is a subdirectory of src (bind-mounted directory with subdirectory)', () => {
+        const destLink = path.join(TEST_DIR, 'dest-symlink')
+        fs.symlinkSync(src, destLink, 'dir')
+
+        const srclenBefore = klawSync(src).length
+        assert(srclenBefore > 2)
+
+        const dest = path.join(destLink, 'dir1')
+        assert(fs.existsSync(dest))
+        let errThrown = false
+        try {
+          fs.copySync(src, dest)
+        } catch (err) {
+          assert.strictEqual(err.message, `Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`)
+          errThrown = true
+        } finally {
+          assert(errThrown)
+          const srclenAfter = klawSync(src).length
+          assert.strictEqual(srclenAfter, srclenBefore, 'src length should not change')
+
+          const link = fs.readlinkSync(destLink)
+          assert.strictEqual(link, src)
+        }
+      })
+
+      it('should error when dest is a subdirectory of src (more than one level depth)', () => {
+        const destLink = path.join(TEST_DIR, 'dest-symlink')
+        fs.symlinkSync(src, destLink, 'dir')
+
+        const srclenBefore = klawSync(src).length
+        assert(srclenBefore > 2)
+
+        const dest = path.join(destLink, 'dir1', 'dir2')
+        assert(fs.existsSync(dest))
+        let errThrown = false
+        try {
+          fs.copySync(src, dest)
+        } catch (err) {
+          assert.strictEqual(err.message, `Cannot copy '${src}' to a subdirectory of itself, '${path.join(destLink, 'dir1')}'.`)
+          errThrown = true
+        } finally {
+          assert(errThrown)
+          const srclenAfter = klawSync(src).length
+          assert.strictEqual(srclenAfter, srclenBefore, 'src length should not change')
+
+          const link = fs.readlinkSync(destLink)
+          assert.strictEqual(link, src)
+        }
+      })
+
       it('should copy the directory successfully when src is a subdir of resolved dest path', () => {
         const srcInDest = path.join(TEST_DIR, 'dest', 'some', 'nested', 'src')
         const destLink = path.join(TEST_DIR, 'dest-symlink')
@@ -351,10 +401,6 @@ function testSuccess (src, dest) {
 
   fs.copySync(src, dest)
 
-  const destlen = klawSync(dest).length
-
-  assert.strictEqual(destlen, srclen)
-
   FILES.forEach(f => assert(fs.existsSync(path.join(dest, f)), 'file copied'))
 
   const o0 = fs.readFileSync(path.join(dest, FILES[0]), 'utf8')

diff --git a/lib/copy-sync/__tests__/copy-sync-readonly-dir.test.js b/lib/copy-sync/__tests__/copy-sync-readonly-dir.test.js
@@ -2,9 +2,8 @@
 
 // relevant: https://github.com/jprichardson/node-fs-extra/issues/599
 
-const fs = require(process.cwd())
 const os = require('os')
-const fse = require('../../')
+const fs = require('../../')
 const path = require('path')
 const assert = require('assert')
 const klawSync = require('klaw-sync')
@@ -22,12 +21,12 @@ const FILES = [
 describe('+ copySync() - copy a readonly directory with content', () => {
   beforeEach(done => {
     TEST_DIR = path.join(os.tmpdir(), 'test', 'fs-extra', 'copy-readonly-dir')
-    fse.emptyDir(TEST_DIR, done)
+    fs.emptyDir(TEST_DIR, done)
   })
 
   afterEach(done => {
     klawSync(TEST_DIR).forEach(data => fs.chmodSync(data.path, 0o777))
-    fse.remove(TEST_DIR, done)
+    fs.remove(TEST_DIR, done)
   })
 
   describe('> when src is readonly directory with content', () => {
@@ -40,7 +39,7 @@ describe('+ copySync() - copy a readonly directory with content', () => {
       sourceHierarchy.forEach(source => fs.chmodSync(source.path, source.stats.isDirectory() ? 0o555 : 0o444))
 
       const targetDir = path.join(TEST_DIR, 'target')
-      fse.copySync(sourceDir, targetDir)
+      fs.copySync(sourceDir, targetDir)
 
       // Make sure copy was made and mode was preserved
       assert(fs.existsSync(targetDir))

diff --git a/lib/copy-sync/__tests__/symlink.test.js b/lib/copy-sync/__tests__/symlink.test.js
@@ -1,8 +1,7 @@
 'use strict'
 
-const fs = require('fs')
 const os = require('os')
-const fse = require(process.cwd())
+const fs = require('../../')
 const path = require('path')
 const assert = require('assert')
 const copySync = require('../copy-sync')
@@ -15,14 +14,14 @@ describe('copy-sync / symlink', () => {
   const out = path.join(TEST_DIR, 'out')
 
   beforeEach(done => {
-    fse.emptyDir(TEST_DIR, err => {
+    fs.emptyDir(TEST_DIR, err => {
       assert.ifError(err)
       createFixtures(src, done)
     })
   })
 
   afterEach(done => {
-    fse.remove(TEST_DIR, done)
+    fs.remove(TEST_DIR, done)
   })
 
   it('copies symlinks by default', () => {

diff --git a/lib/copy-sync/copy-sync.js b/lib/copy-sync/copy-sync.js
@@ -22,7 +22,8 @@ function copySync (src, dest, opts) {
     see https://github.com/jprichardson/node-fs-extra/issues/269`)
   }
 
-  const destStat = checkPaths(src, dest)
+  const {srcStat, destStat} = checkPaths(src, dest)
+  checkParentStats(src, srcStat, dest)
 
   if (opts.filter && !opts.filter(src, dest)) return
 
@@ -114,7 +115,7 @@ function copyDir (src, dest, opts) {
 function copyDirItem (item, src, dest, opts) {
   const srcItem = path.join(src, item)
   const destItem = path.join(dest, item)
-  const destStat = checkPaths(srcItem, destItem)
+  const {destStat} = checkPaths(srcItem, destItem)
   return startCopy(destStat, srcItem, destItem, opts)
 }
 
@@ -162,9 +163,9 @@ function copyLink (resolvedSrc, dest) {
 
 // return true if dest is a subdir of src, otherwise false.
 function isSrcSubdir (src, dest) {
-  const srcArray = path.resolve(src).split(path.sep)
-  const destArray = path.resolve(dest).split(path.sep)
-  return srcArray.reduce((acc, current, i) => acc && destArray[i] === current, true)
+  const srcArr = path.resolve(src).split(path.sep).filter(i => i)
+  const destArr = path.resolve(dest).split(path.sep).filter(i => i)
+  return srcArr.reduce((acc, cur, i) => acc && destArr[i] === cur, true)
 }
 
 function checkStats (src, dest) {
@@ -179,6 +180,26 @@ function checkStats (src, dest) {
   return {srcStat, destStat}
 }
 
+// recursively check if dest parent is a subdirectory of src.
+// It works for all file types including symlinks since it
+// checks the src and dest inodes. It starts from the deepest
+// parent and stops once it reaches the src parent or the root path.
+function checkParentStats (src, srcStat, dest) {
+  const destParent = path.dirname(dest)
+  if (destParent && (destParent === path.dirname(src) || destParent === path.parse(destParent).root)) return
+  let destStat
+  try {
+    destStat = fs.statSync(destParent)
+  } catch (err) {
+    if (err.code === 'ENOENT') return
+    throw err
+  }
+  if (destStat.ino && destStat.ino === srcStat.ino) {
+    throw new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`)
+  }
+  return checkParentStats(src, srcStat, destParent)
+}
+
 function checkPaths (src, dest) {
   const {srcStat, destStat} = checkStats(src, dest)
   if (destStat.ino && destStat.ino === srcStat.ino) {
@@ -187,7 +208,7 @@ function checkPaths (src, dest) {
   if (srcStat.isDirectory() && isSrcSubdir(src, dest)) {
     throw new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`)
   }
-  return destStat
+  return {srcStat, destStat}
 }
 
 module.exports = copySync