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 + 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(:filename) { "my file.txt" } + let(:uploaded_file) do + UploadedFile.new(temp_file.path, filename: filename, 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 + + before do + FileUtils.touch(temp_file) + 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.new(key: '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 + 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