Merge branch '200107-avatar-content-type-does-not-match-file-extension' into 'master'

Resolve "Avatar Content type does not match file extension"

Closes #200107

See merge request gitlab-org/gitlab!25401

app/uploaders/avatar_uploader.rb

@@ -5,9 +5,8 @@ class AvatarUploader < GitlabUploader
   include RecordsUploads::Concern
   include ObjectStorage::Concern
   prepend ObjectStorage::Extension::RecordsUploads
-  include UploadTypeCheck::Concern
 
-  check_upload_type extensions: AvatarUploader::SAFE_IMAGE_EXT
+  MIME_WHITELIST = %w[image/png image/jpeg image/gif image/bmp image/tiff image/vnd.microsoft.icon].freeze
 
   def exists?
     model.avatar.file && model.avatar.file.present?
@@ -29,6 +28,10 @@ class AvatarUploader < GitlabUploader
     super || 'avatar'
   end
 
+  def content_type_whitelist
+    MIME_WHITELIST
+  end
+
   private
 
   def dynamic_segment

app/uploaders/content_type_whitelist.rb

+# frozen_string_literal: true
+
+# Currently we run CarrierWave 1.3.1 which means we can not whitelist files
+# by their content type through magic header parsing.
+#
+# This is a patch to hold us over until we get to CarrierWave 2 :) It's a mashup of
+# CarrierWave's lib/carrierwave/uploader/content_type_whitelist.rb and
+# lib/carrierwave/sanitized_file.rb
+#
+# Include this concern and add a content_type_whitelist method to get the same
+# behavior as you would with CarrierWave 2.
+#
+# This is not an exact replacement as we don't override
+# SanitizedFile#content_type but we do set the content_type attribute when we
+# check the whitelist.
+#
+# Remove this after moving to CarrierWave 2, though on practical terms it shouldn't
+# break anything if left for a while.
+module ContentTypeWhitelist
+  module Concern
+    extend ActiveSupport::Concern
+
+    private
+
+    # CarrierWave calls this method as part of it's before :cache callbacks.
+    # Here we override and extend CarrierWave's method that does not parse the
+    # magic headers.
+    def check_content_type_whitelist!(new_file)
+      new_file.content_type = mime_magic_content_type(new_file.path)
+
+      if content_type_whitelist && !whitelisted_content_type?(new_file.content_type)
+        message = I18n.translate(:"errors.messages.content_type_whitelist_error", allowed_types: Array(content_type_whitelist).join(", "))
+        raise CarrierWave::IntegrityError, message
+      end
+
+      super(new_file)
+    end
+
+    def whitelisted_content_type?(content_type)
+      Array(content_type_whitelist).any? { |item| content_type =~ /#{item}/ }
+    end
+
+    def mime_magic_content_type(path)
+      if path
+        File.open(path) do |file|
+          MimeMagic.by_magic(file).try(:type) || 'invalid/invalid'
+        end
+      end
+    rescue Errno::ENOENT
+      nil
+    end
+  end
+end

app/uploaders/favicon_uploader.rb

 # frozen_string_literal: true
 
 class FaviconUploader < AttachmentUploader
-  include UploadTypeCheck::Concern
-
   EXTENSION_WHITELIST = %w[png ico].freeze
-
-  check_upload_type extensions: EXTENSION_WHITELIST
+  MIME_WHITELIST = %w[image/png image/vnd.microsoft.icon].freeze
 
   def extension_whitelist
     EXTENSION_WHITELIST
   end
 
+  def content_type_whitelist
+    MIME_WHITELIST
+  end
+
   private
 
   def filename_for_different_format(filename, format)

app/uploaders/gitlab_uploader.rb

 # frozen_string_literal: true
 
 class GitlabUploader < CarrierWave::Uploader::Base
+  include ContentTypeWhitelist::Concern
+
   class_attribute :options
 
   class << self

changelogs/unreleased/200107-avatar-content-type-does-not-match-file-extension.yml

+---
+title: Replace avatar and favicon upload type consistency validation with content whitelist validation
+merge_request: 25401
+author:
+type: changed

config/locales/carrierwave.en.yml

@@ -6,8 +6,8 @@ en:
       carrierwave_download_error: could not be downloaded
       extension_whitelist_error: "You are not allowed to upload %{extension} files, allowed types: %{allowed_types}"
       extension_blacklist_error: "You are not allowed to upload %{extension} files, prohibited types: %{prohibited_types}"
-      content_type_whitelist_error: "You are not allowed to upload %{content_type} files"
-      content_type_blacklist_error: "You are not allowed to upload %{content_type} files"
+      content_type_whitelist_error: "file format is not supported. Please try one of the following supported formats: %{allowed_types}"
+      content_type_blacklist_error: "You are not allowed to upload %{content_type} files, prohibited types: %{allowed_types}"
       rmagick_processing_error: "Failed to manipulate with rmagick, maybe it is not an image?"
       mini_magick_processing_error: "Failed to manipulate with MiniMagick, maybe it is not an image? Original Error: %{e}"
       min_size_error: "File size should be greater than %{min_size}"

ee/spec/models/geo_node_status_spec.rb

@@ -156,7 +156,7 @@ describe GeoNodeStatus, :geo, :geo_fdw do
       expect(subject.attachments_count).to eq(1)
       expect(subject.attachments_synced_count).to eq(1)
 
-      user.update(avatar: fixture_file_upload('spec/fixtures/rails_sample.jpg', 'image/jpg'))
+      user.update(avatar: fixture_file_upload('spec/fixtures/rails_sample.jpg', 'image/jpeg'))
 
       subject = described_class.current_node_status
 

ee/spec/uploaders/design_management/design_v432x230_uploader_spec.rb

@@ -58,22 +58,29 @@ describe DesignManagement::DesignV432x230Uploader do
     )
   end
 
