Merge branch 'check-attachment-attribution' into 'develop'

Prevent users from attaching other users' attachments

See merge request pleroma/pleroma!3947
This commit is contained in:
Haelwenn 2023-09-03 09:09:27 +00:00
commit a94cf2ad4f
9 changed files with 82 additions and 31 deletions

View file

@ -0,0 +1 @@
CommonAPI: Prevent users from accessing media of other users by creating a status with reused attachment ID

View file

@ -40,7 +40,11 @@ defmodule Pleroma.ScheduledActivity do
%{changes: %{params: %{"media_ids" => media_ids} = params}} = changeset %{changes: %{params: %{"media_ids" => media_ids} = params}} = changeset
) )
when is_list(media_ids) do when is_list(media_ids) do
media_attachments = Utils.attachments_from_ids(%{media_ids: media_ids}) media_attachments =
Utils.attachments_from_ids(
%{media_ids: media_ids},
User.get_cached_by_id(changeset.data.user_id)
)
params = params =
params params

View file

@ -33,6 +33,7 @@ defmodule Pleroma.Web.CommonAPI do
def post_chat_message(%User{} = user, %User{} = recipient, content, opts \\ []) do def post_chat_message(%User{} = user, %User{} = recipient, content, opts \\ []) do
with maybe_attachment <- opts[:media_id] && Object.get_by_id(opts[:media_id]), with maybe_attachment <- opts[:media_id] && Object.get_by_id(opts[:media_id]),
:ok <- validate_chat_attachment_attribution(maybe_attachment, user),
:ok <- validate_chat_content_length(content, !!maybe_attachment), :ok <- validate_chat_content_length(content, !!maybe_attachment),
{_, {:ok, chat_message_data, _meta}} <- {_, {:ok, chat_message_data, _meta}} <-
{:build_object, {:build_object,
@ -71,6 +72,17 @@ defmodule Pleroma.Web.CommonAPI do
text text
end end
defp validate_chat_attachment_attribution(nil, _), do: :ok
defp validate_chat_attachment_attribution(attachment, user) do
with :ok <- Object.authorize_access(attachment, user) do
:ok
else
e ->
e
end
end
defp validate_chat_content_length(_, true), do: :ok defp validate_chat_content_length(_, true), do: :ok
defp validate_chat_content_length(nil, false), do: {:error, :no_content} defp validate_chat_content_length(nil, false), do: {:error, :no_content}

View file

@ -111,7 +111,7 @@ defmodule Pleroma.Web.CommonAPI.ActivityDraft do
end end
defp attachments(%{params: params} = draft) do defp attachments(%{params: params} = draft) do
attachments = Utils.attachments_from_ids(params) attachments = Utils.attachments_from_ids(params, draft.user)
draft = %__MODULE__{draft | attachments: attachments} draft = %__MODULE__{draft | attachments: attachments}
case Utils.validate_attachments_count(attachments) do case Utils.validate_attachments_count(attachments) do

View file

@ -23,21 +23,21 @@ defmodule Pleroma.Web.CommonAPI.Utils do
require Logger require Logger
require Pleroma.Constants require Pleroma.Constants
def attachments_from_ids(%{media_ids: ids, descriptions: desc}) do def attachments_from_ids(%{media_ids: ids, descriptions: desc}, user) do
attachments_from_ids_descs(ids, desc) attachments_from_ids_descs(ids, desc, user)
end end
def attachments_from_ids(%{media_ids: ids}) do def attachments_from_ids(%{media_ids: ids}, user) do
attachments_from_ids_no_descs(ids) attachments_from_ids_no_descs(ids, user)
end end
def attachments_from_ids(_), do: [] def attachments_from_ids(_, _), do: []
def attachments_from_ids_no_descs([]), do: [] def attachments_from_ids_no_descs([], _), do: []
def attachments_from_ids_no_descs(ids) do def attachments_from_ids_no_descs(ids, user) do
Enum.map(ids, fn media_id -> Enum.map(ids, fn media_id ->
case get_attachment(media_id) do case get_attachment(media_id, user) do
%Object{data: data} -> data %Object{data: data} -> data
_ -> nil _ -> nil
end end
@ -45,22 +45,23 @@ defmodule Pleroma.Web.CommonAPI.Utils do
|> Enum.reject(&is_nil/1) |> Enum.reject(&is_nil/1)
end end
def attachments_from_ids_descs([], _), do: [] def attachments_from_ids_descs([], _, _), do: []
def attachments_from_ids_descs(ids, descs_str) do def attachments_from_ids_descs(ids, descs_str, user) do
{_, descs} = Jason.decode(descs_str) {_, descs} = Jason.decode(descs_str)
Enum.map(ids, fn media_id -> Enum.map(ids, fn media_id ->
with %Object{data: data} <- get_attachment(media_id) do with %Object{data: data} <- get_attachment(media_id, user) do
Map.put(data, "name", descs[media_id]) Map.put(data, "name", descs[media_id])
end end
end) end)
|> Enum.reject(&is_nil/1) |> Enum.reject(&is_nil/1)
end end
defp get_attachment(media_id) do defp get_attachment(media_id, user) do
with %Object{data: data} = object <- Repo.get(Object, media_id), with %Object{data: data} = object <- Repo.get(Object, media_id),
%{"type" => type} when type in Pleroma.Constants.upload_object_types() <- data do %{"type" => type} when type in Pleroma.Constants.upload_object_types() <- data,
:ok <- Object.authorize_access(object, user) do
object object
else else
_ -> nil _ -> nil

View file

@ -586,46 +586,61 @@ defmodule Pleroma.Web.CommonAPI.UtilsTest do
end end
end end
describe "attachments_from_ids_descs/2" do describe "attachments_from_ids_descs/3" do
test "returns [] when attachment ids is empty" do test "returns [] when attachment ids is empty" do
assert Utils.attachments_from_ids_descs([], "{}") == [] assert Utils.attachments_from_ids_descs([], "{}", nil) == []
end end
test "returns list attachments with desc" do test "returns list attachments with desc" do
object = insert(:attachment) user = insert(:user)
object = insert(:attachment, %{user: user})
desc = Jason.encode!(%{object.id => "test-desc"}) desc = Jason.encode!(%{object.id => "test-desc"})
assert Utils.attachments_from_ids_descs(["#{object.id}", "34"], desc) == [ assert Utils.attachments_from_ids_descs(["#{object.id}", "34"], desc, user) == [
Map.merge(object.data, %{"name" => "test-desc"}) Map.merge(object.data, %{"name" => "test-desc"})
] ]
end end
end end
describe "attachments_from_ids/1" do describe "attachments_from_ids/2" do
test "returns attachments with descs" do test "returns attachments with descs" do
object = insert(:attachment) user = insert(:user)
object = insert(:attachment, %{user: user})
desc = Jason.encode!(%{object.id => "test-desc"}) desc = Jason.encode!(%{object.id => "test-desc"})
assert Utils.attachments_from_ids(%{ assert Utils.attachments_from_ids(
%{
media_ids: ["#{object.id}"], media_ids: ["#{object.id}"],
descriptions: desc descriptions: desc
}) == [ },
user
) == [
Map.merge(object.data, %{"name" => "test-desc"}) Map.merge(object.data, %{"name" => "test-desc"})
] ]
end end
test "returns attachments without descs" do test "returns attachments without descs" do
object = insert(:attachment) user = insert(:user)
assert Utils.attachments_from_ids(%{media_ids: ["#{object.id}"]}) == [object.data] object = insert(:attachment, %{user: user})
assert Utils.attachments_from_ids(%{media_ids: ["#{object.id}"]}, user) == [object.data]
end end
test "returns [] when not pass media_ids" do test "returns [] when not pass media_ids" do
assert Utils.attachments_from_ids(%{}) == [] assert Utils.attachments_from_ids(%{}, nil) == []
end
test "returns [] when media_ids not belong to current user" do
user = insert(:user)
user2 = insert(:user)
object = insert(:attachment, %{user: user})
assert Utils.attachments_from_ids(%{media_ids: ["#{object.id}"]}, user2) == []
end end
test "checks that the object is of upload type" do test "checks that the object is of upload type" do
object = insert(:note) object = insert(:note)
assert Utils.attachments_from_ids(%{media_ids: ["#{object.id}"]}) == [] assert Utils.attachments_from_ids(%{media_ids: ["#{object.id}"]}, nil) == []
end end
end end

View file

@ -279,6 +279,24 @@ defmodule Pleroma.Web.CommonAPITest do
assert {:reject, "[KeywordPolicy] Matches with rejected keyword"} == assert {:reject, "[KeywordPolicy] Matches with rejected keyword"} ==
CommonAPI.post_chat_message(author, recipient, "GNO/Linux") CommonAPI.post_chat_message(author, recipient, "GNO/Linux")
end end
test "it reject messages with attachments not belonging to user" do
author = insert(:user)
not_author = insert(:user)
recipient = author
attachment = insert(:attachment, %{user: not_author})
{:error, message} =
CommonAPI.post_chat_message(
author,
recipient,
"123",
media_id: attachment.id
)
assert message == :forbidden
end
end end
describe "unblocking" do describe "unblocking" do

View file

@ -48,7 +48,7 @@ defmodule Pleroma.Web.MastodonAPI.ScheduledActivityViewTest do
id: to_string(scheduled_activity.id), id: to_string(scheduled_activity.id),
media_attachments: media_attachments:
%{media_ids: [upload.id]} %{media_ids: [upload.id]}
|> Utils.attachments_from_ids() |> Utils.attachments_from_ids(user)
|> Enum.map(&StatusView.render("attachment.json", %{attachment: &1})), |> Enum.map(&StatusView.render("attachment.json", %{attachment: &1})),
params: %{ params: %{
in_reply_to_id: to_string(activity.id), in_reply_to_id: to_string(activity.id),

View file

@ -24,7 +24,7 @@ defmodule Pleroma.Web.PleromaAPI.ChatMessageReferenceViewTest do
filename: "an_image.jpg" filename: "an_image.jpg"
} }
{:ok, upload} = ActivityPub.upload(file, actor: user.ap_id) {:ok, upload} = ActivityPub.upload(file, actor: recipient.ap_id)
{:ok, activity} = {:ok, activity} =
CommonAPI.post_chat_message(user, recipient, "kippis :firefox:", idempotency_key: "123") CommonAPI.post_chat_message(user, recipient, "kippis :firefox:", idempotency_key: "123")