From 6aa116eca7d6ef6567dcef03b8c776bd2134bf3f Mon Sep 17 00:00:00 2001 From: lain Date: Tue, 28 Apr 2020 16:26:19 +0200 Subject: [PATCH] Create activity handling: Flip it and reverse it Both objects and create activities will now go through the common pipeline and will be validated. Objects are now created as a side effect of the Create activity, rolling back a transaction if it's not possible to insert the object. --- lib/pleroma/notification.ex | 2 +- lib/pleroma/web/activity_pub/activity_pub.ex | 7 ++++ .../web/activity_pub/object_validator.ex | 4 +- .../chat_message_validator.ex | 2 +- .../create_chat_message_validator.ex | 27 +++++++++++- .../object_validators/types/safe_text.ex | 25 +++++++++++ lib/pleroma/web/activity_pub/pipeline.ex | 12 ++++-- lib/pleroma/web/activity_pub/side_effects.ex | 41 +++++++++++-------- .../transmogrifier/chat_message_handling.ex | 28 ++++++++----- lib/pleroma/web/common_api/common_api.ex | 10 +++-- lib/pleroma/web/common_api/utils.ex | 2 +- .../types/safe_text_test.exs | 23 +++++++++++ test/web/activity_pub/side_effects_test.exs | 21 ++++++---- .../transmogrifier/chat_message_test.exs | 3 +- 14 files changed, 155 insertions(+), 52 deletions(-) create mode 100644 lib/pleroma/web/activity_pub/object_validators/types/safe_text.ex create mode 100644 test/web/activity_pub/object_validators/types/safe_text_test.exs diff --git a/lib/pleroma/notification.ex b/lib/pleroma/notification.ex index 73e19bf97..d96c12440 100644 --- a/lib/pleroma/notification.ex +++ b/lib/pleroma/notification.ex @@ -275,7 +275,7 @@ def dismiss(%{id: user_id} = _user, id) do end def create_notifications(%Activity{data: %{"to" => _, "type" => "Create"}} = activity) do - object = Object.normalize(activity) + object = Object.normalize(activity, false) if object && object.data["type"] == "Answer" do {:ok, []} diff --git a/lib/pleroma/web/activity_pub/activity_pub.ex b/lib/pleroma/web/activity_pub/activity_pub.ex index 69ac06f6b..ecb13d76a 100644 --- a/lib/pleroma/web/activity_pub/activity_pub.ex +++ b/lib/pleroma/web/activity_pub/activity_pub.ex @@ -126,7 +126,14 @@ def increase_poll_votes_if_vote(%{ def increase_poll_votes_if_vote(_create_data), do: :noop + @object_types ["ChatMessage"] @spec persist(map(), keyword()) :: {:ok, Activity.t() | Object.t()} + def persist(%{"type" => type} = object, meta) when type in @object_types do + with {:ok, object} <- Object.create(object) do + {:ok, object, meta} + end + end + def persist(object, meta) do with local <- Keyword.fetch!(meta, :local), {recipients, _, _} <- get_recipients(object), diff --git a/lib/pleroma/web/activity_pub/object_validator.ex b/lib/pleroma/web/activity_pub/object_validator.ex index 03db681ec..a4da9242a 100644 --- a/lib/pleroma/web/activity_pub/object_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validator.ex @@ -23,7 +23,7 @@ def validate(%{"type" => "Like"} = object, meta) do object |> LikeValidator.cast_and_validate() |> Ecto.Changeset.apply_action(:insert) do - object = stringify_keys(object |> Map.from_struct()) + object = stringify_keys(object) {:ok, object, meta} end end @@ -41,7 +41,7 @@ def validate(%{"type" => "ChatMessage"} = object, meta) do def validate(%{"type" => "Create"} = object, meta) do with {:ok, object} <- object - |> CreateChatMessageValidator.cast_and_validate() + |> CreateChatMessageValidator.cast_and_validate(meta) |> Ecto.Changeset.apply_action(:insert) do object = stringify_keys(object) {:ok, object, meta} diff --git a/lib/pleroma/web/activity_pub/object_validators/chat_message_validator.ex b/lib/pleroma/web/activity_pub/object_validators/chat_message_validator.ex index f07045d9d..e87c1ac2e 100644 --- a/lib/pleroma/web/activity_pub/object_validators/chat_message_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validators/chat_message_validator.ex @@ -18,7 +18,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.ChatMessageValidator do field(:id, Types.ObjectID, primary_key: true) field(:to, Types.Recipients, default: []) field(:type, :string) - field(:content, :string) + field(:content, Types.SafeText) field(:actor, Types.ObjectID) field(:published, Types.DateTime) field(:emoji, :map, default: %{}) diff --git a/lib/pleroma/web/activity_pub/object_validators/create_chat_message_validator.ex b/lib/pleroma/web/activity_pub/object_validators/create_chat_message_validator.ex index ce52d5623..21c7a5ba4 100644 --- a/lib/pleroma/web/activity_pub/object_validators/create_chat_message_validator.ex +++ b/lib/pleroma/web/activity_pub/object_validators/create_chat_message_validator.ex @@ -33,8 +33,31 @@ def cast_data(data) do cast(%__MODULE__{}, data, __schema__(:fields)) end - # No validation yet - def cast_and_validate(data) do + def cast_and_validate(data, meta \\ []) do cast_data(data) + |> validate_data(meta) + end + + def validate_data(cng, meta \\ []) do + cng + |> validate_required([:id, :actor, :to, :type, :object]) + |> validate_inclusion(:type, ["Create"]) + |> validate_recipients_match(meta) + end + + def validate_recipients_match(cng, meta) do + object_recipients = meta[:object_data]["to"] || [] + + cng + |> validate_change(:to, fn :to, recipients -> + activity_set = MapSet.new(recipients) + object_set = MapSet.new(object_recipients) + + if MapSet.equal?(activity_set, object_set) do + [] + else + [{:to, "Recipients don't match with object recipients"}] + end + end) end end diff --git a/lib/pleroma/web/activity_pub/object_validators/types/safe_text.ex b/lib/pleroma/web/activity_pub/object_validators/types/safe_text.ex new file mode 100644 index 000000000..822e8d2c1 --- /dev/null +++ b/lib/pleroma/web/activity_pub/object_validators/types/safe_text.ex @@ -0,0 +1,25 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2020 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Web.ActivityPub.ObjectValidators.Types.SafeText do + use Ecto.Type + + alias Pleroma.HTML + + def type, do: :string + + def cast(str) when is_binary(str) do + {:ok, HTML.strip_tags(str)} + end + + def cast(_), do: :error + + def dump(data) do + {:ok, data} + end + + def load(data) do + {:ok, data} + end +end diff --git a/lib/pleroma/web/activity_pub/pipeline.ex b/lib/pleroma/web/activity_pub/pipeline.ex index 7ccee54c9..4213ba751 100644 --- a/lib/pleroma/web/activity_pub/pipeline.ex +++ b/lib/pleroma/web/activity_pub/pipeline.ex @@ -4,20 +4,22 @@ defmodule Pleroma.Web.ActivityPub.Pipeline do alias Pleroma.Activity + alias Pleroma.Object alias Pleroma.Web.ActivityPub.ActivityPub alias Pleroma.Web.ActivityPub.MRF alias Pleroma.Web.ActivityPub.ObjectValidator alias Pleroma.Web.ActivityPub.SideEffects alias Pleroma.Web.Federator - @spec common_pipeline(map(), keyword()) :: {:ok, Activity.t(), keyword()} | {:error, any()} + @spec common_pipeline(map(), keyword()) :: + {:ok, Activity.t() | Object.t(), keyword()} | {:error, any()} def common_pipeline(object, meta) do with {_, {:ok, validated_object, meta}} <- {:validate_object, ObjectValidator.validate(object, meta)}, {_, {:ok, mrfd_object}} <- {:mrf_object, MRF.filter(validated_object)}, - {_, {:ok, %Activity{} = activity, meta}} <- + {_, {:ok, activity, meta}} <- {:persist_object, ActivityPub.persist(mrfd_object, meta)}, - {_, {:ok, %Activity{} = activity, meta}} <- + {_, {:ok, activity, meta}} <- {:execute_side_effects, SideEffects.handle(activity, meta)}, {_, {:ok, _}} <- {:federation, maybe_federate(activity, meta)} do {:ok, activity, meta} @@ -27,7 +29,9 @@ def common_pipeline(object, meta) do end end - defp maybe_federate(activity, meta) do + defp maybe_federate(%Object{}, _), do: {:ok, :not_federated} + + defp maybe_federate(%Activity{} = activity, meta) do with {:ok, local} <- Keyword.fetch(meta, :local) do if local do Federator.publish(activity) diff --git a/lib/pleroma/web/activity_pub/side_effects.ex b/lib/pleroma/web/activity_pub/side_effects.ex index a2b4da8d6..794a46267 100644 --- a/lib/pleroma/web/activity_pub/side_effects.ex +++ b/lib/pleroma/web/activity_pub/side_effects.ex @@ -8,7 +8,9 @@ defmodule Pleroma.Web.ActivityPub.SideEffects do alias Pleroma.Chat alias Pleroma.Notification alias Pleroma.Object + alias Pleroma.Repo alias Pleroma.User + alias Pleroma.Web.ActivityPub.Pipeline alias Pleroma.Web.ActivityPub.Utils def handle(object, meta \\ []) @@ -30,14 +32,17 @@ def handle(%{data: %{"type" => "Like"}} = object, meta) do result end + # Tasks this handles + # - Actually create object + # - Rollback if we couldn't create it + # - Set up notifications def handle(%{data: %{"type" => "Create"}} = activity, meta) do - object = Object.normalize(activity, false) - - {:ok, _object} = handle_object_creation(object) - - Notification.create_notifications(activity) - - {:ok, activity, meta} + with {:ok, _object, _meta} <- handle_object_creation(meta[:object_data], meta) do + Notification.create_notifications(activity) + {:ok, activity, meta} + else + e -> Repo.rollback(e) + end end # Nothing to do @@ -45,18 +50,20 @@ def handle(object, meta) do {:ok, object, meta} end - def handle_object_creation(%{data: %{"type" => "ChatMessage"}} = object) do - actor = User.get_cached_by_ap_id(object.data["actor"]) - recipient = User.get_cached_by_ap_id(hd(object.data["to"])) + def handle_object_creation(%{"type" => "ChatMessage"} = object, meta) do + with {:ok, object, meta} <- Pipeline.common_pipeline(object, meta) do + actor = User.get_cached_by_ap_id(object.data["actor"]) + recipient = User.get_cached_by_ap_id(hd(object.data["to"])) - [[actor, recipient], [recipient, actor]] - |> Enum.each(fn [user, other_user] -> - if user.local do - Chat.bump_or_create(user.id, other_user.ap_id) - end - end) + [[actor, recipient], [recipient, actor]] + |> Enum.each(fn [user, other_user] -> + if user.local do + Chat.bump_or_create(user.id, other_user.ap_id) + end + end) - {:ok, object} + {:ok, object, meta} + end end # Nothing to do diff --git a/lib/pleroma/web/activity_pub/transmogrifier/chat_message_handling.ex b/lib/pleroma/web/activity_pub/transmogrifier/chat_message_handling.ex index cfe3b767b..043d847d1 100644 --- a/lib/pleroma/web/activity_pub/transmogrifier/chat_message_handling.ex +++ b/lib/pleroma/web/activity_pub/transmogrifier/chat_message_handling.ex @@ -3,31 +3,39 @@ # SPDX-License-Identifier: AGPL-3.0-only defmodule Pleroma.Web.ActivityPub.Transmogrifier.ChatMessageHandling do - alias Pleroma.Object + alias Pleroma.Repo alias Pleroma.Web.ActivityPub.ObjectValidator alias Pleroma.Web.ActivityPub.ObjectValidators.ChatMessageValidator alias Pleroma.Web.ActivityPub.ObjectValidators.CreateChatMessageValidator alias Pleroma.Web.ActivityPub.Pipeline def handle_incoming( - %{"type" => "Create", "object" => %{"type" => "ChatMessage"} = object_data} = data, + %{"type" => "Create", "object" => %{"type" => "ChatMessage"}} = data, _options ) do + # Create has to be run inside a transaction because the object is created as a side effect. + # If this does not work, we need to roll back creating the activity. + case Repo.transaction(fn -> do_handle_incoming(data) end) do + {:ok, value} -> + value + + {:error, e} -> + {:error, e} + end + end + + def do_handle_incoming( + %{"type" => "Create", "object" => %{"type" => "ChatMessage"} = object_data} = data + ) do with {_, {:ok, cast_data_sym}} <- {:casting_data, data |> CreateChatMessageValidator.cast_and_apply()}, cast_data = ObjectValidator.stringify_keys(cast_data_sym), {_, {:ok, object_cast_data_sym}} <- {:casting_object_data, object_data |> ChatMessageValidator.cast_and_apply()}, object_cast_data = ObjectValidator.stringify_keys(object_cast_data_sym), - # For now, just strip HTML - stripped_content = Pleroma.HTML.strip_tags(object_cast_data["content"]), - object_cast_data = object_cast_data |> Map.put("content", stripped_content), - {_, true} <- {:to_fields_match, cast_data["to"] == object_cast_data["to"]}, - {_, {:ok, validated_object, _meta}} <- - {:validate_object, ObjectValidator.validate(object_cast_data, %{})}, - {_, {:ok, _created_object}} <- {:persist_object, Object.create(validated_object)}, {_, {:ok, activity, _meta}} <- - {:common_pipeline, Pipeline.common_pipeline(cast_data, local: false)} do + {:common_pipeline, + Pipeline.common_pipeline(cast_data, local: false, object_data: object_cast_data)} do {:ok, activity} else e -> diff --git a/lib/pleroma/web/common_api/common_api.ex b/lib/pleroma/web/common_api/common_api.ex index 5eb221668..c39d1cee6 100644 --- a/lib/pleroma/web/common_api/common_api.ex +++ b/lib/pleroma/web/common_api/common_api.ex @@ -38,13 +38,15 @@ def post_chat_message(%User{} = user, %User{} = recipient, content) do recipient.ap_id, content |> Formatter.html_escape("text/plain") )}, - {_, {:ok, chat_message_object}} <- - {:create_object, Object.create(chat_message_data)}, {_, {:ok, create_activity_data, _meta}} <- {:build_create_activity, - Builder.create(user, chat_message_object.data["id"], [recipient.ap_id])}, + Builder.create(user, chat_message_data["id"], [recipient.ap_id])}, {_, {:ok, %Activity{} = activity, _meta}} <- - {:common_pipeline, Pipeline.common_pipeline(create_activity_data, local: true)} do + {:common_pipeline, + Pipeline.common_pipeline(create_activity_data, + local: true, + object_data: chat_message_data + )} do {:ok, activity} else {:content_length, false} -> {:error, :content_too_long} diff --git a/lib/pleroma/web/common_api/utils.ex b/lib/pleroma/web/common_api/utils.ex index 945e63e22..4afdf80af 100644 --- a/lib/pleroma/web/common_api/utils.ex +++ b/lib/pleroma/web/common_api/utils.ex @@ -425,7 +425,7 @@ def maybe_notify_mentioned_recipients( %Activity{data: %{"to" => _to, "type" => type} = data} = activity ) when type == "Create" do - object = Object.normalize(activity) + object = Object.normalize(activity, false) object_data = cond do diff --git a/test/web/activity_pub/object_validators/types/safe_text_test.exs b/test/web/activity_pub/object_validators/types/safe_text_test.exs new file mode 100644 index 000000000..59ed0a1fe --- /dev/null +++ b/test/web/activity_pub/object_validators/types/safe_text_test.exs @@ -0,0 +1,23 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2020 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Web.ActivityPub.ObjectValidators.Types.SafeTextTest do + use Pleroma.DataCase + + alias Pleroma.Web.ActivityPub.ObjectValidators.Types.SafeText + + test "it lets normal text go through" do + text = "hey how are you" + assert {:ok, text} == SafeText.cast(text) + end + + test "it removes html tags from text" do + text = "hey look xss " + assert {:ok, "hey look xss alert('foo')"} == SafeText.cast(text) + end + + test "errors for non-text" do + assert :error == SafeText.cast(1) + end +end diff --git a/test/web/activity_pub/side_effects_test.exs b/test/web/activity_pub/side_effects_test.exs index 2889a577c..19abac6a6 100644 --- a/test/web/activity_pub/side_effects_test.exs +++ b/test/web/activity_pub/side_effects_test.exs @@ -47,14 +47,14 @@ test "notifies the recipient" do recipient = insert(:user, local: true) {:ok, chat_message_data, _meta} = Builder.chat_message(author, recipient.ap_id, "hey") - {:ok, chat_message_object} = Object.create(chat_message_data) {:ok, create_activity_data, _meta} = - Builder.create(author, chat_message_object.data["id"], [recipient.ap_id]) + Builder.create(author, chat_message_data["id"], [recipient.ap_id]) {:ok, create_activity, _meta} = ActivityPub.persist(create_activity_data, local: false) - {:ok, _create_activity, _meta} = SideEffects.handle(create_activity) + {:ok, _create_activity, _meta} = + SideEffects.handle(create_activity, local: false, object_data: chat_message_data) assert Repo.get_by(Notification, user_id: recipient.id, activity_id: create_activity.id) end @@ -64,14 +64,17 @@ test "it creates a Chat for the local users and bumps the unread count" do recipient = insert(:user, local: true) {:ok, chat_message_data, _meta} = Builder.chat_message(author, recipient.ap_id, "hey") - {:ok, chat_message_object} = Object.create(chat_message_data) {:ok, create_activity_data, _meta} = - Builder.create(author, chat_message_object.data["id"], [recipient.ap_id]) + Builder.create(author, chat_message_data["id"], [recipient.ap_id]) {:ok, create_activity, _meta} = ActivityPub.persist(create_activity_data, local: false) - {:ok, _create_activity, _meta} = SideEffects.handle(create_activity) + {:ok, _create_activity, _meta} = + SideEffects.handle(create_activity, local: false, object_data: chat_message_data) + + # An object is created + assert Object.get_by_ap_id(chat_message_data["id"]) # The remote user won't get a chat chat = Chat.get(author.id, recipient.ap_id) @@ -85,14 +88,14 @@ test "it creates a Chat for the local users and bumps the unread count" do recipient = insert(:user, local: true) {:ok, chat_message_data, _meta} = Builder.chat_message(author, recipient.ap_id, "hey") - {:ok, chat_message_object} = Object.create(chat_message_data) {:ok, create_activity_data, _meta} = - Builder.create(author, chat_message_object.data["id"], [recipient.ap_id]) + Builder.create(author, chat_message_data["id"], [recipient.ap_id]) {:ok, create_activity, _meta} = ActivityPub.persist(create_activity_data, local: false) - {:ok, _create_activity, _meta} = SideEffects.handle(create_activity) + {:ok, _create_activity, _meta} = + SideEffects.handle(create_activity, local: false, object_data: chat_message_data) # Both users are local and get the chat chat = Chat.get(author.id, recipient.ap_id) diff --git a/test/web/activity_pub/transmogrifier/chat_message_test.exs b/test/web/activity_pub/transmogrifier/chat_message_test.exs index a63a31e6e..ceaee614c 100644 --- a/test/web/activity_pub/transmogrifier/chat_message_test.exs +++ b/test/web/activity_pub/transmogrifier/chat_message_test.exs @@ -55,7 +55,8 @@ test "it rejects messages where the `to` field of activity and object don't matc data |> Map.put("to", author.ap_id) - {:error, _} = Transmogrifier.handle_incoming(data) + assert match?({:error, _}, Transmogrifier.handle_incoming(data)) + refute Object.get_by_ap_id(data["object"]["id"]) end test "it inserts it and creates a chat" do