Sanitize bidirectional characters from file names. #3744
Merged
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.
Resolves: https://github.com/nextcloud-gmbh/h1/issues/453
Prerequisite: nextcloud/NextcloudKit#185
This PR fixes the issue of maliciously spoof naming file names that can trick people into opening a file with a fake extension.
Android released a fix here: nextcloud/android#15388 and their approach is to use 2 text views to separate the string, so the extension part is not affected by the bidi chars.
On iOS however having 2 text views on so many old VCs and Storyboards will cause a lot of issues down the line, especially since now we have ~5 different layout styles for displaying files (list, icons, square grid, aspect ratio grid, recommended files). We also display file names in many other screens and views, and IMO handling all of them was not feasable.
Instead I use a way of clearing all previous bidi overrides and forcing LTR on the extension. This way the file name stays intact, but the extension is sanitized:
A few issues along the way that I fixed:
I added a custom truncator that splits the text down the middle but preservers the extension + its own BIDI isolates at the end. One of the main points of the original issue is that we should not modify the base name when sanitizing it. However truncating is an exception, as iOS already modifies it by removing parts of it. I do what is otherwise inevitable while still preserving the extension.
There are a few caveats + TODOs:
Important to note that this would have also been an issue if I used 2 text views, as I am not aware of any non-headache inducing way to align a multi-line text view with another one at the precise place where the extension must be shown.
Comparisons to ensure it works the same:
LTR:
RTL: