Make sure remote Update activities can't affect local actors other than

Groups

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

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)

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}

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
+    if Map.get(additional, :local, false) == true or not match?(%Actor{domain: nil}, old_actor) or
-      {:ok, %Actor{} = new_actor} ->
+         match?(%Actor{type: :Group}, old_actor) do
-        GroupActivity.insert_activity(new_actor,
+      case Actors.update_actor(old_actor, args) do
-          subject: "group_updated",
+        {:ok, %Actor{} = new_actor} ->
-          old_group: old_actor,
+          GroupActivity.insert_activity(new_actor,
-          updater_actor: updater_actor
+            subject: "group_updated",
-        )
+            old_group: old_actor,
+            updater_actor: updater_actor
+          )
 
-        actor_as_data = Convertible.model_to_as(new_actor)
+          actor_as_data = Convertible.model_to_as(new_actor)
-        Cachex.del(:activity_pub, "actor_#{new_actor.preferred_username}")
+          Cachex.del(:activity_pub, "actor_#{new_actor.preferred_username}")
-        audience = Audience.get_audience(new_actor)
+          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))
+          update_data = make_update_data(actor_as_data, Map.merge(audience, additional))
-        {:ok, new_actor, update_data}
+          {:ok, new_actor, update_data}
 
-      {:error, %Ecto.Changeset{} = err} ->
+        {:error, %Ecto.Changeset{} = err} ->
-        {:error, err}
+          {:error, err}
+      end
+    else
+      {:error, :update_not_allowed}
     end
   end
 

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]

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 == "<p>Some bio</p>"
     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!()