Merge pull request #33829 from mtsmfm/encode-filename

Encode Content-Disposition filenames on send_data and send_file

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.
 

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 @@ def send_file_headers!(options)
         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"

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

actionpack/test/controller/send_file_test.rb

@@ -144,7 +144,7 @@ def test_send_file_headers_bang
       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 @@ def test_send_file_headers_bang
   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

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

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 @@ def sanitized
     @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

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 @@ def service_name
       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

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