Merge pull request 'Federation: validate actor hosts and fetched object ids' (#8034) from lambadalambda/pleroma:fix/actor-containment into develop

Reviewed-on: https://git.pleroma.social/pleroma/pleroma/pulls/8034

test/pleroma/object/containment_test.exs

@@ -127,4 +127,56 @@ defmodule Pleroma.Object.ContainmentTest do
       :ok = Containment.contain_child(data)
     end
   end
+
+  describe "get_actor/1" do
+    test "returns nil for actor shapes without an id" do
+      for actor <- [%{"type" => "Person"}, %{"id" => 1}, 1, true] do
+        assert nil == Containment.get_actor(%{"actor" => actor})
+        assert nil == Containment.get_actor(%{"actor" => nil, "attributedTo" => actor})
+      end
+
+      assert nil == Containment.get_actor(%{})
+    end
+
+    test "contain_origin/2 rejects actor shapes without an id" do
+      assert :error ==
+               Containment.contain_origin("https://a.test/o/1", %{
+                 "attributedTo" => %{"type" => "Person"}
+               })
+    end
+  end
+
+  describe "contain_id_to_fetch/2" do
+    test "matches the exact id" do
+      assert :ok =
+               Containment.contain_id_to_fetch("https://a.test/o/1", %{
+                 "id" => "https://a.test/o/1"
+               })
+    end
+
+    test "ignores fragments, a trailing slash and the host case" do
+      assert :ok =
+               Containment.contain_id_to_fetch("https://A.test/o/1/", %{
+                 "id" => "https://a.test/o/1#x"
+               })
+    end
+
+    test "rejects another id on the same host" do
+      assert :error =
+               Containment.contain_id_to_fetch("https://a.test/o/1", %{
+                 "id" => "https://a.test/o/2"
+               })
+    end
+
+    test "rejects a different query" do
+      assert :error =
+               Containment.contain_id_to_fetch("https://a.test/o?page=1", %{
+                 "id" => "https://a.test/o?page=2"
+               })
+    end
+
+    test "rejects documents without an id" do
+      assert :error = Containment.contain_id_to_fetch("https://a.test/o/1", %{})
+    end
+  end
 end

test/pleroma/object/fetcher_test.exs

@@ -48,6 +48,8 @@ defmodule Pleroma.Object.FetcherTest do
         %{method: :get, url: "https://social.sakamoto.gq/notice/9wTkLEnuq47B25EehM"} ->
           %Tesla.Env{
             status: 200,
+            # Pleroma redirects /notice/:id to the object id
+            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()
           }
@@ -463,6 +465,78 @@ 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 author has no id", %{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.delete("actor")
+            |> Map.put("attributedTo", %{"type" => "Person"})
+            |> Jason.encode!()
+        }
+      end)
+
+      assert {:error, _} = Fetcher.refetch_object(o)
+    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
@@ -642,7 +716,23 @@ defmodule Pleroma.Object.FetcherTest do
               })
           }
 
