From: Ivan Tashkinov <ivantashkinov@gmail.com>
Date: Sat, 5 Sep 2020 13:16:35 +0000 (+0300)
Subject: [#2497] Made media preview proxy fall back to media proxy instead of to source url... 
X-Git-Url: https://git.squeep.com/?a=commitdiff_plain;h=c3b02341bf4ab610e9425d6811dca057e9f811a4;p=akkoma

[#2497] Made media preview proxy fall back to media proxy instead of to source url. Adjusted tests. Refactoring.
---

diff --git a/lib/pleroma/helpers/media_helper.ex b/lib/pleroma/helpers/media_helper.ex
index cfb091f82..bb93d4915 100644
--- a/lib/pleroma/helpers/media_helper.ex
+++ b/lib/pleroma/helpers/media_helper.ex
@@ -7,12 +7,14 @@ defmodule Pleroma.Helpers.MediaHelper do
   Handles common media-related operations.
   """
 
+  alias Pleroma.HTTP
+
   @tmp_base "/tmp/pleroma-media_preview-pipe"
 
   def image_resize(url, options) do
     with executable when is_binary(executable) <- System.find_executable("convert"),
          {:ok, args} <- prepare_image_resize_args(options),
-         {:ok, env} <- Pleroma.HTTP.get(url, [], [adapter: [pool: :preview]]),
+         {:ok, env} <- HTTP.get(url, [], adapter: [pool: :preview]),
          {:ok, fifo_path} <- mkfifo() do
       args = List.flatten([fifo_path, args])
       run_fifo(fifo_path, env, executable, args)
@@ -60,7 +62,7 @@ defmodule Pleroma.Helpers.MediaHelper do
 
   def video_framegrab(url) do
     with executable when is_binary(executable) <- System.find_executable("ffmpeg"),
-         {:ok, env} <- Pleroma.HTTP.get(url, [], [adapter: [pool: :preview]]),
+         {:ok, env} <- HTTP.get(url, [], adapter: [pool: :preview]),
          {:ok, fifo_path} <- mkfifo(),
          args = [
            "-y",
diff --git a/lib/pleroma/web/media_proxy/media_proxy.ex b/lib/pleroma/web/media_proxy/media_proxy.ex
index 4cbe1cf89..80017cde1 100644
--- a/lib/pleroma/web/media_proxy/media_proxy.ex
+++ b/lib/pleroma/web/media_proxy/media_proxy.ex
@@ -57,13 +57,11 @@ defmodule Pleroma.Web.MediaProxy do
     end
   end
 
-  # Note: routing all URLs to preview handler (even local and whitelisted).
-  #   Preview handler will call url/1 on decoded URLs, and applicable ones will detour media proxy.
   def preview_url(url, preview_params \\ []) do
     if preview_enabled?() do
       encode_preview_url(url, preview_params)
     else
-      url
+      url(url)
     end
   end
 
diff --git a/lib/pleroma/web/media_proxy/media_proxy_controller.ex b/lib/pleroma/web/media_proxy/media_proxy_controller.ex
index 33daa1e05..469fbae59 100644
--- a/lib/pleroma/web/media_proxy/media_proxy_controller.ex
+++ b/lib/pleroma/web/media_proxy/media_proxy_controller.ex
@@ -48,10 +48,12 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
   end
 
   defp handle_preview(conn, url) do
+    media_proxy_url = MediaProxy.url(url)
+
     with {:ok, %{status: status} = head_response} when status in 200..299 <-
-           Pleroma.HTTP.request("head", MediaProxy.url(url), [], [], [adapter: [pool: :preview]]) do
+           Pleroma.HTTP.request("head", media_proxy_url, [], [], adapter: [pool: :preview]) do
       content_type = Tesla.get_header(head_response, "content-type")
-      handle_preview(content_type, conn, url)
+      handle_preview(content_type, conn, media_proxy_url)
     else
       {_, %{status: status}} ->
         send_resp(conn, :failed_dependency, "Can't fetch HTTP headers (HTTP #{status}).")
@@ -67,40 +69,38 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
   defp handle_preview(
          "image/" <> _ = _content_type,
          %{params: %{"output_format" => "jpeg"}} = conn,
-         url
+         media_proxy_url
        ) do
-    handle_jpeg_preview(conn, url)
+    handle_jpeg_preview(conn, media_proxy_url)
   end
 
-  defp handle_preview("image/gif" = _content_type, conn, url) do
-    mediaproxy_url = url |> MediaProxy.url()
-
-    redirect(conn, external: mediaproxy_url)
+  defp handle_preview("image/gif" = _content_type, conn, media_proxy_url) do
+    redirect(conn, external: media_proxy_url)
   end
 
-  defp handle_preview("image/png" <> _ = _content_type, conn, url) do
-    handle_png_preview(conn, url)
+  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, conn, url) do
-    handle_jpeg_preview(conn, url)
+  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, conn, url) do
-    handle_video_preview(conn, url)
+  defp handle_preview("video/" <> _ = _content_type, conn, media_proxy_url) do
+    handle_video_preview(conn, media_proxy_url)
   end
 
-  defp handle_preview(content_type, conn, _url) do
+  defp handle_preview(content_type, conn, _media_proxy_url) do
     send_resp(conn, :unprocessable_entity, "Unsupported content type: #{content_type}.")
   end
 
-  defp handle_png_preview(%{params: params} = conn, url) do
+  defp handle_png_preview(%{params: params} = conn, media_proxy_url) do
     quality = Config.get!([:media_preview_proxy, :image_quality])
 
     with {thumbnail_max_width, thumbnail_max_height} <- thumbnail_max_dimensions(params),
          {:ok, thumbnail_binary} <-
            MediaHelper.image_resize(
-             url,
+             media_proxy_url,
              %{
                max_width: thumbnail_max_width,
                max_height: thumbnail_max_height,
@@ -109,7 +109,7 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
              }
            ) do
       conn
-      |> put_preview_response_headers("image/png", "preview.png")
+      |> put_preview_response_headers(["image/png", "preview.png"])
       |> send_resp(200, thumbnail_binary)
     else
       _ ->
@@ -117,13 +117,13 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
     end
   end
 
-  defp handle_jpeg_preview(%{params: params} = conn, url) do
+  defp handle_jpeg_preview(%{params: params} = conn, media_proxy_url) do
     quality = Config.get!([:media_preview_proxy, :image_quality])
 
     with {thumbnail_max_width, thumbnail_max_height} <- thumbnail_max_dimensions(params),
          {:ok, thumbnail_binary} <-
            MediaHelper.image_resize(
-             url,
+             media_proxy_url,
              %{max_width: thumbnail_max_width, max_height: thumbnail_max_height, quality: quality}
            ) do
       conn
@@ -135,9 +135,9 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
     end
   end
 
-  defp handle_video_preview(conn, url) do
+  defp handle_video_preview(conn, media_proxy_url) do
     with {:ok, thumbnail_binary} <-
-           MediaHelper.video_framegrab(url) do
+           MediaHelper.video_framegrab(media_proxy_url) do
       conn
       |> put_preview_response_headers()
       |> send_resp(200, thumbnail_binary)
@@ -147,10 +147,14 @@ defmodule Pleroma.Web.MediaProxy.MediaProxyController do
     end
   end
 
-  defp put_preview_response_headers(conn, content_type \\ "image/jpeg", filename \\ "preview.jpg") do
+  defp put_preview_response_headers(
+         conn,
+         [content_type, filename] = _content_info \\ ["image/jpeg", "preview.jpg"]
+       ) do
     conn
     |> put_resp_header("content-type", content_type)
     |> put_resp_header("content-disposition", "inline; filename=\"#{filename}\"")
+    # TODO: enable caching
     |> put_resp_header("cache-control", "max-age=0, private, must-revalidate")
   end
 
diff --git a/test/web/mastodon_api/views/account_view_test.exs b/test/web/mastodon_api/views/account_view_test.exs
index 8f37efa3c..2f56d9c8f 100644
--- a/test/web/mastodon_api/views/account_view_test.exs
+++ b/test/web/mastodon_api/views/account_view_test.exs
@@ -541,8 +541,9 @@ defmodule Pleroma.Web.MastodonAPI.AccountViewTest do
     end
   end
 
-  test "uses mediaproxy urls when it's enabled" do
+  test "uses mediaproxy urls when it's enabled (regardless of media preview proxy state)" do
     clear_config([:media_proxy, :enabled], true)
+    clear_config([:media_preview_proxy, :enabled])
 
     user =
       insert(:user,
@@ -551,20 +552,24 @@ defmodule Pleroma.Web.MastodonAPI.AccountViewTest do
         emoji: %{"joker_smile" => "https://evil.website/society.png"}
       )
 
-    AccountView.render("show.json", %{user: user, skip_visibility_check: true})
-    |> Enum.all?(fn
-      {key, url} when key in [:avatar, :avatar_static, :header, :header_static] ->
-        String.starts_with?(url, Pleroma.Web.base_url())
-
-      {:emojis, emojis} ->
-        Enum.all?(emojis, fn %{url: url, static_url: static_url} ->
-          String.starts_with?(url, Pleroma.Web.base_url()) &&
-            String.starts_with?(static_url, Pleroma.Web.base_url())
-        end)
-
-      _ ->
-        true
-    end)
-    |> assert()
+    with media_preview_enabled <- [false, true] do
+      Config.put([:media_preview_proxy, :enabled], media_preview_enabled)
+
+      AccountView.render("show.json", %{user: user, skip_visibility_check: true})
+      |> Enum.all?(fn
+        {key, url} when key in [:avatar, :avatar_static, :header, :header_static] ->
+          String.starts_with?(url, Pleroma.Web.base_url())
+
+        {:emojis, emojis} ->
+          Enum.all?(emojis, fn %{url: url, static_url: static_url} ->
+            String.starts_with?(url, Pleroma.Web.base_url()) &&
+              String.starts_with?(static_url, Pleroma.Web.base_url())
+          end)
+
+        _ ->
+          true
+      end)
+      |> assert()
+    end
   end
 end