[CLOSED] Fix to prevent moving files by renaming files using FileTreeView

[core-ai-bot]

Issue by tallandroid
Wednesday Oct 28, 2015 at 10:13 GMT
Originally opened as adobe/brackets#11862

---

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

---

tallandroid included the following code: https://github.com/adobe/brackets/pull/11862/commits

[core-ai-bot]

Comment by petetnt
Wednesday Oct 28, 2015 at 10:49 GMT

---

../ 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

[core-ai-bot]

Comment by tallandroid
Wednesday Oct 28, 2015 at 18:27 GMT

---

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.

[core-ai-bot]

Comment by petetnt
Wednesday Oct 28, 2015 at 19:23 GMT

---

@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 adobe/brackets#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:

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

Let's create a folder

and try that again

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

(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.

[core-ai-bot]

Comment by tallandroid
Thursday Oct 29, 2015 at 07:47 GMT

---

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.

[core-ai-bot]

Comment by petetnt
Thursday Oct 29, 2015 at 08:03 GMT

---

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 https://github.com/adobe/brackets/blob/d939740fd5c368c347ac6a8c7cef2941981aa2fb/src/project/ProjectModel.js#L959 and the method createAtPath that it uses to create the file: https://github.com/adobe/brackets/blob/d939740fd5c368c347ac6a8c7cef2941981aa2fb/src/project/ProjectModel.js#L992

[core-ai-bot]

Comment by tallandroid
Thursday Oct 29, 2015 at 08:13 GMT

---

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 :)

[core-ai-bot]

Comment by petetnt
Thursday Oct 29, 2015 at 08:17 GMT

---

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

ping@peterflynn@abose@nethip@humphd

[core-ai-bot]

Comment by tallandroid
Thursday Nov 05, 2015 at 02:33 GMT

---

Ping. I am not sure abt the turnaround time.

[core-ai-bot]

Comment by abose
Tuesday Dec 08, 2015 at 11:52 GMT

---

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

[core-ai-bot]

Comment by petetnt
Tuesday Dec 08, 2015 at 12:24 GMT

---

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

[core-ai-bot]

Comment by tallandroid
Tuesday Dec 22, 2015 at 20:16 GMT

---

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

[core-ai-bot]

Comment by abose
Wednesday Jan 06, 2016 at 04:10 GMT

---

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

[core-ai-bot]

Comment by tallandroid
Monday Mar 21, 2016 at 20:34 GMT

---

Ping ?

[core-ai-bot]

Comment by zaggino
Tuesday Aug 16, 2016 at 05:36 GMT

---

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

[core-ai-bot]

Comment by petetnt
Tuesday Aug 16, 2016 at 05:39 GMT

---

Thanks@zaggino and especially@tallandroid for your patience!