Allow supporting svg images for designs

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

ee/app/controllers/projects/designs_controller.rb

@@ -8,7 +8,7 @@ class Projects::DesignsController < Projects::ApplicationController
   def show
     blob = design_repository.blob_at(ref, design.full_path)
 
-    send_blob(design_repository, blob, inline: (params[:inline] != 'false'))
+    send_blob(design_repository, blob, { inline: false })
   end
 
   private

ee/app/models/design_management/design.rb

@@ -130,10 +130,18 @@ module DesignManagement
       strong_memoize(:head_sha) { versions.ordered.first }
     end
 
+    def allow_dangerous_images?
+      Feature.enabled?(:design_management_allow_dangerous_images, project)
+    end
+
+    def valid_file_extensions
+      allow_dangerous_images? ? (SAFE_IMAGE_EXT + DANGEROUS_IMAGE_EXT) : SAFE_IMAGE_EXT
+    end
+
     def validate_file_is_image
-      unless image?
+      unless image? || (dangerous_image? && allow_dangerous_images?)
         message = _("Only these extensions are supported: %{extension_list}") % {
-          extension_list: Gitlab::FileTypeDetection::IMAGE_EXT.join(", ")
+          extension_list: valid_file_extensions.to_sentence
         }
         errors.add(:filename, message)
       end

ee/changelogs/unreleased/12771-enable-SVGs-for-design-management.yml

+---
+title: Allow files with .svg extensions to be uploaded as designs for Design Management
+merge_request: 16160
+author:
+type: changed

ee/spec/controllers/projects/designs_controller_spec.rb

@@ -5,9 +5,12 @@ require 'spec_helper'
 describe Projects::DesignsController do
   include DesignManagementTestHelpers
 
-  let(:project) { create(:project, :public) }
-  let(:issue) { create(:issue, project: project) }
-  let(:design) { create(:design, :with_file, issue: issue) }
+  set(:project) { create(:project, :public) }
+  set(:issue) { create(:issue, project: project) }
+  let(:file) { fixture_file_upload('spec/fixtures/dk.png', '`/png') }
+  let(:lfs_pointer) { Gitlab::Git::LfsPointerFile.new(file.read) }
+  let(:design) { create(:design, :with_lfs_file, file: lfs_pointer.pointer, issue: issue) }
+  let(:filename) { design.filename }
 
   before do
     enable_design_management
@@ -24,13 +27,35 @@ describe Projects::DesignsController do
       })
     end
 
-    it 'serves the file using workhorse' do
-      subject
+    # For security, .svg images should only ever be served with Content-Disposition: attachment.
+    # If these specs ever fail we must assess whether we should be serving svg images.
+    # See https://gitlab.com/gitlab-org/gitlab/issues/12771
+    describe 'Response headers' do
+      it 'serves LFS files with `Content-Disposition: attachment`' do
+        lfs_object = create(:lfs_object, file: file, oid: lfs_pointer.sha256, size: lfs_pointer.size)
+        create(:lfs_objects_project, project: project, lfs_object: lfs_object, repository_type: :design)
 
