ChatMessages: Better validation.

lib/pleroma/web/activity_pub/object_validator.ex

@@ -31,7 +31,8 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidator do
   def validate(%{"type" => "ChatMessage"} = object, meta) do
     with {:ok, object} <-
            object
-           |> ChatMessageValidator.cast_and_apply() do
+           |> ChatMessageValidator.cast_and_validate()
+           |> Ecto.Changeset.apply_action(:insert) do
       object = stringify_keys(object)
       {:ok, object, meta}
     end
@@ -40,7 +41,8 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidator do
   def validate(%{"type" => "Create"} = object, meta) do
     with {:ok, object} <-
            object
-           |> CreateChatMessageValidator.cast_and_apply() do
+           |> CreateChatMessageValidator.cast_and_validate()
+           |> Ecto.Changeset.apply_action(:insert) do
       object = stringify_keys(object)
       {:ok, object, meta}
     end

lib/pleroma/web/activity_pub/object_validators/chat_message_validator.ex

@@ -6,6 +6,7 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.ChatMessageValidator do
   use Ecto.Schema
 
   alias Pleroma.Web.ActivityPub.ObjectValidators.Types
+  alias Pleroma.User
 
   import Ecto.Changeset
 
@@ -54,5 +55,30 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.ChatMessageValidator do
     data_cng
     |> validate_inclusion(:type, ["ChatMessage"])
     |> validate_required([:id, :actor, :to, :type, :content])
+    |> validate_length(:to, is: 1)
+    |> validate_local_concern()
+  end
+
+  @doc "Validates if at least one of the users in this ChatMessage is a local user, otherwise we don't want the message in our system. It also validates the presence of both users in our system."
+  def validate_local_concern(cng) do
+    with actor_ap <- get_field(cng, :actor),
+         {_, %User{} = actor} <- {:find_actor, User.get_cached_by_ap_id(actor_ap)},
+         {_, %User{} = recipient} <-
+           {:find_recipient, User.get_cached_by_ap_id(get_field(cng, :to) |> hd())},
+         {_, true} <- {:local?, Enum.any?([actor, recipient], & &1.local)} do
+      cng
+    else
+      {:local?, false} ->
+        cng
+        |> add_error(:actor, "actor and recipient are both remote")
+
+      {:find_actor, _} ->
+        cng
+        |> add_error(:actor, "can't find user")
+
+      {:find_recipient, _} ->
+        cng
+        |> add_error(:to, "can't find user")
+    end
   end
 end

lib/pleroma/web/activity_pub/object_validators/common_validations.ex

@@ -8,7 +8,11 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.CommonValidations do
   alias Pleroma.Object
   alias Pleroma.User
 
