From 1dc5794e2996d09dee22f0156c4a442c8338aa8d Mon Sep 17 00:00:00 2001
From: Alex Gleason <alex@alexgleason.me>
Date: Mon, 22 Feb 2021 14:46:59 -0600
Subject: [PATCH] Never forward the client's user-agent through the media proxy

---
 lib/pleroma/reverse_proxy.ex        | 26 +++++++-------------
 test/pleroma/reverse_proxy_test.exs | 38 +++++++++++++----------------
 2 files changed, 26 insertions(+), 38 deletions(-)

diff --git a/lib/pleroma/reverse_proxy.ex b/lib/pleroma/reverse_proxy.ex
index 466906f03..406f7e2b8 100644
--- a/lib/pleroma/reverse_proxy.ex
+++ b/lib/pleroma/reverse_proxy.ex
@@ -4,7 +4,7 @@
 
 defmodule Pleroma.ReverseProxy do
   @range_headers ~w(range if-range)
-  @keep_req_headers ~w(accept user-agent accept-encoding cache-control if-modified-since) ++
+  @keep_req_headers ~w(accept accept-encoding cache-control if-modified-since) ++
                       ~w(if-unmodified-since if-none-match) ++ @range_headers
   @resp_cache_headers ~w(etag date last-modified)
   @keep_resp_headers @resp_cache_headers ++
@@ -57,9 +57,6 @@ def default_cache_control_header, do: @default_cache_control_header
     * `false` will add `content-disposition: attachment` to any request,
     * a list of whitelisted content types
 
-    * `keep_user_agent` will forward the client's user-agent to the upstream. This may be useful if the upstream is
-    doing content transformation (encoding, …) depending on the request.
-
   * `req_headers`, `resp_headers` additional headers.
 
   * `http`: options for [hackney](https://github.com/benoitc/hackney) or [gun](https://github.com/ninenines/gun).
@@ -84,8 +81,7 @@ def default_cache_control_header, do: @default_cache_control_header
   import Plug.Conn
 
   @type option() ::
-          {:keep_user_agent, boolean}
-          | {:max_read_duration, :timer.time() | :infinity}
+          {:max_read_duration, :timer.time() | :infinity}
           | {:max_body_length, non_neg_integer() | :infinity}
           | {:failed_request_ttl, :timer.time() | :infinity}
           | {:http, []}
@@ -291,17 +287,13 @@ defp build_req_range_or_encoding_header(headers, _opts) do
     end
   end
 
-  defp build_req_user_agent_header(headers, opts) do
-    if Keyword.get(opts, :keep_user_agent, false) do
-      List.keystore(
-        headers,
-        "user-agent",
-        0,
-        {"user-agent", Pleroma.Application.user_agent()}
-      )
-    else
-      headers
-    end
+  defp build_req_user_agent_header(headers, _opts) do
+    List.keystore(
+      headers,
+      "user-agent",
+      0,
+      {"user-agent", Pleroma.Application.user_agent()}
+    )
   end
 
   defp build_resp_headers(headers, opts) do
diff --git a/test/pleroma/reverse_proxy_test.exs b/test/pleroma/reverse_proxy_test.exs
index 499d29c06..863e0c50d 100644
--- a/test/pleroma/reverse_proxy_test.exs
+++ b/test/pleroma/reverse_proxy_test.exs
@@ -18,24 +18,23 @@ defmodule Pleroma.ReverseProxyTest do
 
   setup :verify_on_exit!
 
-  defp user_agent_mock(user_agent, invokes) do
-    json = Jason.encode!(%{"user-agent": user_agent})
-
+  defp user_agent_mock(invokes) do
     ClientMock
-    |> expect(:request, fn :get, url, _, _, _ ->
+    |> expect(:request, fn :get, url, headers, _body, _opts ->
       Registry.register(ClientMock, url, 0)
+      body = headers |> Enum.into(%{}) |> Jason.encode!()
 
       {:ok, 200,
        [
          {"content-type", "application/json"},
-         {"content-length", byte_size(json) |> to_string()}
-       ], %{url: url}}
+         {"content-length", byte_size(body) |> to_string()}
+       ], %{url: url, body: body}}
     end)
-    |> expect(:stream_body, invokes, fn %{url: url} = client ->
+    |> expect(:stream_body, invokes, fn %{url: url, body: body} = client ->
       case Registry.lookup(ClientMock, url) do
         [{_, 0}] ->
           Registry.update_value(ClientMock, url, &(&1 + 1))
-          {:ok, json, client}
+          {:ok, body, client}
 
         [{_, 1}] ->
           Registry.unregister(ClientMock, url)
@@ -46,7 +45,7 @@ defp user_agent_mock(user_agent, invokes) do
 
   describe "reverse proxy" do
     test "do not track successful request", %{conn: conn} do
-      user_agent_mock("hackney/1.15.1", 2)
+      user_agent_mock(2)
       url = "/success"
 
       conn = ReverseProxy.call(conn, url)
@@ -56,18 +55,15 @@ test "do not track successful request", %{conn: conn} do
     end
   end
 
-  describe "user-agent" do
-    test "don't keep", %{conn: conn} do
-      user_agent_mock("hackney/1.15.1", 2)
-      conn = ReverseProxy.call(conn, "/user-agent")
-      assert json_response(conn, 200) == %{"user-agent" => "hackney/1.15.1"}
-    end
+  test "use Pleroma's user agent in the request; don't pass the client's", %{conn: conn} do
+    user_agent_mock(2)
 
-    test "keep", %{conn: conn} do
-      user_agent_mock(Pleroma.Application.user_agent(), 2)
-      conn = ReverseProxy.call(conn, "/user-agent-keep", keep_user_agent: true)
-      assert json_response(conn, 200) == %{"user-agent" => Pleroma.Application.user_agent()}
-    end
+    conn =
+      conn
+      |> Plug.Conn.put_req_header("user-agent", "fake/1.0")
+      |> ReverseProxy.call("/user-agent")
+
+    assert json_response(conn, 200) == %{"user-agent" => Pleroma.Application.user_agent()}
   end
 
   test "closed connection", %{conn: conn} do
@@ -114,7 +110,7 @@ defp stream_mock(invokes, with_close? \\ false) do
 
   describe "max_body" do
     test "length returns error if content-length more than option", %{conn: conn} do
-      user_agent_mock("hackney/1.15.1", 0)
+      user_agent_mock(0)
 
       assert capture_log(fn ->
                ReverseProxy.call(conn, "/huge-file", max_body_length: 4)