diff --git a/app/uploaders/base_uploader.rb b/app/uploaders/base_uploader.rb index 8fe367e5e..708943b9a 100644 --- a/app/uploaders/base_uploader.rb +++ b/app/uploaders/base_uploader.rb @@ -2,6 +2,10 @@ class BaseUploader < CarrierWave::Uploader::Base include CarrierWave::BombShelter # limits size to 4096x4096 include CarrierWave::MiniMagick # adds processing operations + FRAME_MAX = 1000 + FRAME_STRIP_MAX = 150 + + process :validate_frame_count process :strip_exif def store_dir @@ -22,9 +26,19 @@ class BaseUploader < CarrierWave::Uploader::Base # strip EXIF (and GPS) data def strip_exif manipulate! do |image| - image.strip + image.strip unless image.frames.count > FRAME_STRIP_MAX image = yield(image) if block_given? image end end + + def validate_frame_count + begin + return unless MiniMagick::Image.new(file.path).frames.count > FRAME_MAX + rescue Timeout::Error + raise CarrierWave::IntegrityError, "Image processing timed out." + end + + raise CarrierWave::IntegrityError, "GIF contains too many frames. Max frame count allowed is #{FRAME_MAX}." + end end diff --git a/config/initializers/carrierwave.rb b/config/initializers/carrierwave.rb index d973f06fb..be3154031 100644 --- a/config/initializers/carrierwave.rb +++ b/config/initializers/carrierwave.rb @@ -1,3 +1,11 @@ +require "mini_magick" + +# Carrierwave uses MiniMagick for image processing. To prevent server timeouts +# we are setting the MiniMagick timeout lower. +MiniMagick.configure do |config| + config.timeout = 10 +end + CarrierWave.configure do |config| if Rails.env.test? config.storage = :file diff --git a/spec/fixtures/files/high_frame_count.gif b/spec/fixtures/files/high_frame_count.gif new file mode 100644 index 000000000..b36b58a64 Binary files /dev/null and b/spec/fixtures/files/high_frame_count.gif differ diff --git a/spec/uploaders/article_image_uploader_spec.rb b/spec/uploaders/article_image_uploader_spec.rb index 08a846fea..e44f7848f 100644 --- a/spec/uploaders/article_image_uploader_spec.rb +++ b/spec/uploaders/article_image_uploader_spec.rb @@ -9,6 +9,7 @@ describe ArticleImageUploader, type: :uploader do let_it_be(:image_png) { fixture_file_upload("files/800x600.png", "image/png") } let_it_be(:image_webp) { fixture_file_upload("files/800x600.webp", "image/webp") } let_it_be(:image_with_gps) { fixture_file_upload("files/image_gps_data.jpg", "image/jpeg") } + let_it_be(:high_frame_count) { fixture_file_upload("files/high_frame_count.gif", "image/gif") } # we need a new uploader before each test, and since the uploader is not a model # we can recreate it quickly in memory with `let!` @@ -59,13 +60,36 @@ describe ArticleImageUploader, type: :uploader do end end + describe "frame validation" do + it "raises an error if frame count is > FRAME_MAX" do + stub_const("BaseUploader::FRAME_MAX", 20) + + expect { uploader.store!(high_frame_count) }.to raise_error(CarrierWave::IntegrityError, /too many frames/) + end + + it "raises a CarrierWave error which can be parsed if MiniMagick timeout occurs" do + allow(MiniMagick::Image).to receive(:new).and_raise(TimeoutError) + + expect { uploader.store!(image_jpg) }.to raise_error(CarrierWave::IntegrityError, /Image processing timed out/) + end + end + describe "exif removal" do - it "removes EXIF and GPS data on upload" do + it "removes EXIF and GPS data on single frame image upload" do expect(EXIFR::JPEG.new(image_with_gps.path).exif?).to be(true) expect(EXIFR::JPEG.new(image_with_gps.path).gps.present?).to be(true) uploader.store!(image_with_gps) expect(EXIFR::JPEG.new(uploader.file.path).exif?).to be(false) expect(EXIFR::JPEG.new(uploader.file.path).gps.present?).to be(false) end + + it "does NOT remove EXIF and GPS data if frame count is > FRAME_STRIP_MAX" do + stub_const("BaseUploader::FRAME_STRIP_MAX", 0) + expect(EXIFR::JPEG.new(image_with_gps.path).exif?).to be(true) + expect(EXIFR::JPEG.new(image_with_gps.path).gps.present?).to be(true) + uploader.store!(image_with_gps) + expect(EXIFR::JPEG.new(uploader.file.path).exif?).to be(true) + expect(EXIFR::JPEG.new(uploader.file.path).gps.present?).to be(true) + end end end