From ceffb8a8918b83d482e9c1da64fec22b428a61f3 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 23 Aug 2024 13:52:19 -0400 Subject: [PATCH 01/16] Drop incoming Delete activities from unknown actors --- changelog.d/drop-unknown-deletes.change | 1 + lib/pleroma/workers/receiver_worker.ex | 16 +++++++++++++- test/pleroma/workers/receiver_worker_test.exs | 22 +++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 changelog.d/drop-unknown-deletes.change diff --git a/changelog.d/drop-unknown-deletes.change b/changelog.d/drop-unknown-deletes.change new file mode 100644 index 0000000000..0be7f54502 --- /dev/null +++ b/changelog.d/drop-unknown-deletes.change @@ -0,0 +1 @@ +Drop incoming Delete activities from unknown actors diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index d4db97b639..ea86a3a12e 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -33,7 +33,8 @@ def perform(%Job{ query_string: query_string } - with {:ok, %User{} = _actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), + with {_, false} <- {:unknown_delete, unknown_delete?(params)}, + User.get_or_fetch_by_ap_id(conn_data.params["actor"]), {:ok, _public_key} <- Signature.refetch_public_key(conn_data), {:signature, true} <- {:signature, Signature.validate_signature(conn_data)}, {:ok, res} <- Federator.perform(:incoming_ap_doc, params) do @@ -58,6 +59,7 @@ def timeout(_job), do: :timer.seconds(5) defp process_errors(errors) do case errors do + {:unknown_delete, true} -> {:cancel, "Delete from unknown actor"} {:error, :origin_containment_failed} -> {:cancel, :origin_containment_failed} {:error, :already_present} -> {:cancel, :already_present} {:error, {:validate_object, _} = reason} -> {:cancel, reason} @@ -71,4 +73,16 @@ defp process_errors(errors) do e -> {:error, e} end end + + defp unknown_delete?(%{ + "type" => "Delete", + "actor" => actor + }) do + case User.get_cached_by_ap_id(actor) do + %User{} -> false + _ -> true + end + end + + defp unknown_delete?(_), do: false end diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index 33be910853..91fbb1fe88 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -245,4 +245,26 @@ test "it can validate the signature" do assert {:ok, %Pleroma.Activity{}} = ReceiverWorker.perform(oban_job) end + + # When activity is delivered to the inbox and we cannot immediately verify signature + # we capture all the params and process it later in the Oban job. + # This requires we replicate the same scenario by including additional fields in the params + test "Deletes cancelled for an unknown actor" do + params = %{ + "type" => "Delete", + "actor" => "https://unknown.mastodon.instance/users/somebody" + } + + assert {:cancel, "Delete from unknown actor"} = + ReceiverWorker.perform(%Oban.Job{ + args: %{ + "op" => "incoming_ap_doc", + "method" => :post, + "req_headers" => [], + "request_path" => "/inbox", + "query_string" => "", + "params" => params + } + }) + end end From 4bc6f334f49c27e1f466052fee6fa2f3d5d2ee74 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 23 Aug 2024 14:18:04 -0400 Subject: [PATCH 02/16] Revert unintentional change --- lib/pleroma/workers/receiver_worker.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index ea86a3a12e..4033fec0fb 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -34,7 +34,7 @@ def perform(%Job{ } with {_, false} <- {:unknown_delete, unknown_delete?(params)}, - User.get_or_fetch_by_ap_id(conn_data.params["actor"]), + {:ok, %User{} = _actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), {:ok, _public_key} <- Signature.refetch_public_key(conn_data), {:signature, true} <- {:signature, Signature.validate_signature(conn_data)}, {:ok, res} <- Federator.perform(:incoming_ap_doc, params) do From 1c394dd18c5d61dc716a9b9cda981a359de32456 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 23 Aug 2024 14:24:04 -0400 Subject: [PATCH 03/16] Move the check to the inbox --- .../activity_pub/activity_pub_controller.ex | 32 +++++++++++++++---- lib/pleroma/workers/receiver_worker.ex | 15 +-------- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index cdd054e1a3..a24dcfc9c9 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -294,13 +294,19 @@ def inbox(%{assigns: %{valid_signature: true}} = conn, params) do end def inbox(%{assigns: %{valid_signature: false}} = conn, params) do - Federator.incoming_ap_doc(%{ - method: conn.method, - req_headers: conn.req_headers, - request_path: conn.request_path, - params: params, - query_string: conn.query_string - }) + case unknown_delete?(params) do + true -> + :ok + + false -> + Federator.incoming_ap_doc(%{ + method: conn.method, + req_headers: conn.req_headers, + request_path: conn.request_path, + params: params, + query_string: conn.query_string + }) + end json(conn, "ok") end @@ -558,4 +564,16 @@ def pinned(conn, %{"nickname" => nickname}) do |> json(UserView.render("featured.json", %{user: user})) end end + + defp unknown_delete?(%{ + "type" => "Delete", + "actor" => actor + }) do + case User.get_cached_by_ap_id(actor) do + %User{} -> false + _ -> true + end + end + + defp unknown_delete?(_), do: false end diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index 4033fec0fb..ce92ef9ddf 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -33,8 +33,7 @@ def perform(%Job{ query_string: query_string } - with {_, false} <- {:unknown_delete, unknown_delete?(params)}, - {:ok, %User{} = _actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), + with {:ok, %User{} = _actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), {:ok, _public_key} <- Signature.refetch_public_key(conn_data), {:signature, true} <- {:signature, Signature.validate_signature(conn_data)}, {:ok, res} <- Federator.perform(:incoming_ap_doc, params) do @@ -73,16 +72,4 @@ defp process_errors(errors) do e -> {:error, e} end end - - defp unknown_delete?(%{ - "type" => "Delete", - "actor" => actor - }) do - case User.get_cached_by_ap_id(actor) do - %User{} -> false - _ -> true - end - end - - defp unknown_delete?(_), do: false end From 27fcc421719062d5de9bf4dc90f3349595eb278d Mon Sep 17 00:00:00 2001 From: feld Date: Sat, 24 Aug 2024 16:53:22 +0000 Subject: [PATCH 04/16] Use Pleroma.Object.Containment.get_actor/1 to reliably find the actor of an incoming activity or object --- lib/pleroma/web/activity_pub/activity_pub_controller.ex | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index a24dcfc9c9..77ec26f148 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -567,9 +567,8 @@ def pinned(conn, %{"nickname" => nickname}) do defp unknown_delete?(%{ "type" => "Delete", - "actor" => actor - }) do - case User.get_cached_by_ap_id(actor) do + } = data) do + case data |> Pleroma.Object.Containment.get_actor() |> User.get_cached_by_ap_id() do %User{} -> false _ -> true end From 7bcc21ad6f1fdf9dbc16990e9891f9de7a21011d Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Sat, 24 Aug 2024 13:01:28 -0400 Subject: [PATCH 05/16] Switch test to the inbox --- .../activity_pub_controller_test.exs | 21 ++++++++++++++++++ test/pleroma/workers/receiver_worker_test.exs | 22 ------------------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index af1a32fed8..b9067533c4 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -684,6 +684,27 @@ test "without valid signature, " <> |> json_response(400) end + # When activity is delivered to the inbox and we cannot immediately verify signature + # we capture all the params and process it later in the Oban job. + # Once we begin processing it through Oban we risk fetching the actor to validate the + # activity which just leads to inserting a new user to process a Delete not relevant to us. + test "Deletes from an unknown actor are discarded", %{conn: conn} do + params = + %{ + "type" => "Delete", + "actor" => "https://unknown.mastodon.instance/users/somebody" + } + |> Jason.encode!() + + conn + |> assign(:valid_signature, false) + |> put_req_header("content-type", "application/activity+json") + |> post("/inbox", params) + |> json_response(200) + + assert all_enqueued() == [] + end + test "accepts Add/Remove activities", %{conn: conn} do object_id = "c61d6733-e256-4fe1-ab13-1e369789423f" diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index 91fbb1fe88..33be910853 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -245,26 +245,4 @@ test "it can validate the signature" do assert {:ok, %Pleroma.Activity{}} = ReceiverWorker.perform(oban_job) end - - # When activity is delivered to the inbox and we cannot immediately verify signature - # we capture all the params and process it later in the Oban job. - # This requires we replicate the same scenario by including additional fields in the params - test "Deletes cancelled for an unknown actor" do - params = %{ - "type" => "Delete", - "actor" => "https://unknown.mastodon.instance/users/somebody" - } - - assert {:cancel, "Delete from unknown actor"} = - ReceiverWorker.perform(%Oban.Job{ - args: %{ - "op" => "incoming_ap_doc", - "method" => :post, - "req_headers" => [], - "request_path" => "/inbox", - "query_string" => "", - "params" => params - } - }) - end end From 06deacd58e6aa676847530f24c6799162ed06777 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Sat, 24 Aug 2024 20:03:50 -0400 Subject: [PATCH 06/16] Formatting --- lib/pleroma/web/activity_pub/activity_pub_controller.ex | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index 77ec26f148..4c83d19274 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -565,9 +565,11 @@ def pinned(conn, %{"nickname" => nickname}) do end end - defp unknown_delete?(%{ - "type" => "Delete", - } = data) do + defp unknown_delete?( + %{ + "type" => "Delete" + } = data + ) do case data |> Pleroma.Object.Containment.get_actor() |> User.get_cached_by_ap_id() do %User{} -> false _ -> true From 16a9b34876f3a9289c02253e802bffdac4901ac0 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 14:16:59 -0400 Subject: [PATCH 07/16] Convert to an Plug called InboxGuard --- lib/pleroma/constants.ex | 12 ++++ .../activity_pub/activity_pub_controller.ex | 33 ++-------- lib/pleroma/web/plugs/inbox_guard_plug.ex | 66 +++++++++++++++++++ lib/pleroma/web/router.ex | 6 +- .../activity_pub_controller_test.exs | 2 +- 5 files changed, 91 insertions(+), 28 deletions(-) create mode 100644 lib/pleroma/web/plugs/inbox_guard_plug.ex diff --git a/lib/pleroma/constants.ex b/lib/pleroma/constants.ex index 3a5e353012..f969bfdbb0 100644 --- a/lib/pleroma/constants.ex +++ b/lib/pleroma/constants.ex @@ -85,6 +85,18 @@ defmodule Pleroma.Constants do ] ) + const(allowed_activity_types_from_strangers, + do: [ + "Block", + "Create", + "Flag", + "Follow", + "Like", + "Move", + "React" + ] + ) + # basic regex, just there to weed out potential mistakes # https://datatracker.ietf.org/doc/html/rfc2045#section-5.1 const(mime_regex, diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index 4c83d19274..cdd054e1a3 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -294,19 +294,13 @@ def inbox(%{assigns: %{valid_signature: true}} = conn, params) do end def inbox(%{assigns: %{valid_signature: false}} = conn, params) do - case unknown_delete?(params) do - true -> - :ok - - false -> - Federator.incoming_ap_doc(%{ - method: conn.method, - req_headers: conn.req_headers, - request_path: conn.request_path, - params: params, - query_string: conn.query_string - }) - end + Federator.incoming_ap_doc(%{ + method: conn.method, + req_headers: conn.req_headers, + request_path: conn.request_path, + params: params, + query_string: conn.query_string + }) json(conn, "ok") end @@ -564,17 +558,4 @@ def pinned(conn, %{"nickname" => nickname}) do |> json(UserView.render("featured.json", %{user: user})) end end - - defp unknown_delete?( - %{ - "type" => "Delete" - } = data - ) do - case data |> Pleroma.Object.Containment.get_actor() |> User.get_cached_by_ap_id() do - %User{} -> false - _ -> true - end - end - - defp unknown_delete?(_), do: false end diff --git a/lib/pleroma/web/plugs/inbox_guard_plug.ex b/lib/pleroma/web/plugs/inbox_guard_plug.ex new file mode 100644 index 0000000000..643b586d45 --- /dev/null +++ b/lib/pleroma/web/plugs/inbox_guard_plug.ex @@ -0,0 +1,66 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2022 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Web.Plugs.InboxGuardPlug do + import Plug.Conn + import Pleroma.Constants, only: [allowed_activity_types_from_strangers: 0] + + alias Pleroma.Config + alias Pleroma.User + + def init(options) do + options + end + + def call(%{assigns: %{valid_signature: true}} = conn, _opts) do + conn + end + + def call(conn, _opts) do + with {_, true} <- {:federating, Config.get!([:instance, :federating])}, + true <- known_actor?(conn) do + conn + else + {:federating, false} -> + conn + |> json(403, "Not federating") + + _ -> + conn + |> filter_from_strangers() + end + end + + # If signature failed but we know this actor we should + # accept it as we may only need to refetch their public key + # during processing + defp known_actor?(%{body_params: data}) do + case Pleroma.Object.Containment.get_actor(data) |> User.get_cached_by_ap_id() do + %User{} -> true + _ -> false + end + end + + # Only permit a subset of activity types from strangers + # or else it will add actors you've never interacted with + # to the database + defp filter_from_strangers(%{body_params: %{"type" => type}} = conn) do + with true <- type in allowed_activity_types_from_strangers() do + conn + else + _ -> + conn + |> json(400, "Invalid activity type for an unknown actor") + end + end + + defp json(conn, status, resp) do + json_resp = Jason.encode!(resp) + + conn + |> put_resp_content_type("application/json") + |> resp(status, json_resp) + |> halt() + end +end diff --git a/lib/pleroma/web/router.ex b/lib/pleroma/web/router.ex index 6492e38619..9b9ee421cf 100644 --- a/lib/pleroma/web/router.ex +++ b/lib/pleroma/web/router.ex @@ -217,6 +217,10 @@ defmodule Pleroma.Web.Router do plug(Pleroma.Web.Plugs.MappedSignatureToIdentityPlug) end + pipeline :inbox_guard do + plug(Pleroma.Web.Plugs.InboxGuardPlug) + end + pipeline :static_fe do plug(Pleroma.Web.Plugs.StaticFEPlug) end @@ -920,7 +924,7 @@ defmodule Pleroma.Web.Router do end scope "/", Pleroma.Web.ActivityPub do - pipe_through(:activitypub) + pipe_through([:activitypub, :inbox_guard]) post("/inbox", ActivityPubController, :inbox) post("/users/:nickname/inbox", ActivityPubController, :inbox) end diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index b9067533c4..1b959f3245 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -700,7 +700,7 @@ test "Deletes from an unknown actor are discarded", %{conn: conn} do |> assign(:valid_signature, false) |> put_req_header("content-type", "application/activity+json") |> post("/inbox", params) - |> json_response(200) + |> json_response(400) assert all_enqueued() == [] end From e2cdae2c88eb22588209923d83c2a9c52d16c48c Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 14:23:07 -0400 Subject: [PATCH 08/16] Change relay inbox response when not federating to a 403 for consistency --- lib/pleroma/web/activity_pub/activity_pub_controller.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index cdd054e1a3..a08eda5f47 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -311,7 +311,7 @@ def inbox(conn, %{"type" => "Create"} = params) do post_inbox_relayed_create(conn, params) else conn - |> put_status(:bad_request) + |> put_status(403) |> json("Not federating") end end From 990b2058df11cc32f260d0ffcf7dd0f342d435b4 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 14:30:23 -0400 Subject: [PATCH 09/16] Remove unnecessary error match in ReceiverWorker --- lib/pleroma/workers/receiver_worker.ex | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index ce92ef9ddf..d4db97b639 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -58,7 +58,6 @@ def timeout(_job), do: :timer.seconds(5) defp process_errors(errors) do case errors do - {:unknown_delete, true} -> {:cancel, "Delete from unknown actor"} {:error, :origin_containment_failed} -> {:cancel, :origin_containment_failed} {:error, :already_present} -> {:cancel, :already_present} {:error, {:validate_object, _} = reason} -> {:cancel, reason} From 2b39956acbc3ccd87a43cd4ddbd5976adcac5936 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 14:40:31 -0400 Subject: [PATCH 10/16] Fix test title to be more specific as it has a broader but incorrect meaning --- test/pleroma/web/activity_pub/activity_pub_controller_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index 1b959f3245..762fca0a1f 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -657,7 +657,7 @@ test "accept follow activity", %{conn: conn} do end test "without valid signature, " <> - "it only accepts Create activities and requires enabled federation", + "it accepts Create activities and requires enabled federation", %{conn: conn} do data = File.read!("test/fixtures/mastodon-post-activity.json") |> Jason.decode!() non_create_data = File.read!("test/fixtures/mastodon-announce.json") |> Jason.decode!() From 012132303f79c0d693a8fba7236433443261b757 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 14:40:45 -0400 Subject: [PATCH 11/16] Test more types we do not want to receive from strangers --- .../activity_pub_controller_test.exs | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index 762fca0a1f..453dbaf0cd 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -688,21 +688,25 @@ test "without valid signature, " <> # we capture all the params and process it later in the Oban job. # Once we begin processing it through Oban we risk fetching the actor to validate the # activity which just leads to inserting a new user to process a Delete not relevant to us. - test "Deletes from an unknown actor are discarded", %{conn: conn} do - params = - %{ - "type" => "Delete", - "actor" => "https://unknown.mastodon.instance/users/somebody" - } - |> Jason.encode!() + test "Activities of certain types from an unknown actor are discarded", %{conn: conn} do + example_bad_types = ["Announce", "Delete", "Undo"] - conn - |> assign(:valid_signature, false) - |> put_req_header("content-type", "application/activity+json") - |> post("/inbox", params) - |> json_response(400) + Enum.each(example_bad_types, fn bad_type -> + params = + %{ + "type" => bad_type, + "actor" => "https://unknown.mastodon.instance/users/somebody" + } + |> Jason.encode!() - assert all_enqueued() == [] + conn + |> assign(:valid_signature, false) + |> put_req_header("content-type", "application/activity+json") + |> post("/inbox", params) + |> json_response(400) + + assert all_enqueued() == [] + end) end test "accepts Add/Remove activities", %{conn: conn} do From 094da5d6343507eb1540f0a6357accc67573e02e Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 29 Aug 2024 14:44:52 -0400 Subject: [PATCH 12/16] Update changelog --- changelog.d/drop-unknown-deletes.change | 1 - changelog.d/drop-unwanted.change | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 changelog.d/drop-unknown-deletes.change create mode 100644 changelog.d/drop-unwanted.change diff --git a/changelog.d/drop-unknown-deletes.change b/changelog.d/drop-unknown-deletes.change deleted file mode 100644 index 0be7f54502..0000000000 --- a/changelog.d/drop-unknown-deletes.change +++ /dev/null @@ -1 +0,0 @@ -Drop incoming Delete activities from unknown actors diff --git a/changelog.d/drop-unwanted.change b/changelog.d/drop-unwanted.change new file mode 100644 index 0000000000..59447d68f6 --- /dev/null +++ b/changelog.d/drop-unwanted.change @@ -0,0 +1 @@ +Restrict incoming activities from unknown actors to a subset that does not imply a previous relationship From 5205e846eb5cfbd0adfa5031ad75e96fccbc86d8 Mon Sep 17 00:00:00 2001 From: feld Date: Fri, 30 Aug 2024 13:14:05 +0000 Subject: [PATCH 13/16] Update allowed activity types from strangers Move is emitted from the old account EmojiReact is ~ Like Announced TBD --- lib/pleroma/constants.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/pleroma/constants.ex b/lib/pleroma/constants.ex index f969bfdbb0..bbd1836831 100644 --- a/lib/pleroma/constants.ex +++ b/lib/pleroma/constants.ex @@ -92,8 +92,8 @@ defmodule Pleroma.Constants do "Flag", "Follow", "Like", - "Move", - "React" + "EmojiReact", + "Announce" ] ) From e38f5f1a817d6da30e9a128ec74a2a7c78faf174 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 30 Aug 2024 09:35:04 -0400 Subject: [PATCH 14/16] Add recognized activity types to a constant and use it in the test --- lib/pleroma/constants.ex | 18 ++++++++++++++++++ .../activity_pub_controller_test.exs | 4 +++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/constants.ex b/lib/pleroma/constants.ex index bbd1836831..5268ebe7a2 100644 --- a/lib/pleroma/constants.ex +++ b/lib/pleroma/constants.ex @@ -85,6 +85,24 @@ defmodule Pleroma.Constants do ] ) + const(activity_types, + do: [ + "Create", + "Update", + "Delete", + "Follow", + "Accept", + "Reject", + "Add", + "Remove", + "Like", + "Announce", + "Undo", + "Flag", + "EmojiReact" + ] + ) + const(allowed_activity_types_from_strangers, do: [ "Block", diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index 453dbaf0cd..c32f6c1a37 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -689,7 +689,9 @@ test "without valid signature, " <> # Once we begin processing it through Oban we risk fetching the actor to validate the # activity which just leads to inserting a new user to process a Delete not relevant to us. test "Activities of certain types from an unknown actor are discarded", %{conn: conn} do - example_bad_types = ["Announce", "Delete", "Undo"] + example_bad_types = + Pleroma.Constants.activity_types() -- + Pleroma.Constants.allowed_activity_types_from_strangers() Enum.each(example_bad_types, fn bad_type -> params = From 11ee94ae17094a2bc33505a31671b8c705f768a4 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 30 Aug 2024 09:46:10 -0400 Subject: [PATCH 15/16] InboxGuardPlug: Add early rejection of unknown activity types --- lib/pleroma/web/plugs/inbox_guard_plug.ex | 31 ++++++++++++++++--- .../activity_pub_controller_test.exs | 21 +++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/web/plugs/inbox_guard_plug.ex b/lib/pleroma/web/plugs/inbox_guard_plug.ex index 643b586d45..0064cce766 100644 --- a/lib/pleroma/web/plugs/inbox_guard_plug.ex +++ b/lib/pleroma/web/plugs/inbox_guard_plug.ex @@ -4,7 +4,7 @@ defmodule Pleroma.Web.Plugs.InboxGuardPlug do import Plug.Conn - import Pleroma.Constants, only: [allowed_activity_types_from_strangers: 0] + import Pleroma.Constants, only: [activity_types: 0, allowed_activity_types_from_strangers: 0] alias Pleroma.Config alias Pleroma.User @@ -14,24 +14,46 @@ def init(options) do end def call(%{assigns: %{valid_signature: true}} = conn, _opts) do - conn + with {_, true} <- {:federating, Config.get!([:instance, :federating])} do + conn + |> filter_activity_types() + else + {:federating, false} -> + conn + |> json(403, "Not federating") + |> halt() + end end def call(conn, _opts) do with {_, true} <- {:federating, Config.get!([:instance, :federating])}, - true <- known_actor?(conn) do + conn = filter_activity_types(conn), + {:known, true} <- {:known, known_actor?(conn)} do conn else {:federating, false} -> conn |> json(403, "Not federating") + |> halt() - _ -> + {:known, false} -> conn |> filter_from_strangers() end end + # Early rejection of unrecognized types + defp filter_activity_types(%{body_params: %{"type" => type}} = conn) do + with true <- type in activity_types() do + conn + else + _ -> + conn + |> json(400, "Invalid activity type") + |> halt() + end + end + # If signature failed but we know this actor we should # accept it as we may only need to refetch their public key # during processing @@ -52,6 +74,7 @@ defp filter_from_strangers(%{body_params: %{"type" => type}} = conn) do _ -> conn |> json(400, "Invalid activity type for an unknown actor") + |> halt() end end diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index c32f6c1a37..3bd589f490 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -711,6 +711,27 @@ test "Activities of certain types from an unknown actor are discarded", %{conn: end) end + test "Unknown activity types are discarded", %{conn: conn} do + unknown_types = ["Poke", "Read", "Dazzle"] + + Enum.each(unknown_types, fn bad_type -> + params = + %{ + "type" => bad_type, + "actor" => "https://unknown.mastodon.instance/users/somebody" + } + |> Jason.encode!() + + conn + |> assign(:valid_signature, true) + |> put_req_header("content-type", "application/activity+json") + |> post("/inbox", params) + |> json_response(400) + + assert all_enqueued() == [] + end) + end + test "accepts Add/Remove activities", %{conn: conn} do object_id = "c61d6733-e256-4fe1-ab13-1e369789423f" From bb235f913fb88f925abc791285808afe63d14bca Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Fri, 30 Aug 2024 10:03:51 -0400 Subject: [PATCH 16/16] Update changelog --- changelog.d/drop-unwanted.change | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/drop-unwanted.change b/changelog.d/drop-unwanted.change index 59447d68f6..459d4bfe64 100644 --- a/changelog.d/drop-unwanted.change +++ b/changelog.d/drop-unwanted.change @@ -1 +1 @@ -Restrict incoming activities from unknown actors to a subset that does not imply a previous relationship +Restrict incoming activities from unknown actors to a subset that does not imply a previous relationship and early rejection of unrecognized activity types.