-  def validate_actor_presence(cng, field_name \\ :actor) do
+  def validate_actor_presence(cng) do
+    validate_user_presence(cng, :actor)
+  end
+
+  def validate_user_presence(cng, field_name) do
     cng
     |> validate_change(field_name, fn field_name, actor ->
       if User.get_cached_by_ap_id(actor) do

lib/pleroma/web/activity_pub/object_validators/create_chat_message_validator.ex

@@ -32,4 +32,9 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidators.CreateChatMessageValidator do
   def cast_data(data) do
     cast(%__MODULE__{}, data, __schema__(:fields))
   end
+
+  # No validation yet
+  def cast_and_validate(data) do
+    cast_data(data)
+  end
 end

lib/pleroma/web/activity_pub/transmogrifier/chat_message_handling.ex

@@ -25,6 +25,9 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.ChatMessageHandling do
          {_, {:ok, activity, _meta}} <-
            {:common_pipeline, Pipeline.common_pipeline(cast_data, local: false)} do
       {:ok, activity}
+    else
+      e ->
+        {:error, e}
     end
   end
 end

test/fixtures/create-chat-message.json

@@ -3,7 +3,7 @@
     "id": "http://2hu.gensokyo/objects/1",
     "object": {
         "attributedTo": "http://2hu.gensokyo/users/raymoo",
-        "content": "You expected a cute girl? Too bad.",
+        "content": "You expected a cute girl? Too bad. <script>alert('XSS')</script>",
         "id": "http://2hu.gensokyo/objects/2",
         "published": "2020-02-12T14:08:20Z",
         "to": [

test/web/activity_pub/object_validator_test.exs

@@ -5,9 +5,61 @@ defmodule Pleroma.Web.ActivityPub.ObjectValidatorTest do
   alias Pleroma.Web.ActivityPub.ObjectValidators.LikeValidator
   alias Pleroma.Web.ActivityPub.Utils
   alias Pleroma.Web.CommonAPI
+  alias Pleroma.Web.ActivityPub.Builder
 
   import Pleroma.Factory
 
+  describe "chat messages" do
+    setup do
+      user = insert(:user)
+      recipient = insert(:user, local: false)
+
+      {:ok, valid_chat_message, _} = Builder.chat_message(user, recipient.ap_id, "hey")
+
+      %{user: user, recipient: recipient, valid_chat_message: valid_chat_message}
+    end
+
+    test "validates for a basic object we build", %{valid_chat_message: valid_chat_message} do
+      assert {:ok, _object, _meta} = ObjectValidator.validate(valid_chat_message, [])
+    end
+
+    test "does not validate if the actor or the recipient is not in our system", %{
+      valid_chat_message: valid_chat_message
+    } do
+      chat_message =
+        valid_chat_message
+        |> Map.put("actor", "https://raymoo.com/raymoo")
+
+      {:error, _} = ObjectValidator.validate(chat_message, [])
+
+      chat_message =
+        valid_chat_message
+        |> Map.put("to", ["https://raymoo.com/raymoo"])
+
+      {:error, _} = ObjectValidator.validate(chat_message, [])
+    end
+
+    test "does not validate for a message with multiple recipients", %{
+      valid_chat_message: valid_chat_message,
+      user: user,
+      recipient: recipient
+    } do
+      chat_message =
+        valid_chat_message
+        |> Map.put("to", [user.ap_id, recipient.ap_id])
+
+      assert {:error, _} = ObjectValidator.validate(chat_message, [])
+    end
+
+    test "does not validate if it doesn't concern local users" do
+      user = insert(:user, local: false)
+      recipient = insert(:user, local: false)
+
+      {:ok, valid_chat_message, _} = Builder.chat_message(user, recipient.ap_id, "hey")
+      assert {:error, _} = ObjectValidator.validate(valid_chat_message, [])
+    end
+  end
+
   describe "likes" do
     setup do
       user = insert(:user)

test/web/activity_pub/transmogrifier/chat_message_test.exs

@@ -12,13 +12,43 @@ defmodule Pleroma.Web.ActivityPub.Transmogrifier.ChatMessageTest do
   alias Pleroma.Web.ActivityPub.Transmogrifier
 
   describe "handle_incoming" do
-    test "it insert it" do
+    test "it rejects messages that don't contain content" do
+      data =
+        File.read!("test/fixtures/create-chat-message.json")
+        |> Poison.decode!()
+
+      object =
+        data["object"]
+        |> Map.delete("content")
+
+      data =
+        data
+        |> Map.put("object", object)
+
+      _author = insert(:user, ap_id: data["actor"], local: false)
+      _recipient = insert(:user, ap_id: List.first(data["to"]), local: true)
+
+      {:error, _} = Transmogrifier.handle_incoming(data)
+    end
+
+    test "it rejects messages that don't concern local users" do
+      data =
+        File.read!("test/fixtures/create-chat-message.json")
+        |> Poison.decode!()
+
+      _author = insert(:user, ap_id: data["actor"], local: false)
+      _recipient = insert(:user, ap_id: List.first(data["to"]), local: false)
+
+      {:error, _} = Transmogrifier.handle_incoming(data)
+    end
+
+    test "it inserts it and creates a chat" do
       data =
         File.read!("test/fixtures/create-chat-message.json")
         |> Poison.decode!()
 
       author = insert(:user, ap_id: data["actor"], local: false)
-      recipient = insert(:user, ap_id: List.first(data["to"]), local: false)
+      recipient = insert(:user, ap_id: List.first(data["to"]), local: true)
 
       {:ok, %Activity{} = activity} = Transmogrifier.handle_incoming(data)