Merge branch 'fix/2047-rich-media-parser' into 'develop'

RichMedia parser fix

Closes #2047

See merge request pleroma/pleroma!2941

4 files changed, 49 insertions, 38 deletions

diff --git a/lib/pleroma/web/rich_media/parser.ex b/lib/pleroma/web/rich_media/parser.ex
index ca592833f..e9aa2dd03 100644
--- a/lib/pleroma/web/rich_media/parser.ex
+++ b/lib/pleroma/web/rich_media/parser.ex
@@ -3,6 +3,8 @@
 # SPDX-License-Identifier: AGPL-3.0-only
 
 defmodule Pleroma.Web.RichMedia.Parser do
+  require Logger
+
   defp parsers do
     Pleroma.Config.get([:rich_media, :parsers])
   end
@@ -10,18 +12,19 @@ defmodule Pleroma.Web.RichMedia.Parser do
   def parse(nil), do: {:error, "No URL provided"}
 
   if Pleroma.Config.get(:env) == :test do
+    @spec parse(String.t()) :: {:ok, map()} | {:error, any()}
     def parse(url), do: parse_url(url)
   else
+    @spec parse(String.t()) :: {:ok, map()} | {:error, any()}
     def parse(url) do
-      try do
-        Cachex.fetch!(:rich_media_cache, url, fn _ ->
-          {:commit, parse_url(url)}
-        end)
-        |> set_ttl_based_on_image(url)
-      rescue
-        e ->
-          {:error, "Cachex error: #{inspect(e)}"}
-      end
+      Cachex.fetch!(:rich_media_cache, url, fn _ ->
+        with {:ok, data} <- parse_url(url) do
+          {:commit, {:ok, data}}
+        else
+          error -> {:ignore, error}
+        end
+      end)
+      |> set_ttl_based_on_image(url)
     end
   end
 
