From 9a91299f96c4c405cfdb8b8f78d0906b1cf98001 Mon Sep 17 00:00:00 2001 From: Oneric Date: Thu, 25 Apr 2024 18:16:21 +0200 Subject: [PATCH] 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 | 1 + lib/pleroma/constants.ex | 3 +++ lib/pleroma/web/common_api/utils.ex | 9 +++++-- .../controllers/media_controller.ex | 11 +++++++-- test/pleroma/web/common_api/utils_test.exs | 8 ++++++- .../controllers/media_controller_test.exs | 24 +++++++++++++++++++ 6 files changed, 51 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 48e487bf1..edd51b74b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/pleroma/constants.ex b/lib/pleroma/constants.ex index 94608a99b..2681d7671 100644 --- a/lib/pleroma/constants.ex +++ b/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 diff --git a/lib/pleroma/web/common_api/utils.ex b/lib/pleroma/web/common_api/utils.ex index 6d2113100..635143621 100644 --- a/lib/pleroma/web/common_api/utils.ex +++ b/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())} diff --git a/lib/pleroma/web/mastodon_api/controllers/media_controller.ex b/lib/pleroma/web/mastodon_api/controllers/media_controller.ex index 5918b288d..3e5a64b51 100644 --- a/lib/pleroma/web/mastodon_api/controllers/media_controller.ex +++ b/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 diff --git a/test/pleroma/web/common_api/utils_test.exs b/test/pleroma/web/common_api/utils_test.exs index 10abb89e5..3d639d389 100644 --- a/test/pleroma/web/common_api/utils_test.exs +++ b/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, %{}) == [] diff --git a/test/pleroma/web/mastodon_api/controllers/media_controller_test.exs b/test/pleroma/web/mastodon_api/controllers/media_controller_test.exs index 7b5f5850d..a224f063b 100644 --- a/test/pleroma/web/mastodon_api/controllers/media_controller_test.exs +++ b/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"])