From 2fc25980d12c4f892cef850100e9d7890fc722b6 Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Sat, 13 Apr 2024 23:55:26 +0100 Subject: [PATCH] fix pattern matching in fetch errors --- lib/pleroma/object/fetcher.ex | 37 +++++++++++++++---- lib/pleroma/web/activity_pub/activity_pub.ex | 2 +- .../web/activity_pub/transmogrifier.ex | 2 +- lib/pleroma/workers/remote_fetcher_worker.ex | 12 ++++++ test/pleroma/object/fetcher_test.exs | 8 ++-- .../web/activity_pub/transmogrifier_test.exs | 12 ------ test/support/http_request_mock.ex | 1 - 7 files changed, 47 insertions(+), 27 deletions(-) diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index 97d8ebed5..b9d8dbaaa 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -136,10 +136,13 @@ def refetch_object(%Object{data: %{"id" => id}} = object) do def fetch_object_from_id(id, options \\ []) do with %URI{} = uri <- URI.parse(id), # let's check the URI is even vaguely valid first - {:valid_uri_scheme, true} <- {:valid_uri_scheme, uri.scheme == "http" or uri.scheme == "https"}, + {:valid_uri_scheme, true} <- + {:valid_uri_scheme, uri.scheme == "http" or uri.scheme == "https"}, # If we have instance restrictions, apply them here to prevent fetching from unwanted instances - {:ok, nil} <- Pleroma.Web.ActivityPub.MRF.SimplePolicy.check_reject(uri), - {:ok, _} <- Pleroma.Web.ActivityPub.MRF.SimplePolicy.check_accept(uri), + {:mrf_reject_check, {:ok, nil}} <- + {:mrf_reject_check, Pleroma.Web.ActivityPub.MRF.SimplePolicy.check_reject(uri)}, + {:mrf_accept_check, {:ok, _}} <- + {:mrf_accept_check, Pleroma.Web.ActivityPub.MRF.SimplePolicy.check_accept(uri)}, {_, nil} <- {:fetch_object, Object.get_cached_by_ap_id(id)}, {_, true} <- {:allowed_depth, Federator.allowed_thread_distance?(options[:depth])}, {_, {:ok, data}} <- {:fetch, fetch_and_contain_remote_object_from_id(id)}, @@ -159,6 +162,14 @@ def fetch_object_from_id(id, options \\ []) do log_fetch_error(id, e) {:error, :invalid_uri_scheme} + {:mrf_reject_check, _} = e -> + log_fetch_error(id, e) + {:reject, :mrf} + + {:mrf_accept_check, _} = e -> + log_fetch_error(id, e) + {:reject, :mrf} + {:containment, reason} = e -> log_fetch_error(id, e) {:error, reason} @@ -188,9 +199,6 @@ def fetch_object_from_id(id, options \\ []) do log_fetch_error(id, e) {:error, reason} - {:reject, reason} -> - {:reject, reason} - e -> log_fetch_error(id, e) {:error, e} @@ -255,7 +263,12 @@ def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do Logger.debug("Fetching object #{id} via AP") with {:valid_uri_scheme, true} <- {:valid_uri_scheme, String.starts_with?(id, "http")}, - {_, :ok} <- {:local_fetch, Containment.contain_local_fetch(id)}, + %URI{} = uri <- URI.parse(id), + {:mrf_reject_check, {:ok, nil}} <- + {:mrf_reject_check, Pleroma.Web.ActivityPub.MRF.SimplePolicy.check_reject(uri)}, + {:mrf_accept_check, {:ok, _}} <- + {:mrf_accept_check, Pleroma.Web.ActivityPub.MRF.SimplePolicy.check_accept(uri)}, + {:local_fetch, :ok} <- {:local_fetch, Containment.contain_local_fetch(id)}, {:ok, final_id, body} <- get_object(id), {:ok, data} <- safe_json_decode(body), {_, :ok} <- {:strict_id, Containment.contain_id_to_fetch(final_id, data)}, @@ -266,10 +279,18 @@ def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do {:ok, data} else - {:strict_id, _} = e-> + {:strict_id, _} = e -> log_fetch_error(id, e) {:error, :id_mismatch} + {:mrf_reject_check, _} = e -> + log_fetch_error(id, e) + {:reject, :mrf} + + {:mrf_accept_check, _} = e -> + log_fetch_error(id, e) + {:reject, :mrf} + {:valid_uri_scheme, _} = e -> log_fetch_error(id, e) {:error, :invalid_uri_scheme} diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 7c857530c..505fa7462 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1730,7 +1730,7 @@ def fetch_and_prepare_user_from_ap_id(ap_id, additional \\ []) do Logger.debug("Could not decode user at fetch #{ap_id}, #{inspect(e)}") {:error, e} - {:error, {:reject, reason} = e} -> + {:reject, reason} = e -> Logger.debug("Rejected user #{ap_id}: #{inspect(reason)}") {:error, e} diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index d10704a92..033fc9e78 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -162,7 +162,7 @@ def fix_quote_url(%{"quoteUri" => quote_url} = object, options) object end else - object + object end end diff --git a/lib/pleroma/workers/remote_fetcher_worker.ex b/lib/pleroma/workers/remote_fetcher_worker.ex index 844096f9a..1951aec7c 100644 --- a/lib/pleroma/workers/remote_fetcher_worker.ex +++ b/lib/pleroma/workers/remote_fetcher_worker.ex @@ -22,6 +22,18 @@ def perform(%Job{args: %{"op" => "fetch_remote", "id" => id} = args}) do {:error, :allowed_depth} -> {:discard, :allowed_depth} + {:error, :invalid_uri_scheme} -> + {:discard, :invalid_uri_scheme} + + {:error, :local_resource} -> + {:discard, :local_resource} + + {:reject, _} -> + {:discard, :reject} + + {:error, :id_mismatch} -> + {:discard, :id_mismatch} + _ -> :error end diff --git a/test/pleroma/object/fetcher_test.exs b/test/pleroma/object/fetcher_test.exs index 1cfd294cb..12154cb05 100644 --- a/test/pleroma/object/fetcher_test.exs +++ b/test/pleroma/object/fetcher_test.exs @@ -252,12 +252,12 @@ test "it does not fetch a spoofed object with wrong content type" do end test "it does not fetch a spoofed object with id different from URL" do - assert {:error, "Object's ActivityPub id/url does not match final fetch URL"} = + assert {:error, :id_mismatch} = Fetcher.fetch_and_contain_remote_object_from_id( "https://patch.cx/media/03ca3c8b4ac3ddd08bf0f84be7885f2f88de0f709112131a22d83650819e36c2.json" ) - assert {:error, "Object's ActivityPub id/url does not match final fetch URL"} = + assert {:error, :id_mismatch} = Fetcher.fetch_and_contain_remote_object_from_id( "https://patch.cx/media/spoof_stage1.json" ) @@ -287,14 +287,14 @@ test "it accepts same-domain redirects" do end test "it does not fetch a spoofed object with a foreign actor" do - assert {:error, "Object containment failed."} = + assert {:error, _} = Fetcher.fetch_and_contain_remote_object_from_id( "https://patch.cx/objects/spoof_foreign_actor" ) end test "it does not fetch from localhost" do - assert {:error, "Trying to fetch local resource"} = + assert {:error, :local_resource} = Fetcher.fetch_and_contain_remote_object_from_id( Pleroma.Web.Endpoint.url() <> "/spoof_local" ) diff --git a/test/pleroma/web/activity_pub/transmogrifier_test.exs b/test/pleroma/web/activity_pub/transmogrifier_test.exs index b35f4a04a..dd7977593 100644 --- a/test/pleroma/web/activity_pub/transmogrifier_test.exs +++ b/test/pleroma/web/activity_pub/transmogrifier_test.exs @@ -125,18 +125,6 @@ test "it fixes both the Create and object contexts in a reply" do assert activity.data["context"] == object.data["context"] end - test "it keeps link tags" do - insert(:user, ap_id: "https://example.org/users/alice") - - message = File.read!("test/fixtures/fep-e232.json") |> Jason.decode!() - - assert capture_log(fn -> - assert {:ok, activity} = Transmogrifier.handle_incoming(message) - object = Object.normalize(activity) - assert [%{"type" => "Mention"}, %{"type" => "Link"}] = object.data["tag"] - end) =~ "Object rejected while fetching" - end - test "it accepts quote posts" do insert(:user, ap_id: "https://misskey.io/users/7rkrarq81i") diff --git a/test/support/http_request_mock.ex b/test/support/http_request_mock.ex index 4df9b1770..042e4110e 100644 --- a/test/support/http_request_mock.ex +++ b/test/support/http_request_mock.ex @@ -1340,7 +1340,6 @@ def get("https://misskey.io/users/83ssedkv53", _, _, _) do }} end - def get("https://example.org/emoji/firedfox.png", _, _, _) do {:ok, %Tesla.Env{status: 200, body: File.read!("test/fixtures/image.jpg")}} end