Prevent copying a directory into itself

[iwege]

fse.copy('src','src/dest') will create the dest continues to recurse, and finally it throws an error ENAMETOOLONG

It links to AvianFlu/ncp#4

[danyshaanan]

How does require('fs').copy handle this case?

[jprichardson]

Off the cuff, I'd say that I could just resolve the paths and check if one is a substring of the other. But I'd be concerned about introducing regressions. Thoughts?

[iwege]

I have collected some cases in this pr AvianFlu/ncp#61, maybe you can use it as a reference.

[jprichardson]

@lwege are you still interested in this getting fixed? I'd like to tackle this - it'd be awesome to take your list of cases and turn them into tests for both *nix and Windows.

[iwege]

@jimhigson I copied testcase to https://github.com/iwege/node-fs-extra/tree/feature/prevent-copy-to-self , but how can I test it first? Or do I need to fix this issue with my old code?

[jprichardson]

Would you link directly to the test case so that I can take a look? Thanks.

[iwege]

@jprichardson iwege@0dc1610

[jprichardson]

Thank you. Tests look good. Any thoughts on a reliable fix?

[iwege]

I have used the AvianFlu/ncp#61 in my project and no one reports relative issue to me. But I don't know it reliable or not. Maybe my user doesn't do this in my product.

[DaneEveritt]

Sorry to bring this back up, but any progress on implementing this, or should I add my own checks?

[jprichardson]

or should I add my own checks

What checks would you add?

[DaneEveritt]

I built off @iwege's changes, and made the following function that I'm using before I call the move function.

isSelf(moveTo, moveFrom) {
    const target = this.server.path(moveTo); // these two lines are simply building a path to the files
    const source = this.server.path(moveFrom);

    if (!_.startsWith(target, source)) {
        return false;
    }

    const end = target.slice(source.length);
    if (!end) {
        return true;
    }

    return _.startsWith(end, '/');
}

Prevents moving folders into themselves, but doesn't block moving a folder into another folder that starts with the same name (so src can still move into src-dest, but not into src).

Renaming files to the same name also triggers the catch, but that doesn't bother me so much.

[RyanZim]

Off the cuff, I'd say that I could just resolve the paths and check if one is a substring of the other. But I'd be concerned about introducing regressions.

@jprichardson That is the most simple way to do it; any edge cases that this wouldn't work?

[jprichardson]

@jprichardson That is the most simple way to do it; any edge cases that this wouldn't work?

None that I could think of. This could could get harry though if symlinks/hardlinks/junctions are involved though.

[RyanZim]

This could could get hairy though if symlinks/hardlinks/junctions are involved though.

Hadn't thought of that. Perhaps we should check how other 3rd party copy libs handle this.

@jprichardson Is this still v1.0.0-scope?

[jprichardson]

@jprichardson Is this still v1.0.0-scope?

Nope.

[RyanZim]

Just finding out how few directory copiers there really are; most copy modules only copy files that match a glob.

Starting with:

.
└── src
    └── file

https://github.com/timkendrick/recursive-copy will copy the directory into itself once, then stop. The end result will be:

.
└── src
    ├── dest
    │   └── file
    └── file

This isn't a terrible idea. I haven't looked at the code, but I would guess that they are getting a full recursive list of files before starting the copy operation.

@jprichardson Thoughts?

[jprichardson]

@jprichardson Thoughts?

What do rimraf or cpr doing as it pertains to globs?

[RyanZim]

Sorry so long, I must have missed or forgotten this.

What do rimraf or cpr doing as it pertains to globs?

cpr has the same problem we do. Not sure what you're asking about rimraf.

Edit: Sorry, I made a mistake in my testing; cpr behaves the same a recursive-copy.

[manidlou]

So, after digging into this for a few days, and following

@jprichardson That is the most simple way to do it; any edge cases that this wouldn't work?

I've found a solution that prevents a directory from copying into itself, or in other words, if dest is somehow a subdir of src like src/dest, it throws an error and inform the user. It works for symlinks as well. I've written unit tests that includes all different cases that @iwege introduced + a few more.

This is the commit https://github.com/manidlou/node-fs-extra/commit/53dcb50c1da94bf007b3071c42052b7f5da2471e.

I appreciate if you take a look at it. I also included a solution that resolves #198 for copySync.

I haven't fully applied this solution to copy because of #292 and also waiting to see what you think about the suggested ideas and maybe discuss on them a little. I am completely open to any feedback. Let's solve this issue!

Edit

This is basically how it prevents copying directory into itself

// if dest is a substring (subdir) of src, prevent copying dir into itself
// by extracting dest base dir and check if that is the same as src basename
if (dest.includes(src) && (dest.split(path.dirname(src) + path.sep)[1].split(path.sep)[0] === path.basename(src))) {
  throw new Error('Cannot copy directory \'' + src + '\' into itself \'' + dest + '\'')
}

[jprichardson]

Nice work @manidlou . I am admittedly worried about regressions, but if we have regressions, that just means our test suite isn't good enough.

I didn't look at this with as much clarity as may require, but at a cursory glance, I don't see an issue. What do you think @RyanZim?

[manidlou]

I am admittedly worried about regressions

I am a little worried about regressions too. I am trying to think of any cases that may cause an issue.

[RyanZim]

@manidlou I left one comment on the commit, other than that, looks pretty good.

@jprichardson What are we going to do about #292? The code and tests need rewritten sooner or later. See my comment there.

Don't have much time to think/investigate; I'm working at the same place I worked over Christmas...you know what that means...I'm busy.

[manidlou]

@jprichardson, @RyanZim thanks a lot. I appreciate your feedback. As I said, I am trying to refactor the changes a little and add more unit tests.

[manidlou]

Well, regarding preventing copying a dir into itself, when dealing only with files or dirs, we are pretty much safe. But, when symlinks are involved, then problems may arise. Therefore, I tried to think of various cases that symlinks are somehow involved in copying src to dest:

• src is symlink points exactly to dest, and vice versa (No copy, just return)
• src is symlink points to a subdir in dest (OK to copy)
• dest is symlink points to a subdir in src (Not OK to copy)

So, I am refactoring my latest changes to include all these cases with their unit tests. I will then link the commit here for you to review it again.

Edit

More cases when both src and dest are symlinks:

• src resolved path points to the exact dest resolved path, and vice versa (No copy, just return)
• src resolved path points to a subdir in dest resolved path (OK to copy)
• dest resolved path points to a subdir in src resolved path (Not OK to copy)

[manidlou]

Alright, I refactored my latest changes for copySync and added more unit tests for various weird cases. Would you please take a loot at it? Thanks.

This is the commit https://github.com/manidlou/node-fs-extra/commit/483bab5a383d4dfd4b69ca0871f819d33ad01ec9.

Edit

Please consider this commit since I added one more unit test for the case that dest is deeply nested and all its parent dirs need to be created.

This is the commit https://github.com/manidlou/node-fs-extra/commit/fd69bac5b99b52fcc6e4b1e4899f0dda83ad3a65.

[RyanZim]

Sorry, don't have time to review this, I'll let @jprichardson do it.

[RyanZim]

Fixed in 5.0.0 🎉