@@ -47,9 +50,11 @@ defmodule Pleroma.Web.RichMedia.Parser do
       config :pleroma, :rich_media,
         ttl_setters: [MyModule]
   """
+  @spec set_ttl_based_on_image({:ok, map()} | {:error, any()}, String.t()) ::
+          {:ok, map()} | {:error, any()}
   def set_ttl_based_on_image({:ok, data}, url) do
     with {:ok, nil} <- Cachex.ttl(:rich_media_cache, url),
-         ttl when is_number(ttl) <- get_ttl_from_image(data, url) do
+         {:ok, ttl} when is_number(ttl) <- get_ttl_from_image(data, url) do
       Cachex.expire_at(:rich_media_cache, url, ttl * 1000)
       {:ok, data}
     else
@@ -58,8 +63,14 @@ defmodule Pleroma.Web.RichMedia.Parser do
     end
   end
 
+  def set_ttl_based_on_image({:error, _} = error, _) do
+    Logger.error("parsing error: #{inspect(error)}")
+    error
+  end
+
   defp get_ttl_from_image(data, url) do
-    Pleroma.Config.get([:rich_media, :ttl_setters])
+    [:rich_media, :ttl_setters]
+    |> Pleroma.Config.get()
     |> Enum.reduce({:ok, nil}, fn
       module, {:ok, _ttl} ->
         module.ttl(data, url)
@@ -70,23 +81,16 @@ defmodule Pleroma.Web.RichMedia.Parser do
   end
 
   defp parse_url(url) do
-    try do
-      {:ok, %Tesla.Env{body: html}} = Pleroma.Web.RichMedia.Helpers.rich_media_get(url)
-
+    with {:ok, %Tesla.Env{body: html}} <- Pleroma.Web.RichMedia.Helpers.rich_media_get(url),
+         {:ok, html} <- Floki.parse_document(html) do
       html
-      |> parse_html()
       |> maybe_parse()
       |> Map.put("url", url)
       |> clean_parsed_data()
       |> check_parsed_data()
-    rescue
-      e ->
-        {:error, "Parsing error: #{inspect(e)} #{inspect(__STACKTRACE__)}"}
     end
   end
 
-  defp parse_html(html), do: Floki.parse_document!(html)
-
   defp maybe_parse(html) do
     Enum.reduce_while(parsers(), %{}, fn parser, acc ->
       case parser.parse(html, acc) do
diff --git a/lib/pleroma/web/rich_media/parsers/ttl/aws_signed_url.ex b/lib/pleroma/web/rich_media/parsers/ttl/aws_signed_url.ex
index 0dc1efdaf..c5aaea2d4 100644
--- a/lib/pleroma/web/rich_media/parsers/ttl/aws_signed_url.ex
+++ b/lib/pleroma/web/rich_media/parsers/ttl/aws_signed_url.ex
@@ -10,20 +10,15 @@ defmodule Pleroma.Web.RichMedia.Parser.TTL.AwsSignedUrl do
       |> parse_query_params()
       |> format_query_params()
       |> get_expiration_timestamp()
+    else
+      {:error, "Not aws signed url #{inspect(image)}"}
     end
   end
 
-  defp is_aws_signed_url(""), do: nil
-  defp is_aws_signed_url(nil), do: nil
-
-  defp is_aws_signed_url(image) when is_binary(image) do
+  defp is_aws_signed_url(image) when is_binary(image) and image != "" do
     %URI{host: host, query: query} = URI.parse(image)
 
-    if String.contains?(host, "amazonaws.com") and String.contains?(query, "X-Amz-Expires") do
-      image
-    else
-      nil
-    end
+    String.contains?(host, "amazonaws.com") and String.contains?(query, "X-Amz-Expires")
   end
 
   defp is_aws_signed_url(_), do: nil
@@ -46,6 +41,6 @@ defmodule Pleroma.Web.RichMedia.Parser.TTL.AwsSignedUrl do
       |> Map.get("X-Amz-Date")
       |> Timex.parse("{ISO:Basic:Z}")
 
-    Timex.to_unix(date) + String.to_integer(Map.get(params, "X-Amz-Expires"))
+    {:ok, Timex.to_unix(date) + String.to_integer(Map.get(params, "X-Amz-Expires"))}
   end
 end
diff --git a/test/web/rich_media/aws_signed_url_test.exs b/test/web/rich_media/aws_signed_url_test.exs
index b30f4400e..a21f3c935 100644
--- a/test/web/rich_media/aws_signed_url_test.exs
+++ b/test/web/rich_media/aws_signed_url_test.exs
@@ -21,7 +21,7 @@ defmodule Pleroma.Web.RichMedia.TTL.AwsSignedUrlTest do
     expire_time =
       Timex.parse!(timestamp, "{ISO:Basic:Z}") |> Timex.to_unix() |> Kernel.+(valid_till)
 
-    assert expire_time == Pleroma.Web.RichMedia.Parser.TTL.AwsSignedUrl.ttl(metadata, url)
+    assert {:ok, expire_time} == Pleroma.Web.RichMedia.Parser.TTL.AwsSignedUrl.ttl(metadata, url)
   end
 
   test "s3 signed url is parsed and correct ttl is set for rich media" do
diff --git a/test/web/rich_media/parser_test.exs b/test/web/rich_media/parser_test.exs
index 420a612c6..1e09cbf84 100644
--- a/test/web/rich_media/parser_test.exs
+++ b/test/web/rich_media/parser_test.exs
@@ -5,6 +5,8 @@
 defmodule Pleroma.Web.RichMedia.ParserTest do
   use ExUnit.Case, async: true
 
+  alias Pleroma.Web.RichMedia.Parser
+
   setup do
     Tesla.Mock.mock(fn
       %{
@@ -48,23 +50,29 @@ defmodule Pleroma.Web.RichMedia.ParserTest do
 
       %{method: :get, url: "http://example.com/empty"} ->
         %Tesla.Env{status: 200, body: "hello"}
+
+      %{method: :get, url: "http://example.com/malformed"} ->
+        %Tesla.Env{status: 200, body: File.read!("test/fixtures/rich_media/malformed-data.html")}
+
+      %{method: :get, url: "http://example.com/error"} ->
+        {:error, :overload}
     end)
 
     :ok
   end
 
   test "returns error when no metadata present" do
-    assert {:error, _} = Pleroma.Web.RichMedia.Parser.parse("http://example.com/empty")
+    assert {:error, _} = Parser.parse("http://example.com/empty")
   end
 
   test "doesn't just add a title" do
-    assert Pleroma.Web.RichMedia.Parser.parse("http://example.com/non-ogp") ==
+    assert Parser.parse("http://example.com/non-ogp") ==
              {:error,
               "Found metadata was invalid or incomplete: %{\"url\" => \"http://example.com/non-ogp\"}"}
   end
 
   test "parses ogp" do
-    assert Pleroma.Web.RichMedia.Parser.parse("http://example.com/ogp") ==
+    assert Parser.parse("http://example.com/ogp") ==
              {:ok,
               %{
                 "image" => "http://ia.media-imdb.com/images/rock.jpg",
@@ -77,7 +85,7 @@ defmodule Pleroma.Web.RichMedia.ParserTest do
   end
 
   test "falls back to <title> when ogp:title is missing" do
-    assert Pleroma.Web.RichMedia.Parser.parse("http://example.com/ogp-missing-title") ==
+    assert Parser.parse("http://example.com/ogp-missing-title") ==
              {:ok,
               %{
                 "image" => "http://ia.media-imdb.com/images/rock.jpg",
@@ -90,7 +98,7 @@ defmodule Pleroma.Web.RichMedia.ParserTest do
   end
 
   test "parses twitter card" do
-    assert Pleroma.Web.RichMedia.Parser.parse("http://example.com/twitter-card") ==
+    assert Parser.parse("http://example.com/twitter-card") ==
              {:ok,
               %{
                 "card" => "summary",
@@ -103,7 +111,7 @@ defmodule Pleroma.Web.RichMedia.ParserTest do
   end
 
   test "parses OEmbed" do
-    assert Pleroma.Web.RichMedia.Parser.parse("http://example.com/oembed") ==
+    assert Parser.parse("http://example.com/oembed") ==
              {:ok,
               %{
                 "author_name" => "‮‭‬bees‬",
@@ -132,6 +140,10 @@ defmodule Pleroma.Web.RichMedia.ParserTest do
   end
 
   test "rejects invalid OGP data" do
-    assert {:error, _} = Pleroma.Web.RichMedia.Parser.parse("http://example.com/malformed")
+    assert {:error, _} = Parser.parse("http://example.com/malformed")
+  end
+
+  test "returns error if getting page was not successful" do
+    assert {:error, :overload} = Parser.parse("http://example.com/error")
   end
 end