-        # No redirect - same domain
+        # Same-domain redirect to the object id
+        %{method: :get, url: "https://original.test/@actor/redirect"} ->
+          %Tesla.Env{
+            status: 200,
+            url: "https://original.test/objects/redirect-target",
+            headers: [{"content-type", "application/activity+json"}],
+            body:
+              Jason.encode!(%{
+                "id" => "https://original.test/objects/redirect-target",
+                "type" => "Note",
+                "content" => "This has a same-domain redirect",
+                "actor" => "https://original.test/users/actor",
+                "attributedTo" => "https://original.test/users/actor"
+              })
+          }
+
+        # Same-domain redirect to a document claiming the original URL as id
         %{method: :get, url: "https://original.test/objects/same-domain-redirect"} ->
           %Tesla.Env{
             status: 200,
@@ -695,12 +785,19 @@ defmodule Pleroma.Object.FetcherTest do
     test "it accepts objects with same-domain redirects" do
       assert {:ok, data} =
                Fetcher.fetch_and_contain_remote_object_from_id(
-                 "https://original.test/objects/same-domain-redirect"
+                 "https://original.test/@actor/redirect"
                )
 
       assert data["content"] == "This has a same-domain redirect"
     end
 
+    test "it rejects redirect targets that claim the original URL as id" do
+      assert {:error, _} =
+               Fetcher.fetch_and_contain_remote_object_from_id(
+                 "https://original.test/objects/same-domain-redirect"
+               )
+    end
+
     test "it handles responses without URL field (common in tests)" do
       assert {:ok, data} =
                Fetcher.fetch_and_contain_remote_object_from_id(
@@ -711,6 +808,150 @@ defmodule Pleroma.Object.FetcherTest do
     end
   end
 
+  describe "id containment" do
+    setup do
+      note = fn id, content ->
+        %Tesla.Env{
+          status: 200,
+          headers: [{"content-type", "application/activity+json"}],
+          body:
+            Jason.encode!(%{
+              "id" => id,
+              "type" => "Note",
+              "content" => content,
+              "actor" => "https://host.test/users/actor",
+              "attributedTo" => "https://host.test/users/actor"
+            })
+        }
+      end
+
+      mock(fn
+        # display URL serving the canonical object
+        %{method: :get, url: "https://host.test/@actor/1"} ->
+          note.("https://host.test/objects/1", "display")
+
+        %{method: :get, url: "https://host.test/objects/1"} ->
+          note.("https://host.test/objects/1", "canonical")
+
+        # uploaded file claiming to be another object on the same host
+        %{method: :get, url: "https://host.test/media/evil.json"} ->
+          note.("https://host.test/objects/2", "spoofed")
+
+        %{method: :get, url: "https://host.test/objects/2"} ->
+          note.("https://host.test/objects/2", "real")
+
+        # canonical id does not serve itself
+        %{method: :get, url: "https://host.test/media/loop.json"} ->
+          note.("https://host.test/objects/3", "loop")
+
+        %{method: :get, url: "https://host.test/objects/3"} ->
+          note.("https://host.test/objects/4", "other")
+
+        # canonical id redirects
+        %{method: :get, url: "https://host.test/media/redirect.json"} ->
+          note.("https://host.test/objects/5", "redirect")
+
+        %{method: :get, url: "https://host.test/objects/5"} ->
+          %{note.("https://host.test/objects/5", "moved") | url: "https://host.test/media/x"}
+
+        %{method: :get, url: "https://host.test/objects/slash"} ->
+          note.("https://host.test/objects/slash/", "slash")
+
+        %{method: :get, url: "https://host.test/objects/no-id"} ->
+          note.(nil, "no id")
+      end)
+
+      :ok
+    end
+
+    test "it refetches display URLs from the canonical id" do
+      assert {:ok, %{"id" => "https://host.test/objects/1", "content" => "canonical"}} =
+               Fetcher.fetch_and_contain_remote_object_from_id("https://host.test/@actor/1")
+    end
+
+    test "it does not trust a document claiming another id on the same host" do
+      assert {:ok, %{"id" => "https://host.test/objects/2", "content" => "real"}} =
+               Fetcher.fetch_and_contain_remote_object_from_id(
+                 "https://host.test/media/evil.json"
+               )
+    end
+
+    test "it refetches at most once" do
+      assert {:error, :id_mismatch} =
+               Fetcher.fetch_and_contain_remote_object_from_id(
+                 "https://host.test/media/loop.json"
+               )
+    end
+
+    test "it does not follow redirects when refetching from the canonical id" do
+      assert {:error, _} =
+               Fetcher.fetch_and_contain_remote_object_from_id(
+                 "https://host.test/media/redirect.json"
+               )
+    end
+
+    test "it ignores a trailing slash" do
+      assert {:ok, %{"content" => "slash"}} =
+               Fetcher.fetch_and_contain_remote_object_from_id("https://host.test/objects/slash")
+    end
+
+    test "it accepts ids that only differ by URL encoding" do
+      clear_config(:test_url_encoding, true)
+
+      mock(fn %{method: :get, url: url} ->
+        # the encoder sends uppercase escapes
+        assert url in [
+                 "https://host.test/users/%E3%81%82",
+                 "https://host.test/notes/%E3%81%82"
+               ]
+
+        id =
+          if url =~ "users",
+            do: "https://host.test/users/あ",
+            else: "https://host.test/notes/%e3%81%82"
+
+        %Tesla.Env{
+          status: 200,
+          url: url,
+          headers: [{"content-type", "application/activity+json"}],
+          body: Jason.encode!(%{"id" => id, "type" => "Note"})
+        }
+      end)
+
+      assert {:ok, _} =
+               Fetcher.fetch_and_contain_remote_object_from_id("https://host.test/users/あ")
+
+      assert {:ok, _} =
+               Fetcher.fetch_and_contain_remote_object_from_id(
+                 "https://host.test/notes/%e3%81%82"
+               )
+    end
+
+    test "it rejects objects whose author has no id" do
+      mock(fn %{method: :get, url: "https://host.test/objects/anon" = url} ->
+        %Tesla.Env{
+          status: 200,
+          headers: [{"content-type", "application/activity+json"}],
+          body:
+            Jason.encode!(%{
+              "id" => url,
+              "type" => "Note",
+              "content" => "anon",
+              "attributedTo" => %{"type" => "Person", "name" => "nobody"}
+            })
+        }
+      end)
+
+      assert {:containment, :error} =
+               Fetcher.fetch_object_from_id("https://host.test/objects/anon")
+    end
+
+    test "it rejects documents without an id" do
+      assert {:error, _} =
+               Fetcher.fetch_and_contain_remote_object_from_id("https://host.test/objects/no-id")
+    end
+  end
+
   describe "fetch with history" do
     setup do
       object2 = %{

test/pleroma/web/activity_pub/activity_pub_test.exs

@@ -244,6 +244,58 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do
              }
     end
 
+    test "rejects actors whose inbox is on another host" do
+      ap_id = "https://example.com/users/evil"
+
+      user_data =
+        "test/fixtures/users_mock/user.json"
+        |> File.read!()
+        |> String.replace("{{nickname}}", "evil")
+        |> Jason.decode!()
+        |> Map.put("inbox", "https://victim.example/inbox")
+        |> Map.put("featured", nil)
+        |> Jason.encode!()
+
+      Tesla.Mock.mock(fn
+        %{method: :get, url: ^ap_id} ->
+          %Tesla.Env{
+            status: 200,
+            body: user_data,
+            headers: [{"content-type", "application/activity+json"}]
+          }
+      end)
+
+      assert {:error, _} = ActivityPub.make_user_from_ap_id(ap_id)
+      refute User.get_by_ap_id(ap_id)
+    end
+
+    test "updates the known user when fetched from a display URL" do
+      ap_id = "https://example.com/users/lain"
+      display_url = "https://example.com/@lain"
+      user = insert(:user, local: false, ap_id: ap_id, nickname: "lain@example.com")
+
+      user_data =
+        "test/fixtures/users_mock/user.json"
+        |> File.read!()
+        |> String.replace("{{nickname}}", "lain")
+        |> Jason.decode!()
+        |> Map.merge(%{"featured" => nil, "name" => "new name"})
+        |> Jason.encode!()
+
+      Tesla.Mock.mock(fn %{method: :get, url: url} when url in [ap_id, display_url] ->
+        %Tesla.Env{
+          status: 200,
+          body: user_data,
+          headers: [{"content-type", "application/activity+json"}]
+        }
+      end)
+
+      assert {:ok, %User{id: id, name: "new name"}} =
+               ActivityPub.make_user_from_ap_id(display_url)
+
+      assert id == user.id
+    end
+
     test "fetches user featured collection" do
       ap_id = "https://example.com/users/lain"
 
@@ -516,7 +568,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do
     end
 
     test "fetches user birthday information from misskey" do
-      user_id = "https://misskey.io/@mkljczk"
+      user_id = "https://misskey.io/users/8dhi2ne167"
 
       Tesla.Mock.mock(fn
         %{
@@ -2033,7 +2085,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do
             json(
               %{
                 "@context" => "https://www.w3.org/ns/activitystreams",
-                "id" => "https://remote.org/users/masto_hidden_counters/followers"
+                "id" => "https://remote.org/users/masto_hidden_counters/following"
               },
               headers: HttpRequestMock.activitypub_object_headers()
             )
@@ -2045,7 +2097,7 @@ defmodule Pleroma.Web.ActivityPub.ActivityPubTest do
             json(
               %{
                 "@context" => "https://www.w3.org/ns/activitystreams",
-                "id" => "https://remote.org/users/masto_hidden_counters/following"
+                "id" => "https://remote.org/users/masto_hidden_counters/followers"
               },
               headers: HttpRequestMock.activitypub_object_headers()
             )

test/pleroma/web/activity_pub/object_validators/user_validator_test.exs

@@ -0,0 +1,75 @@
+# Pleroma: A lightweight social networking server
+# Copyright © 2017-2022 Pleroma Authors <https://pleroma.social/>
+# SPDX-License-Identifier: AGPL-3.0-only
+
+defmodule Pleroma.Web.ActivityPub.ObjectValidators.UserValidatorTest do
+  use ExUnit.Case, async: true
+
+  alias Pleroma.Web.ActivityPub.ObjectValidators.UserValidator
+
+  defp actor do
+    "test/fixtures/users_mock/user.json"
+    |> File.read!()
+    |> String.replace("{{nickname}}", "lain")
+    |> Jason.decode!()
+  end
+
+  test "accepts an actor whose endpoints and collections are on its own host" do
+    data = actor()
+    assert {:ok, ^data, []} = UserValidator.validate(data, [])
+  end
+
+  test "accepts an actor without optional fields" do
+    data = Map.drop(actor(), ["endpoints", "featured", "followers", "following", "publicKey"])
+    assert {:ok, _, _} = UserValidator.validate(data, [])
+  end
+
+  test "accepts all actor fixtures" do
+    for path <- Path.wildcard("test/fixtures/**/*.json"),
+        {:ok, %{"inbox" => _, "id" => id} = data} when is_binary(id) <-
+          [Jason.decode(File.read!(path))] do
+      assert {{:ok, _, _}, ^path} = {UserValidator.validate(data, []), path}
+    end
+  end
+
+  test "treats empty and null fields as missing" do
+    data =
+      actor()
+      |> Map.merge(%{"inbox" => "", "followers" => nil, "following" => "", "featured" => ""})
+      |> put_in(["endpoints", "sharedInbox"], "")
+      |> put_in(["publicKey", "id"], "")
+
+    assert {:ok, _, _} = UserValidator.validate(data, [])
+  end
+
+  for field <- ["inbox", "followers", "following", "featured"] do
+    test "rejects an actor whose #{field} is on another host" do
+      data = Map.put(actor(), unquote(field), "https://victim.example/users/lain/x")
+      assert {:error, _} = UserValidator.validate(data, [])
+    end
+  end
+
+  test "rejects an actor whose sharedInbox is on another host" do
+    data = put_in(actor(), ["endpoints", "sharedInbox"], "https://victim.example/inbox")
+    assert {:error, _} = UserValidator.validate(data, [])
+  end
+
+  test "rejects an actor whose collection is an object on another host" do
+    data = Map.put(actor(), "followers", %{"id" => "https://victim.example/users/v/followers"})
+    assert {:error, _} = UserValidator.validate(data, [])
+  end
+
+  test "rejects an actor whose key id is on another host" do
+    data = put_in(actor(), ["publicKey", "id"], "https://victim.example/users/lain#main-key")
+    assert {:error, _} = UserValidator.validate(data, [])
+  end
+
+  test "rejects an actor whose key id has no host" do
+    data = put_in(actor(), ["publicKey", "id"], "lain")
+    assert {:error, _} = UserValidator.validate(data, [])
+  end
+
+  test "rejects an actor without an id" do
+    assert {:error, _} = UserValidator.validate(Map.delete(actor(), "id"), [])
+  end
+end

test/pleroma/web/activity_pub/transmogrifier/user_update_handling_test.exs

@@ -11,8 +11,11 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.UserUpdateHandlingTest do
 
   import Pleroma.Factory
 
+  # matches the host of the actor in mastodon-update.json
+  @gargron "http://mastodon.example.org/users/gargron"
+
   test "it works for incoming update activities" do
-    user = insert(:user, local: false)
+    user = insert(:user, local: false, ap_id: @gargron)
 
     update_data = File.read!("test/fixtures/mastodon-update.json") |> Jason.decode!()
 
@@ -50,8 +53,28 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.UserUpdateHandlingTest do
     assert user.bio == "<p>Some bio</p>"
   end
 
+  test "it rejects updates that move the shared inbox to another host" do
+    user = insert(:user, local: false, ap_id: @gargron)
+
+    update_data = File.read!("test/fixtures/mastodon-update.json") |> Jason.decode!()
+
+    object =
+      update_data["object"]
+      |> Map.put("actor", user.ap_id)
+      |> Map.put("id", user.ap_id)
+      |> put_in(["endpoints", "sharedInbox"], "https://victim.example/inbox")
+
+    update_data =
+      update_data
+      |> Map.put("actor", user.ap_id)
+      |> Map.put("object", object)
+
+    assert {:error, _} = Transmogrifier.handle_incoming(update_data)
+    assert User.get_cached_by_ap_id(user.ap_id).shared_inbox == user.shared_inbox
+  end
+
   test "it selects an actor type after an extension type" do
-    user = insert(:user, local: false)
+    user = insert(:user, local: false, ap_id: @gargron)
     update_data = File.read!("test/fixtures/mastodon-update.json") |> Jason.decode!()
 
     object =
@@ -70,7 +93,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.UserUpdateHandlingTest do
   end
 
   test "it works with alsoKnownAs" do
-    %{ap_id: actor} = insert(:user, local: false)
+    %{ap_id: actor} = insert(:user, local: false, ap_id: @gargron)
 
     assert User.get_cached_by_ap_id(actor).also_known_as == []
 
@@ -97,7 +120,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.UserUpdateHandlingTest do
   end
 
   test "it works with custom profile fields" do
-    user = insert(:user, local: false)
+    user = insert(:user, local: false, ap_id: @gargron)
 
     assert user.fields == []
 
@@ -155,7 +178,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.UserUpdateHandlingTest do
   end
 
   test "it works for incoming update activities which lock the account" do
-    user = insert(:user, local: false)
+    user = insert(:user, local: false, ap_id: @gargron)
 
     update_data = File.read!("test/fixtures/mastodon-update.json") |> Jason.decode!()