lib/pleroma/object/containment.ex

@@ -11,6 +11,9 @@ defmodule Pleroma.Object.Containment do
   Object containment is an important step in validating remote objects to prevent
   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
@@ -47,6 +50,18 @@ def get_object(_) do
   defp compare_uris(%URI{host: host} = _id_uri, %URI{host: host} = _other_uri), do: :ok
   defp compare_uris(_id_uri, _other_uri), do: :error
 
+  defp compare_uris_exact(uri, uri), do: :ok
+
+  defp compare_uris_exact(%URI{} = id, %URI{} = other),
+    do: compare_uris_exact(URI.to_string(id), URI.to_string(other))
+
+  defp compare_uris_exact(id_uri, other_uri)
+       when is_binary(id_uri) and is_binary(other_uri) do
+    norm_id = String.replace_suffix(id_uri, "/", "")
+    norm_other = String.replace_suffix(other_uri, "/", "")
+    if norm_id == norm_other, do: :ok, else: :error
+  end
+
   @doc """
   Checks whether an URL to fetch from is from the local server.
 
@@ -77,6 +92,26 @@ def contain_origin(id, %{"attributedTo" => actor} = params),
 
   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)
+
+  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
+  end
+
+  def contain_id_to_fetch(_url, _data), do: :error
+
   @doc """
   Check whether the object id is from the same host as another id
   """

lib/pleroma/object/fetcher.ex

@@ -263,7 +263,7 @@ def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do
          {_, :ok} <- {:local_fetch, Containment.contain_local_fetch(id)},
          {:ok, final_id, body} <- get_object(id),
          {:ok, data} <- safe_json_decode(body),
-         {_, :ok} <- {:containment, Containment.contain_origin_from_id(final_id, data)},
+         {_, :ok} <- {:strict_id, Containment.contain_id_to_fetch(final_id, data)},
          {_, :ok} <- {:containment, Containment.contain_origin(final_id, data)} do
       unless Instances.reachable?(final_id) do
         Instances.set_reachable(final_id)
@@ -271,6 +271,9 @@ def fetch_and_contain_remote_object_from_id(id) when is_binary(id) do
 
       {:ok, data}
     else
+      {:strict_id, _} ->
+        {:error, "Object's ActivityPub id/url does not match final fetch URL"}
+
       {:scheme, _} ->
         {:error, "Unsupported URI scheme"}
 

test/fixtures/tesla_mock/https__info.pleroma.site_activity3.json

@@ -3,7 +3,7 @@
         "attributedTo": "http://mastodon.example.org/users/admin",
         "attachment": [],
         "content": "<p>this post was not actually written by Haelwenn</p>",
-        "id": "https://info.pleroma.site/activity2.json",
+        "id": "https://info.pleroma.site/activity3.json",
         "published": "2018-09-01T22:15:00Z",
         "tag": [],
         "to": [

test/fixtures/tesla_mock/relay@mastdon.example.org.json

@@ -11,7 +11,7 @@
     "toot": "http://joinmastodon.org/ns#",
     "Emoji": "toot:Emoji"
   }],
-  "id": "http://mastodon.example.org/users/admin",
+  "id": "http://mastodon.example.org/users/relay",
   "type": "Application",
   "invisible": true,
   "following": "http://mastodon.example.org/users/admin/following",
@@ -24,8 +24,8 @@
   "url": "http://mastodon.example.org/@admin",
   "manuallyApprovesFollowers": false,
   "publicKey": {
-    "id": "http://mastodon.example.org/users/admin#main-key",
-    "owner": "http://mastodon.example.org/users/admin",
+    "id": "http://mastodon.example.org/users/relay#main-key",
+    "owner": "http://mastodon.example.org/users/relay",
     "publicKeyPem": "-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAtc4Tir+3ADhSNF6VKrtW\nOU32T01w7V0yshmQei38YyiVwVvFu8XOP6ACchkdxbJ+C9mZud8qWaRJKVbFTMUG\nNX4+6Q+FobyuKrwN7CEwhDALZtaN2IPbaPd6uG1B7QhWorrY+yFa8f2TBM3BxnUy\nI4T+bMIZIEYG7KtljCBoQXuTQmGtuffO0UwJksidg2ffCF5Q+K//JfQagJ3UzrR+\nZXbKMJdAw4bCVJYs4Z5EhHYBwQWiXCyMGTd7BGlmMkY6Av7ZqHKC/owp3/0EWDNz\nNqF09Wcpr3y3e8nA10X40MJqp/wR+1xtxp+YGbq/Cj5hZGBG7etFOmIpVBrDOhry\nBwIDAQAB\n-----END PUBLIC KEY-----\n"
   },
   "attachment": [{

test/pleroma/collections/collections_fetcher_test.exs

@@ -12,11 +12,14 @@ defmodule Akkoma.Collections.FetcherTest do
   end
 
   test "it should extract items from an embedded array in a Collection" do
+    ap_id = "https://example.com/collection/ordered_array"
+
     unordered_collection =
       "test/fixtures/collections/unordered_array.json"
       |> File.read!()
-
-    ap_id = "https://example.com/collection/ordered_array"
+      |> Jason.decode!()
+      |> Map.put("id", ap_id)
+      |> Jason.encode!(pretty: true)
 
     Tesla.Mock.mock(fn
       %{

test/pleroma/object/containment_test.exs

@@ -105,6 +105,56 @@ test "contain_origin_from_id() allows matching IDs" do
         )
     end
 
+    test "contain_id_to_fetch() refuses alternate IDs within the same origin domain" do
+      data = %{
+        "id" => "http://example.com/~alyssa/activities/1234.json",
+        "url" => "http://example.com/@alyssa/status/1234"
+      }
+
+      :error =
+        Containment.contain_id_to_fetch(
+          "http://example.com/~alyssa/activities/1234",
+          data
+        )
+    end
+
+    test "contain_id_to_fetch() allows matching IDs" do
+      data = %{
+        "id" => "http://example.com/~alyssa/activities/1234.json/"
+      }
+
+      :ok =
+        Containment.contain_id_to_fetch(
+          "http://example.com/~alyssa/activities/1234.json/",
+          data
+        )
+
+      :ok =
+        Containment.contain_id_to_fetch(
+          "http://example.com/~alyssa/activities/1234.json",
+          data
+        )
+    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, %{

test/pleroma/object/fetcher_test.exs

@@ -57,6 +57,46 @@ defp spoofed_object_with_ids(
           body: spoofed_object_with_ids("https://patch.cx/objects/spoof_content_type")
         }
 
+      # Spoof: mismatching ids
+      # Variant 1: Non-exisitng fake id
+      %{
+        method: :get,
+        url:
+          "https://patch.cx/media/03ca3c8b4ac3ddd08bf0f84be7885f2f88de0f709112131a22d83650819e36c2.json"
+      } ->
+        %Tesla.Env{
+          status: 200,
+          url:
+            "https://patch.cx/media/03ca3c8b4ac3ddd08bf0f84be7885f2f88de0f709112131a22d83650819e36c2.json",
+          headers: [{"content-type", "application/activity+json"}],
+          body: spoofed_object_with_ids()
+        }
+
+      %{method: :get, url: "https://patch.cx/objects/spoof"} ->
+        %Tesla.Env{
+          status: 404,
+          url: "https://patch.cx/objects/spoof",
+          headers: [],
+          body: "Not found"
+        }
+
+      # Varaint 2: two-stage payload
+      %{method: :get, url: "https://patch.cx/media/spoof_stage1.json"} ->
+        %Tesla.Env{
+          status: 200,
+          url: "https://patch.cx/media/spoof_stage1.json",
+          headers: [{"content-type", "application/activity+json"}],
+          body: spoofed_object_with_ids("https://patch.cx/media/spoof_stage2.json")
+        }
+
+      %{method: :get, url: "https://patch.cx/media/spoof_stage2.json"} ->
+        %Tesla.Env{
+          status: 200,
+          url: "https://patch.cx/media/spoof_stage2.json",
+          headers: [{"content-type", "application/activity+json"}],
+          body: spoofed_object_with_ids("https://patch.cx/media/unpredictable.json")
+        }
+
       # Spoof: cross-domain redirect with original domain id
       %{method: :get, url: "https://patch.cx/objects/spoof_media_redirect1"} ->
         %Tesla.Env{
@@ -79,7 +119,7 @@ defp spoofed_object_with_ids(
       %{method: :get, url: "https://patch.cx/objects/spoof_redirect"} ->
         %Tesla.Env{
           status: 200,
-          url: "https://patch.cx/objects/spoof",
+          url: "https://patch.cx/objects/spoof_redirect",
           headers: [{"content-type", "application/activity+json"}],
           body: spoofed_object_with_ids("https://patch.cx/objects/spoof_redirect")
         }
@@ -110,6 +150,7 @@ defp spoofed_object_with_ids(
         %{method: :get, url: "https://social.sakamoto.gq/notice/9wTkLEnuq47B25EehM"} ->
           %Tesla.Env{
             status: 200,
+            url: "https://social.sakamoto.gq/objects/f20f2497-66d9-4a52-a2e1-1be2a39c32c1",
             body: File.read!("test/fixtures/fetch_mocks/9wTkLEnuq47B25EehM.json"),
             headers: HttpRequestMock.activitypub_object_headers()
           }
@@ -208,6 +249,18 @@ test "it does not fetch a spoofed object with wrong content type" do
                )
     end
 
+    test "it does not fetch a spoofed object with id different from URL" do
+      assert {:error, "Object's ActivityPub id/url does not match final fetch URL"} =
+               Fetcher.fetch_and_contain_remote_object_from_id(
+                 "https://patch.cx/media/03ca3c8b4ac3ddd08bf0f84be7885f2f88de0f709112131a22d83650819e36c2.json"
+               )
+
+      assert {:error, "Object's ActivityPub id/url does not match final fetch URL"} =
+               Fetcher.fetch_and_contain_remote_object_from_id(
+                 "https://patch.cx/media/spoof_stage1.json"
+               )
+    end
+
     test "it does not fetch an object via cross-domain redirects (initial id)" do
       assert {:error, {:cross_domain_redirect, true}} =
                Fetcher.fetch_and_contain_remote_object_from_id(

test/pleroma/web/activity_pub/activity_pub_test.exs

@@ -312,7 +312,7 @@ test "fetches user featured collection" do
   end
 
   test "fetches user featured collection using the first property" do
-    featured_url = "https://friendica.example.com/raha/collections/featured"
+    featured_url = "https://friendica.example.com/featured/raha"
     first_url = "https://friendica.example.com/featured/raha?page=1"
 
     featured_data =
@@ -350,7 +350,7 @@ test "fetches user featured collection using the first property" do
   end
 
   test "fetches user featured when it has string IDs" do
-    featured_url = "https://example.com/alisaie/collections/featured"
+    featured_url = "https://example.com/users/alisaie/collections/featured"
     dead_url = "https://example.com/users/alisaie/statuses/108311386746229284"
 
     featured_data =
@@ -1720,7 +1720,7 @@ test "doesn't crash when follower and following counters are hidden" do
             json(
               %{
                 "@context" => "https://www.w3.org/ns/activitystreams",
-                "id" => "http://remote.org/users/masto_hidden_counters/followers"
+                "id" => "http://remote.org/users/masto_hidden_counters/following"
               },
               headers: HttpRequestMock.activitypub_object_headers()
             )
@@ -1732,7 +1732,7 @@ test "doesn't crash when follower and following counters are hidden" do
             json(
               %{
                 "@context" => "https://www.w3.org/ns/activitystreams",
-                "id" => "http://remote.org/users/masto_hidden_counters/following"
+                "id" => "http://remote.org/users/masto_hidden_counters/followers"
               },
               headers: HttpRequestMock.activitypub_object_headers()
             )

test/support/http_request_mock.ex

@@ -572,6 +572,7 @@ def get("https://social.stopwatchingus-heidelberg.de/.well-known/host-meta", _,
      }}
   end
 
+  # Mastodon status via display URL
   def get(
         "http://mastodon.example.org/@admin/99541947525187367",
         _,
@@ -581,6 +582,23 @@ def get(
     {:ok,
      %Tesla.Env{
        status: 200,
+       url: "http://mastodon.example.org/@admin/99541947525187367",
+       body: File.read!("test/fixtures/mastodon-note-object.json"),
+       headers: activitypub_object_headers()
+     }}
+  end
+
+  # same status via its canonical ActivityPub id
+  def get(
+        "http://mastodon.example.org/users/admin/statuses/99541947525187367",
+        _,
+        _,
+        _
+      ) do
+    {:ok,
+     %Tesla.Env{
+       status: 200,
+       url: "http://mastodon.example.org/users/admin/statuses/99541947525187367",
        body: File.read!("test/fixtures/mastodon-note-object.json"),
        headers: activitypub_object_headers()
      }}