-      expect(response).to have_gitlab_http_status(200)
-      expect(response.header['Content-Disposition']).to eq('inline')
-      expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
-      expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+        subject
+
+        expect(response.header['Content-Disposition']).to eq(%Q(attachment; filename*=UTF-8''#{filename}; filename=\"#{filename}\"))
+      end
+
+      context 'when the design is not an LFS file' do
+        let(:design) { create(:design, :with_file, issue: issue) }
+
+        it 'serves files with `Content-Disposition: attachment`' do
+          subject
+
+          expect(response.header['Content-Disposition']).to eq('attachment')
+        end
+
+        it 'serves files with Workhorse' do
+          subject
+
+          expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
+          expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+        end
+      end
     end
 
     # Pass `skip_lfs_disabled_tests: true` to this shared example to disable
@@ -41,9 +66,7 @@ describe Projects::DesignsController do
     # controller will never authorize the user. Therefore #show will return a 403 and
     # we cannot test the data that it serves.
     it_behaves_like 'a controller that can serve LFS files', skip_lfs_disabled_tests: true do
-      let(:design) { create(:design, :with_lfs_file, issue: issue) }
       let(:lfs_oid) { project.design_repository.blob_at('HEAD', design.full_path).lfs_oid }
-      let(:filename) { design.filename }
       let(:filepath) { design.full_path }
     end
   end

ee/spec/features/projects/issues/design_management/user_uploads_designs_spec.rb

@@ -3,9 +3,9 @@ require 'spec_helper'
 describe 'User uploads new design', :js do
   include DesignManagementTestHelpers
 
-  let(:user) { project.owner }
-  let(:project) { create(:project_empty_repo, :public) }
-  let(:issue) { create(:issue, project: project) }
+  set(:project) { create(:project_empty_repo, :public) }
+  set(:user) { project.owner }
+  set(:issue) { create(:issue, project: project) }
 
   before do
     sign_in(user)

ee/spec/features/projects/issues/design_management/user_views_design_images_spec.rb

@@ -5,9 +5,9 @@ require 'spec_helper'
 describe 'Users views raw design image files' do
   include DesignManagementTestHelpers
 
-  let(:project) { create(:project, :public) }
-  let(:issue) { create(:issue, project: project) }
-  let(:design) { create(:design, :with_file, issue: issue, versions_count: 2) }
+  set(:project) { create(:project, :public) }
+  set(:issue) { create(:issue, project: project) }
+  set(:design) { create(:design, :with_file, issue: issue, versions_count: 2) }
   let(:newest_version) { design.versions.ordered.first }
   let(:oldest_version) { design.versions.ordered.last }
 

ee/spec/features/projects/issues/design_management/user_views_design_spec.rb

@@ -3,8 +3,8 @@ require 'spec_helper'
 describe 'User views issue designs', :js do
   include DesignManagementTestHelpers
 
-  let(:project) { create(:project_empty_repo, :public) }
-  let(:issue) { create(:issue, project: project) }
+  set(:project) { create(:project_empty_repo, :public) }
+  set(:issue) { create(:issue, project: project) }
 
   before do
     enable_design_management

ee/spec/features/projects/issues/design_management/user_views_designs_spec.rb

@@ -3,8 +3,8 @@ require 'spec_helper'
 describe 'User views issue designs', :js do
   include DesignManagementTestHelpers
 
-  let(:project) { create(:project_empty_repo, :public) }
-  let(:issue) { create(:issue, project: project) }
+  set(:project) { create(:project_empty_repo, :public) }
+  set(:issue) { create(:issue, project: project) }
 
   before do
     enable_design_management

ee/spec/features/projects/issues/design_management/user_views_designs_with_svg_xss_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'User views an SVG design that contains XSS', :js do
+  include DesignManagementTestHelpers
+
+  let(:project) { create(:project_empty_repo, :public) }
+  let(:issue) { create(:issue, project: project) }
+  let(:file) { Rails.root.join('spec', 'fixtures', 'logo_sample.svg') }
+  let(:design) { create(:design, :with_file, filename: 'xss.svg', file: file, issue: issue) }
+
+  before do
+    enable_design_management
+
+    visit designs_project_issue_path(
+      project,
+      issue,
+      { vueroute: design.filename }
+    )
+
+    wait_for_requests
+  end
+
+  it 'has XSS within the SVG file' do
+    file_content = File.read(file)
+
+    expect(file_content).to include("<script>alert('FAIL')</script>")
+  end
+
+  it 'displays the SVG' do
+    expect(page).to have_selector("img.design-img[alt='xss.svg']", count: 1)
+  end
+
+  it 'does not execute the JavaScript within the SVG' do
+    # The expectation is that we can call the capybara `page.dismiss_prompt`
+    # method to close a JavaScript alert prompt without a `Capybara::ModalNotFound`
+    # being raised.
+    run_expectation = -> {
+      page.dismiss_prompt(wait: 1)
+    }
+
+    # With the page loaded, there should be no alert modal
+    expect(run_expectation).to raise_error(
+      Capybara::ModalNotFound,
+      'Unable to find modal dialog'
+    )
+
+    # Perform a negative control test of the above expectation.
+    # With an alert modal displaying, the modal should be dismissable.
+    execute_script('alert(true)')
+
+    expect(run_expectation).not_to raise_error
+  end
+end

ee/spec/models/design_management/design_spec.rb

@@ -28,12 +28,35 @@ describe DesignManagement::Design do
     it { is_expected.to validate_presence_of(:filename) }
     it { is_expected.to validate_uniqueness_of(:filename).scoped_to(:issue_id) }
 
-    it "validates that the file is an image" do
+    it "validates that the extension is an image" do
       design.filename = "thing.txt"
+      extensions = described_class::SAFE_IMAGE_EXT + described_class::DANGEROUS_IMAGE_EXT
 
       expect(design).not_to be_valid
-      expect(design.errors[:filename].first)
-        .to match %r/Only these extensions are supported/
+      expect(design.errors[:filename].first).to eq(
+        "Only these extensions are supported: #{extensions.to_sentence}"
+      )
+    end
+
+    describe 'validating files with .svg extension' do
+      before do
+        design.filename = "thing.svg"
+      end
+
+      it "allows .svg files when feature flag is enabled" do
+        stub_feature_flags(design_management_allow_dangerous_images: true)
+
+        expect(design).to be_valid
+      end
+
+      it "does not allow .svg files when feature flag is disabled" do
+        stub_feature_flags(design_management_allow_dangerous_images: false)
+
+        expect(design).not_to be_valid
+        expect(design.errors[:filename].first).to eq(
+          "Only these extensions are supported: #{described_class::SAFE_IMAGE_EXT.to_sentence}"
+        )
+      end
     end
   end