From 4b4f3cf0b7533e1118048a7c746037e632592b93 Mon Sep 17 00:00:00 2001 From: Mani Maghsoudlou Date: Tue, 15 May 2018 21:54:26 -0700 Subject: [PATCH 1/2] copy*(): use ino to check identical paths --- .../__tests__/broken-symlink.test.js | 5 +- lib/copy-sync/__tests__/copy-sync-dir.test.js | 21 ++- .../__tests__/copy-sync-file.test.js | 4 +- ...opy-sync-prevent-copying-identical.test.js | 98 ++++++++++- ...y-sync-prevent-copying-into-itself.test.js | 48 +++--- lib/copy-sync/copy-sync.js | 115 +++++-------- .../copy-prevent-copying-identical.test.js | 92 ++++++++-- .../copy-prevent-copying-into-itself.test.js | 19 +-- lib/copy/__tests__/copy.test.js | 18 ++ lib/copy/__tests__/ncp/broken-symlink.test.js | 5 +- lib/copy/copy.js | 161 ++++++++---------- lib/json/__tests__/output-json-sync.test.js | 2 +- lib/json/__tests__/output-json.test.js | 2 +- 13 files changed, 350 insertions(+), 240 deletions(-) diff --git a/lib/copy-sync/__tests__/broken-symlink.test.js b/lib/copy-sync/__tests__/broken-symlink.test.js index 3def54b6..2744c64e 100644 --- a/lib/copy-sync/__tests__/broken-symlink.test.js +++ b/lib/copy-sync/__tests__/broken-symlink.test.js @@ -23,9 +23,8 @@ describe('copy-sync / broken symlink', () => { afterEach(done => fse.remove(TEST_DIR, done)) - it('should copy broken symlinks by default', () => { - assert.doesNotThrow(() => copySync(src, out)) - assert.strictEqual(fs.readlinkSync(path.join(out, 'broken-symlink')), path.join(src, 'does-not-exist')) + it('should error if symlink is broken', () => { + assert.throws(() => copySync(src, out)) }) it('should throw an error when dereference=true', () => { diff --git a/lib/copy-sync/__tests__/copy-sync-dir.test.js b/lib/copy-sync/__tests__/copy-sync-dir.test.js index 9c326736..7ce6e981 100644 --- a/lib/copy-sync/__tests__/copy-sync-dir.test.js +++ b/lib/copy-sync/__tests__/copy-sync-dir.test.js @@ -6,9 +6,9 @@ const path = require('path') const assert = require('assert') const crypto = require('crypto') -/* global beforeEach, describe, it */ +/* global beforeEach, afterEach, describe, it */ -describe('+ copySync()', () => { +describe('+ copySync() / dir', () => { const SIZE = 16 * 64 * 1024 + 7 let TEST_DIR let src, dest @@ -20,6 +20,8 @@ describe('+ copySync()', () => { fs.emptyDir(TEST_DIR, done) }) + afterEach(done => fs.remove(TEST_DIR, done)) + describe('> when src is a directory', () => { describe('> when dest exists and is a file', () => { it('should throw error', () => { @@ -70,13 +72,15 @@ describe('+ copySync()', () => { }) it('should preserve symbolic links', () => { + const srcTarget = path.join(TEST_DIR, 'destination') fs.mkdirSync(src) - fs.symlinkSync('destination', path.join(src, 'symlink')) + fs.mkdirSync(srcTarget) + fs.symlinkSync(srcTarget, path.join(src, 'symlink')) fs.copySync(src, dest) const link = fs.readlinkSync(path.join(dest, 'symlink')) - assert.strictEqual(link, 'destination') + assert.strictEqual(link, srcTarget) }) describe('> when the destination dir does not exist', () => { @@ -183,14 +187,9 @@ describe('+ copySync()', () => { const dest = path.join(TEST_DIR, 'dest') setTimeout(() => { - fs.mkdirSync(src) - fs.writeFileSync(path.join(src, 'somefile.html'), 'some data') + fs.outputFileSync(path.join(src, 'somefile.html'), 'some data') fs.mkdirSync(dest) - try { - fs.copySync(src, dest, filter) - } catch (err) { - assert.ifError(err) - } + fs.copySync(src, dest, filter) assert(!fs.existsSync(path.join(dest, 'somefile.html'))) done() }, 1000) diff --git a/lib/copy-sync/__tests__/copy-sync-file.test.js b/lib/copy-sync/__tests__/copy-sync-file.test.js index fd58c1c7..45ef334d 100644 --- a/lib/copy-sync/__tests__/copy-sync-file.test.js +++ b/lib/copy-sync/__tests__/copy-sync-file.test.js @@ -10,11 +10,11 @@ const crypto = require('crypto') const SIZE = 16 * 64 * 1024 + 7 -describe('+ copySync()', () => { +describe('+ copySync() / file', () => { let TEST_DIR beforeEach(done => { - TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-sync') + TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-sync-file') fs.emptyDir(TEST_DIR, done) }) 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 056ca8ab..d9dca5e0 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 @@ -23,6 +23,8 @@ describe('+ copySync() - prevent copying identical files and dirs', () => { it('should return an error if src and dest are the same', () => { const fileSrc = path.join(TEST_DIR, 'TEST_fs-extra_copy_sync') const fileDest = path.join(TEST_DIR, 'TEST_fs-extra_copy_sync') + fs.ensureFileSync(fileSrc) + try { fs.copySync(fileSrc, fileDest) } catch (err) { @@ -30,6 +32,80 @@ describe('+ copySync() - prevent copying identical files and dirs', () => { } }) + describe('dest with parent symlink', () => { + describe('first parent is symlink', () => { + it('should error when src is file', () => { + const src = path.join(TEST_DIR, 'a', 'file.txt') + const dest = path.join(TEST_DIR, 'b', 'file.txt') + const srcParent = path.join(TEST_DIR, 'a') + const destParent = path.join(TEST_DIR, 'b') + fs.ensureFileSync(src) + fs.ensureSymlinkSync(srcParent, destParent, 'dir') + + try { + fs.copySync(src, dest) + } catch (err) { + assert.strictEqual(err.message, 'Source and destination must not be the same.') + } finally { + assert(fs.existsSync(src)) + } + }) + + it('should error when src is directory', () => { + const src = path.join(TEST_DIR, 'a', 'foo') + const dest = path.join(TEST_DIR, 'b', 'foo') + const srcParent = path.join(TEST_DIR, 'a') + const destParent = path.join(TEST_DIR, 'b') + fs.ensureDirSync(src) + fs.ensureSymlinkSync(srcParent, destParent, 'dir') + + try { + fs.copySync(src, dest) + } catch (err) { + assert.strictEqual(err.message, 'Source and destination must not be the same.') + } finally { + assert(fs.existsSync(src)) + } + }) + }) + + describe('nested dest', () => { + it('should error when src is file', () => { + const src = path.join(TEST_DIR, 'a', 'dir', 'file.txt') + const dest = path.join(TEST_DIR, 'b', 'dir', 'file.txt') + const srcParent = path.join(TEST_DIR, 'a') + const destParent = path.join(TEST_DIR, 'b') + fs.ensureFileSync(src) + fs.ensureSymlinkSync(srcParent, destParent, 'dir') + + try { + fs.copySync(src, dest) + } catch (err) { + assert.strictEqual(err.message, 'Source and destination must not be the same.') + } finally { + assert(fs.existsSync(src)) + } + }) + + it('should error when src is directory', () => { + const src = path.join(TEST_DIR, 'a', 'dir', 'foo') + const dest = path.join(TEST_DIR, 'b', 'dir', 'foo') + const srcParent = path.join(TEST_DIR, 'a') + const destParent = path.join(TEST_DIR, 'b') + fs.ensureDirSync(src) + fs.ensureSymlinkSync(srcParent, destParent, 'dir') + + try { + fs.copySync(src, dest) + } catch (err) { + assert.strictEqual(err.message, 'Source and destination must not be the same.') + } finally { + assert(fs.existsSync(src)) + } + }) + }) + }) + // src is directory: // src is regular, dest is symlink // src is symlink, dest is regular @@ -90,7 +166,7 @@ describe('+ copySync() - prevent copying identical files and dirs', () => { }) describe('>> when src and dest are symlinks that point to the exact same path', () => { - it('should not copy and return', () => { + it('should error src and dest are the same', () => { src = path.join(TEST_DIR, 'src') fs.mkdirsSync(src) const srcLink = path.join(TEST_DIR, 'src_symlink') @@ -101,7 +177,11 @@ describe('+ copySync() - prevent copying identical files and dirs', () => { const srclenBefore = klawSync(srcLink).length const destlenBefore = klawSync(destLink).length - fs.copySync(srcLink, destLink) + try { + fs.copySync(srcLink, destLink) + } catch (err) { + assert.strictEqual(err.message, 'Source and destination must not be the same.') + } const srclenAfter = klawSync(srcLink).length assert.strictEqual(srclenAfter, srclenBefore, 'src length should not change') @@ -146,8 +226,7 @@ describe('+ copySync() - prevent copying identical files and dirs', () => { describe(`>> when src is a symlink that points to a regular dest`, () => { it('should throw error', () => { dest = path.join(TEST_DIR, 'dest', 'somefile.txt') - fs.ensureFileSync(dest) - fs.writeFileSync(dest, 'some data') + fs.outputFileSync(dest, 'some data') const srcLink = path.join(TEST_DIR, 'src-symlink') fs.symlinkSync(dest, srcLink, 'file') @@ -164,10 +243,9 @@ describe('+ copySync() - prevent copying identical files and dirs', () => { }) describe('>> when src and dest are symlinks that point to the exact same path', () => { - it('should not copy and return', () => { + it('should error src and dest are the same', () => { src = path.join(TEST_DIR, 'src', 'srcfile.txt') - fs.ensureFileSync(src) - fs.writeFileSync(src, 'src data') + fs.outputFileSync(src, 'src data') const srcLink = path.join(TEST_DIR, 'src_symlink') fs.symlinkSync(src, srcLink, 'file') @@ -175,7 +253,11 @@ describe('+ copySync() - prevent copying identical files and dirs', () => { const destLink = path.join(TEST_DIR, 'dest_symlink') fs.symlinkSync(src, destLink, 'file') - fs.copySync(srcLink, destLink) + try { + fs.copySync(srcLink, destLink) + } catch (err) { + assert.strictEqual(err.message, 'Source and destination must not be the same.') + } const srcln = fs.readlinkSync(srcLink) assert.strictEqual(srcln, src) 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 e5178044..487c997a 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 @@ -39,7 +39,7 @@ describe('+ copySync() - prevent copying into itself', () => { afterEach(done => fs.remove(TEST_DIR, done)) describe('> when source is a file', () => { - it('should copy the file successfully even when dest parent is a subdir of src', () => { + it('should copy the file successfully even if dest parent is a subdir of src', () => { const srcFile = path.join(TEST_DIR, 'src', 'srcfile.txt') const destFile = path.join(TEST_DIR, 'src', 'dest', 'destfile.txt') fs.writeFileSync(srcFile, dat0) @@ -273,7 +273,7 @@ describe('+ copySync() - prevent copying into itself', () => { }) describe('>> when dest is a symlink', () => { - it('should silently return when resolved dest path is exactly the same as resolved src path', () => { + it('should error when resolved dest path is exactly the same as resolved src path', () => { const srcLink = path.join(TEST_DIR, 'src-symlink') fs.symlinkSync(src, srcLink, 'dir') const destLink = path.join(TEST_DIR, 'dest-symlink') @@ -284,16 +284,21 @@ describe('+ copySync() - prevent copying into itself', () => { assert(srclenBefore > 2) assert(destlenBefore > 2) - fs.copySync(srcLink, destLink) - const srclenAfter = klawSync(srcLink).length - assert.strictEqual(srclenAfter, srclenBefore, 'src length should not change') - const destlenAfter = klawSync(destLink).length - assert.strictEqual(destlenAfter, destlenBefore, 'dest length should not change') - - const srcln = fs.readlinkSync(srcLink) - assert.strictEqual(srcln, src) - const destln = fs.readlinkSync(destLink) - assert.strictEqual(destln, src) + try { + fs.copySync(srcLink, destLink) + } catch (err) { + assert.strictEqual(err.message, 'Source and destination must not be the same.') + } finally { + const srclenAfter = klawSync(srcLink).length + assert.strictEqual(srclenAfter, srclenBefore, 'src length should not change') + const destlenAfter = klawSync(destLink).length + assert.strictEqual(destlenAfter, destlenBefore, 'dest length should not change') + + const srcln = fs.readlinkSync(srcLink) + assert.strictEqual(srcln, src) + const destln = fs.readlinkSync(destLink) + assert.strictEqual(destln, src) + } }) it('should error when resolved dest path is a subdir of resolved src path', () => { @@ -309,10 +314,11 @@ describe('+ copySync() - prevent copying into itself', () => { try { fs.copySync(srcLink, destLink) } catch (err) { - assert(err) + assert.strictEqual(err.message, `Cannot copy '${src}' to a subdirectory of itself, '${resolvedDestPath}'.`) + } finally { + const destln = fs.readlinkSync(destLink) + assert.strictEqual(destln, resolvedDestPath) } - const destln = fs.readlinkSync(destLink) - assert.strictEqual(destln, src) }) it('should error when resolved src path is a subdir of resolved dest path', () => { @@ -322,18 +328,18 @@ describe('+ copySync() - prevent copying into itself', () => { const dest = path.join(TEST_DIR, 'dest') - fs.mkdirSync(dest) - - fs.symlinkSync(srcInDest, srcLink, 'dir') - fs.symlinkSync(dest, destLink, 'dir') + fs.ensureDirSync(srcInDest) + fs.ensureSymlinkSync(srcInDest, srcLink, 'dir') + fs.ensureSymlinkSync(dest, destLink, 'dir') try { fs.copySync(srcLink, destLink) } catch (err) { assert.strictEqual(err.message, `Cannot overwrite '${dest}' with '${srcInDest}'.`) + } finally { + const destln = fs.readlinkSync(destLink) + assert.strictEqual(destln, dest) } - const destln = fs.readlinkSync(destLink) - assert.strictEqual(destln, dest) }) }) }) diff --git a/lib/copy-sync/copy-sync.js b/lib/copy-sync/copy-sync.js index 201cbf24..3985b6bc 100644 --- a/lib/copy-sync/copy-sync.js +++ b/lib/copy-sync/copy-sync.js @@ -6,7 +6,6 @@ const mkdirpSync = require('../mkdirs').mkdirsSync const utimesSync = require('../util/utimes.js').utimesMillisSync const notExist = Symbol('notExist') -const existsReg = Symbol('existsReg') function copySync (src, dest, opts) { if (typeof opts === 'function') { @@ -23,34 +22,33 @@ function copySync (src, dest, opts) { see https://github.com/jprichardson/node-fs-extra/issues/269`) } - const resolvedDest = checkPaths(src, dest) + const destStat = checkPaths(src, dest) if (opts.filter && !opts.filter(src, dest)) return const destParent = path.dirname(dest) if (!fs.existsSync(destParent)) mkdirpSync(destParent) - return startCopy(resolvedDest, src, dest, opts) + return startCopy(destStat, src, dest, opts) } -function startCopy (resolvedDest, src, dest, opts) { +function startCopy (destStat, src, dest, opts) { if (opts.filter && !opts.filter(src, dest)) return - return getStats(resolvedDest, src, dest, opts) + return getStats(destStat, src, dest, opts) } -function getStats (resolvedDest, src, dest, opts) { +function getStats (destStat, src, dest, opts) { const statSync = opts.dereference ? fs.statSync : fs.lstatSync - const st = statSync(src) + const srcStat = statSync(src) - if (st.isDirectory()) return onDir(st, resolvedDest, src, dest, opts) - else if (st.isFile() || - st.isCharacterDevice() || - st.isBlockDevice()) return onFile(st, resolvedDest, src, dest, opts) - else if (st.isSymbolicLink()) return onLink(resolvedDest, src, dest, opts) + if (srcStat.isDirectory()) return onDir(srcStat, destStat, src, dest, opts) + else if (srcStat.isFile() || + srcStat.isCharacterDevice() || + srcStat.isBlockDevice()) return onFile(srcStat, destStat, src, dest, opts) + else if (srcStat.isSymbolicLink()) return onLink(destStat, src, dest, opts) } -function onFile (srcStat, resolvedDest, src, dest, opts) { - if (resolvedDest === notExist) return copyFile(srcStat, src, dest, opts) - else if (resolvedDest === existsReg) return mayCopyFile(srcStat, src, dest, opts) +function onFile (srcStat, destStat, src, dest, opts) { + if (destStat === notExist) return copyFile(srcStat, src, dest, opts) return mayCopyFile(srcStat, src, dest, opts) } @@ -95,23 +93,9 @@ function copyFileFallback (srcStat, src, dest, opts) { fs.closeSync(fdw) } -function onDir (srcStat, resolvedDest, src, dest, opts) { - if (resolvedDest === notExist) { - if (isSrcSubdir(src, dest)) { - throw new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`) - } - return mkDirAndCopy(srcStat, src, dest, opts) - } else if (resolvedDest === existsReg) { - if (isSrcSubdir(src, dest)) { - throw new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`) - } - return mayCopyDir(src, dest, opts) - } - return copyDir(src, dest, opts) -} - -function mayCopyDir (src, dest, opts) { - if (!fs.statSync(dest).isDirectory()) { +function onDir (srcStat, destStat, src, dest, opts) { + if (destStat === notExist) return mkDirAndCopy(srcStat, src, dest, opts) + if (destStat && !destStat.isDirectory()) { throw new Error(`Cannot overwrite non-directory '${dest}' with directory '${src}'.`) } return copyDir(src, dest, opts) @@ -130,26 +114,36 @@ function copyDir (src, dest, opts) { function copyDirItem (item, src, dest, opts) { const srcItem = path.join(src, item) const destItem = path.join(dest, item) - const resolvedDest = checkPaths(srcItem, destItem) - return startCopy(resolvedDest, srcItem, destItem, opts) + const destStat = checkPaths(srcItem, destItem) + return startCopy(destStat, srcItem, destItem, opts) } -function onLink (resolvedDest, src, dest, opts) { +function onLink (destStat, src, dest, opts) { let resolvedSrc = fs.readlinkSync(src) if (opts.dereference) { resolvedSrc = path.resolve(process.cwd(), resolvedSrc) } - if (resolvedDest === notExist || resolvedDest === existsReg) { - // if dest already exists, fs throws error anyway, - // so no need to guard against it here. + if (destStat === notExist) { return fs.symlinkSync(resolvedSrc, dest) } else { + let resolvedDest + try { + resolvedDest = fs.readlinkSync(dest) + } catch (err) { + // dest exists and is a regular file or directory, + // Windows may throw UNKNOWN error. If dest already exists, + // fs throws error anyway, so no need to guard against it here. + if (err.code === 'EINVAL' || err.code === 'UNKNOWN') return fs.symlinkSync(resolvedSrc, dest) + throw err + } if (opts.dereference) { resolvedDest = path.resolve(process.cwd(), resolvedDest) } - if (pathsAreIdentical(resolvedSrc, resolvedDest)) return + if (isSrcSubdir(resolvedSrc, resolvedDest)) { + throw new Error(`Cannot copy '${resolvedSrc}' to a subdirectory of itself, '${resolvedDest}'.`) + } // prevent copy if src is a subdir of dest since unlinking // dest in this case would result in removing src contents @@ -167,7 +161,6 @@ function copyLink (resolvedSrc, dest) { } // 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) { const srcArray = path.resolve(src).split(path.sep) const destArray = path.resolve(dest).split(path.sep) @@ -177,43 +170,27 @@ function isSrcSubdir (src, dest) { }, true) } -// check if dest exists and is a symlink. -function checkDest (dest) { - let resolvedPath +function checkStats (src, dest) { + const srcStat = fs.statSync(src) + let destStat try { - resolvedPath = fs.readlinkSync(dest) + destStat = fs.statSync(dest) } catch (err) { - if (err.code === 'ENOENT') return notExist - - // dest exists and is a regular file or directory, Windows may throw UNKNOWN error. - if (err.code === 'EINVAL' || err.code === 'UNKNOWN') return existsReg - + if (err.code === 'ENOENT') return {srcStat, destStat: notExist} throw err } - return resolvedPath // dest exists and is a symlink -} - -function pathsAreIdentical (src, dest) { - const os = process.platform - const resolvedSrc = path.resolve(src) - const resolvedDest = path.resolve(dest) - // case-insensitive paths - if (os === 'darwin' || os === 'win32') { - return resolvedSrc.toLowerCase() === resolvedDest.toLowerCase() - } - return resolvedSrc === resolvedDest + return {srcStat, destStat} } function checkPaths (src, dest) { - const resolvedDest = checkDest(dest) - if (resolvedDest === notExist || resolvedDest === existsReg) { - if (pathsAreIdentical(src, dest)) throw new Error('Source and destination must not be the same.') - return resolvedDest - } else { - // check resolved dest path if dest is a symlink - if (pathsAreIdentical(src, resolvedDest)) throw new Error('Source and destination must not be the same.') - return resolvedDest + const {srcStat, destStat} = checkStats(src, dest) + if (srcStat && destStat && destStat !== notExist && srcStat.ino === destStat.ino) { + throw new Error('Source and destination must not be the same.') + } + if (srcStat && srcStat.isDirectory() && isSrcSubdir(src, dest)) { + throw new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`) } + return destStat } module.exports = copySync diff --git a/lib/copy/__tests__/copy-prevent-copying-identical.test.js b/lib/copy/__tests__/copy-prevent-copying-identical.test.js index d0a67e9a..b2aa211b 100644 --- a/lib/copy/__tests__/copy-prevent-copying-identical.test.js +++ b/lib/copy/__tests__/copy-prevent-copying-identical.test.js @@ -23,6 +23,7 @@ describe('+ copy() - prevent copying identical files and dirs', () => { it('should return an error if src and dest are the same', done => { const fileSrc = path.join(TEST_DIR, 'TEST_fs-extra_copy') const fileDest = path.join(TEST_DIR, 'TEST_fs-extra_copy') + fs.ensureFileSync(fileSrc) fs.copy(fileSrc, fileDest, err => { assert.strictEqual(err.message, 'Source and destination must not be the same.') @@ -30,6 +31,72 @@ describe('+ copy() - prevent copying identical files and dirs', () => { }) }) + describe('dest with parent symlink', () => { + describe('first parent is symlink', () => { + it('should error when src is file', done => { + const src = path.join(TEST_DIR, 'a', 'file.txt') + const dest = path.join(TEST_DIR, 'b', 'file.txt') + const srcParent = path.join(TEST_DIR, 'a') + const destParent = path.join(TEST_DIR, 'b') + fs.ensureFileSync(src) + fs.ensureSymlinkSync(srcParent, destParent, 'dir') + + fs.copy(src, dest, err => { + assert.strictEqual(err.message, 'Source and destination must not be the same.') + assert(fs.existsSync(src)) + done() + }) + }) + + it('should error when src is directory', done => { + const src = path.join(TEST_DIR, 'a', 'foo') + const dest = path.join(TEST_DIR, 'b', 'foo') + const srcParent = path.join(TEST_DIR, 'a') + const destParent = path.join(TEST_DIR, 'b') + fs.ensureDirSync(src) + fs.ensureSymlinkSync(srcParent, destParent, 'dir') + + fs.copy(src, dest, err => { + assert.strictEqual(err.message, 'Source and destination must not be the same.') + assert(fs.existsSync(src)) + done() + }) + }) + }) + + describe('nested dest', () => { + it('should error when src is file', done => { + const src = path.join(TEST_DIR, 'a', 'dir', 'file.txt') + const dest = path.join(TEST_DIR, 'b', 'dir', 'file.txt') + const srcParent = path.join(TEST_DIR, 'a') + const destParent = path.join(TEST_DIR, 'b') + fs.ensureFileSync(src) + fs.ensureSymlinkSync(srcParent, destParent, 'dir') + + fs.copy(src, dest, err => { + assert.strictEqual(err.message, 'Source and destination must not be the same.') + assert(fs.existsSync(src)) + done() + }) + }) + + it('should error when src is directory', done => { + const src = path.join(TEST_DIR, 'a', 'dir', 'foo') + const dest = path.join(TEST_DIR, 'b', 'dir', 'foo') + const srcParent = path.join(TEST_DIR, 'a') + const destParent = path.join(TEST_DIR, 'b') + fs.ensureDirSync(src) + fs.ensureSymlinkSync(srcParent, destParent, 'dir') + + fs.copy(src, dest, err => { + assert.strictEqual(err.message, 'Source and destination must not be the same.') + assert(fs.existsSync(src)) + done() + }) + }) + }) + }) + // src is directory: // src is regular, dest is symlink // src is symlink, dest is regular @@ -88,7 +155,7 @@ describe('+ copy() - prevent copying identical files and dirs', () => { }) describe('>> when src and dest are symlinks that point to the exact same path', () => { - it('should not copy and return', done => { + it('should error src and dest are the same', done => { src = path.join(TEST_DIR, 'src') fs.mkdirsSync(src) const srcLink = path.join(TEST_DIR, 'src_symlink') @@ -100,7 +167,7 @@ describe('+ copy() - prevent copying identical files and dirs', () => { const destlenBefore = klawSync(destLink).length fs.copy(srcLink, destLink, err => { - assert.ifError(err) + assert.strictEqual(err.message, 'Source and destination must not be the same.') const srclenAfter = klawSync(srcLink).length assert.strictEqual(srclenAfter, srclenBefore, 'src length should not change') @@ -125,19 +192,14 @@ describe('+ copy() - prevent copying identical files and dirs', () => { describe('> when src is a file', () => { describe(`>> when src is regular and dest is a symlink that points to src`, () => { it('should error', done => { - src = path.join(TEST_DIR, 'src', 'somefile.txt') - fs.ensureFileSync(src) - fs.writeFileSync(src, 'some data') + src = path.join(TEST_DIR, 'src.txt') + fs.outputFileSync(src, 'some data') const destLink = path.join(TEST_DIR, 'dest-symlink') fs.symlinkSync(src, destLink, 'file') fs.copy(src, destLink, err => { assert.strictEqual(err.message, 'Source and destination must not be the same.') - - const link = fs.readlinkSync(destLink) - assert.strictEqual(link, src) - assert(fs.readFileSync(link, 'utf8'), 'some data') done() }) }) @@ -146,14 +208,13 @@ describe('+ copy() - prevent copying identical files and dirs', () => { describe(`>> when src is a symlink that points to a regular dest`, () => { it('should throw error', done => { dest = path.join(TEST_DIR, 'dest', 'somefile.txt') - fs.ensureFileSync(dest) - fs.writeFileSync(dest, 'some data') + fs.outputFileSync(dest, 'some data') const srcLink = path.join(TEST_DIR, 'src-symlink') fs.symlinkSync(dest, srcLink, 'file') fs.copy(srcLink, dest, err => { - assert.ok(err) + assert.strictEqual(err.message, 'Source and destination must not be the same.') const link = fs.readlinkSync(srcLink) assert.strictEqual(link, dest) @@ -164,10 +225,9 @@ describe('+ copy() - prevent copying identical files and dirs', () => { }) describe('>> when src and dest are symlinks that point to the exact same path', () => { - it('should not copy and return', done => { + it('should error src and dest are the same', done => { src = path.join(TEST_DIR, 'src', 'srcfile.txt') - fs.ensureFileSync(src) - fs.writeFileSync(src, 'src data') + fs.outputFileSync(src, 'src data') const srcLink = path.join(TEST_DIR, 'src_symlink') fs.symlinkSync(src, srcLink, 'file') @@ -176,7 +236,7 @@ describe('+ copy() - prevent copying identical files and dirs', () => { fs.symlinkSync(src, destLink, 'file') fs.copy(srcLink, destLink, err => { - assert.ifError(err) + assert.strictEqual(err.message, 'Source and destination must not be the same.') const srcln = fs.readlinkSync(srcLink) assert.strictEqual(srcln, src) 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 60a75921..a3b944f7 100644 --- a/lib/copy/__tests__/copy-prevent-copying-into-itself.test.js +++ b/lib/copy/__tests__/copy-prevent-copying-into-itself.test.js @@ -39,7 +39,7 @@ describe('+ copy() - prevent copying into itself', () => { afterEach(done => fs.remove(TEST_DIR, done)) describe('> when source is a file', () => { - it('should copy the file successfully even when dest parent is a subdir of src', done => { + it('should copy the file successfully even if dest parent is a subdir of src', done => { const srcFile = path.join(TEST_DIR, 'src', 'srcfile.txt') const destFile = path.join(TEST_DIR, 'src', 'dest', 'destfile.txt') fs.writeFileSync(srcFile, dat0) @@ -279,7 +279,7 @@ describe('+ copy() - prevent copying into itself', () => { }) describe('>> when dest is a symlink', () => { - it('should silently return when resolved dest path is exactly the same as resolved src path', done => { + it('should error when resolved dest path is exactly the same as resolved src path', done => { const srcLink = path.join(TEST_DIR, 'src-symlink') fs.symlinkSync(src, srcLink, 'dir') const destLink = path.join(TEST_DIR, 'dest-symlink') @@ -291,7 +291,7 @@ describe('+ copy() - prevent copying into itself', () => { assert(destlenBefore > 2) fs.copy(srcLink, destLink, err => { - assert.ifError(err) + assert.strictEqual(err.message, 'Source and destination must not be the same.') const srclenAfter = klawSync(srcLink).length assert.strictEqual(srclenAfter, srclenBefore, 'src length should not change') @@ -313,13 +313,12 @@ describe('+ copy() - prevent copying into itself', () => { const destLink = path.join(TEST_DIR, 'dest-symlink') const resolvedDestPath = path.join(TEST_DIR, 'src', 'some', 'nested', 'dest') fs.ensureFileSync(path.join(resolvedDestPath, 'subdir', 'somefile.txt')) - fs.symlinkSync(resolvedDestPath, destLink, 'dir') fs.copy(srcLink, destLink, err => { - assert.ifError(err) + assert.strictEqual(err.message, `Cannot copy '${src}' to a subdirectory of itself, '${resolvedDestPath}'.`) const destln = fs.readlinkSync(destLink) - assert.strictEqual(destln, src) + assert.strictEqual(destln, resolvedDestPath) done() }) }) @@ -328,13 +327,11 @@ describe('+ copy() - prevent copying into itself', () => { const srcInDest = path.join(TEST_DIR, 'dest', 'some', 'nested', 'src') const srcLink = path.join(TEST_DIR, 'src-symlink') const destLink = path.join(TEST_DIR, 'dest-symlink') - const dest = path.join(TEST_DIR, 'dest') - fs.mkdirSync(dest) - - fs.symlinkSync(srcInDest, srcLink, 'dir') - fs.symlinkSync(dest, destLink, 'dir') + fs.ensureDirSync(srcInDest) + fs.ensureSymlinkSync(srcInDest, srcLink, 'dir') + fs.ensureSymlinkSync(dest, destLink, 'dir') fs.copy(srcLink, destLink, err => { assert.strictEqual(err.message, `Cannot overwrite '${dest}' with '${srcInDest}'.`) diff --git a/lib/copy/__tests__/copy.test.js b/lib/copy/__tests__/copy.test.js index db7a0146..23d80863 100644 --- a/lib/copy/__tests__/copy.test.js +++ b/lib/copy/__tests__/copy.test.js @@ -24,6 +24,8 @@ describe('fs-extra', () => { it('should return an error if src and dest are the same', done => { const fileSrc = path.join(TEST_DIR, 'TEST_fs-extra_copy') const fileDest = path.join(TEST_DIR, 'TEST_fs-extra_copy') + fse.ensureFileSync(fileSrc) + fse.copy(fileSrc, fileDest, err => { assert.strictEqual(err.message, 'Source and destination must not be the same.') done() @@ -136,6 +138,22 @@ describe('fs-extra', () => { }) }) + it('should preserve symbolic links', done => { + const src = path.join(TEST_DIR, 'src') + const dest = path.join(TEST_DIR, 'dest') + const srcTarget = path.join(TEST_DIR, 'destination') + fse.mkdirSync(src) + fse.mkdirSync(srcTarget) + fse.symlinkSync(srcTarget, path.join(src, 'symlink')) + + fse.copy(src, dest, err => { + assert.ifError(err) + const link = fs.readlinkSync(path.join(dest, 'symlink')) + assert.strictEqual(link, srcTarget) + done() + }) + }) + it('should copy the directory asynchronously', done => { const FILES = 2 const src = path.join(TEST_DIR, 'src') diff --git a/lib/copy/__tests__/ncp/broken-symlink.test.js b/lib/copy/__tests__/ncp/broken-symlink.test.js index e837491d..461fa85d 100644 --- a/lib/copy/__tests__/ncp/broken-symlink.test.js +++ b/lib/copy/__tests__/ncp/broken-symlink.test.js @@ -23,10 +23,9 @@ describe('ncp broken symlink', () => { afterEach(done => fse.remove(TEST_DIR, done)) - it('should copy broken symlinks by default', done => { + it('should error if symlink is broken', done => { ncp(src, out, err => { - if (err) return done(err) - assert.strictEqual(fs.readlinkSync(path.join(out, 'broken-symlink')), path.join(src, 'does-not-exist')) + assert(err) done() }) }) diff --git a/lib/copy/copy.js b/lib/copy/copy.js index 51d8fe83..49e6265c 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -7,7 +7,6 @@ const pathExists = require('../path-exists').pathExists const utimes = require('../util/utimes').utimesMillis const notExist = Symbol('notExist') -const existsReg = Symbol('existsReg') function copy (src, dest, opts, cb) { if (typeof opts === 'function' && !cb) { @@ -29,56 +28,55 @@ function copy (src, dest, opts, cb) { see https://github.com/jprichardson/node-fs-extra/issues/269`) } - checkPaths(src, dest, (err, resolvedDest) => { + checkPaths(src, dest, (err, destStat) => { if (err) return cb(err) - if (opts.filter) return handleFilter(checkParentDir, resolvedDest, src, dest, opts, cb) - return checkParentDir(resolvedDest, src, dest, opts, cb) + if (opts.filter) return handleFilter(checkParentDir, destStat, src, dest, opts, cb) + return checkParentDir(destStat, src, dest, opts, cb) }) } -function checkParentDir (resolvedDest, src, dest, opts, cb) { +function checkParentDir (destStat, src, dest, opts, cb) { const destParent = path.dirname(dest) pathExists(destParent, (err, dirExists) => { if (err) return cb(err) - if (dirExists) return startCopy(resolvedDest, src, dest, opts, cb) + if (dirExists) return startCopy(destStat, src, dest, opts, cb) mkdirp(destParent, err => { if (err) return cb(err) - return startCopy(resolvedDest, src, dest, opts, cb) + return startCopy(destStat, src, dest, opts, cb) }) }) } -function startCopy (resolvedDest, src, dest, opts, cb) { - if (opts.filter) return handleFilter(getStats, resolvedDest, src, dest, opts, cb) - return getStats(resolvedDest, src, dest, opts, cb) -} - -function handleFilter (onInclude, resolvedDest, src, dest, opts, cb) { +function handleFilter (onInclude, destStat, src, dest, opts, cb) { Promise.resolve(opts.filter(src, dest)).then(include => { if (include) { - if (resolvedDest) return onInclude(resolvedDest, src, dest, opts, cb) + if (destStat) return onInclude(destStat, src, dest, opts, cb) return onInclude(src, dest, opts, cb) } return cb() }, error => cb(error)) } -function getStats (resolvedDest, src, dest, opts, cb) { +function startCopy (destStat, src, dest, opts, cb) { + if (opts.filter) return handleFilter(getStats, destStat, src, dest, opts, cb) + return getStats(destStat, src, dest, opts, cb) +} + +function getStats (destStat, src, dest, opts, cb) { const stat = opts.dereference ? fs.stat : fs.lstat - stat(src, (err, st) => { + stat(src, (err, srcStat) => { if (err) return cb(err) - if (st.isDirectory()) return onDir(st, resolvedDest, src, dest, opts, cb) - else if (st.isFile() || - st.isCharacterDevice() || - st.isBlockDevice()) return onFile(st, resolvedDest, src, dest, opts, cb) - else if (st.isSymbolicLink()) return onLink(resolvedDest, src, dest, opts, cb) + if (srcStat.isDirectory()) return onDir(srcStat, destStat, src, dest, opts, cb) + else if (srcStat.isFile() || + srcStat.isCharacterDevice() || + srcStat.isBlockDevice()) return onFile(srcStat, destStat, src, dest, opts, cb) + else if (srcStat.isSymbolicLink()) return onLink(destStat, src, dest, opts, cb) }) } -function onFile (srcStat, resolvedDest, src, dest, opts, cb) { - if (resolvedDest === notExist) return copyFile(srcStat, src, dest, opts, cb) - else if (resolvedDest === existsReg) return mayCopyFile(srcStat, src, dest, opts, cb) +function onFile (srcStat, destStat, src, dest, opts, cb) { + if (destStat === notExist) return copyFile(srcStat, src, dest, opts, cb) return mayCopyFile(srcStat, src, dest, opts, cb) } @@ -123,31 +121,14 @@ function setDestModeAndTimestamps (srcStat, dest, opts, cb) { }) } -function onDir (srcStat, resolvedDest, src, dest, opts, cb) { - if (resolvedDest === notExist) { - if (isSrcSubdir(src, dest)) { - return cb(new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`)) - } - return mkDirAndCopy(srcStat, src, dest, opts, cb) - } else if (resolvedDest === existsReg) { - if (isSrcSubdir(src, dest)) { - return cb(new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`)) - } - return mayCopyDir(src, dest, opts, cb) +function onDir (srcStat, destStat, src, dest, opts, cb) { + if (destStat === notExist) return mkDirAndCopy(srcStat, src, dest, opts, cb) + if (destStat && !destStat.isDirectory()) { + return cb(new Error(`Cannot overwrite non-directory '${dest}' with directory '${src}'.`)) } return copyDir(src, dest, opts, cb) } -function mayCopyDir (src, dest, opts, cb) { - fs.stat(dest, (err, st) => { - if (err) return cb(err) - if (!st.isDirectory()) { - return cb(new Error(`Cannot overwrite non-directory '${dest}' with directory '${src}'.`)) - } - return copyDir(src, dest, opts, cb) - }) -} - function mkDirAndCopy (srcStat, src, dest, opts, cb) { fs.mkdir(dest, srcStat.mode, err => { if (err) return cb(err) @@ -174,16 +155,16 @@ 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, resolvedDest) => { + checkPaths(srcItem, destItem, (err, destStat) => { if (err) return cb(err) - startCopy(resolvedDest, srcItem, destItem, opts, err => { + startCopy(destStat, srcItem, destItem, opts, err => { if (err) return cb(err) return copyDirItems(items, src, dest, opts, cb) }) }) } -function onLink (resolvedDest, src, dest, opts, cb) { +function onLink (destStat, src, dest, opts, cb) { fs.readlink(src, (err, resolvedSrc) => { if (err) return cb(err) @@ -191,22 +172,28 @@ function onLink (resolvedDest, src, dest, opts, cb) { resolvedSrc = path.resolve(process.cwd(), resolvedSrc) } - if (resolvedDest === notExist || resolvedDest === existsReg) { - // if dest already exists, fs throws error anyway, - // so no need to guard against it here. + if (destStat === notExist) { return fs.symlink(resolvedSrc, dest, cb) } else { - if (opts.dereference) { - resolvedDest = path.resolve(process.cwd(), resolvedDest) - } - if (pathsAreIdentical(resolvedSrc, resolvedDest)) return cb() - - // 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. - fs.stat(dest, (err, st) => { - if (err) return cb(err) - if (st.isDirectory() && isSrcSubdir(resolvedDest, resolvedSrc)) { + fs.readlink(dest, (err, resolvedDest) => { + if (err) { + // dest exists and is a regular file or directory, + // Windows may throw UNKNOWN error. If dest already exists, + // fs throws error anyway, so no need to guard against it here. + if (err.code === 'EINVAL' || err.code === 'UNKNOWN') return fs.symlink(resolvedSrc, dest, cb) + return cb(err) + } + if (opts.dereference) { + resolvedDest = path.resolve(process.cwd(), resolvedDest) + } + if (isSrcSubdir(resolvedSrc, resolvedDest)) { + return cb(new Error(`Cannot copy '${resolvedSrc}' to a subdirectory of itself, '${resolvedDest}'.`)) + } + + // do not 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 (destStat.isDirectory() && isSrcSubdir(resolvedDest, resolvedSrc)) { return cb(new Error(`Cannot overwrite '${resolvedDest}' with '${resolvedSrc}'.`)) } return copyLink(resolvedSrc, dest, cb) @@ -223,7 +210,6 @@ function copyLink (resolvedSrc, dest, cb) { } // 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) { const srcArray = path.resolve(src).split(path.sep) const destArray = path.resolve(dest).split(path.sep) @@ -233,43 +219,30 @@ function isSrcSubdir (src, dest) { }, true) } -// check if dest exists and is a symlink. -function checkDest (dest, cb) { - fs.readlink(dest, (err, resolvedPath) => { - if (err) { - if (err.code === 'ENOENT') return cb(null, notExist) - - // dest exists and is a regular file or directory, Windows may throw UNKNOWN error. - if (err.code === 'EINVAL' || err.code === 'UNKNOWN') return cb(null, existsReg) - - return cb(err) - } - return cb(null, resolvedPath) // dest exists and is a symlink +function checkStats (src, dest, cb) { + fs.stat(src, (err, srcStat) => { + if (err) return cb(err) + fs.stat(dest, (err, destStat) => { + if (err) { + if (err.code === 'ENOENT') return cb(null, {srcStat, destStat: notExist}) + return cb(err) + } + return cb(null, {srcStat, destStat}) + }) }) } -function pathsAreIdentical (src, dest) { - const os = process.platform - const resolvedSrc = path.resolve(src) - const resolvedDest = path.resolve(dest) - // case-insensitive paths - if (os === 'darwin' || os === 'win32') { - return resolvedSrc.toLowerCase() === resolvedDest.toLowerCase() - } - return resolvedSrc === resolvedDest -} - function checkPaths (src, dest, cb) { - checkDest(dest, (err, resolvedDest) => { + checkStats(src, dest, (err, stats) => { if (err) return cb(err) - if (resolvedDest === notExist || resolvedDest === existsReg) { - if (pathsAreIdentical(src, dest)) return cb(new Error('Source and destination must not be the same.')) - return cb(null, resolvedDest) - } else { - // check resolved dest path if dest is a symlink - if (pathsAreIdentical(src, resolvedDest)) return cb(new Error('Source and destination must not be the same.')) - return cb(null, resolvedDest) + const {srcStat, destStat} = stats + if (srcStat && destStat && destStat !== notExist && srcStat.ino === destStat.ino) { + return cb(new Error('Source and destination must not be the same.')) + } + if (srcStat && srcStat.isDirectory() && isSrcSubdir(src, dest)) { + return cb(new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`)) } + return cb(null, destStat) }) } diff --git a/lib/json/__tests__/output-json-sync.test.js b/lib/json/__tests__/output-json-sync.test.js index 44629dd1..a4bc8c50 100644 --- a/lib/json/__tests__/output-json-sync.test.js +++ b/lib/json/__tests__/output-json-sync.test.js @@ -13,7 +13,7 @@ describe('json', () => { let TEST_DIR beforeEach(done => { - TEST_DIR = path.join(os.tmpdir(), 'fs-extra') + TEST_DIR = path.join(os.tmpdir(), 'fs-extra-output-json-sync') fse.emptyDir(TEST_DIR, done) }) diff --git a/lib/json/__tests__/output-json.test.js b/lib/json/__tests__/output-json.test.js index 6668fa4c..f69108a6 100644 --- a/lib/json/__tests__/output-json.test.js +++ b/lib/json/__tests__/output-json.test.js @@ -13,7 +13,7 @@ describe('json', () => { let TEST_DIR beforeEach(done => { - TEST_DIR = path.join(os.tmpdir(), 'fs-extra') + TEST_DIR = path.join(os.tmpdir(), 'fs-extra-output-json') fse.emptyDir(TEST_DIR, done) }) From 456241a4114e81511e7537054a116a3572c40a84 Mon Sep 17 00:00:00 2001 From: Mani Maghsoudlou Date: Thu, 24 May 2018 20:39:25 -0700 Subject: [PATCH 2/2] refactor a bit --- lib/copy-sync/copy-sync.js | 9 +++------ lib/copy/copy.js | 9 +++------ 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/lib/copy-sync/copy-sync.js b/lib/copy-sync/copy-sync.js index 3985b6bc..775647db 100644 --- a/lib/copy-sync/copy-sync.js +++ b/lib/copy-sync/copy-sync.js @@ -164,10 +164,7 @@ function copyLink (resolvedSrc, dest) { 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) => { - return acc && destArray[i] === current - }, true) + return srcArray.reduce((acc, current, i) => acc && destArray[i] === current, true) } function checkStats (src, dest) { @@ -184,10 +181,10 @@ function checkStats (src, dest) { function checkPaths (src, dest) { const {srcStat, destStat} = checkStats(src, dest) - if (srcStat && destStat && destStat !== notExist && srcStat.ino === destStat.ino) { + if (destStat.ino && destStat.ino === srcStat.ino) { throw new Error('Source and destination must not be the same.') } - if (srcStat && srcStat.isDirectory() && isSrcSubdir(src, dest)) { + if (srcStat.isDirectory() && isSrcSubdir(src, dest)) { throw new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`) } return destStat diff --git a/lib/copy/copy.js b/lib/copy/copy.js index 49e6265c..af91a53e 100644 --- a/lib/copy/copy.js +++ b/lib/copy/copy.js @@ -213,10 +213,7 @@ function copyLink (resolvedSrc, dest, cb) { 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) => { - return acc && destArray[i] === current - }, true) + return srcArray.reduce((acc, current, i) => acc && destArray[i] === current, true) } function checkStats (src, dest, cb) { @@ -236,10 +233,10 @@ function checkPaths (src, dest, cb) { checkStats(src, dest, (err, stats) => { if (err) return cb(err) const {srcStat, destStat} = stats - if (srcStat && destStat && destStat !== notExist && srcStat.ino === destStat.ino) { + if (destStat.ino && destStat.ino === srcStat.ino) { return cb(new Error('Source and destination must not be the same.')) } - if (srcStat && srcStat.isDirectory() && isSrcSubdir(src, dest)) { + if (srcStat.isDirectory() && isSrcSubdir(src, dest)) { return cb(new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`)) } return cb(null, destStat)