Add lenient file-type checking mode #10862 #10863
Conversation
There was a problem hiding this comment.
I like where this is heading, but I do have some concerns here.
One of the problems with file type checking (and the primary reason that it's optional) is because the filetype guesser is not exhaustive. There are lots of file types that it will not (and cannot) validate. A simple example is a text file with a txt extension. So if file type checking is permanently true, even if you add "txt" to FILE_TYPES - it will be returned as "unknown" and validation will fail. See test_unknown_filetypes in file_validator.py. We do some manual checking of xlsx and csv in there for this reason (they cannot be validated any other way - and this is admittedly not the best way to validate them - that is, actually reading potentially malicious content into the csv reader to verify that the file is safe!). I don't know of a good way around this issue. We could potentially allow for custom validation of files. That is to say, if you want a txt file, you can write a custom validator for said txt files and use your own logic to verify that it's acceptable. Maybe this is not an issue that needs to be solved now, since the option is still around until 8. But if we are telling people to turn it on to test it - a subset of users are almost certain to do so in production, in which case we're going to get questions on why files are not uploading after enabling the setting we warned them to enable.
|
What about keeping the option and having three values:
We could deprecate any value other than None, "lenient", or "strict" and then map falsy values to None, and truthy values to "strict" until the deprecation period is over, and then set the new project template to "lenient". That would solve the basic use case sending me here: we have useful checks hidden behind this setting, and we shouldn't need to off all of them just to let a few useful things through. |
jacobtylerwalls
changed the title
FILE_TYPE_CHECKING = False #10862
jacobtylerwalls
force-pushed
the
jtw/deprecate-file-type-checking-setting
branch
from
62e82f4
to
bcaae04
Compare
| try: | ||
| self.read_zip_file(content, result) | ||
| except zipfile.BadZipFile as e: | ||
| return { | ||
| "success": False, | ||
| "data": FileValidationError( | ||
| message=e.args[0], | ||
| code=400, | ||
| ), | ||
| } |
There was a problem hiding this comment.
Noticed that in #10885 I forgot to surface this error in the UI where FILE_TYPE_CHECKING is false and we go straight to trying read a file as a zip file.
Follow-up to 4c14610.
| @@ -312,6 +312,11 @@ def test_save_provisional_from_athoritative(self): | |||
|
|
|||
| provisionaledits = provisional_tile.provisionaledits | |||
| self.assertEqual(tiles.count(), 2) | |||
| self.assertIn( | |||
There was a problem hiding this comment.
Unrelated, but not really worth a whole new PR for it: follow-up to #10860 where I wanted to add some more debug output, but was stymied by the KeyError being raised much earlier.
|
Thanks for the review! |
Types of changes
Description of Change
There are several useful safety checks guarded behind the FILE_TYPE_CHECKING setting, not all of which have to do with the file type, strictly speaking.
The main reason this was off by default was the lack of a "lenient" option to allow through files whose extensions cannot be determined.
Add this lenient mode, default new projects to it, and deprecate the old true/false values.
Issues Solved
Closes #10862
Checklist
Ticket Background
Demo
Run any management command (runserver, check, test, etc.) with
python -Wall::DeprecationWarning manage.py ...