New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to prevent moving files by renaming files using FileTreeView #11862

Merged
merged 4 commits into from Aug 16, 2016

Conversation

Projects
None yet
4 participants
@tallandroid

tallandroid commented Oct 28, 2015

  1. Added / to be an invalid file name character for mac platform
  2. Return baseName to include ../ if the fullpath contains ../
harshitr
1) Added / to be an invalid file name character for mac platform
2) Return baseName to include ../ if the fullpath contains ../
@petetnt

This comment has been minimized.

Show comment
Hide comment
@petetnt

petetnt Oct 28, 2015

Collaborator

../ is an valid expression on Windows too. So the function should be re-factored as:

    function getBaseName(fullPath) {
        var lastSlash = fullPath.lastIndexOf("/");
        var preFinalSlash = fullPath.substring(0, lastSlash).lastIndexOf("/");

        if (lastSlash === fullPath.length - 1) {  // directory: exclude trailing "/" too
            return fullPath.slice(fullPath.lastIndexOf("/", fullPath.length - 2) + 1, -1);
        } else if (fullPath.substring(preFinalSlash, lastSlash + 1) === "/../") {
            return fullPath.slice(preFinalSlash + 1);
        }
        return fullPath.slice(lastSlash + 1);
    }

However, it's only a partial fix for all the issues listed in #11609. It solves the main one, but the others still remain.

Some tests cases that should fail:

../helloworld.js
hello/world.js
/helloworld.js
helloworld.js/
../hello/../world.js
Collaborator

petetnt commented Oct 28, 2015

../ is an valid expression on Windows too. So the function should be re-factored as:

    function getBaseName(fullPath) {
        var lastSlash = fullPath.lastIndexOf("/");
        var preFinalSlash = fullPath.substring(0, lastSlash).lastIndexOf("/");

        if (lastSlash === fullPath.length - 1) {  // directory: exclude trailing "/" too
            return fullPath.slice(fullPath.lastIndexOf("/", fullPath.length - 2) + 1, -1);
        } else if (fullPath.substring(preFinalSlash, lastSlash + 1) === "/../") {
            return fullPath.slice(preFinalSlash + 1);
        }
        return fullPath.slice(lastSlash + 1);
    }

However, it's only a partial fix for all the issues listed in #11609. It solves the main one, but the others still remain.

Some tests cases that should fail:

../helloworld.js
hello/world.js
/helloworld.js
helloworld.js/
../hello/../world.js
@tallandroid

This comment has been minimized.

Show comment
Hide comment
@tallandroid

tallandroid Oct 28, 2015

The fix covers the cases like :
../helloworld.js
../hello/../world.js

hello/world.js and helloworld.js/ are already covered by appshell rename validation logic. /helloworld.js is already covered.

tallandroid commented Oct 28, 2015

The fix covers the cases like :
../helloworld.js
../hello/../world.js

hello/world.js and helloworld.js/ are already covered by appshell rename validation logic. /helloworld.js is already covered.

@petetnt

This comment has been minimized.

Show comment
Hide comment
@petetnt

petetnt Oct 28, 2015

Collaborator

@tallandroid yeah, this PR does fix the original issue that #11609 so maybe the other things I mentioned are out of the scope for this PR (albeit related to my original issue #11609 (comment)).

Which platform are you on? At least on Windows the validation fails with /:s (these are tested against this patch). Check something like this:

image

image

➡️ folder foo doesn't exist, it throws an error

image

Let's create a folder

image

and try that again

image

