Refetch on AP ID mismatch

As hinted at in the commit message when strict checking
was added in 8684964c5d,
refetching is more robust than display URL comparison
but in exchange is harder to implement correctly.

A similar refetch approach is also employed by
e.g. Mastodon, IceShrimp and FireFish.

To make sure no checks can be bypassed by forcing
a refetch, id checking is placed at the very end.

This will fix:
 - Peertube display URL arrays our transmogrifier fails to normalise
 - non-canonical display URLs from alternative frontends
   (theoretical; we didnt’t get any actual reports about this)

It will also be helpful in the planned key handling overhaul.

The modified user collision test was introduced in
https://git.pleroma.social/pleroma/pleroma/-/merge_requests/461
and unfortunately the issues this fixes aren’t public.
Afaict it was just meant to guard against someone serving
faked data belonging to an unrelated domain. Since we now
refetch and the id actually is mocked, lookup now succeeds
but will use the real data from the authorative server
making it unproblematic. Instead modify the fake data further
and make sure we don’t end up using the spoofed version.

lib/pleroma/object/containment.ex

@@ -12,8 +12,6 @@ defmodule Pleroma.Object.Containment do
   spoofing, therefore removal of object containment functions is NOT recommended.
   """
 
-  alias Pleroma.Web.ActivityPub.Transmogrifier
-
   def get_actor(%{"actor" => actor}) when is_binary(actor) do
     actor
   end
@@ -93,21 +91,17 @@ defmodule Pleroma.Object.Containment do
   def contain_origin(_id, _data), do: :ok
 
   @doc """
