fix(file_upload): validate mimetype as configured #1459
Conversation
qvalentin
force-pushed
the
fix/mimetype-validation-file-upload
branch
from
08f1f68
to
2e4689b
Compare
qvalentin
force-pushed
the
fix/mimetype-validation-file-upload
branch
from
2e4689b
to
f094ebc
Compare
dosubot
bot
added
size:L
dokterbob
added
evaluate-with-priority
There was a problem hiding this comment.
Tests look very clean. Thanks this already great as it is, but I would prefer some slight changes to validation architecture -- separating HTTP logic from validation logic. Important as we'll add more such functionality going forward, good to have the API clean now.
| file_response = await session.persist_file( | ||
| name=file.filename, content=content, mime=file.content_type | ||
| ) | ||
|
|
||
| return JSONResponse(content=file_response) | ||
|
|
||
|
|
||
| def validate_file_upload(file: UploadFile): | ||
| if config.features.spontaneous_file_upload is None: | ||
| return # TODO: if it is not configured what should happen? |
There was a problem hiding this comment.
The most elegant solution is to change the default value for config.features.spontaneous_file_upload to always be populated with SpontaneousFileUploadFeature then to have enabled default to False and move max_files and max_size_mb default values from the frontend to the backend config. This would remove the if ... is None in favour of more explicit code. But I get it if perhaps that's out of scope and left for a later PR.
The second best would be to follow the frontend in this and not allow uploads unless explicitly enabled. Definitely, we should try to avoid having a TODO in a PR. ;)
| file_response = await session.persist_file( | ||
| name=file.filename, content=content, mime=file.content_type | ||
| ) | ||
|
|
||
| return JSONResponse(content=file_response) | ||
|
|
||
|
|
||
| def validate_file_upload(file: UploadFile): |
There was a problem hiding this comment.
I would prefer to have validation functions raise a well-defined Exception and to have a docstring specifying it's behaviour.
For example, in pydantic this is a ValueError and in Django it's a ValidationError. It should definitely not be a HTTPException as the file validation code should not have anything to do with HTTP, it's just validating an UploadFile.
We can catch validation errors and then re-raise them in a HTTP request handler (upload_file).
|
|
||
| def validate_file_mime_type(file: UploadFile): | ||
| if config.features.spontaneous_file_upload.accept is None: | ||
| return |
There was a problem hiding this comment.
Maybe add an explicit comment here that we'll allow any mime type unless a value is defined.
| validate_file_size(file) | ||
|
|
||
|
|
||
| def validate_file_mime_type(file: UploadFile): |
| for pattern in accept: | ||
| if fnmatch.fnmatch(file.content_type, pattern): | ||
| return | ||
| else: |
There was a problem hiding this comment.
It's either a list or a dict. Maybe add an assertion here so that assumption is clear (and the code will bulk if we ever haphazardly decide to change the typing).
| @@ -1,19 +1,24 @@ | |||
| import datetime # Added import for datetime | |||
There was a problem hiding this comment.
Maybe it's a good time to remove this redundant comment. ;)
dokterbob
added
the
blocked
Where would you put the validation logic? |
This is what I referred to in that comment: #1459 (comment) |
#1458