-  context 'uploading a whitelisted file extension' do
-    it 'stores the image successfully' do
-      fixture_file = fixture_file_upload('spec/fixtures/dk.png')
+  context 'accept whitelist file content type' do
+    # We need to feed through a valid path, but we force the parsed mime type
+    # in a stub below so we can set any path.
+    let_it_be(:path) { File.join('spec', 'fixtures', 'dk.png') }
 
-      expect { uploader.cache!(fixture_file) }.to change { uploader.file }.from(nil).to(kind_of(CarrierWave::SanitizedFile))
+    where(:mime_type) { described_class::MIME_TYPE_WHITELIST }
+
+    with_them do
+      include_context 'force content type detection to mime_type'
+
+      it_behaves_like 'accepted carrierwave upload'
     end
   end
 
-  context 'uploading a non-whitelisted file' do
-    it 'will deny the upload' do
-      fixture_file = fixture_file_upload('spec/fixtures/logo_sample.svg', 'image/svg+xml')
+  context 'upload non-whitelisted file content type' do
+    let_it_be(:path) { File.join('spec', 'fixtures', 'logo_sample.svg') }
 
-      expect { uploader.cache!(fixture_file) }.to raise_exception(
-        CarrierWave::IntegrityError,
-        'You are not allowed to upload image/svg+xml files'
-      )
-    end
+    it_behaves_like 'denied carrierwave upload'
+  end
+
+  context 'upload misnamed non-whitelisted file content type' do
+    let_it_be(:path) { File.join('spec', 'fixtures', 'not_a_png.png') }
+
+    it_behaves_like 'denied carrierwave upload'
   end
 end

spec/requests/api/groups_spec.rb

@@ -48,7 +48,7 @@ describe API::Groups do
 
       context 'when file format is not supported' do
         let(:file_path) { 'spec/fixtures/doc_sample.txt' }
-        let(:message)   { 'file format is not supported. Please try one of the following supported formats: png, jpg, jpeg, gif, bmp, tiff, ico' }
+        let(:message)   { 'file format is not supported. Please try one of the following supported formats: image/png, image/jpeg, image/gif, image/bmp, image/tiff, image/vnd.microsoft.icon' }
 
         it_behaves_like 'invalid file upload request'
       end

spec/support/shared_contexts/upload_type_check_shared_context.rb

@@ -20,6 +20,7 @@ RSpec.shared_context 'uploader with type check' do
   end
 end
 
+# This works with the UploadTypeCheck::Concern
 RSpec.shared_context 'stubbed MimeMagic mime type detection' do
   let(:mime_type) { '' }
   let(:magic_mime) { mime_type }
@@ -31,3 +32,19 @@ RSpec.shared_context 'stubbed MimeMagic mime type detection' do
     allow(MimeMagic).to receive(:by_path).with(anything).and_return(ext_mime_obj)
   end
 end
+
+# @param uploader [CarrierWave::Uploader::Base] uploader with extension_whitelist method.
+RSpec.shared_context 'ignore extension whitelist check' do
+  before do
+    allow(uploader).to receive(:extension_whitelist).and_return(nil)
+  end
+end
+
+# This works with a content_type_whitelist and content_type_blacklist type check.
+# @param mime_type [String] mime type to forcibly detect.
+RSpec.shared_context 'force content type detection to mime_type' do
+  before do
+    magic_mime_obj = MimeMagic.new(mime_type)
+    allow(MimeMagic).to receive(:by_magic).with(anything).and_return(magic_mime_obj)
+  end
+end

spec/support/shared_examples/uploaders/upload_type_shared_examples.rb

 # frozen_string_literal: true
 
+# @param path [String] the path to file to upload. E.g. File.join('spec', 'fixtures', 'sanitized.svg')
+# @param uploader [CarrierWave::Uploader::Base] uploader to handle the upload.
+shared_examples 'denied carrierwave upload' do
+  it 'will deny upload' do
+    fixture_file = fixture_file_upload(path)
+    expect { uploader.cache!(fixture_file) }.to raise_exception(CarrierWave::IntegrityError)
+  end
+end
+
+# @param path [String] the path to file to upload. E.g. File.join('spec', 'fixtures', 'sanitized.svg')
+# @param uploader [CarrierWave::Uploader::Base] uploader to handle the upload.
+shared_examples 'accepted carrierwave upload' do
+  let(:fixture_file) { fixture_file_upload(path) }
+
+  before do
+    uploader.remove!
+  end
+
+  it 'will accept upload' do
+    expect { uploader.cache!(fixture_file) }.not_to raise_exception
+  end
+
+  it 'will cache uploaded file' do
+    expect { uploader.cache!(fixture_file) }.to change { uploader.file }.from(nil).to(kind_of(CarrierWave::SanitizedFile))
+  end
+end
+
 def check_content_matches_extension!(file = double(read: nil, path: ''))
   magic_file = UploadTypeCheck::MagicFile.new(file)
   uploader.check_content_matches_extension!(magic_file)

