Merge branch 'jprovazn-fix-filename' into 'master'

Fix filename for accelerated uploads

See merge request gitlab-org/gitlab-ce!20672

5 files changed, 86 insertions, 18 deletions

diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index 83f7b99d2a5..b1365659834 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -136,10 +136,6 @@ class FileUploader < GitlabUploader
     }
   end
 
-  def filename
-    self.file.filename
-  end
-
   def upload=(value)
     super
 
diff --git a/changelogs/unreleased/fix-filename-for-direct-uploads.yml b/changelogs/unreleased/fix-filename-for-direct-uploads.yml
new file mode 100644
index 00000000000..a1bbf3704c0
--- /dev/null
+++ b/changelogs/unreleased/fix-filename-for-direct-uploads.yml
@@ -0,0 +1,5 @@
+---
+title: Fix filename for accelerated uploads
+merge_request:
+author:
+type: fixed
diff --git a/lib/uploaded_file.rb b/lib/uploaded_file.rb
index 4b9cb59eab5..53e5ac02e42 100644
--- a/lib/uploaded_file.rb
+++ b/lib/uploaded_file.rb
@@ -21,7 +21,7 @@ class UploadedFile
     raise InvalidPathError, "#{path} file does not exist" unless ::File.exist?(path)
 
     @content_type = content_type
-    @original_filename = filename || ::File.basename(path)
+    @original_filename = sanitize_filename(filename || path)
     @content_type = content_type
     @sha256 = sha256
     @remote_id = remote_id
@@ -55,6 +55,16 @@ class UploadedFile
     end
   end
 
+  # copy-pasted from CarrierWave::SanitizedFile
+  def sanitize_filename(name)
+    name = name.tr("\\", "/") # work-around for IE
+    name = ::File.basename(name)
+    name = name.gsub(CarrierWave::SanitizedFile.sanitize_regexp, "_")
+    name = "_#{name}" if name =~ /\A\.+\z/
+    name = "unnamed" if name.empty?
+    name.mb_chars.to_s
+  end
+
   def path
     @tempfile.path
   end
diff --git a/spec/lib/uploaded_file_spec.rb b/spec/lib/uploaded_file_spec.rb
index cc99e7e8911..a2f5c2e7121 100644
--- a/spec/lib/uploaded_file_spec.rb
+++ b/spec/lib/uploaded_file_spec.rb
@@ -1,24 +1,28 @@
 require 'spec_helper'
 
 describe UploadedFile do
-  describe ".from_params" do
-    let(:temp_dir) { Dir.tmpdir }
-    let(:temp_file) { Tempfile.new("test", temp_dir) }
-    let(:upload_path) { nil }
+  let(:temp_dir) { Dir.tmpdir }
+  let(:temp_file) { Tempfile.new("test", temp_dir) }
 
-    subject do
-      described_class.from_params(params, :file, upload_path)
-    end
+  before do
+    FileUtils.touch(temp_file)
+  end
 
-    before do
-      FileUtils.touch(temp_file)
-    end
+  after do
+    FileUtils.rm_f(temp_file)
+  end
+
+  describe ".from_params" do
+    let(:upload_path) { nil }
 
     after do
-      FileUtils.rm_f(temp_file)
       FileUtils.rm_r(upload_path) if upload_path
     end
 
+    subject do
+      described_class.from_params(params, :file, upload_path)
+    end
+
     context 'when valid file is specified' do
       context 'only local path is specified' do
         let(:params) do
@@ -37,7 +41,7 @@ describe UploadedFile do
       context 'all parameters are specified' do
         let(:params) do
           { 'file.path' => temp_file.path,
-            'file.name' => 'my_file.txt',
+            'file.name' => 'dir/my file&.txt',
             'file.type' => 'my/type',
             'file.sha256' => 'sha256',
             'file.remote_id' => 'remote_id' }
@@ -48,7 +52,7 @@ describe UploadedFile do
         end
 
         it "generates filename from path" do
-          expect(subject.original_filename).to eq('my_file.txt')
+          expect(subject.original_filename).to eq('my_file_.txt')
           expect(subject.content_type).to eq('my/type')
           expect(subject.sha256).to eq('sha256')
           expect(subject.remote_id).to eq('remote_id')
@@ -113,4 +117,11 @@ describe UploadedFile do
       end
     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('_____') }
+    it { expect(described_class.new(temp_file.path).sanitize_filename('..')).to eq('_..') }
+    it { expect(described_class.new(temp_file.path).sanitize_filename('')).to eq('unnamed') }
+  end
 end
diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb
index 3efe512a59c..7e24efda5dd 100644
--- a/spec/uploaders/file_uploader_spec.rb
+++ b/spec/uploaders/file_uploader_spec.rb
@@ -166,4 +166,50 @@ describe FileUploader do
       uploader.upload = upload
     end
   end
+
+  describe '#cache!' do
+    subject do
+      uploader.store!(uploaded_file)
+    end
+
+    context 'when remote file is used' do
+      let(:temp_file) { Tempfile.new("test") }
+
+      let!(:fog_connection) do
+        stub_uploads_object_storage(described_class)
+      end
+
+      let(:uploaded_file) do
+        UploadedFile.new(temp_file.path, filename: "my file.txt", remote_id: "test/123123")
+      end
+
+      let!(:fog_file) do
+        fog_connection.directories.get('uploads').files.create(
+          key: 'tmp/uploads/test/123123',
+          body: 'content'
+        )
+      end
+
+      before do
+        FileUtils.touch(temp_file)
+      end
+
+      after do
+        FileUtils.rm_f(temp_file)
+      end
+
+      it 'file is stored remotely in permament location with sanitized name' do
+        subject
+
+        expect(uploader).to be_exists
+        expect(uploader).not_to be_cached
+        expect(uploader).not_to be_file_storage
+        expect(uploader.path).not_to be_nil
+        expect(uploader.path).not_to include('tmp/upload')
+        expect(uploader.path).not_to include('tmp/cache')
+        expect(uploader.url).to include('/my_file.txt')
+        expect(uploader.object_store).to eq(described_class::Store::REMOTE)
+      end
+    end
+  end
 end