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

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 @@ defp prepare_image_resize_args(_), do: {:error, :missing_options}
 
   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",

lib/pleroma/web/media_proxy/media_proxy.ex

@@ -57,13 +57,11 @@ def url_proxiable?(url) 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
 

lib/pleroma/web/media_proxy/media_proxy_controller.ex

@@ -48,10 +48,12 @@ def preview(conn, %{"sig" => sig64, "url" => url64}) 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 @@ defp handle_preview(conn, url) 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 @@ defp handle_png_preview(%{params: params} = conn, url) 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 @@ defp handle_png_preview(%{params: params} = conn, url) 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 @@ defp handle_jpeg_preview(%{params: params} = conn, url) 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 @@ defp handle_video_preview(conn, url) 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
 

test/web/mastodon_api/views/account_view_test.exs

@@ -541,8 +541,9 @@ test "shows non-zero when historical unapproved requests are present" 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 @@ test "uses mediaproxy urls when it's enabled" 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())
+    with media_preview_enabled <- [false, true] do
+      Config.put([:media_preview_proxy, :enabled], media_preview_enabled)
 
-      {: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)
+      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())
 
-      _ ->
-        true
-    end)
-    |> assert()
+        {: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