Added filename validation for Mac #7377
Conversation
|
This is for Issue 7326 |
|
I resigned the contributors agreement, that is what caused the build to fail. |
|
Hi @thomastaylor312 |
|
@JeffryBooher I already went and signed it. Did it not go through? |
|
hey, @thomastaylor312. Just checked our database, and I think I see the problem. Can you please re-sign the Agreement again, but where it asks for "Github Username", can you please provide your username without the ampersand? So, for you, please just enter "thomastaylor312", instead of "@thomastaylor312". I think that extra character causes the mismatch in our build system. Thanks in advance. Sorry for the trouble. |
|
@bchintx Not a problem, thank you for letting me know. I signed it again. |
|
@JeffryBooher You mentioned in the issue that you'd like Linux in there as well. The issue made it seem like it was a Mac-only problem. Is it an issue on Linux as well? If it is, I can add it fairly easily and add the commit to this pull request. |
| //Get only the filename (without the final slash) and not the rest of the path | ||
| var filename = selectedPath.substr(selectedPath.lastIndexOf("/") + 1); | ||
| //Validation for file name path only on a Mac | ||
| if (new RegExp("[/?*|:]+").test(filename) && brackets.platform === "mac") { |
There was a problem hiding this comment.
just for mac and linux to exclude files with asterisk and question mark since they may interfere with glob syntax (used in filename/path filtering)
There was a problem hiding this comment.
you don't need to create a new regex object here. /\?|:|\*/.test(filename) works. You also don't need a set or to match on more than one.
There was a problem hiding this comment.
@peterflynn correct me if I'm wrong about the glob-match syntax.
There was a problem hiding this comment.
@JeffryBooher So to make sure I have this right, you only want to match the ?, * , and :, but not the slash or backslash.
There was a problem hiding this comment.
@JeffryBooher So I found that if you use a slash in the native file dialog on a Mac, it converts it to a colon. That would explain why you don't put the slash in the regex. I have made the changes and am pushing the code.
|
@thomastaylor312 Thanks for resigning without the '@' (I meant to say the at-sign, not the ampersand, above.) Your username has been registered, and I re-ran the build check to confirm that you're all set up now. :-) |
|
If we want to test this change on Linux after we merge this code, I can do that. |
| @@ -2287,7 +2287,7 @@ define(function (require, exports, module) { | |||
|
|
|||
| // Init invalid characters string | |||
| if (brackets.platform === "mac") { | |||
| _invalidChars = "?*|:"; | |||
| _invalidChars = "/?:*"; | |||
There was a problem hiding this comment.
We also shouldn't allow the vertical bar | char
…nvalid characters.
|
Is it possible to refactor this code into |
|
Isn't the only other way to create a file in the |
|
Bumping to medium priority since we marked #7326 medium priority. Looks like this stalled a bit, but @thomastaylor312 I think the idea was that we might want to do validation at an even lower level (in the FileSystem code) since no one anywhere in the system should be able to try to read or write a file with an invalid name. @peterflynn I wonder what the best way to do that is - it almost feels like even trying to get a file entry for an invalid path should return null, but that might break a lot of stuff. Maybe the right thing to do is just return an "invalid filename" error (via the standard errback) whenever a caller tries to do an operation on an entry with an invalid filename. (FileSystem.resolve() would obviously do the same already anyway.) |
|
@njx and @peterflynn If you would like help implementing it, I can do it if it will be helpful to you. |
|
@njx FileSystem.resolve() already throws in certain invalid-filename cases, so we could just extend that precedent. We could also offer a simple boolean validation call, which might be useful e.g. for callers who want to continuously validate filenames while the user is typing without actually creating a File entry for each valid filename the user transitions through. |
| */ | ||
| function checkForValidFilename(filename, isFolder) { | ||
| // Validate file name | ||
| if ((filename.search(new RegExp("[" + _invalidChars + "]+")) !== -1) || filename.match(_illegalFilenamesRegEx)) { |
There was a problem hiding this comment.
I'm not sure why you're creating a new RegExp each time. You can just have the initialization code below set _invalidChars directly to a regexp instance (e.g. _invalidChars = /[\/?:|*]/; for the Mac case) and then reuse it every time.
| if ((filename.search(_invalidChars) !== -1) || filename.match(_illegalFilenamesRegEx)) { | ||
| //Invalid chars string for dialog box | ||
| var invalidCharsString = _invalidChars.source.substr(1, _invalidChars.source.indexOf(']') - 1).replace(/\\/g, ''); | ||
| Dialogs.showModalDialog( |
There was a problem hiding this comment.
The caller should decide whether to show the dialog or not. Utility function should avoid taking user-visible actions.
There was a problem hiding this comment.
So should this be a flag in the function or let whatever calls it handle the dialog creation?
There was a problem hiding this comment.
The latter is the best option, IMO, -- just let the caller handler the return value. No need to have any visualization logic in this function.
There was a problem hiding this comment.
I agree, the only problem I have is how we are supposed to display the invalid characters string. Should that be added as a property in FileUtils?
There was a problem hiding this comment.
Yes, just export the string that you already have defined (and remove @Private from the jsdoc).
…e dialog logic out of the method. Also added unit tests for the checkForValidFilename method.
|
@peterflynn As it stands now, the |
|
Looks like this is assigned to @bchintx but is waiting on an answer from @peterflynn. It looks like this now has a general function that can be used everywhere, which seems right - do we also want to enforce this in the FileSystem, or should we just merge this as-is and leave that till later? @thomastaylor312 - thanks for the unit test. I know this has been dragging on awhile :), but it might be nice to have a longer list of valid/invalid test cases (especially since it's pretty easy to add tests for them), just in case we have to make more changes to this later and want to make sure we don't break existing cases if we fiddle with the regexps. |
|
@njx I will write up some more unit tests. Quick question on that, how can I test Windows when on a Mac and vice versa? The regexps will be different in each case. |
|
A couple quick thoughts:
|
|
One reason why you shouldn't put validation code in the FileSystem is because it will error opening a project that has an illegal filename (or what we think is an illegal filename). I tried this with paths ending in dots and it wouldn't let me open the projects that had filenames or folders with dots. This is bad because if you name a file 'aux` on the Mac (which is legal) then performing the validation in FileSystem.js, it will cause the project to not load when it opens and that's not what we want. There may be some refactoring that we can do to FileSystem to make this work but, out of the box, the validation handler in FileSystem will prevent the projects from loading when the validation check fails. I think the refactoring by moving the validation code out of ProjectManager is fine but, IMO, we should just check for backslash, question-mark, and asterisk characters and that's all in the File Save/Save As command handlers. And we need only do this on the Mac and Linux because Windows' File Save dialog validation code already does this so we don't need to check it again. Here's why we only need to check those: Asterisk and question-mark may not work with our Backslash because our path parsing library may end up converting backslashes to forward slashes. I'm not sure these are all being done on a per-platform basis at the moment and we cannot guarantee that extension writers aren't just blindly converting backslashes to forward slashes either. This change has the validator checking for the same values that you can't use from Project Manager and I think that we don't want to do that here. Let the user name the file whatever they want and the Save As dialog should validate it. We don't have an OS level validator for project manager so we check for all possible illegal values. |
|
@peterflynn and @JeffryBooher So just for clarity sake (and so I don't make any unneeded changes to the code) can you guys summarize the changes you want made and what things you want to stay the same? I believe I understand the things we want to change. I just want to be sure I am on the same page with everyone. |
|
Just check for: asterisk, backslash, and questionmark |
|
@JeffryBooher Did we want to pull out the dialog box creation and put it in the FileUtils like @peterflynn suggested as well? |
|
@thomastaylor312 yeah we should clean up the duplicate instances to make it easier to read and port. |
|
@JeffryBooher I wasn't suggesting FileSystem enforce the validating -- just that we have the validation code live there, since that makes more sense than FileUtils. Re only doing validation on Mac & Linux -- I'm confused why you'd say that, since your fix in #7772 relies on using this exact same validation code on Windows. If you're suggesting splitting out a "basic" validator that we check in Save As and a "full" validator that we check only for the project tree, I guess I'm not sure what the value is in doing that. It's not terrible to re-run the same validation that the Save dialog does natively on one platform, and if we have to maintain the "full" validator for one use case no matter what, we might as well just use it everywhere (instead of also maintaining a second validator). |
|
@peterflynn It makes perfect sense to have all of the file validation code live in file utils. I tried adding to the existing validator and it broke the open project. You're right -- we could check for *? on all platforms without any issue. Windows already checks this so I figured we didn't need to in that case. As for running the full v. limited validation check in save as -- I remember having a conversation a while back that if users really want to name something that isn't cross platform then we shouldn't prevent them if they use the OS to do so. I was just thinking since they are using an OS Save As dialog then we should let them do whatever they want as long as the OS allows for it. But I'm OK with doing the full Validation on save if you think users are OK with it in that context. If definitely feels like we should be consistent. |
… changed the invalid characters.
|
Ok, I made the changes requested including the added unit tests. Let me know what can be improved. |
|
@peterflynn or @JeffryBooher Any comments/updates on this? |
|
@thomastaylor312 you need to merge master into your branch since #7772 has already been merged. It looks like it have broken your fix as well. |
…ect new filename validations Conflicts: src/project/ProjectManager.js
|
Ok @JeffryBooher I think I got everything merged ok. Let me know if you see any problems. |
|
Any updates? |
|
Sorry, a lot of us are out at JSConf this week. Will take a look when we can. |
|
Ah, not a problem. I was going out of town soon too and wanted to make sure On Tuesday, May 27, 2014, Narciso Jaramillo notifications@github.com
|
|
@thomastaylor312 Thanks for going thru all this trouble. Your implementation looks good to me. I've tested it on both Windows and Mac, and check that it's catching the invalid characters that were noted in the original issue #7326. Unfortunately, having said that, now that I'm actually exercising which characters are valid and which are not, I'm not sure that we should be doing this extra validation. In particular, it appears that OSX actually fully supports both astericks and the question mark in filenames. For example, I can create/rename files using these characters both using the Finder and on the command-line in a Terminal window. It would appear that OSX only prohibits certain characters, like the colon or names that start with a period. However, our current implementation already checks for these conditions. I'm really sorry for not catching this earlier, but I don't think we actually need this PR and that we should close Issue #7326 as not a bug b/c it's already working correctly. @JeffryBooher perhaps I'm missing something? |
|
Closing this Pull Request after further discussion reveals that Issue #7326 isn't necessary. @thomastaylor312 Again, my apologies for asking you to go thru all this work. Good work though. |
|
@bchintx No problem. Thanks! |
The ProjectManager change was to add a check for the backslash when renaming inline as well.