Fix regex usage in MRF (#254)

fixes #235
fixes #228

Co-authored-by: FloatingGhost <hannah@coffee-and-dreams.uk>
Reviewed-on: AkkomaGang/akkoma#254

CHANGELOG.md

@@ -6,6 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ## Unreleased
 
+## UPGRADE NOTES
+- Change your instance blocks to remove any `*.` prefixes. `example.com` will block `*.example.com` by default now
+
 ## Added
 - Officially supported docker release
 - Ability to remove followers unilaterally without a block
@@ -14,6 +17,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ## Changes
 - Follows no longer override domain blocks, a domain block is final
 - Deletes are now the lowest priority to publish and will be handled after creates
+- Domain blocks are now subdomain-matches by default
 
 ## Fixed
 - Registrations via ldap are now compatible with the latest OTP24

lib/pleroma/web/activity_pub/mrf.ex

@@ -149,9 +149,18 @@ defp get_policies(policy) when is_atom(policy), do: [policy]
   defp get_policies(policies) when is_list(policies), do: policies
   defp get_policies(_), do: []
 
+  # Matches the following:
+  # - https://baddomain.net
+  # - https://extra.baddomain.net/
+  # Does NOT match the following:
+  # - https://maybebaddomain.net/
+  def subdomain_regex(domain) do
+    ~r/^(.+\.)?#{Regex.escape(domain)}$/i
+  end
+
   @spec subdomains_regex([String.t()]) :: [Regex.t()]
   def subdomains_regex(domains) when is_list(domains) do
-    for domain <- domains, do: ~r(^#{String.replace(domain, "*.", "(.*\\.)*")}$)i
+    Enum.map(domains, &subdomain_regex/1)
   end
 
   @spec subdomain_match?([Regex.t()], String.t()) :: boolean()

test/pleroma/user_test.exs

@@ -444,17 +444,20 @@ test "it sends a welcome message if it is set" do
     end
 
     setup do:
-            clear_config(:mrf_simple,
-              media_removal: [],
-              media_nsfw: [],
-              federated_timeline_removal: [],
-              report_removal: [],
-              reject: [],
-              followers_only: [],
-              accept: [],
-              avatar_removal: [],
-              banner_removal: [],
-              reject_deletes: []
+            clear_config(
+              [:mrf_simple],
+              %{
+                media_removal: [],
+                media_nsfw: [],
+                federated_timeline_removal: [],
+                report_removal: [],
+                reject: [],
+                followers_only: [],
+                accept: [],
+                avatar_removal: [],
+                banner_removal: [],
+                reject_deletes: []
+              }
             )
 
     setup do:
@@ -1324,7 +1327,7 @@ test "does not block domain with same end if wildcard added" do
       collateral_user =
         insert(:user, %{ap_id: "https://another-awful-and-rude-instance.com/user/bully"})
 
-      {:ok, user} = User.block_domain(user, "*.awful-and-rude-instance.com")
+      {:ok, user} = User.block_domain(user, "awful-and-rude-instance.com")
 
       refute User.blocks?(user, collateral_user)
     end
@@ -1342,7 +1345,7 @@ test "blocks domain with wildcard for subdomain" do
 
       user_domain = insert(:user, %{ap_id: "https://awful-and-rude-instance.com/user/bully"})
 
-      {:ok, user} = User.block_domain(user, "*.awful-and-rude-instance.com")
+      {:ok, user} = User.block_domain(user, "awful-and-rude-instance.com")
 
       assert User.blocks?(user, user_from_subdomain)
       assert User.blocks?(user, user_with_two_subdomains)

test/pleroma/web/activity_pub/mrf/simple_policy_test.exs

@@ -46,8 +46,8 @@ test "has a matching host" do
     end
 
     test "match with wildcard domain" do
-      clear_config([:mrf_simple, :media_removal], [{"*.remote.instance", "Whatever reason"}])
-      media_message = build_media_message()
+      clear_config([:mrf_simple, :media_removal], [{"remote.instance", "Whatever reason"}])
+      media_message = build_media_message("sub.remote.instance")
       local_message = build_local_message()
 
       assert SimplePolicy.filter(media_message) ==
@@ -81,8 +81,8 @@ test "has a matching host" do
     end
 
     test "match with wildcard domain" do
-      clear_config([:mrf_simple, :media_nsfw], [{"*.remote.instance", "yeah yeah"}])
-      media_message = build_media_message()
+      clear_config([:mrf_simple, :media_nsfw], [{"remote.instance", "yeah yeah"}])
+      media_message = build_media_message("sub.remote.instance")
       local_message = build_local_message()
 
       assert SimplePolicy.filter(media_message) ==
@@ -92,9 +92,9 @@ test "match with wildcard domain" do
     end
   end
 
-  defp build_media_message do
+  defp build_media_message(domain \\ "remote.instance") do
     %{
-      "actor" => "https://remote.instance/users/bob",
+      "actor" => "https://#{domain}/users/bob",
       "type" => "Create",
       "object" => %{
         "attachment" => [%{}],
@@ -124,8 +124,8 @@ test "has a matching host" do
     end
 
     test "match with wildcard domain" do
-      clear_config([:mrf_simple, :report_removal], [{"*.remote.instance", "suya"}])
-      report_message = build_report_message()
+      clear_config([:mrf_simple, :report_removal], [{"remote.instance", "suya"}])
+      report_message = build_report_message("sub.remote.instance")
       local_message = build_local_message()
 
       assert {:reject, _} = SimplePolicy.filter(report_message)
@@ -133,9 +133,9 @@ test "match with wildcard domain" do
     end
   end
 
-  defp build_report_message do
+  defp build_report_message(domain \\ "remote.instance") do
     %{
-      "actor" => "https://remote.instance/users/bob",
+      "actor" => "https://#{domain}/users/bob",
       "type" => "Flag"
     }
   end
@@ -143,7 +143,7 @@ defp build_report_message do
   describe "when :federated_timeline_removal" do
     test "is empty" do
       clear_config([:mrf_simple, :federated_timeline_removal], [])
-      {_, ftl_message} = build_ftl_actor_and_message()
+      {_, ftl_message} = build_ftl_actor_and_message("https://remote.instance/users/bob")
       local_message = build_local_message()
 
       assert SimplePolicy.filter(ftl_message) == {:ok, ftl_message}
@@ -151,7 +151,7 @@ test "is empty" do
     end
 
     test "has a matching host" do
-      {actor, ftl_message} = build_ftl_actor_and_message()
+      {actor, ftl_message} = build_ftl_actor_and_message("https://remote.instance/users/bob")
 
       ftl_message_actor_host =
         ftl_message
@@ -172,7 +172,7 @@ test "has a matching host" do
     end
 
     test "match with wildcard domain" do
-      {actor, ftl_message} = build_ftl_actor_and_message()
+      {actor, ftl_message} = build_ftl_actor_and_message("https://sub.remote.instance/users/bob")
 
       ftl_message_actor_host =
         ftl_message
@@ -181,7 +181,7 @@ test "match with wildcard domain" do
         |> Map.fetch!(:host)
 
       clear_config([:mrf_simple, :federated_timeline_removal], [
-        {"*." <> ftl_message_actor_host, "owo"}
+        {ftl_message_actor_host, "owo"}
       ])
 
       local_message = build_local_message()
@@ -196,7 +196,7 @@ test "match with wildcard domain" do
     end
 
     test "has a matching host but only as:Public in to" do
-      {_actor, ftl_message} = build_ftl_actor_and_message()
+      {_actor, ftl_message} = build_ftl_actor_and_message("https://remote.instance/users/bob")
 
       ftl_message_actor_host =
         ftl_message
@@ -253,8 +253,8 @@ test "obfuscates domains listed in :transparency_obfuscate_domains" do
     end
   end
 
-  defp build_ftl_actor_and_message do
-    actor = insert(:user)
+  defp build_ftl_actor_and_message(ap_id) do
+    actor = insert(:user, ap_id: ap_id)
 
     {actor,
      %{
@@ -282,9 +282,9 @@ test "activity has a matching host" do
     end
 
     test "activity matches with wildcard domain" do
-      clear_config([:mrf_simple, :reject], [{"*.remote.instance", ""}])
+      clear_config([:mrf_simple, :reject], [{"remote.instance", ""}])
 
-      remote_message = build_remote_message()
+      remote_message = build_remote_message("sub.remote.instance")
 
       assert {:reject, _} = SimplePolicy.filter(remote_message)
     end
@@ -325,7 +325,7 @@ test "reject by URI object" do
   describe "when :followers_only" do
     test "is empty" do
       clear_config([:mrf_simple, :followers_only], [])
-      {_, ftl_message} = build_ftl_actor_and_message()
+      {_, ftl_message} = build_ftl_actor_and_message("https://remote.instance/users/alice")
       local_message = build_local_message()
 
       assert SimplePolicy.filter(ftl_message) == {:ok, ftl_message}
@@ -412,10 +412,10 @@ test "activity has a matching host" do
     end
 
     test "activity matches with wildcard domain" do
-      clear_config([:mrf_simple, :accept], [{"*.remote.instance", ""}])
+      clear_config([:mrf_simple, :accept], [{"remote.instance", ""}])
 
       local_message = build_local_message()
-      remote_message = build_remote_message()
+      remote_message = build_remote_message("sub.remote.instance")
 
       assert SimplePolicy.filter(local_message) == {:ok, local_message}
       assert SimplePolicy.filter(remote_message) == {:ok, remote_message}
@@ -457,9 +457,9 @@ test "has a matching host" do
     end
 
     test "match with wildcard domain" do
-      clear_config([:mrf_simple, :avatar_removal], [{"*.remote.instance", ""}])
+      clear_config([:mrf_simple, :avatar_removal], [{"remote.instance", ""}])
 
-      remote_user = build_remote_user()
+      remote_user = build_remote_user("sub.remote.instance")
       {:ok, filtered} = SimplePolicy.filter(remote_user)
 
       refute filtered["icon"]
@@ -493,9 +493,9 @@ test "has a matching host" do
     end
 
     test "match with wildcard domain" do
-      clear_config([:mrf_simple, :banner_removal], [{"*.remote.instance", ""}])
+      clear_config([:mrf_simple, :banner_removal], [{"remote.instance", ""}])
 
-      remote_user = build_remote_user()
+      remote_user = build_remote_user("sub.remote.instance")
       {:ok, filtered} = SimplePolicy.filter(remote_user)
 
       refute filtered["image"]
@@ -553,10 +553,10 @@ test "it rejects the deletion" do
   end
 
   describe "when :reject_deletes match with wildcard domain" do
-    setup do: clear_config([:mrf_simple, :reject_deletes], [{"*.remote.instance", ""}])
+    setup do: clear_config([:mrf_simple, :reject_deletes], [{"remote.instance", ""}])
 
     test "it rejects the deletion" do
-      deletion_message = build_remote_deletion_message()
+      deletion_message = build_remote_deletion_message("sub.remote.instance")
 
       assert {:reject, _} = SimplePolicy.filter(deletion_message)
     end
@@ -570,13 +570,13 @@ defp build_local_message do
     }
   end
 
-  defp build_remote_message do
-    %{"actor" => "https://remote.instance/users/bob"}
+  defp build_remote_message(domain \\ "remote.instance") do
+    %{"actor" => "https://#{domain}/users/bob"}
   end
 
-  defp build_remote_user do
+  defp build_remote_user(domain \\ "remote.instance") do
     %{
-      "id" => "https://remote.instance/users/bob",
+      "id" => "https://#{domain}/users/bob",
       "icon" => %{
         "url" => "http://example.com/image.jpg",
         "type" => "Image"
@@ -589,10 +589,10 @@ defp build_remote_user do
     }
   end
 
-  defp build_remote_deletion_message do
+  defp build_remote_deletion_message(domain \\ "remote.instance") do
     %{
       "type" => "Delete",
-      "actor" => "https://remote.instance/users/bob"
+      "actor" => "https://#{domain}/users/bob"
     }
   end
 end

test/pleroma/web/activity_pub/mrf_test.exs

@@ -9,8 +9,8 @@ defmodule Pleroma.Web.ActivityPub.MRFTest do
 
   test "subdomains_regex/1" do
     assert MRF.subdomains_regex(["unsafe.tld", "*.unsafe.tld"]) == [
-             ~r/^unsafe.tld$/i,
-             ~r/^(.*\.)*unsafe.tld$/i
+             ~r/^(.+\.)?unsafe\.tld$/i,
+             ~r/^(.+\.)?\*\.unsafe\.tld$/i
            ]
   end
 
@@ -18,7 +18,7 @@ test "subdomains_regex/1" do
     test "common domains" do
       regexes = MRF.subdomains_regex(["unsafe.tld", "unsafe2.tld"])
 
-      assert regexes == [~r/^unsafe.tld$/i, ~r/^unsafe2.tld$/i]
+      assert regexes == [~r/^(.+\.)?unsafe\.tld$/i, ~r/^(.+\.)?unsafe2\.tld$/i]
 
       assert MRF.subdomain_match?(regexes, "unsafe.tld")
       assert MRF.subdomain_match?(regexes, "unsafe2.tld")
@@ -27,9 +27,9 @@ test "common domains" do
     end
 
     test "wildcard domains with one subdomain" do
-      regexes = MRF.subdomains_regex(["*.unsafe.tld"])
+      regexes = MRF.subdomains_regex(["unsafe.tld"])
 
-      assert regexes == [~r/^(.*\.)*unsafe.tld$/i]
+      assert regexes == [~r/^(.+\.)?unsafe\.tld$/i]
 
       assert MRF.subdomain_match?(regexes, "unsafe.tld")
       assert MRF.subdomain_match?(regexes, "sub.unsafe.tld")
@@ -38,9 +38,9 @@ test "wildcard domains with one subdomain" do
     end
 
     test "wildcard domains with two subdomains" do
-      regexes = MRF.subdomains_regex(["*.unsafe.tld"])
+      regexes = MRF.subdomains_regex(["unsafe.tld"])
 
-      assert regexes == [~r/^(.*\.)*unsafe.tld$/i]
+      assert regexes == [~r/^(.+\.)?unsafe\.tld$/i]
 
       assert MRF.subdomain_match?(regexes, "unsafe.tld")
       assert MRF.subdomain_match?(regexes, "sub.sub.unsafe.tld")
@@ -51,7 +51,7 @@ test "wildcard domains with two subdomains" do
     test "matches are case-insensitive" do
       regexes = MRF.subdomains_regex(["UnSafe.TLD", "UnSAFE2.Tld"])
 
-      assert regexes == [~r/^UnSafe.TLD$/i, ~r/^UnSAFE2.Tld$/i]
+      assert regexes == [~r/^(.+\.)?UnSafe\.TLD$/i, ~r/^(.+\.)?UnSAFE2\.Tld$/i]
 
       assert MRF.subdomain_match?(regexes, "UNSAFE.TLD")
       assert MRF.subdomain_match?(regexes, "UNSAFE2.TLD")