Merge pull request 'Harden rate limiter to deal with configuration issues' (#7795) from gitlab-mr-iid-4418 into develop

Reviewed-on: https://git.pleroma.social/pleroma/pleroma/pulls/7795

changelog.d/rate-limiter-hardening.fix

@@ -0,0 +1 @@
+Stop the rate limiter from crashing when run with wrong settings.

lib/pleroma/config_db.ex

@@ -10,6 +10,7 @@ defmodule Pleroma.ConfigDB do
   import Pleroma.Web.Gettext
 
   alias __MODULE__
+  alias Pleroma.EctoType.Config.RateLimit
   alias Pleroma.Repo
 
   @type t :: %__MODULE__{}
@@ -60,8 +61,59 @@ defmodule Pleroma.ConfigDB do
     |> cast(params, [:key, :group, :value])
     |> validate_required([:key, :group, :value])
     |> unique_constraint(:key, name: :config_group_key_index)
+    |> validate_rate_limit()
   end
 
+  defp validate_rate_limit(changeset) do
+    group = get_field(changeset, :group)
+    key = get_field(changeset, :key)
+
+    if group == :pleroma and key == :rate_limit do
+      value = get_field(changeset, :value)
+
+      case normalize_rate_limit(value) do
+        {:ok, normalized_value} ->
+          put_change(changeset, :value, normalized_value)
+
+        {:error, {limiter_name, reason}} ->
+          add_error(
+            changeset,
+            :value,
+            "invalid :rate_limit value for #{inspect(limiter_name)}: #{reason}"
+          )
+      end
+    else
+      changeset
+    end
+  end
+
+  defp normalize_rate_limit(nil), do: {:ok, nil}
+
+  defp normalize_rate_limit(%{} = value), do: normalize_rate_limit(Map.to_list(value))
+
+  defp normalize_rate_limit(value) when is_list(value) do
+    if Keyword.keyword?(value) do
+      value
+      |> Enum.reduce_while({:ok, []}, fn {limiter_name, limiter_value}, {:ok, acc} ->
+        case RateLimit.cast_with_error(limiter_value) do
+          {:ok, normalized_limiter_value} ->
+            {:cont, {:ok, [{limiter_name, normalized_limiter_value} | acc]}}
+
+          {:error, reason} ->
+            {:halt, {:error, {limiter_name, reason}}}
+        end
+      end)
+      |> case do
+        {:ok, acc} -> {:ok, Enum.reverse(acc)}
+        {:error, _} = error -> error
+      end
+    else
+      {:error, {:rate_limit, "must be a keyword list"}}
+    end
+  end
+
+  defp normalize_rate_limit(_), do: {:error, {:rate_limit, "must be a keyword list"}}
+
   defp create(params) do
     %ConfigDB{}
     |> changeset(params)

lib/pleroma/ecto_type/config/rate_limit.ex

@@ -0,0 +1,71 @@
+# Pleroma: A lightweight social networking server
+# Copyright © Pleroma Authors <https://pleroma.social/>
+# SPDX-License-Identifier: AGPL-3.0-only
+
+defmodule Pleroma.EctoType.Config.RateLimit do
+  @moduledoc false
+
+  use Ecto.Type
+
+  @type t ::
+          nil
+          | {non_neg_integer(), non_neg_integer()}
+          | [{non_neg_integer(), non_neg_integer()}]
+
+  @impl true
+  def type, do: :term
+
+  @impl true
+  def cast(value) do
+    case cast_with_error(value) do
+      {:ok, normalized} -> {:ok, normalized}
+      {:error, _reason} -> :error
+    end
+  end
+
+  @impl true
+  def load(value), do: cast(value)
+
+  @impl true
+  def dump(value), do: cast(value)
+
+  @spec cast_with_error(term()) :: {:ok, t()} | {:error, String.t()}
+  def cast_with_error(nil), do: {:ok, nil}
+
+  def cast_with_error({scale, limit}) do
+    with {:ok, scale} <- parse_integer(scale, "scale"),
+         {:ok, limit} <- parse_integer(limit, "limit"),
+         true <- scale >= 1 and limit >= 1 do
+      {:ok, {scale, limit}}
+    else
+      false -> {:error, "scale and limit must be >= 1"}
+      {:error, reason} -> {:error, reason}
+    end
+  end
+
+  def cast_with_error([{_, _} = unauth, {_, _} = auth]) do
+    with {:ok, unauth} <- cast_with_error(unauth),
+         {:ok, auth} <- cast_with_error(auth) do
+      {:ok, [unauth, auth]}
+    else
+      {:error, reason} -> {:error, reason}
+    end
+  end
+
+  def cast_with_error(_),
+    do:
+      {:error, "must be a {scale, limit} tuple, a [{scale, limit}, {scale, limit}] list, or nil"}
+
+  defp parse_integer(value, _label) when is_integer(value), do: {:ok, value}
+
+  defp parse_integer(value, label) when is_binary(value) do
+    value = String.trim(value)
+
+    case Integer.parse(value) do
+      {number, ""} -> {:ok, number}
+      _ -> {:error, "#{label} must be an integer"}
+    end
+  end
+
+  defp parse_integer(_value, label), do: {:error, "#{label} must be an integer"}
+end

lib/pleroma/web/plugs/rate_limiter.ex

@@ -67,6 +67,8 @@ defmodule Pleroma.Web.Plugs.RateLimiter do
   import Plug.Conn
 
   alias Pleroma.Config
+  alias Pleroma.Config.Holder
+  alias Pleroma.EctoType.Config.RateLimit
   alias Pleroma.User
   alias Pleroma.Web.Plugs.RateLimiter.LimiterSupervisor
 
@@ -143,7 +145,7 @@ defmodule Pleroma.Web.Plugs.RateLimiter do
 
   def action_settings(plug_opts) do
     with limiter_name when is_atom(limiter_name) <- plug_opts[:name],
-         limits when not is_nil(limits) <- Config.get([:rate_limit, limiter_name]) do
+         {:ok, limits} <- fetch_and_normalize_limits(limiter_name) do
       bucket_name_root = Keyword.get(plug_opts, :bucket_name, limiter_name)
 
       %{
@@ -151,6 +153,72 @@ defmodule Pleroma.Web.Plugs.RateLimiter do
         limits: limits,
         opts: plug_opts
       }
+    else
+      :disabled -> nil
+    end
+  end
+
+  defp fetch_and_normalize_limits(limiter_name) do
+    limits = Config.get([:rate_limit, limiter_name])
+
+    case normalize_limits(limits) do
+      {:ok, limits} ->
+        {:ok, limits}
+
+      :disabled ->
+        :disabled
+
+      :error ->
+        default_limits =
+          Holder.default_config(:pleroma, :rate_limit)
+          |> get_default_limits(limiter_name)
+
+        case normalize_limits(default_limits) do
+          {:ok, normalized_limits} ->
+            warn_invalid_limits_once(limiter_name, limits)
+            {:ok, normalized_limits}
+
+          _ ->
+            warn_invalid_limits_once(limiter_name, limits)
+            :disabled
+        end
+    end
+  end
+
+  defp get_default_limits(%{} = rate_limit, limiter_name), do: Map.get(rate_limit, limiter_name)
+
+  defp get_default_limits(rate_limit, limiter_name) when is_list(rate_limit) do
+    if Keyword.keyword?(rate_limit) do
+      Keyword.get(rate_limit, limiter_name)
+    else
+      nil
+    end
+  end
+
+  defp get_default_limits(_, _), do: nil
+
+  @invalid_limits_warned_key {__MODULE__, :invalid_limits_warned}
+
+  defp warn_invalid_limits_once(limiter_name, limits) do
+    warned = :persistent_term.get(@invalid_limits_warned_key, MapSet.new())
+
+    if MapSet.member?(warned, limiter_name) do
+      :ok
+    else
+      :persistent_term.put(@invalid_limits_warned_key, MapSet.put(warned, limiter_name))
+
+      Logger.warning(
+        "Invalid rate limiter config for #{inspect(limiter_name)}: #{inspect(limits)}. Falling back to defaults or disabling this limiter."
+      )
+    end
+  end
+
+  defp normalize_limits(nil), do: :disabled
+
+  defp normalize_limits(limits) do
+    case RateLimit.cast(limits) do
+      {:ok, normalized_limits} -> {:ok, normalized_limits}
+      :error -> :error
     end
   end
 

test/pleroma/config_db_test.exs

@@ -174,6 +174,22 @@ defmodule Pleroma.ConfigDBTest do
       assert updated1.value == [groups: [c: 3, d: 4], key: [a: 1, b: 2]]
       assert updated2.value == [mascots: [c: 3, d: 4], key: [a: 1, b: 2]]
     end
+
+    test "rejects invalid :rate_limit values (e.g. empty-string scale from AdminFE)" do
+      assert {:error, _changeset} =
+               ConfigDB.update_or_create(%{
+                 group: ":pleroma",
+                 key: ":rate_limit",
+                 value: [
+                   %{
+                     "tuple" => [
+                       ":statuses_actions",
+                       [%{"tuple" => ["", 0]}, %{"tuple" => ["", ""]}]
+                     ]
+                   }
+                 ]
+               })
+    end
   end
 
   describe "delete/1" do

test/pleroma/web/plugs/rate_limiter_test.exs

@@ -268,6 +268,23 @@ defmodule Pleroma.Web.Plugs.RateLimiterTest do
     refute {:err, :not_found} == RateLimiter.inspect_bucket(conn, limiter_name, opts)
   end
 
+  test "doesn't crash if rate limit scale is invalid (e.g. broken DB config)" do
+    limiter_name = :test_invalid_rate_limit_config
+
+    clear_config([:rate_limit, limiter_name], [{"", 0}, {"", ""}])
+    clear_config([Pleroma.Web.Endpoint, :http, :ip], {8, 8, 8, 8})
+
+    opts = RateLimiter.init(name: limiter_name)
+
+    conn = %{build_conn(:get, "/") | remote_ip: {127, 0, 0, 1}}
+
+    conn_limited = RateLimiter.call(conn, opts)
+
+    refute conn_limited.status == Conn.Status.code(:too_many_requests)
+    refute conn_limited.resp_body
+    refute conn_limited.halted
+  end
+
   def expire_ttl(%{remote_ip: remote_ip} = _conn, bucket_name_root) do
     bucket_name = "anon:#{bucket_name_root}" |> String.to_atom()
     key_name = "ip::#{remote_ip |> Tuple.to_list() |> Enum.join(".")}"