CE-specific changes to allow svg design images12771-enable-SVGs-for-design-management-ce

This change renames the previous `DANGEROUS_EXT` constant to
`DANGEROUS_IMAGE_EXT`, as it contains only `'svg'`, and allows
developers to allow the inclusion of `'svg'` in their checks by passing
`allow_dangerous_ext: true` to the relevant methods.

https://gitlab.com/gitlab-org/gitlab-ee/issues/12771

11 files changed, 188 insertions, 35 deletions

diff --git a/app/models/blob.rb b/app/models/blob.rb
index d528bef8b19..a0941de1242 100644
--- a/app/models/blob.rb
+++ b/app/models/blob.rb
@@ -176,7 +176,7 @@ class Blob < SimpleDelegator
   end
 
   def video?
-    UploaderHelper::VIDEO_EXT.include?(extension)
+    UploaderHelper::SAFE_VIDEO_EXT.include?(extension)
   end
 
   def readable_text?
diff --git a/app/models/blob_viewer/image.rb b/app/models/blob_viewer/image.rb
index 56e27839fca..cbebef46c60 100644
--- a/app/models/blob_viewer/image.rb
+++ b/app/models/blob_viewer/image.rb
@@ -6,7 +6,7 @@ module BlobViewer
     include ClientSide
 
     self.partial_name = 'image'
-    self.extensions = UploaderHelper::IMAGE_EXT
+    self.extensions = UploaderHelper::SAFE_IMAGE_EXT
     self.binary = true
     self.switcher_icon = 'picture-o'
     self.switcher_title = 'image'
diff --git a/app/models/blob_viewer/video.rb b/app/models/blob_viewer/video.rb
index 48bb2a13518..d35b8e7342e 100644
--- a/app/models/blob_viewer/video.rb
+++ b/app/models/blob_viewer/video.rb
@@ -6,7 +6,7 @@ module BlobViewer
     include ClientSide
 
     self.partial_name = 'video'
-    self.extensions = UploaderHelper::VIDEO_EXT
+    self.extensions = UploaderHelper::SAFE_VIDEO_EXT
     self.binary = true
     self.switcher_icon = 'film'
     self.switcher_title = 'video'
diff --git a/app/models/concerns/avatarable.rb b/app/models/concerns/avatarable.rb
index 80278e07e65..28177cc6d1f 100644
--- a/app/models/concerns/avatarable.rb
+++ b/app/models/concerns/avatarable.rb
@@ -38,7 +38,7 @@ module Avatarable
 
   def avatar_type
     unless self.avatar.image?
-      errors.add :avatar, "file format is not supported. Please try one of the following supported formats: #{AvatarUploader::IMAGE_EXT.join(', ')}"
+      errors.add :avatar, "file format is not supported. Please try one of the following supported formats: #{AvatarUploader::SAFE_IMAGE_EXT.join(', ')}"
     end
   end
 
diff --git a/app/models/diff_viewer/image.rb b/app/models/diff_viewer/image.rb
index 350bef1d42a..cfda0058d81 100644
--- a/app/models/diff_viewer/image.rb
+++ b/app/models/diff_viewer/image.rb
@@ -6,7 +6,7 @@ module DiffViewer
     include ClientSide
 
     self.partial_name = 'image'
-    self.extensions = UploaderHelper::IMAGE_EXT
+    self.extensions = UploaderHelper::SAFE_IMAGE_EXT
     self.binary = true
     self.switcher_icon = 'picture-o'
     self.switcher_title = _('image diff')
diff --git a/doc/user/project/issues/design_management.md b/doc/user/project/issues/design_management.md
index 1324a90e00b..83d197bb8e9 100644
--- a/doc/user/project/issues/design_management.md
+++ b/doc/user/project/issues/design_management.md
@@ -36,7 +36,6 @@ to be enabled:
 ## Limitations
 
 - Files uploaded must have a file extension of either `png`, `jpg`, `jpeg`, `gif`, `bmp`, `tiff` or `ico`.
