Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 150 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
I saw that files types are handled differently for |
|
Should I completely remove type |
We can make sure to set |
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
Looks good overall, but there's a couple of details we should iron out.
It's probably fine to remove it for now. There may however be the rare false positive for any files that cannot be identified correctly by libmagic. Generally pdfs, zips and any other binary file formats can end up as |
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
We're in a pretty good spot now. But we should clean up and refactor things a bit.
If mimetypes is an argument to upload fields, we no longer need to pass around instances of WhitelistedMimeType everywhere, we should use the new parameter and avoid inserting a validator object, instead let's rely on the post_validate method of Field, which we can override in UploadField (we don't need to override it in UploadMultipleField, but we should add an explicit mimetypes parameter, so we can easily access mimetypes from UploadMultipleField.mimetypes in addition to each subfield, this means this parameter should both be assigned to self.mimetypes and passed to the self.upload_field_class call.
src/onegov/form/fields.py
Outdated
| if not any(isinstance(validator, WhitelistedMimeType) | ||
| for validator in validators | ||
| ): | ||
| validators.append(validator) |
There was a problem hiding this comment.
I think it would still be a bit cleaner to move the validator into post_validate like in my code suggestion above, instead of trying to modify the given validator chain. If someone passes in a WhitelistValidator we can emit a warning or raise an exception to use the mimetypes parameter instead.
src/onegov/form/fields.py
Outdated
| id=id, | ||
| default=default, | ||
| widget=widget, # type:ignore[arg-type] | ||
| validators=[*(validators or ())], |
There was a problem hiding this comment.
You're just looking for the validator in the wrong place in that test. The validator is on each field in the field list, not the field list itself. So I would get rid of this again and simplify the validators, otherwise the validator will run twice for each file. Once you move the validation from a validator in validators to post_validate you no longer will have to check for the presence of that validator anyways, since it's baked into the field itself.
…st, it's good enough if it works for each field in the list. Co-authored-by: David Salvisberg <[email protected]>
…ub.com:OneGov/onegov-cloud into feature/ogc-2738-pentest-arbitrary-file-upload
|
I would suggest reverting 01b0af2, where you changed some things into sets, it should no longer be necessary, now that the argument type on the field is correct and we definitely don't want some of these things to appear as mutable. |
This reverts commit 01b0af2.
|
I believe I am ready for a final review |
2 participants
Org: Ensure mime type validation on file upload fields in form code
TYPE: Feature
LINK: ogc-2738