From c241b5b09f46d2b65da904b68647988913d58e9b Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 26 Dec 2023 14:20:33 -0500 Subject: [PATCH 01/33] Remove Fetcher.fetch_object_from_id!/2 It was only being called once and can be replaced with a case statement. --- lib/pleroma/object.ex | 5 ++++- lib/pleroma/object/fetcher.ex | 27 +++++---------------------- 2 files changed, 9 insertions(+), 23 deletions(-) diff --git a/lib/pleroma/object.ex b/lib/pleroma/object.ex index 844251a18..379b361f8 100644 --- a/lib/pleroma/object.ex +++ b/lib/pleroma/object.ex @@ -178,7 +178,10 @@ def normalize(ap_id, options) when is_binary(ap_id) do ap_id Keyword.get(options, :fetch) -> - Fetcher.fetch_object_from_id!(ap_id, options) + case Fetcher.fetch_object_from_id(ap_id, options) do + {:ok, object} -> object + _ -> nil + end true -> get_cached_by_ap_id(ap_id) diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index 267a82b27..12f7fa1bc 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -158,9 +158,11 @@ def fetch_object_from_id(id, options \\ []) do {:error, "URI Scheme Invalid"} {:transmogrifier, {:error, {:reject, e}}} -> + Logger.info("Rejected #{id} while fetching: #{inspect(e)}") {:reject, e} {:transmogrifier, {:reject, e}} -> + Logger.info("Rejected #{id} while fetching: #{inspect(e)}") {:reject, e} {:transmogrifier, _} = e -> @@ -176,13 +178,15 @@ def fetch_object_from_id(id, options \\ []) do {:ok, object} {:fetch, {:error, error}} -> + Logger.error("Error while fetching #{id}: #{inspect(error)}") {:error, error} {:reject, reason} -> {:reject, reason} e -> - e + Logger.error("Error while fetching #{id}: #{inspect(e)}") + {:error, e} end end @@ -199,27 +203,6 @@ defp prepare_activity_params(data) do |> Maps.put_if_present("bcc", data["bcc"]) end - @doc "Identical to `fetch_object_from_id/2` but just directly returns the object or on error `nil`" - def fetch_object_from_id!(id, options \\ []) do - with {:ok, object} <- fetch_object_from_id(id, options) do - object - else - {:error, %Tesla.Mock.Error{}} -> - nil - - {:error, {"Object has been deleted", _id, _code}} -> - nil - - {:reject, reason} -> - Logger.debug("Rejected #{id} while fetching: #{inspect(reason)}") - nil - - e -> - Logger.error("Error while fetching #{id}: #{inspect(e)}") - nil - end - end - defp make_signature(id, date) do uri = URI.parse(id) From ac4cc619eae869d431bd275ced9aeadd83ec2e8f Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 26 Dec 2023 14:22:04 -0500 Subject: [PATCH 02/33] Fix Transmogrifier tests These tests relied on the removed Fetcher.fetch_object_from_id!/2 function injecting the error tuple into a log message with the exact words "Object containment failed." We will keep this behavior by generating a similar log message, but perhaps this should do a better job of matching on the error tuple returned by Transmogrifier.handle_incoming/1 --- lib/pleroma/object/fetcher.ex | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index 12f7fa1bc..c0be6dc49 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -157,6 +157,10 @@ def fetch_object_from_id(id, options \\ []) do {:scheme, false} -> {:error, "URI Scheme Invalid"} + {:containment, e} -> + Logger.info("Error while fetching #{id}: Object containment failed. #{inspect(e)}") + {:error, e} + {:transmogrifier, {:error, {:reject, e}}} -> Logger.info("Rejected #{id} while fetching: #{inspect(e)}") {:reject, e} From 4c29366fe503fb383544efc71a5f78f0602613cb Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 26 Dec 2023 15:54:14 -0500 Subject: [PATCH 03/33] Mark instances as unreachable when returning a 403 from an object fetch This is a definite sign the instance is blocked and they are enforcing authorized_fetch --- lib/pleroma/object/fetcher.ex | 9 +++++++++ lib/pleroma/workers/remote_fetcher_worker.ex | 11 +++++++++++ test/pleroma/object/fetcher_test.exs | 14 ++++++++++++++ 3 files changed, 34 insertions(+) diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index c0be6dc49..dc13810c8 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -181,6 +181,15 @@ def fetch_object_from_id(id, options \\ []) do {:fetch_object, %Object{} = object} -> {:ok, object} + {:fetch, {:error, {:ok, %Tesla.Env{status: 403}}}} -> + Instances.set_consistently_unreachable(id) + + Logger.error( + "Error while fetching #{id}: HTTP 403 likely due to instance block rejecting the signed fetch." + ) + + {:error, "Object fetch has been denied"} + {:fetch, {:error, error}} -> Logger.error("Error while fetching #{id}: #{inspect(error)}") {:error, error} diff --git a/lib/pleroma/workers/remote_fetcher_worker.ex b/lib/pleroma/workers/remote_fetcher_worker.ex index ad4d785a1..80d34e303 100644 --- a/lib/pleroma/workers/remote_fetcher_worker.ex +++ b/lib/pleroma/workers/remote_fetcher_worker.ex @@ -10,5 +10,16 @@ defmodule Pleroma.Workers.RemoteFetcherWorker do @impl Oban.Worker def perform(%Job{args: %{"op" => "fetch_remote", "id" => id} = args}) do {:ok, _object} = Fetcher.fetch_object_from_id(id, depth: args["depth"]) + + case Fetcher.fetch_object_from_id(id, depth: args["depth"]) do + {:ok, _object} -> + :ok + + {:error, reason = "Object fetch has been denied"} -> + {:cancel, reason} + + _ -> + :error + end end end diff --git a/test/pleroma/object/fetcher_test.exs b/test/pleroma/object/fetcher_test.exs index 4c4831af3..fa6259ff4 100644 --- a/test/pleroma/object/fetcher_test.exs +++ b/test/pleroma/object/fetcher_test.exs @@ -57,6 +57,8 @@ defp spoofed_object_with_ids( body: spoofed_object_with_ids("https://patch.cx/objects/spoof_content_type") } + %{method: :get, url: "https://octodon.social/users/cwebber/statuses/111647596861000656"} -> + %Tesla.Env{status: 403} # Spoof: mismatching ids # Variant 1: Non-exisitng fake id %{ @@ -417,6 +419,18 @@ test "handle HTTP 404 response" do ) end + test "handle HTTP 403 response" do + object_id = "https://octodon.social/users/cwebber/statuses/111647596861000656" + Instances.set_reachable(object_id) + + assert Instances.reachable?(object_id) + + assert {:error, "Object fetch has been denied"} == + Fetcher.fetch_object_from_id(object_id) + + refute Instances.reachable?(object_id) + end + test "it can fetch pleroma polls with attachments" do {:ok, object} = Fetcher.fetch_object_from_id("https://patch.cx/objects/tesla_mock/poll_attachment") From 4ff22a409a4f6fba83cf97b3ba89e66a6ba2f107 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 26 Dec 2023 16:05:28 -0500 Subject: [PATCH 04/33] Consolidate the HTTP status code checking into the private get_object/1 --- lib/pleroma/object/fetcher.ex | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index dc13810c8..431ad4845 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -181,15 +181,6 @@ def fetch_object_from_id(id, options \\ []) do {:fetch_object, %Object{} = object} -> {:ok, object} - {:fetch, {:error, {:ok, %Tesla.Env{status: 403}}}} -> - Instances.set_consistently_unreachable(id) - - Logger.error( - "Error while fetching #{id}: HTTP 403 likely due to instance block rejecting the signed fetch." - ) - - {:error, "Object fetch has been denied"} - {:fetch, {:error, error}} -> Logger.error("Error while fetching #{id}: #{inspect(error)}") {:error, error} @@ -352,6 +343,10 @@ def get_object(id) do {:error, {:content_type, content_type}} end else + {:ok, %{status: 403}} -> + Instances.set_consistently_unreachable(id) + {:error, "Object fetch has been denied"} + {:ok, %{status: code}} when code in [404, 410] -> {:error, {"Object has been deleted", id, code}} From 132036f951ca52423ea4a45d71085cd2b51fb6db Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 26 Dec 2023 16:05:44 -0500 Subject: [PATCH 05/33] Cancel remote fetch jobs for deleted objects --- lib/pleroma/workers/remote_fetcher_worker.ex | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/pleroma/workers/remote_fetcher_worker.ex b/lib/pleroma/workers/remote_fetcher_worker.ex index 80d34e303..d4ffbea3c 100644 --- a/lib/pleroma/workers/remote_fetcher_worker.ex +++ b/lib/pleroma/workers/remote_fetcher_worker.ex @@ -18,6 +18,9 @@ def perform(%Job{args: %{"op" => "fetch_remote", "id" => id} = args}) do {:error, reason = "Object fetch has been denied"} -> {:cancel, reason} + {:error, reason = "Object has been deleted"} -> + {:cancel, reason} + _ -> :error end From 160d113b303696396fc2a3798b689bddf0c7cfe6 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 26 Dec 2023 16:08:36 -0500 Subject: [PATCH 06/33] Changelogs --- changelog.d/handle_object_fetch_failures.change | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelog.d/handle_object_fetch_failures.change diff --git a/changelog.d/handle_object_fetch_failures.change b/changelog.d/handle_object_fetch_failures.change new file mode 100644 index 000000000..413084322 --- /dev/null +++ b/changelog.d/handle_object_fetch_failures.change @@ -0,0 +1,2 @@ +Remote object fetch failures will prevent the object fetch job from retrying if the object has been deleted or the fetch was denied with a 403 due to instance block behavior with authorized_fetch enabled. +Mark instances as unreachable when object fetch is denied due to instance block and authorized_fetch. From 6d368808d31b1f1bf4ef7504b874105581d53586 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 26 Dec 2023 16:23:41 -0500 Subject: [PATCH 07/33] Remove mistaken duplicate fetch --- lib/pleroma/workers/remote_fetcher_worker.ex | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/pleroma/workers/remote_fetcher_worker.ex b/lib/pleroma/workers/remote_fetcher_worker.ex index d4ffbea3c..4cae7e108 100644 --- a/lib/pleroma/workers/remote_fetcher_worker.ex +++ b/lib/pleroma/workers/remote_fetcher_worker.ex @@ -9,8 +9,6 @@ defmodule Pleroma.Workers.RemoteFetcherWorker do @impl Oban.Worker def perform(%Job{args: %{"op" => "fetch_remote", "id" => id} = args}) do - {:ok, _object} = Fetcher.fetch_object_from_id(id, depth: args["depth"]) - case Fetcher.fetch_object_from_id(id, depth: args["depth"]) do {:ok, _object} -> :ok From e2b04fac5a9e257918be8c2bc0c8ab009515e7a6 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 26 Dec 2023 16:28:05 -0500 Subject: [PATCH 08/33] Skip remote fetch jobs for unreachable instances --- .../handle_object_fetch_failures.change | 1 + lib/pleroma/workers/remote_fetcher_worker.ex | 23 +++++++++++-------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/changelog.d/handle_object_fetch_failures.change b/changelog.d/handle_object_fetch_failures.change index 413084322..410f95efa 100644 --- a/changelog.d/handle_object_fetch_failures.change +++ b/changelog.d/handle_object_fetch_failures.change @@ -1,2 +1,3 @@ Remote object fetch failures will prevent the object fetch job from retrying if the object has been deleted or the fetch was denied with a 403 due to instance block behavior with authorized_fetch enabled. Mark instances as unreachable when object fetch is denied due to instance block and authorized_fetch. +Skip fetching objects from unreachable instances. diff --git a/lib/pleroma/workers/remote_fetcher_worker.ex b/lib/pleroma/workers/remote_fetcher_worker.ex index 4cae7e108..d091dfa84 100644 --- a/lib/pleroma/workers/remote_fetcher_worker.ex +++ b/lib/pleroma/workers/remote_fetcher_worker.ex @@ -3,24 +3,29 @@ # SPDX-License-Identifier: AGPL-3.0-only defmodule Pleroma.Workers.RemoteFetcherWorker do + alias Pleroma.Instances alias Pleroma.Object.Fetcher use Pleroma.Workers.WorkerHelper, queue: "remote_fetcher" @impl Oban.Worker def perform(%Job{args: %{"op" => "fetch_remote", "id" => id} = args}) do - case Fetcher.fetch_object_from_id(id, depth: args["depth"]) do - {:ok, _object} -> - :ok + if Instances.reachable?(id) do + case Fetcher.fetch_object_from_id(id, depth: args["depth"]) do + {:ok, _object} -> + :ok - {:error, reason = "Object fetch has been denied"} -> - {:cancel, reason} + {:error, reason = "Object fetch has been denied"} -> + {:cancel, reason} - {:error, reason = "Object has been deleted"} -> - {:cancel, reason} + {:error, reason = "Object has been deleted"} -> + {:cancel, reason} - _ -> - :error + _ -> + :error + end + else + {:cancel, "Unreachable instance"} end end end From 30d63aaa6e55f109052e2107417e7a458675b881 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 26 Dec 2023 17:12:58 -0500 Subject: [PATCH 09/33] Revert "Mark instances as unreachable when returning a 403 from an object fetch" This reverts commit d472bafec19cee269e7c943bafae7c805785acd7. --- changelog.d/handle_object_fetch_failures.change | 1 - lib/pleroma/object/fetcher.ex | 1 - test/pleroma/object/fetcher_test.exs | 12 ------------ 3 files changed, 14 deletions(-) diff --git a/changelog.d/handle_object_fetch_failures.change b/changelog.d/handle_object_fetch_failures.change index 410f95efa..0b1dda38d 100644 --- a/changelog.d/handle_object_fetch_failures.change +++ b/changelog.d/handle_object_fetch_failures.change @@ -1,3 +1,2 @@ Remote object fetch failures will prevent the object fetch job from retrying if the object has been deleted or the fetch was denied with a 403 due to instance block behavior with authorized_fetch enabled. -Mark instances as unreachable when object fetch is denied due to instance block and authorized_fetch. Skip fetching objects from unreachable instances. diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index 431ad4845..8bcd6ed95 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -344,7 +344,6 @@ def get_object(id) do end else {:ok, %{status: 403}} -> - Instances.set_consistently_unreachable(id) {:error, "Object fetch has been denied"} {:ok, %{status: code}} when code in [404, 410] -> diff --git a/test/pleroma/object/fetcher_test.exs b/test/pleroma/object/fetcher_test.exs index fa6259ff4..8617090d9 100644 --- a/test/pleroma/object/fetcher_test.exs +++ b/test/pleroma/object/fetcher_test.exs @@ -419,18 +419,6 @@ test "handle HTTP 404 response" do ) end - test "handle HTTP 403 response" do - object_id = "https://octodon.social/users/cwebber/statuses/111647596861000656" - Instances.set_reachable(object_id) - - assert Instances.reachable?(object_id) - - assert {:error, "Object fetch has been denied"} == - Fetcher.fetch_object_from_id(object_id) - - refute Instances.reachable?(object_id) - end - test "it can fetch pleroma polls with attachments" do {:ok, object} = Fetcher.fetch_object_from_id("https://patch.cx/objects/tesla_mock/poll_attachment") From eeed051a0f60c7e14d21b59b66ca4a26c7bd55b1 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 27 Dec 2023 15:27:37 -0500 Subject: [PATCH 10/33] Fix detection of user follower collection being private We were overzealous with matching on a raw error from the object fetch that should have never been relied on like this. If we can't fetch successfully we should assume that the collection is private. Building a more expressive and universal error struct to match on may be something to consider. --- lib/pleroma/web/activity_pub/activity_pub.ex | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 1e06bc809..7c857530c 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -1705,9 +1705,7 @@ defp collection_private(%{"first" => first}) do Fetcher.fetch_and_contain_remote_object_from_id(first) do {:ok, false} else - {:error, {:ok, %{status: code}}} when code in [401, 403] -> {:ok, true} - {:error, _} = e -> e - e -> {:error, e} + {:error, _} -> {:ok, true} end end From 331710b6bb1aede8d00beb461487bc29a25f0293 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 27 Dec 2023 16:06:10 -0500 Subject: [PATCH 11/33] RemoteFetcherWorker Oban job tests --- .../workers/remote_fetcher_worker_test.exs | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 test/pleroma/workers/remote_fetcher_worker_test.exs diff --git a/test/pleroma/workers/remote_fetcher_worker_test.exs b/test/pleroma/workers/remote_fetcher_worker_test.exs new file mode 100644 index 000000000..d8cb52f52 --- /dev/null +++ b/test/pleroma/workers/remote_fetcher_worker_test.exs @@ -0,0 +1,67 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2023 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Workers.RemoteFetcherWorkerTest do + use Pleroma.DataCase + use Oban.Testing, repo: Pleroma.Repo + + alias Pleroma.Instances + alias Pleroma.Workers.RemoteFetcherWorker + + @deleted_object_one "https://deleted-404.example.com/" + @deleted_object_two "https://deleted-410.example.com/" + @unauthorized_object "https://unauthorized.example.com/" + @unreachable_object "https://unreachable.example.com/" + + describe "it does not" do + setup do + Tesla.Mock.mock(fn + %{method: :get, url: @deleted_object_one} -> + %Tesla.Env{ + status: 404 + } + + %{method: :get, url: @deleted_object_two} -> + %Tesla.Env{ + status: 410 + } + + %{method: :get, url: @unauthorized_object} -> + %Tesla.Env{ + status: 403 + } + end) + end + + test "requeue a deleted object" do + assert {:cancel, _} = + RemoteFetcherWorker.perform(%Oban.Job{ + args: %{"op" => "fetch_remote", "id" => @deleted_object_one} + }) + + assert {:cancel, _} = + RemoteFetcherWorker.perform(%Oban.Job{ + args: %{"op" => "fetch_remote", "id" => @deleted_object_two} + }) + end + + test "requeue an unauthorized object" do + assert {:cancel, _} = + RemoteFetcherWorker.perform(%Oban.Job{ + args: %{"op" => "fetch_remote", "id" => @unauthorized_object} + }) + end + + test "fetch an unreachable instance" do + Instances.set_consistently_unreachable(@unreachable_object) + + refute Instances.reachable?(@unreachable_object) + + assert {:cancel, _} = + RemoteFetcherWorker.perform(%Oban.Job{ + args: %{"op" => "fetch_remote", "id" => @unreachable_object} + }) + end + end +end From 825ae46bfabacaf3fb6efc5ec2b1ac1c5aeec135 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 27 Dec 2023 20:18:14 -0500 Subject: [PATCH 12/33] Set Logger level to error --- lib/pleroma/object/fetcher.ex | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index 8bcd6ed95..d05a936fb 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -158,15 +158,15 @@ def fetch_object_from_id(id, options \\ []) do {:error, "URI Scheme Invalid"} {:containment, e} -> - Logger.info("Error while fetching #{id}: Object containment failed. #{inspect(e)}") + Logger.error("Error while fetching #{id}: Object containment failed. #{inspect(e)}") {:error, e} {:transmogrifier, {:error, {:reject, e}}} -> - Logger.info("Rejected #{id} while fetching: #{inspect(e)}") + Logger.error("Rejected #{id} while fetching: #{inspect(e)}") {:reject, e} {:transmogrifier, {:reject, e}} -> - Logger.info("Rejected #{id} while fetching: #{inspect(e)}") + Logger.error("Rejected #{id} while fetching: #{inspect(e)}") {:reject, e} {:transmogrifier, _} = e -> From 3c54f407c568869c4e3fb30670ef251ede5178e5 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 27 Dec 2023 20:47:18 -0500 Subject: [PATCH 13/33] Conslidate log messages for object fetcher failures and leverage Logger.metadata --- lib/pleroma/object/fetcher.ex | 39 ++++++++++-------- .../web/activity_pub/transmogrifier_test.exs | 40 +++++++++++++++++-- 2 files changed, 60 insertions(+), 19 deletions(-) diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index d05a936fb..a9118b11c 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -151,26 +151,28 @@ def fetch_object_from_id(id, options \\ []) do {:object, data, Object.normalize(activity, fetch: false)} do {:ok, object} else - {:allowed_depth, false} -> + {:allowed_depth, false} = e -> + log_fetch_error(id, e) {:error, "Max thread distance exceeded."} {:scheme, false} -> {:error, "URI Scheme Invalid"} - {:containment, e} -> - Logger.error("Error while fetching #{id}: Object containment failed. #{inspect(e)}") - {:error, e} + {:containment, reason} = e -> + log_fetch_error(id, e) + {:error, reason} - {:transmogrifier, {:error, {:reject, e}}} -> - Logger.error("Rejected #{id} while fetching: #{inspect(e)}") - {:reject, e} + {:transmogrifier, {:error, {:reject, reason}}} = e -> + log_fetch_error(id, e) + {:reject, reason} - {:transmogrifier, {:reject, e}} -> - Logger.error("Rejected #{id} while fetching: #{inspect(e)}") - {:reject, e} + {:transmogrifier, {:reject, reason}} = e -> + log_fetch_error(id, e) + {:reject, reason} - {:transmogrifier, _} = e -> - {:error, e} + {:transmogrifier, reason} = e -> + log_fetch_error(id, e) + {:error, reason} {:object, data, nil} -> reinject_object(%Object{}, data) @@ -181,19 +183,24 @@ def fetch_object_from_id(id, options \\ []) do {:fetch_object, %Object{} = object} -> {:ok, object} - {:fetch, {:error, error}} -> - Logger.error("Error while fetching #{id}: #{inspect(error)}") - {:error, error} + {:fetch, {:error, reason}} = e -> + log_fetch_error(id, e) + {:error, reason} {:reject, reason} -> {:reject, reason} e -> - Logger.error("Error while fetching #{id}: #{inspect(e)}") + log_fetch_error(id, e) {:error, e} end end + defp log_fetch_error(id, error) do + Logger.metadata([object: id]) + Logger.error("Object rejected while fetching #{id} #{inspect(error)}") + end + defp prepare_activity_params(data) do %{ "type" => "Create", diff --git a/test/pleroma/web/activity_pub/transmogrifier_test.exs b/test/pleroma/web/activity_pub/transmogrifier_test.exs index e473ae659..164e9f9ad 100644 --- a/test/pleroma/web/activity_pub/transmogrifier_test.exs +++ b/test/pleroma/web/activity_pub/transmogrifier_test.exs @@ -124,6 +124,40 @@ 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") + + object = File.read!("test/fixtures/quote_post/misskey_quote_post.json") |> Jason.decode!() + + message = %{ + "@context" => "https://www.w3.org/ns/activitystreams", + "type" => "Create", + "actor" => "https://misskey.io/users/7rkrarq81i", + "object" => object + } + + assert {:ok, activity} = Transmogrifier.handle_incoming(message) + + # Object was created in the database + object = Object.normalize(activity) + assert object.data["quoteUrl"] == "https://misskey.io/notes/8vs6wxufd0" + + # It fetched the quoted post + assert Object.normalize("https://misskey.io/notes/8vs6wxufd0") + end end describe "prepare outgoing" do @@ -413,7 +447,7 @@ test "it rejects activities which reference objects with bogus origins" do assert capture_log(fn -> {:error, _} = Transmogrifier.handle_incoming(data) - end) =~ "Object containment failed" + end) =~ "Object rejected while fetching" end test "it rejects activities which reference objects that have an incorrect attribution (variant 1)" do @@ -428,7 +462,7 @@ test "it rejects activities which reference objects that have an incorrect attri assert capture_log(fn -> {:error, _} = Transmogrifier.handle_incoming(data) - end) =~ "Object containment failed" + end) =~ "Object rejected while fetching" end test "it rejects activities which reference objects that have an incorrect attribution (variant 2)" do @@ -443,7 +477,7 @@ test "it rejects activities which reference objects that have an incorrect attri assert capture_log(fn -> {:error, _} = Transmogrifier.handle_incoming(data) - end) =~ "Object containment failed" + end) =~ "Object rejected while fetching" end end From d69cba1b93270894e7d29d24cf3e75dc929a1e3e Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 27 Dec 2023 21:39:17 -0500 Subject: [PATCH 14/33] Remove duplicate log messages from Transmogrifier Object fetch errors are logged in the fetcher module --- lib/pleroma/web/activity_pub/transmogrifier.ex | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index a72a431b2..ccbbba981 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -25,7 +25,6 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do import Ecto.Query - require Logger require Pleroma.Constants @doc """ @@ -135,8 +134,7 @@ def fix_in_reply_to(%{"inReplyTo" => in_reply_to} = object, options) |> Map.put("context", replied_object.data["context"] || object["conversation"]) |> Map.drop(["conversation", "inReplyToAtomUri"]) else - e -> - Logger.warning("Couldn't fetch reply@#{inspect(in_reply_to_id)}, error: #{inspect(e)}") + _ -> object end else @@ -163,7 +161,11 @@ def fix_quote_url(%{"quoteUri" => quote_url} = object, options) object end else - object + {:quoting?, _} -> + object + + _ -> + object end end @@ -833,8 +835,7 @@ def maybe_fix_object_url(%{"object" => object} = data) when is_binary(object) do relative_object do Map.put(data, "object", external_url) else - {:fetch, e} -> - Logger.error("Couldn't fetch fixed_object@#{object} #{inspect(e)}") + {:fetch, _} -> data _ -> From 53a9413b954ed00ae89cd3c03f05c04dfa1d9e3f Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 27 Dec 2023 21:55:07 -0500 Subject: [PATCH 15/33] Formatting --- lib/pleroma/object/fetcher.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index a9118b11c..59fee83a9 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -197,7 +197,7 @@ def fetch_object_from_id(id, options \\ []) do end defp log_fetch_error(id, error) do - Logger.metadata([object: id]) + Logger.metadata(object: id) Logger.error("Object rejected while fetching #{id} #{inspect(error)}") end From 7e5004b3e234d3a552682c68006a23962d0f6131 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 27 Dec 2023 21:57:47 -0500 Subject: [PATCH 16/33] Leverage existing atoms as return errors for the object fetcher --- lib/pleroma/object/fetcher.ex | 4 ++-- lib/pleroma/workers/remote_fetcher_worker.ex | 8 ++++---- test/pleroma/object/fetcher_test.exs | 6 ++---- .../web/twitter_api/remote_follow_controller_test.exs | 2 +- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index 59fee83a9..edaf8c62d 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -351,10 +351,10 @@ def get_object(id) do end else {:ok, %{status: 403}} -> - {:error, "Object fetch has been denied"} + {:error, :forbidden} {:ok, %{status: code}} when code in [404, 410] -> - {:error, {"Object has been deleted", id, code}} + {:error, :not_found} {:error, e} -> {:error, e} diff --git a/lib/pleroma/workers/remote_fetcher_worker.ex b/lib/pleroma/workers/remote_fetcher_worker.ex index d091dfa84..1120a3e4e 100644 --- a/lib/pleroma/workers/remote_fetcher_worker.ex +++ b/lib/pleroma/workers/remote_fetcher_worker.ex @@ -15,11 +15,11 @@ def perform(%Job{args: %{"op" => "fetch_remote", "id" => id} = args}) do {:ok, _object} -> :ok - {:error, reason = "Object fetch has been denied"} -> - {:cancel, reason} + {:error, :forbidden} -> + {:cancel, :forbidden} - {:error, reason = "Object has been deleted"} -> - {:cancel, reason} + {:error, :not_found} -> + {:cancel, :not_found} _ -> :error diff --git a/test/pleroma/object/fetcher_test.exs b/test/pleroma/object/fetcher_test.exs index 8617090d9..55902ba49 100644 --- a/test/pleroma/object/fetcher_test.exs +++ b/test/pleroma/object/fetcher_test.exs @@ -404,16 +404,14 @@ test "all objects with fake directions are rejected by the object fetcher" do end test "handle HTTP 410 Gone response" do - assert {:error, - {"Object has been deleted", "https://mastodon.example.org/users/userisgone", 410}} == + assert {:error, :not_found} == Fetcher.fetch_and_contain_remote_object_from_id( "https://mastodon.example.org/users/userisgone" ) end test "handle HTTP 404 response" do - assert {:error, - {"Object has been deleted", "https://mastodon.example.org/users/userisgone404", 404}} == + assert {:error, :not_found} == Fetcher.fetch_and_contain_remote_object_from_id( "https://mastodon.example.org/users/userisgone404" ) diff --git a/test/pleroma/web/twitter_api/remote_follow_controller_test.exs b/test/pleroma/web/twitter_api/remote_follow_controller_test.exs index d2bc7840f..5a94e4396 100644 --- a/test/pleroma/web/twitter_api/remote_follow_controller_test.exs +++ b/test/pleroma/web/twitter_api/remote_follow_controller_test.exs @@ -132,7 +132,7 @@ test "show follow page with error when user can not be fetched by `acct` link", |> html_response(200) assert response =~ "Error fetching user" - end) =~ "Object has been deleted" + end) =~ ":not_found" end end From ff515c05c36825f28c547bc50694e203182708cb Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 27 Dec 2023 22:28:41 -0500 Subject: [PATCH 17/33] Prevent requeuing Remote Fetcher jobs that exceed thread depth --- changelog.d/handle_object_fetch_failures.change | 2 -- lib/pleroma/object/fetcher.ex | 2 +- lib/pleroma/workers/remote_fetcher_worker.ex | 3 +++ test/pleroma/object/fetcher_test.exs | 3 +-- .../workers/remote_fetcher_worker_test.exs | 15 +++++++++++++++ 5 files changed, 20 insertions(+), 5 deletions(-) delete mode 100644 changelog.d/handle_object_fetch_failures.change diff --git a/changelog.d/handle_object_fetch_failures.change b/changelog.d/handle_object_fetch_failures.change deleted file mode 100644 index 0b1dda38d..000000000 --- a/changelog.d/handle_object_fetch_failures.change +++ /dev/null @@ -1,2 +0,0 @@ -Remote object fetch failures will prevent the object fetch job from retrying if the object has been deleted or the fetch was denied with a 403 due to instance block behavior with authorized_fetch enabled. -Skip fetching objects from unreachable instances. diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index edaf8c62d..e2d0908d3 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -153,7 +153,7 @@ def fetch_object_from_id(id, options \\ []) do else {:allowed_depth, false} = e -> log_fetch_error(id, e) - {:error, "Max thread distance exceeded."} + {:error, :allowed_depth} {:scheme, false} -> {:error, "URI Scheme Invalid"} diff --git a/lib/pleroma/workers/remote_fetcher_worker.ex b/lib/pleroma/workers/remote_fetcher_worker.ex index 1120a3e4e..0816d5737 100644 --- a/lib/pleroma/workers/remote_fetcher_worker.ex +++ b/lib/pleroma/workers/remote_fetcher_worker.ex @@ -21,6 +21,9 @@ def perform(%Job{args: %{"op" => "fetch_remote", "id" => id} = args}) do {:error, :not_found} -> {:cancel, :not_found} + {:error, :allowed_depth} -> + {:cancel, :allowed_depth} + _ -> :error end diff --git a/test/pleroma/object/fetcher_test.exs b/test/pleroma/object/fetcher_test.exs index 55902ba49..6a919e6ef 100644 --- a/test/pleroma/object/fetcher_test.exs +++ b/test/pleroma/object/fetcher_test.exs @@ -205,8 +205,7 @@ test "it works when fetching the OP actor errors out" do test "it returns thread depth exceeded error if thread depth is exceeded" do clear_config([:instance, :federation_incoming_replies_max_depth], 0) - assert {:error, "Max thread distance exceeded."} = - Fetcher.fetch_object_from_id(@ap_id, depth: 1) + assert {:error, :allowed_depth} = Fetcher.fetch_object_from_id(@ap_id, depth: 1) end test "it fetches object if max thread depth is restricted to 0 and depth is not specified" do diff --git a/test/pleroma/workers/remote_fetcher_worker_test.exs b/test/pleroma/workers/remote_fetcher_worker_test.exs index d8cb52f52..df7d77ea0 100644 --- a/test/pleroma/workers/remote_fetcher_worker_test.exs +++ b/test/pleroma/workers/remote_fetcher_worker_test.exs @@ -13,6 +13,7 @@ defmodule Pleroma.Workers.RemoteFetcherWorkerTest do @deleted_object_two "https://deleted-410.example.com/" @unauthorized_object "https://unauthorized.example.com/" @unreachable_object "https://unreachable.example.com/" + @depth_object "https://depth.example.com/" describe "it does not" do setup do @@ -31,6 +32,11 @@ defmodule Pleroma.Workers.RemoteFetcherWorkerTest do %Tesla.Env{ status: 403 } + + %{method: :get, url: @depth_object} -> + %Tesla.Env{ + status: 200 + } end) end @@ -63,5 +69,14 @@ test "fetch an unreachable instance" do args: %{"op" => "fetch_remote", "id" => @unreachable_object} }) end + + test "requeue an object that exceeded depth" do + clear_config([:instance, :federation_incoming_replies_max_depth], 0) + + assert {:cancel, _} = + RemoteFetcherWorker.perform(%Oban.Job{ + args: %{"op" => "fetch_remote", "id" => @depth_object, "depth" => 1} + }) + end end end From f31b262aec476cb0923e821edb4ee9ca18e6c563 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 27 Dec 2023 22:37:06 -0500 Subject: [PATCH 18/33] Improve test descriptions --- test/pleroma/workers/remote_fetcher_worker_test.exs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/pleroma/workers/remote_fetcher_worker_test.exs b/test/pleroma/workers/remote_fetcher_worker_test.exs index df7d77ea0..84899cc5f 100644 --- a/test/pleroma/workers/remote_fetcher_worker_test.exs +++ b/test/pleroma/workers/remote_fetcher_worker_test.exs @@ -15,7 +15,7 @@ defmodule Pleroma.Workers.RemoteFetcherWorkerTest do @unreachable_object "https://unreachable.example.com/" @depth_object "https://depth.example.com/" - describe "it does not" do + describe "RemoteFetcherWorker" do setup do Tesla.Mock.mock(fn %{method: :get, url: @deleted_object_one} -> @@ -40,7 +40,7 @@ defmodule Pleroma.Workers.RemoteFetcherWorkerTest do end) end - test "requeue a deleted object" do + test "does not requeue a deleted object" do assert {:cancel, _} = RemoteFetcherWorker.perform(%Oban.Job{ args: %{"op" => "fetch_remote", "id" => @deleted_object_one} @@ -52,14 +52,14 @@ test "requeue a deleted object" do }) end - test "requeue an unauthorized object" do + test "does not requeue an unauthorized object" do assert {:cancel, _} = RemoteFetcherWorker.perform(%Oban.Job{ args: %{"op" => "fetch_remote", "id" => @unauthorized_object} }) end - test "fetch an unreachable instance" do + test "does not fetch an unreachable instance" do Instances.set_consistently_unreachable(@unreachable_object) refute Instances.reachable?(@unreachable_object) @@ -70,7 +70,7 @@ test "fetch an unreachable instance" do }) end - test "requeue an object that exceeded depth" do + test "does not requeue an object that exceeded depth" do clear_config([:instance, :federation_incoming_replies_max_depth], 0) assert {:cancel, _} = From c0532bcae093c3d497619070b4a14565d23867fd Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 28 Dec 2023 23:09:33 -0500 Subject: [PATCH 19/33] Handle 401s as I have observed it in the wild --- lib/pleroma/object/fetcher.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index e2d0908d3..9d2a02db6 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -350,7 +350,7 @@ def get_object(id) do {:error, {:content_type, content_type}} end else - {:ok, %{status: 403}} -> + {:ok, %{status: code}} when code in [401, 403] -> {:error, :forbidden} {:ok, %{status: code}} when code in [404, 410] -> From fed7a78c773ca94ffb164e8746c7901b7c5f9d8d Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 28 Dec 2023 23:52:05 -0500 Subject: [PATCH 20/33] Oban jobs should be discarded on permanent errors --- lib/pleroma/workers/remote_fetcher_worker.ex | 8 ++++---- test/pleroma/workers/remote_fetcher_worker_test.exs | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/pleroma/workers/remote_fetcher_worker.ex b/lib/pleroma/workers/remote_fetcher_worker.ex index 0816d5737..618b72a19 100644 --- a/lib/pleroma/workers/remote_fetcher_worker.ex +++ b/lib/pleroma/workers/remote_fetcher_worker.ex @@ -16,19 +16,19 @@ def perform(%Job{args: %{"op" => "fetch_remote", "id" => id} = args}) do :ok {:error, :forbidden} -> - {:cancel, :forbidden} + {:discard, :forbidden} {:error, :not_found} -> - {:cancel, :not_found} + {:discard, :not_found} {:error, :allowed_depth} -> - {:cancel, :allowed_depth} + {:discard, :allowed_depth} _ -> :error end else - {:cancel, "Unreachable instance"} + {:discard, "Unreachable instance"} end end end diff --git a/test/pleroma/workers/remote_fetcher_worker_test.exs b/test/pleroma/workers/remote_fetcher_worker_test.exs index 84899cc5f..493b10fdc 100644 --- a/test/pleroma/workers/remote_fetcher_worker_test.exs +++ b/test/pleroma/workers/remote_fetcher_worker_test.exs @@ -41,19 +41,19 @@ defmodule Pleroma.Workers.RemoteFetcherWorkerTest do end test "does not requeue a deleted object" do - assert {:cancel, _} = + assert {:discard, _} = RemoteFetcherWorker.perform(%Oban.Job{ args: %{"op" => "fetch_remote", "id" => @deleted_object_one} }) - assert {:cancel, _} = + assert {:discard, _} = RemoteFetcherWorker.perform(%Oban.Job{ args: %{"op" => "fetch_remote", "id" => @deleted_object_two} }) end test "does not requeue an unauthorized object" do - assert {:cancel, _} = + assert {:discard, _} = RemoteFetcherWorker.perform(%Oban.Job{ args: %{"op" => "fetch_remote", "id" => @unauthorized_object} }) @@ -64,7 +64,7 @@ test "does not fetch an unreachable instance" do refute Instances.reachable?(@unreachable_object) - assert {:cancel, _} = + assert {:discard, _} = RemoteFetcherWorker.perform(%Oban.Job{ args: %{"op" => "fetch_remote", "id" => @unreachable_object} }) @@ -73,7 +73,7 @@ test "does not fetch an unreachable instance" do test "does not requeue an object that exceeded depth" do clear_config([:instance, :federation_incoming_replies_max_depth], 0) - assert {:cancel, _} = + assert {:discard, _} = RemoteFetcherWorker.perform(%Oban.Job{ args: %{"op" => "fetch_remote", "id" => @depth_object, "depth" => 1} }) From 2e369aef71a087909846f92784613ef4ebae16c6 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Sun, 14 Jan 2024 13:23:17 -0500 Subject: [PATCH 21/33] Allow the Remote Fetcher to attempt fetching an unreachable instance --- lib/pleroma/workers/remote_fetcher_worker.ex | 27 ++++++++----------- .../workers/remote_fetcher_worker_test.exs | 13 --------- 2 files changed, 11 insertions(+), 29 deletions(-) diff --git a/lib/pleroma/workers/remote_fetcher_worker.ex b/lib/pleroma/workers/remote_fetcher_worker.ex index 618b72a19..844096f9a 100644 --- a/lib/pleroma/workers/remote_fetcher_worker.ex +++ b/lib/pleroma/workers/remote_fetcher_worker.ex @@ -3,32 +3,27 @@ # SPDX-License-Identifier: AGPL-3.0-only defmodule Pleroma.Workers.RemoteFetcherWorker do - alias Pleroma.Instances alias Pleroma.Object.Fetcher use Pleroma.Workers.WorkerHelper, queue: "remote_fetcher" @impl Oban.Worker def perform(%Job{args: %{"op" => "fetch_remote", "id" => id} = args}) do - if Instances.reachable?(id) do - case Fetcher.fetch_object_from_id(id, depth: args["depth"]) do - {:ok, _object} -> - :ok + case Fetcher.fetch_object_from_id(id, depth: args["depth"]) do + {:ok, _object} -> + :ok - {:error, :forbidden} -> - {:discard, :forbidden} + {:error, :forbidden} -> + {:discard, :forbidden} - {:error, :not_found} -> - {:discard, :not_found} + {:error, :not_found} -> + {:discard, :not_found} - {:error, :allowed_depth} -> - {:discard, :allowed_depth} + {:error, :allowed_depth} -> + {:discard, :allowed_depth} - _ -> - :error - end - else - {:discard, "Unreachable instance"} + _ -> + :error end end end diff --git a/test/pleroma/workers/remote_fetcher_worker_test.exs b/test/pleroma/workers/remote_fetcher_worker_test.exs index 493b10fdc..c30e773d4 100644 --- a/test/pleroma/workers/remote_fetcher_worker_test.exs +++ b/test/pleroma/workers/remote_fetcher_worker_test.exs @@ -6,13 +6,11 @@ defmodule Pleroma.Workers.RemoteFetcherWorkerTest do use Pleroma.DataCase use Oban.Testing, repo: Pleroma.Repo - alias Pleroma.Instances alias Pleroma.Workers.RemoteFetcherWorker @deleted_object_one "https://deleted-404.example.com/" @deleted_object_two "https://deleted-410.example.com/" @unauthorized_object "https://unauthorized.example.com/" - @unreachable_object "https://unreachable.example.com/" @depth_object "https://depth.example.com/" describe "RemoteFetcherWorker" do @@ -59,17 +57,6 @@ test "does not requeue an unauthorized object" do }) end - test "does not fetch an unreachable instance" do - Instances.set_consistently_unreachable(@unreachable_object) - - refute Instances.reachable?(@unreachable_object) - - assert {:discard, _} = - RemoteFetcherWorker.perform(%Oban.Job{ - args: %{"op" => "fetch_remote", "id" => @unreachable_object} - }) - end - test "does not requeue an object that exceeded depth" do clear_config([:instance, :federation_incoming_replies_max_depth], 0) From 7f6e35ece41179f7c59c36d5ce7212662f5ca767 Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Fri, 12 Apr 2024 20:33:33 +0100 Subject: [PATCH 22/33] formatting --- test/pleroma/object/fetcher_test.exs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/pleroma/object/fetcher_test.exs b/test/pleroma/object/fetcher_test.exs index 6a919e6ef..1cfd294cb 100644 --- a/test/pleroma/object/fetcher_test.exs +++ b/test/pleroma/object/fetcher_test.exs @@ -59,6 +59,7 @@ defp spoofed_object_with_ids( %{method: :get, url: "https://octodon.social/users/cwebber/statuses/111647596861000656"} -> %Tesla.Env{status: 403} + # Spoof: mismatching ids # Variant 1: Non-exisitng fake id %{ From 49ed27cd960b878595c2b7c8835e9db65b3bbb39 Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Sat, 13 Apr 2024 22:25:31 +0100 Subject: [PATCH 23/33] require logger --- lib/pleroma/web/activity_pub/transmogrifier.ex | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index ccbbba981..d10704a92 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -26,6 +26,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do import Ecto.Query require Pleroma.Constants + require Logger @doc """ Modifies an incoming AP object (mastodon format) to our internal format. @@ -161,10 +162,6 @@ def fix_quote_url(%{"quoteUri" => quote_url} = object, options) object end else - {:quoting?, _} -> - object - - _ -> object end end From 33fb74043df1a3d64d2138cb069d0a36585b7403 Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Sat, 13 Apr 2024 22:56:04 +0100 Subject: [PATCH 24/33] Bring our adjustments into line with atom-failure --- lib/pleroma/collections/fetcher.ex | 5 ++- lib/pleroma/object/fetcher.ex | 33 +++++++++++-------- .../web/activity_pub/transmogrifier_test.exs | 2 +- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/lib/pleroma/collections/fetcher.ex b/lib/pleroma/collections/fetcher.ex index a2fcb7794..9ab883cc2 100644 --- a/lib/pleroma/collections/fetcher.ex +++ b/lib/pleroma/collections/fetcher.ex @@ -68,7 +68,10 @@ defp fetch_page_items(id, items \\ []) do items end else - {:error, {"Object has been deleted", _, _}} -> + {:error, :not_found} -> + items + + {:error, :forbidden} -> items {:error, error} -> diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index 9d2a02db6..97d8ebed5 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -122,7 +122,7 @@ def refetch_object(%Object{data: %{"id" => id}} = object) do {:ok, object} else {:local, true} -> {:ok, object} - {:id, false} -> {:error, "Object id changed on refetch"} + {:id, false} -> {:error, :id_mismatch} e -> {:error, e} end end @@ -136,7 +136,7 @@ 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 - {:scheme, true} <- {: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), @@ -155,8 +155,9 @@ def fetch_object_from_id(id, options \\ []) do log_fetch_error(id, e) {:error, :allowed_depth} - {:scheme, false} -> - {:error, "URI Scheme Invalid"} + {:valid_uri_scheme, _} = e -> + log_fetch_error(id, e) + {:error, :invalid_uri_scheme} {:containment, reason} = e -> log_fetch_error(id, e) @@ -253,7 +254,7 @@ def fetch_and_contain_remote_object_from_id(%{"id" => id}), def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do Logger.debug("Fetching object #{id} via AP") - with {:scheme, true} <- {:scheme, String.starts_with?(id, "http")}, + with {:valid_uri_scheme, true} <- {:valid_uri_scheme, String.starts_with?(id, "http")}, {_, :ok} <- {:local_fetch, Containment.contain_local_fetch(id)}, {:ok, final_id, body} <- get_object(id), {:ok, data} <- safe_json_decode(body), @@ -265,17 +266,21 @@ def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do {:ok, data} else - {:strict_id, _} -> - {:error, "Object's ActivityPub id/url does not match final fetch URL"} + {:strict_id, _} = e-> + log_fetch_error(id, e) + {:error, :id_mismatch} - {:scheme, _} -> - {:error, "Unsupported URI scheme"} + {:valid_uri_scheme, _} = e -> + log_fetch_error(id, e) + {:error, :invalid_uri_scheme} - {:local_fetch, _} -> - {:error, "Trying to fetch local resource"} + {:local_fetch, _} = e -> + log_fetch_error(id, e) + {:error, :local_resource} - {:containment, _} -> - {:error, "Object containment failed."} + {:containment, reason} -> + log_fetch_error(id, reason) + {:error, reason} {:error, e} -> {:error, e} @@ -286,7 +291,7 @@ def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do end def fetch_and_contain_remote_object_from_id(_id), - do: {:error, "id must be a string"} + do: {:error, :invalid_id} defp check_crossdomain_redirect(final_host, original_url) diff --git a/test/pleroma/web/activity_pub/transmogrifier_test.exs b/test/pleroma/web/activity_pub/transmogrifier_test.exs index 164e9f9ad..8b93c38c1 100644 --- a/test/pleroma/web/activity_pub/transmogrifier_test.exs +++ b/test/pleroma/web/activity_pub/transmogrifier_test.exs @@ -570,7 +570,7 @@ test "returns non-modified object" do test "returns nil when cannot normalize object" do assert capture_log(fn -> refute Transmogrifier.get_obj_helper("test-obj-id") - end) =~ "URI Scheme Invalid" + end) =~ ":valid_uri_scheme" end @tag capture_log: true From 18442dcc7e73c02b7f5ffe2526ce9930ad8f5389 Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Sat, 13 Apr 2024 23:05:52 +0100 Subject: [PATCH 25/33] Fix quote test --- test/fixtures/tesla_mock/aimu@misskey.io.json | 64 +++++++++++++++++++ .../tesla_mock/misskey.io_8vs6wxufd0.json | 44 +++++++++++++ .../web/activity_pub/transmogrifier_test.exs | 2 +- test/support/http_request_mock.ex | 20 ++++++ 4 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/tesla_mock/aimu@misskey.io.json create mode 100644 test/fixtures/tesla_mock/misskey.io_8vs6wxufd0.json diff --git a/test/fixtures/tesla_mock/aimu@misskey.io.json b/test/fixtures/tesla_mock/aimu@misskey.io.json new file mode 100644 index 000000000..9ff4cb6d0 --- /dev/null +++ b/test/fixtures/tesla_mock/aimu@misskey.io.json @@ -0,0 +1,64 @@ +{ + "@context": [ + "https://www.w3.org/ns/activitystreams", + "https://w3id.org/security/v1", + { + "manuallyApprovesFollowers": "as:manuallyApprovesFollowers", + "sensitive": "as:sensitive", + "Hashtag": "as:Hashtag", + "quoteUrl": "as:quoteUrl", + "toot": "http://joinmastodon.org/ns#", + "Emoji": "toot:Emoji", + "featured": "toot:featured", + "discoverable": "toot:discoverable", + "schema": "http://schema.org#", + "PropertyValue": "schema:PropertyValue", + "value": "schema:value", + "misskey": "https://misskey.io/ns#", + "_misskey_content": "misskey:_misskey_content", + "_misskey_quote": "misskey:_misskey_quote", + "_misskey_reaction": "misskey:_misskey_reaction", + "_misskey_votes": "misskey:_misskey_votes", + "_misskey_talk": "misskey:_misskey_talk", + "isCat": "misskey:isCat", + "vcard": "http://www.w3.org/2006/vcard/ns#" + } + ], + "type": "Person", + "id": "https://misskey.io/users/83ssedkv53", + "inbox": "https://misskey.io/users/83ssedkv53/inbox", + "outbox": "https://misskey.io/users/83ssedkv53/outbox", + "followers": "https://misskey.io/users/83ssedkv53/followers", + "following": "https://misskey.io/users/83ssedkv53/following", + "sharedInbox": "https://misskey.io/inbox", + "endpoints": { + "sharedInbox": "https://misskey.io/inbox" + }, + "url": "https://misskey.io/@aimu", + "preferredUsername": "aimu", + "name": "あいむ", + "summary": "

