diff --git a/CHANGELOG.md b/CHANGELOG.md index 2284f5c8d..48e487bf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## Unreleased +## Fixed +- Issue allowing non-owners to use media objects in posts + ## 2024.04 ## Added diff --git a/lib/pleroma/scheduled_activity.ex b/lib/pleroma/scheduled_activity.ex index 3e1c51abb..5292b1491 100644 --- a/lib/pleroma/scheduled_activity.ex +++ b/lib/pleroma/scheduled_activity.ex @@ -40,19 +40,29 @@ defp with_media_attachments( %{changes: %{params: %{"media_ids" => media_ids} = params}} = changeset ) when is_list(media_ids) do - media_attachments = Utils.attachments_from_ids(%{media_ids: media_ids}) + user = User.get_by_id(changeset.data.user_id) - params = - params - |> Map.put("media_attachments", media_attachments) - |> Map.put("media_ids", media_ids) + case Utils.attachments_from_ids(user, %{media_ids: media_ids}) do + media_attachments when is_list(media_attachments) -> + params = + params + |> Map.put("media_attachments", media_attachments) + |> Map.put("media_ids", media_ids) - put_change(changeset, :params, params) + put_change(changeset, :params, params) + + {:error, _} = e -> + e + + e -> + {:error, e} + end end defp with_media_attachments(changeset), do: changeset defp update_changeset(%ScheduledActivity{} = scheduled_activity, attrs) do + # note: should this ever allow swapping media attachments, make sure ownership is checked scheduled_activity |> cast(attrs, [:scheduled_at]) |> validate_required([:scheduled_at]) @@ -115,13 +125,22 @@ defp new(%User{} = user, attrs) do @doc """ Creates ScheduledActivity and add to queue to perform at scheduled_at date """ - @spec create(User.t(), map()) :: {:ok, ScheduledActivity.t()} | {:error, Ecto.Changeset.t()} + @spec create(User.t(), map()) :: {:ok, ScheduledActivity.t()} | {:error, any()} def create(%User{} = user, attrs) do - Multi.new() - |> Multi.insert(:scheduled_activity, new(user, attrs)) - |> maybe_add_jobs(Config.get([ScheduledActivity, :enabled])) - |> Repo.transaction() - |> transaction_response + case new(user, attrs) do + %Ecto.Changeset{} = sched_data -> + Multi.new() + |> Multi.insert(:scheduled_activity, sched_data) + |> maybe_add_jobs(Config.get([ScheduledActivity, :enabled])) + |> Repo.transaction() + |> transaction_response + + {:error, _} = e -> + e + + e -> + {:error, e} + end end defp maybe_add_jobs(multi, true) do diff --git a/lib/pleroma/web/common_api/activity_draft.ex b/lib/pleroma/web/common_api/activity_draft.ex index 4555efd61..2d24edec4 100644 --- a/lib/pleroma/web/common_api/activity_draft.ex +++ b/lib/pleroma/web/common_api/activity_draft.ex @@ -92,9 +92,14 @@ defp full_payload(%{status: status, summary: summary} = draft) do end end - defp attachments(%{params: params} = draft) do - attachments = Utils.attachments_from_ids(params) - %__MODULE__{draft | attachments: attachments} + defp attachments(%{params: params, user: user} = draft) do + case Utils.attachments_from_ids(user, params) do + attachments when is_list(attachments) -> + %__MODULE__{draft | attachments: attachments} + + {:error, reason} -> + add_error(draft, reason) + end end defp in_reply_to(%{params: %{in_reply_to_status_id: ""}} = draft), do: draft diff --git a/lib/pleroma/web/common_api/utils.ex b/lib/pleroma/web/common_api/utils.ex index d80109a98..6d2113100 100644 --- a/lib/pleroma/web/common_api/utils.ex +++ b/lib/pleroma/web/common_api/utils.ex @@ -22,24 +22,24 @@ defmodule Pleroma.Web.CommonAPI.Utils do require Logger require Pleroma.Constants - def attachments_from_ids(%{media_ids: ids}) do - attachments_from_ids(ids) + def attachments_from_ids(user, %{media_ids: ids}) do + attachments_from_ids(user, ids, []) end - def attachments_from_ids([]), do: [] + def attachments_from_ids(_, _), do: [] - def attachments_from_ids(ids) when is_list(ids) do - Enum.map(ids, fn media_id -> - case get_attachment(media_id) do - %Object{data: data} -> data - _ -> nil - end - end) - |> Enum.reject(&is_nil/1) + defp attachments_from_ids(_user, [], acc), do: Enum.reverse(acc) + + defp attachments_from_ids(user, [media_id | ids], acc) do + with {_, %Object{} = object} <- {:get, get_attachment(media_id)}, + :ok <- Object.authorize_access(object, user) do + attachments_from_ids(user, ids, [object.data | acc]) + else + {:get, _} -> attachments_from_ids(user, ids, acc) + {:error, reason} -> {:error, reason} + end end - def attachments_from_ids(_), do: [] - defp get_attachment(media_id) do Repo.get(Object, media_id) end diff --git a/test/pleroma/web/common_api/utils_test.exs b/test/pleroma/web/common_api/utils_test.exs index d98eb76ad..10abb89e5 100644 --- a/test/pleroma/web/common_api/utils_test.exs +++ b/test/pleroma/web/common_api/utils_test.exs @@ -592,12 +592,14 @@ test "returns recipients when object not found" do describe "attachments_from_ids/1" do test "returns attachments without descs" do - object = insert(:note) - assert Utils.attachments_from_ids(%{media_ids: ["#{object.id}"]}) == [object.data] + user = insert(:user) + object = insert(:note, user: user) + assert Utils.attachments_from_ids(user, %{media_ids: ["#{object.id}"]}) == [object.data] end test "returns [] when not pass media_ids" do - assert Utils.attachments_from_ids(%{}) == [] + user = insert(:user) + assert Utils.attachments_from_ids(user, %{}) == [] end end diff --git a/test/pleroma/web/mastodon_api/controllers/status_controller_test.exs b/test/pleroma/web/mastodon_api/controllers/status_controller_test.exs index f58045640..a15fd42fc 100644 --- a/test/pleroma/web/mastodon_api/controllers/status_controller_test.exs +++ b/test/pleroma/web/mastodon_api/controllers/status_controller_test.exs @@ -220,6 +220,28 @@ test "posting an undefined status with an attachment", %{user: user, conn: conn} assert json_response_and_validate_schema(conn, 200) end + test "refuses to post non-owned media", %{conn: conn} do + other_user = insert(:user) + + file = %Plug.Upload{ + content_type: "image/jpeg", + path: Path.absname("test/fixtures/image.jpg"), + filename: "an_image.jpg" + } + + {:ok, upload} = ActivityPub.upload(file, actor: other_user.ap_id) + + conn = + conn + |> put_req_header("content-type", "application/json") + |> post("/api/v1/statuses", %{ + "status" => "mew", + "media_ids" => [to_string(upload.id)] + }) + + assert json_response(conn, 422) == %{"error" => "forbidden"} + end + test "posting a status with an invalid language", %{conn: conn} do conn = conn @@ -569,6 +591,29 @@ test "creates a scheduled activity with a media attachment", %{user: user, conn: assert %{"type" => "image"} = media_attachment end + test "refuses to schedule post with non-owned media", %{conn: conn} do + other_user = insert(:user) + + file = %Plug.Upload{ + content_type: "image/jpeg", + path: Path.absname("test/fixtures/image.jpg"), + filename: "an_image.jpg" + } + + {:ok, upload} = ActivityPub.upload(file, actor: other_user.ap_id) + + conn = + conn + |> put_req_header("content-type", "application/json") + |> post("/api/v1/statuses", %{ + "status" => "mew", + "scheduled_at" => DateTime.add(DateTime.utc_now(), 6, :minute), + "media_ids" => [to_string(upload.id)] + }) + + assert json_response(conn, 403) == %{"error" => "Access denied"} + end + test "skips the scheduling and creates the activity if scheduled_at is earlier than 5 minutes from now", %{conn: conn} do scheduled_at = @@ -2406,6 +2451,25 @@ test "it updates the attachments", %{conn: conn, user: user} do assert [%{"id" => ^attachment_id}] = response["media_attachments"] end + test "it does not update to non-owned attachments", %{conn: conn, user: user} do + other_user = insert(:user) + attachment = insert(:attachment, user: other_user) + attachment_id = to_string(attachment.id) + + {:ok, activity} = CommonAPI.post(user, %{status: "mew mew #abc", spoiler_text: "#def"}) + + conn = + conn + |> put_req_header("content-type", "application/json") + |> put("/api/v1/statuses/#{activity.id}", %{ + "status" => "mew mew #abc", + "spoiler_text" => "#def", + "media_ids" => [attachment_id] + }) + + assert json_response(conn, 400) == %{"error" => "internal_server_error"} + end + test "it does not update visibility", %{conn: conn, user: user} do {:ok, activity} = CommonAPI.post(user, %{ diff --git a/test/pleroma/web/mastodon_api/views/scheduled_activity_view_test.exs b/test/pleroma/web/mastodon_api/views/scheduled_activity_view_test.exs index e323f3a1f..cc893f94e 100644 --- a/test/pleroma/web/mastodon_api/views/scheduled_activity_view_test.exs +++ b/test/pleroma/web/mastodon_api/views/scheduled_activity_view_test.exs @@ -47,8 +47,7 @@ test "A scheduled activity with a media attachment" do expected = %{ id: to_string(scheduled_activity.id), media_attachments: - %{media_ids: [upload.id]} - |> Utils.attachments_from_ids() + Utils.attachments_from_ids(user, %{media_ids: [upload.id]}) |> Enum.map(&StatusView.render("attachment.json", %{attachment: &1})), params: %{ in_reply_to_id: to_string(activity.id),