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
This commit is contained in:
rinpatch 2020-03-13 21:15:42 +03:00
parent 00d1752031
commit fc4496d4fa
5 changed files with 55 additions and 69 deletions

View File

@ -92,6 +92,8 @@
config :pleroma, Pleroma.Emails.NewUsersDigestEmail, enabled: true config :pleroma, Pleroma.Emails.NewUsersDigestEmail, enabled: true
config :pleroma, Pleroma.Plugs.RemoteIp, enabled: false
if File.exists?("./config/test.secret.exs") do if File.exists?("./config/test.secret.exs") do
import_config "test.secret.exs" import_config "test.secret.exs"
else else

View File

@ -78,7 +78,7 @@ def init(plug_opts) do
end end
def call(conn, plug_opts) do def call(conn, plug_opts) do
if disabled?() do if disabled?(conn) do
handle_disabled(conn) handle_disabled(conn)
else else
action_settings = action_settings(plug_opts) action_settings = action_settings(plug_opts)
@ -87,9 +87,9 @@ def call(conn, plug_opts) do
end end
defp handle_disabled(conn) do defp handle_disabled(conn) do
if Config.get(:env) == :prod do Logger.warn(
Logger.warn("Rate limiter is disabled for localhost/socket") "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."
end )
conn conn
end end
@ -109,16 +109,21 @@ defp handle(conn, action_settings) do
end end
end end
def disabled? do def disabled?(conn) do
localhost_or_socket = localhost_or_socket =
Config.get([Pleroma.Web.Endpoint, :http, :ip]) case Config.get([Pleroma.Web.Endpoint, :http, :ip]) do
|> Tuple.to_list() {127, 0, 0, 1} -> true
|> Enum.join(".") {0, 0, 0, 0, 0, 0, 0, 1} -> true
|> String.match?(~r/^local|^127.0.0.1/) {: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 end
@inspect_bucket_not_found {:error, :not_found} @inspect_bucket_not_found {:error, :not_found}

View File

@ -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. This is a shim to call [`RemoteIp`](https://git.pleroma.social/pleroma/remote_ip) but with runtime configuration.
""" """
import Plug.Conn
@behaviour Plug @behaviour Plug
@headers ~w[ @headers ~w[
@ -26,11 +28,12 @@ defmodule Pleroma.Plugs.RemoteIp do
def init(_), do: nil def init(_), do: nil
def call(conn, _) do def call(%{remote_ip: original_remote_ip} = conn, _) do
config = Pleroma.Config.get(__MODULE__, []) config = Pleroma.Config.get(__MODULE__, [])
if Keyword.get(config, :enabled, false) do 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 else
conn conn
end end

View File

@ -3,8 +3,7 @@
# SPDX-License-Identifier: AGPL-3.0-only # SPDX-License-Identifier: AGPL-3.0-only
defmodule Pleroma.Plugs.RateLimiterTest do defmodule Pleroma.Plugs.RateLimiterTest do
use ExUnit.Case, async: true use Pleroma.Web.ConnCase
use Plug.Test
alias Pleroma.Config alias Pleroma.Config
alias Pleroma.Plugs.RateLimiter alias Pleroma.Plugs.RateLimiter
@ -36,29 +35,11 @@ test "config is required for plug to work" do
|> RateLimiter.init() |> RateLimiter.init()
|> RateLimiter.action_settings() |> RateLimiter.action_settings()
end end
end
test "it is disabled for localhost" do test "it is disabled if it remote ip plug is enabled but no remote ip is found" do
Config.put([:rate_limit, @limiter_name], {1, 1})
Config.put([Pleroma.Web.Endpoint, :http, :ip], {127, 0, 0, 1}) Config.put([Pleroma.Web.Endpoint, :http, :ip], {127, 0, 0, 1})
Config.put([Pleroma.Plugs.RemoteIp, :enabled], false) assert RateLimiter.disabled?(Plug.Conn.assign(build_conn(), :remote_ip_found, false))
assert RateLimiter.disabled?() == true
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)
assert RateLimiter.disabled?() == true
end
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)
assert RateLimiter.disabled?() == false
end end
test "it restricts based on config values" do test "it restricts based on config values" do
@ -93,7 +74,6 @@ test "it restricts based on config values" do
refute conn.resp_body refute conn.resp_body
refute conn.halted refute conn.halted
end end
end
describe "options" do describe "options" do
test "`bucket_name` option overrides default bucket name" do test "`bucket_name` option overrides default bucket name" do

View File

@ -756,10 +756,6 @@ test "returns forbidden if token is invalid", %{conn: conn, valid_params: valid_
end end
describe "create account by app / rate limit" do 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 clear_config([:rate_limit, :app_account_creation]) do
Pleroma.Config.put([:rate_limit, :app_account_creation], {10_000, 2}) Pleroma.Config.put([:rate_limit, :app_account_creation], {10_000, 2})
end end