From 92d5f0ac141e679af048ebcbec9cb91df68fd929 Mon Sep 17 00:00:00 2001 From: feld Date: Wed, 4 Sep 2024 02:22:25 +0000 Subject: [PATCH 1/6] Revert "Merge branch 'oauth-app-spam' into 'develop'" This reverts merge request !4244 --- changelog.d/oauth-app.fix | 1 - .../controllers/app_controller.ex | 3 +- lib/pleroma/web/o_auth/app.ex | 24 ++-- .../controllers/app_controller_test.exs | 110 ------------------ test/pleroma/web/o_auth/app_test.exs | 15 +-- 5 files changed, 24 insertions(+), 129 deletions(-) delete mode 100644 changelog.d/oauth-app.fix diff --git a/changelog.d/oauth-app.fix b/changelog.d/oauth-app.fix deleted file mode 100644 index eb917462fd..0000000000 --- a/changelog.d/oauth-app.fix +++ /dev/null @@ -1 +0,0 @@ -Prevent OAuth App flow from creating duplicate entries diff --git a/lib/pleroma/web/mastodon_api/controllers/app_controller.ex b/lib/pleroma/web/mastodon_api/controllers/app_controller.ex index 4677ac40aa..844673ae01 100644 --- a/lib/pleroma/web/mastodon_api/controllers/app_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/app_controller.ex @@ -36,7 +36,8 @@ def create(%{body_params: params} = conn, _params) do |> Map.put(:scopes, scopes) |> Maps.put_if_present(:user_id, user_id) - with {:ok, app} <- App.get_or_make(app_attrs) do + with cs <- App.register_changeset(%App{}, app_attrs), + {:ok, app} <- Repo.insert(cs) do render(conn, "show.json", app: app) end end diff --git a/lib/pleroma/web/o_auth/app.ex b/lib/pleroma/web/o_auth/app.ex index 889850c73c..d1bf6dd18f 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -67,27 +67,35 @@ def update(id, params) do with %__MODULE__{} = app <- Repo.get(__MODULE__, id) do app |> changeset(params) - |> validate_required([:scopes]) |> Repo.update() end end @doc """ - Gets app by attrs or create new with attrs. - Updates the attrs if needed. + Gets app by attrs or create new with attrs. + And updates the scopes if need. """ - @spec get_or_make(map()) :: {:ok, t()} | {:error, Ecto.Changeset.t()} - def get_or_make(attrs) do - with %__MODULE__{} = app <- Repo.get_by(__MODULE__, client_name: attrs.client_name) do - __MODULE__.update(app.id, Map.take(attrs, [:scopes, :website])) + @spec get_or_make(map(), list(String.t())) :: {:ok, t()} | {:error, Ecto.Changeset.t()} + def get_or_make(attrs, scopes) do + with %__MODULE__{} = app <- Repo.get_by(__MODULE__, attrs) do + update_scopes(app, scopes) else _e -> %__MODULE__{} - |> register_changeset(attrs) + |> register_changeset(Map.put(attrs, :scopes, scopes)) |> Repo.insert() end end + defp update_scopes(%__MODULE__{} = app, []), do: {:ok, app} + defp update_scopes(%__MODULE__{scopes: scopes} = app, scopes), do: {:ok, app} + + defp update_scopes(%__MODULE__{} = app, scopes) do + app + |> change(%{scopes: scopes}) + |> Repo.update() + end + @spec search(map()) :: {:ok, [t()], non_neg_integer()} def search(params) do query = from(a in __MODULE__) diff --git a/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs b/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs index df28f20108..bc9d4048c4 100644 --- a/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs +++ b/test/pleroma/web/mastodon_api/controllers/app_controller_test.exs @@ -89,114 +89,4 @@ test "creates an oauth app with a user", %{conn: conn} do assert expected == json_response_and_validate_schema(conn, 200) assert app.user_id == user.id end - - test "creates an oauth app without a user", %{conn: conn} do - app_attrs = build(:oauth_app) - - conn = - conn - |> put_req_header("content-type", "application/json") - |> post("/api/v1/apps", %{ - client_name: app_attrs.client_name, - redirect_uris: app_attrs.redirect_uris - }) - - [app] = Repo.all(App) - - expected = %{ - "name" => app.client_name, - "website" => app.website, - "client_id" => app.client_id, - "client_secret" => app.client_secret, - "id" => app.id |> to_string(), - "redirect_uri" => app.redirect_uris, - "vapid_key" => Push.vapid_config() |> Keyword.get(:public_key) - } - - assert expected == json_response_and_validate_schema(conn, 200) - end - - test "does not duplicate apps with the same client name", %{conn: conn} do - client_name = "BleromaSE" - redirect_uris = "https://bleroma.app/oauth-callback" - - for _i <- 1..3 do - conn - |> put_req_header("content-type", "application/json") - |> post("/api/v1/apps", %{ - client_name: client_name, - redirect_uris: redirect_uris - }) - |> json_response_and_validate_schema(200) - end - - apps = Repo.all(App) - - assert length(apps) == 1 - assert List.first(apps).client_name == client_name - assert List.first(apps).redirect_uris == redirect_uris - end - - test "app scopes can be updated", %{conn: conn} do - client_name = "BleromaSE" - redirect_uris = "https://bleroma.app/oauth-callback" - website = "https://bleromase.com" - scopes = "read write" - - conn - |> put_req_header("content-type", "application/json") - |> post("/api/v1/apps", %{ - client_name: client_name, - redirect_uris: redirect_uris, - website: website, - scopes: scopes - }) - |> json_response_and_validate_schema(200) - - assert List.first(Repo.all(App)).scopes == String.split(scopes, " ") - - updated_scopes = "read write push" - - conn - |> put_req_header("content-type", "application/json") - |> post("/api/v1/apps", %{ - client_name: client_name, - redirect_uris: redirect_uris, - website: website, - scopes: updated_scopes - }) - |> json_response_and_validate_schema(200) - - assert List.first(Repo.all(App)).scopes == String.split(updated_scopes, " ") - end - - test "app website URL can be updated", %{conn: conn} do - client_name = "BleromaSE" - redirect_uris = "https://bleroma.app/oauth-callback" - website = "https://bleromase.com" - - conn - |> put_req_header("content-type", "application/json") - |> post("/api/v1/apps", %{ - client_name: client_name, - redirect_uris: redirect_uris, - website: website - }) - |> json_response_and_validate_schema(200) - - assert List.first(Repo.all(App)).website == website - - updated_website = "https://bleromase2ultimateedition.com" - - conn - |> put_req_header("content-type", "application/json") - |> post("/api/v1/apps", %{ - client_name: client_name, - redirect_uris: redirect_uris, - website: updated_website - }) - |> json_response_and_validate_schema(200) - - assert List.first(Repo.all(App)).website == updated_website - end end diff --git a/test/pleroma/web/o_auth/app_test.exs b/test/pleroma/web/o_auth/app_test.exs index 423b660ea7..96a67de6bd 100644 --- a/test/pleroma/web/o_auth/app_test.exs +++ b/test/pleroma/web/o_auth/app_test.exs @@ -12,23 +12,20 @@ defmodule Pleroma.Web.OAuth.AppTest do test "gets exist app" do attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} app = insert(:oauth_app, Map.merge(attrs, %{scopes: ["read", "write"]})) - {:ok, %App{} = exist_app} = App.get_or_make(attrs) + {:ok, %App{} = exist_app} = App.get_or_make(attrs, []) assert exist_app == app end test "make app" do - attrs = %{client_name: "Mastodon-Local", redirect_uris: ".", scopes: ["write"]} - {:ok, %App{} = app} = App.get_or_make(attrs) + attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} + {:ok, %App{} = app} = App.get_or_make(attrs, ["write"]) assert app.scopes == ["write"] end test "gets exist app and updates scopes" do - attrs = %{client_name: "Mastodon-Local", redirect_uris: ".", scopes: ["read", "write"]} - app = insert(:oauth_app, attrs) - - {:ok, %App{} = exist_app} = - App.get_or_make(%{attrs | scopes: ["read", "write", "follow", "push"]}) - + attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} + app = insert(:oauth_app, Map.merge(attrs, %{scopes: ["read", "write"]})) + {:ok, %App{} = exist_app} = App.get_or_make(attrs, ["read", "write", "follow", "push"]) assert exist_app.id == app.id assert exist_app.scopes == ["read", "write", "follow", "push"] end From 427da7a99a30ebc7a7deb54e7704b5d8dffea199 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 4 Sep 2024 09:19:07 -0400 Subject: [PATCH 2/6] Rate Limit the OAuth App spam --- changelog.d/oauth-app-spam.fix | 1 + config/config.exs | 1 + lib/pleroma/web/mastodon_api/controllers/app_controller.ex | 2 ++ 3 files changed, 4 insertions(+) create mode 100644 changelog.d/oauth-app-spam.fix diff --git a/changelog.d/oauth-app-spam.fix b/changelog.d/oauth-app-spam.fix new file mode 100644 index 0000000000..0e95c01d70 --- /dev/null +++ b/changelog.d/oauth-app-spam.fix @@ -0,0 +1 @@ +Add a rate limiter to the OAuth App creation endpoint diff --git a/config/config.exs b/config/config.exs index ad6b1cb94c..a4fedff459 100644 --- a/config/config.exs +++ b/config/config.exs @@ -711,6 +711,7 @@ timeline: {500, 3}, search: [{1000, 10}, {1000, 30}], app_account_creation: {1_800_000, 25}, + oauth_app_creation: {900_000, 5}, relations_actions: {10_000, 10}, relation_id_action: {60_000, 2}, statuses_actions: {10_000, 15}, diff --git a/lib/pleroma/web/mastodon_api/controllers/app_controller.ex b/lib/pleroma/web/mastodon_api/controllers/app_controller.ex index 844673ae01..6cfeb712ec 100644 --- a/lib/pleroma/web/mastodon_api/controllers/app_controller.ex +++ b/lib/pleroma/web/mastodon_api/controllers/app_controller.ex @@ -19,6 +19,8 @@ defmodule Pleroma.Web.MastodonAPI.AppController do action_fallback(Pleroma.Web.MastodonAPI.FallbackController) + plug(Pleroma.Web.Plugs.RateLimiter, [name: :oauth_app_creation] when action == :create) + plug(:skip_auth when action in [:create, :verify_credentials]) plug(Pleroma.Web.ApiSpec.CastAndValidate) From 7bd0750787859cb30382d90162d70380441abc05 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 4 Sep 2024 10:40:37 -0400 Subject: [PATCH 3/6] Ensure apps are assigned to users --- changelog.d/oauth-app-spam.fix | 2 +- lib/pleroma/web/o_auth/app.ex | 10 +++++++++ lib/pleroma/web/o_auth/o_auth_controller.ex | 2 ++ .../20240904142434_assign_app_user.exs | 21 +++++++++++++++++++ .../web/o_auth/o_auth_controller_test.exs | 8 +++++++ 5 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 priv/repo/migrations/20240904142434_assign_app_user.exs diff --git a/changelog.d/oauth-app-spam.fix b/changelog.d/oauth-app-spam.fix index 0e95c01d70..cdc2e816d9 100644 --- a/changelog.d/oauth-app-spam.fix +++ b/changelog.d/oauth-app-spam.fix @@ -1 +1 @@ -Add a rate limiter to the OAuth App creation endpoint +Add a rate limiter to the OAuth App creation endpoint and ensure registered apps are assigned to users. diff --git a/lib/pleroma/web/o_auth/app.ex b/lib/pleroma/web/o_auth/app.ex index d1bf6dd18f..f0f54bb466 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -8,6 +8,7 @@ defmodule Pleroma.Web.OAuth.App do import Ecto.Query alias Pleroma.Repo alias Pleroma.User + alias Pleroma.Web.OAuth.Token @type t :: %__MODULE__{} @@ -155,4 +156,13 @@ def errors(changeset) do Map.put(acc, key, error) end) end + + @spec maybe_update_owner(Token.t()) :: :ok + def maybe_update_owner(%Token{app_id: app_id, user_id: user_id}) when not is_nil(user_id) do + __MODULE__.update(app_id, %{user_id: user_id}) + + :ok + end + + def maybe_update_owner(_), do: :ok end diff --git a/lib/pleroma/web/o_auth/o_auth_controller.ex b/lib/pleroma/web/o_auth/o_auth_controller.ex index 47b03215fe..0b3de54816 100644 --- a/lib/pleroma/web/o_auth/o_auth_controller.ex +++ b/lib/pleroma/web/o_auth/o_auth_controller.ex @@ -318,6 +318,8 @@ def token_exchange(%Plug.Conn{} = conn, %{"grant_type" => "client_credentials"} def token_exchange(%Plug.Conn{} = conn, params), do: bad_request(conn, params) def after_token_exchange(%Plug.Conn{} = conn, %{token: token} = view_params) do + App.maybe_update_owner(token) + conn |> AuthHelper.put_session_token(token.token) |> json(OAuthView.render("token.json", view_params)) diff --git a/priv/repo/migrations/20240904142434_assign_app_user.exs b/priv/repo/migrations/20240904142434_assign_app_user.exs new file mode 100644 index 0000000000..11bec529bf --- /dev/null +++ b/priv/repo/migrations/20240904142434_assign_app_user.exs @@ -0,0 +1,21 @@ +defmodule Pleroma.Repo.Migrations.AssignAppUser do + use Ecto.Migration + + alias Pleroma.Repo + alias Pleroma.Web.OAuth.App + alias Pleroma.Web.OAuth.Token + + def up do + Repo.all(Token) + |> Enum.group_by(fn x -> Map.get(x, :app_id) end) + |> Enum.each(fn {_app_id, tokens} -> + token = + Enum.filter(tokens, fn x -> not is_nil(x.user_id) end) + |> List.first() + + App.maybe_update_owner(token) + end) + end + + def down, do: :ok +end diff --git a/test/pleroma/web/o_auth/o_auth_controller_test.exs b/test/pleroma/web/o_auth/o_auth_controller_test.exs index 83a08d9fc2..260442771f 100644 --- a/test/pleroma/web/o_auth/o_auth_controller_test.exs +++ b/test/pleroma/web/o_auth/o_auth_controller_test.exs @@ -12,6 +12,7 @@ defmodule Pleroma.Web.OAuth.OAuthControllerTest do alias Pleroma.MFA.TOTP alias Pleroma.Repo alias Pleroma.User + alias Pleroma.Web.OAuth.App alias Pleroma.Web.OAuth.Authorization alias Pleroma.Web.OAuth.OAuthController alias Pleroma.Web.OAuth.Token @@ -770,6 +771,9 @@ test "issues a token for an all-body request" do {:ok, auth} = Authorization.create_authorization(app, user, ["write"]) + # Verify app has no associated user yet + assert %Pleroma.Web.OAuth.App{user_id: nil} = Repo.get_by(App, %{id: app.id}) + conn = build_conn() |> post("/oauth/token", %{ @@ -786,6 +790,10 @@ test "issues a token for an all-body request" do assert token assert token.scopes == auth.scopes assert user.ap_id == ap_id + + # Verify app has an associated user now + user_id = user.id + assert %Pleroma.Web.OAuth.App{user_id: ^user_id} = Repo.get_by(App, %{id: app.id}) end test "issues a token for `password` grant_type with valid credentials, with full permissions by default" do From a1951f3af7e1d5c4d53819962c3e68df5ba4475b Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 4 Sep 2024 10:59:58 -0400 Subject: [PATCH 4/6] Add Cron worker to clean up orphaned apps hourly --- config/config.exs | 3 ++- lib/pleroma/web/o_auth/app.ex | 6 ++++++ .../workers/cron/app_cleanup_worker.ex | 21 +++++++++++++++++++ test/pleroma/web/o_auth/app_test.exs | 12 +++++++++++ 4 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 lib/pleroma/workers/cron/app_cleanup_worker.ex diff --git a/config/config.exs b/config/config.exs index a4fedff459..2bc28b2561 100644 --- a/config/config.exs +++ b/config/config.exs @@ -597,7 +597,8 @@ plugins: [{Oban.Plugins.Pruner, max_age: 900}], crontab: [ {"0 0 * * 0", Pleroma.Workers.Cron.DigestEmailsWorker}, - {"0 0 * * *", Pleroma.Workers.Cron.NewUsersDigestWorker} + {"0 0 * * *", Pleroma.Workers.Cron.NewUsersDigestWorker}, + {"0 0 * * *", Pleroma.Workers.Cron.AppCleanupWorker} ] config :pleroma, Pleroma.Formatter, diff --git a/lib/pleroma/web/o_auth/app.ex b/lib/pleroma/web/o_auth/app.ex index f0f54bb466..6ae419d090 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -165,4 +165,10 @@ def maybe_update_owner(%Token{app_id: app_id, user_id: user_id}) when not is_nil end def maybe_update_owner(_), do: :ok + + @spec remove_orphans() :: :ok + def remove_orphans() do + from(a in __MODULE__, where: is_nil(a.user_id)) + |> Repo.delete_all() + end end diff --git a/lib/pleroma/workers/cron/app_cleanup_worker.ex b/lib/pleroma/workers/cron/app_cleanup_worker.ex new file mode 100644 index 0000000000..ee71cd7b65 --- /dev/null +++ b/lib/pleroma/workers/cron/app_cleanup_worker.ex @@ -0,0 +1,21 @@ +# Pleroma: A lightweight social networking server +# Copyright © 2017-2022 Pleroma Authors +# SPDX-License-Identifier: AGPL-3.0-only + +defmodule Pleroma.Workers.Cron.AppCleanupWorker do + @moduledoc """ + Cleans up registered apps that were never associated with a user. + """ + + use Oban.Worker, queue: "background" + + alias Pleroma.Web.OAuth.App + + @impl true + def perform(_job) do + App.remove_orphans() + end + + @impl true + def timeout(_job), do: :timer.seconds(30) +end diff --git a/test/pleroma/web/o_auth/app_test.exs b/test/pleroma/web/o_auth/app_test.exs index 96a67de6bd..725ea3eb82 100644 --- a/test/pleroma/web/o_auth/app_test.exs +++ b/test/pleroma/web/o_auth/app_test.exs @@ -53,4 +53,16 @@ test "get_user_apps/1" do assert Enum.sort(App.get_user_apps(user)) == Enum.sort(apps) end + + test "removes orphaned apps" do + attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} + {:ok, %App{} = app} = App.get_or_make(attrs, ["write"]) + assert app.scopes == ["write"] + + assert app == Pleroma.Repo.get_by(App, %{id: app.id}) + + App.remove_orphans() + + assert nil == Pleroma.Repo.get_by(App, %{id: app.id}) + end end From 53744bf146f157ee1ecfc9ba4de9e5d65fa80784 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 4 Sep 2024 11:43:43 -0400 Subject: [PATCH 5/6] Limit the number of orphaned to delete at 100 every 10 mins due to the cascading queries that have to check oauth_authorizations and oauth_tokens tables. This should keep ahead of most app registration spam and not overwhelm lower powered servers. --- config/config.exs | 2 +- lib/pleroma/web/o_auth/app.ex | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/config/config.exs b/config/config.exs index 2bc28b2561..80a3b8d57f 100644 --- a/config/config.exs +++ b/config/config.exs @@ -598,7 +598,7 @@ crontab: [ {"0 0 * * 0", Pleroma.Workers.Cron.DigestEmailsWorker}, {"0 0 * * *", Pleroma.Workers.Cron.NewUsersDigestWorker}, - {"0 0 * * *", Pleroma.Workers.Cron.AppCleanupWorker} + {"*/10 * * * *", Pleroma.Workers.Cron.AppCleanupWorker} ] config :pleroma, Pleroma.Formatter, diff --git a/lib/pleroma/web/o_auth/app.ex b/lib/pleroma/web/o_auth/app.ex index 6ae419d090..0320114332 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -166,9 +166,14 @@ def maybe_update_owner(%Token{app_id: app_id, user_id: user_id}) when not is_nil def maybe_update_owner(_), do: :ok - @spec remove_orphans() :: :ok - def remove_orphans() do - from(a in __MODULE__, where: is_nil(a.user_id)) - |> Repo.delete_all() + @spec remove_orphans(pos_integer()) :: :ok + def remove_orphans(limit \\ 100) do + Repo.transaction(fn -> + from(a in __MODULE__, where: is_nil(a.user_id), limit: ^limit) + |> Repo.all() + |> Enum.each(&Repo.delete(&1)) + end) + + :ok end end From 1797f5958a92f78dc79c5bf313755b16319c5d2d Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 5 Sep 2024 20:55:28 +0000 Subject: [PATCH 6/6] App orphans should only be removed if they are older than 15 mins --- lib/pleroma/web/o_auth/app.ex | 7 ++++++- test/pleroma/web/o_auth/app_test.exs | 13 +++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/pleroma/web/o_auth/app.ex b/lib/pleroma/web/o_auth/app.ex index 0320114332..7661c25668 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -168,8 +168,13 @@ def maybe_update_owner(_), do: :ok @spec remove_orphans(pos_integer()) :: :ok def remove_orphans(limit \\ 100) do + fifteen_mins_ago = DateTime.add(DateTime.utc_now(), -900, :second) + Repo.transaction(fn -> - from(a in __MODULE__, where: is_nil(a.user_id), limit: ^limit) + from(a in __MODULE__, + where: is_nil(a.user_id) and a.inserted_at < ^fifteen_mins_ago, + limit: ^limit + ) |> Repo.all() |> Enum.each(&Repo.delete(&1)) end) diff --git a/test/pleroma/web/o_auth/app_test.exs b/test/pleroma/web/o_auth/app_test.exs index 725ea3eb82..44219cf90d 100644 --- a/test/pleroma/web/o_auth/app_test.exs +++ b/test/pleroma/web/o_auth/app_test.exs @@ -56,13 +56,18 @@ test "get_user_apps/1" do test "removes orphaned apps" do attrs = %{client_name: "Mastodon-Local", redirect_uris: "."} - {:ok, %App{} = app} = App.get_or_make(attrs, ["write"]) - assert app.scopes == ["write"] + {:ok, %App{} = old_app} = App.get_or_make(attrs, ["write"]) - assert app == Pleroma.Repo.get_by(App, %{id: app.id}) + attrs = %{client_name: "PleromaFE", redirect_uris: "."} + {:ok, %App{} = app} = App.get_or_make(attrs, ["write"]) + + # backdate the old app so it's within the threshold for being cleaned up + {:ok, _} = + "UPDATE apps SET inserted_at = now() - interval '1 hour' WHERE id = #{old_app.id}" + |> Pleroma.Repo.query() App.remove_orphans() - assert nil == Pleroma.Repo.get_by(App, %{id: app.id}) + assert [app] == Pleroma.Repo.all(App) end end