Repository: jprichardson/node-fs-extra
Pull request #458 (2017-07-10 04:55:42)

Prevent move into itself
Fixed the issue with `move()` when dest is a subdirectory of src. Also, removed `shouldMkdirp` option as we discussed before.

To quickly remind the issue, with existing behavior, running something like this
```js
const src = 'src'
const dest = 'src/dest'
fs.move(src, dest, err => {
  if (err) return console.log(err)
  console.log('done.')
})
```
results in just removing `src` and essentially losing all `src` contents.

I believe we should prevent this from happening. Any feedback appreciated!


Patch:
diff --git a/lib/move/index.js b/lib/move/index.js
index e5eb2c1..eeeb30f 100644
--- a/lib/move/index.js
+++ b/lib/move/index.js
@@ -13,40 +13,35 @@ const path = require('path')
 const remove = require('../remove').remove
 const mkdirp = require('../mkdirs').mkdirs
 
-function move (source, dest, options, callback) {
+function move (src, dest, options, callback) {
   if (typeof options === 'function') {
     callback = options
     options = {}
   }
 
-  const shouldMkdirp = ('mkdirp' in options) ? options.mkdirp : true
   const overwrite = options.overwrite || options.clobber || false
 
-  if (shouldMkdirp) {
-    mkdirs()
-  } else {
-    doRename()
-  }
-
-  function mkdirs () {
+  isSrcSubdir(src, dest, (err, itIs) => {
+    if (err) return callback(err)
+    if (itIs) return callback(new Error(`Cannot move '${src}' to a subdirectory of itself, '${dest}'.`))
     mkdirp(path.dirname(dest), err => {
       if (err) return callback(err)
       doRename()
     })
-  }
+  })
 
   function doRename () {
-    if (path.resolve(source) === path.resolve(dest)) {
-      fs.access(source, callback)
+    if (path.resolve(src) === path.resolve(dest)) {
+      fs.access(src, callback)
     } else if (overwrite) {
-      fs.rename(source, dest, err => {
+      fs.rename(src, dest, err => {
         if (!err) return callback()
 
         if (err.code === 'ENOTEMPTY' || err.code === 'EEXIST') {
           remove(dest, err => {
             if (err) return callback(err)
             options.overwrite = false // just overwriteed it, no need to do it again
-            move(source, dest, options, callback)
+            move(src, dest, options, callback)
           })
           return
         }
@@ -57,49 +52,44 @@ function move (source, dest, options, callback) {
             remove(dest, err => {
               if (err) return callback(err)
               options.overwrite = false
-              move(source, dest, options, callback)
+              move(src, dest, options, callback)
             })
           }, 200)
           return
         }
 
         if (err.code !== 'EXDEV') return callback(err)
-        moveAcrossDevice(source, dest, overwrite, callback)
+        moveAcrossDevice(src, dest, overwrite, callback)
       })
     } else {
-      fs.link(source, dest, err => {
+      fs.link(src, dest, err => {
         if (err) {
           if (err.code === 'EXDEV' || err.code === 'EISDIR' || err.code === 'EPERM' || err.code === 'ENOTSUP') {
-            moveAcrossDevice(source, dest, overwrite, callback)
-            return
+            return moveAcrossDevice(src, dest, overwrite, callback)
           }
-          callback(err)
-          return
+          return callback(err)
         }
-        fs.unlink(source, callback)
+        return fs.unlink(src, callback)
       })
     }
   }
 }
 
-function moveAcrossDevice (source, dest, overwrite, callback) {
-  fs.stat(source, (err, stat) => {
-    if (err) {
-      callback(err)
-      return
-    }
+function moveAcrossDevice (src, dest, overwrite, callback) {
+  fs.stat(src, (err, stat) => {
+    if (err) return callback(err)
 
     if (stat.isDirectory()) {
-      moveDirAcrossDevice(source, dest, overwrite, callback)
+      moveDirAcrossDevice(src, dest, overwrite, callback)
     } else {
-      moveFileAcrossDevice(source, dest, overwrite, callback)
+      moveFileAcrossDevice(src, dest, overwrite, callback)
     }
   })
 }
 
-function moveFileAcrossDevice (source, dest, overwrite, callback) {
+function moveFileAcrossDevice (src, dest, overwrite, callback) {
   const flags = overwrite ? 'w' : 'wx'
-  const ins = fs.createReadStream(source)
+  const ins = fs.createReadStream(src)
   const outs = fs.createWriteStream(dest, { flags })
 
   ins.on('error', err => {
@@ -113,7 +103,7 @@ function moveFileAcrossDevice (source, dest, overwrite, callback) {
     fs.unlink(dest, () => {
       // note: `err` here is from the input stream errror
       if (err.code === 'EISDIR' || err.code === 'EPERM') {
-        moveDirAcrossDevice(source, dest, overwrite, callback)
+        moveDirAcrossDevice(src, dest, overwrite, callback)
       } else {
         callback(err)
       }
@@ -131,11 +121,11 @@ function moveFileAcrossDevice (source, dest, overwrite, callback) {
   ins.pipe(outs)
 
   function onClose () {
-    fs.unlink(source, callback)
+    fs.unlink(src, callback)
   }
 }
 
-function moveDirAcrossDevice (source, dest, overwrite, callback) {
+function moveDirAcrossDevice (src, dest, overwrite, callback) {
   const options = {
     overwrite: false
   }
@@ -150,13 +140,31 @@ function moveDirAcrossDevice (source, dest, overwrite, callback) {
   }
 
   function startNcp () {
-    ncp(source, dest, options, err => {
+    ncp(src, dest, options, err => {
       if (err) return callback(err)
-      remove(source, callback)
+      remove(src, callback)
     })
   }
 }
 
+// return true if dest is a subdir of src, otherwise false.
+// extract dest base dir and check if that is the same as src basename
+function isSrcSubdir (src, dest, cb) {
+  fs.stat(src, (err, st) => {
+    if (err) return cb(err)
+    if (st.isDirectory()) {
+      const baseDir = dest.split(path.dirname(src) + path.sep)[1]
+      if (baseDir) {
+        const destBasename = baseDir.split(path.sep)[0]
+        if (destBasename) return cb(null, src !== dest && dest.indexOf(src) > -1 && destBasename === path.basename(src))
+        return cb(null, false)
+      }
+      return cb(null, false)
+    }
+    return cb(null, false)
+  })
+}
+
 module.exports = {
   move: u(move)
 }


Test patch:
diff --git a/lib/move/__tests__/move-prevent-moving-into-itself.test.js b/lib/move/__tests__/move-prevent-moving-into-itself.test.js
new file mode 100644
index 0000000..4b98fc6
--- /dev/null
+++ b/lib/move/__tests__/move-prevent-moving-into-itself.test.js
@@ -0,0 +1,192 @@
+'use strict'
+
+const assert = require('assert')
+const os = require('os')
+const path = require('path')
+const fs = require(process.cwd())
+const klawSync = require('klaw-sync')
+
+/* global beforeEach, afterEach, describe, it */
+
+const FILES = [
+  'file0.txt',
+  path.join('dir1', 'file1.txt'),
+  path.join('dir1', 'dir2', 'file2.txt'),
+  path.join('dir1', 'dir2', 'dir3', 'file3.txt')
+]
+
+const dat0 = 'file0'
+const dat1 = 'file1'
+const dat2 = 'file2'
+const dat3 = 'file3'
+
+describe('+ move() - prevent moving into itself', () => {
+  let TEST_DIR, src, dest
+
+  beforeEach(() => {
+    TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'move-prevent-moving-into-itself')
+    src = path.join(TEST_DIR, 'src')
+    fs.mkdirpSync(src)
+
+    fs.outputFileSync(path.join(src, FILES[0]), dat0)
+    fs.outputFileSync(path.join(src, FILES[1]), dat1)
+    fs.outputFileSync(path.join(src, FILES[2]), dat2)
+    fs.outputFileSync(path.join(src, FILES[3]), dat3)
+  })
+
+  afterEach(() => fs.removeSync(TEST_DIR))
+
+  describe('> when source is a file', () => {
+    it(`should move the file successfully even when dest parent is 'src/dest'`, done => {
+      const destFile = path.join(TEST_DIR, 'src', 'dest', 'destfile.txt')
+      return testSuccessFile(src, destFile, done)
+    })
+
+    it(`should move the file successfully when dest parent is 'src/src_dest'`, done => {
+      const destFile = path.join(TEST_DIR, 'src', 'src_dest', 'destfile.txt')
+      return testSuccessFile(src, destFile, done)
+    })
+
+    it(`should move the file successfully when dest parent is 'src/dest_src'`, done => {
+      const destFile = path.join(TEST_DIR, 'src', 'dest_src', 'destfile.txt')
+      return testSuccessFile(src, destFile, done)
+    })
+
+    it(`should move the file successfully when dest parent is 'src/dest/src'`, done => {
+      const destFile = path.join(TEST_DIR, 'src', 'dest', 'src', 'destfile.txt')
+      return testSuccessFile(src, destFile, done)
+    })
+
+    it(`should move the file successfully when dest parent is 'srcsrc/dest'`, done => {
+      const destFile = path.join(TEST_DIR, 'srcsrc', 'dest', 'destfile.txt')
+      return testSuccessFile(src, destFile, done)
+    })
+  })
+
+  describe('> when source is a directory', () => {
+    it(`should move the directory successfully when dest is 'src_dest'`, done => {
+      dest = path.join(TEST_DIR, 'src_dest')
+      return testSuccessDir(src, dest, done)
+    })
+
+    it(`should move the directory successfully when dest is 'src-dest'`, done => {
+      dest = path.join(TEST_DIR, 'src-dest')
+      return testSuccessDir(src, dest, done)
+    })
+
+    it(`should move the directory successfully when dest is 'dest_src'`, done => {
+      dest = path.join(TEST_DIR, 'dest_src')
+      return testSuccessDir(src, dest, done)
+    })
+
+    it(`should move the directory successfully when dest is 'src_dest/src'`, done => {
+      dest = path.join(TEST_DIR, 'src_dest', 'src')
+      return testSuccessDir(src, dest, done)
+    })
+
+    it(`should move the directory successfully when dest is 'src-dest/src'`, done => {
+      dest = path.join(TEST_DIR, 'src-dest', 'src')
+      return testSuccessDir(src, dest, done)
+    })
+
+    it(`should move the directory successfully when dest is 'dest_src/src'`, done => {
+      dest = path.join(TEST_DIR, 'dest_src', 'src')
+      return testSuccessDir(src, dest, done)
+    })
+
+    it(`should move the directory successfully when dest is 'src_src/dest'`, done => {
+      dest = path.join(TEST_DIR, 'src_src', 'dest')
+      return testSuccessDir(src, dest, done)
+    })
+
+    it(`should move the directory successfully when dest is 'src-src/dest'`, done => {
+      dest = path.join(TEST_DIR, 'src-src', 'dest')
+      return testSuccessDir(src, dest, done)
+    })
+
+    it(`should move the directory successfully when dest is 'srcsrc/dest'`, done => {
+      dest = path.join(TEST_DIR, 'srcsrc', 'dest')
+      return testSuccessDir(src, dest, done)
+    })
+
+    it(`should move the directory successfully when dest is 'dest/src'`, done => {
+      dest = path.join(TEST_DIR, 'dest', 'src')
+      return testSuccessDir(src, dest, done)
+    })
+
+    it('should move the directory successfully when dest is very nested that all its parents need to be created', done => {
+      dest = path.join(TEST_DIR, 'dest', 'src', 'foo', 'bar', 'baz', 'qux', 'quux', 'waldo',
+        'grault', 'garply', 'fred', 'plugh', 'thud', 'some', 'highly', 'deeply',
+        'badly', 'nasty', 'crazy', 'mad', 'nested', 'dest')
+      assert(!fs.existsSync(dest))
+      return testSuccessDir(src, dest, done)
+    })
+
+    it(`should return error when dest is 'src/dest'`, done => {
+      dest = path.join(TEST_DIR, 'src', 'dest')
+      return testError(src, dest, done)
+    })
+
+    it(`should return error when dest is 'src/src_dest'`, done => {
+      dest = path.join(TEST_DIR, 'src', 'src_dest')
+      return testError(src, dest, done)
+    })
+
+    it(`should return error when dest is 'src/dest_src'`, done => {
+      dest = path.join(TEST_DIR, 'src', 'dest_src')
+      return testError(src, dest, done)
+    })
+
+    it(`should return error when dest is 'src/dest/src'`, done => {
+      dest = path.join(TEST_DIR, 'src', 'dest', 'src')
+      return testError(src, dest, done)
+    })
+  })
+})
+
+function testSuccessFile (src, destFile, done) {
+  const srcFile = path.join(src, FILES[0])
+
+  fs.move(srcFile, destFile, err => {
+    assert.ifError(err)
+    const f0 = fs.readFileSync(destFile, 'utf8')
+    assert.strictEqual(f0, dat0, 'file contents matched')
+    assert(!fs.existsSync(srcFile))
+    return done()
+  })
+}
+
+function testSuccessDir (src, dest, done) {
+  const srclen = klawSync(src).length
+
+  assert(srclen > 2) // assert src has contents
+
+  fs.move(src, dest, err => {
+    assert.ifError(err)
+    const destlen = klawSync(dest).length
+
+    assert.strictEqual(destlen, srclen, 'src and dest length should be equal')
+
+    const f0 = fs.readFileSync(path.join(dest, FILES[0]), 'utf8')
+    const f1 = fs.readFileSync(path.join(dest, FILES[1]), 'utf8')
+    const f2 = fs.readFileSync(path.join(dest, FILES[2]), 'utf8')
+    const f3 = fs.readFileSync(path.join(dest, FILES[3]), 'utf8')
+
+    assert.strictEqual(f0, dat0, 'file contents matched')
+    assert.strictEqual(f1, dat1, 'file contents matched')
+    assert.strictEqual(f2, dat2, 'file contents matched')
+    assert.strictEqual(f3, dat3, 'file contents matched')
+    assert(!fs.existsSync(src))
+    return done()
+  })
+}
+
+function testError (src, dest, done) {
+  fs.move(src, dest, err => {
+    assert(err)
+    assert.strictEqual(err.message, `Cannot move '${src}' to a subdirectory of itself, '${dest}'.`)
+    assert(fs.existsSync(src))
+    assert(!fs.existsSync(dest))
+    return done()
+  })
+}
diff --git a/lib/move/__tests__/move.test.js b/lib/move/__tests__/move.test.js
index 988a75c..342a8d2 100644
--- a/lib/move/__tests__/move.test.js
+++ b/lib/move/__tests__/move.test.js
@@ -173,19 +173,6 @@ describe('move', () => {
     })
   })
 
-  it('should not create directory structure if mkdirp is false', done => {
-    const src = `${TEST_DIR}/a-file`
-    const dest = `${TEST_DIR}/does/not/exist/a-file-dest`
-
-    // verify dest directory does not exist
-    assert(!fs.existsSync(path.dirname(dest)))
-
-    fse.move(src, dest, {mkdirp: false}, err => {
-      assert.strictEqual(err.code, 'ENOENT')
-      done()
-    })
-  })
-
   it('should create directory structure by default', done => {
     const src = `${TEST_DIR}/a-file`
     const dest = `${TEST_DIR}/does/not/exist/a-file-dest`
@@ -331,7 +318,7 @@ describe('move', () => {
     })
   })
 
-  describe.skip('> when trying to move a folder into itself', () => {
+  describe('> when trying to move a folder into itself', () => {
     it('should produce an error', done => {
       const SRC_DIR = path.join(TEST_DIR, 'test')
       const DEST_DIR = path.join(TEST_DIR, 'test', 'test')
@@ -342,7 +329,7 @@ describe('move', () => {
 
       fse.move(SRC_DIR, DEST_DIR, err => {
         assert(fs.existsSync(SRC_DIR))
-        assert(err)
+        assert.equal(err.message, `Cannot move '${SRC_DIR}' to a subdirectory of itself, '${DEST_DIR}'.`)
         done()
       })
     })