Details
- Reviewers
Gijs - Group Reviewers
extension-reviewers - Commits
- rMOZILLACENTRAL353d7271c0cc: Bug 1891234, additional filename filter checks, r=Gijs,extension-reviewers…
rMOZILLACENTRAL91862a7f6297: Bug 1891234, additional filename filter checks, r=Gijs,extension-reviewers
rMOZILLACENTRAL4b7fe66103e5: Bug 1891234, additional filename filter checks, r=Gijs,extension-reviewers,robwu - Bugzilla Bug ID
- 1891234
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
Event Timeline
Just speaking personally, as a Linux user, I'd generally prefer U+005F to U+0020 as a replacement character.
I would concur with this sentiment, fwiw.
| uriloader/exthandler/nsExternalHelperAppService.cpp | ||
|---|---|---|
| 3680–3681 | Instead of matching that, can we change the extension filter to match the underscore bits? Given the source of this bug that feels safer - or am I missing something here? | |
| uriloader/exthandler/nsExternalHelperAppService.cpp | ||
|---|---|---|
| 3680–3681 | https://bugzilla.mozilla.org/show_bug.cgi?id=1891234#c9 suggests that we shouldn't change the filepicker handling. This part doesn't relate to the underscore change. This handles when the filename is, for example 'sample. png' | |
This seems reasonable to me.
Can we move the new test coverage to a separate patch/bug that we land after this makes it to release, so as not to paint quite such a bulls-eye on the vulnerability?
| uriloader/exthandler/nsExternalHelperAppService.cpp | ||
|---|---|---|
| 3671–3672 | This is a useful statement of fact, but can we extend the comment to include what we're doing in response and why? I _think_ that means something like:
but I'm not sure. | |
This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!
The QuotaManager code seems to have a separate set of constants that it expects to be identical to FILE_ILLEGAL_CHARACTERS but also expects certain characters -- in this case the percent sign -- not to be in that constant. The filenames it uses are escaped urls which often contain percent signs. Fixing that seems to require more work and a better understanding of that code, so I'm just handling the percent sign separately as it is only an issue with the Windows file picker, and will file a separate bug on the QuotaManager sanitization.