spec/uploaders/avatar_uploader_spec.rb

@@ -47,15 +47,29 @@ describe AvatarUploader do
     end
   end
 
-  context 'upload type check' do
-    AvatarUploader::SAFE_IMAGE_EXT.each do |ext|
-      context "#{ext} extension" do
-        it_behaves_like 'type checked uploads', filenames: "image.#{ext}"
-      end
-    end
+  context 'accept whitelist file content type' do
+    # We need to feed through a valid path, but we force the parsed mime type
+    # in a stub below so we can set any path.
+    let_it_be(:path) { File.join('spec', 'fixtures', 'video_sample.mp4') }
+
+    where(:mime_type) { described_class::MIME_WHITELIST }
+
+    with_them do
+      include_context 'force content type detection to mime_type'
 
-    context 'skip image/svg+xml integrity check' do
-      it_behaves_like 'skipped type checked uploads', filenames: 'image.svg'
+      it_behaves_like 'accepted carrierwave upload'
     end
   end
+
+  context 'upload non-whitelisted file content type' do
+    let_it_be(:path) { File.join('spec', 'fixtures', 'sanitized.svg') }
+
+    it_behaves_like 'denied carrierwave upload'
+  end
+
+  context 'upload misnamed non-whitelisted file content type' do
+    let_it_be(:path) { File.join('spec', 'fixtures', 'not_a_png.png') }
+
+    it_behaves_like 'denied carrierwave upload'
+  end
 end

spec/uploaders/content_type_whitelist_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ContentTypeWhitelist do
+  class DummyUploader < CarrierWave::Uploader::Base
+    include ContentTypeWhitelist::Concern
+
+    def content_type_whitelist
+      %w[image/png image/jpeg]
+    end
+  end
+
+  let_it_be(:model) { build_stubbed(:user) }
+  let_it_be(:uploader) { DummyUploader.new(model, :dummy) }
+
+  context 'upload whitelisted file content type' do
+    let(:path) { File.join('spec', 'fixtures', 'rails_sample.jpg') }
+
+    it_behaves_like 'accepted carrierwave upload'
+  end
+
+  context 'upload non-whitelisted file content type' do
+    let(:path) { File.join('spec', 'fixtures', 'sanitized.svg') }
+
+    it_behaves_like 'denied carrierwave upload'
+  end
+
+  context 'upload misnamed non-whitelisted file content type' do
+    let(:path) { File.join('spec', 'fixtures', 'not_a_png.png') }
+
+    it_behaves_like 'denied carrierwave upload'
+  end
+end

spec/uploaders/favicon_uploader_spec.rb

@@ -6,19 +6,35 @@ describe FaviconUploader do
   let_it_be(:model) { build_stubbed(:user) }
   let_it_be(:uploader) { described_class.new(model, :favicon) }
 
-  context 'upload type check' do
-    FaviconUploader::EXTENSION_WHITELIST.each do |ext|
-      context "#{ext} extension" do
-        it_behaves_like 'type checked uploads', filenames: "image.#{ext}"
-      end
+  context 'accept whitelist file content type' do
+    include_context 'ignore extension whitelist check'
+
+    # We need to feed through a valid path, but we force the parsed mime type
+    # in a stub below so we can set any path.
+    let_it_be(:path) { File.join('spec', 'fixtures', 'video_sample.mp4') }
+
+    where(:mime_type) { described_class::MIME_WHITELIST }
+
+    with_them do
+      include_context 'force content type detection to mime_type'
+
+      it_behaves_like 'accepted carrierwave upload'
     end
   end
 
-  context 'upload non-whitelisted file extensions' do
-    it 'will deny upload' do
-      path = File.join('spec', 'fixtures', 'banana_sample.gif')
-      fixture_file = fixture_file_upload(path)
-      expect { uploader.cache!(fixture_file) }.to raise_exception(CarrierWave::IntegrityError)
-    end
+  context 'upload non-whitelisted file content type' do
+    include_context 'ignore extension whitelist check'
+
+    let_it_be(:path) { File.join('spec', 'fixtures', 'sanitized.svg') }
+
+    it_behaves_like 'denied carrierwave upload'
+  end
+
+  context 'upload misnamed non-whitelisted file content type' do
+    include_context 'ignore extension whitelist check'
+
+    let_it_be(:path) { File.join('spec', 'fixtures', 'not_a_png.png') }
+
+    it_behaves_like 'denied carrierwave upload'
   end
 end