From 2a55239ca816a78428bfba25cf078b5a8a56fec2 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

Current logic unconditionally adds public adressing to "cc"
and follower adressing to "to" after attempting to strip it
from the other one. This creates serious problems:

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

It’s also just 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 are mangled — and most importantly:
any belatedly received DM or follower-only post
also got public adressing added!
Shockingly this last point was actually asserted as "correct" in tests;
it appears to be a mistake from mindless match adjustments
while fixing crashes on nil adressing in
10c792110e6ea8ed21f739ef8f4f0eff4659ebf9.

Clean up this sloppy logic up, making sure no more duplicates are
added by us, all instances of relevant adresses are purged and only
readded when they actually existed to begin with.

(cherry picked from commit 932810c35ecba9d11fa4f11112c3444b62c45b65)
---
 .../web/activity_pub/mrf/object_age_policy.ex | 34 ++++++++++++++-----
 .../mrf/object_age_policy_test.exs            |  2 +-
 2 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 00f4cf84b..25ae02f60 100644
--- a/lib/pleroma/web/activity_pub/mrf/object_age_policy.ex
+++ b/lib/pleroma/web/activity_pub/mrf/object_age_policy.ex
@@ -36,16 +36,34 @@ defmodule Pleroma.Web.ActivityPub.MRF.ObjectAgePolicy 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(activity, actions) do
     if :delist in actions do
       with %User{} = user <- User.get_cached_by_ap_id(activity["actor"]) do
-        to =
-          List.delete(activity["to"] || [], Pleroma.Constants.as_public()) ++
-            [user.follower_address]
+        {pubcnt, to} = delete_and_count(activity["to"] || [], Pleroma.Constants.as_public())
+        {flwcnt, cc} = delete_and_count(activity["cc"] || [], user.follower_address)
 
-        cc =
-          List.delete(activity["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)
 
         activity =
           activity
@@ -67,8 +85,8 @@ defmodule Pleroma.Web.ActivityPub.MRF.ObjectAgePolicy do
   defp check_strip_followers(activity, actions) do
     if :strip_followers in actions do
       with %User{} = user <- User.get_cached_by_ap_id(activity["actor"]) do
-        to = List.delete(activity["to"] || [], user.follower_address)
-        cc = List.delete(activity["cc"] || [], user.follower_address)
+        {_, to} = delete_and_count(activity["to"] || [], user.follower_address)
+        {_, cc} = delete_and_count(activity["cc"] || [], user.follower_address)
 
         activity =
           activity
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 7a67645aa..8c463f7f2 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 @@ defmodule Pleroma.Web.ActivityPub.MRF.ObjectAgePolicyTest 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