Repository: tallarium/reverse_proxy_plug (elixir)

Issue:
Consistently remove hop-by-hop headers
Hop-by-hop headers are stripped from buffer mode responses as of #7, but we should ensure they are stripped from forwarded requests and responses in both stream and buffer mode.


https://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-14#section-7.1.3

Pull request description:
Strip all hop by hop headers on request and response
fixes #9 

Patch:
diff --git a/lib/reverse_proxy_plug.ex b/lib/reverse_proxy_plug.ex
index d759e6c..33fd2ec 100644
--- a/lib/reverse_proxy_plug.ex
+++ b/lib/reverse_proxy_plug.ex
@@ -64,7 +64,7 @@ defmodule ReverseProxyPlug do
   defp process_response(:buffer, conn, %{status_code: status, body: body, headers: headers}) do
     resp_headers =
       headers
-      |> remove_hop_by_hop_headers
+      |> normalize_headers
 
     conn
     |> Conn.prepend_resp_headers(resp_headers)
@@ -81,9 +81,8 @@ defmodule ReverseProxyPlug do
 
       %HTTPoison.AsyncHeaders{headers: headers} ->
         headers
-        |> Enum.map(fn {header, value} -> {header |> String.downcase(), value} end)
+        |> normalize_headers
         |> Enum.reject(fn {header, _} -> header == "content-length" end)
-        |> Enum.reject(fn {header, _} -> header == "transfer-encoding" end)
         |> Enum.concat([{"transfer-encoding", "chunked"}])
         |> Enum.reduce(conn, fn {header, value}, conn ->
           Conn.put_resp_header(conn, header, value)
@@ -131,14 +130,12 @@ defmodule ReverseProxyPlug do
   end
 
   defp prepare_request(conn, options) do
-    # With HTTP/2 `transfer-encoding` shouldn't be included.
-    conn =
-      conn
-      |> Conn.delete_req_header("transfer-encoding")
-
     method = conn.method |> String.downcase() |> String.to_atom()
     url = prepare_url(conn, options)
-    headers = conn.req_headers
+
+    headers =
+      conn.req_headers
+      |> normalize_headers
 
     headers =
       if options[:preserve_host_header],
@@ -167,6 +164,17 @@ defmodule ReverseProxyPlug do
     |> Keyword.put_new(:recv_timeout, :infinity)
   end
 
+  defp normalize_headers(headers) do
+    headers
+    |> downcase_headers
+    |> remove_hop_by_hop_headers
+  end
+
+  defp downcase_headers(headers) do
+    headers
+    |> Enum.map(fn {header, value} -> {String.downcase(header), value} end)
+  end
+
   defp remove_hop_by_hop_headers(headers) do
     hop_by_hop_headers = [
       "te",
@@ -180,7 +188,6 @@ defmodule ReverseProxyPlug do
     ]
 
     headers
-    |> Enum.map(fn {header, value} -> {String.downcase(header), value} end)
     |> Enum.reject(fn {header, _} -> Enum.member?(hop_by_hop_headers, header) end)
   end
 


Test patch:
diff --git a/test/reverse_proxy_plug_buffer_test.exs b/test/reverse_proxy_plug_buffer_test.exs
index b62f1f3..5e8d803 100644
--- a/test/reverse_proxy_plug_buffer_test.exs
+++ b/test/reverse_proxy_plug_buffer_test.exs
@@ -10,14 +10,43 @@ defmodule ReverseProxyBufferTest do
           client: ReverseProxyPlug.HTTPClientMock
         )
 
+  @hop_by_hop_headers [
+    {"connection", "keep-alive"},
+    {"keep-alive", "timeout=5, max=1000"},
+    {"upgrade", "h2c"},
+    {"transfer-encoding", "chunked"},
+    {"proxy-authenticate", "Basic"},
+    {"proxy-authorization", "Basic abcd"},
+    {"te", "compress"},
+    {"trailer", "Expires"}
+  ]
+
+  @end_to_end_headers [
+    {"content-length", "42"},
+    {"cache-control", "max-age=3600"}
+  ]
+
   defp get_buffer_responder(status \\ 200, headers \\ [], body \\ "Success") do
     fn _method, _url, _body, _headers, _options ->
       {:ok, %HTTPoison.Response{body: body, headers: headers, status_code: status}}
     end
   end
 
-  defp default_buffer_responder do
-    get_buffer_responder().(nil, nil, nil)
+  defp get_mock_request(expected_headers) do
+    fn _method, _url, _body, headers, _options ->
+      assert headers == expected_headers
+    end
+  end
+
+  test "removes hop-by-hop headers before forwarding request" do
+    ReverseProxyPlug.HTTPClientMock
+    |> expect(:request, get_mock_request(@end_to_end_headers))
+
+    conn(:get, "/")
+    |> Map.put(:req_headers, @hop_by_hop_headers ++ @end_to_end_headers)
+    |> ReverseProxyPlug.call(@opts)
+
+    ReverseProxyPlug.HTTPClientMock |> verify!
   end
 
   test "receives response" do
@@ -36,30 +65,17 @@ defmodule ReverseProxyBufferTest do
   end
 
   test "removes hop-by-hop headers from response" do