it actually created the other file too (like one could except if relative paths would be valid filename:s

image

(The foo/bar.js file opens the correct file too, so there's that).

Similar things can be done with things like creating folders by naming a file with ending slash et cetera and it can be abused to infinity. That's why I think it's important that the fix would apply to all the cases regarding /'s in filenames.

Collaborator

petetnt commented Oct 28, 2015

@tallandroid yeah, this PR does fix the original issue that #11609 so maybe the other things I mentioned are out of the scope for this PR (albeit related to my original issue #11609 (comment)).

Which platform are you on? At least on Windows the validation fails with /:s (these are tested against this patch). Check something like this:

image

image

➡️ folder foo doesn't exist, it throws an error

image

Let's create a folder

image

and try that again

image

it actually created the other file too (like one could except if relative paths would be valid filename:s

image

(The foo/bar.js file opens the correct file too, so there's that).

Similar things can be done with things like creating folders by naming a file with ending slash et cetera and it can be abused to infinity. That's why I think it's important that the fix would apply to all the cases regarding /'s in filenames.

harshitr
1) Added / to be an invalid file name character for mac platform
2) Fixed the file rename logic to validate against the newName against base name
@tallandroid

This comment has been minimized.

Show comment
Hide comment
@tallandroid

tallandroid Oct 29, 2015

I get your point. I think it makes more sense to weed out validation against base name completely. That is the safest path to go. I have updated the PR. I would appreciate if you can validate it on windows platform.

I verified the use cases on mac platform.

tallandroid commented Oct 29, 2015

I get your point. I think it makes more sense to weed out validation against base name completely. That is the safest path to go. I have updated the PR. I would appreciate if you can validate it on windows platform.

I verified the use cases on mac platform.

@petetnt

This comment has been minimized.

Show comment
Hide comment
@petetnt

petetnt Oct 29, 2015

Collaborator

Works great for renaming old files on Windows 👍

However, creating a new file and renaming that instantly skips _renameItem all together so at that part exploiting /'s is still possible:

Check out this if-clause

if (renameInfo.type === FILE_CREATING) {
and the method createAtPath that it uses to create the file:
ProjectModel.prototype.createAtPath = function (path) {

Collaborator

petetnt commented Oct 29, 2015

Works great for renaming old files on Windows 👍

However, creating a new file and renaming that instantly skips _renameItem all together so at that part exploiting /'s is still possible:

Check out this if-clause

if (renameInfo.type === FILE_CREATING) {
and the method createAtPath that it uses to create the file:
ProjectModel.prototype.createAtPath = function (path) {

@tallandroid

This comment has been minimized.

Show comment
Hide comment
@tallandroid

tallandroid Oct 29, 2015

I intentionally left out createAtPath workflow as I believe its out of the scope of this PR. I completely understand the workflow mentioned above but I am skeptical of this being called a bug or not. Couple of reasons :

  1. The file gets created at the path intended and gets shown correctly in Working Files section.
  2. FileTreeView shows it as a separate file, but on refresh it gets pushed to the right path, so I am assuming that is what is expected. What is required is refreshing the FileTreeView on create like the way we do it for rename.

I would prefer discussing that over the group once and once confirmed , I myself am willing to make that change too :)

tallandroid commented Oct 29, 2015

I intentionally left out createAtPath workflow as I believe its out of the scope of this PR. I completely understand the workflow mentioned above but I am skeptical of this being called a bug or not. Couple of reasons :

  1. The file gets created at the path intended and gets shown correctly in Working Files section.
  2. FileTreeView shows it as a separate file, but on refresh it gets pushed to the right path, so I am assuming that is what is expected. What is required is refreshing the FileTreeView on create like the way we do it for rename.

I would prefer discussing that over the group once and once confirmed , I myself am willing to make that change too :)

@petetnt

This comment has been minimized.

Show comment
Hide comment
@petetnt

petetnt Oct 29, 2015

Collaborator

Yeah, I am okay with this PR focusing on the renaming files part too 👍

ping @peterflynn @abose @nethip @humphd

Collaborator

petetnt commented Oct 29, 2015

Yeah, I am okay with this PR focusing on the renaming files part too 👍

ping @peterflynn @abose @nethip @humphd

@tallandroid

This comment has been minimized.

Show comment
Hide comment
@tallandroid

tallandroid Nov 5, 2015

Ping. I am not sure abt the turnaround time.

tallandroid commented Nov 5, 2015

Ping. I am not sure abt the turnaround time.

@abose

This comment has been minimized.

Show comment
Hide comment
@abose

abose Dec 8, 2015

Contributor

Sorry for the late response .
@tallandroid @petetnt What is left to be done in this PR?

Contributor

abose commented Dec 8, 2015

Sorry for the late response .
@tallandroid @petetnt What is left to be done in this PR?

@petetnt

This comment has been minimized.

Show comment
Hide comment
@petetnt

petetnt Dec 8, 2015

Collaborator

@abose I think it's ready to be merged, the other related issues can be (and should be) fixed and tracked separately.

Collaborator

petetnt commented Dec 8, 2015

@abose I think it's ready to be merged, the other related issues can be (and should be) fixed and tracked separately.

@tallandroid

This comment has been minimized.

Show comment
Hide comment
@tallandroid

tallandroid Dec 22, 2015

@abose is this done ? Let me know if you are waiting on something.

tallandroid commented Dec 22, 2015

@abose is this done ? Let me know if you are waiting on something.

@abose

This comment has been minimized.

Show comment
Hide comment
@abose

abose Jan 6, 2016

Contributor

Thanks @tallandroid .
Will merge once 1.6 is branched out to release.

Contributor

abose commented Jan 6, 2016

Thanks @tallandroid .
Will merge once 1.6 is branched out to release.

@abose abose added this to the Release 1.7 milestone Jan 6, 2016

@tallandroid

This comment has been minimized.

Show comment
Hide comment
@tallandroid

tallandroid commented Mar 21, 2016

Ping ?

@zaggino zaggino modified the milestones: Release 1.8, Release 1.7 Aug 16, 2016

@zaggino zaggino merged commit 1b006e2 into adobe:master Aug 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zaggino

This comment has been minimized.

Show comment
Hide comment
@zaggino

zaggino Aug 16, 2016

Contributor

Reviewed and merged (as it should have been a while ago).

Contributor

zaggino commented Aug 16, 2016

Reviewed and merged (as it should have been a while ago).

@petetnt

This comment has been minimized.

Show comment
Hide comment
@petetnt

petetnt Aug 16, 2016

Collaborator

Thanks @zaggino and especially @tallandroid for your patience!

Collaborator

petetnt commented Aug 16, 2016

Thanks @zaggino and especially @tallandroid for your patience!

@sdalmeida sdalmeida referenced this pull request Feb 13, 2017

Closed

User is able to create file/folders using relative path #13099

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment