diff --git a/changelog.d/refetch-containment.security b/changelog.d/refetch-containment.security new file mode 100644 index 000000000..bfd118ab1 --- /dev/null +++ b/changelog.d/refetch-containment.security @@ -0,0 +1 @@ +Do not let a refetched object (such as a poll) change its id or author diff --git a/lib/pleroma/object/fetcher.ex b/lib/pleroma/object/fetcher.ex index 8a6f5561f..706ee317c 100644 --- a/lib/pleroma/object/fetcher.ex +++ b/lib/pleroma/object/fetcher.ex @@ -11,6 +11,7 @@ defmodule Pleroma.Object.Fetcher do alias Pleroma.Web.ActivityPub.InternalFetchActor alias Pleroma.Web.ActivityPub.MRF alias Pleroma.Web.ActivityPub.ObjectValidator + alias Pleroma.Web.ActivityPub.ObjectValidators.CommonFixes alias Pleroma.Web.ActivityPub.Pipeline alias Pleroma.Web.ActivityPub.Transmogrifier alias Pleroma.Web.Federator @@ -46,9 +47,14 @@ defmodule Pleroma.Object.Fetcher do end end - def refetch_object(%Object{data: %{"id" => id}} = object) do + # The stored id is canonical, so the refetch must not redirect, and the + # remote must not swap in another object or author. + # Based on Akkoma 9061d148b and fee57eb37. + def refetch_object(%Object{data: %{"id" => id} = data} = object) do with {:local, false} <- {:local, Object.local?(object)}, - {:ok, new_data} <- fetch_and_contain_remote_object_from_id(id), + {:ok, new_data} <- fetch_and_contain(id, true), + {:id, true} <- {:id, new_data["id"] == id}, + {:actor, true} <- {:actor, actor_of(new_data) == actor_of(data)}, {:ok, object} <- reinject_object(object, new_data) do {:ok, object} else @@ -57,6 +63,8 @@ defmodule Pleroma.Object.Fetcher do end end + defp actor_of(data), do: CommonFixes.fix_actor(data)["actor"] + @typep fetcher_errors :: :error | :reject | :allowed_depth | :fetch | :containment | :transmogrifier diff --git a/test/pleroma/object/fetcher_test.exs b/test/pleroma/object/fetcher_test.exs index 411f69bea..c9c4890cd 100644 --- a/test/pleroma/object/fetcher_test.exs +++ b/test/pleroma/object/fetcher_test.exs @@ -465,6 +465,60 @@ defmodule Pleroma.Object.FetcherTest do %{object1: local_object1, object2: local_object2} end + test "it rejects a refetched object whose actor changed", %{object1: object1} do + insert(:user, ap_id: "https://mastodon.social/users/someone_else") + {:ok, o} = Object.create(object1) + + mock(fn %{method: :get, url: "https://mastodon.social/1"} -> + %Tesla.Env{ + status: 200, + headers: [{"content-type", "application/activity+json"}], + body: + object1 + |> Map.merge(%{ + "actor" => "https://mastodon.social/users/someone_else", + "attributedTo" => "https://mastodon.social/users/someone_else", + "content" => "hijacked" + }) + |> Jason.encode!() + } + end) + + assert {:error, _} = Fetcher.refetch_object(o) + assert Object.get_by_ap_id("https://mastodon.social/1").data["content"] == "test 1" + end + + test "it rejects a refetched object whose id changed", %{object1: object1} do + {:ok, o} = Object.create(object1) + + mock(fn %{method: :get, url: "https://mastodon.social/1"} -> + %Tesla.Env{ + status: 200, + url: "https://mastodon.social/2", + headers: [{"content-type", "application/activity+json"}], + body: object1 |> Map.put("id", "https://mastodon.social/2") |> Jason.encode!() + } + end) + + assert {:error, _} = Fetcher.refetch_object(o) + end + + test "it rejects a refetched object whose id differs only by a trailing slash", %{ + object1: object1 + } do + {:ok, o} = Object.create(object1) + + mock(fn %{method: :get, url: "https://mastodon.social/1"} -> + %Tesla.Env{ + status: 200, + headers: [{"content-type", "application/activity+json"}], + body: object1 |> Map.put("id", "https://mastodon.social/1/") |> Jason.encode!() + } + end) + + assert {:error, _} = Fetcher.refetch_object(o) + end + test "it keeps formerRepresentations if remote does not have this attr", %{object1: object1} do full_object1 = object1