Merge branch '53357-fix-plus-in-upload-file-names' into 'master'

Use GitlabUploader#filename when generating upload URLs

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

4 files changed, 43 insertions, 28 deletions

diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index 1c7582533ad..b326b266017 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -203,6 +203,6 @@ class FileUploader < GitlabUploader
   end
 
   def secure_url
-    File.join('/uploads', @secret, file.filename)
+    File.join('/uploads', @secret, filename)
   end
 end
diff --git a/app/uploaders/personal_file_uploader.rb b/app/uploaders/personal_file_uploader.rb
index b43162f0935..1ac69601d18 100644
--- a/app/uploaders/personal_file_uploader.rb
+++ b/app/uploaders/personal_file_uploader.rb
@@ -93,6 +93,6 @@ class PersonalFileUploader < FileUploader
   end
 
   def secure_url
-    File.join('/', base_dir, secret, file.filename)
+    File.join('/', base_dir, secret, filename)
   end
 end
diff --git a/changelogs/unreleased/53357-fix-plus-in-upload-file-names.yml b/changelogs/unreleased/53357-fix-plus-in-upload-file-names.yml
new file mode 100644
index 00000000000..c11d74bd4fe
--- /dev/null
+++ b/changelogs/unreleased/53357-fix-plus-in-upload-file-names.yml
@@ -0,0 +1,5 @@
+---
+title: Fix broken URLs for uploads with a plus in the filename
+merge_request: 29915
+author:
+type: fixed
diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb
index 185c62491ce..04206de3dc6 100644
--- a/spec/uploaders/file_uploader_spec.rb
+++ b/spec/uploaders/file_uploader_spec.rb
@@ -184,40 +184,37 @@ describe FileUploader do
     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") }
 
-    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!(:fog_connection) do
-        stub_uploads_object_storage(described_class)
-      end
+    let(:filename) { "my file.txt" }
+    let(:uploaded_file) do
+      UploadedFile.new(temp_file.path, filename: filename, remote_id: "test/123123")
+    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.new(key: 'uploads').files.create(
+        key: 'tmp/uploads/test/123123',
+        body: 'content'
+      )
+    end
 
-      let!(:fog_file) do
-        fog_connection.directories.new(key: 'uploads').files.create(
-          key: 'tmp/uploads/test/123123',
-          body: 'content'
-        )
-      end
+    before do
+      FileUtils.touch(temp_file)
 
-      before do
-        FileUtils.touch(temp_file)
-      end
+      uploader.store!(uploaded_file)
+    end
 
-      after do
-        FileUtils.rm_f(temp_file)
-      end
+    after do
+      FileUtils.rm_f(temp_file)
+    end
 
+    describe '#cache!' do
       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
@@ -228,5 +225,18 @@ describe FileUploader do
         expect(uploader.object_store).to eq(described_class::Store::REMOTE)
       end
     end
+
+    describe '#to_h' do
+      subject { uploader.to_h }
+
+      let(:filename) { 'my+file.txt' }
+
+      it 'generates URL using original file name instead of filename returned by object storage' do
+        # GCS returns a URL with a `+` instead of `%2B`
+        allow(uploader.file).to receive(:url).and_return('https://storage.googleapis.com/gitlab-test-uploads/@hashed/6b/86/6b86b273ff34fce19d6b804eff5a3f5747ada4eaa22f1d49c01e52ddb7875b4b/64c5065e62100b1a12841644256a98be/my+file.txt')
+
+        expect(subject[:url]).to end_with(filename)
+      end
+    end
   end
 end