-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds mime_type field to resource serializer #1363
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good. But I think we may need tests for the following 2 cases:
- Since you indicated in the serializer that the MIME type is not required, then there should be a test to check that you can create/update a resource that has no MIME type.
- Since only images are expected to have thumbnails, should we have a test that if the specified MIME type is not an image (or is missing), then no thumbnail is created? (What about for the unlikely case that a file's MIME type was updated from a non-image type to an image type?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this address the issues that @amplifi raised here? #1322 (comment)
Resolving this issue will require adding a secure process by which we derive and/or verify the mimetype of the uploaded file, saving mimetype to the resources table in the db (and writing to S3 metadata if not already populated), quarantining files for which a user-specified mimetype doesn't match the derived mimetype and applying additional processing, then generating and displaying the appropriate preview image. Much of this is adjacent to #490.
@linzjax It doesn't address the mime type security issues, but it's an early component for handling proper processing end-to-end. Good looking out! |
@seav: I added two tests, one to verify that you can create a resource without specifying a mime type, and one to verify that no thumbnail is created for files other than images. Since creating a thumbnail is handled in |
@linzjax, 2nd review is all yours! |
Look good 👍 |
d4c6158
to
b9f441b
Compare
Proposed changes in this pull request
When should this PR be merged
Soon; this is important to move forward with data imports.
Risks
None
Follow-up actions
None
Checklist (for reviewing)
General
migration
label if a new migration is added.Functionality
Code
Tests
Security
Documentation