Merge branch '237848-image-scaling-only-png-jpg' into 'master'

Only allow PNGs and JPGs in image scaling requests

See merge request gitlab-org/gitlab!39965

app/controllers/concerns/send_file_upload.rb

@@ -2,6 +2,8 @@
 
 module SendFileUpload
   def send_upload(file_upload, send_params: {}, redirect_params: {}, attachment: nil, proxy: false, disposition: 'attachment')
+    content_type = content_type_for(attachment)
+
     if attachment
       response_disposition = ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: attachment)
 
@@ -9,7 +11,7 @@ module SendFileUpload
       # Google Cloud Storage, so the metadata needs to be cleared on GCS for
       # this to work. However, this override works with AWS.
       redirect_params[:query] = { "response-content-disposition" => response_disposition,
-                                  "response-content-type" => guess_content_type(attachment) }
+                                  "response-content-type" => content_type }
       # By default, Rails will send uploads with an extension of .js with a
       # content-type of text/javascript, which will trigger Rails'
       # cross-origin JavaScript protection.
@@ -20,7 +22,7 @@ module SendFileUpload
 
     if image_scaling_request?(file_upload)
       location = file_upload.file_storage? ? file_upload.path : file_upload.url
-      headers.store(*Gitlab::Workhorse.send_scaled_image(location, params[:width].to_i))
+      headers.store(*Gitlab::Workhorse.send_scaled_image(location, params[:width].to_i, content_type))
       head :ok
     elsif file_upload.file_storage?
       send_file file_upload.path, send_params
@@ -32,6 +34,12 @@ module SendFileUpload
     end
   end
 
+  def content_type_for(attachment)
+    return '' unless attachment
+
+    guess_content_type(attachment)
+  end
+
   def guess_content_type(filename)
     types = MIME::Types.type_for(filename)
 
@@ -45,12 +53,16 @@ module SendFileUpload
   private
 
   def image_scaling_request?(file_upload)
-    avatar_image_upload?(file_upload) && valid_image_scaling_width? && current_user &&
-      Feature.enabled?(:dynamic_image_resizing, current_user)
+    Feature.enabled?(:dynamic_image_resizing, current_user) &&
+      avatar_safe_for_scaling?(file_upload) && valid_image_scaling_width? && current_user
+  end
+
+  def avatar_safe_for_scaling?(file_upload)
+    file_upload.try(:image_safe_for_scaling?) && mounted_as_avatar?(file_upload)
   end
 
-  def avatar_image_upload?(file_upload)
-    file_upload.try(:image?) && file_upload.try(:mounted_as)&.to_sym == :avatar
+  def mounted_as_avatar?(file_upload)
+    file_upload.try(:mounted_as)&.to_sym == :avatar
   end
 
   def valid_image_scaling_width?

lib/gitlab/file_type_detection.rb

@@ -20,6 +20,8 @@
 module Gitlab
   module FileTypeDetection
     SAFE_IMAGE_EXT = %w[png jpg jpeg gif bmp tiff ico].freeze
+    SAFE_IMAGE_FOR_SCALING_EXT = %w[png jpg jpeg].freeze
+
     PDF_EXT = 'pdf'
     # We recommend using the .mp4 format over .mov. Videos in .mov format can
     # still be used but you really need to make sure they are served with the
@@ -46,6 +48,12 @@ module Gitlab
       extension_match?(SAFE_IMAGE_EXT)
     end
 
+    # For the time being, we restrict image scaling requests to the most popular and safest formats only,
+    # which are JPGs and PNGs. See https://gitlab.com/gitlab-org/gitlab/-/issues/237848 for more info.
+    def image_safe_for_scaling?
+      extension_match?(SAFE_IMAGE_FOR_SCALING_EXT)
+    end
+
     def video?
       extension_match?(SAFE_VIDEO_EXT)
     end

lib/gitlab/workhorse.rb

@@ -156,10 +156,11 @@ module Gitlab
         ]
       end
 