-    hop_by_hop_headers = [
-      {"connection", "keep-alive"},
-      {"keep-alive", "timeout=5, max=1000"},
-      {"upgrade", "h2c"},
-      {"transfer-encoding", "chunked"}
-    ]
-
-    other_headers = [
-      {"host", "example.com"},
-      {"content-length", "42"},
-      {"cache-control", "max-age=3600"}
-    ]
-
     ReverseProxyPlug.HTTPClientMock
-    |> expect(:request, get_buffer_responder(200, Enum.concat(hop_by_hop_headers, other_headers)))
+    |> expect(:request, get_buffer_responder(200, @hop_by_hop_headers ++ @end_to_end_headers))
 
     conn =
       conn(:get, "/")
       |> ReverseProxyPlug.call(@opts)
 
-    assert Enum.all?(hop_by_hop_headers, fn x -> x not in conn.resp_headers end),
+    assert Enum.all?(@hop_by_hop_headers, fn x -> x not in conn.resp_headers end),
            "deletes hop-by-hop headers"
 
-    assert Enum.all?(other_headers, fn x -> x in conn.resp_headers end),
+    assert Enum.all?(@end_to_end_headers, fn x -> x in conn.resp_headers end),
            "passes other headers through"
   end
 
diff --git a/test/reverse_proxy_plug_stream_test.exs b/test/reverse_proxy_plug_stream_test.exs
index b1e2740..960fc6f 100644
--- a/test/reverse_proxy_plug_stream_test.exs
+++ b/test/reverse_proxy_plug_stream_test.exs
@@ -4,6 +4,31 @@ defmodule ReverseProxyStreamTest do
 
   import Mox
 
+  @opts ReverseProxyPlug.init(
+          upstream: "example.com",
+          client: ReverseProxyPlug.HTTPClientMock
+        )
+
+  @hop_by_hop_headers [
+    {"connection", "keep-alive"},
+    {"keep-alive", "timeout=5, max=1000"},
+    {"upgrade", "h2c"},
+    {"proxy-authenticate", "Basic"},
+    {"proxy-authorization", "Basic abcd"},
+    {"te", "compress"},
+    {"trailer", "Expires"}
+  ]
+
+  @end_to_end_headers [
+    {"cache-control", "max-age=3600"},
+    {"cookie", "acookie"},
+    {"date", "Tue, 15 Nov 1994 08:12:31 GMT"}
+  ]
+
+  @host_header [
+    {"host", "example.com"}
+  ]
+
   defp get_stream_responder(status \\ 200, headers \\ [], body \\ "Success", no_chunks \\ 1) do
     fn _method, _url, _body, _headers, _options ->
       send(self(), %HTTPoison.AsyncStatus{code: status})
@@ -26,26 +51,59 @@ defmodule ReverseProxyStreamTest do
     get_stream_responder().(nil, nil, nil, nil, nil)
   end
 
+  defp get_mock_request(expected_headers) do
+    fn _method, _url, _body, headers, _options ->
+      assert headers == expected_headers
+    end
+  end
+
   test "receives response" do
     ReverseProxyPlug.HTTPClientMock
-    |> expect(:request, get_stream_responder(200, [{"host", "example.com"}], "Success", 2))
+    |> expect(:request, get_stream_responder(200, @host_header, "Success", 2))
 
     conn =
       conn(:get, "/")
-      |> ReverseProxyPlug.call(upstream: "example.com", client: ReverseProxyPlug.HTTPClientMock)
+      |> ReverseProxyPlug.call(@opts)
 
     assert conn.status == 200, "passes status through"
     assert {"host", "example.com"} in conn.resp_headers, "passes headers through"
     assert conn.resp_body == "Success", "passes body through"
   end
 
+  test "removes hop-by-hop headers before forwarding request" do
+    ReverseProxyPlug.HTTPClientMock
+    |> expect(:request, get_mock_request(@end_to_end_headers))
+
+    conn =
+      conn(:get, "/")
+      |> Map.put(:req_headers, @hop_by_hop_headers ++ @end_to_end_headers)
+      |> ReverseProxyPlug.call(@opts)
+
+    ReverseProxyPlug.HTTPClientMock |> verify!
+  end
+
+  test "removes hop-by-hop headers from response" do
+    ReverseProxyPlug.HTTPClientMock
+    |> expect(:request, get_stream_responder(200, @hop_by_hop_headers ++ @end_to_end_headers))
+
+    conn =
+      conn(:get, "/")
+      |> ReverseProxyPlug.call(@opts)
+
+    assert Enum.all?(@hop_by_hop_headers, fn x -> x not in conn.resp_headers end),
+           "deletes hop-by-hop headers"
+
+    assert Enum.all?(@end_to_end_headers, fn x -> x in conn.resp_headers end),
+           "passes other headers through"
+  end
+
   test "sets correct chunked transfer-encoding headers" do
     ReverseProxyPlug.HTTPClientMock
     |> expect(:request, get_stream_responder(200, [{"content-length", "7"}]))
 
     conn =
       conn(:get, "/")
-      |> ReverseProxyPlug.call(upstream: "example.com", client: ReverseProxyPlug.HTTPClientMock)
+      |> ReverseProxyPlug.call(@opts)
 
     assert {"transfer-encoding", "chunked"} in conn.resp_headers,
            "sets transfer-encoding header"