Don't try to handle non-media objects as media

Trying to display non-media as media crashed the renderer,
but when posting a status with a valid, non-media object id
the post was still created, but then crashed e.g. timeline rendering.
It also crashed C2S inbox reads, so this could not be used to leak
private posts.

CHANGELOG.md

@@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ## Fixed
 - Issue allowing non-owners to use media objects in posts
+- Issue allowing use of non-media objects as attachments and crashing timeline rendering
 
 ## 2024.04
 

lib/pleroma/constants.ex

@@ -64,4 +64,7 @@ defmodule Pleroma.Constants do
       "Service"
     ]
   )
+
+  # Internally used as top-level types for media attachments and user images
+  const(attachment_types, do: ["Document", "Image"])
 end

lib/pleroma/web/common_api/utils.ex

@@ -40,8 +40,13 @@ defp attachments_from_ids(user, [media_id | ids], acc) do
     end
   end
 
-  defp get_attachment(media_id) do
-    Repo.get(Object, media_id)
+  def get_attachment(media_id) do
+    with %Object{} = object <- Repo.get(Object, media_id),
+         true <- object.data["type"] in Pleroma.Constants.attachment_types() do
+      object
+    else
+      _ -> nil
+    end
   end
 
   @spec get_to_and_cc(ActivityDraft.t()) :: {list(String.t()), list(String.t())}

lib/pleroma/web/mastodon_api/controllers/media_controller.ex

@@ -8,6 +8,7 @@ defmodule Pleroma.Web.MastodonAPI.MediaController do
   alias Pleroma.Object
   alias Pleroma.User
   alias Pleroma.Web.ActivityPub.ActivityPub
+  alias Pleroma.Web.CommonAPI.Utils
   alias Pleroma.Web.Plugs.OAuthScopesPlug
 
   action_fallback(Pleroma.Web.MastodonAPI.FallbackController)
@@ -55,12 +56,15 @@ def create2(_conn, _data), do: {:error, :bad_request}
 
   @doc "PUT /api/v1/media/:id"
   def update(%{assigns: %{user: user}, body_params: %{description: description}} = conn, %{id: id}) do
-    with %Object{} = object <- Object.get_by_id(id),
+    with {_, %Object{} = object} <- {:get, Utils.get_attachment(id)},
          :ok <- Object.authorize_access(object, user),
          {:ok, %Object{data: data}} <- Object.update_data(object, %{"name" => description}) do
       attachment_data = Map.put(data, "id", object.id)
 
       render(conn, "attachment.json", %{attachment: attachment_data})
+    else
+      {:get, _} -> {:error, :not_found}
+      e -> e
     end
   end
 
@@ -68,11 +72,14 @@ def update(conn, data), do: show(conn, data)
 
   @doc "GET /api/v1/media/:id"
   def show(%{assigns: %{user: user}} = conn, %{id: id}) do
-    with %Object{data: data, id: object_id} = object <- Object.get_by_id(id),
+    with {_, %Object{data: data, id: object_id} = object} <- {:get, Utils.get_attachment(id)},
          :ok <- Object.authorize_access(object, user) do
       attachment_data = Map.put(data, "id", object_id)
 
       render(conn, "attachment.json", %{attachment: attachment_data})
+    else
+      {:get, _} -> {:error, :not_found}
+      e -> e
     end
   end
 

test/pleroma/web/common_api/utils_test.exs

@@ -593,10 +593,16 @@ test "returns recipients when object not found" do
   describe "attachments_from_ids/1" do
     test "returns attachments without descs" do
       user = insert(:user)
-      object = insert(:note, user: user)
+      object = insert(:attachment, user: user)
       assert Utils.attachments_from_ids(user, %{media_ids: ["#{object.id}"]}) == [object.data]
     end
 
+    test "returns [] when passed non-media object ids" do
+      user = insert(:user)
+      object = insert(:note, user: user)
+      assert Utils.attachments_from_ids(user, %{media_ids: ["#{object.id}"]}) == []
+    end
+
     test "returns [] when not pass media_ids" do
       user = insert(:user)
       assert Utils.attachments_from_ids(user, %{}) == []

test/pleroma/web/mastodon_api/controllers/media_controller_test.exs

@@ -6,6 +6,7 @@ defmodule Pleroma.Web.MastodonAPI.MediaControllerTest do
   use Pleroma.Web.ConnCase, async: false
 
   import ExUnit.CaptureLog
+  import Pleroma.Factory
 
   alias Pleroma.Object
   alias Pleroma.User
@@ -174,6 +175,18 @@ test "/api/v1/media/:id good request", %{conn: conn, object: object} do
       assert media["description"] == "test-media"
       assert refresh_record(object).data["name"] == "test-media"
     end
+
+    test "won't update non-media", %{conn: conn, user: user} do
+      object = insert(:note, user: user)
+
+      response =
+        conn
+        |> put_req_header("content-type", "multipart/form-data")
+        |> put("/api/v1/media/#{object.id}", %{"description" => "test-media"})
+        |> json_response(404)
+
+      assert response == %{"error" => "Record not found"}
+    end
   end
 
   describe "Get media by id (/api/v1/media/:id)" do
@@ -207,6 +220,17 @@ test "it returns media object when requested by owner", %{conn: conn, object: ob
       assert media["id"]
     end
 
+    test "it returns 404 when requesting non-media object", %{conn: conn, user: user} do
+      object = insert(:note, user: user)
+
+      response =
+        conn
+        |> get("/api/v1/media/#{object.id}")
+        |> json_response(404)
+
+      assert response == %{"error" => "Record not found"}
+    end
+
     test "it returns 403 if media object requested by non-owner", %{object: object, user: user} do
       %{conn: conn, user: other_user} = oauth_access(["read:media"])