summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/controllers/concerns/send_file_upload.rb19
-rw-r--r--changelogs/unreleased/sh-encode-content-disposition.yml5
-rw-r--r--lib/api/helpers.rb10
-rw-r--r--lib/gitlab/content_disposition.rb54
-rw-r--r--spec/controllers/concerns/send_file_upload_spec.rb25
-rw-r--r--spec/controllers/projects/artifacts_controller_spec.rb16
-rw-r--r--spec/features/projects/artifacts/user_downloads_artifacts_spec.rb2
-rw-r--r--spec/features/projects/jobs_spec.rb2
-rw-r--r--spec/lib/api/helpers_spec.rb2
-rw-r--r--spec/requests/api/files_spec.rb2
-rw-r--r--spec/requests/api/jobs_spec.rb4
-rw-r--r--spec/requests/api/runner_spec.rb2
-rw-r--r--spec/support/shared_examples/controllers/repository_lfs_file_load_examples.rb10
13 files changed, 129 insertions, 24 deletions
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..32207514ce5
--- /dev/null
+++ b/lib/gitlab/content_disposition.rb
@@ -0,0 +1,54 @@
+# 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:
+ # Make sure we remove this patch starting with Rails 6.0.
+ if Rails.version.start_with?('6.0')
+ raise <<~MSG
+ Please remove this file and use `ActionDispatch::Http::ContentDisposition` instead.
+ MSG
+ end
+
+ 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
View Lorry Controller Status