わずかな作曲要素 巣穴で独り言
Twitter https://twitter.com/aimu_53
Soundcloud https://soundcloud.com/aimu-53

", + "icon": { + "type": "Image", + "url": "https://s3.arkjp.net/misskey/webpublic-3f7e93c0-34f5-443c-acc0-f415cb2342b4.jpg", + "sensitive": false, + "name": null + }, + "image": { + "type": "Image", + "url": "https://s3.arkjp.net/misskey/webpublic-2db63d1d-490b-488b-ab62-c93c285f26b6.png", + "sensitive": false, + "name": null + }, + "tag": [], + "manuallyApprovesFollowers": false, + "discoverable": true, + "publicKey": { + "id": "https://misskey.io/users/83ssedkv53#main-key", + "type": "Key", + "owner": "https://misskey.io/users/83ssedkv53", + "publicKeyPem": "-----BEGIN PUBLIC KEY-----\nMIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEA1ylhePJ6qGHmwHSBP17b\nIosxGaiFKvgDBgZdm8vzvKeRSqJV9uLHfZL3pO/Zt02EwaZd2GohZAtBZEF8DbMA\n3s93WAesvyGF9mjGrYYKlhp/glwyrrrbf+RdD0DLtyDwRRlrxp3pS2lLmv5Tp1Zl\npH+UKpOnNrpQqjHI5P+lEc9bnflzbRrX+UiyLNsVAP80v4wt7SZfT/telrU6mDru\n998UdfhUo7bDKeDsHG1PfLpyhhtfdoZub4kBpkyacHiwAd+CdCjR54Eu7FDwVK3p\nY3JcrT2q5stgMqN1m4QgSL4XAADIotWwDYttTJejM1n9dr+6VWv5bs0F2Q/6gxOp\nu5DQZLk4Q+64U4LWNox6jCMOq3fYe0g7QalJIHnanYQQo+XjoH6S1Aw64gQ3Ip2Y\nZBmZREAOR7GMFVDPFnVnsbCHnIAv16TdgtLgQBAihkWEUuPqITLi8PMu6kMr3uyq\nYkObEfH0TNTcqaiVpoXv791GZLEUV5ROl0FSUANLNkHZZv29xZ5JDOBOR1rNBLyH\ngVtW8rpszYqOXwzX23hh4WsVXfB7YgNvIijwjiaWbzsecleaENGEnLNMiVKVumTj\nmtyTeFJpH0+OaSrUYpemRRJizmqIjklKsNwUEwUb2WcUUg92o56T2obrBkooabZe\nwgSXSKTOcjsR/ju7+AuIyvkCAwEAAQ==\n-----END PUBLIC KEY-----\n" + }, + "isCat": true, + "vcard:bday": "5353-05-03" +} diff --git a/test/fixtures/tesla_mock/misskey.io_8vs6wxufd0.json b/test/fixtures/tesla_mock/misskey.io_8vs6wxufd0.json new file mode 100644 index 000000000..323ca10ed --- /dev/null +++ b/test/fixtures/tesla_mock/misskey.io_8vs6wxufd0.json @@ -0,0 +1,44 @@ +{ + "@context": [ + "https://www.w3.org/ns/activitystreams", + "https://w3id.org/security/v1", + { + "manuallyApprovesFollowers": "as:manuallyApprovesFollowers", + "sensitive": "as:sensitive", + "Hashtag": "as:Hashtag", + "quoteUrl": "as:quoteUrl", + "toot": "http://joinmastodon.org/ns#", + "Emoji": "toot:Emoji", + "featured": "toot:featured", + "discoverable": "toot:discoverable", + "schema": "http://schema.org#", + "PropertyValue": "schema:PropertyValue", + "value": "schema:value", + "misskey": "https://misskey.io/ns#", + "_misskey_content": "misskey:_misskey_content", + "_misskey_quote": "misskey:_misskey_quote", + "_misskey_reaction": "misskey:_misskey_reaction", + "_misskey_votes": "misskey:_misskey_votes", + "_misskey_talk": "misskey:_misskey_talk", + "isCat": "misskey:isCat", + "vcard": "http://www.w3.org/2006/vcard/ns#" + } + ], + "id": "https://misskey.io/notes/8vs6wxufd0", + "type": "Note", + "attributedTo": "https://misskey.io/users/83ssedkv53", + "summary": null, + "content": "

