Repository: phoenixframework/phoenix
Pull request #3344 (2019-04-03 13:52:51)

Enable to use send_download without encode
- Motivation
I was creating a system the enable users to download files with a
variety of names the put spaces and other characters. When downloading
this kind of file names like `my custom report.xlsx` was returning
`my+custom+report.xlsx`. Esthetically it is ugly to users and they need to edit it to download with the correct nam

- Solution
I created the possibility to send option `encode`, the default value of
the option is nil, and it only avoid the encode_www_form if it was
passed as `false`

- Issue: #3343

michalmuskala: Wouldn't that be a potential [request splitting][1] vulnerability if the filename is not encoded and potentially controlled by the user?

[1]: https://www.owasp.org/index.php/HTTP_Response_Splitting
AllanKlaus: > Wouldn't that be a potential [request splitting](https://www.owasp.org/index.php/HTTP_Response_Splitting) vulnerability if the filename is not encoded and potentially controlled by the user?

I don't think so.

I tryed to do the attack inside my code like this

```
my_report = "./my_report.xlsx"
filename = my_report <> "\"\r\n%0d%0aConnection:%20Keep-Alive%0d%0aContent-Length:%200%0d%0a%0d%0aHTTP/1.0%20200%20OK%0d%0aContent-Type:%20text/html%0a%0aContent-Length:%2020%0d%0a%0d%0a<html>YOU_ARE_DONE!</html>"

    conn
    |> put_resp_content_type(content_type, "utf-8")
    |> put_resp_header("content-disposition", ~s[attachment; filename="#{filename}"])
    |> send_file(conn.status || 200, my_report, 0, :all)
```

I tryed using `\r\n` and using`%0d%0a`. Tryed to close the quote of file name using a escaped quote and nothing interfered on the request.
josevalim: Oh, I commented in the wrong place and I missed this discussion.

Even if we are safe request splitting in this case (likely because cowboy rejects \r\n), I think it would be unsafe for Phoenix to assume something else is catching his back. Maybe in the future we are using another  server or Cowboy stops checking it, and now Phoenix has a large vulnerability.

So as mentioned in my comment above, I would first test if `URI.encode` is a better choice. In case it isn't, we can provide a verbose way to disable encoding, but it should most certainly be enabled  out of the box.
AllanKlaus: > Oh, I commented in the wrong place and I missed this discussion.
> 
> Even if we are safe request splitting in this case (likely because cowboy rejects \r\n), I think it would be unsafe for Phoenix to assume something else is catching his back. Maybe in the future we are using another server or Cowboy stops checking it, and now Phoenix has a large vulnerability.
> 
> So as mentioned in my comment above, I would first test if `URI.encode` is a better choice. In case it isn't, we can provide a verbose way to disable encoding, but it should most certainly be enabled out of the box.

@josevalim when the name of the file is **my report.xlsx** using `URI.encode` will return **my%20report.xlxs**. So it's discarted to use `URI.encode`.

To don't change the current behavior the default value to encode is `nil`, in this case it will always encode, only when `encode` is `false` it will don't apply the encode. Thinking in the cowboy i agree we need to try another approach. What do you mean in verbose way? 



AllanKlaus: @josevalim I applied the changes you suggested. I'm new using elixir, I learned a lot with this pr, thanks.

Patch:
diff --git a/lib/phoenix/controller.ex b/lib/phoenix/controller.ex
index 6c151701e..ffc154c5f 100644
--- a/lib/phoenix/controller.ex
+++ b/lib/phoenix/controller.ex
@@ -884,6 +884,11 @@ defmodule Phoenix.Controller do
       Defaults to none
     * `:offset` - the bytes to offset when reading. Defaults to `0`
     * `:length` - the total bytes to read. Defaults to `:all`
+    * `:encode` - encodes the filename using `URI.encode_www_form/1`.
+      Defaults to `true`. When `false`, disables encoding. If you
+      disable encoding, you need to guarantee there are no special
+      characters in the filename, such as quotes, newlines, etc.
+      Otherwise you can expose your application to security attacks
 
   ## Examples
 
@@ -925,13 +930,16 @@ defmodule Phoenix.Controller do
 
   defp prepare_send_download(conn, filename, opts) do
     content_type = opts[:content_type] || MIME.from_path(filename)
-    encoded_filename = URI.encode_www_form(filename)
+    encoded_filename = encode_filename(filename, Keyword.get(opts, :encode, true))
     warn_if_ajax(conn)
     conn
     |> put_resp_content_type(content_type, opts[:charset])
     |> put_resp_header("content-disposition", ~s[attachment; filename="#{encoded_filename}"])
   end
 
+  defp encode_filename(filename, false), do: filename
+  defp encode_filename(filename, true), do: URI.encode_www_form(filename)
+
   defp ajax?(conn) do
     case get_req_header(conn, "x-requested-with") do
       [value] -> value in ["XMLHttpRequest", "xmlhttprequest"]


Test patch:
diff --git a/test/phoenix/controller/controller_test.exs b/test/phoenix/controller/controller_test.exs
index 817dd6327..a35fb97bd 100644
--- a/test/phoenix/controller/controller_test.exs
+++ b/test/phoenix/controller/controller_test.exs
@@ -402,6 +402,17 @@ defmodule Phoenix.Controller.ControllerTest do
              "world"
     end
 
+    test "sends file for download with custom :filename and :encode false" do
+      conn = send_download(conn(:get, "/"), {:file, @hello_txt}, filename: "dev's hello world.json", encode: false)
+      assert conn.status == 200
+      assert get_resp_header(conn, "content-disposition") ==
+             ["attachment; filename=\"dev's hello world.json\""]
+      assert get_resp_header(conn, "content-type") ==
+             ["application/json"]
+      assert conn.resp_body ==
+             "world"
+    end
+
     test "sends file for download with custom :content_type and :charset" do
       conn = send_download(conn(:get, "/"), {:file, @hello_txt}, content_type: "application/json", charset: "utf8")
       assert conn.status == 200