Merge branch 'khanchi-designs-patch2' into 'master'

designs: return error if uploading filenames with special chars

See merge request gitlab-org/gitlab!44136

app/services/design_management/save_designs_service.rb

@@ -18,6 +18,7 @@ module DesignManagement
       return error("Only #{MAX_FILES} files are allowed simultaneously") if files.size > MAX_FILES
       return error("Duplicate filenames are not allowed!") if files.map(&:original_filename).uniq.length != files.length
       return error("Design copy is in progress") if design_collection.copy_in_progress?
+      return error("Filenames contained invalid characters and could not be saved") if files.any?(&:filename_sanitized?)
 
       uploaded_designs, version = upload_designs!
       skipped_designs = designs - uploaded_designs

changelogs/unreleased/khanchi-designs-patch2.yml

+---
+title: 'Designs: return error if uploading filenames with special chars'
+merge_request: 44136
+author: Sushil Khanchi @khanchi97
+type: fixed

ee/spec/services/ee/design_management/save_designs_service_spec.rb

@@ -3,11 +3,12 @@ require 'spec_helper'
 
 RSpec.describe DesignManagement::SaveDesignsService do
   include DesignManagementTestHelpers
+  using FixtureFileRefinements
 
   let_it_be(:project) { create(:project) }
   let_it_be(:issue) { create(:issue, project: project) }
   let_it_be(:user) { create(:user) }
-  let_it_be(:design_file) { fixture_file_upload('spec/fixtures/rails_sample.jpg') }
+  let_it_be(:design_file) { fixture_file_upload('spec/fixtures/rails_sample.jpg').to_gitlab_uploaded_file }
   let_it_be(:design_repository) { ::Gitlab::GlRepository::DESIGN.repository_resolver.call(project)}
 
   subject { described_class.new(project, user, issue: issue, files: [design_file]) }

lib/uploaded_file.rb

@@ -78,9 +78,15 @@ class UploadedFile
   def sanitize_filename(name)
     name = name.tr("\\", "/") # work-around for IE
     name = ::File.basename(name)
+
+    pre_sanitized_name = name
+
     name = name.gsub(CarrierWave::SanitizedFile.sanitize_regexp, "_")
     name = "_#{name}" if name =~ /\A\.+\z/
     name = "unnamed" if name.empty?
+
+    @filename_sanitized = name != pre_sanitized_name
+
     name.mb_chars.to_s
   end
 
@@ -92,6 +98,10 @@ class UploadedFile
     @tempfile&.close
   end
 
+  def filename_sanitized?
+    @filename_sanitized
+  end
+
   alias_method :local_path, :path
 
   def method_missing(method_name, *args, &block) #:nodoc:

spec/graphql/mutations/design_management/upload_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 RSpec.describe Mutations::DesignManagement::Upload do
   include DesignManagementTestHelpers
   include ConcurrentHelpers
+  using FixtureFileRefinements
 
   let(:issue) { create(:issue) }
   let(:user) { issue.author }
@@ -18,8 +19,12 @@ RSpec.describe Mutations::DesignManagement::Upload do
     mutation.resolve(project_path: project_path, iid: iid, files: files_to_upload)
   end
 
+  def uploaded_file(filename)
+    fixture_file_upload(expand_fixture_path(filename))
+  end
+
   describe "#resolve" do
-    let(:files) { [fixture_file_upload('spec/fixtures/dk.png')] }
+    let(:files) { [uploaded_file('dk.png').to_gitlab_uploaded_file] }
 
     subject(:resolve) do
       mutation.resolve(project_path: project.full_path, iid: issue.iid, files: files)
@@ -49,7 +54,7 @@ RSpec.describe Mutations::DesignManagement::Upload do
           ['dk.png', 'rails_sample.jpg', 'banana_sample.gif']
            .cycle
            .take(Concurrent.processor_count * 2)
-           .map { |f| RenameableUpload.unique_file(f) }
+           .map { |f| uploaded_file(f).uniquely_named.to_gitlab_uploaded_file }
         end
 
         def creates_designs

spec/lib/uploaded_file_spec.rb

@@ -218,6 +218,20 @@ RSpec.describe UploadedFile do
     end
   end
 
