From e5e9f9aebda807020abd34673fff5cec21b02c63 Mon Sep 17 00:00:00 2001
From: Oneric <oneric@oneric.stub>
Date: Sat, 16 Nov 2024 00:43:38 +0100
Subject: [PATCH] mrf/object_age: fix handling of non-public objects
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently logic unconditionally adds public adressing to "cc"
and follower adressing to "to" after attempting to strip it
from the other one. This is horrible for two reasons:

First the bug prompting this investigation and fix,
this creates duplicates when adressing URIs already
were in their intended final field; e.g. prominently
the case for all "unlisted" posts.
Since List.delete only removes the first occurence,
this then broke follower-adress stripping later on.

It’s also not safe in general wrt to non-public adressing:
e.g. pre-existing duplicates didn’t get fully stripped,
bespoke adressing modes with only one of public addressing
or follower addressing — and most importantly:
any belatedly received DM also got public adressing added!
Frighently this last point was actually checked as "correct" in tests
albeit this appears to be a mistake from mindless match adjustments
from when genuine crashes on nil adressing lists got fixed in
10c792110e6ea8ed21f739ef8f4f0eff4659ebf9.

Clean up this sloppy logic up, making sure all instances of relevant
adresses are purged and only readded when they actually existed to begin
with. To avoid problems with any List.delete usages remaining in other
places, also ensure we never create duplicate entries.
---
 .../web/activity_pub/mrf/object_age_policy.ex | 34 ++++++++++++++-----
 .../mrf/object_age_policy_test.exs            |  2 +-
 3 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/lib/pleroma/web/activity_pub/mrf/object_age_policy.ex b/lib/pleroma/web/activity_pub/mrf/object_age_policy.ex
index 02c9b18ed..b0c940339 100644
--- a/lib/pleroma/web/activity_pub/mrf/object_age_policy.ex
+++ b/lib/pleroma/web/activity_pub/mrf/object_age_policy.ex
@@ -34,16 +34,34 @@ defp check_reject(message, actions) do
     end
   end
 
+  @spec delete_and_count(list(), term()) :: {integer(), list()}
+  defp delete_and_count(list, element), do: delete_and_count(list, element, {0, [], list})
+
+  defp delete_and_count([], _element, {0, _nlist, olist}), do: {0, olist}
+  defp delete_and_count([], _element, {count, nlist, _olist}), do: {count, Enum.reverse(nlist)}
+
+  defp delete_and_count([h | r], h, {count, nlist, olist}),
+    do: delete_and_count(r, h, {count + 1, nlist, olist})
+
+  defp delete_and_count([h | r], element, {count, nlist, olist}),
+    do: delete_and_count(r, element, {count, [h | nlist], olist})
+
+  defp insert_if_needed(list, oldcount, element) do
+    if oldcount <= 0 || Enum.member?(list, element) do
+      list
+    else
+      [element | list]
+    end
+  end
+
   defp check_delist(message, actions) do
     if :delist in actions do
       with %User{} = user <- User.get_cached_by_ap_id(message["actor"]) do
-        to =
-          List.delete(message["to"] || [], Pleroma.Constants.as_public()) ++
-            [user.follower_address]
+        {pubcnt, to} = delete_and_count(message["to"] || [], Pleroma.Constants.as_public())
+        {flwcnt, cc} = delete_and_count(message["cc"] || [], user.follower_address)
 
-        cc =
-          List.delete(message["cc"] || [], user.follower_address) ++
-            [Pleroma.Constants.as_public()]
+        cc = insert_if_needed(cc, pubcnt, Pleroma.Constants.as_public())
+        to = insert_if_needed(to, flwcnt, user.follower_address)
 
         message =
           message
@@ -65,8 +83,8 @@ defp check_delist(message, actions) do
   defp check_strip_followers(message, actions) do
     if :strip_followers in actions do
       with %User{} = user <- User.get_cached_by_ap_id(message["actor"]) do
-        to = List.delete(message["to"] || [], user.follower_address)
-        cc = List.delete(message["cc"] || [], user.follower_address)
+        {_, to} = delete_and_count(message["to"] || [], user.follower_address)
+        {_, cc} = delete_and_count(message["cc"] || [], user.follower_address)
 
         message =
           message
diff --git a/test/pleroma/web/activity_pub/mrf/object_age_policy_test.exs b/test/pleroma/web/activity_pub/mrf/object_age_policy_test.exs
index 2f649a0a4..9b61d31f4 100644
--- a/test/pleroma/web/activity_pub/mrf/object_age_policy_test.exs
+++ b/test/pleroma/web/activity_pub/mrf/object_age_policy_test.exs
@@ -79,7 +79,7 @@ test "works with objects with empty to or cc fields" do
 
       {:ok, data} = ObjectAgePolicy.filter(data)
 
-      assert Visibility.get_visibility(%{data: data}) == "unlisted"
+      assert Visibility.get_visibility(%{data: data}) == "direct"
     end
 
     test "it delists an old post" do
-- 
2.43.0