From 3b3aad606508ecf1801a24420143d700c4cf01c6 Mon Sep 17 00:00:00 2001 From: Lain Soykaf Date: Fri, 24 Jul 2026 22:45:50 +0400 Subject: [PATCH] fix(api): isolate sensitive idempotency responses --- .../admin_api/controllers/user_controller.ex | 7 + lib/pleroma/web/plugs/idempotency_plug.ex | 58 ++++++-- .../controllers/user_controller_test.exs | 45 ++++++ .../web/plugs/idempotency_plug_test.exs | 133 ++++++++++++++++++ 4 files changed, 231 insertions(+), 12 deletions(-) diff --git a/lib/pleroma/web/admin_api/controllers/user_controller.ex b/lib/pleroma/web/admin_api/controllers/user_controller.ex index 7146e4bea..c94964937 100644 --- a/lib/pleroma/web/admin_api/controllers/user_controller.ex +++ b/lib/pleroma/web/admin_api/controllers/user_controller.ex @@ -152,6 +152,13 @@ defmodule Pleroma.Web.AdminAPI.UserController do } = conn, _ ) do + conn = + if Enum.any?(users, fn user -> Map.get(user, :password) in ["", nil] end) do + put_private(conn, :skip_idempotency_cache, true) + else + conn + end + multi = users |> Enum.map(fn %{nickname: nickname, email: email} = attrs -> diff --git a/lib/pleroma/web/plugs/idempotency_plug.ex b/lib/pleroma/web/plugs/idempotency_plug.ex index a3b7af869..81983efba 100644 --- a/lib/pleroma/web/plugs/idempotency_plug.ex +++ b/lib/pleroma/web/plugs/idempotency_plug.ex @@ -6,6 +6,8 @@ defmodule Pleroma.Web.Plugs.IdempotencyPlug do import Phoenix.Controller, only: [json: 2] import Plug.Conn + alias Pleroma.Config + @behaviour Plug @cachex Pleroma.Config.get([:cachex, :provider], Cachex) @@ -27,9 +29,11 @@ defmodule Pleroma.Web.Plugs.IdempotencyPlug do def call(conn, _), do: conn def process_request(conn, key) do - case @cachex.get(:idempotency_cache, key) do + cache_key = {conn.method, conn.request_path, actor_key(conn), key} + + case @cachex.get(:idempotency_cache, cache_key) do {:ok, nil} -> - cache_resposnse(conn, key) + cache_resposnse(conn, key, cache_key) {:ok, record} -> send_cached(conn, key, record) @@ -39,18 +43,48 @@ defmodule Pleroma.Web.Plugs.IdempotencyPlug do end end - defp cache_resposnse(conn, key) do - register_before_send(conn, fn conn -> - [request_id] = get_resp_header(conn, "x-request-id") - content_type = get_content_type(conn) + defp cache_resposnse(conn, key, cache_key) do + register_before_send(conn, &maybe_cache_response(&1, key, cache_key)) + end - record = {request_id, content_type, conn.status, conn.resp_body} - {:ok, _} = @cachex.put(:idempotency_cache, key, record) + defp maybe_cache_response( + %{private: %{skip_idempotency_cache: true}} = conn, + _key, + _cache_key + ), + do: conn - conn - |> put_resp_header("idempotency-key", key) - |> put_resp_header("x-original-request-id", request_id) - end) + defp maybe_cache_response(conn, key, cache_key) do + [request_id] = get_resp_header(conn, "x-request-id") + content_type = get_content_type(conn) + + record = {request_id, content_type, conn.status, conn.resp_body} + {:ok, _} = @cachex.put(:idempotency_cache, cache_key, record) + + conn + |> put_resp_header("idempotency-key", key) + |> put_resp_header("x-original-request-id", request_id) + end + + defp actor_key(%{assigns: assigns}) do + {principal_key(assigns), token_key(assigns[:token]), staff_privileges()} + end + + defp principal_key(%{user: %{id: id} = user}) do + {:user, id, Map.get(user, :is_admin), Map.get(user, :is_moderator)} + end + + defp principal_key(%{app: %{id: id}}), do: {:app, id} + defp principal_key(_assigns), do: nil + + defp token_key(%{id: id} = token), do: {id, token |> Map.get(:scopes, []) |> Enum.sort()} + defp token_key(_token), do: nil + + defp staff_privileges do + { + Config.get([:instance, :admin_privileges], []) |> Enum.sort(), + Config.get([:instance, :moderator_privileges], []) |> Enum.sort() + } end defp send_cached(conn, key, record) do diff --git a/test/pleroma/web/admin_api/controllers/user_controller_test.exs b/test/pleroma/web/admin_api/controllers/user_controller_test.exs index b6c19f37c..32f2c1d30 100644 --- a/test/pleroma/web/admin_api/controllers/user_controller_test.exs +++ b/test/pleroma/web/admin_api/controllers/user_controller_test.exs @@ -238,6 +238,51 @@ defmodule Pleroma.Web.AdminAPI.UserControllerTest do assert Repo.aggregate(Pleroma.PasswordResetToken, :count) == 0 end + test "Password reset links are not cached by idempotency key", %{conn: conn} do + idempotency_key = Ecto.UUID.generate() + + request = fn conn -> + conn + |> put_req_header("accept", "application/json") + |> put_req_header("content-type", "application/json") + |> put_req_header("idempotency-key", idempotency_key) + |> post("/api/pleroma/admin/users", %{ + "users" => [%{"nickname" => "lain", "email" => "lain@example.org"}] + }) + end + + assert [%{"data" => %{"password_reset_link" => _link}}] = + conn |> request.() |> json_response(200) + + second_conn = request.(conn) + + assert [_error] = json_response(second_conn, 409) + refute get_resp_header(second_conn, "idempotent-replayed") == ["true"] + end + + test "Explicit-password responses remain idempotent", %{conn: conn} do + idempotency_key = Ecto.UUID.generate() + + request = fn conn -> + conn + |> put_req_header("accept", "application/json") + |> put_req_header("content-type", "application/json") + |> put_req_header("idempotency-key", idempotency_key) + |> post("/api/pleroma/admin/users", %{ + "users" => [ + %{"nickname" => "lain", "email" => "lain@example.org", "password" => "test"} + ] + }) + end + + assert [_account] = conn |> request.() |> json_response(200) + + second_conn = request.(conn) + + assert [_account] = json_response(second_conn, 200) + assert get_resp_header(second_conn, "idempotent-replayed") == ["true"] + end + test "Duplicate nicknames in a batch return a conflict", %{conn: conn} do conn = conn diff --git a/test/pleroma/web/plugs/idempotency_plug_test.exs b/test/pleroma/web/plugs/idempotency_plug_test.exs index 3b0131596..cc7580ca2 100644 --- a/test/pleroma/web/plugs/idempotency_plug_test.exs +++ b/test/pleroma/web/plugs/idempotency_plug_test.exs @@ -70,6 +70,139 @@ defmodule Pleroma.Web.Plugs.IdempotencyPlugTest do assert [^key] = Conn.get_resp_header(conn, "idempotency-key") end + test "does not cache responses marked as sensitive" do + key = Ecto.UUID.generate() + + :post + |> conn("/cofe") + |> put_req_header("idempotency-key", key) + |> Conn.put_resp_header("x-request-id", "sensitive-request") + |> Conn.put_resp_content_type("application/json") + |> IdempotencyPlug.call([]) + |> Conn.put_private(:skip_idempotency_cache, true) + |> Conn.send_resp(200, "secret") + + conn = + :post + |> conn("/cofe") + |> put_req_header("idempotency-key", key) + |> Conn.put_resp_header("x-request-id", "second-request") + |> Conn.put_resp_content_type("application/json") + |> IdempotencyPlug.call([]) + + refute conn.halted + refute conn.resp_body + end + + test "does not replay legacy unscoped cache entries" do + key = Ecto.UUID.generate() + record = {"legacy-request", "application/json", 200, "secret"} + assert {:ok, true} = Cachex.put(:idempotency_cache, key, record) + + conn = + :post + |> conn("/cofe") + |> put_req_header("idempotency-key", key) + |> Conn.put_resp_header("x-request-id", "new-request") + |> Conn.put_resp_content_type("application/json") + |> IdempotencyPlug.call([]) + + refute conn.halted + refute conn.resp_body + end + + test "scopes cached responses to the route and authenticated actor" do + key = Ecto.UUID.generate() + + :post + |> conn("/cofe") + |> Conn.assign(:user, %{id: "user-one", is_admin: false, is_moderator: false}) + |> put_req_header("idempotency-key", key) + |> Conn.put_resp_header("x-request-id", "original-request") + |> Conn.put_resp_content_type("application/json") + |> IdempotencyPlug.call([]) + |> Conn.send_resp(200, "one") + + for {path, user} <- [ + {"/tea", %{id: "user-one", is_admin: false, is_moderator: false}}, + {"/cofe", %{id: "user-two", is_admin: false, is_moderator: false}}, + {"/cofe", %{id: "user-one", is_admin: true, is_moderator: false}} + ] do + conn = + :post + |> conn(path) + |> Conn.assign(:user, user) + |> put_req_header("idempotency-key", key) + |> Conn.put_resp_header("x-request-id", Ecto.UUID.generate()) + |> Conn.put_resp_content_type("application/json") + |> IdempotencyPlug.call([]) + + refute conn.halted + refute conn.resp_body + end + end + + test "scopes cached responses to the OAuth application and token" do + key = Ecto.UUID.generate() + + :post + |> conn("/cofe") + |> Conn.assign(:app, %{id: "app-one"}) + |> Conn.assign(:token, %{id: "token-one", scopes: ["write"]}) + |> put_req_header("idempotency-key", key) + |> Conn.put_resp_header("x-request-id", "original-request") + |> Conn.put_resp_content_type("application/json") + |> IdempotencyPlug.call([]) + |> Conn.send_resp(200, "one") + + for {app, token} <- [ + {%{id: "app-two"}, %{id: "token-two", scopes: ["write"]}}, + {%{id: "app-one"}, %{id: "token-two", scopes: ["write"]}}, + {%{id: "app-one"}, %{id: "token-one", scopes: ["read"]}} + ] do + conn = + :post + |> conn("/cofe") + |> Conn.assign(:app, app) + |> Conn.assign(:token, token) + |> put_req_header("idempotency-key", key) + |> Conn.put_resp_header("x-request-id", Ecto.UUID.generate()) + |> Conn.put_resp_content_type("application/json") + |> IdempotencyPlug.call([]) + + refute conn.halted + refute conn.resp_body + end + end + + test "scopes cached responses to configured staff privileges" do + key = Ecto.UUID.generate() + clear_config([:instance, :admin_privileges], [:users_delete]) + + :post + |> conn("/cofe") + |> Conn.assign(:user, %{id: "admin", is_admin: true, is_moderator: false}) + |> put_req_header("idempotency-key", key) + |> Conn.put_resp_header("x-request-id", "original-request") + |> Conn.put_resp_content_type("application/json") + |> IdempotencyPlug.call([]) + |> Conn.send_resp(200, "one") + + clear_config([:instance, :admin_privileges], []) + + conn = + :post + |> conn("/cofe") + |> Conn.assign(:user, %{id: "admin", is_admin: true, is_moderator: false}) + |> put_req_header("idempotency-key", key) + |> Conn.put_resp_header("x-request-id", "second-request") + |> Conn.put_resp_content_type("application/json") + |> IdempotencyPlug.call([]) + + refute conn.halted + refute conn.resp_body + end + test "passes conn downstream if idempotency is not present in headers" do orig_request_id = "test4" body = "testing"