Skip to content

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.

Backlog Refinement by @alexkalderimis:

Documentation 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).

Backlog Refinement by @.luke:

Documentation 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

Backlog Refinement by @toupeira:

Documentation 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

Edited by Darva Satcher