CE-specific changes to allow svg design images

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

CE-specific changes to allow svg design images
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
0a22ccda · Luke Duncalfe · a52935fb · 0a22ccda · 0a22ccda · 0a22ccda
--- a/app/models/blob.rb
+++ b/app/models/blob.rb
@@ -176,7 +176,7 @@ def file_type
  end

  def video?
-    UploaderHelper::VIDEO_EXT.include?(extension)
+    UploaderHelper::SAFE_VIDEO_EXT.include?(extension)
  end

  def readable_text?

--- a/app/models/blob_viewer/image.rb
+++ b/app/models/blob_viewer/image.rb
@@ -6,7 +6,7 @@ class Image < Base
    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'

--- a/app/models/blob_viewer/video.rb
+++ b/app/models/blob_viewer/video.rb
@@ -6,7 +6,7 @@ class Video < Base
    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'

--- a/app/models/concerns/avatarable.rb
+++ b/app/models/concerns/avatarable.rb
@@ -38,7 +38,7 @@ def retrieve_upload(identifier, paths)

  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


--- a/app/models/diff_viewer/image.rb
+++ b/app/models/diff_viewer/image.rb
@@ -6,7 +6,7 @@ class Image < Base
    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')

--- 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

--- a/lib/banzai/filter/video_link_filter.rb
+++ b/lib/banzai/filter/video_link_filter.rb
@@ -19,13 +19,13 @@ def call

      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
            )

--- a/lib/gitlab/file_markdown_link_builder.rb
+++ b/lib/gitlab/file_markdown_link_builder.rb
@@ -10,7 +10,7 @@ def markdown_link
      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


--- a/lib/gitlab/file_type_detection.rb
+++ b/lib/gitlab/file_type_detection.rb
 # 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 @@ def image_or_video?
      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

--- a/spec/lib/banzai/filter/video_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/video_link_filter_spec.rb
@@ -18,7 +18,7 @@ def link_to_image(path)
  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


--- 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 @@ def upload_fixture(filename)
        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 @@ def upload_fixture(filename)
      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 @@ def upload_fixture(filename)
        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 @@ def upload_fixture(filename)
        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