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

Encode Content-Disposition filenames

Closes #47673

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

9 files changed, 51 insertions, 14 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
diff --git a/spec/features/projects/artifacts/user_downloads_artifacts_spec.rb b/spec/features/projects/artifacts/user_downloads_artifacts_spec.rb
index 554f0b49052..5cb015e80be 100644
--- a/spec/features/projects/artifacts/user_downloads_artifacts_spec.rb
+++ b/spec/features/projects/artifacts/user_downloads_artifacts_spec.rb
@@ -7,7 +7,7 @@ describe "User downloads artifacts" do
 
   shared_examples "downloading" do
     it "downloads the zip" do
-      expect(page.response_headers["Content-Disposition"]).to eq(%Q{attachment; filename="#{job.artifacts_file.filename}"})
+      expect(page.response_headers["Content-Disposition"]).to eq(%Q{attachment; filename*=UTF-8''#{job.artifacts_file.filename}; filename="#{job.artifacts_file.filename}"})
       expect(page.response_headers['Content-Transfer-Encoding']).to eq("binary")
       expect(page.response_headers['Content-Type']).to eq("application/zip")
       expect(page.source.b).to eq(job.artifacts_file.file.read.b)
diff --git a/spec/features/projects/jobs_spec.rb b/spec/features/projects/jobs_spec.rb
index 24830b2bd3e..65ce872363f 100644
--- a/spec/features/projects/jobs_spec.rb
+++ b/spec/features/projects/jobs_spec.rb
@@ -220,7 +220,7 @@ describe 'Jobs', :clean_gitlab_redis_shared_state do
 
         artifact_request = requests.find { |req| req.url.match(%r{artifacts/download}) }
 
-        expect(artifact_request.response_headers["Content-Disposition"]).to eq(%Q{attachment; filename="#{job.artifacts_file.filename}"})
+        expect(artifact_request.response_headers["Content-Disposition"]).to eq(%Q{attachment; filename*=UTF-8''#{job.artifacts_file.filename}; filename="#{job.artifacts_file.filename}"})
         expect(artifact_request.response_headers['Content-Transfer-Encoding']).to eq("binary")
         expect(artifact_request.response_headers['Content-Type']).to eq("image/gif")
         expect(artifact_request.body).to eq(job.artifacts_file.file.read.b)
diff --git a/spec/lib/api/helpers_spec.rb b/spec/lib/api/helpers_spec.rb
index e1aea82653d..08165f147bb 100644
--- a/spec/lib/api/helpers_spec.rb
+++ b/spec/lib/api/helpers_spec.rb
@@ -179,7 +179,7 @@ describe API::Helpers do
 
       context 'when blob name is not null' do
         it 'returns disposition with the blob name' do
-          expect(send_git_blob['Content-Disposition']).to eq 'inline; filename="foobar"'
+          expect(send_git_blob['Content-Disposition']).to eq %q(inline; filename="foobar"; filename*=UTF-8''foobar)
         end
       end
     end
diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb
index 9b32dc78274..1ad536258ba 100644
--- a/spec/requests/api/files_spec.rb
+++ b/spec/requests/api/files_spec.rb
@@ -191,7 +191,7 @@ describe API::Files do
 
         get api(url, current_user), params: params
 
-        expect(headers['Content-Disposition']).to eq('inline; filename="popen.rb"')
+        expect(headers['Content-Disposition']).to eq(%q(inline; filename="popen.rb"; filename*=UTF-8''popen.rb))
       end
 
       context 'when mandatory params are not given' do
diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb
index 97aa71bf231..3defe8bbf51 100644
--- a/spec/requests/api/jobs_spec.rb
+++ b/spec/requests/api/jobs_spec.rb
@@ -403,7 +403,7 @@ describe API::Jobs do
     shared_examples 'downloads artifact' do
       let(:download_headers) do
         { 'Content-Transfer-Encoding' => 'binary',
-          'Content-Disposition' => 'attachment; filename=ci_build_artifacts.zip' }
+          'Content-Disposition' => %q(attachment; filename="ci_build_artifacts.zip"; filename*=UTF-8''ci_build_artifacts.zip) }
       end
 
       it 'returns specific job artifacts' do
@@ -555,7 +555,7 @@ describe API::Jobs do
           let(:download_headers) do
             { 'Content-Transfer-Encoding' => 'binary',
               'Content-Disposition' =>
-                "attachment; filename=#{job.artifacts_file.filename}" }
+              %Q(attachment; filename="#{job.artifacts_file.filename}"; filename*=UTF-8''#{job.artifacts_file.filename}) }
           end
 
           it { expect(response).to have_http_status(:ok) }
diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index ed0108c846a..d7ddd97e8c8 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -1584,7 +1584,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             context 'when artifacts are stored locally' do
               let(:download_headers) do
                 { 'Content-Transfer-Encoding' => 'binary',
-                  'Content-Disposition' => 'attachment; filename=ci_build_artifacts.zip' }
+                  'Content-Disposition' => %q(attachment; filename="ci_build_artifacts.zip"; filename*=UTF-8''ci_build_artifacts.zip) }
               end
 
               before do
diff --git a/spec/support/shared_examples/controllers/repository_lfs_file_load_examples.rb b/spec/support/shared_examples/controllers/repository_lfs_file_load_examples.rb
index a3d31e26498..982e0317f7f 100644
--- a/spec/support/shared_examples/controllers/repository_lfs_file_load_examples.rb
+++ b/spec/support/shared_examples/controllers/repository_lfs_file_load_examples.rb
@@ -28,7 +28,13 @@ shared_examples 'repository lfs file load' do
         end
 
         it 'serves the file' do
-          expect(controller).to receive(:send_file).with("#{LfsObjectUploader.root}/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: filename, disposition: 'attachment')
+          # 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(
+                              "#{LfsObjectUploader.root}/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897",
+                              filename: filename,
+                              disposition: %Q(attachment; filename*=UTF-8''#{filename}))
 
           subject
 
@@ -56,7 +62,7 @@ shared_examples 'repository lfs file load' do
             file_uri = URI.parse(response.location)
             params = CGI.parse(file_uri.query)
 
-            expect(params["response-content-disposition"].first).to eq "attachment;filename=\"#{filename}\""
+            expect(params["response-content-disposition"].first).to eq(%q(attachment; filename="lfs_object.iso"; filename*=UTF-8''lfs_object.iso))
           end
         end
       end