Encode Content-Disposition filenames

Users downloading non-ASCII attachments would see garbled characters.
When used with object storage, AWS S3 would return an InvalidArgument
error: Header value cannot be represented using ISO-8859-1.

Per RFC 5987 and RFC 6266, Content-Disposition should be encoded
properly. This commit takes the Rails 6 implementation of
ActiveSuppport::Http::ContentDisposition
(https://github.com/rails/rails/pull/33829) and ports it here.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/47673

app/controllers/concerns/send_file_upload.rb

@@ -3,16 +3,19 @@
 module SendFileUpload
   def send_upload(file_upload, send_params: {}, redirect_params: {}, attachment: nil, proxy: false, disposition: 'attachment')
     if attachment
+      response_disposition = ::Gitlab::ContentDisposition.format(disposition: 'attachment', filename: attachment)
+
       # Response-Content-Type will not override an existing Content-Type in
       # Google Cloud Storage, so the metadata needs to be cleared on GCS for
       # this to work. However, this override works with AWS.
-      redirect_params[:query] = { "response-content-disposition" => "#{disposition};filename=#{attachment.inspect}",
+      redirect_params[:query] = { "response-content-disposition" => response_disposition,
                                   "response-content-type" => guess_content_type(attachment) }
       # By default, Rails will send uploads with an extension of .js with a
       # content-type of text/javascript, which will trigger Rails'
       # cross-origin JavaScript protection.
       send_params[:content_type] = 'text/plain' if File.extname(attachment) == '.js'
-      send_params.merge!(filename: attachment, disposition: disposition)
+
+      send_params.merge!(filename: attachment, disposition: utf8_encoded_disposition(disposition, attachment))
     end
 
     if file_upload.file_storage?
@@ -25,6 +28,18 @@ module SendFileUpload
     end
   end
 
+  # Since Rails 5 doesn't properly support support non-ASCII filenames,
+  # we have to add our own to ensure RFC 5987 compliance. However, Rails
+  # 5 automatically appends `filename#{filename}` here:
+  # https://github.com/rails/rails/blob/v5.0.7/actionpack/lib/action_controller/metal/data_streaming.rb#L137
+  # Rails 6 will have https://github.com/rails/rails/pull/33829, so we
+  # can get rid of this special case handling when we upgrade.
+  def utf8_encoded_disposition(disposition, filename)
+    content = ::Gitlab::ContentDisposition.new(disposition: disposition, filename: filename)
+
+    "#{disposition}; #{content.utf8_filename}"
+  end
+
   def guess_content_type(filename)
     types = MIME::Types.type_for(filename)
 

changelogs/unreleased/sh-encode-content-disposition.yml

@@ -0,0 +1,5 @@
+---
+title: Encode Content-Disposition filenames
+merge_request: 24919
+author:
+type: fixed

lib/api/helpers.rb

@@ -422,7 +422,7 @@ module API
 
     def present_disk_file!(path, filename, content_type = 'application/octet-stream')
       filename ||= File.basename(path)
-      header['Content-Disposition'] = "attachment; filename=#{filename}"
+      header['Content-Disposition'] = ::Gitlab::ContentDisposition.format(disposition: 'attachment', filename: filename)
       header['Content-Transfer-Encoding'] = 'binary'
       content_type content_type
 
@@ -496,7 +496,7 @@ module API
     def send_git_blob(repository, blob)
       env['api.format'] = :txt
       content_type 'text/plain'
-      header['Content-Disposition'] = content_disposition('inline', blob.name)
+      header['Content-Disposition'] = ::Gitlab::ContentDisposition.format(disposition: 'inline', filename: blob.name)
 
       # Let Workhorse examine the content and determine the better content disposition
       header[Gitlab::Workhorse::DETECT_HEADER] = "true"
@@ -533,11 +533,5 @@ module API
 
       params[:archived]
     end
-
-    def content_disposition(disposition, filename)
-      disposition += %(; filename=#{filename.inspect}) if filename.present?
-
-      disposition
-    end
   end
 end

lib/gitlab/content_disposition.rb

@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+# This ports ActionDispatch::Http::ContentDisposition (https://github.com/rails/rails/pull/33829,
+# which will be available in Rails 6.
+module Gitlab
+  class ContentDisposition # :nodoc:
+    def self.format(disposition:, filename:)
+      new(disposition: disposition, filename: filename).to_s
+    end
+
+    attr_reader :disposition, :filename
+
+    def initialize(disposition:, filename:)
+      @disposition = disposition
+      @filename = filename
+    end
+
+    # rubocop:disable Style/VariableInterpolation
+    TRADITIONAL_ESCAPED_CHAR = /[^ A-Za-z0-9!#$+.^_`|~-]/
+
+    def ascii_filename
+      'filename="' + percent_escape(::I18n.transliterate(filename), TRADITIONAL_ESCAPED_CHAR) + '"'
+    end
+
+    RFC_5987_ESCAPED_CHAR = /[^A-Za-z0-9!#$&+.^_`|~-]/
+    # rubocop:enable Style/VariableInterpolation
+
+    def utf8_filename
+      "filename*=UTF-8''" + percent_escape(filename, RFC_5987_ESCAPED_CHAR)
+    end
+
+    def to_s
+      if filename
+        "#{disposition}; #{ascii_filename}; #{utf8_filename}"
+      else
+        "#{disposition}"
+      end
+    end
+
+    private
+
+    def percent_escape(string, pattern)
+      string.gsub(pattern) do |char|
+        char.bytes.map { |byte| "%%%02X" % byte }.join
+      end
+    end
+  end
+end

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:/)
 

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

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)

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)

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

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

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) }

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

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