Fantiaこれできないように過去のやつは従量課金だった気がする

", + "_misskey_content": "Fantiaこれできないように過去のやつは従量課金だった気がする", + "published": "2022-01-21T16:37:12.663Z", + "to": [ + "https://www.w3.org/ns/activitystreams#Public" + ], + "cc": [ + "https://misskey.io/users/83ssedkv53/followers" + ], + "inReplyTo": null, + "attachment": [], + "sensitive": false, + "tag": [] +} diff --git a/test/pleroma/web/activity_pub/transmogrifier_test.exs b/test/pleroma/web/activity_pub/transmogrifier_test.exs index 8b93c38c1..b35f4a04a 100644 --- a/test/pleroma/web/activity_pub/transmogrifier_test.exs +++ b/test/pleroma/web/activity_pub/transmogrifier_test.exs @@ -153,7 +153,7 @@ test "it accepts quote posts" do # Object was created in the database object = Object.normalize(activity) - assert object.data["quoteUrl"] == "https://misskey.io/notes/8vs6wxufd0" + assert object.data["quoteUri"] == "https://misskey.io/notes/8vs6wxufd0" # It fetched the quoted post assert Object.normalize("https://misskey.io/notes/8vs6wxufd0") diff --git a/test/support/http_request_mock.ex b/test/support/http_request_mock.ex index e487d2e6b..4df9b1770 100644 --- a/test/support/http_request_mock.ex +++ b/test/support/http_request_mock.ex @@ -1321,6 +1321,26 @@ def get("https://skippers-bin.com/notes/7x9tmrp97i", _, _, _) do }} end + # A misskey quote + def get("https://misskey.io/notes/8vs6wxufd0", _, _, _) do + {:ok, + %Tesla.Env{ + status: 200, + body: File.read!("test/fixtures/tesla_mock/misskey.io_8vs6wxufd0.json"), + headers: activitypub_object_headers() + }} + end + + def get("https://misskey.io/users/83ssedkv53", _, _, _) do + {:ok, + %Tesla.Env{ + status: 200, + body: File.read!("test/fixtures/tesla_mock/aimu@misskey.io.json"), + headers: activitypub_object_headers() + }} + end + + def get("https://example.org/emoji/firedfox.png", _, _, _) do {:ok, %Tesla.Env{status: 200, body: File.read!("test/fixtures/image.jpg")}} end From 2fc25980d12c4f892cef850100e9d7890fc722b6 Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Sat, 13 Apr 2024 23:55:26 +0100 Subject: [PATCH 26/33] 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 From b7dd739de1ab2b7fab36429724f004dfa7b29440 Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Tue, 16 Apr 2024 02:35:21 +0100 Subject: [PATCH 27/33] Make sure we return the right format for oban --- lib/pleroma/workers/remote_fetcher_worker.ex | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/pleroma/workers/remote_fetcher_worker.ex b/lib/pleroma/workers/remote_fetcher_worker.ex index 1951aec7c..afdf3055b 100644 --- a/lib/pleroma/workers/remote_fetcher_worker.ex +++ b/lib/pleroma/workers/remote_fetcher_worker.ex @@ -34,8 +34,11 @@ def perform(%Job{args: %{"op" => "fetch_remote", "id" => id} = args}) do {:error, :id_mismatch} -> {:discard, :id_mismatch} - _ -> - :error + {:error, _} = e -> + e + + e -> + {:error, e} end end end From 1896ff1ab00e7a9118da0ebe74c2ae0aa765754a Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Tue, 16 Apr 2024 02:35:59 +0100 Subject: [PATCH 28/33] changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e397a75d0..18ed8c8ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Verified support for elixir 1.16 ## Changed +- Inbound pipeline error handing was modified somewhat, which should lead to less incomprehensible log spam. Hopefully. ## Fixed - Issue preventing fetching anything from IPv6-only instances From 50435710843529bf95d7602ad564dbb63f106191 Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Tue, 16 Apr 2024 02:53:24 +0100 Subject: [PATCH 29/33] Enable oban job uniqueness by default just prevent job floods with a 1-seconds uniqueness check, but override in RemoteFetcherWorker for 5 minute uniqueness check over all states :infinity is an option we can go for maybe at some point, but that would prevent any refetches so maybe not idk. --- lib/pleroma/workers/remote_fetcher_worker.ex | 4 +++- lib/pleroma/workers/worker_helper.ex | 6 +++++- mix.exs | 2 +- mix.lock | 2 +- 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/workers/remote_fetcher_worker.ex b/lib/pleroma/workers/remote_fetcher_worker.ex index afdf3055b..8e5824899 100644 --- a/lib/pleroma/workers/remote_fetcher_worker.ex +++ b/lib/pleroma/workers/remote_fetcher_worker.ex @@ -5,7 +5,9 @@ defmodule Pleroma.Workers.RemoteFetcherWorker do alias Pleroma.Object.Fetcher - use Pleroma.Workers.WorkerHelper, queue: "remote_fetcher" + use Pleroma.Workers.WorkerHelper, + queue: "remote_fetcher", + unique: [period: 300, states: Oban.Job.states()] @impl Oban.Worker def perform(%Job{args: %{"op" => "fetch_remote", "id" => id} = args}) do diff --git a/lib/pleroma/workers/worker_helper.ex b/lib/pleroma/workers/worker_helper.ex index 4c0a55774..6d27151de 100644 --- a/lib/pleroma/workers/worker_helper.ex +++ b/lib/pleroma/workers/worker_helper.ex @@ -25,12 +25,16 @@ def sidekiq_backoff(attempt, pow \\ 4, base_backoff \\ 15) do defmacro __using__(opts) do caller_module = __CALLER__.module queue = Keyword.fetch!(opts, :queue) + # by default just stop unintended duplicates - this can and should be overridden + # if you want to have a more complex uniqueness constraint + uniqueness = Keyword.get(opts, :unique, period: 1) quote do # Note: `max_attempts` is intended to be overridden in `new/2` call use Oban.Worker, queue: unquote(queue), - max_attempts: 1 + max_attempts: 1, + unique: unquote(uniqueness) alias Oban.Job diff --git a/mix.exs b/mix.exs index b942c6c23..237503f84 100644 --- a/mix.exs +++ b/mix.exs @@ -125,7 +125,7 @@ defp deps do {:ecto_enum, "~> 1.4"}, {:ecto_sql, "~> 3.10.0"}, {:postgrex, "~> 0.17.2"}, - {:oban, "~> 2.15.2"}, + {:oban, "~> 2.17.8"}, {:gettext, "~> 0.22.3"}, {:bcrypt_elixir, "~> 3.0.1"}, {:fast_sanitize, "~> 0.2.3"}, diff --git a/mix.lock b/mix.lock index c62df06f8..118f17699 100644 --- a/mix.lock +++ b/mix.lock @@ -83,7 +83,7 @@ "nimble_options": {:hex, :nimble_options, "1.1.0", "3b31a57ede9cb1502071fade751ab0c7b8dbe75a9a4c2b5bbb0943a690b63172", [:mix], [], "hexpm", "8bbbb3941af3ca9acc7835f5655ea062111c9c27bcac53e004460dfd19008a99"}, "nimble_parsec": {:hex, :nimble_parsec, "1.4.0", "51f9b613ea62cfa97b25ccc2c1b4216e81df970acd8e16e8d1bdc58fef21370d", [:mix], [], "hexpm", "9c565862810fb383e9838c1dd2d7d2c437b3d13b267414ba6af33e50d2d1cf28"}, "nimble_pool": {:hex, :nimble_pool, "1.1.0", "bf9c29fbdcba3564a8b800d1eeb5a3c58f36e1e11d7b7fb2e084a643f645f06b", [:mix], [], "hexpm", "af2e4e6b34197db81f7aad230c1118eac993acc0dae6bc83bac0126d4ae0813a"}, - "oban": {:hex, :oban, "2.15.4", "d49ab4ffb7153010e32f80fe9e56f592706238149ec579eb50f8a4e41d218856", [:mix], [{:ecto_sql, "~> 3.6", [hex: :ecto_sql, repo: "hexpm", optional: false]}, {:ecto_sqlite3, "~> 0.9", [hex: :ecto_sqlite3, repo: "hexpm", optional: true]}, {:jason, "~> 1.1", [hex: :jason, repo: "hexpm", optional: false]}, {:postgrex, "~> 0.16", [hex: :postgrex, repo: "hexpm", optional: true]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "5fce611fdfffb13e9148df883116e5201adf1e731eb302cc88cde0588510079c"}, + "oban": {:hex, :oban, "2.17.8", "7fd7c8e82c7819afc1b5b5ed8d6d92bf0ecdd7ba170328fb043301eb06d32521", [:mix], [{:ecto_sql, "~> 3.10", [hex: :ecto_sql, repo: "hexpm", optional: false]}, {:ecto_sqlite3, "~> 0.9", [hex: :ecto_sqlite3, repo: "hexpm", optional: true]}, {:jason, "~> 1.1", [hex: :jason, repo: "hexpm", optional: false]}, {:postgrex, "~> 0.16", [hex: :postgrex, repo: "hexpm", optional: true]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "a2165bf93843b7bcb68182c82725ddd4cb43c0c3719f114e7aa3b6c99c4b6129"}, "open_api_spex": {:hex, :open_api_spex, "3.18.3", "fefb84fe323cacfc92afdd0ecb9e89bc0261ae00b7e3167ffc2028ce3944de42", [:mix], [{:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:plug, "~> 1.7", [hex: :plug, repo: "hexpm", optional: false]}, {:poison, "~> 3.0 or ~> 4.0 or ~> 5.0", [hex: :poison, repo: "hexpm", optional: true]}, {:ymlr, "~> 2.0 or ~> 3.0 or ~> 4.0 or ~> 5.0", [hex: :ymlr, repo: "hexpm", optional: true]}], "hexpm", "c0cfc31570199ce7e7520b494a591027da609af45f6bf9adce51e2469b1609fb"}, "parse_trans": {:hex, :parse_trans, "3.4.1", "6e6aa8167cb44cc8f39441d05193be6e6f4e7c2946cb2759f015f8c56b76e5ff", [:rebar3], [], "hexpm", "620a406ce75dada827b82e453c19cf06776be266f5a67cff34e1ef2cbb60e49a"}, "phoenix": {:hex, :phoenix, "1.7.12", "1cc589e0eab99f593a8aa38ec45f15d25297dd6187ee801c8de8947090b5a9d3", [:mix], [{:castore, ">= 0.0.0", [hex: :castore, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:phoenix_pubsub, "~> 2.1", [hex: :phoenix_pubsub, repo: "hexpm", optional: false]}, {:phoenix_template, "~> 1.0", [hex: :phoenix_template, repo: "hexpm", optional: false]}, {:phoenix_view, "~> 2.0", [hex: :phoenix_view, repo: "hexpm", optional: true]}, {:plug, "~> 1.14", [hex: :plug, repo: "hexpm", optional: false]}, {:plug_cowboy, "~> 2.7", [hex: :plug_cowboy, repo: "hexpm", optional: true]}, {:plug_crypto, "~> 1.2 or ~> 2.0", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}, {:websock_adapter, "~> 0.5.3", [hex: :websock_adapter, repo: "hexpm", optional: false]}], "hexpm", "d646192fbade9f485b01bc9920c139bfdd19d0f8df3d73fd8eaf2dfbe0d2837c"}, From d70fa16383071f69ca8e6f4a0c96cfbb4fd48c25 Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Tue, 16 Apr 2024 02:58:50 +0100 Subject: [PATCH 30/33] oban options should be a keyword list --- lib/pleroma/workers/worker_helper.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/workers/worker_helper.ex b/lib/pleroma/workers/worker_helper.ex index 6d27151de..8aef24fa5 100644 --- a/lib/pleroma/workers/worker_helper.ex +++ b/lib/pleroma/workers/worker_helper.ex @@ -27,7 +27,7 @@ defmacro __using__(opts) do queue = Keyword.fetch!(opts, :queue) # by default just stop unintended duplicates - this can and should be overridden # if you want to have a more complex uniqueness constraint - uniqueness = Keyword.get(opts, :unique, period: 1) + uniqueness = Keyword.get(opts, :unique, [period: 1]) quote do # Note: `max_attempts` is intended to be overridden in `new/2` call From d2cee15c15052d5e933c8dea5e238d5440621b3d Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Tue, 16 Apr 2024 03:07:28 +0100 Subject: [PATCH 31/33] mix format says no --- lib/pleroma/workers/worker_helper.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/workers/worker_helper.ex b/lib/pleroma/workers/worker_helper.ex index 8aef24fa5..6d27151de 100644 --- a/lib/pleroma/workers/worker_helper.ex +++ b/lib/pleroma/workers/worker_helper.ex @@ -27,7 +27,7 @@ defmacro __using__(opts) do queue = Keyword.fetch!(opts, :queue) # by default just stop unintended duplicates - this can and should be overridden # if you want to have a more complex uniqueness constraint - uniqueness = Keyword.get(opts, :unique, [period: 1]) + uniqueness = Keyword.get(opts, :unique, period: 1) quote do # Note: `max_attempts` is intended to be overridden in `new/2` call From b2c29527fb628b915736e33c27f150a4d16e17b0 Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Tue, 16 Apr 2024 10:19:30 +0100 Subject: [PATCH 32/33] make xmerl shut up about markup --- lib/pleroma/web/xml.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/web/xml.ex b/lib/pleroma/web/xml.ex index e68341e20..7fe7730ea 100644 --- a/lib/pleroma/web/xml.ex +++ b/lib/pleroma/web/xml.ex @@ -26,7 +26,7 @@ def string_from_xpath(xpath, doc) do def parse_document(text) do try do - doc = SweetXml.parse(text, dtd: :none) + doc = SweetXml.parse(text, dtd: :none, quiet: true) {:ok, doc} rescue From 370576474c790ea281f81ee899c0ebb15ab21232 Mon Sep 17 00:00:00 2001 From: Floatingghost Date: Fri, 19 Apr 2024 11:39:27 +0100 Subject: [PATCH 33/33] only consider :op and :id args in duplicate checks --- lib/pleroma/workers/remote_fetcher_worker.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/workers/remote_fetcher_worker.ex b/lib/pleroma/workers/remote_fetcher_worker.ex index 8e5824899..1bc0e5d0c 100644 --- a/lib/pleroma/workers/remote_fetcher_worker.ex +++ b/lib/pleroma/workers/remote_fetcher_worker.ex @@ -7,7 +7,7 @@ defmodule Pleroma.Workers.RemoteFetcherWorker do use Pleroma.Workers.WorkerHelper, queue: "remote_fetcher", - unique: [period: 300, states: Oban.Job.states()] + unique: [period: 300, states: Oban.Job.states(), keys: [:op, :id]] @impl Oban.Worker def perform(%Job{args: %{"op" => "fetch_remote", "id" => id} = args}) do