CarrierWave::IntegrityError when uploading designs with unsupported file formats
Summary
See #227175 (comment 378761858).
The exception is handled and is being sent to Sentry #227175 (comment 380255217).
https://sentry.gitlab.net/gitlab/gitlabcom/issues/1647620/?referrer=gitlab_plugin
CarrierWave::IntegrityError: file format is not supported. Please try one of the following supported formats: image/png, image/jpeg, image/bmp, image/gif
(92 additional frame(s) were not displayed)
...
carrierwave/uploader/cache.rb:137:in `cache!'
with_callbacks(:cache, @file) do
carrierwave/uploader/callbacks.rb:14:in `with_callbacks'
self.class._before_callbacks[kind].each { |c| send c, *args }
carrierwave/uploader/callbacks.rb:14:in `each'
self.class._before_callbacks[kind].each { |c| send c, *args }
carrierwave/uploader/callbacks.rb:14:in `block in with_callbacks'
self.class._before_callbacks[kind].each { |c| send c, *args }
content_type_whitelist.rb:34:in `check_content_type_whitelist!'
raise CarrierWave::IntegrityError, message
CarrierWave::IntegrityError: file format is not supported. Please try one of the following supported formats: image/png, image/jpeg, image/bmp, image/gif
Proposal
We should catch this error at the API layer and return an appropriate response.
@alexkalderimis:
Backlog Refinement byDocumentation needed:
None needed. We already document the kinds of files we support.
Test Activity Planned:
GraphQL request tests for handling illegal file-types.
Add tests in the NewVersionWorker
for similar scenarios.
Security Tests Planned:
This test is essentially entirely about security.
Explanation for Approach to be taken:
Either we just add tests for this scenario and encode the current behavior as defined behavior, or we aim to capture this error and report to the user.
Currently this does not cause errors during version creation, where we should
catch it, but during image processing in the NewVersionWorker
. If we capture
this state in version creation, then we can avoid needed changes to
NewVersionWorker
, but if the range of types is not co-incident, then we may
need changes to both.
This is somewhat odd, since we currently do check the mimetype of the file
during the NewVersionWorker
before processing (see
design_management/generate_image_versions_service.rb#L49
in
#generate_image
), so this is either a case of a mismatch between the values of
DesignManagement::DesignV432x230Uploader::MIME_TYPE_WHITELIST
and the logic of
#content_type_whitelist.rb#L34
in check_content_type_whitelist!
or something
subtler, such as the content not matching the declared mimetype.
Since this is a very low incidence event, and encountered during offline processing, tolerating some level of failure is likely acceptable.
MR Breakdown:
A single MR, if needed.
Weight Estimate:
2 to eliminate the error, otherwise 1 to tolerate it (possibly with test).
@.luke:
Backlog Refinement byDocumentation needed: N
Test Activity Planned: Change the test of GenerateImageVersionsService
to test for logging
Security Tests Planned: N
Explanation for Approach to be taken: Switch to logging the error #227175 (comment 381333333)
MR Breakdown: 1 MR
Weight Estimate: 1
@toupeira:
Backlog Refinement byDocumentation needed: N
Test Activity Planned: Test the worker with an invalid file.
Security Tests Planned: n/a
Explanation for Approach to be taken: This is an expected error, so I think we just need to catch it to avoid the worker retrying multiple times.
MR Breakdown: 1 MR
Weight Estimate: 1