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

5 files changed, 51 insertions, 65 deletions

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