From 8243fc0ef482a28daf2bcae2c64a9510bdb76489 Mon Sep 17 00:00:00 2001 From: Oneric Date: Fri, 31 Jan 2025 19:51:53 +0100 Subject: [PATCH] federation: strip internal fields from incoming updates and history When note editing support was added, it was omitted to strip internal fields from edited notes and their history. This was uncovered due to Mastodon inlining the like count as a "likes" collection conflicting with our internal "likes" list causing validation failures. In a spot check with likes/like_count it was not possible to inject those internal fields into the local db via Update, but this was not extensively tested for all fields and avenues. Similarly address normalisation did not normalise addressing in the object history, although this was never at risk of being exploitable. The revision history of the Pleroma MR adding edit support reveals recusrive stripping was intentionally avoided, since it will end up removing e.g. emoji from outgoing activities. This appears to still be true. However, all current internal fields ("pleroma_interal" appears to be unused) contain data already publicised otherwise anyway. In the interest of fixing a federation bug (and at worst potential data injection) quickly outgoing stripping is left non-recursive for now. Of course the ultimate fix here is to not mix remote and internal data into the same map in the first place, but unfortunately having a single map of all truth is a core assumption of *oma's AP doc processing. Changing this is a masive undertaking and not suitable for providing a short-term fix. --- .../web/activity_pub/transmogrifier.ex | 54 ++++-- .../web/activity_pub/transmogrifier_test.exs | 177 ++++++++++++++++++ 2 files changed, 219 insertions(+), 12 deletions(-) diff --git a/lib/pleroma/web/activity_pub/transmogrifier.ex b/lib/pleroma/web/activity_pub/transmogrifier.ex index ecf6d9cd8..9ed54fa6e 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier.ex @@ -29,6 +29,7 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier do @doc """ Modifies an incoming AP object (mastodon format) to our internal format. + (This only deals with non-activity AP objects) """ def fix_object(object, options \\ []) do object @@ -44,6 +45,38 @@ def fix_object(object, options \\ []) do |> fix_content_map() |> fix_addressing() |> fix_summary() + |> fix_history(&fix_object/1) + end + + defp maybe_fix_object(%{"attributedTo" => _} = object), do: fix_object(object) + defp maybe_fix_object(object), do: object + + defp fix_history(%{"formerRepresentations" => %{"orderedItems" => list}} = obj, fix_fun) + when is_list(list) do + update_in(obj["formerRepresentations"]["orderedItems"], fn h -> Enum.map(h, fix_fun) end) + end + + defp fix_history(obj, _), do: obj + + defp fix_recursive(obj, fun) do + # unlike Erlang, Elixir does not support recursive inline functions + # which would allow us to avoid reconstructing this on every recursion + rec_fun = fn + obj when is_map(obj) -> fix_recursive(obj, fun) + # there may be simple AP IDs in history (or object field) + obj -> obj + end + + obj + |> fun.() + |> fix_history(rec_fun) + |> then(fn + %{"object" => object} = doc when is_map(object) -> + update_in(doc["object"], rec_fun) + + apdoc -> + apdoc + end) end def fix_summary(%{"summary" => nil} = object) do @@ -414,17 +447,10 @@ defp get_reported(objects) do end def handle_incoming(data, options \\ []) do - data = normalise_addressing_public(data) - - data = - if data["object"] != nil do - object = normalise_addressing_public(data["object"]) - Map.put(data, "object", object) - else - data - end - - handle_incoming_normalised(data, options) + data + |> fix_recursive(&normalise_addressing_public/1) + |> fix_recursive(&strip_internal_fields/1) + |> handle_incoming_normalised(options) end defp handle_incoming_normalised(data, options) @@ -490,7 +516,6 @@ defp handle_incoming_normalised( object = data["object"] - |> strip_internal_fields() |> fix_type(fetch_options) |> fix_in_reply_to(fetch_options) |> fix_quote_url(fetch_options) @@ -545,6 +570,9 @@ defp handle_incoming_normalised( _options ) when type in ~w{Update Block Follow Accept Reject} do + fixed_obj = maybe_fix_object(data["object"]) + data = if fixed_obj != nil, do: %{data | "object" => fixed_obj}, else: data + with {:ok, %User{}} <- ObjectValidator.fetch_actor(data), {:ok, activity, _} <- Pipeline.common_pipeline(data, local: false) do @@ -1043,6 +1071,8 @@ def prepare_attachments(object) do Map.put(object, "attachment", attachments) end + # for outgoing docs immediately stripping internal fields recursively breaks later emoji transformations + # (XXX: it would be better to reorder operations so we can always use recursive stripping) def strip_internal_fields(object) do Map.drop(object, Pleroma.Constants.object_internal_fields()) end diff --git a/test/pleroma/web/activity_pub/transmogrifier_test.exs b/test/pleroma/web/activity_pub/transmogrifier_test.exs index be07a0fe4..b90692370 100644 --- a/test/pleroma/web/activity_pub/transmogrifier_test.exs +++ b/test/pleroma/web/activity_pub/transmogrifier_test.exs @@ -164,6 +164,183 @@ test "it accepts quote posts" do # It fetched the quoted post assert Object.normalize("https://misskey.io/notes/8vs6wxufd0") end + + test "doesn't allow remote edits to fake local likes" do + # as a spot check for no internal fields getting injected + now = DateTime.utc_now() + pub_date = DateTime.to_iso8601(Timex.subtract(now, Timex.Duration.from_minutes(3))) + edit_date = DateTime.to_iso8601(now) + + local_user = insert(:user) + + create_data = %{ + "type" => "Create", + "id" => "http://mastodon.example.org/users/admin/statuses/2619539638/activity", + "actor" => "http://mastodon.example.org/users/admin", + "to" => ["as:Public"], + "cc" => [], + "object" => %{ + "type" => "Note", + "id" => "http://mastodon.example.org/users/admin/statuses/2619539638", + "attributedTo" => "http://mastodon.example.org/users/admin", + "to" => ["as:Public"], + "cc" => [], + "published" => pub_date, + "content" => "miaow", + "likes" => [local_user.ap_id] + } + } + + update_data = + create_data + |> Map.put("type", "Update") + |> Map.put("id", create_data["object"]["id"] <> "/update/1") + |> put_in(["object", "content"], "miaow :3") + |> put_in(["object", "updated"], edit_date) + |> put_in(["object", "formerRepresentations"], %{ + "type" => "OrderedCollection", + "totalItems" => 1, + "orderedItems" => [create_data["object"]] + }) + + {:ok, %Pleroma.Activity{} = activity} = Transmogrifier.handle_incoming(create_data) + %Pleroma.Object{} = object = Object.get_by_ap_id(activity.data["object"]) + assert object.data["content"] == "miaow" + assert object.data["likes"] == [] + assert object.data["like_count"] == 0 + + {:ok, %Pleroma.Activity{} = activity} = Transmogrifier.handle_incoming(update_data) + %Pleroma.Object{} = object = Object.get_by_ap_id(activity.data["object"]["id"]) + assert object.data["content"] == "miaow :3" + assert object.data["likes"] == [] + assert object.data["like_count"] == 0 + end + + test "doesn't trip over remote likes in notes" do + now = DateTime.utc_now() + pub_date = DateTime.to_iso8601(Timex.subtract(now, Timex.Duration.from_minutes(3))) + edit_date = DateTime.to_iso8601(now) + + create_data = %{ + "type" => "Create", + "id" => "http://mastodon.example.org/users/admin/statuses/3409297097/activity", + "actor" => "http://mastodon.example.org/users/admin", + "to" => ["as:Public"], + "cc" => [], + "object" => %{ + "type" => "Note", + "id" => "http://mastodon.example.org/users/admin/statuses/3409297097", + "attributedTo" => "http://mastodon.example.org/users/admin", + "to" => ["as:Public"], + "cc" => [], + "published" => pub_date, + "content" => "miaow", + "likes" => %{ + "id" => "http://mastodon.example.org/users/admin/statuses/3409297097/likes", + "totalItems" => 0, + "type" => "Collection" + } + } + } + + update_data = + create_data + |> Map.put("type", "Update") + |> Map.put("id", create_data["object"]["id"] <> "/update/1") + |> put_in(["object", "content"], "miaow :3") + |> put_in(["object", "updated"], edit_date) + |> put_in(["object", "likes", "totalItems"], 666) + |> put_in(["object", "formerRepresentations"], %{ + "type" => "OrderedCollection", + "totalItems" => 1, + "orderedItems" => [create_data["object"]] + }) + + {:ok, %Pleroma.Activity{} = activity} = Transmogrifier.handle_incoming(create_data) + %Pleroma.Object{} = object = Object.get_by_ap_id(activity.data["object"]) + assert object.data["content"] == "miaow" + assert object.data["likes"] == [] + assert object.data["like_count"] == 0 + + {:ok, %Pleroma.Activity{} = activity} = Transmogrifier.handle_incoming(update_data) + %Pleroma.Object{} = object = Object.get_by_ap_id(activity.data["object"]["id"]) + assert object.data["content"] == "miaow :3" + assert object.data["likes"] == [] + # in the future this should retain remote likes, but for now: + assert object.data["like_count"] == 0 + end + + test "doesn't trip over remote likes in polls" do + now = DateTime.utc_now() + pub_date = DateTime.to_iso8601(Timex.subtract(now, Timex.Duration.from_minutes(3))) + edit_date = DateTime.to_iso8601(now) + + create_data = %{ + "type" => "Create", + "id" => "http://mastodon.example.org/users/admin/statuses/2471790073/activity", + "actor" => "http://mastodon.example.org/users/admin", + "to" => ["as:Public"], + "cc" => [], + "object" => %{ + "type" => "Question", + "id" => "http://mastodon.example.org/users/admin/statuses/2471790073", + "attributedTo" => "http://mastodon.example.org/users/admin", + "to" => ["as:Public"], + "cc" => [], + "published" => pub_date, + "content" => "vote!", + "anyOf" => [ + %{ + "type" => "Note", + "name" => "a", + "replies" => %{ + "type" => "Collection", + "totalItems" => 3 + } + }, + %{ + "type" => "Note", + "name" => "b", + "replies" => %{ + "type" => "Collection", + "totalItems" => 1 + } + } + ], + "likes" => %{ + "id" => "http://mastodon.example.org/users/admin/statuses/2471790073/likes", + "totalItems" => 0, + "type" => "Collection" + } + } + } + + update_data = + create_data + |> Map.put("type", "Update") + |> Map.put("id", create_data["object"]["id"] <> "/update/1") + |> put_in(["object", "content"], "vote now!") + |> put_in(["object", "updated"], edit_date) + |> put_in(["object", "likes", "totalItems"], 666) + |> put_in(["object", "formerRepresentations"], %{ + "type" => "OrderedCollection", + "totalItems" => 1, + "orderedItems" => [create_data["object"]] + }) + + {:ok, %Pleroma.Activity{} = activity} = Transmogrifier.handle_incoming(create_data) + %Pleroma.Object{} = object = Object.get_by_ap_id(activity.data["object"]) + assert object.data["content"] == "vote!" + assert object.data["likes"] == [] + assert object.data["like_count"] == 0 + + {:ok, %Pleroma.Activity{} = activity} = Transmogrifier.handle_incoming(update_data) + %Pleroma.Object{} = object = Object.get_by_ap_id(activity.data["object"]["id"]) + assert object.data["content"] == "vote now!" + assert object.data["likes"] == [] + # in the future this should retain remote likes, but for now: + assert object.data["like_count"] == 0 + end end describe "prepare outgoing" do