From: rinpatch Date: Fri, 13 Mar 2020 18:15:42 +0000 (+0300) Subject: rate limiter: disable based on if remote ip was found, not on if the plug was enabled X-Git-Url: https://git.squeep.com/?a=commitdiff_plain;h=c46d035f7bc79f451c8b2356f3b809c29684cfe4;p=akkoma rate limiter: disable based on if remote ip was found, not on if the plug was enabled The current rate limiter disable logic won't trigger when the remote ip is not forwarded, only when the remoteip plug is not enabled, which is not the case on most instances since it's enabled by default. This changes the behavior to warn and disable when the remote ip was not forwarded, even if the RemoteIP plug is enabled. Also closes #1620 --- diff --git a/config/test.exs b/config/test.exs index a17886265..b8ea63c94 100644 --- a/config/test.exs +++ b/config/test.exs @@ -92,6 +92,8 @@ config :pleroma, :modules, runtime_dir: "test/fixtures/modules" config :pleroma, Pleroma.Emails.NewUsersDigestEmail, enabled: true +config :pleroma, Pleroma.Plugs.RemoteIp, enabled: false + if File.exists?("./config/test.secret.exs") do import_config "test.secret.exs" else diff --git a/lib/pleroma/plugs/rate_limiter/rate_limiter.ex b/lib/pleroma/plugs/rate_limiter/rate_limiter.ex index c3f6351c8..1529da717 100644 --- a/lib/pleroma/plugs/rate_limiter/rate_limiter.ex +++ b/lib/pleroma/plugs/rate_limiter/rate_limiter.ex @@ -78,7 +78,7 @@ defmodule Pleroma.Plugs.RateLimiter do end def call(conn, plug_opts) do - if disabled?() do + if disabled?(conn) do handle_disabled(conn) else action_settings = action_settings(plug_opts) @@ -87,9 +87,9 @@ defmodule Pleroma.Plugs.RateLimiter do end defp handle_disabled(conn) do - if Config.get(:env) == :prod do - Logger.warn("Rate limiter is disabled for localhost/socket") - end + Logger.warn( + "Rate limiter disabled due to forwarded IP not being found. Please ensure your reverse proxy is providing the X-Forwarded-For header or disable the RemoteIP plug/rate limiter." + ) conn end @@ -109,16 +109,21 @@ defmodule Pleroma.Plugs.RateLimiter do end end - def disabled? do + def disabled?(conn) do localhost_or_socket = - Config.get([Pleroma.Web.Endpoint, :http, :ip]) - |> Tuple.to_list() - |> Enum.join(".") - |> String.match?(~r/^local|^127.0.0.1/) + case Config.get([Pleroma.Web.Endpoint, :http, :ip]) do + {127, 0, 0, 1} -> true + {0, 0, 0, 0, 0, 0, 0, 1} -> true + {:local, _} -> true + _ -> false + end - remote_ip_disabled = not Config.get([Pleroma.Plugs.RemoteIp, :enabled]) + remote_ip_not_found = + if Map.has_key?(conn.assigns, :remote_ip_found), + do: !conn.assigns.remote_ip_found, + else: false - localhost_or_socket and remote_ip_disabled + localhost_or_socket and remote_ip_not_found end @inspect_bucket_not_found {:error, :not_found} diff --git a/lib/pleroma/plugs/remote_ip.ex b/lib/pleroma/plugs/remote_ip.ex index 2eca4f8f6..0ac9050d0 100644 --- a/lib/pleroma/plugs/remote_ip.ex +++ b/lib/pleroma/plugs/remote_ip.ex @@ -7,6 +7,8 @@ defmodule Pleroma.Plugs.RemoteIp do This is a shim to call [`RemoteIp`](https://git.pleroma.social/pleroma/remote_ip) but with runtime configuration. """ + import Plug.Conn + @behaviour Plug @headers ~w[ @@ -26,11 +28,12 @@ defmodule Pleroma.Plugs.RemoteIp do def init(_), do: nil - def call(conn, _) do + def call(%{remote_ip: original_remote_ip} = conn, _) do config = Pleroma.Config.get(__MODULE__, []) if Keyword.get(config, :enabled, false) do - RemoteIp.call(conn, remote_ip_opts(config)) + %{remote_ip: new_remote_ip} = conn = RemoteIp.call(conn, remote_ip_opts(config)) + assign(conn, :remote_ip_found, original_remote_ip != new_remote_ip) else conn end diff --git a/test/plugs/rate_limiter_test.exs b/test/plugs/rate_limiter_test.exs index 8023271e4..81e2009c8 100644 --- a/test/plugs/rate_limiter_test.exs +++ b/test/plugs/rate_limiter_test.exs @@ -3,8 +3,7 @@ # SPDX-License-Identifier: AGPL-3.0-only defmodule Pleroma.Plugs.RateLimiterTest do - use ExUnit.Case, async: true - use Plug.Test + use Pleroma.Web.ConnCase alias Pleroma.Config alias Pleroma.Plugs.RateLimiter @@ -36,63 +35,44 @@ defmodule Pleroma.Plugs.RateLimiterTest do |> RateLimiter.init() |> RateLimiter.action_settings() end + end - test "it is disabled for localhost" do - Config.put([:rate_limit, @limiter_name], {1, 1}) - Config.put([Pleroma.Web.Endpoint, :http, :ip], {127, 0, 0, 1}) - Config.put([Pleroma.Plugs.RemoteIp, :enabled], false) - - assert RateLimiter.disabled?() == true - end + test "it is disabled if it remote ip plug is enabled but no remote ip is found" do + Config.put([Pleroma.Web.Endpoint, :http, :ip], {127, 0, 0, 1}) + assert RateLimiter.disabled?(Plug.Conn.assign(build_conn(), :remote_ip_found, false)) + end - test "it is disabled for socket" do - Config.put([:rate_limit, @limiter_name], {1, 1}) - Config.put([Pleroma.Web.Endpoint, :http, :ip], {:local, "/path/to/pleroma.sock"}) - Config.put([Pleroma.Plugs.RemoteIp, :enabled], false) + test "it restricts based on config values" do + limiter_name = :test_plug_opts + scale = 80 + limit = 5 - assert RateLimiter.disabled?() == true - end + Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) + Config.put([:rate_limit, limiter_name], {scale, limit}) - test "it is enabled for socket when remote ip is enabled" do - Config.put([:rate_limit, @limiter_name], {1, 1}) - Config.put([Pleroma.Web.Endpoint, :http, :ip], {:local, "/path/to/pleroma.sock"}) - Config.put([Pleroma.Plugs.RemoteIp, :enabled], true) + plug_opts = RateLimiter.init(name: limiter_name) + conn = conn(:get, "/") - assert RateLimiter.disabled?() == false + for i <- 1..5 do + conn = RateLimiter.call(conn, plug_opts) + assert {^i, _} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) + Process.sleep(10) end - test "it restricts based on config values" do - limiter_name = :test_plug_opts - scale = 80 - limit = 5 - - Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8}) - Config.put([:rate_limit, limiter_name], {scale, limit}) - - plug_opts = RateLimiter.init(name: limiter_name) - conn = conn(:get, "/") - - for i <- 1..5 do - conn = RateLimiter.call(conn, plug_opts) - assert {^i, _} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) - Process.sleep(10) - end + conn = RateLimiter.call(conn, plug_opts) + assert %{"error" => "Throttled"} = Phoenix.ConnTest.json_response(conn, :too_many_requests) + assert conn.halted - conn = RateLimiter.call(conn, plug_opts) - assert %{"error" => "Throttled"} = Phoenix.ConnTest.json_response(conn, :too_many_requests) - assert conn.halted + Process.sleep(50) - Process.sleep(50) + conn = conn(:get, "/") - conn = conn(:get, "/") + conn = RateLimiter.call(conn, plug_opts) + assert {1, 4} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) - conn = RateLimiter.call(conn, plug_opts) - assert {1, 4} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts) - - refute conn.status == Plug.Conn.Status.code(:too_many_requests) - refute conn.resp_body - refute conn.halted - end + refute conn.status == Plug.Conn.Status.code(:too_many_requests) + refute conn.resp_body + refute conn.halted end describe "options" do diff --git a/test/web/mastodon_api/controllers/account_controller_test.exs b/test/web/mastodon_api/controllers/account_controller_test.exs index 7f7d8cea3..7efccd9c4 100644 --- a/test/web/mastodon_api/controllers/account_controller_test.exs +++ b/test/web/mastodon_api/controllers/account_controller_test.exs @@ -756,10 +756,6 @@ defmodule Pleroma.Web.MastodonAPI.AccountControllerTest do end describe "create account by app / rate limit" do - clear_config([Pleroma.Plugs.RemoteIp, :enabled]) do - Pleroma.Config.put([Pleroma.Plugs.RemoteIp, :enabled], true) - end - clear_config([:rate_limit, :app_account_creation]) do Pleroma.Config.put([:rate_limit, :app_account_creation], {10_000, 2}) end