+  describe '#filename_sanitized?' do
+    it 'is true when filename has been sanitized' do
+      file = described_class.new(temp_file.path, filename: 'foo①.png')
+
+      expect(file).to be_filename_sanitized
+    end
+
+    it 'is false when filename has not been sanitized' do
+      file = described_class.new(temp_file.path, filename: 'foo.png')
+
+      expect(file).not_to be_filename_sanitized
+    end
+  end
+
   describe '#sanitize_filename' do
     it { expect(described_class.new(temp_file.path).sanitize_filename('spaced name')).to eq('spaced_name') }
     it { expect(described_class.new(temp_file.path).sanitize_filename('#$%^&')).to eq('_____') }

spec/services/design_management/save_designs_service_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 RSpec.describe DesignManagement::SaveDesignsService do
   include DesignManagementTestHelpers
   include ConcurrentHelpers
+  using FixtureFileRefinements
 
   let_it_be_with_reload(:issue) { create(:issue) }
   let_it_be(:developer) { create(:user, developer_projects: [issue.project]) }
@@ -12,11 +13,11 @@ RSpec.describe DesignManagement::SaveDesignsService do
   let(:files) { [rails_sample] }
   let(:design_repository) { ::Gitlab::GlRepository::DESIGN.repository_resolver.call(project) }
   let(:rails_sample_name) { 'rails_sample.jpg' }
-  let(:rails_sample) { sample_image(rails_sample_name) }
-  let(:dk_png) { sample_image('dk.png') }
+  let(:rails_sample) { uploaded_file(rails_sample_name).to_gitlab_uploaded_file }
+  let(:dk_png) { uploaded_file('dk.png').to_gitlab_uploaded_file }
 
-  def sample_image(filename)
-    fixture_file_upload("spec/fixtures/#{filename}")
+  def uploaded_file(filename)
+    fixture_file_upload(expand_fixture_path(filename))
   end
 
   def commit_count
@@ -122,7 +123,8 @@ RSpec.describe DesignManagement::SaveDesignsService do
         parellism = 4
 
         blocks = Array.new(parellism).map do
-          unique_files = [RenameableUpload.unique_file('rails_sample.jpg')]
+          unique_file = uploaded_file('dk.png').uniquely_named.to_gitlab_uploaded_file
+          unique_files = [unique_file]
 
           -> { run_service(unique_files) }
         end
@@ -306,6 +308,14 @@ RSpec.describe DesignManagement::SaveDesignsService do
             expect(response[:message]).to match('Duplicate filenames are not allowed!')
           end
         end
+
+        context 'when uploading files with special characters in filenames' do
+          let(:files) { [uploaded_file('dk.png').renamed_as('special_char①.png').to_gitlab_uploaded_file] }
+
+          it 'returns the correct error' do
+            expect(response[:message]).to match('Filenames contained invalid characters and could not be saved')
+          end
+        end
       end
 
       context 'when the user is not allowed to upload designs' do

spec/support/refinements/fixture_file_refinements.rb

+# frozen_string_literal: true
+
+module FixtureFileRefinements
+  refine Rack::Test::UploadedFile do
+    # Recast this instance of `Rack::Test::UploadedFile` to an `::UploadedFile`.
+    def to_gitlab_uploaded_file
+      ::UploadedFile.new(path, filename: original_filename, content_type: content_type || 'application/octet-stream').tap do |file|
+        # `UploadedFile#tempfile` is read-only, so replace this with the writeable fixture file
+        file.instance_variable_set(:@tempfile, self)
+      end
+    end
+
+    # Renames `original_filename` to something guaranteed to be unique.
+    def uniquely_named
+      name = File.basename(FactoryBot.generate(:filename), '.*')
+      extension = File.extname(original_filename)
+      unique_filename = name + extension
+
+      renamed_as(unique_filename)
+    end
+
+    def renamed_as(new_filename)
+      tap { @original_filename = new_filename }
+    end
+  end
+end

spec/support/renameable_upload.rb

-# frozen_string_literal: true
-
-class RenameableUpload < SimpleDelegator
-  attr_accessor :original_filename
-
-  # Get a fixture file with a new unique name, and the same extension
-  def self.unique_file(name)
-    upload = new(fixture_file_upload("spec/fixtures/#{name}"))
-    ext = File.extname(name)
-    new_name = File.basename(FactoryBot.generate(:filename), '.*')
-    upload.original_filename = new_name + ext
-
-    upload
-  end
-end