Build Content-Disposition per RFC 6266 and RFC 8187 (#813)

Author: sdogruyol

CHANGELOG.md

@@ -17,6 +17,7 @@ error 405 do |env|
 end
 ```
 
+- Build the `Content-Disposition` of `send_file` per [RFC 6266](https://www.rfc-editor.org/rfc/rfc6266#section-4.3) and [RFC 8187](https://www.rfc-editor.org/rfc/rfc8187). The filename was dropped into the quoted-string as it was: a `"` in it ended the parameter early, a non-ASCII name went out raw where the parameter is defined as ASCII, and a control character made the standard library reject the header with a `500`. `"` and `\\` are now escaped, other characters outside printable ASCII become `_` in `filename`, and when that loses anything the original name follows as `filename*=UTF-8''…`, which user agents prefer. A plain ASCII name produces the same header as before. `Kemal::Utils.content_disposition` is the builder.
 - `params.raw_body` returns the body of any request, not only a form or JSON one. It came back empty for `text/plain`, XML, or a request with no `Content-Type` at all, so such a body looked absent rather than unread; it is now read and cached the same way, under `max_request_body_size`. A `multipart/form-data` body is the one exception and still returns `""`, since `parse_files` streams it part by part. JSON detection now goes by media type instead of a string prefix: `application/vnd.api+json` and other `+json` types ([RFC 6839](https://www.rfc-editor.org/rfc/rfc6839)) parse into `params.json`, `Application/JSON` matches, and `application/jsonp` no longer does.
 - Skip the WebSocket upgrade when a `before` filter has already answered. A `halt` in a `before_all` — an authentication check answering `401` — closed the response, and `Kemal::WebSocketHandler` went on to attempt the upgrade regardless; the standard library handler raised `IO::Error: Closed stream` on the closed response. The client had its `401`, but every rejected handshake was logged as a server error. The handler now returns once it finds the response closed.
 - Stop confirming that a directory under `public/` exists when nothing is served for it. The standard library adds the trailing slash to a directory URL whenever its own `directory_listing` flag is on, and Kemal never set that flag from `serve_static`, so with `dir_listing` and `dir_index` both off `/admin` still answered `302 /admin/` while `/nope` answered `404` — the redirect was the only difference, and it told a scanner which directories are there. The redirect now happens only when a listing or an `index.html` would be served at the slashed URL; otherwise a directory falls through to the same `404` as a missing path.

spec/helpers_spec.cr

@@ -219,6 +219,15 @@ describe "Macros" do
       response.headers["Content-Disposition"].should eq("attachment; filename=\"image.jpg\"")
     end
 
+    it "sends a non-ASCII filename as an RFC 8187 filename* with an ASCII fallback" do
+      get "/" do |env|
+        send_file env, "#{__DIR__}/asset/hello.ecr", filename: "rapor ünlü.pdf"
+      end
+      response = call_request_on_app(HTTP::Request.new("GET", "/"))
+      response.status_code.should eq(200)
+      response.headers["Content-Disposition"].should eq(%(attachment; filename="rapor _nl_.pdf"; filename*=UTF-8''rapor%20%C3%BCnl%C3%BC.pdf))
+    end
+
     it "handles multiple range requests" do
       get "/" do |env|
         send_file env, "#{__DIR__}/asset/hello.ecr"

spec/utils_spec.cr

@@ -194,4 +194,28 @@ describe Kemal::Utils do
       end
     end
   {% end %}
+
+  describe ".content_disposition" do
+    it "quotes a plain ASCII name as it is" do
+      Kemal::Utils.content_disposition("attachment", "report.pdf").should eq(%(attachment; filename="report.pdf"))
+      Kemal::Utils.content_disposition("inline", "a b.txt").should eq(%(inline; filename="a b.txt"))
+    end
+
+    it "escapes the quote and the backslash inside the quoted-string" do
+      # An unescaped `"` ended the parameter early: `filename="report "final".pdf"`.
+      Kemal::Utils.content_disposition("attachment", %(report "final".pdf)).should eq(%q(attachment; filename="report \"final\".pdf"))
+      Kemal::Utils.content_disposition("attachment", %q(back\slash.txt)).should eq(%q(attachment; filename="back\\slash.txt"))
+    end
+
+    it "adds an RFC 8187 filename* for a name outside ASCII" do
+      Kemal::Utils.content_disposition("attachment", "rapor ünlü.pdf")
+        .should eq(%(attachment; filename="rapor _nl_.pdf"; filename*=UTF-8''rapor%20%C3%BCnl%C3%BC.pdf))
+    end
+
+    it "keeps control characters out of the header" do
+      # A CR/LF in the name used to make the stdlib reject the header, a 500.
+      Kemal::Utils.content_disposition("attachment", "line\r\nX: 1.txt")
+        .should eq(%(attachment; filename="line__X: 1.txt"; filename*=UTF-8''line%0D%0AX%3A%201.txt))
+    end
+  end
 end

src/kemal/helpers/helpers.cr

@@ -373,7 +373,7 @@ end
 private def attachment(env : HTTP::Server::Context, filename : String? = nil, disposition : String? = nil)
   disposition = "attachment" if disposition.nil? && filename
   if disposition && filename
-    env.response.headers["Content-Disposition"] = "#{disposition}; filename=\"#{File.basename(filename)}\""
+    env.response.headers["Content-Disposition"] = Kemal::Utils.content_disposition(disposition, File.basename(filename))
   end
 end
 

src/kemal/helpers/utils.cr

@@ -207,5 +207,57 @@ module Kemal
         headers["Etag"] = etag_with_coding(etag, coding)
       end
     end
+
+    # Builds a `Content-Disposition` field value for *filename*
+    # ([RFC 6266 §4.3](https://www.rfc-editor.org/rfc/rfc6266#section-4.3)).
+    #
+    # The `filename` parameter is a quoted-string and has to stay in ASCII, so `"` and
+    # `\\` are escaped as quoted-pairs and every other character outside printable ASCII
+    # becomes `_`. When that loses something, the original name follows as
+    # `filename*=UTF-8''...` percent-encoded per
+    # [RFC 8187](https://www.rfc-editor.org/rfc/rfc8187), which user agents that
+    # understand it prefer. A name that is plain ASCII to begin with gets only the
+    # first form, unchanged.
+    #
+    # ```
+    # Kemal::Utils.content_disposition("attachment", "report.pdf")  # => %(attachment; filename="report.pdf")
+    # Kemal::Utils.content_disposition("attachment", "rapor ü.pdf") # => %(attachment; filename="rapor _.pdf"; filename*=UTF-8''rapor%20%C3%BC.pdf)
+    # ```
+    def self.content_disposition(disposition : String, filename : String) : String
+      String.build do |header|
+        header << disposition << %(; filename=")
+        lossy = false
+        filename.each_char do |char|
+          case char
+          when '"', '\\'
+            header << '\\' << char
+          when ' '..'~'
+            header << char
+          else
+            lossy = true
+            header << '_'
+          end
+        end
+        header << '"'
+
+        if lossy
+          header << "; filename*=UTF-8''"
+          percent_encode_attr_chars(filename, header)
+        end
+      end
+    end
+
+    # Percent-encodes everything in *value* that is not an RFC 8187 `attr-char`.
+    private def self.percent_encode_attr_chars(value : String, io : IO) : Nil
+      value.each_byte do |byte|
+        char = byte.unsafe_chr
+        if char.ascii_alphanumeric? || "!#$&+-.^_`|~".includes?(char)
+          io << char
+        else
+          io << '%'
+          io << byte.to_s(16, upcase: true).rjust(2, '0')
+        end
+      end
+    end
   end
 end