Commit f04910f2 authored by Sean McGivern's avatar Sean McGivern

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

Encode Content-Disposition filenames

Closes #47673

See merge request gitlab-org/gitlab-ce!24919
parents fedecbbe e6850f73
......@@ -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)
......
---
title: Encode Content-Disposition filenames
merge_request: 24919
author:
type: fixed
......@@ -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
# 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
......@@ -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:/)
......
......@@ -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
......
......@@ -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)
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......
......@@ -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) }
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment