Merge branch 'fix/rate-limiter-remoteip-behavior' into 'develop'

rate limiter: disable based on if remote ip was found, not on if the plug was enabled

Closes #1620

See merge request pleroma/pleroma!2296

config/test.exs

@@ -92,6 +92,8 @@
 
 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

lib/pleroma/plugs/rate_limiter/rate_limiter.ex

@@ -78,7 +78,7 @@ def init(plug_opts) 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 @@ def call(conn, plug_opts) do
   end
 
   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
   end
@@ -109,16 +109,21 @@ defp handle(conn, action_settings) do
     end
   end
 
-  def disabled? do
+  def disabled?(conn) do
     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
 
   @inspect_bucket_not_found {:error, :not_found}

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

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 Pleroma.Web.ConnCase
-  use Plug.Test
 
   alias Pleroma.Config
   alias Pleroma.Plugs.RateLimiter
@@ -36,63 +35,44 @@ test "config is required for plug to work" do
                |> RateLimiter.init()
                |> RateLimiter.action_settings()
     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})
+    assert RateLimiter.disabled?(Plug.Conn.assign(build_conn(), :remote_ip_found, false))
-      Config.put([Pleroma.Plugs.RemoteIp, :enabled], false)
+  end
 
-      assert RateLimiter.disabled?() == true
+  test "it restricts based on config values" do
-    end
+    limiter_name = :test_plug_opts
+    scale = 80
+    limit = 5
 
-    test "it is disabled for socket" do
+    Config.put([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8})
-      Config.put([:rate_limit, @limiter_name], {1, 1})
+    Config.put([:rate_limit, limiter_name], {scale, limit})
-      Config.put([Pleroma.Web.Endpoint, :http, :ip], {:local, "/path/to/pleroma.sock"})
-      Config.put([Pleroma.Plugs.RemoteIp, :enabled], false)
 
-      assert RateLimiter.disabled?() == true
+    plug_opts = RateLimiter.init(name: limiter_name)
-    end
+    conn = conn(:get, "/")
-
-    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
-
-    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
 
+    for i <- 1..5 do
       conn = RateLimiter.call(conn, plug_opts)
-      assert %{"error" => "Throttled"} = Phoenix.ConnTest.json_response(conn, :too_many_requests)
+      assert {^i, _} = RateLimiter.inspect_bucket(conn, limiter_name, plug_opts)
-      assert conn.halted
+      Process.sleep(10)
-
-      Process.sleep(50)
-
-      conn = conn(:get, "/")
-
-      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
+
+    conn = RateLimiter.call(conn, plug_opts)
+    assert %{"error" => "Throttled"} = Phoenix.ConnTest.json_response(conn, :too_many_requests)
+    assert conn.halted
+
+    Process.sleep(50)
+
+    conn = conn(:get, "/")
+
+    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
 
   describe "options" do

test/web/mastodon_api/controllers/account_controller_test.exs

@@ -756,10 +756,6 @@ test "returns forbidden if token is invalid", %{conn: conn, valid_params: valid_
   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