Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

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

Closed
3 tasks done
sdalmeida opened this issue Feb 13, 2017 · 12 comments
Closed
3 tasks done

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

sdalmeida opened this issue Feb 13, 2017 · 12 comments
Assignees

Comments

@sdalmeida
Copy link
Contributor

sdalmeida commented Feb 13, 2017

Prerequisites

  • Can you reproduce the problem with Debug -> Reload Without Extensions?
  • Did you perform a cursory search to see if your bug or enhancement is already reported?
  • Did you read the Troubleshooting guide?

Description

This bug was first found by @Pomax(mozilla#434) and then fixed by @tallandroid (#11862).

"It's possible to right-click in the file tree and create a file (or folder) name it ../something, and the path isn't resolved, but rather the .. and / characters are treated as literals to be used in the filename itself." - @humphd(#11609)

It looks like this bug wasn't fully fixed. I'm using the latest version of Brackets and I'm still able to reproduce it.

screen shot 2017-02-12 at 12 30 43 pm
screen shot 2017-02-12 at 12 31 40 pm

Steps to Reproduce

  1. Create/Rename a file
  2. Give it a valid relative path with a unique file name (Eg. ../../file1)
  3. Save it
  4. File is now created in whatever path you gave it

Expected behavior: An error message should show complaining about the relative path.

Actual behavior: File is renamed/created without an error message.

Versions

OS: macOS sierra 11.2.3
Brackets Version: 1.8 build 1.8.0-17108

@ficristo
Copy link
Collaborator

Do you want to take a look at this?

@tallandroid
Copy link

@Simon66 Thanks for bringing it up. I can take a look into this later this week. If you want to pick it up before that , feel free to pick it up.

@Pomax
Copy link

Pomax commented Feb 14, 2017

@Simon66 is a student currently helping us (the Mozilla Foundation) run through a bug list for https://thimble.mozilla.org, which relies on brackets for its code editor part (it's on 1.4 at the moment but we're uplifting to 1.8), finding and fixing issues that are good first bugs for first time contributors. In this particular case, unless simon wants to dive very deep, I'd recommend having someone more knowledgeable w.r.t. the Brackets codebase look at this particular issue =)

@humphd
Copy link
Contributor

humphd commented Feb 14, 2017

I think it's fine if he wants to try it. @Simon66 I'd suggest beginning with converting your STR above to a failing test case or two in https://github.com/tallandroid/brackets/blob/master/test/spec/ProjectModel-test.js, and once you can get your tests to fail, move on to a fix.

@sdalmeida
Copy link
Contributor Author

@ficristo Yep, I'd like to fix this issue, I'll take @humphd approach and once everything is done, I'll do a PR 👍
I'll have my PR later on this week :)

@sdalmeida
Copy link
Contributor Author

Hi @humphd
Looking at the test files, I noticed that there are a collection of file tests already in https://github.com/tallandroid/brackets/blob/master/test/spec/FileSystem-test.js
After running Jasmine Spec runner on "FileSystem", it shows that the test passed.
Where should the test case go? Under FileSystem-test.js or ProjectModel-test?
Thanks :)

@zaggino
Copy link
Contributor

zaggino commented Feb 20, 2017

@Simon66
Where you put the tests depends mostly on where you fix the code. If you fix the code in FileSystem.js you'd put the tests in FileSystem-test.js, when you fix ProjectModel.js you'd put that into ProjectModel-test.js. It's a rough guideline and of course there are exceptions to this but you should see what the other tests are testing and see where your test fits the best. Since this is a file tree issue, I'd also look if your test doesn't fit into FileTreeView-test.js suite.

@sdalmeida
Copy link
Contributor Author

sdalmeida commented Feb 24, 2017

just wanted to update my progress on this bug. My main development unit died last week and I'm waiting for it to be repaired (I should've backed up my data haha). I should be getting it back today. I'll continue working on it as soon as I get my computer back :)

Also, thanks @zaggino for letting me know where I can put my test cases 👍

@ficristo
Copy link
Collaborator

@Simon66 thank you for the update!

@Pomax
Copy link

Pomax commented Feb 24, 2017

@Simon66 that's the nice thing about github forks: remember to push to your repo often, even if your code is "not ready for a PR" yet - you can't lose work that way, even if your computer catches fire =)

(although hopefully of course they were able to fix your machine with all the data intact)

@sdalmeida
Copy link
Contributor Author

sdalmeida commented Apr 2, 2017

Hey guys! I'm back from the "dead" haha
Wow! What a headache :/ Lesson learned, always "push" your modifications.

Going back to the code, why is the invalid chars different based on the OS?
screen shot 2017-04-02 at 12 06 44 am

Why is < and > valid on MacOS and Linux but invalid on Windows? Shouldn't they be invalid for every OS?

Also, the reason why this bug still exists is because .. is not being checked.

screen shot 2017-04-02 at 12 12 14 am

These are the chars that I believe should be invalid:

  • \?
  • \\
  • \*
  • \.{2,} <-- .. or more should not be allowed, but allow .
  • \.$ <-- allow a file to have . but not end with .
  • \/
  • \|
  • \>
  • \<

Are there any others chars that should be added/removed?

Also, my regex is not so good, but this is the pattern that I wrote:
([\?\*\|\:\/\<\>]+|\.{2,}|\.$) <-- I know I don't have to escape every char but for consistency I always do :)

Any help on writing a better pattern? What do you think?
Thanks :)

@zaggino
Copy link
Contributor

zaggino commented Apr 2, 2017

Hi, I agree the list should be same for every OS as generally you don't want someone to push an invalid file to a project which other developer on different OS can't use.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants