Merge branch 'sh-encode-content-disposition' into 'master'

Encode Content-Disposition filenames

Closes #47673

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

2 files changed, 36 insertions, 5 deletions

diff --git a/spec/controllers/concerns/send_file_upload_spec.rb b/spec/controllers/concerns/send_file_upload_spec.rb
index 379b2d6b935..a07113a6156 100644
--- a/spec/controllers/concerns/send_file_upload_spec.rb
+++ b/spec/controllers/concerns/send_file_upload_spec.rb
@@ -53,19 +53,38 @@ describe SendFileUpload do
     end
 
     context 'with attachment' do
-      let(:params) { { attachment: 'test.js' } }
+      let(:filename) { 'test.js' }
+      let(:params) { { attachment: filename } }
 
       it 'sends a file with content-type of text/plain' do
+        # Notice the filename= is omitted from the disposition; this is because
+        # Rails 5 will append this header in send_file
         expected_params = {
           content_type: 'text/plain',
           filename: 'test.js',
-          disposition: 'attachment'
+          disposition: "attachment; filename*=UTF-8''test.js"
         }
         expect(controller).to receive(:send_file).with(uploader.path, expected_params)
 
         subject
       end
 
+      context 'with non-ASCII encoded filename' do
+        let(:filename) { 'テスト.txt' }
+
+        # Notice the filename= is omitted from the disposition; this is because
+        # Rails 5 will append this header in send_file
+        it 'sends content-disposition for non-ASCII encoded filenames' do
+          expected_params = {
+            filename: filename,
+            disposition: "attachment; filename*=UTF-8''%E3%83%86%E3%82%B9%E3%83%88.txt"
+          }
+          expect(controller).to receive(:send_file).with(uploader.path, expected_params)
+
+          subject
+        end
+      end
+
       context 'with a proxied file in object storage' do
         before do
           stub_uploads_object_storage(uploader: uploader_class)
@@ -76,7 +95,7 @@ describe SendFileUpload do
 
         it 'sends a file with a custom type' do
           headers = double
-          expected_headers = %r(response-content-disposition=attachment%3Bfilename%3D%22test.js%22&response-content-type=application/ecmascript)
+          expected_headers = %r(response-content-disposition=attachment%3B%20filename%3D%22test.js%22%3B%20filename%2A%3DUTF-8%27%27test.js&response-content-type=application/ecmascript)
           expect(Gitlab::Workhorse).to receive(:send_url).with(expected_headers).and_call_original
           expect(headers).to receive(:store).with(Gitlab::Workhorse::SEND_DATA_HEADER, /^send-url:/)
 
diff --git a/spec/controllers/projects/artifacts_controller_spec.rb b/spec/controllers/projects/artifacts_controller_spec.rb
index bd10de45b67..29df00e6bb0 100644
--- a/spec/controllers/projects/artifacts_controller_spec.rb
+++ b/spec/controllers/projects/artifacts_controller_spec.rb
@@ -26,8 +26,15 @@ describe Projects::ArtifactsController do
     end
 
     context 'when no file type is supplied' do
+      let(:filename) { job.artifacts_file.filename }
+
       it 'sends the artifacts file' do
-        expect(controller).to receive(:send_file).with(job.artifacts_file.path, hash_including(disposition: 'attachment')).and_call_original
+        # Notice the filename= is omitted from the disposition; this is because
+        # Rails 5 will append this header in send_file
+        expect(controller).to receive(:send_file)
+                          .with(
+                            job.artifacts_file.file.path,
+                            hash_including(disposition: %Q(attachment; filename*=UTF-8''#{filename}))).and_call_original
 
         download_artifact
       end
@@ -46,6 +53,7 @@ describe Projects::ArtifactsController do
 
       context 'when codequality file type is supplied' do
         let(:file_type) { 'codequality' }
+        let(:filename) { job.job_artifacts_codequality.filename }
 
         context 'when file is stored locally' do
           before do
@@ -53,7 +61,11 @@ describe Projects::ArtifactsController do
           end
 
           it 'sends the codequality report' do
-            expect(controller).to receive(:send_file).with(job.job_artifacts_codequality.file.path, hash_including(disposition: 'attachment')).and_call_original
+            # Notice the filename= is omitted from the disposition; this is because
+            # Rails 5 will append this header in send_file
+            expect(controller).to receive(:send_file)
+                              .with(job.job_artifacts_codequality.file.path,
+                                    hash_including(disposition: %Q(attachment; filename*=UTF-8''#{filename}))).and_call_original
 
             download_artifact(file_type: file_type)
           end