From e3838c25cf192e2933b22ffc47c12f03c0af8fd1 Mon Sep 17 00:00:00 2001 From: Mani Maghsoudlou Date: Sun, 2 Sep 2018 11:33:05 -0700 Subject: [PATCH] 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 --- .../__tests__/broken-symlink.test.js | 2 +- .../copy-sync-case-insensitive-paths.test.js | 53 ++++++++---------- lib/copy-sync/__tests__/copy-sync-dir.test.js | 4 +- .../__tests__/copy-sync-file.test.js | 2 +- .../__tests__/copy-sync-preserve-time.test.js | 2 +- ...opy-sync-prevent-copying-identical.test.js | 2 +- ...y-sync-prevent-copying-into-itself.test.js | 56 +++++++++++++++++-- .../__tests__/copy-sync-readonly-dir.test.js | 9 ++- lib/copy-sync/__tests__/symlink.test.js | 7 +-- lib/copy-sync/copy-sync.js | 33 +++++++++-- .../copy-case-insensitive-paths.test.js | 45 +++++++-------- lib/copy/__tests__/copy-gh-89.test.js | 2 +- lib/copy/__tests__/copy-preserve-time.test.js | 2 +- .../copy-prevent-copying-identical.test.js | 2 +- .../copy-prevent-copying-into-itself.test.js | 48 ++++++++++++++-- lib/copy/__tests__/copy-readonly-dir.test.js | 2 +- lib/copy/__tests__/copy.test.js | 15 +++-- lib/copy/copy.js | 40 ++++++++++--- 18 files changed, 222 insertions(+), 104 deletions(-) diff --git a/lib/copy-sync/__tests__/broken-symlink.test.js b/lib/copy-sync/__tests__/broken-symlink.test.js index 2744c64e..a5e6d57f 100644 --- 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 index 216c58b2..fcffa53e 100644 --- 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 index 7ce6e981..7a5ecf51 100644 --- 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 index 45ef334d..b1122a23 100644 --- 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 index c190da70..76dbfe09 100644 --- 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 index d9dca5e0..c1f3091d 100644 --- 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 index 487c997a..51f864ac 100644 --- 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 index b5648043..30ace539 100644 --- 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 index 062ea0ef..1825eece 100644 --- 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 index 14ad9939..fd86a67f 100644 --- 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 diff --git a/lib/copy/__tests__/copy-case-insensitive-paths.test.js b/lib/copy/__tests__/copy-case-insensitive-paths.test.js index 9c581324..502616e2 100644 --- a/lib/copy/__tests__/copy-case-insensitive-paths.test.js +++ b/lib/copy/__tests__/copy-case-insensitive-paths.test.js @@ -3,7 +3,10 @@ const assert = require('assert') const os = require('os') const path = require('path') -const fs = require(process.cwd()) +const fs = require('../../') +const platform = os.platform() + +console.log('platform: ' + platform) /* global beforeEach, afterEach, describe, it */ @@ -26,15 +29,13 @@ describe('+ copy() - case insensitive paths', () => { dest = path.join(TEST_DIR, 'srcDir') fs.copy(src, dest, err => { - if (os === 'linux') { + if (platform === 'linux') { assert.ifError(err) assert(fs.existsSync(dest)) assert.strictEqual(fs.readFileSync(path.join(dest, 'subdir', 'file.txt'), 'utf8'), 'some data') } - 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)) } done() }) @@ -48,15 +49,13 @@ describe('+ copy() - case insensitive paths', () => { dest = path.join(TEST_DIR, 'srcFile') fs.copy(src, dest, err => { - if (os === 'linux') { + if (platform === 'linux') { assert.ifError(err) assert(fs.existsSync(dest)) assert.strictEqual(fs.readFileSync(dest, 'utf8'), 'some data') } - 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)) } done() }) @@ -69,20 +68,18 @@ describe('+ copy() - 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') - fs.copy(src, dest, err => { - if (os === 'linux') { + fs.copy(srcLink, dest, err => { + if (platform === 'linux') { assert.ifError(err) 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) } - 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)) } done() }) @@ -93,20 +90,18 @@ describe('+ copy() - 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') - fs.copy(src, dest, err => { - if (os === 'linux') { + fs.copy(srcLink, dest, err => { + if (platform === 'linux') { assert.ifError(err) 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) } - 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)) } done() }) diff --git a/lib/copy/__tests__/copy-gh-89.test.js b/lib/copy/__tests__/copy-gh-89.test.js index 339fc567..537dae8f 100644 --- a/lib/copy/__tests__/copy-gh-89.test.js +++ b/lib/copy/__tests__/copy-gh-89.test.js @@ -5,7 +5,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') diff --git a/lib/copy/__tests__/copy-preserve-time.test.js b/lib/copy/__tests__/copy-preserve-time.test.js index ae83ab4c..8c10692c 100644 --- a/lib/copy/__tests__/copy-preserve-time.test.js +++ b/lib/copy/__tests__/copy-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 copy = require('../copy') diff --git a/lib/copy/__tests__/copy-prevent-copying-identical.test.js b/lib/copy/__tests__/copy-prevent-copying-identical.test.js index b2aa211b..d30ec91b 100644 --- a/lib/copy/__tests__/copy-prevent-copying-identical.test.js +++ b/lib/copy/__tests__/copy-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/__tests__/copy-prevent-copying-into-itself.test.js b/lib/copy/__tests__/copy-prevent-copying-into-itself.test.js index a3b944f7..552cbded 100644 --- a/lib/copy/__tests__/copy-prevent-copying-into-itself.test.js +++ b/lib/copy/__tests__/copy-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 */ @@ -168,6 +168,48 @@ describe('+ copy() - prevent copying into itself', () => { }) }) + it('should error when dest is a subdirectory of src (bind-mounted directory with subdirectory)', done => { + 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)) + fs.copy(src, dest, err => { + assert.strictEqual(err.message, `Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`) + + const srclenAfter = klawSync(src).length + assert.strictEqual(srclenAfter, srclenBefore, 'src length should not change') + + const link = fs.readlinkSync(destLink) + assert.strictEqual(link, src) + done() + }) + }) + + it('should error when dest is a subdirectory of src (more than one level depth)', done => { + 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)) + fs.copy(src, dest, err => { + assert.strictEqual(err.message, `Cannot copy '${src}' to a subdirectory of itself, '${path.join(destLink, 'dir1')}'.`) + + const srclenAfter = klawSync(src).length + assert.strictEqual(srclenAfter, srclenBefore, 'src length should not change') + + const link = fs.readlinkSync(destLink) + assert.strictEqual(link, src) + done() + }) + }) + it('should copy the directory successfully when src is a subdir of resolved dest path', done => { const srcInDest = path.join(TEST_DIR, 'dest', 'some', 'nested', 'src') const destLink = path.join(TEST_DIR, 'dest-symlink') @@ -350,10 +392,6 @@ function testSuccess (src, dest, done) { fs.copy(src, dest, err => { assert.ifError(err) - 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/__tests__/copy-readonly-dir.test.js b/lib/copy/__tests__/copy-readonly-dir.test.js index 99a3d9af..7a21f4aa 100644 --- a/lib/copy/__tests__/copy-readonly-dir.test.js +++ b/lib/copy/__tests__/copy-readonly-dir.test.js @@ -2,7 +2,7 @@ // relevant: https://github.com/jprichardson/node-fs-extra/issues/599 -const fs = require(process.cwd()) +const fs = require('../../') const os = require('os') const fse = require('../../') const path = require('path') diff --git a/lib/copy/__tests__/copy.test.js b/lib/copy/__tests__/copy.test.js index 23d80863..9c1ce7f8 100644 --- a/lib/copy/__tests__/copy.test.js +++ b/lib/copy/__tests__/copy.test.js @@ -144,7 +144,9 @@ describe('fs-extra', () => { const srcTarget = path.join(TEST_DIR, 'destination') fse.mkdirSync(src) fse.mkdirSync(srcTarget) - fse.symlinkSync(srcTarget, path.join(src, 'symlink')) + // symlink type is only used for Windows and the default is 'file'. + // https://nodejs.org/api/fs.html#fs_fs_symlink_target_path_type_callback + fse.symlinkSync(srcTarget, path.join(src, 'symlink'), 'dir') fse.copy(src, dest, err => { assert.ifError(err) @@ -340,11 +342,12 @@ describe('fs-extra', () => { const dest = path.join(TEST_DIR, 'dest') - fse.copySync(src, dest, filter) - - assert(!fs.existsSync(path.join(dest, IGNORE)), 'directory was not ignored') - assert(!fs.existsSync(path.join(dest, IGNORE, 'file')), 'file was not ignored') - done() + fse.copy(src, dest, filter, err => { + assert.ifError(err) + assert(!fs.existsSync(path.join(dest, IGNORE)), 'directory was not ignored') + assert(!fs.existsSync(path.join(dest, IGNORE, 'file')), 'file was not ignored') + done() + }) }) it('should apply filter when it is applied only to dest', done => { diff --git a/lib/copy/copy.js b/lib/copy/copy.js index 3dfbc540..5890f7ce 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -28,10 +28,14 @@ function copy (src, dest, opts, cb) { see https://github.com/jprichardson/node-fs-extra/issues/269`) } - checkPaths(src, dest, (err, destStat) => { + checkPaths(src, dest, (err, stats) => { if (err) return cb(err) - if (opts.filter) return handleFilter(checkParentDir, destStat, src, dest, opts, cb) - return checkParentDir(destStat, src, dest, opts, cb) + const {srcStat, destStat} = stats + checkParentStats(src, srcStat, dest, err => { + if (err) return cb(err) + if (opts.filter) return handleFilter(checkParentDir, destStat, src, dest, opts, cb) + return checkParentDir(destStat, src, dest, opts, cb) + }) }) } @@ -155,8 +159,9 @@ function copyDirItems (items, src, dest, opts, cb) { function copyDirItem (items, item, src, dest, opts, cb) { const srcItem = path.join(src, item) const destItem = path.join(dest, item) - checkPaths(srcItem, destItem, (err, destStat) => { + checkPaths(srcItem, destItem, (err, stats) => { if (err) return cb(err) + const {destStat} = stats startCopy(destStat, srcItem, destItem, opts, err => { if (err) return cb(err) return copyDirItems(items, src, dest, opts, cb) @@ -211,9 +216,9 @@ function copyLink (resolvedSrc, dest, cb) { // 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, cb) { @@ -229,6 +234,25 @@ function checkStats (src, dest, cb) { }) } +// 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, cb) { + const destParent = path.dirname(dest) + if (destParent && (destParent === path.dirname(src) || destParent === path.parse(destParent).root)) return cb() + fs.stat(destParent, (err, destStat) => { + if (err) { + if (err.code === 'ENOENT') return cb() + return cb(err) + } + if (destStat.ino && destStat.ino === srcStat.ino) { + return cb(new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`)) + } + return checkParentStats(src, srcStat, destParent, cb) + }) +} + function checkPaths (src, dest, cb) { checkStats(src, dest, (err, stats) => { if (err) return cb(err) @@ -239,7 +263,7 @@ function checkPaths (src, dest, cb) { if (srcStat.isDirectory() && isSrcSubdir(src, dest)) { return cb(new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`)) } - return cb(null, destStat) + return cb(null, {srcStat, destStat}) }) }