Skip to content

Commit

Permalink
copy*(): refactor to check paths more efficiently
Browse files Browse the repository at this point in the history
  • Loading branch information
@manidlou
manidlou committed Aug 31, 2018
1 parent 7a5cf61 commit d96adb3
Show file tree
Hide file tree
Showing 7 changed files with 119 additions and 49 deletions.
2 changes: 1 addition & 1 deletion lib/copy-sync/__tests__/copy-sync-dir.test.js
View file
Expand Up @@ -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)

Expand Down
41 changes: 33 additions & 8 deletions lib/copy-sync/__tests__/copy-sync-prevent-copying-into-itself.test.js
View file
Expand Up @@ -174,17 +174,46 @@ describe('+ copySync() - prevent copying into itself', () => {
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)
}
})

const srclenAfter = klawSync(src).length
assert.strictEqual(srclenAfter, srclenBefore, 'src length should not change')
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 link = fs.readlinkSync(destLink)
assert.strictEqual(link, src)
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', () => {
Expand Down Expand Up @@ -372,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')
Expand Down
25 changes: 19 additions & 6 deletions lib/copy-sync/copy-sync.js
View file
Expand Up @@ -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

Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -179,23 +180,35 @@ function checkStats (src, dest) {
return {srcStat, destStat}
}

function checkParentStats (src, dest, cb) {
const {srcStat, destStat} = checkStats(src, path.dirname(dest))
// 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) {
checkParentStats(src, dest)
const {srcStat, destStat} = checkStats(src, dest)
if (destStat.ino && destStat.ino === srcStat.ino) {
throw new Error('Source and destination must not be the same.')
}
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
2 changes: 2 additions & 0 deletions lib/copy/__tests__/copy-case-insensitive-paths.test.js
View file
Expand Up @@ -5,7 +5,9 @@ const os = require('os')
const path = require('path')
const fs = require('../../')
const platform = os.platform()

console.log('platform: ' + platform)

/* global beforeEach, afterEach, describe, it */

describe('+ copy() - case insensitive paths', () => {
Expand Down
26 changes: 22 additions & 4 deletions lib/copy/__tests__/copy-prevent-copying-into-itself.test.js
View file
Expand Up @@ -176,6 +176,7 @@ describe('+ copy() - prevent copying into itself', () => {
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}'.`)

Expand All @@ -188,6 +189,27 @@ describe('+ copy() - prevent copying into itself', () => {
})
})

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')
Expand Down Expand Up @@ -370,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')
Expand Down
15 changes: 9 additions & 6 deletions lib/copy/__tests__/copy.test.js
View file
Expand Up @@ -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)
Expand Down Expand Up @@ -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 => {
Expand Down
57 changes: 33 additions & 24 deletions lib/copy/copy.js
View file
Expand Up @@ -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)
})
})
}

Expand Down Expand Up @@ -155,7 +159,7 @@ 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, {destStat}) => {
if (err) return cb(err)
startCopy(destStat, srcItem, destItem, opts, err => {
if (err) return cb(err)
Expand Down Expand Up @@ -211,9 +215,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) {
Expand All @@ -229,31 +233,36 @@ function checkStats (src, dest, cb) {
})
}

function checkParentStats (src, dest, cb) {
checkStats(src, path.dirname(dest), (err, stats) => {
if (err) return cb(err)
const {srcStat, destStat} = stats
// 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 cb()
return checkParentStats(src, srcStat, destParent, cb)
})
}

function checkPaths (src, dest, cb) {
checkParentStats(src, dest, err => {
checkStats(src, dest, (err, stats) => {
if (err) return cb(err)
checkStats(src, dest, (err, stats) => {
if (err) return cb(err)
const {srcStat, destStat} = stats
if (destStat.ino && destStat.ino === srcStat.ino) {
return cb(new Error('Source and destination must not be the same.'))
}
if (srcStat.isDirectory() && isSrcSubdir(src, dest)) {
return cb(new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`))
}
return cb(null, destStat)
})
const {srcStat, destStat} = stats
if (destStat.ino && destStat.ino === srcStat.ino) {
return cb(new Error('Source and destination must not be the same.'))
}
if (srcStat.isDirectory() && isSrcSubdir(src, dest)) {
return cb(new Error(`Cannot copy '${src}' to a subdirectory of itself, '${dest}'.`))
}
return cb(null, {srcStat, destStat})
})
}

Expand Down

0 comments on commit d96adb3

Please sign in to comment.