From 427da7a99a30ebc7a7deb54e7704b5d8dffea199 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 4 Sep 2024 09:19:07 -0400 Subject: [PATCH 1/5] 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 000000000..0e95c01d7 --- /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 ad6b1cb94..a4fedff45 100644 --- a/config/config.exs +++ b/config/config.exs @@ -711,6 +711,7 @@ config :pleroma, :rate_limit, 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 844673ae0..6cfeb712e 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 2/5] 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 0e95c01d7..cdc2e816d 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 d1bf6dd18..f0f54bb46 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 @@ defmodule Pleroma.Web.OAuth.App 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 47b03215f..0b3de5481 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 @@ defmodule Pleroma.Web.OAuth.OAuthController do 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 000000000..11bec529b --- /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 83a08d9fc..260442771 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 @@ defmodule Pleroma.Web.OAuth.OAuthControllerTest 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 @@ defmodule Pleroma.Web.OAuth.OAuthControllerTest 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 3/5] 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 a4fedff45..2bc28b256 100644 --- a/config/config.exs +++ b/config/config.exs @@ -597,7 +597,8 @@ config :pleroma, Oban, 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 f0f54bb46..6ae419d09 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -165,4 +165,10 @@ defmodule Pleroma.Web.OAuth.App do 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 000000000..ee71cd7b6 --- /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 96a67de6b..725ea3eb8 100644 --- a/test/pleroma/web/o_auth/app_test.exs +++ b/test/pleroma/web/o_auth/app_test.exs @@ -53,4 +53,16 @@ defmodule Pleroma.Web.OAuth.AppTest 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 4/5] 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 2bc28b256..80a3b8d57 100644 --- a/config/config.exs +++ b/config/config.exs @@ -598,7 +598,7 @@ config :pleroma, Oban, 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 6ae419d09..032011433 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -166,9 +166,14 @@ defmodule Pleroma.Web.OAuth.App do 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 5/5] 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 032011433..7661c2566 100644 --- a/lib/pleroma/web/o_auth/app.ex +++ b/lib/pleroma/web/o_auth/app.ex @@ -168,8 +168,13 @@ defmodule Pleroma.Web.OAuth.App do @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 725ea3eb8..44219cf90 100644 --- a/test/pleroma/web/o_auth/app_test.exs +++ b/test/pleroma/web/o_auth/app_test.exs @@ -56,13 +56,18 @@ defmodule Pleroma.Web.OAuth.AppTest 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