From b519f902e8d30f49a3475ac07311f69722bba188 Mon Sep 17 00:00:00 2001 From: SimpleTest Date: Tue, 28 Jul 2026 05:51:15 +0300 Subject: [PATCH] Harden authentication token delivery limits --- .env.example | 3 + docs/architecture.md | 5 +- docs/performance.md | 25 ++++ docs/trust-safety.md | 21 ++-- lib/who_need_help/accounts.ex | 30 ++++- lib/who_need_help/trust/rate_limiter.ex | 108 +++++++++++++----- lib/who_need_help_web/auth_rate_limit.ex | 16 +++ lib/who_need_help_web/client_ip.ex | 20 ++++ .../controllers/google_auth_controller.ex | 14 ++- .../user_registration_controller.ex | 5 +- .../controllers/user_session_controller.ex | 12 +- .../controllers/user_settings_controller.ex | 10 +- lib/who_need_help_web/endpoint.ex | 5 + test/who_need_help/accounts_test.exs | 29 +++++ test/who_need_help/trust_safety_test.exs | 103 ++++++++++++++++- .../user_session_controller_test.exs | 35 ++++++ 16 files changed, 386 insertions(+), 55 deletions(-) create mode 100644 lib/who_need_help_web/auth_rate_limit.ex create mode 100644 lib/who_need_help_web/client_ip.ex diff --git a/.env.example b/.env.example index cd77860..e3a73c4 100644 --- a/.env.example +++ b/.env.example @@ -223,4 +223,7 @@ SUPPORT_INBOX_ADDRESS= CODEX_SESSION_ID=copy-the-main-local-codex-session-id # Optional shared PostgreSQL-backed policies. Keep {} until product thresholds are approved. # Shape: {"action_name":{"limit":POSITIVE_INTEGER,"window_seconds":POSITIVE_INTEGER}} +# Authentication delivery uses paired email/IP actions: +# registration_email + registration_ip, magic_link_email + magic_link_ip, +# password_login_email + password_login_ip, email_change_email + email_change_ip. RATE_LIMIT_POLICIES_JSON={} diff --git a/docs/architecture.md b/docs/architecture.md index b05f674..14a916d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -206,7 +206,10 @@ Action buckets live in PostgreSQL and use an atomic upsert keyed by action, hashed scope, and aligned time window. This works across all web replicas without an in-memory or Redis singleton. Policies are supplied through `RATE_LIMIT_POLICIES_JSON`; no numeric product policy is compiled into the -application. Expired buckets are pruned by the maintenance worker. +application. Authentication checks combine the email and client-IP buckets in +one `INSERT ... ON CONFLICT` statement. Failed transactional-email delivery +removes only the token created for that failed attempt. Expired buckets and +tokens are pruned by the maintenance worker. The Compose and kind scripts finish by subscribing on one live BEAM node, broadcasting through a different connected node, and failing if the PubSub diff --git a/docs/performance.md b/docs/performance.md index 3a601dc..4574a85 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -5,6 +5,31 @@ pool size, or autoscaling threshold is known yet. The repository therefore contains a reproducible measurement profile, not a capacity claim or blocking resource preflight. +## Authentication limiter micro-measurement + +Observed locally on 2026-07-28 with Elixir 1.20.2 / OTP 29.0.3, PostgreSQL +18.4, Docker 29.6.2, and the current uncommitted authentication-limiter +candidate on an AMD Ryzen 9 7950X3D workstation. PostgreSQL used an isolated +tmpfs data directory and the application connected over a Docker bridge. + +Each operation incremented an email bucket and an IP bucket together. A +telemetry-backed integration test confirmed that the operation executes one +PostgreSQL `INSERT ... ON CONFLICT` statement, not two independent counter +queries. + +| Observation | Result | +| --- | ---: | +| Sequential operations | 1,000 | +| Sequential p50 / p95 / p99 | 155 / 237 / 273 µs | +| Concurrent operations / concurrency | 1,000 / 50 | +| Concurrent elapsed time | 107.4 ms | +| Concurrent observed throughput | 9,308 operations/s | + +This isolates the limiter on a fast local tmpfs database. It does not include +SMTP delivery, account lookup, production storage latency, internet latency, +or production contention, and therefore is not a production capacity claim. +It provides no evidence that Redis is currently required. + ## Latest local candidate measurement Observed on 2026-07-25 at application commit diff --git a/docs/trust-safety.md b/docs/trust-safety.md index 26e7c23..e80ec32 100644 --- a/docs/trust-safety.md +++ b/docs/trust-safety.md @@ -88,17 +88,22 @@ verification. and configured action velocity create review signals. They do not automatically punish an account. - PostgreSQL action buckets enforce only operator-supplied policies across all - replicas. Registration and magic-link email scopes can also be configured. + replicas. Registration, password login, magic-link delivery, and email + changes can each be limited by both normalized email and client IP in one + atomic database statement. - No public exact location by default. No numeric rate policy is enabled by default because no threshold has been -approved or measured for this deployment. Supported action names currently -include `registration_email`, `magic_link_email`, `create_request`, -`accept_request`, `start`, `confirm`, `verify_handover`, `cancel_request`, -`withdraw_assignment`, `send_message`, `start_tracking`, `tracking_position`, -`review`, `report`, `block`, `category_proposal`, and `category_vote`. -Activity actions additionally include `create_activity`, `join_activity`, and -`send_activity_message`. +approved for this deployment. Authentication action pairs are +`registration_email`/`registration_ip`, +`magic_link_email`/`magic_link_ip`, +`password_login_email`/`password_login_ip`, and +`email_change_email`/`email_change_ip`. Other supported action names include +`create_request`, `accept_request`, `start`, `confirm`, `verify_handover`, +`cancel_request`, `withdraw_assignment`, `send_message`, `start_tracking`, +`tracking_position`, `review`, `report`, `block`, `category_proposal`, and +`category_vote`. Activity actions additionally include `create_activity`, +`join_activity`, and `send_activity_message`. The system does not claim to be bot-proof. Email confirmation, database uniqueness, unique-counterpart ranking, location evidence, velocity policies, diff --git a/lib/who_need_help/accounts.ex b/lib/who_need_help/accounts.ex index 0eaf618..a5f80b0 100644 --- a/lib/who_need_help/accounts.ex +++ b/lib/who_need_help/accounts.ex @@ -900,8 +900,9 @@ defmodule WhoNeedHelp.Accounts do when is_function(update_email_url_fun, 1) do {encoded_token, user_token} = UserToken.build_email_token(user, "change:#{current_email}") - Repo.insert!(user_token) - UserNotifier.deliver_update_email_instructions(user, update_email_url_fun.(encoded_token)) + persist_email_token_and_deliver(user_token, fn -> + UserNotifier.deliver_update_email_instructions(user, update_email_url_fun.(encoded_token)) + end) end @doc """ @@ -910,16 +911,20 @@ defmodule WhoNeedHelp.Accounts do def deliver_login_instructions(%User{} = user, magic_link_url_fun) when is_function(magic_link_url_fun, 1) do {encoded_token, user_token} = UserToken.build_email_token(user, "login") - Repo.insert!(user_token) - UserNotifier.deliver_login_instructions(user, magic_link_url_fun.(encoded_token)) + + persist_email_token_and_deliver(user_token, fn -> + UserNotifier.deliver_login_instructions(user, magic_link_url_fun.(encoded_token)) + end) end @doc "Delivers a one-time local-account verification link before connecting Google." def deliver_google_link_instructions(%User{} = user, magic_link_url_fun) when is_function(magic_link_url_fun, 1) do {encoded_token, user_token} = UserToken.build_email_token(user, "login") - Repo.insert!(user_token) - UserNotifier.deliver_google_link_instructions(user, magic_link_url_fun.(encoded_token)) + + persist_email_token_and_deliver(user_token, fn -> + UserNotifier.deliver_google_link_instructions(user, magic_link_url_fun.(encoded_token)) + end) end @doc """ @@ -938,6 +943,19 @@ defmodule WhoNeedHelp.Accounts do ## Token helper + defp persist_email_token_and_deliver(user_token, deliver_fun) do + persisted_token = Repo.insert!(user_token) + + case deliver_fun.() do + {:ok, _email} = delivered -> + delivered + + {:error, _reason} = error -> + Repo.delete_all(from token in UserToken, where: token.id == ^persisted_token.id) + error + end + end + defp before_moderation_user(query, nil), do: query defp before_moderation_user(query, {inserted_at, id}) do diff --git a/lib/who_need_help/trust/rate_limiter.ex b/lib/who_need_help/trust/rate_limiter.ex index 137f769..23cd7f7 100644 --- a/lib/who_need_help/trust/rate_limiter.ex +++ b/lib/who_need_help/trust/rate_limiter.ex @@ -11,15 +11,38 @@ defmodule WhoNeedHelp.Trust.RateLimiter do alias WhoNeedHelp.Repo alias WhoNeedHelp.Trust.RateLimitBucket - def check(action, scope) when is_atom(action), do: check(Atom.to_string(action), scope) + def check(action, scope) do + case check_many([{action, scope}]) do + {:ok, [limit]} -> {:ok, limit} + {:ok, []} -> {:ok, :not_configured} + {:error, :rate_limited} = error -> error + end + end - def check(action, scope) when is_binary(action) do - case policy(action) do - nil -> - {:ok, :not_configured} + @doc """ + Atomically increments every configured action/scope pair in one PostgreSQL + statement. - %{limit: limit, window_seconds: window_seconds} -> - increment(action, scope, limit, window_seconds) + Unconfigured actions are ignored. The call fails when any configured bucket + exceeds its policy, while still counting the whole attempt consistently. + """ + def check_many(action_scopes) when is_list(action_scopes) do + now = DateTime.utc_now(:second) + + prepared = + action_scopes + |> Enum.map(fn {action, scope} -> {normalize_action(action), scope} end) + |> Enum.uniq() + |> Enum.flat_map(fn {action, scope} -> + case policy(action) do + nil -> [] + policy -> [prepare_bucket(action, scope, policy, now)] + end + end) + + case prepared do + [] -> {:ok, []} + buckets -> increment_many(buckets) end end @@ -52,37 +75,70 @@ defmodule WhoNeedHelp.Trust.RateLimiter do ArgumentError -> nil end - defp increment(action, scope, limit, window_seconds) do - now = DateTime.utc_now(:second) + defp prepare_bucket(action, scope, %{limit: limit, window_seconds: window_seconds}, now) do unix = DateTime.to_unix(now) window_start = (div(unix, window_seconds) * window_seconds) |> DateTime.from_unix!() expires_at = DateTime.add(window_start, window_seconds, :second) scope_hash = :crypto.hash(:sha256, to_string(scope)) - {_count, [bucket]} = + %{ + action: action, + scope_hash: scope_hash, + window_started_at: window_start, + expires_at: expires_at, + limit: limit, + now: now + } + end + + defp increment_many(buckets) do + now = buckets |> hd() |> Map.fetch!(:now) + + rows = + Enum.map(buckets, fn bucket -> + %{ + id: Ecto.UUID.generate(), + action: bucket.action, + scope_hash: bucket.scope_hash, + window_started_at: bucket.window_started_at, + count: 1, + expires_at: bucket.expires_at, + inserted_at: now, + updated_at: now + } + end) + + {_count, returned} = Repo.insert_all( RateLimitBucket, - [ - %{ - id: Ecto.UUID.generate(), - action: action, - scope_hash: scope_hash, - window_started_at: window_start, - count: 1, - expires_at: expires_at, - inserted_at: now, - updated_at: now - } - ], - on_conflict: [inc: [count: 1], set: [expires_at: expires_at, updated_at: now]], + rows, + on_conflict: [inc: [count: 1], set: [updated_at: now]], conflict_target: [:action, :scope_hash, :window_started_at], - returning: [:count] + returning: [:action, :scope_hash, :count, :expires_at] ) - if bucket.count <= limit do - {:ok, %{count: bucket.count, limit: limit, resets_at: expires_at}} + limits = + Map.new(buckets, fn bucket -> + {{bucket.action, bucket.scope_hash}, bucket.limit} + end) + + results = + Enum.map(returned, fn bucket -> + %{ + action: bucket.action, + count: bucket.count, + limit: Map.fetch!(limits, {bucket.action, bucket.scope_hash}), + resets_at: bucket.expires_at + } + end) + + if Enum.all?(results, &(&1.count <= &1.limit)) do + {:ok, results} else {:error, :rate_limited} end end + + defp normalize_action(action) when is_atom(action), do: Atom.to_string(action) + defp normalize_action(action) when is_binary(action), do: action end diff --git a/lib/who_need_help_web/auth_rate_limit.ex b/lib/who_need_help_web/auth_rate_limit.ex new file mode 100644 index 0000000..3b1e45d --- /dev/null +++ b/lib/who_need_help_web/auth_rate_limit.ex @@ -0,0 +1,16 @@ +defmodule WhoNeedHelpWeb.AuthRateLimit do + @moduledoc """ + Applies account- and network-scoped authentication limits atomically. + """ + + alias Plug.Conn + alias WhoNeedHelp.Trust.RateLimiter + alias WhoNeedHelpWeb.ClientIp + + def check(%Conn{} = conn, email_action, email_scope, ip_action) do + RateLimiter.check_many([ + {email_action, email_scope}, + {ip_action, ClientIp.rate_limit_scope(conn)} + ]) + end +end diff --git a/lib/who_need_help_web/client_ip.ex b/lib/who_need_help_web/client_ip.ex new file mode 100644 index 0000000..2775962 --- /dev/null +++ b/lib/who_need_help_web/client_ip.ex @@ -0,0 +1,20 @@ +defmodule WhoNeedHelpWeb.ClientIp do + @moduledoc """ + Produces a stable, non-secret rate-limit scope from the client address. + + `Plug.RewriteOn` populates `conn.remote_ip` from the forwarding header that + Caddy sanitizes at the public edge. The PostgreSQL limiter hashes this value + before storing it. + """ + + alias Plug.Conn + + def rate_limit_scope(%Conn{remote_ip: address}) when is_tuple(address) do + case :inet.ntoa(address) do + value when is_list(value) -> List.to_string(value) + _invalid -> "unknown" + end + end + + def rate_limit_scope(_conn), do: "unknown" +end diff --git a/lib/who_need_help_web/controllers/google_auth_controller.ex b/lib/who_need_help_web/controllers/google_auth_controller.ex index 3d08b28..a37ccac 100644 --- a/lib/who_need_help_web/controllers/google_auth_controller.ex +++ b/lib/who_need_help_web/controllers/google_auth_controller.ex @@ -4,8 +4,7 @@ defmodule WhoNeedHelpWeb.GoogleAuthController do require Logger alias WhoNeedHelp.{Accounts, GoogleAuth, Locales, ProductAnalytics, Repo, Trust} - alias WhoNeedHelp.Trust.RateLimiter - alias WhoNeedHelpWeb.{GoogleAuthPending, UserAuth} + alias WhoNeedHelpWeb.{AuthRateLimit, GoogleAuthPending, UserAuth} import WhoNeedHelpWeb.UserAuth, only: [require_sudo_mode: 2] @@ -92,7 +91,13 @@ defmodule WhoNeedHelpWeb.GoogleAuthController do def complete_registration(conn, %{"google_registration" => params}) when is_map(params) do with {:ok, pending} <- GoogleAuthPending.fetch(conn), nil <- Accounts.get_user_by_email(pending.email), - {:ok, _limit} <- RateLimiter.check(:registration_email, pending.email), + {:ok, _limit} <- + AuthRateLimit.check( + conn, + :registration_email, + pending.email, + :registration_ip + ), {:ok, {user, _identity}} <- Accounts.register_user_by_google( identity_attrs(pending), @@ -156,7 +161,8 @@ defmodule WhoNeedHelpWeb.GoogleAuthController do %Accounts.User{moderation_status: status} = user when status != :suspended <- Accounts.get_user_by_email(pending.email), {:ok, pending_token} <- GoogleAuthPending.token(conn), - {:ok, _limit} <- RateLimiter.check(:magic_link_email, pending.email), + {:ok, _limit} <- + AuthRateLimit.check(conn, :magic_link_email, pending.email, :magic_link_ip), {:ok, _email} <- deliver_google_verification(conn, user, pending_token) do conn |> put_flash( diff --git a/lib/who_need_help_web/controllers/user_registration_controller.ex b/lib/who_need_help_web/controllers/user_registration_controller.ex index 0caddba..4f1b4bc 100644 --- a/lib/who_need_help_web/controllers/user_registration_controller.ex +++ b/lib/who_need_help_web/controllers/user_registration_controller.ex @@ -5,7 +5,7 @@ defmodule WhoNeedHelpWeb.UserRegistrationController do alias WhoNeedHelp.{Accounts, GoogleAuth, ProductAnalytics} alias WhoNeedHelp.Accounts.User - alias WhoNeedHelp.Trust.RateLimiter + alias WhoNeedHelpWeb.AuthRateLimit plug :put_no_store @@ -29,7 +29,8 @@ defmodule WhoNeedHelpWeb.UserRegistrationController do user_params = normalize_registration_params(user_params) email_scope = normalize_email_scope(user_params["email"]) - with {:ok, _limit} <- RateLimiter.check(:registration_email, email_scope), + with {:ok, _limit} <- + AuthRateLimit.check(conn, :registration_email, email_scope, :registration_ip), result <- Accounts.register_user(user_params) do case result do {:ok, user} -> diff --git a/lib/who_need_help_web/controllers/user_session_controller.ex b/lib/who_need_help_web/controllers/user_session_controller.ex index 5b894ac..2ab2cdc 100644 --- a/lib/who_need_help_web/controllers/user_session_controller.ex +++ b/lib/who_need_help_web/controllers/user_session_controller.ex @@ -5,8 +5,7 @@ defmodule WhoNeedHelpWeb.UserSessionController do alias WhoNeedHelp.{Accounts, GoogleAuth, Notifications} alias WhoNeedHelp.Accounts.Scope - alias WhoNeedHelp.Trust.RateLimiter - alias WhoNeedHelpWeb.{GoogleAuthPending, UserAuth} + alias WhoNeedHelpWeb.{AuthRateLimit, GoogleAuthPending, UserAuth} plug :assign_magic_link_form plug :assign_page_title @@ -58,7 +57,12 @@ defmodule WhoNeedHelpWeb.UserSessionController do when is_binary(email) and is_binary(password) do email_scope = email |> String.trim() |> String.downcase() - case RateLimiter.check(:password_login_email, email_scope) do + case AuthRateLimit.check( + conn, + :password_login_email, + email_scope, + :password_login_ip + ) do {:ok, _limit} -> if user = Accounts.get_user_by_email_and_password(email, password) do conn @@ -81,7 +85,7 @@ defmodule WhoNeedHelpWeb.UserSessionController do def create(conn, %{"user" => %{"email" => email}}) when is_binary(email) do email_scope = email |> String.trim() |> String.downcase() - case RateLimiter.check(:magic_link_email, email_scope) do + case AuthRateLimit.check(conn, :magic_link_email, email_scope, :magic_link_ip) do {:ok, _limit} -> case Accounts.get_user_by_email(email) do %Accounts.User{moderation_status: status} = user when status != :suspended -> diff --git a/lib/who_need_help_web/controllers/user_settings_controller.ex b/lib/who_need_help_web/controllers/user_settings_controller.ex index b8b0d17..0dca49c 100644 --- a/lib/who_need_help_web/controllers/user_settings_controller.ex +++ b/lib/who_need_help_web/controllers/user_settings_controller.ex @@ -4,8 +4,7 @@ defmodule WhoNeedHelpWeb.UserSettingsController do require Logger alias WhoNeedHelp.{Accounts, GoogleAuth} - alias WhoNeedHelp.Trust.RateLimiter - alias WhoNeedHelpWeb.UserAuth + alias WhoNeedHelpWeb.{AuthRateLimit, UserAuth} import WhoNeedHelpWeb.UserAuth, only: [require_sudo_mode: 2] @@ -29,7 +28,12 @@ defmodule WhoNeedHelpWeb.UserSettingsController do |> String.trim() |> String.downcase() - case RateLimiter.check(:email_change_email, new_email) do + case AuthRateLimit.check( + conn, + :email_change_email, + new_email, + :email_change_ip + ) do {:ok, _limit} -> deliver_email_change_instructions(conn, user, changeset) diff --git a/lib/who_need_help_web/endpoint.ex b/lib/who_need_help_web/endpoint.ex index 32215ca..d7bea78 100644 --- a/lib/who_need_help_web/endpoint.ex +++ b/lib/who_need_help_web/endpoint.ex @@ -42,6 +42,11 @@ defmodule WhoNeedHelpWeb.Endpoint do param_key: "request_logger", cookie_key: "request_logger" + # Caddy is the only public entry point and replaces incoming X-Forwarded-For + # instead of trusting a client-supplied value. Keep the application bound to + # loopback/private networks when this rewrite is enabled. + plug Plug.RewriteOn, [:x_forwarded_for] + plug Plug.RequestId plug Plug.Telemetry, event_prefix: [:phoenix, :endpoint] diff --git a/test/who_need_help/accounts_test.exs b/test/who_need_help/accounts_test.exs index 94a42ee..a63b3c0 100644 --- a/test/who_need_help/accounts_test.exs +++ b/test/who_need_help/accounts_test.exs @@ -1,3 +1,10 @@ +defmodule WhoNeedHelp.FailingMailerAdapter do + use Swoosh.Adapter + + @impl true + def deliver(_email, _config), do: {:error, :delivery_failed} +end + defmodule WhoNeedHelp.AccountsTest do use WhoNeedHelp.DataCase @@ -639,6 +646,28 @@ defmodule WhoNeedHelp.AccountsTest do assert user_token.sent_to == user.email assert user_token.context == "login" end + + test "removes the newly created token when delivery fails", %{user: user} do + previous = Application.get_env(:who_need_help, WhoNeedHelp.Mailer) + + Application.put_env( + :who_need_help, + WhoNeedHelp.Mailer, + adapter: WhoNeedHelp.FailingMailerAdapter + ) + + on_exit(fn -> + Application.put_env(:who_need_help, WhoNeedHelp.Mailer, previous) + end) + + assert {:error, :delivery_failed} = + Accounts.deliver_login_instructions(user, &"https://example.test/#{&1}") + + refute Repo.exists?( + from token in UserToken, + where: token.user_id == ^user.id and token.context == "login" + ) + end end describe "inspect/2 for the User module" do diff --git a/test/who_need_help/trust_safety_test.exs b/test/who_need_help/trust_safety_test.exs index 2c6f7c7..fef4541 100644 --- a/test/who_need_help/trust_safety_test.exs +++ b/test/who_need_help/trust_safety_test.exs @@ -7,7 +7,7 @@ defmodule WhoNeedHelp.TrustSafetyTest do alias WhoNeedHelp.Help.Assignment alias WhoNeedHelp.Repo alias WhoNeedHelp.Tracking.Position - alias WhoNeedHelp.Trust.{AbuseSignal, AuditEvent, RateLimiter} + alias WhoNeedHelp.Trust.{AbuseSignal, AuditEvent, RateLimitBucket, RateLimiter} setup do category = Catalog.seed_defaults() @@ -431,6 +431,107 @@ defmodule WhoNeedHelp.TrustSafetyTest do ) end + test "multiple configured scopes are counted in one limiter call", context do + old = Application.get_env(:who_need_help, :rate_limit_policies) + + Application.put_env(:who_need_help, :rate_limit_policies, %{ + "test_email" => %{"limit" => 1, "window_seconds" => 60}, + "test_ip" => %{"limit" => 2, "window_seconds" => 60} + }) + + on_exit(fn -> Application.put_env(:who_need_help, :rate_limit_policies, old) end) + + assert {:ok, results} = + RateLimiter.check_many([ + {:test_email, context.helper.email}, + {:test_ip, "203.0.113.10"} + ]) + + assert Map.new(results, &{&1.action, {&1.count, &1.limit}}) == %{ + "test_email" => {1, 1}, + "test_ip" => {1, 2} + } + + assert Enum.all?(results, &match?(%DateTime{}, &1.resets_at)) + + assert {:error, :rate_limited} = + RateLimiter.check_many([ + {:test_email, context.helper.email}, + {:test_ip, "203.0.113.10"} + ]) + + assert Repo.aggregate( + from(bucket in RateLimitBucket, where: bucket.action in ["test_email", "test_ip"]), + :sum, + :count + ) == 4 + end + + test "multiple limiter scopes use one PostgreSQL statement", context do + old = Application.get_env(:who_need_help, :rate_limit_policies) + + Application.put_env(:who_need_help, :rate_limit_policies, %{ + "query_count_email" => %{"limit" => 2, "window_seconds" => 60}, + "query_count_ip" => %{"limit" => 2, "window_seconds" => 60} + }) + + on_exit(fn -> Application.put_env(:who_need_help, :rate_limit_policies, old) end) + + handler_id = "rate-limiter-query-count-#{System.unique_integer([:positive])}" + test_process = self() + + :ok = + :telemetry.attach( + handler_id, + [:who_need_help, :repo, :query], + fn _event, _measurements, metadata, recipient -> + send(recipient, {:rate_limiter_query, metadata.query}) + end, + test_process + ) + + on_exit(fn -> :telemetry.detach(handler_id) end) + + assert {:ok, [_email, _ip]} = + RateLimiter.check_many([ + {:query_count_email, context.helper.email}, + {:query_count_ip, "203.0.113.20"} + ]) + + assert_receive {:rate_limiter_query, query} + assert query =~ ~s(INSERT INTO "rate_limit_buckets") + refute_receive {:rate_limiter_query, _another_query}, 50 + end + + test "concurrent limiter calls share one database counter across processes", context do + old = Application.get_env(:who_need_help, :rate_limit_policies) + + Application.put_env(:who_need_help, :rate_limit_policies, %{ + "concurrent_test" => %{"limit" => 10, "window_seconds" => 60} + }) + + on_exit(fn -> Application.put_env(:who_need_help, :rate_limit_policies, old) end) + + results = + 1..20 + |> Task.async_stream( + fn _attempt -> RateLimiter.check(:concurrent_test, context.helper.id) end, + max_concurrency: 20, + ordered: false, + timeout: 5_000 + ) + |> Enum.map(fn {:ok, result} -> result end) + + assert Enum.count(results, &match?({:ok, _}, &1)) == 10 + assert Enum.count(results, &match?({:error, :rate_limited}, &1)) == 10 + + assert Repo.one!( + from bucket in RateLimitBucket, + where: bucket.action == "concurrent_test", + select: bucket.count + ) == 20 + end + test "invalid request forms do not consume the publication rate limit", context do old = Application.get_env(:who_need_help, :rate_limit_policies) diff --git a/test/who_need_help_web/controllers/user_session_controller_test.exs b/test/who_need_help_web/controllers/user_session_controller_test.exs index 2b66817..2abdbb3 100644 --- a/test/who_need_help_web/controllers/user_session_controller_test.exs +++ b/test/who_need_help_web/controllers/user_session_controller_test.exs @@ -192,6 +192,41 @@ defmodule WhoNeedHelpWeb.UserSessionControllerTest do end) end + test "limits magic-link requests by forwarded client address", %{user: user} do + previous = Application.get_env(:who_need_help, :rate_limit_policies) + + Application.put_env(:who_need_help, :rate_limit_policies, %{ + "magic_link_email" => %{"limit" => 10, "window_seconds" => 60}, + "magic_link_ip" => %{"limit" => 1, "window_seconds" => 60} + }) + + on_exit(fn -> Application.put_env(:who_need_help, :rate_limit_policies, previous) end) + + params = %{"user" => %{"email" => user.email}} + + first = + build_conn() + |> put_req_header("x-forwarded-for", "203.0.113.10") + |> post(~p"/users/log-in", params) + + assert redirected_to(first) == ~p"/users/check-email?flow=login" + + limited = + build_conn() + |> put_req_header("x-forwarded-for", "203.0.113.10") + |> post(~p"/users/log-in", params) + + assert limited.status == 429 + assert html_response(limited, 429) =~ "Too many sign-in emails" + + other_address = + build_conn() + |> put_req_header("x-forwarded-for", "203.0.113.11") + |> post(~p"/users/log-in", params) + + assert redirected_to(other_address) == ~p"/users/check-email?flow=login" + end + test "does not send a magic link for a suspended user", %{conn: conn, user: user} do {:ok, user} = user