Uh oh!
There was an error while loading. Please reload this page.
Do not allow RTLO char in filenames - #10030
Conversation
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
| * @dataProvider providesInvalidUnicode | ||
| */ | ||
| public function testPathVerificationInvalidUnicode(string $fileName) { | ||
| $this->expectException(InvalidPathException::class); |
There was a problem hiding this comment.
Aren't you throwing an InvalidCharacterInPathException instance?
There was a problem hiding this comment.
I see that it's a specialization but I just wanted to bring it up in case this wasn't intentional 😉
There was a problem hiding this comment.
Yeah we are. But it is internal mainly. The point here is that we get that the path is invalid :)
nickvergessen
commented
Jun 28, 2018
Hmm I guess it's fine, but of course it could cause problems in RTL countries if people name their files in their language, which is why I didn't plan to do this yet. Just imagine you would have to name your files |
| } | ||
| // Do not allow RTLO char | ||
| if (mb_strpos($fileName, mb_chr(8238, 'utf8')) !== false) { |
There was a problem hiding this comment.
mb_chr is php 7.2+ so we can't use it
There was a problem hiding this comment.
rullzer
commented
Jun 28, 2018
mmm fair enough. Maybe then we should just forbid the char anywhere but the first char? |
jaller94
commented
Jun 29, 2018
I don't see an issue connected to this PR. |
nickvergessen
commented
Jul 2, 2018
Related forum topic: https://help.nextcloud.com/t/need-input-from-rtl-people/33444 Yeah, there is a non-public issue about this, but we are still trying to find the best way to deal with this. |
MorrisJobke
commented
Jul 26, 2018
As @nickvergessen already wrote in #10410 (comment): maybe it's better to not go for that approach. |
alexacra
commented
Jul 29, 2018
If you allow RTLO, please consider, that it can be used to spoof file names by malicious users.
Source: https://resources.infosecinstitute.com/spoof-using-right-to-left-override-rtlo-technique-2/ |
nickvergessen
commented
Jul 30, 2018
yes @alexara that was exactly why this PR was started. But we asked users of RTL languages for feedback and they are using mixed and RTL only names, so we would cause a lot of problems for them, to prevent fraud by your colleagues and friends, people that share the instance with you. Which is why I vote for no |
Signed-off-by: Roeland Jago Douma roeland@famdouma.nl