ResourceController#download : répare réponse HEAD (#3740)

* ResourceController#download : répare réponse HEAD

* Refactor: allowlist of forwarded headers

apps/shared/lib/proxy.ex

@@ -0,0 +1,22 @@
+defmodule Shared.Proxy do
+  @moduledoc """
+  Shared methods useful when proxying requests in our apps.
+  """
+
+  @doc """
+  A list of HTTP headers that will be forwarded by our proxy.
+
+  For now we use an allowlist we can gradually expand.
+  Make sure to avoid including "hop-by-hop" headers here.
+  https://book.hacktricks.xyz/pentesting-web/abusing-hop-by-hop-headers
+  """
+  def forwarded_headers_allowlist do
+    [
+      "content-type",
+      "content-length",
+      "date",
+      "last-modified",
+      "etag"
+    ]
+  end
+end

apps/transport/lib/transport_web/controllers/resource_controller.ex

@@ -274,11 +274,9 @@ defmodule TransportWeb.ResourceController do
     else
       case Transport.Shared.Wrapper.HTTPoison.impl().head(resource.url, []) do
         {:ok, %HTTPoison.Response{status_code: status_code, headers: headers}} ->
-          Logger.info("PROBE #1 - match")
-          send_response_with_status_headers(conn, status_code, headers)
+          send_head_response(conn, status_code, headers)
 
-        e ->
-          Logger.info("PROBE #2 - no match - #{e |> inspect}")
+        _ ->
           conn |> Plug.Conn.send_resp(:bad_gateway, "")
       end
     end
@@ -292,7 +290,6 @@ defmodule TransportWeb.ResourceController do
     else
       case Transport.Shared.Wrapper.HTTPoison.impl().get(resource.url, [], hackney: [follow_redirect: true]) do
         {:ok, %HTTPoison.Response{status_code: 200} = response} ->
-          Logger.info("PROBE #3 - match")
           headers = Enum.into(response.headers, %{}, &downcase_header(&1))
           %{"content-type" => content_type} = headers
 
@@ -302,9 +299,7 @@ defmodule TransportWeb.ResourceController do
             filename: Transport.FileDownloads.guess_filename(headers, resource.url)
           )
 
-        e ->
-          Logger.info("PROBE #4 - no match - #{e |> inspect}")
-
+        _ ->
           conn
           |> put_flash(:error, dgettext("resource", "Resource is not available on remote server"))
           |> put_status(:not_found)
@@ -314,14 +309,17 @@ defmodule TransportWeb.ResourceController do
     end
   end
 
-  defp downcase_header({h, v}), do: {String.downcase(h), v}
+  defp send_head_response(%Plug.Conn{} = conn, status_code, headers) do
+    resp_headers =
+      headers
+      |> Enum.map(&downcase_header/1)
+      |> Enum.filter(fn {h, _v} -> Enum.member?(Shared.Proxy.forwarded_headers_allowlist(), h) end)
 
-  defp send_response_with_status_headers(%Plug.Conn{} = conn, status_code, headers) do
-    headers
-    |> Enum.reduce(conn, fn {k, v}, conn -> Plug.Conn.put_resp_header(conn, String.downcase(k), v) end)
-    |> Plug.Conn.send_resp(status_code, "")
+    conn |> Plug.Conn.merge_resp_headers(resp_headers) |> Plug.Conn.send_resp(status_code, "")
   end
 
+  defp downcase_header({h, v}), do: {String.downcase(h), v}
+
   @spec post_file(Plug.Conn.t(), map) :: Plug.Conn.t()
   def post_file(conn, params) do
     success_message =

apps/transport/test/transport_web/controllers/resource_controller_test.exs

@@ -230,14 +230,27 @@ defmodule TransportWeb.ResourceControllerTest do
     Transport.HTTPoison.Mock
     |> expect(:head, fn url, [] ->
       assert url == resource.url
-      {:ok, %HTTPoison.Response{status_code: 200, headers: [{"Content-Type", "application/zip"}, {"foo", "bar"}]}}
+
+      {:ok,
+       %HTTPoison.Response{
+         status_code: 200,
+         headers: [
+           {"Content-Type", "application/zip"},
+           {"foo", "bar"},
+           {"transfer-encoding", "chunked"},
+           {"date", "date_value"}
+         ]
+       }}
     end)
 
-    conn = conn |> head(resource_path(conn, :download, resource.id))
+    assert %Plug.Conn{assigns: %{original_method: "HEAD"}, resp_body: "", status: 200} =
+             conn = conn |> head(resource_path(conn, :download, resource.id))
+
     assert ["application/zip"] == conn |> get_resp_header("content-type")
-    assert ["bar"] == conn |> get_resp_header("foo")
-    assert conn.assigns[:original_method] == "HEAD"
-    assert conn |> response(200) == ""
+    assert ["date_value"] == conn |> get_resp_header("date")
+    # Headers absent from the allowlist have been removed
+    assert [] == conn |> get_resp_header("foo")
+    assert [] == conn |> get_resp_header("transfer-encoding")
 
     # With a resource that can be directly downloaded
     resource = DB.Resource |> DB.Repo.get_by(datagouv_id: "1")

apps/unlock/lib/controller.ex

@@ -51,16 +51,6 @@ defmodule Unlock.Controller do
     text(conn, "Unlock Proxy")
   end
 
-  # for now we use a allowlist which we'll gradually expand.
-  # make sure to avoid including "hop-by-hop" headers here.
-  @forwarded_headers_allowlist [
-    "content-type",
-    "content-length",
-    "date",
-    "last-modified",
-    "etag"
-  ]
-
   def fetch(conn, %{"id" => id}) do
     config = Application.fetch_env!(:unlock, :config_fetcher).fetch_config!()
 
@@ -231,8 +221,7 @@ defmodule Unlock.Controller do
 
   # Inspiration (MIT) here https://github.com/tallarium/reverse_proxy_plug
   defp filter_response_headers(headers) do
-    headers
-    |> Enum.filter(fn {h, _v} -> Enum.member?(@forwarded_headers_allowlist, h) end)
+    Enum.filter(headers, fn {h, _v} -> Enum.member?(Shared.Proxy.forwarded_headers_allowlist(), h) end)
   end
 
   defp prepare_response_headers(headers) do