Sanitise the note's path #148
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Henni
reviewed
Nov 13, 2017
| \xF0[\x90-\xBF][\x80-\xBF]{2} # planes 1-3 | ||
| | [\xF1-\xF3][\x80-\xBF]{3} # planes 4-15 | ||
| | \xF4[\x80-\x8F][\x80-\xBF]{2} # plane 16 | ||
| )%xs', '', $str); |
There was a problem hiding this comment.
Would you mind adding some explanatory comments to this regex magic?
| @@ -222,9 +246,6 @@ private function getSafeTitleFromContent($content) { | |||
| $title = $this->l10n->t('New note'); | |||
| } | |||
|
|
|||
| // prevent directory traversal | |||
| $title = str_replace(array('/', '\\'), '', $title); | |||
There was a problem hiding this comment.
Did you look at the core code and how it is prevented there?
I would prefer if we wouldn't have to implement this ourselves...
|
Thanks for reviewing this and the other PRs.
Some background information on this one:
I checked the core code for possible exceptions during rename. There, several types of checks are executed (see lib/private/files/Storage/Common.php, verifyPath()). However, if one of the checks fails, an Exception is thrown. Catching those exceptions is not sufficient for us, since renaming is then aborted and the note's title is left in its old state which confuses the user who didn't do anything wrong (e.g. used a unicode smiley in the note's first line).
Therefore, I took those checks and eliminate everything during title generation that leads to an Exception. The magic regexp comes from a check from core, too (eliminate 4byte Unicode characters if MySQL can't save them). With this approach, the user can write everything in the first line of a note and the app only uses suitable parts for the file name.
I think we have to go this approach in order to provide a good user experience. But, of course, I can improve the documentation of the respective code.
|
|
@korelstar thanks for the clarification. |
|
done |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've implemented some enhancements in order to let the notes app to be more fail-safe.
Math <> Philosophyon a (linux) server, but when the user wants to synchronize the account with a Windows system, synchronization fails. Therefore, we should remove all characters that lead to failed synchronization (fixes Solution for reserved characters in filenames notes-android#228).NotPermittedException(user has not the right to write to a (shared) folder), but other exceptions can happen, too (e.g.InvalidPathExceptionif the filename is invalid due to several reasons). Those are now catched, too. This allows the note content to be saved while although renaming has failed.InvalidPathExceptionis thrown, is when using an emoji in the title and having a MySQL database without 4-byte support. The new functionsanitisePathremoves such illegal characters from the path in such a case (fixes Creating a note with emoji in the title constantly spawns New Note, New Note (2) etc while trying to sync notes-android#237).