From: Ivan Tashkinov <ivantashkinov@gmail.com>
Date: Thu, 17 Sep 2020 14:13:40 +0000 (+0300)
Subject: [#2497] Configurability of :min_content_length (preview proxy). Refactoring, document... 
X-Git-Url: https://git.squeep.com/?a=commitdiff_plain;h=7cdbd91d83c02a79c22783ca489ef82e82b31a51;p=akkoma

[#2497] Configurability of :min_content_length (preview proxy). Refactoring, documentation, tests.
---

diff --git a/config/config.exs b/config/config.exs
index 2ca2236a9..98c31ef86 100644
--- a/config/config.exs
+++ b/config/config.exs
@@ -444,7 +444,8 @@ config :pleroma, :media_preview_proxy,
   enabled: false,
   thumbnail_max_width: 600,
   thumbnail_max_height: 600,
-  image_quality: 85
+  image_quality: 85,
+  min_content_length: 100 * 1024
 
 config :pleroma, :chat, enabled: true
 
diff --git a/config/description.exs b/config/description.exs
index 79e3cc259..4a5d5f2ea 100644
--- a/config/description.exs
+++ b/config/description.exs
@@ -1961,17 +1961,25 @@ config :pleroma, :config_description, [
       %{
         key: :thumbnail_max_width,
         type: :integer,
-        description: "Max width of preview thumbnail."
+        description:
+          "Max width of preview thumbnail for images (video preview always has original dimensions)."
       },
       %{
         key: :thumbnail_max_height,
         type: :integer,
-        description: "Max height of preview thumbnail."
+        description:
+          "Max height of preview thumbnail for images (video preview always has original dimensions)."
       },
       %{
         key: :image_quality,
         type: :integer,
         description: "Quality of the output. Ranges from 0 (min quality) to 100 (max quality)."
+      },
+      %{
+        key: :min_content_length,
+        type: :integer,
+        description:
+          "Min content length to perform preview, in bytes. If greater than 0, media smaller in size will be served as is, without thumbnailing."
       }
     ]
   },
diff --git a/docs/configuration/cheatsheet.md b/docs/configuration/cheatsheet.md
index 054b8fe43..d7c342383 100644
--- a/docs/configuration/cheatsheet.md
+++ b/docs/configuration/cheatsheet.md
@@ -314,6 +314,14 @@ This section describe PWA manifest instance-specific values. Currently this opti
   * `enabled`: Enables purge cache
   * `provider`: Which one of  the [purge cache strategy](#purge-cache-strategy) to use.
 
+## :media_preview_proxy
+
+* `enabled`: Enables proxying of remote media preview to the instance’s proxy. Requires enabled media proxy (`media_proxy/enabled`).
+* `thumbnail_max_width`: Max width of preview thumbnail for images (video preview always has original dimensions).
+* `thumbnail_max_height`: Max height of preview thumbnail for images (video preview always has original dimensions).
+* `image_quality`: Quality of the output. Ranges from 0 (min quality) to 100 (max quality).
+* `min_content_length`: Min content length to perform preview, in bytes. If greater than 0, media smaller in size will be served as is, without thumbnailing.
+
 ### Purge cache strategy
 
 #### Pleroma.Web.MediaProxy.Invalidation.Script
diff --git a/lib/pleroma/helpers/media_helper.ex b/lib/pleroma/helpers/media_helper.ex
index 9b7348ee2..b6f35a24b 100644
--- a/lib/pleroma/helpers/media_helper.ex
+++ b/lib/pleroma/helpers/media_helper.ex
@@ -58,6 +58,7 @@ defmodule Pleroma.Helpers.MediaHelper do
 
   defp prepare_image_resize_args(_), do: {:error, :missing_options}
 
+  # Note: video thumbnail is intentionally not resized (always has original dimensions)
   def video_framegrab(url) do
     with executable when is_binary(executable) <- System.find_executable("ffmpeg"),
          {:ok, env} <- HTTP.get(url, [], pool: :media),
diff --git a/lib/pleroma/web/media_proxy/media_proxy_controller.ex b/lib/pleroma/web/media_proxy/media_proxy_controller.ex
index fe279e964..90651ed9b 100644
--- a/lib/pleroma/web/media_proxy/media_proxy_controller.ex
+++ b/lib/pleroma/web/media_proxy/media_proxy_controller.ex
@@ -12,8 +12,6 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
   alias Pleroma.Web.MediaProxy
   alias Plug.Conn
 
-  @min_content_length_for_preview 100 * 1024
-
   def remote(conn, %{"sig" => sig64, "url" => url64}) do
     with {_, true} <- {:enabled, MediaProxy.enabled?()},
          {:ok, url} <- MediaProxy.decode_url(sig64, url64),
@@ -37,7 +35,8 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
 
   def preview(%Conn{} = conn, %{"sig" => sig64, "url" => url64}) do
     with {_, true} <- {:enabled, MediaProxy.preview_enabled?()},
-         {:ok, url} <- MediaProxy.decode_url(sig64, url64) do
+         {:ok, url} <- MediaProxy.decode_url(sig64, url64),
+         :ok <- MediaProxy.verify_request_path_and_url(conn, url) do
       handle_preview(conn, url)
     else
       {:enabled, false} ->
@@ -59,8 +58,25 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
       content_type = Tesla.get_header(head_response, "content-type")
       content_length = Tesla.get_header(head_response, "content-length")
       content_length = content_length && String.to_integer(content_length)
+      static = conn.params["static"] in ["true", true]
+
+      cond do
+        static and content_type == "image/gif" ->
+          handle_jpeg_preview(conn, media_proxy_url)
 
-      handle_preview(content_type, content_length, conn, media_proxy_url)
+        static ->
+          drop_static_param_and_redirect(conn)
+
+        content_type == "image/gif" ->
+          redirect(conn, external: media_proxy_url)
+
+        min_content_length_for_preview() > 0 and content_length > 0 and
+            content_length < min_content_length_for_preview() ->
+          redirect(conn, external: media_proxy_url)
+
+        true ->
+          handle_preview(content_type, conn, media_proxy_url)
+      end
     else
       # If HEAD failed, redirecting to media proxy URI doesn't make much sense; returning an error
       {_, %{status: status}} ->
@@ -74,58 +90,27 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
     end
   end
 
-  defp handle_preview(
-         "image/gif" = _content_type,
-         _content_length,
-         %{params: %{"static" => static}} = conn,
-         media_proxy_url
-       )
-       when static in ["true", true] do
-    handle_jpeg_preview(conn, media_proxy_url)
-  end
-
-  defp handle_preview(
-         _content_type,
-         _content_length,
-         %{params: %{"static" => static}} = conn,
-         _media_proxy_url
-       )
-       when static in ["true", true] do
-    uri_without_static_param = UriHelper.modify_uri_params(current_url(conn), %{}, ["static"])
-    redirect(conn, external: uri_without_static_param)
-  end
-
-  defp handle_preview("image/gif" = _content_type, _content_length, conn, media_proxy_url) do
-    redirect(conn, external: media_proxy_url)
-  end
-
-  defp handle_preview("image/" <> _ = _content_type, content_length, conn, media_proxy_url)
-       when is_integer(content_length) and content_length > 0 and
-              content_length < @min_content_length_for_preview do
-    redirect(conn, external: media_proxy_url)
-  end
-
-  defp handle_preview("image/png" <> _ = _content_type, _content_length, conn, media_proxy_url) do
+  defp handle_preview("image/png" <> _ = _content_type, conn, media_proxy_url) do
     handle_png_preview(conn, media_proxy_url)
   end
 
-  defp handle_preview("image/" <> _ = _content_type, _content_length, conn, media_proxy_url) do
+  defp handle_preview("image/" <> _ = _content_type, conn, media_proxy_url) do
     handle_jpeg_preview(conn, media_proxy_url)
   end
 
-  defp handle_preview("video/" <> _ = _content_type, _content_length, conn, media_proxy_url) do
+  defp handle_preview("video/" <> _ = _content_type, conn, media_proxy_url) do
     handle_video_preview(conn, media_proxy_url)
   end
 
-  defp handle_preview(_unsupported_content_type, _content_length, conn, media_proxy_url) do
+  defp handle_preview(_unsupported_content_type, conn, media_proxy_url) do
     fallback_on_preview_error(conn, media_proxy_url)
   end
 
   defp handle_png_preview(conn, media_proxy_url) do
     quality = Config.get!([:media_preview_proxy, :image_quality])
+    {thumbnail_max_width, thumbnail_max_height} = thumbnail_max_dimensions()
 
-    with {thumbnail_max_width, thumbnail_max_height} <- thumbnail_max_dimensions(),
-         {:ok, thumbnail_binary} <-
+    with {:ok, thumbnail_binary} <-
            MediaHelper.image_resize(
              media_proxy_url,
              %{
@@ -146,9 +131,9 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
 
   defp handle_jpeg_preview(conn, media_proxy_url) do
     quality = Config.get!([:media_preview_proxy, :image_quality])
+    {thumbnail_max_width, thumbnail_max_height} = thumbnail_max_dimensions()
 
-    with {thumbnail_max_width, thumbnail_max_height} <- thumbnail_max_dimensions(),
-         {:ok, thumbnail_binary} <-
+    with {:ok, thumbnail_binary} <-
            MediaHelper.image_resize(
              media_proxy_url,
              %{max_width: thumbnail_max_width, max_height: thumbnail_max_height, quality: quality}
@@ -174,6 +159,15 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
     end
   end
 
+  defp drop_static_param_and_redirect(conn) do
+    uri_without_static_param =
+      conn
+      |> current_url()
+      |> UriHelper.modify_uri_params(%{}, ["static"])
+
+    redirect(conn, external: uri_without_static_param)
+  end
+
   defp fallback_on_preview_error(conn, media_proxy_url) do
     redirect(conn, external: media_proxy_url)
   end
@@ -189,7 +183,7 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
   end
 
   defp thumbnail_max_dimensions do
-    config = Config.get([:media_preview_proxy], [])
+    config = media_preview_proxy_config()
 
     thumbnail_max_width = Keyword.fetch!(config, :thumbnail_max_width)
     thumbnail_max_height = Keyword.fetch!(config, :thumbnail_max_height)
@@ -197,6 +191,14 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
     {thumbnail_max_width, thumbnail_max_height}
   end
 
+  defp min_content_length_for_preview do
+    Keyword.get(media_preview_proxy_config(), :min_content_length, 0)
+  end
+
+  defp media_preview_proxy_config do
+    Config.get!([:media_preview_proxy])
+  end
+
   defp media_proxy_opts do
     Config.get([:media_proxy, :proxy_opts], [])
   end
diff --git a/test/fixtures/image.gif b/test/fixtures/image.gif
new file mode 100755
index 000000000..9df64778b
Binary files /dev/null and b/test/fixtures/image.gif differ
diff --git a/test/fixtures/image.png b/test/fixtures/image.png
new file mode 100755
index 000000000..e999e8800
Binary files /dev/null and b/test/fixtures/image.png differ
diff --git a/test/web/media_proxy/media_proxy_controller_test.exs b/test/web/media_proxy/media_proxy_controller_test.exs
index 0dd2fd10c..33e6873f7 100644
--- a/test/web/media_proxy/media_proxy_controller_test.exs
+++ b/test/web/media_proxy/media_proxy_controller_test.exs
@@ -14,27 +14,28 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyControllerTest do
     on_exit(fn -> Cachex.clear(:banned_urls_cache) end)
   end
 
-  test "it returns 404 when MediaProxy disabled", %{conn: conn} do
-    clear_config([:media_proxy, :enabled], false)
-
-    assert %Conn{
-             status: 404,
-             resp_body: "Not Found"
-           } = get(conn, "/proxy/hhgfh/eeeee")
-
-    assert %Conn{
-             status: 404,
-             resp_body: "Not Found"
-           } = get(conn, "/proxy/hhgfh/eeee/fff")
-  end
-
-  describe "" do
+  describe "Media Proxy" do
     setup do
       clear_config([:media_proxy, :enabled], true)
       clear_config([Pleroma.Web.Endpoint, :secret_key_base], "00000000000")
+
       [url: MediaProxy.encode_url("https://google.fn/test.png")]
     end
 
+    test "it returns 404 when disabled", %{conn: conn} do
+      clear_config([:media_proxy, :enabled], false)
+
+      assert %Conn{
+               status: 404,
+               resp_body: "Not Found"
+             } = get(conn, "/proxy/hhgfh/eeeee")
+
+      assert %Conn{
+               status: 404,
+               resp_body: "Not Found"
+             } = get(conn, "/proxy/hhgfh/eeee/fff")
+    end
+
     test "it returns 403 for invalid signature", %{conn: conn, url: url} do
       Pleroma.Config.put([Pleroma.Web.Endpoint, :secret_key_base], "000")
       %{path: path} = URI.parse(url)
@@ -55,7 +56,7 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyControllerTest do
              } = get(conn, "/proxy/hhgfh/eeee/fff")
     end
 
-    test "redirects on valid url when filename is invalidated", %{conn: conn, url: url} do
+    test "redirects to valid url when filename is invalidated", %{conn: conn, url: url} do
       invalid_url = String.replace(url, "test.png", "test-file.png")
       response = get(conn, invalid_url)
       assert response.status == 302
@@ -78,4 +79,249 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyControllerTest do
       end
     end
   end
+
+  describe "Media Preview Proxy" do
+    setup do
+      clear_config([:media_proxy, :enabled], true)
+      clear_config([:media_preview_proxy, :enabled], true)
+      clear_config([Pleroma.Web.Endpoint, :secret_key_base], "00000000000")
+
+      original_url = "https://google.fn/test.png"
+
+      [
+        url: MediaProxy.encode_preview_url(original_url),
+        media_proxy_url: MediaProxy.encode_url(original_url)
+      ]
+    end
+
+    test "returns 404 when media proxy is disabled", %{conn: conn} do
+      clear_config([:media_proxy, :enabled], false)
+
+      assert %Conn{
+               status: 404,
+               resp_body: "Not Found"
+             } = get(conn, "/proxy/preview/hhgfh/eeeee")
+
+      assert %Conn{
+               status: 404,
+               resp_body: "Not Found"
+             } = get(conn, "/proxy/preview/hhgfh/fff")
+    end
+
+    test "returns 404 when disabled", %{conn: conn} do
+      clear_config([:media_preview_proxy, :enabled], false)
+
+      assert %Conn{
+               status: 404,
+               resp_body: "Not Found"
+             } = get(conn, "/proxy/preview/hhgfh/eeeee")
+
+      assert %Conn{
+               status: 404,
+               resp_body: "Not Found"
+             } = get(conn, "/proxy/preview/hhgfh/fff")
+    end
+
+    test "it returns 403 for invalid signature", %{conn: conn, url: url} do
+      Pleroma.Config.put([Pleroma.Web.Endpoint, :secret_key_base], "000")
+      %{path: path} = URI.parse(url)
+
+      assert %Conn{
+               status: 403,
+               resp_body: "Forbidden"
+             } = get(conn, path)
+
+      assert %Conn{
+               status: 403,
+               resp_body: "Forbidden"
+             } = get(conn, "/proxy/preview/hhgfh/eeee")
+
+      assert %Conn{
+               status: 403,
+               resp_body: "Forbidden"
+             } = get(conn, "/proxy/preview/hhgfh/eeee/fff")
+    end
+
+    test "redirects to valid url when filename is invalidated", %{conn: conn, url: url} do
+      invalid_url = String.replace(url, "test.png", "test-file.png")
+      response = get(conn, invalid_url)
+      assert response.status == 302
+      assert redirected_to(response) == url
+    end
+
+    test "responds with 424 Failed Dependency if HEAD request to media proxy fails", %{
+      conn: conn,
+      url: url,
+      media_proxy_url: media_proxy_url
+    } do
+      Tesla.Mock.mock(fn
+        %{method: "head", url: ^media_proxy_url} ->
+          %Tesla.Env{status: 500, body: ""}
+      end)
+
+      response = get(conn, url)
+      assert response.status == 424
+      assert response.resp_body == "Can't fetch HTTP headers (HTTP 500)."
+    end
+
+    test "redirects to media proxy URI on unsupported content type", %{
+      conn: conn,
+      url: url,
+      media_proxy_url: media_proxy_url
+    } do
+      Tesla.Mock.mock(fn
+        %{method: "head", url: ^media_proxy_url} ->
+          %Tesla.Env{status: 200, body: "", headers: [{"content-type", "application/pdf"}]}
+      end)
+
+      response = get(conn, url)
+      assert response.status == 302
+      assert redirected_to(response) == media_proxy_url
+    end
+
+    test "with `static=true` and GIF image preview requested, responds with JPEG image", %{
+      conn: conn,
+      url: url,
+      media_proxy_url: media_proxy_url
+    } do
+      # Setting a high :min_content_length to ensure this scenario is not affected by its logic
+      clear_config([:media_preview_proxy, :min_content_length], 1_000_000_000)
+
+      Tesla.Mock.mock(fn
+        %{method: "head", url: ^media_proxy_url} ->
+          %Tesla.Env{
+            status: 200,
+            body: "",
+            headers: [{"content-type", "image/gif"}, {"content-length", "1001718"}]
+          }
+
+        %{method: :get, url: ^media_proxy_url} ->
+          %Tesla.Env{status: 200, body: File.read!("test/fixtures/image.gif")}
+      end)
+
+      response = get(conn, url <> "?static=true")
+
+      assert response.status == 200
+      assert Conn.get_resp_header(response, "content-type") == ["image/jpeg"]
+      assert response.resp_body != ""
+    end
+
+    test "with GIF image preview requested and no `static` param, redirects to media proxy URI",
+         %{
+           conn: conn,
+           url: url,
+           media_proxy_url: media_proxy_url
+         } do
+      Tesla.Mock.mock(fn
+        %{method: "head", url: ^media_proxy_url} ->
+          %Tesla.Env{status: 200, body: "", headers: [{"content-type", "image/gif"}]}
+      end)
+
+      response = get(conn, url)
+
+      assert response.status == 302
+      assert redirected_to(response) == media_proxy_url
+    end
+
+    test "with `static` param and non-GIF image preview requested, " <>
+           "redirects to media preview proxy URI without `static` param",
+         %{
+           conn: conn,
+           url: url,
+           media_proxy_url: media_proxy_url
+         } do
+      Tesla.Mock.mock(fn
+        %{method: "head", url: ^media_proxy_url} ->
+          %Tesla.Env{status: 200, body: "", headers: [{"content-type", "image/jpeg"}]}
+      end)
+
+      response = get(conn, url <> "?static=true")
+
+      assert response.status == 302
+      assert redirected_to(response) == url
+    end
+
+    test "with :min_content_length setting not matched by Content-Length header, " <>
+           "redirects to media proxy URI",
+         %{
+           conn: conn,
+           url: url,
+           media_proxy_url: media_proxy_url
+         } do
+      clear_config([:media_preview_proxy, :min_content_length], 100_000)
+
+      Tesla.Mock.mock(fn
+        %{method: "head", url: ^media_proxy_url} ->
+          %Tesla.Env{
+            status: 200,
+            body: "",
+            headers: [{"content-type", "image/gif"}, {"content-length", "5000"}]
+          }
+      end)
+
+      response = get(conn, url)
+
+      assert response.status == 302
+      assert redirected_to(response) == media_proxy_url
+    end
+
+    test "thumbnails PNG images into PNG", %{
+      conn: conn,
+      url: url,
+      media_proxy_url: media_proxy_url
+    } do
+      Tesla.Mock.mock(fn
+        %{method: "head", url: ^media_proxy_url} ->
+          %Tesla.Env{status: 200, body: "", headers: [{"content-type", "image/png"}]}
+
+        %{method: :get, url: ^media_proxy_url} ->
+          %Tesla.Env{status: 200, body: File.read!("test/fixtures/image.png")}
+      end)
+
+      response = get(conn, url)
+
+      assert response.status == 200
+      assert Conn.get_resp_header(response, "content-type") == ["image/png"]
+      assert response.resp_body != ""
+    end
+
+    test "thumbnails JPEG images into JPEG", %{
+      conn: conn,
+      url: url,
+      media_proxy_url: media_proxy_url
+    } do
+      Tesla.Mock.mock(fn
+        %{method: "head", url: ^media_proxy_url} ->
+          %Tesla.Env{status: 200, body: "", headers: [{"content-type", "image/jpeg"}]}
+
+        %{method: :get, url: ^media_proxy_url} ->
+          %Tesla.Env{status: 200, body: File.read!("test/fixtures/image.jpg")}
+      end)
+
+      response = get(conn, url)
+
+      assert response.status == 200
+      assert Conn.get_resp_header(response, "content-type") == ["image/jpeg"]
+      assert response.resp_body != ""
+    end
+
+    test "redirects to media proxy URI in case of thumbnailing error", %{
+      conn: conn,
+      url: url,
+      media_proxy_url: media_proxy_url
+    } do
+      Tesla.Mock.mock(fn
+        %{method: "head", url: ^media_proxy_url} ->
+          %Tesla.Env{status: 200, body: "", headers: [{"content-type", "image/jpeg"}]}
+
+        %{method: :get, url: ^media_proxy_url} ->
+          %Tesla.Env{status: 200, body: "<html><body>error</body></html>"}
+      end)
+
+      response = get(conn, url)
+
+      assert response.status == 302
+      assert redirected_to(response) == media_proxy_url
+    end
+  end
 end