From 850b4e2a735e335c4737caa8b60e190613e778ef Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 26 May 2023 15:55:17 +0200 Subject: [PATCH] fix(rich media): fix error handling when resource preview URL leads to empty parsed data Closes #1279 Signed-off-by: Thomas Citharel --- lib/graphql/resolvers/resource.ex | 9 ++-- lib/service/rich_media/parser.ex | 42 ++++++++++--------- .../rich_media/parsers/oembed_parser.ex | 4 +- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/lib/graphql/resolvers/resource.ex b/lib/graphql/resolvers/resource.ex index 5df01944..d86fff8c 100644 --- a/lib/graphql/resolvers/resource.ex +++ b/lib/graphql/resolvers/resource.ex @@ -80,6 +80,8 @@ defmodule Mobilizon.GraphQL.Resolvers.Resource do } } = _resolution ) do + Logger.debug("Getting resource for group with username #{username}") + with {:group, %Actor{id: group_id}} <- {:group, Actors.get_actor_by_name(username, :Group)}, {:member, true} <- {:member, Actors.is_member?(actor_id, group_id)}, {:resource, %Resource{} = resource} <- @@ -222,13 +224,8 @@ defmodule Mobilizon.GraphQL.Resolvers.Resource do {:ok, data} when is_map(data) -> {:ok, struct(Metadata, data)} - {:error, :invalid_parsed_data} -> + {:error, _error_type, _} -> {:error, dgettext("errors", "Unable to fetch resource details from this URL.")} - - {:error, err} -> - Logger.warn("Error while fetching preview from #{inspect(resource_url)}") - Logger.debug(inspect(err)) - {:error, :unknown_resource} end end diff --git a/lib/service/rich_media/parser.ex b/lib/service/rich_media/parser.ex index 1873ca83..2821f823 100644 --- a/lib/service/rich_media/parser.ex +++ b/lib/service/rich_media/parser.ex @@ -29,19 +29,19 @@ defmodule Mobilizon.Service.RichMedia.Parser do def parse(nil), do: {:error, "No URL provided"} - @spec parse(String.t()) :: {:ok, map()} | {:error, any()} + @spec parse(String.t()) :: {:ok, map()} | {:error, :http | :parsing | :unknown, any()} def parse(url) do case Cachex.fetch(:rich_media_cache, url, fn _ -> case parse_url(url) do {:ok, data} -> {:commit, data} - {:error, err} -> {:ignore, err} + {:error, error_type, error} -> {:ignore, {error_type, error}} end end) do {status, value} when status in [:ok, :commit] -> {:ok, value} - {_, err} -> - {:error, err} + {_, {error_type, err}} -> + {:error, error_type, err} end rescue e -> @@ -56,7 +56,8 @@ defmodule Mobilizon.Service.RichMedia.Parser do get_filename_from_headers(response_headers) || get_filename_from_url(url) end - @spec parse_url(String.t(), Enum.t()) :: {:ok, map()} | {:error, any()} + @spec parse_url(String.t(), Enum.t()) :: + {:ok, map()} | {:error, :http | :parsing | :unknown, any()} defp parse_url(url, options \\ []) do user_agent = Keyword.get(options, :user_agent, default_user_agent(url)) headers = [{"User-Agent", user_agent}] @@ -64,13 +65,14 @@ defmodule Mobilizon.Service.RichMedia.Parser do try do with {:ok, _} <- prevent_local_address(url), - {:ok, %{body: body, status: code, headers: response_headers}} + {:fetch, {:ok, %{body: body, status: code, headers: response_headers}}} when code in 200..299 <- - RichMediaPreviewClient.get( - url, - headers: headers, - opts: @options - ), + {:fetch, + RichMediaPreviewClient.get( + url, + headers: headers, + opts: @options + )}, {:is_html, _response_headers, true} <- {:is_html, response_headers, is_html(response_headers)} do body @@ -87,17 +89,17 @@ defmodule Mobilizon.Service.RichMedia.Parser do {:ok, data} - {:ok, err} -> + {:fetch, {_, err}} -> Logger.debug("HTTP error: #{inspect(err)}") - {:error, "HTTP error: #{inspect(err)}"} + {:error, :http, err} {:error, err} -> - Logger.debug("HTTP error: #{inspect(err)}") - {:error, "HTTP error: #{inspect(err)}"} + Logger.debug("Parsing error: #{inspect(err)}") + {:error, :parsing, err} end rescue e -> - {:error, "Parsing error: #{inspect(e)} #{inspect(__STACKTRACE__)}"} + {:error, :unknown, "Parsing error: #{inspect(e)} #{inspect(__STACKTRACE__)}"} end end @@ -228,7 +230,7 @@ defmodule Mobilizon.Service.RichMedia.Parser do check_parsed_data(data, html, false) else Logger.debug("Found metadata was invalid or incomplete: #{inspect(data)}") - {:error, :invalid_parsed_data} + {:error, :parsing, :invalid_parsed_data} end end @@ -252,11 +254,11 @@ defmodule Mobilizon.Service.RichMedia.Parser do validate_ip(host) do {:ok, url} else - {:error, "Host violates local access rules"} + {:error, :local_address, "Host violates local access rules"} end _ -> - {:error, "Could not detect any host"} + {:error, :no_host, "Could not detect any host"} end end @@ -302,6 +304,8 @@ defmodule Mobilizon.Service.RichMedia.Parser do {:ok, data} end + defp check_remote_picture_path({:error, _, _} = err), do: err + defp check_remote_picture_path(data), do: {:ok, data} @spec format_url(String.t(), String.t()) :: String.t() diff --git a/lib/service/rich_media/parsers/oembed_parser.ex b/lib/service/rich_media/parsers/oembed_parser.ex index bcddd220..b76c3956 100644 --- a/lib/service/rich_media/parsers/oembed_parser.ex +++ b/lib/service/rich_media/parsers/oembed_parser.ex @@ -67,8 +67,8 @@ defmodule Mobilizon.Service.RichMedia.Parsers.OEmbed do {:ok, data} <- Jason.decode(json), data <- data - |> Map.new(fn {k, v} -> {String.to_existing_atom(k), String.trim(v)} end) - |> Map.take(@oembed_allowed_attributes) do + |> Map.new(fn {k, v} -> {k, if(is_binary(v), do: String.trim(v), else: v)} end) + |> Map.take(Enum.map(@oembed_allowed_attributes, &to_string/1)) do {:ok, data} end end