-  The [`svg` extension is not yet supported](https://gitlab.com/gitlab-org/gitlab-ee/issues/12771).
 - Design uploads are limited to 10 files at a time.
 - [Designs cannot yet be deleted](https://gitlab.com/gitlab-org/gitlab-ee/issues/11089).
 - Design Management is
diff --git a/lib/banzai/filter/video_link_filter.rb b/lib/banzai/filter/video_link_filter.rb
index a278fcfdb47..58006cc6c13 100644
--- a/lib/banzai/filter/video_link_filter.rb
+++ b/lib/banzai/filter/video_link_filter.rb
@@ -19,13 +19,13 @@ module Banzai
 
       def query
         @query ||= begin
-          src_query = UploaderHelper::VIDEO_EXT.map do |ext|
+          src_query = UploaderHelper::SAFE_VIDEO_EXT.map do |ext|
             "'.#{ext}' = substring(@src, string-length(@src) - #{ext.size})"
           end
 
           if context[:asset_proxy_enabled].present?
             src_query.concat(
-              UploaderHelper::VIDEO_EXT.map do |ext|
+              UploaderHelper::SAFE_VIDEO_EXT.map do |ext|
                 "'.#{ext}' = substring(@data-canonical-src, string-length(@data-canonical-src) - #{ext.size})"
               end
             )
diff --git a/lib/gitlab/file_markdown_link_builder.rb b/lib/gitlab/file_markdown_link_builder.rb
index 180140e7da2..e9e5172e6f8 100644
--- a/lib/gitlab/file_markdown_link_builder.rb
+++ b/lib/gitlab/file_markdown_link_builder.rb
@@ -10,7 +10,7 @@ module Gitlab
       return unless name = markdown_name
 
       markdown = "[#{name.gsub(']', '\\]')}](#{secure_url})"
-      markdown = "!#{markdown}" if image_or_video? || dangerous?
+      markdown = "!#{markdown}" if image_or_video? || dangerous_image_or_video?
       markdown
     end
 
diff --git a/lib/gitlab/file_type_detection.rb b/lib/gitlab/file_type_detection.rb
index 25ee07cf940..c4a142df8dc 100644
--- a/lib/gitlab/file_type_detection.rb
+++ b/lib/gitlab/file_type_detection.rb
@@ -1,22 +1,42 @@
 # frozen_string_literal: true
 
-# File helpers methods.
-# It needs the method filename to be defined.
+# The method `filename` must be defined in classes that use this module.
+#
+# This module is intended to be used as a helper and not a security gate
+# to validate that a file is safe, as it identifies files only by the
+# file extension and not its actual contents.
+#
+# An example useage of this module is in `FileMarkdownLinkBuilder` that
+# renders markdown depending on a file name.
+#
+# We use Workhorse to detect the real extension when we serve files with
+# the `SendsBlob` helper methods, and ask Workhorse to set the content
+# type when it serves the file:
+# https://gitlab.com/gitlab-org/gitlab-ce/blob/33e5955/app/helpers/workhorse_helper.rb#L48.
+#
+# Because Workhorse has access to the content when it is downloaded, if
+# the type/extension doesn't match the real type, we adjust the
+# `Content-Type` and `Content-Disposition` to the one we get from the detection.
 module Gitlab
   module FileTypeDetection
-    IMAGE_EXT = %w[png jpg jpeg gif bmp tiff ico].freeze
+    SAFE_IMAGE_EXT = %w[png jpg jpeg gif bmp tiff ico].freeze
     # 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
     # proper MIME type video/mp4 and not video/quicktime or your videos won't play
     # on IE >= 9.
     # http://archive.sublimevideo.info/20150912/docs.sublimevideo.net/troubleshooting.html
-    VIDEO_EXT = %w[mp4 m4v mov webm ogv].freeze
+    SAFE_VIDEO_EXT = %w[mp4 m4v mov webm ogv].freeze
+
     # These extension types can contain dangerous code and should only be embedded inline with
     # proper filtering. They should always be tagged as "Content-Disposition: attachment", not "inline".
-    DANGEROUS_EXT = %w[svg].freeze
+    DANGEROUS_IMAGE_EXT = %w[svg].freeze
+    DANGEROUS_VIDEO_EXT = [].freeze # None, yet
+
+    VIDEO_EXT = (SAFE_VIDEO_EXT + DANGEROUS_VIDEO_EXT).freeze
+    IMAGE_EXT = (SAFE_IMAGE_EXT + DANGEROUS_IMAGE_EXT).freeze
 
     def image?
-      extension_match?(IMAGE_EXT)
+      extension_match?(SAFE_IMAGE_EXT)
     end
 
     def video?
@@ -27,8 +47,16 @@ module Gitlab
       image? || video?
     end
 
-    def dangerous?
-      extension_match?(DANGEROUS_EXT)
+    def dangerous_image?
+      extension_match?(DANGEROUS_IMAGE_EXT)
+    end
+
+    def dangerous_video?
+      extension_match?(DANGEROUS_VIDEO_EXT)
+    end
+
+    def dangerous_image_or_video?
+      dangerous_image? || dangerous_video?
     end
 
     private
diff --git a/spec/lib/banzai/filter/video_link_filter_spec.rb b/spec/lib/banzai/filter/video_link_filter_spec.rb
index cd932f502f3..b5be204d680 100644
--- a/spec/lib/banzai/filter/video_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/video_link_filter_spec.rb
@@ -18,7 +18,7 @@ describe Banzai::Filter::VideoLinkFilter do
   let(:project) { create(:project, :repository) }
 
   context 'when the element src has a video extension' do
-    UploaderHelper::VIDEO_EXT.each do |ext|
+    UploaderHelper::SAFE_VIDEO_EXT.each do |ext|
       it "replaces the image tag 'path/video.#{ext}' with a video tag" do
         container = filter(link_to_image("/path/video.#{ext}")).children.first
 
diff --git a/spec/lib/gitlab/file_type_detection_spec.rb b/spec/lib/gitlab/file_type_detection_spec.rb
index 22ec7d414e8..d41d3f3e283 100644
--- a/spec/lib/gitlab/file_type_detection_spec.rb
+++ b/spec/lib/gitlab/file_type_detection_spec.rb
@@ -2,22 +2,51 @@
 require 'spec_helper'
 
 describe Gitlab::FileTypeDetection do
-  def upload_fixture(filename)
-    fixture_file_upload(File.join('spec', 'fixtures', filename))
-  end
+  context 'when class is an uploader' do
+    let(:uploader) do
+      example_uploader = Class.new(CarrierWave::Uploader::Base) do
+        include Gitlab::FileTypeDetection
+
+        storage :file
+      end
+
+      example_uploader.new
+    end
+
+    def upload_fixture(filename)
+      fixture_file_upload(File.join('spec', 'fixtures', filename))
+    end
+
+    describe '#image?' do
+      it 'returns true for an image file' do
+        uploader.store!(upload_fixture('dk.png'))
+
+        expect(uploader).to be_image
+      end
+
+      it 'returns false if filename has a dangerous image extension' do
+        uploader.store!(upload_fixture('unsanitized.svg'))
+
+        expect(uploader).to be_dangerous_image
+        expect(uploader).not_to be_image
+      end
 
-  describe '#image_or_video?' do
-    context 'when class is an uploader' do
-      let(:uploader) do
-        example_uploader = Class.new(CarrierWave::Uploader::Base) do
-          include Gitlab::FileTypeDetection
+      it 'returns false for a video file' do
+        uploader.store!(upload_fixture('video_sample.mp4'))
 
-          storage :file
-        end
+        expect(uploader).not_to be_image
+      end
+
+      it 'returns false if filename is blank' do
+        uploader.store!(upload_fixture('dk.png'))
+
+        allow(uploader).to receive(:filename).and_return(nil)
 
-        example_uploader.new
+        expect(uploader).not_to be_image
       end
+    end
 
+    describe '#image_or_video?' do
       it 'returns true for an image file' do
         uploader.store!(upload_fixture('dk.png'))
 
@@ -30,6 +59,13 @@ describe Gitlab::FileTypeDetection do
         expect(uploader).to be_image_or_video
       end
 
+      it 'returns false when file has a dangerous extension' do
+        uploader.store!(upload_fixture('unsanitized.svg'))
+
+        expect(uploader).to be_dangerous_image_or_video
+        expect(uploader).not_to be_image_or_video
+      end
+
       it 'returns false for other extensions' do
         uploader.store!(upload_fixture('doc_sample.txt'))
 
@@ -45,15 +81,72 @@ describe Gitlab::FileTypeDetection do
       end
     end
 
-    context 'when class is a regular class' do
-      let(:custom_class) do
-        custom_class = Class.new do
-          include Gitlab::FileTypeDetection
-        end
+    describe '#dangerous_image?' do
+      it 'returns true if filename has a dangerous extension' do
+        uploader.store!(upload_fixture('unsanitized.svg'))
+
+        expect(uploader).to be_dangerous_image
+      end
+
+      it 'returns false for an image file' do
+        uploader.store!(upload_fixture('dk.png'))
+
+        expect(uploader).not_to be_dangerous_image
+      end
+
+      it 'returns false for a video file' do
+        uploader.store!(upload_fixture('video_sample.mp4'))
 
-        custom_class.new
+        expect(uploader).not_to be_dangerous_image
       end
 
+      it 'returns false if filename is blank' do
+        uploader.store!(upload_fixture('dk.png'))
+
+        allow(uploader).to receive(:filename).and_return(nil)
+
+        expect(uploader).not_to be_dangerous_image
+      end
+    end
+  end
+
+  context 'when class is a regular class' do
+    let(:custom_class) do
+      custom_class = Class.new do
+        include Gitlab::FileTypeDetection
+      end
+
+      custom_class.new
+    end
+
+    describe '#image?' do
+      it 'returns true for an image file' do
+        allow(custom_class).to receive(:filename).and_return('dk.png')
+
+        expect(custom_class).to be_image
+      end
+
+      it 'returns false if file has a dangerous image extension' do
+        allow(custom_class).to receive(:filename).and_return('unsanitized.svg')
+
+        expect(custom_class).to be_dangerous_image
+        expect(custom_class).not_to be_image
+      end
+
+      it 'returns false for any non image file' do
+        allow(custom_class).to receive(:filename).and_return('video_sample.mp4')
+
+        expect(custom_class).not_to be_image
+      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
+      end
+    end
+
+    describe '#image_or_video?' do
       it 'returns true for an image file' do
         allow(custom_class).to receive(:filename).and_return('dk.png')
 
@@ -66,6 +159,13 @@ describe Gitlab::FileTypeDetection do
         expect(custom_class).to be_image_or_video
       end
 
+      it 'returns false if file has a dangerous extension' do
+        allow(custom_class).to receive(:filename).and_return('unsanitized.svg')
+
+        expect(custom_class).to be_dangerous_image_or_video
+        expect(custom_class).not_to be_image_or_video
+      end
+
       it 'returns false for other extensions' do
         allow(custom_class).to receive(:filename).and_return('doc_sample.txt')
 
@@ -78,5 +178,31 @@ describe Gitlab::FileTypeDetection do
         expect(custom_class).not_to be_image_or_video
       end
     end
+
+    describe '#dangerous_image?' do
+      it 'returns true if file has a dangerous image extension' do
+        allow(custom_class).to receive(:filename).and_return('unsanitized.svg')
+
+        expect(custom_class).to be_dangerous_image
+      end
+
+      it 'returns false for an image file' do
+        allow(custom_class).to receive(:filename).and_return('dk.png')
+
+        expect(custom_class).not_to be_dangerous_image
+      end
+
+      it 'returns false for any non image file' do
+        allow(custom_class).to receive(:filename).and_return('video_sample.mp4')
+
+        expect(custom_class).not_to be_dangerous_image
+      end
+
+      it 'returns false if filename is blank' do
+        allow(custom_class).to receive(:filename).and_return(nil)
+
+        expect(custom_class).not_to be_dangerous_image
+      end
+    end
   end
 end