-  Check whether the fetch URL (after redirects) exactly (sans tralining slash) matches either
-  the canonical ActivityPub id or the objects url field (for display URLs from *key and Mastodon)
+  Check whether the fetch URL (after redirects) exactly
+  matches the canonical ActivityPub id (sans trailing slash).
 
   Since this is meant to be used for fetches, anonymous or transient objects are not accepted here.
   """
-  def contain_id_to_fetch(url, %{"id" => id} = data) when is_binary(id) do
-    with {:id, :error} <- {:id, compare_uris_exact(id, url)},
-         # "url" can be a "Link" object and this is checked before full normalisation
-         display_url <- Transmogrifier.fix_url(data)["url"],
-         true <- display_url != nil do
-      compare_uris_exact(display_url, url)
-    else
-      {:id, :ok} -> :ok
-      _ -> :error
-    end
+  def contain_id_to_fetch(url, %{"id" => id}) when is_binary(id) do
+    # XXX: should we also ignore fragments?
+    #      This would save us a refetch for fragment keys,
+    #      but might be confusing for Mastodon’s unfetchable fragment activities
+    #      (instead of the expected activity we get the object it’s referencing)
+    compare_uris_exact(url, id)
   end
 
   def contain_id_to_fetch(_url, _data), do: :error

lib/pleroma/object/fetcher.ex

@@ -116,7 +116,7 @@ defmodule Pleroma.Object.Fetcher do
   @doc "Assumes object already is in our database and refetches from remote to update (e.g. for polls)"
   def refetch_object(%Object{data: %{"id" => id}} = 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_remote_object_from_id(id, true),
          {:id, true} <- {:id, new_data["id"] == id},
          {:ok, object} <- reinject_object(object, new_data) do
       {:ok, object}
@@ -253,14 +253,17 @@ defmodule Pleroma.Object.Fetcher do
     end
   end
 
-  @doc "Fetches arbitrary remote object and performs basic safety and authenticity checks"
-  def fetch_and_contain_remote_object_from_id(id)
+  @doc """
+  Fetches arbitrary remote object and performs basic safety and authenticity checks.
+  When the fetch URL is known to already be a canonical AP id, checks are stricter.
+  """
+  def fetch_and_contain_remote_object_from_id(id, is_ap_id \\ false)
 
-  def fetch_and_contain_remote_object_from_id(%{"id" => id}),
-    do: fetch_and_contain_remote_object_from_id(id)
+  def fetch_and_contain_remote_object_from_id(%{"id" => id}, is_ap_id),
+    do: fetch_and_contain_remote_object_from_id(id, is_ap_id)
 
-  def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do
-    Logger.debug("Fetching object #{id} via AP")
+  def fetch_and_contain_remote_object_from_id(id, is_ap_id) when is_binary(id) do
+    Logger.debug("Fetching object #{id} via AP [ap_id=#{is_ap_id}]")
 
     with {:valid_uri_scheme, true} <- {:valid_uri_scheme, String.starts_with?(id, "http")},
          %URI{} = uri <- URI.parse(id),
@@ -270,18 +273,31 @@ defmodule Pleroma.Object.Fetcher do
            {: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),
+         # a canonical ID shouldn't be a redirect
+         true <- !is_ap_id || final_id == id,
          {:ok, data} <- safe_json_decode(body),
-         {_, :ok} <- {:strict_id, Containment.contain_id_to_fetch(final_id, data)},
-         {_, :ok} <- {:containment, Containment.contain_origin(final_id, data)} do
+         {_, :ok} <- {:containment, Containment.contain_origin(final_id, data)},
+         {_, _, :ok} <- {:strict_id, data["id"], Containment.contain_id_to_fetch(final_id, data)} do
       unless Instances.reachable?(final_id) do
         Instances.set_reachable(final_id)
       end
 
       {:ok, data}
     else
-      {:strict_id, _} = e ->
+      # E.g.  Mastodon and *key serve the AP object directly under their display URLs without
+      # redirecting to their canonical location first, thus ids will expectedly differ.
+      # Similarly keys, either use a fragment ID and are a subobjects or a distinct ID
+      # but for compatibility are still a subobject presenting their owning actors ID at the toplevel.
+      # Refetching _once_ from the listed id, should yield a strict match afterwards.
+      {:strict_id, ap_id, _} = e ->
+        case is_ap_id do
+          false ->
+            fetch_and_contain_remote_object_from_id(ap_id, true)
+
+          true ->
             log_fetch_error(id, e)
             {:error, :id_mismatch}
+        end
 
       {:mrf_reject_check, _} = e ->
         log_fetch_error(id, e)
@@ -311,7 +327,7 @@ defmodule Pleroma.Object.Fetcher do
     end
   end
 
-  def fetch_and_contain_remote_object_from_id(_id),
+  def fetch_and_contain_remote_object_from_id(_id, _is_ap_id),
     do: {:error, :invalid_id}
 
   defp check_crossdomain_redirect(final_host, original_url)

test/pleroma/object/containment_test.exs

@@ -9,7 +9,6 @@ defmodule Pleroma.Object.ContainmentTest do
   alias Pleroma.User
 
   import Pleroma.Factory
-  import ExUnit.CaptureLog
 
   setup_all do
     Tesla.Mock.mock_global(fn env -> apply(HttpRequestMock, :request, [env]) end)
@@ -136,25 +135,6 @@ defmodule Pleroma.Object.ContainmentTest do
         )
     end
 
-    test "contain_id_to_fetch() allows display URLs" do
-      data = %{
-        "id" => "http://example.com/~alyssa/activities/1234.json",
-        "url" => "http://example.com/@alyssa/status/1234"
-      }
-
-      :ok =
-        Containment.contain_id_to_fetch(
-          "http://example.com/@alyssa/status/1234",
-          data
-        )
-
-      :ok =
-        Containment.contain_id_to_fetch(
-          "http://example.com/@alyssa/status/1234/",
-          data
-        )
-    end
-
     test "users cannot be collided through fake direction spoofing attempts" do
       _user =
         insert(:user, %{
@@ -164,10 +144,14 @@ defmodule Pleroma.Object.ContainmentTest do
           follower_address: User.ap_followers(%User{nickname: "rye@niu.moe"})
         })
 
-      assert capture_log(fn ->
-               {:error, _} = User.get_or_fetch_by_ap_id("https://n1u.moe/users/rye")
-             end) =~
-               "[error] Could not decode user at fetch https://n1u.moe/users/rye"
+      # Fetch from an attempted spoof id will suceed, but automatically retrieve
+      # the real data from the homeserver instead of naïvely using the spoof
+      {:ok, fetched_user} = User.get_or_fetch_by_ap_id("https://n1u.moe/users/rye")
+
+      refute fetched_user.name == "evil rye"
+      refute fetched_user.raw_bio == "boooo!"
+      assert fetched_user.name == "♡ rye ♡"
+      assert fetched_user.nickname == "rye@niu.moe"
     end
 
     test "contain_origin_from_id() gracefully handles cases where no ID is present" do

test/pleroma/object/fetcher_test.exs

@@ -252,7 +252,7 @@ defmodule Pleroma.Object.FetcherTest do
     end
 
     test "it does not fetch a spoofed object with id different from URL" do
-      assert {:error, :id_mismatch} =
+      assert {:error, :not_found} =
                Fetcher.fetch_and_contain_remote_object_from_id(
                  "https://patch.cx/media/03ca3c8b4ac3ddd08bf0f84be7885f2f88de0f709112131a22d83650819e36c2.json"
                )

test/support/http_request_mock.ex

@@ -263,7 +263,12 @@ defmodule HttpRequestMock do
     {:ok,
      %Tesla.Env{
        status: 200,
-       body: File.read!("test/fixtures/tesla_mock/rye.json"),
+       body:
+         File.read!("test/fixtures/tesla_mock/rye.json")
+         |> Jason.decode!()
+         |> Map.put("name", "evil rye")
+         |> Map.put("bio", "boooo!")
+         |> Jason.encode!(),
        headers: activitypub_object_headers()
      }}
   end