From 41b51c065604091579a2308adc527fe5bb187abe Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 4 Feb 2019 17:27:22 -0800 Subject: [PATCH] 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 | 19 +++++++- .../sh-encode-content-disposition.yml | 5 ++ lib/api/helpers.rb | 10 +--- lib/gitlab/content_disposition.rb | 47 +++++++++++++++++++ .../concerns/send_file_upload_spec.rb | 25 ++++++++-- .../projects/artifacts_controller_spec.rb | 16 ++++++- .../user_downloads_artifacts_spec.rb | 2 +- spec/features/projects/jobs_spec.rb | 2 +- spec/lib/api/helpers_spec.rb | 2 +- spec/requests/api/files_spec.rb | 2 +- spec/requests/api/jobs_spec.rb | 4 +- spec/requests/api/runner_spec.rb | 2 +- .../repository_lfs_file_load_examples.rb | 10 +++- 13 files changed, 122 insertions(+), 24 deletions(-) create mode 100644 changelogs/unreleased/sh-encode-content-disposition.yml create mode 100644 lib/gitlab/content_disposition.rb diff --git a/app/controllers/concerns/send_file_upload.rb b/app/controllers/concerns/send_file_upload.rb index 515a9eede8e..9ca54c5519b 100644 --- a/app/controllers/concerns/send_file_upload.rb +++ b/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) diff --git a/changelogs/unreleased/sh-encode-content-disposition.yml b/changelogs/unreleased/sh-encode-content-disposition.yml new file mode 100644 index 00000000000..b40ee6a85a8 --- /dev/null +++ b/changelogs/unreleased/sh-encode-content-disposition.yml @@ -0,0 +1,5 @@ +--- +title: Encode Content-Disposition filenames +merge_request: 24919 +author: +type: fixed diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index fa6c9777824..e3d0b981065 100644 --- a/lib/api/helpers.rb +++ b/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 diff --git a/lib/gitlab/content_disposition.rb b/lib/gitlab/content_disposition.rb new file mode 100644 index 00000000000..96ead8a9fbf --- /dev/null +++ b/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 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