From 890485cfce4c361c03a41ec23b0ba187007818cc Mon Sep 17 00:00:00 2001 From: Fumiaki MATSUSHIMA Date: Sun, 9 Sep 2018 16:35:48 +0900 Subject: Encode Content-Disposition filenames on send_data and send_file --- actionpack/CHANGELOG.md | 12 ++++++ .../lib/action_controller/metal/data_streaming.rb | 7 ++-- .../action_dispatch/http/content_disposition.rb | 45 ++++++++++++++++++++++ actionpack/test/controller/send_file_test.rb | 4 +- .../test/dispatch/content_disposition_test.rb | 37 ++++++++++++++++++ .../app/models/active_storage/filename.rb | 6 --- .../models/active_storage/filename/parameters.rb | 36 ----------------- activestorage/lib/active_storage/service.rb | 5 ++- activestorage/test/models/blob_test.rb | 2 +- .../test/models/filename/parameters_test.rb | 32 --------------- 10 files changed, 104 insertions(+), 82 deletions(-) create mode 100644 actionpack/lib/action_dispatch/http/content_disposition.rb create mode 100644 actionpack/test/dispatch/content_disposition_test.rb delete mode 100644 activestorage/app/models/active_storage/filename/parameters.rb delete mode 100644 activestorage/test/models/filename/parameters_test.rb diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 7781980cab..dfe6e00865 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,15 @@ +* Encode Content-Disposition filenames on `send_data` and `send_file`. + Previously, `send_data 'data', filename: "\u{3042}.txt"` sends + `"filename=\"\u{3042}.txt\""` as Content-Disposition and it can be + garbled. + Now it follows [RFC 2231](https://tools.ietf.org/html/rfc2231) and + [RFC 5987](https://tools.ietf.org/html/rfc5987) and sends + `"filename=\"%3F.txt\"; filename*=UTF-8''%E3%81%82.txt"`. + Most browsers can find filename correctly and old browsers fallback to ASCII + converted name. + + *Fumiaki Matsushima* + * Expose `ActionController::Parameters#each_key` which allows iterating over keys without allocating an array. diff --git a/actionpack/lib/action_controller/metal/data_streaming.rb b/actionpack/lib/action_controller/metal/data_streaming.rb index 5a82ccf668..5140a667de 100644 --- a/actionpack/lib/action_controller/metal/data_streaming.rb +++ b/actionpack/lib/action_controller/metal/data_streaming.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "action_controller/metal/exceptions" +require "action_dispatch/http/content_disposition" module ActionController #:nodoc: # Methods for sending arbitrary data and for streaming files to the browser, @@ -132,10 +133,8 @@ module ActionController #:nodoc: end disposition = options.fetch(:disposition, DEFAULT_SEND_FILE_DISPOSITION) - unless disposition.nil? - disposition = disposition.to_s - disposition += %(; filename="#{options[:filename]}") if options[:filename] - headers["Content-Disposition"] = disposition + if disposition + headers["Content-Disposition"] = ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: options[:filename]) end headers["Content-Transfer-Encoding"] = "binary" diff --git a/actionpack/lib/action_dispatch/http/content_disposition.rb b/actionpack/lib/action_dispatch/http/content_disposition.rb new file mode 100644 index 0000000000..58164c1522 --- /dev/null +++ b/actionpack/lib/action_dispatch/http/content_disposition.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module ActionDispatch + module Http + 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 + + 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!#$&+.^_`|~-]/ + + 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 +end diff --git a/actionpack/test/controller/send_file_test.rb b/actionpack/test/controller/send_file_test.rb index 7b1a52b277..c917cdf761 100644 --- a/actionpack/test/controller/send_file_test.rb +++ b/actionpack/test/controller/send_file_test.rb @@ -144,7 +144,7 @@ class SendFileTest < ActionController::TestCase get :test_send_file_headers_bang assert_equal "image/png", response.content_type - assert_equal 'disposition; filename="filename"', response.get_header("Content-Disposition") + assert_equal %(disposition; filename="filename"; filename*=UTF-8''filename), response.get_header("Content-Disposition") assert_equal "binary", response.get_header("Content-Transfer-Encoding") assert_equal "private", response.get_header("Cache-Control") end @@ -153,7 +153,7 @@ class SendFileTest < ActionController::TestCase def test_send_file_headers_with_disposition_as_a_symbol get :test_send_file_headers_with_disposition_as_a_symbol - assert_equal 'disposition; filename="filename"', response.get_header("Content-Disposition") + assert_equal %(disposition; filename="filename"; filename*=UTF-8''filename), response.get_header("Content-Disposition") end def test_send_file_headers_with_mime_lookup_with_symbol diff --git a/actionpack/test/dispatch/content_disposition_test.rb b/actionpack/test/dispatch/content_disposition_test.rb new file mode 100644 index 0000000000..3f5959da6e --- /dev/null +++ b/actionpack/test/dispatch/content_disposition_test.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require "abstract_unit" + +module ActionDispatch + class ContentDispositionTest < ActiveSupport::TestCase + test "encoding a Latin filename" do + disposition = Http::ContentDisposition.new(disposition: :inline, filename: "racecar.jpg") + + assert_equal %(filename="racecar.jpg"), disposition.ascii_filename + assert_equal "filename*=UTF-8''racecar.jpg", disposition.utf8_filename + assert_equal "inline; #{disposition.ascii_filename}; #{disposition.utf8_filename}", disposition.to_s + end + + test "encoding a Latin filename with accented characters" do + disposition = Http::ContentDisposition.new(disposition: :inline, filename: "råcëçâr.jpg") + + assert_equal %(filename="racecar.jpg"), disposition.ascii_filename + assert_equal "filename*=UTF-8''r%C3%A5c%C3%AB%C3%A7%C3%A2r.jpg", disposition.utf8_filename + assert_equal "inline; #{disposition.ascii_filename}; #{disposition.utf8_filename}", disposition.to_s + end + + test "encoding a non-Latin filename" do + disposition = Http::ContentDisposition.new(disposition: :inline, filename: "автомобиль.jpg") + + assert_equal %(filename="%3F%3F%3F%3F%3F%3F%3F%3F%3F%3F.jpg"), disposition.ascii_filename + assert_equal "filename*=UTF-8''%D0%B0%D0%B2%D1%82%D0%BE%D0%BC%D0%BE%D0%B1%D0%B8%D0%BB%D1%8C.jpg", disposition.utf8_filename + assert_equal "inline; #{disposition.ascii_filename}; #{disposition.utf8_filename}", disposition.to_s + end + + test "without filename" do + disposition = Http::ContentDisposition.new(disposition: :inline, filename: nil) + + assert_equal "inline", disposition.to_s + end + end +end diff --git a/activestorage/app/models/active_storage/filename.rb b/activestorage/app/models/active_storage/filename.rb index bebb5e61b3..2a03e0173d 100644 --- a/activestorage/app/models/active_storage/filename.rb +++ b/activestorage/app/models/active_storage/filename.rb @@ -3,8 +3,6 @@ # Encapsulates a string representing a filename to provide convenient access to parts of it and sanitization. # A Filename instance is returned by ActiveStorage::Blob#filename, and is comparable so it can be used for sorting. class ActiveStorage::Filename - require_dependency "active_storage/filename/parameters" - include Comparable class << self @@ -60,10 +58,6 @@ class ActiveStorage::Filename @filename.encode(Encoding::UTF_8, invalid: :replace, undef: :replace, replace: "�").strip.tr("\u{202E}%$|:;/\t\r\n\\", "-") end - def parameters #:nodoc: - Parameters.new self - end - # Returns the sanitized version of the filename. def to_s sanitized.to_s diff --git a/activestorage/app/models/active_storage/filename/parameters.rb b/activestorage/app/models/active_storage/filename/parameters.rb deleted file mode 100644 index fb9ea10e49..0000000000 --- a/activestorage/app/models/active_storage/filename/parameters.rb +++ /dev/null @@ -1,36 +0,0 @@ -# frozen_string_literal: true - -class ActiveStorage::Filename::Parameters #:nodoc: - attr_reader :filename - - def initialize(filename) - @filename = filename - end - - def combined - "#{ascii}; #{utf8}" - end - - TRADITIONAL_ESCAPED_CHAR = /[^ A-Za-z0-9!#$+.^_`|~-]/ - - def ascii - 'filename="' + percent_escape(I18n.transliterate(filename.sanitized), TRADITIONAL_ESCAPED_CHAR) + '"' - end - - RFC_5987_ESCAPED_CHAR = /[^A-Za-z0-9!#$&+.^_`|~-]/ - - def utf8 - "filename*=UTF-8''" + percent_escape(filename.sanitized, RFC_5987_ESCAPED_CHAR) - end - - def to_s - combined - end - - private - def percent_escape(string, pattern) - string.gsub(pattern) do |char| - char.bytes.map { |byte| "%%%02X" % byte }.join - end - end -end diff --git a/activestorage/lib/active_storage/service.rb b/activestorage/lib/active_storage/service.rb index f915518f52..54ba08fb87 100644 --- a/activestorage/lib/active_storage/service.rb +++ b/activestorage/lib/active_storage/service.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true require "active_storage/log_subscriber" +require "action_dispatch" +require "action_dispatch/http/content_disposition" module ActiveStorage # Abstract class serving as an interface for concrete services. @@ -122,7 +124,8 @@ module ActiveStorage end def content_disposition_with(type: "inline", filename:) - (type.to_s.presence_in(%w( attachment inline )) || "inline") + "; #{filename.parameters}" + disposition = (type.to_s.presence_in(%w( attachment inline )) || "inline") + ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: filename.sanitized) end end end diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index 88c106a08b..1a6a89de56 100644 --- a/activestorage/test/models/blob_test.rb +++ b/activestorage/test/models/blob_test.rb @@ -185,7 +185,7 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase private def expected_url_for(blob, disposition: :inline, filename: nil) filename ||= blob.filename - query_string = { content_type: blob.content_type, disposition: "#{disposition}; #{filename.parameters}" }.to_param + query_string = { content_type: blob.content_type, disposition: ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: filename.sanitized) }.to_param "https://example.com/rails/active_storage/disk/#{ActiveStorage.verifier.generate(blob.key, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query_string}" end end diff --git a/activestorage/test/models/filename/parameters_test.rb b/activestorage/test/models/filename/parameters_test.rb deleted file mode 100644 index 431be00639..0000000000 --- a/activestorage/test/models/filename/parameters_test.rb +++ /dev/null @@ -1,32 +0,0 @@ -# frozen_string_literal: true - -require "test_helper" - -class ActiveStorage::Filename::ParametersTest < ActiveSupport::TestCase - test "parameterizing a Latin filename" do - filename = ActiveStorage::Filename.new("racecar.jpg") - - assert_equal %(filename="racecar.jpg"), filename.parameters.ascii - assert_equal "filename*=UTF-8''racecar.jpg", filename.parameters.utf8 - assert_equal "#{filename.parameters.ascii}; #{filename.parameters.utf8}", filename.parameters.combined - assert_equal filename.parameters.combined, filename.parameters.to_s - end - - test "parameterizing a Latin filename with accented characters" do - filename = ActiveStorage::Filename.new("råcëçâr.jpg") - - assert_equal %(filename="racecar.jpg"), filename.parameters.ascii - assert_equal "filename*=UTF-8''r%C3%A5c%C3%AB%C3%A7%C3%A2r.jpg", filename.parameters.utf8 - assert_equal "#{filename.parameters.ascii}; #{filename.parameters.utf8}", filename.parameters.combined - assert_equal filename.parameters.combined, filename.parameters.to_s - end - - test "parameterizing a non-Latin filename" do - filename = ActiveStorage::Filename.new("автомобиль.jpg") - - assert_equal %(filename="%3F%3F%3F%3F%3F%3F%3F%3F%3F%3F.jpg"), filename.parameters.ascii - assert_equal "filename*=UTF-8''%D0%B0%D0%B2%D1%82%D0%BE%D0%BC%D0%BE%D0%B1%D0%B8%D0%BB%D1%8C.jpg", filename.parameters.utf8 - assert_equal "#{filename.parameters.ascii}; #{filename.parameters.utf8}", filename.parameters.combined - assert_equal filename.parameters.combined, filename.parameters.to_s - end -end -- cgit v1.2.3