-      def send_scaled_image(location, width)
+      def send_scaled_image(location, width, content_type)
         params = {
           'Location' => location,
-          'Width' => width
+          'Width' => width,
+          'ContentType' => content_type
         }
 
         [

spec/controllers/concerns/send_file_upload_spec.rb

@@ -49,10 +49,14 @@ RSpec.describe SendFileUpload do
     end
 
     shared_examples 'handles image resize requests' do
+      let(:params) do
+        { attachment: 'avatar.png' }
+      end
+
       let(:headers) { double }
 
       before do
-        allow(uploader).to receive(:image?).and_return(true)
+        allow(uploader).to receive(:image_safe_for_scaling?).and_return(true)
         allow(uploader).to receive(:mounted_as).and_return(:avatar)
 
         allow(controller).to receive(:headers).and_return(headers)
@@ -75,7 +79,10 @@ RSpec.describe SendFileUpload do
             expect(controller).not_to receive(:send_file)
             expect(controller).to receive(:params).at_least(:once).and_return(width: '64')
             expect(controller).to receive(:head).with(:ok)
-            expect(headers).to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
+            expect(Gitlab::Workhorse).to receive(:send_scaled_image).with(a_string_matching('^(/.+|https://.+)'), 64, 'image/png').and_return([
+              Gitlab::Workhorse::SEND_DATA_HEADER, "send-scaled-img:faux"
+            ])
+            expect(headers).to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, "send-scaled-img:faux")
 
             subject
           end
@@ -115,6 +122,15 @@ RSpec.describe SendFileUpload do
             subject
           end
         end
+
+        context 'when image file type is not considered safe for scaling' do
+          it 'does not write workhorse command header' do
+            expect(uploader).to receive(:image_safe_for_scaling?).and_return(false)
+            expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
+
+            subject
+          end
+        end
       end
 
       context 'when feature is disabled' do
@@ -123,7 +139,6 @@ RSpec.describe SendFileUpload do
         end
 
         it 'does not write workhorse command header' do
-          expect(controller).to receive(:params).at_least(:once).and_return(width: '64')
           expect(headers).not_to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-scaled-img:/)
 
           subject

spec/lib/gitlab/file_type_detection_spec.rb

@@ -192,6 +192,20 @@ RSpec.describe Gitlab::FileTypeDetection do
       end
     end
 
+    describe '#image_safe_for_scaling?' do
+      it 'returns true for allowed image formats' do
+        uploader.store!(upload_fixture('rails_sample.jpg'))
+
+        expect(uploader).to be_image_safe_for_scaling
+      end
+
+      it 'returns false for other files' do
+        uploader.store!(upload_fixture('unsanitized.svg'))
+
+        expect(uploader).not_to be_image_safe_for_scaling
+      end
+    end
+
     describe '#dangerous_image?' do
       it 'returns true if filename has a dangerous extension' do
         uploader.store!(upload_fixture('unsanitized.svg'))
@@ -377,6 +391,31 @@ RSpec.describe Gitlab::FileTypeDetection do
       end
     end
 
+    describe '#image_safe_for_scaling?' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:filename, :expectation) do
+        'img.jpg'  | true
+        'img.jpeg' | true
+        'img.png'  | true
+        'img.svg'  | false
+      end
+
+      with_them do
+        it "returns expected result" do
+          allow(custom_class).to receive(:filename).and_return(filename)
+
+          expect(custom_class.image_safe_for_scaling?).to be(expectation)
+        end
+      end
+
+      it 'returns false if filename is blank' do
+        allow(custom_class).to receive(:filename).and_return(nil)
+
+        expect(custom_class).not_to be_image_safe_for_scaling
+      end
+    end
+
     describe '#video?' do
       it 'returns true for a video file' do
         allow(custom_class).to receive(:filename).and_return('video_sample.mp4')

spec/lib/gitlab/workhorse_spec.rb

@@ -424,8 +424,9 @@ RSpec.describe Gitlab::Workhorse do
   describe '.send_scaled_image' do
     let(:location) { 'http://example.com/avatar.png' }
     let(:width) { '150' }
+    let(:content_type) { 'image/png' }
 
-    subject { described_class.send_scaled_image(location, width) }
+    subject { described_class.send_scaled_image(location, width, content_type) }
 
     it 'sets the header correctly' do
       key, command, params = decode_workhorse_header(subject)
@@ -434,7 +435,8 @@ RSpec.describe Gitlab::Workhorse do
       expect(command).to eq("send-scaled-img")
       expect(params).to eq({
         'Location' => location,
-        'Width' => width
+        'Width' => width,
+        'ContentType' => content_type
       }.deep_stringify_keys)
     end
   end