From c3b02341bf4ab610e9425d6811dca057e9f811a4 Mon Sep 17 00:00:00 2001 From: Ivan Tashkinov Date: Sat, 5 Sep 2020 16:16:35 +0300 Subject: [PATCH] [#2497] Made media preview proxy fall back to media proxy instead of to source url. Adjusted tests. Refactoring. --- lib/pleroma/helpers/media_helper.ex | 6 ++- lib/pleroma/web/media_proxy/media_proxy.ex | 4 +- .../web/media_proxy/media_proxy_controller.ex | 50 ++++++++++--------- .../mastodon_api/views/account_view_test.exs | 33 ++++++------ 4 files changed, 51 insertions(+), 42 deletions(-) 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 @@ 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", 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 @@ 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 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 @@ 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 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 @@ 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