From 2a55239ca816a78428bfba25cf078b5a8a56fec2 Mon Sep 17 00:00:00 2001 From: Oneric Date: Sat, 16 Nov 2024 00:43:38 +0100 Subject: [PATCH 1/4] 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 From 0d93c903f7917e10cf5c2676463efaef4a6428b5 Mon Sep 17 00:00:00 2001 From: Lain Soykaf Date: Tue, 27 Oct 2026 12:27:09 +0400 Subject: [PATCH 2/4] test(mrf): cover ObjectAgePolicy with private, direct and unlisted posts --- changelog.d/object-age-non-public.security | 1 + .../mrf/object_age_policy_test.exs | 86 +++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 changelog.d/object-age-non-public.security diff --git a/changelog.d/object-age-non-public.security b/changelog.d/object-age-non-public.security new file mode 100644 index 000000000..aee358d38 --- /dev/null +++ b/changelog.d/object-age-non-public.security @@ -0,0 +1 @@ +ObjectAgePolicy no longer makes old DMs and followers-only posts public 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 8c463f7f2..119a54717 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 @@ -8,6 +8,8 @@ defmodule Pleroma.Web.ActivityPub.MRF.ObjectAgePolicyTest do alias Pleroma.Web.ActivityPub.MRF.ObjectAgePolicy alias Pleroma.Web.ActivityPub.Visibility + require Pleroma.Constants + setup do: clear_config(:mrf_object_age, threshold: 172_800, @@ -94,6 +96,90 @@ defmodule Pleroma.Web.ActivityPub.MRF.ObjectAgePolicyTest do assert Visibility.get_visibility(%{data: data}) == "unlisted" end + test "it keeps an old followers-only post private" do + clear_config([:mrf_object_age, :actions], [:delist]) + + data = get_old_message() + {:ok, user} = User.get_or_fetch_by_ap_id(data["actor"]) + + data = + data + |> Map.put("to", [user.follower_address]) + |> Map.put("cc", []) + |> put_in(["object", "to"], [user.follower_address]) + |> put_in(["object", "cc"], []) + + {:ok, data} = ObjectAgePolicy.filter(data) + + assert data["to"] == [user.follower_address] + assert data["cc"] == [] + assert Visibility.get_visibility(%{data: data}) == "private" + assert data["object"]["to"] == data["to"] + assert data["object"]["cc"] == data["cc"] + end + + test "it keeps an old direct message direct" do + clear_config([:mrf_object_age, :actions], [:delist]) + + data = get_old_message() + {:ok, _user} = User.get_or_fetch_by_ap_id(data["actor"]) + recipient = "https://example.com/users/someone" + + data = + data + |> Map.put("to", [recipient]) + |> Map.put("cc", []) + |> put_in(["object", "to"], [recipient]) + |> put_in(["object", "cc"], []) + + {:ok, data} = ObjectAgePolicy.filter(data) + + assert data["to"] == [recipient] + assert data["cc"] == [] + assert Visibility.get_visibility(%{data: data}) == "direct" + end + + test "it removes every copy of the follower address" do + clear_config([:mrf_object_age, :actions], [:strip_followers]) + + data = get_old_message() + {:ok, user} = User.get_or_fetch_by_ap_id(data["actor"]) + public = Pleroma.Constants.as_public() + followers = [user.follower_address, user.follower_address] + + data = + data + |> Map.put("to", [public]) + |> Map.put("cc", followers) + |> put_in(["object", "to"], [public]) + |> put_in(["object", "cc"], followers) + + {:ok, data} = ObjectAgePolicy.filter(data) + + assert data["to"] == [public] + assert data["cc"] == [] + end + + test "it does not duplicate addresses of an old unlisted post" do + clear_config([:mrf_object_age, :actions], [:delist, :strip_followers]) + + data = get_old_message() + {:ok, user} = User.get_or_fetch_by_ap_id(data["actor"]) + public = Pleroma.Constants.as_public() + + data = + data + |> Map.put("to", [user.follower_address]) + |> Map.put("cc", [public]) + |> put_in(["object", "to"], [user.follower_address]) + |> put_in(["object", "cc"], [public]) + + {:ok, data} = ObjectAgePolicy.filter(data) + + assert data["to"] == [] + assert data["cc"] == [public] + end + test "it allows a new post" do clear_config([:mrf_object_age, :actions], [:delist]) From 21a5d1dca2edfda01d75038ec4452c7ea8967428 Mon Sep 17 00:00:00 2001 From: Lain Soykaf Date: Tue, 27 Oct 2026 12:53:21 +0400 Subject: [PATCH 3/4] fix(notifications): skip the filter check for objects without content Regex.match? raised on objects without content (for example posts with only attachments), so no notifications were created for them when the recipient had an irreversible filter. --- changelog.d/notification-filter-nil-content.fix | 1 + lib/pleroma/notification.ex | 3 +++ test/pleroma/notification_test.exs | 17 +++++++++++++++++ 3 files changed, 21 insertions(+) create mode 100644 changelog.d/notification-filter-nil-content.fix diff --git a/changelog.d/notification-filter-nil-content.fix b/changelog.d/notification-filter-nil-content.fix new file mode 100644 index 000000000..56b3ad6d1 --- /dev/null +++ b/changelog.d/notification-filter-nil-content.fix @@ -0,0 +1 @@ +Do not crash when creating notifications for posts without content while the user has filters diff --git a/lib/pleroma/notification.ex b/lib/pleroma/notification.ex index afd750823..6e4a97a25 100644 --- a/lib/pleroma/notification.ex +++ b/lib/pleroma/notification.ex @@ -821,6 +821,9 @@ defmodule Pleroma.Notification do object.data["actor"] == user.ap_id -> false + not is_binary(object.data["content"]) -> + false + not is_nil(regex = Pleroma.Filter.compose_regex(user, :re)) -> Regex.match?(regex, object.data["content"]) diff --git a/test/pleroma/notification_test.exs b/test/pleroma/notification_test.exs index 992b15f8b..1ff014a8d 100644 --- a/test/pleroma/notification_test.exs +++ b/test/pleroma/notification_test.exs @@ -9,6 +9,7 @@ defmodule Pleroma.NotificationTest do alias Pleroma.FollowingRelationship alias Pleroma.Notification + alias Pleroma.Object alias Pleroma.Repo alias Pleroma.Tests.ObanHelpers alias Pleroma.User @@ -276,6 +277,22 @@ defmodule Pleroma.NotificationTest do assert {:ok, []} == Notification.create_notifications(status) end + test "it creates notifications for objects without content when the user has filters" do + author = insert(:user) + user = insert(:user) + insert(:filter, user: user, phrase: "cofe", hide: true) + + {:ok, status} = CommonAPI.post(author, %{status: "hey @#{user.nickname}"}) + object = Object.normalize(status, fetch: false) + + {:ok, _} = + object + |> Object.change(%{data: Map.delete(object.data, "content")}) + |> Object.update_and_set_cache() + + assert {:ok, [_]} = Notification.create_notifications(Pleroma.Activity.get_by_id(status.id)) + end + test "it creates notifications if content matches with a not irreversible filter" do user = insert(:user) subscriber = insert(:user) From 985ad29724636ae66ec217f4075aa992b0925e06 Mon Sep 17 00:00:00 2001 From: Lain Soykaf Date: Tue, 27 Oct 2026 16:10:14 +0400 Subject: [PATCH 4/4] test(rate_limiter): use windows that cannot expire during the test The bucket entry expires one window after the first request. With a 50 or 80 ms window, a slow CI run could lose it between two requests, so inspect_bucket/3 returned {0, 5} (seen on #8021 with Elixir 1.17). The reset test expires the bucket itself, so a long window costs nothing. --- changelog.d/rate-limiter-test-flake.skip | 0 test/pleroma/web/plugs/rate_limiter_test.exs | 7 +++++-- 2 files changed, 5 insertions(+), 2 deletions(-) create mode 100644 changelog.d/rate-limiter-test-flake.skip diff --git a/changelog.d/rate-limiter-test-flake.skip b/changelog.d/rate-limiter-test-flake.skip new file mode 100644 index 000000000..e69de29bb diff --git a/test/pleroma/web/plugs/rate_limiter_test.exs b/test/pleroma/web/plugs/rate_limiter_test.exs index 10c93fa73..058c501f5 100644 --- a/test/pleroma/web/plugs/rate_limiter_test.exs +++ b/test/pleroma/web/plugs/rate_limiter_test.exs @@ -50,7 +50,9 @@ defmodule Pleroma.Web.Plugs.RateLimiterTest do test "it restricts based on config values" do limiter_name = :test_plug_opts - scale = 80 + # Long enough that the bucket cannot expire during the test on slow CI. + # The test expires it explicitly with expire_ttl/2. + scale = 60_000 limit = 5 clear_config([Pleroma.Web.Endpoint, :http, :ip], {127, 0, 0, 1}) @@ -184,7 +186,8 @@ defmodule Pleroma.Web.Plugs.RateLimiterTest do test "can have limits separate from unauthenticated connections" do limiter_name = :test_authenticated1 - scale = 50 + # Long enough that the bucket cannot expire during the test on slow CI. + scale = 60_000 limit = 5 clear_config([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) clear_config([:rate_limit, limiter_name], [{1000, 1}, {scale, limit}])