From 932810c35ecba9d11fa4f11112c3444b62c45b65 Mon Sep 17 00:00:00 2001 From: Oneric 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. --- CHANGELOG.md | 2 ++ .../web/activity_pub/mrf/object_age_policy.ex | 34 ++++++++++++++----- .../mrf/object_age_policy_test.exs | 2 +- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f1644dad..3959f66f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - AP objects with additional JSON-LD profiles beyond ActivityStreams can now be fetched - Single-selection polls no longer expose the voter_count; MastoAPI demands it be null and this confused some clients leading to vote distributions >100% +- ObjectAge policy no longer lets unlisted posts slip through +- ObjectAge policy no longer leaks belated DMs and follower-only posts ## Changed - Refactored Rich Media to cache the content in the database. Fetching operations that could block status rendering have been eliminated. 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