From 35ac672b8d4a6711754a5f88ad65e52d356c4c67 Mon Sep 17 00:00:00 2001
From: rinpatch <rinpatch@sdf.org>
Date: Wed, 17 Apr 2019 17:59:15 +0300
Subject: [PATCH] Remove containment tests from transmogrifier and fix thread
 visibility solver

---
 lib/pleroma/object/containment.ex             |  3 -
 lib/pleroma/web/activity_pub/visibility.ex    | 21 ++++---
 test/web/activity_pub/activity_pub_test.exs   |  4 +-
 test/web/activity_pub/transmogrifier_test.exs | 56 -------------------
 4 files changed, 15 insertions(+), 69 deletions(-)

diff --git a/lib/pleroma/object/containment.ex b/lib/pleroma/object/containment.ex
index 27e89d87f..25bd911fb 100644
--- a/lib/pleroma/object/containment.ex
+++ b/lib/pleroma/object/containment.ex
@@ -9,9 +9,6 @@ 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.
   """
-
-  require Logger
-
   def get_actor(%{"actor" => actor}) when is_binary(actor) do
     actor
   end
diff --git a/lib/pleroma/web/activity_pub/visibility.ex b/lib/pleroma/web/activity_pub/visibility.ex
index db52fe933..3da709b3d 100644
--- a/lib/pleroma/web/activity_pub/visibility.ex
+++ b/lib/pleroma/web/activity_pub/visibility.ex
@@ -41,16 +41,19 @@ def visible_for_user?(activity, user) do
   # guard
   def entire_thread_visible_for_user?(nil, _user), do: false
 
-  # child
+  # XXX: Probably even more inefficient than the previous implementation, intended to be a placeholder untill https://git.pleroma.social/pleroma/pleroma/merge_requests/971 is in develop
   def entire_thread_visible_for_user?(
-        %Activity{data: %{"object" => %{"inReplyTo" => parent_id}}} = tail,
+        %Activity{} = tail,
+        # %Activity{data: %{"object" => %{"inReplyTo" => parent_id}}} = tail,
         user
-      )
-      when is_binary(parent_id) do
-    parent = Activity.get_in_reply_to_activity(tail)
-    visible_for_user?(tail, user) && entire_thread_visible_for_user?(parent, user)
-  end
+      ) do
+    case Object.normalize(tail) do
+      %{data: %{"inReplyTo" => parent_id}} when is_binary(parent_id) ->
+        parent = Activity.get_in_reply_to_activity(tail)
+        visible_for_user?(tail, user) && entire_thread_visible_for_user?(parent, user)
 
-  # root
-  def entire_thread_visible_for_user?(tail, user), do: visible_for_user?(tail, user)
+      _ ->
+        visible_for_user?(tail, user)
+    end
+  end
 end
diff --git a/test/web/activity_pub/activity_pub_test.exs b/test/web/activity_pub/activity_pub_test.exs
index 291f3df4b..4a9acae69 100644
--- a/test/web/activity_pub/activity_pub_test.exs
+++ b/test/web/activity_pub/activity_pub_test.exs
@@ -832,7 +832,9 @@ test "it filters broken threads" do
       activities = ActivityPub.fetch_activities([user1.ap_id | user1.following])
 
       private_activity_1 = Activity.get_by_ap_id_with_object(private_activity_1.data["id"])
-      assert [public_activity, private_activity_1, private_activity_3] == activities
+      assert [public_activity, private_activity_1, private_activity_3] ==
+               activities
+
       assert length(activities) == 3
 
       activities = ActivityPub.contain_timeline(activities, user1)
diff --git a/test/web/activity_pub/transmogrifier_test.exs b/test/web/activity_pub/transmogrifier_test.exs
index 34ae3a20e..6bb81a054 100644
--- a/test/web/activity_pub/transmogrifier_test.exs
+++ b/test/web/activity_pub/transmogrifier_test.exs
@@ -1136,62 +1136,6 @@ test "it rejects activities which reference objects that have an incorrect attri
     end
   end
 
-  describe "general origin containment" do
-    test "contain_origin_from_id() catches obvious spoofing attempts" do
-      data = %{
-        "id" => "http://example.com/~alyssa/activities/1234.json"
-      }
-
-      :error =
-        Transmogrifier.contain_origin_from_id(
-          "http://example.org/~alyssa/activities/1234.json",
-          data
-        )
-    end
-
-    test "contain_origin_from_id() allows alternate IDs within the same origin domain" do
-      data = %{
-        "id" => "http://example.com/~alyssa/activities/1234.json"
-      }
-
-      :ok =
-        Transmogrifier.contain_origin_from_id(
-          "http://example.com/~alyssa/activities/1234",
-          data
-        )
-    end
-
-    test "contain_origin_from_id() allows matching IDs" do
-      data = %{
-        "id" => "http://example.com/~alyssa/activities/1234.json"
-      }
-
-      :ok =
-        Transmogrifier.contain_origin_from_id(
-          "http://example.com/~alyssa/activities/1234.json",
-          data
-        )
-    end
-
-    test "users cannot be collided through fake direction spoofing attempts" do
-      insert(:user, %{
-        nickname: "rye@niu.moe",
-        local: false,
-        ap_id: "https://niu.moe/users/rye",
-        follower_address: User.ap_followers(%User{nickname: "rye@niu.moe"})
-      })
-
-      {:error, _} = User.get_or_fetch_by_ap_id("https://n1u.moe/users/rye")
-    end
-
-    test "all objects with fake directions are rejected by the object fetcher" do
-      {:error, _} =
-        Fetcher.fetch_and_contain_remote_object_from_id(
-          "https://info.pleroma.site/activity4.json"
-        )
-    end
-  end
-
   describe "reserialization" do
     test "successfully reserializes a message with inReplyTo == nil" do
       user = insert(:user)