Only allow PNGs and JPGs in image scaling requests

This is to constraint this experimental feature more for now
and acts as an additional safety check.

We decided it's OK to rely on just the filename extension for now,
since we're already checking file contents during upload.

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