From 00f4c0b02cbd62d058038c17f86ab2c1e5e67130 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Wed, 20 Apr 2022 11:33:13 +0200 Subject: [PATCH] Make sure remote Update activities can't affect local actors other than Groups Signed-off-by: Thomas Citharel --- lib/federation/activity_pub/actions/update.ex | 2 +- lib/federation/activity_pub/transmogrifier.ex | 7 ++++ lib/federation/activity_pub/types/actors.ex | 35 +++++++++++-------- .../activity_pub/activity_pub_test.exs | 2 +- .../transmogrifier/update_test.exs | 26 +++++++++++++- 5 files changed, 54 insertions(+), 18 deletions(-) diff --git a/lib/federation/activity_pub/actions/update.ex b/lib/federation/activity_pub/actions/update.ex index 1306934b6..9c0f860ac 100644 --- a/lib/federation/activity_pub/actions/update.ex +++ b/lib/federation/activity_pub/actions/update.ex @@ -28,7 +28,7 @@ defmodule Mobilizon.Federation.ActivityPub.Actions.Update do Logger.debug("updating an activity") Logger.debug(inspect(args)) - case Managable.update(old_entity, args, additional) do + case Managable.update(old_entity, args, Map.put(additional, :local, local)) do {:ok, entity, update_data} -> {:ok, activity} = create_activity(update_data, local) maybe_federate(activity) diff --git a/lib/federation/activity_pub/transmogrifier.ex b/lib/federation/activity_pub/transmogrifier.ex index 2d904ad1a..bac97bee6 100644 --- a/lib/federation/activity_pub/transmogrifier.ex +++ b/lib/federation/activity_pub/transmogrifier.ex @@ -407,6 +407,13 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier do Actions.Update.update(old_actor, object_data, false, %{updater_actor: author}) do {:ok, activity, new_actor} else + {:error, :update_not_allowed} -> + Logger.warn("Activity tried to update an actor that's local or not a group", + activity: params + ) + + :error + e -> Sentry.capture_message("Error while handling an Update activity", extra: %{params: params} diff --git a/lib/federation/activity_pub/types/actors.ex b/lib/federation/activity_pub/types/actors.ex index 99fdd859c..48aa34fd6 100644 --- a/lib/federation/activity_pub/types/actors.ex +++ b/lib/federation/activity_pub/types/actors.ex @@ -45,25 +45,30 @@ defmodule Mobilizon.Federation.ActivityPub.Types.Actors do def update(%Actor{} = old_actor, args, additional) do updater_actor = Map.get(args, :updater_actor) || Map.get(additional, :updater_actor) - case Actors.update_actor(old_actor, args) do - {:ok, %Actor{} = new_actor} -> - GroupActivity.insert_activity(new_actor, - subject: "group_updated", - old_group: old_actor, - updater_actor: updater_actor - ) + if Map.get(additional, :local, false) == true or not match?(%Actor{domain: nil}, old_actor) or + match?(%Actor{type: :Group}, old_actor) do + case Actors.update_actor(old_actor, args) do + {:ok, %Actor{} = new_actor} -> + GroupActivity.insert_activity(new_actor, + subject: "group_updated", + old_group: old_actor, + updater_actor: updater_actor + ) - actor_as_data = Convertible.model_to_as(new_actor) - Cachex.del(:activity_pub, "actor_#{new_actor.preferred_username}") - audience = Audience.get_audience(new_actor) + actor_as_data = Convertible.model_to_as(new_actor) + Cachex.del(:activity_pub, "actor_#{new_actor.preferred_username}") + audience = Audience.get_audience(new_actor) - additional = Map.merge(additional, %{"actor" => (updater_actor || old_actor).url}) + additional = Map.merge(additional, %{"actor" => (updater_actor || old_actor).url}) - update_data = make_update_data(actor_as_data, Map.merge(audience, additional)) - {:ok, new_actor, update_data} + update_data = make_update_data(actor_as_data, Map.merge(audience, additional)) + {:ok, new_actor, update_data} - {:error, %Ecto.Changeset{} = err} -> - {:error, err} + {:error, %Ecto.Changeset{} = err} -> + {:error, err} + end + else + {:error, :update_not_allowed} end end diff --git a/test/federation/activity_pub/activity_pub_test.exs b/test/federation/activity_pub/activity_pub_test.exs index 80f452142..70e7ce8d5 100644 --- a/test/federation/activity_pub/activity_pub_test.exs +++ b/test/federation/activity_pub/activity_pub_test.exs @@ -195,7 +195,7 @@ defmodule Mobilizon.Federation.ActivityPubTest do actor = insert(:actor) actor_data = %{summary: @updated_actor_summary} - {:ok, update, _} = Actions.Update.update(actor, actor_data, false) + {:ok, update, _} = Actions.Update.update(actor, actor_data, true) assert update.data["actor"] == actor.url assert update.data["to"] == [@activity_pub_public_audience] diff --git a/test/federation/activity_pub/transmogrifier/update_test.exs b/test/federation/activity_pub/transmogrifier/update_test.exs index a5724ff60..b8411df20 100644 --- a/test/federation/activity_pub/transmogrifier/update_test.exs +++ b/test/federation/activity_pub/transmogrifier/update_test.exs @@ -3,12 +3,13 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier.UpdateTest do use Oban.Testing, repo: Mobilizon.Storage.Repo import Mobilizon.Factory import Mox + import ExUnit.CaptureLog alias Mobilizon.{Actors, Events, Posts} alias Mobilizon.Actors.{Actor, Member} alias Mobilizon.Events.Event alias Mobilizon.Posts.Post - alias Mobilizon.Federation.ActivityPub.{Activity, Transmogrifier} + alias Mobilizon.Federation.ActivityPub.{Activity, Relay, Transmogrifier} alias Mobilizon.Federation.ActivityStream.Convertible alias Mobilizon.Service.HTTP.ActivityPub.Mock @@ -50,6 +51,29 @@ defmodule Mobilizon.Federation.ActivityPub.Transmogrifier.UpdateTest do assert actor.summary == "

Some bio

" end + test "it fails for incoming update activies on local actors" do + %Actor{url: relay_actor_url} = Relay.get_actor() + + update_data = File.read!("test/fixtures/mastodon-update.json") |> Jason.decode!() + + object = + update_data["object"] + |> Map.put("actor", relay_actor_url) + |> Map.put("id", relay_actor_url) + + update_data = + update_data + |> Map.put("actor", relay_actor_url) + |> Map.put("object", object) + + assert capture_log([level: :warn], fn -> + :error = Transmogrifier.handle_incoming(update_data) + end) =~ "[warning] Activity tried to update an actor that's local or not a group" + + {:ok, %Actor{keys: keys}} = Actors.get_actor_by_url(relay_actor_url) + assert Regex.match?(~r/BEGIN RSA PRIVATE KEY/, keys) + end + test "it works for incoming update activities on events" do data = File.read!("test/fixtures/mobilizon-post-activity.json") |> Jason.decode!()