fix: throttle user registration email requests #20

Merged
graham merged 6 commits from fix/w3-registration-throttle into main 2026-09-15 17:39:20 -05:00
Owner

Fixes W3 from bugs.md.

Adds Aprsme.Accounts.RegistrationThrottle — a per-IP fixed-window budget (5/hour) checked before any insert or mail in Accounts.register_user/2. The LiveView resolves the client IP at mount and renders the existing generic notice when throttled, so a looping client gets an indistinguishable response and no confirmation mail is relayed. Internal callers (fixtures, seeding) omit the IP and stay unlimited.

Verification: focused tests passed (57), full suite passed (2236), mix credo --strict, and commit hooks including Dialyzer passed.

Fixes W3 from bugs.md. Adds `Aprsme.Accounts.RegistrationThrottle` — a per-IP fixed-window budget (5/hour) checked before any insert or mail in `Accounts.register_user/2`. The LiveView resolves the client IP at mount and renders the existing generic notice when throttled, so a looping client gets an indistinguishable response and no confirmation mail is relayed. Internal callers (fixtures, seeding) omit the IP and stay unlimited. Verification: focused tests passed (57), full suite passed (2236), `mix credo --strict`, and commit hooks including Dialyzer passed.
fix: throttle user registration email requests
Some checks failed
Elixir CI / Dialyzer (pull_request) Successful in 40s
Elixir CI / Build and test (pull_request) Successful in 1m44s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread
e98a2ca5ac
skippy-bot left a comment

🤖 Skippy PR review

3 findings — 2 blocking before merge.

Severity Location Issue
🟡 Warning lib/aprsme/accounts.ex:126 Failed-validation submits burn the registration budget
🟡 Warning lib/aprsme/accounts/registration_throttle.ex:18 5/hour per IP is a shared-egress lockout, and the refusal is silent
🔵 Suggestion lib/aprsme/accounts/registration_throttle.ex:32 Denials leave no trace anywhere

Reviewed e98a2ca5ac24. Comment skippy review to re-run.

### 🤖 Skippy PR review **3 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `lib/aprsme/accounts.ex:126` | Failed-validation submits burn the registration budget | | 🟡 Warning | `lib/aprsme/accounts/registration_throttle.ex:18` | 5/hour per IP is a shared-egress lockout, and the refusal is silent | | 🔵 Suggestion | `lib/aprsme/accounts/registration_throttle.ex:32` | Denials leave no trace anywhere | <sub>Reviewed `e98a2ca5ac24`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -120,2 +125,2 @@
|> User.registration_changeset(attrs, validate_email: false, validate_callsign: false)
|> Repo.insert()
def register_user(attrs, client_ip \\ nil) do
if client_ip != nil and RegistrationThrottle.check(client_ip) == :throttled do
First-time contributor

🟡 Warning — Failed-validation submits burn the registration budget

The budget is drawn before the changeset is validated, so any submit — including one that only fails local validation (short password, malformed email) — costs a unit. A normal user who mistypes the form 5 times in an hour and then submits a correct one gets {:error, :throttled}, the LiveView renders the "check your inbox" notice, no account exists and no mail was sent, and there is no error for them to act on. Draw the budget after the changeset is known valid (still before Repo.insert, so no DB work or mail happens first) — Repo.insert/1 on an invalid changeset returns the same {:error, %Ecto.Changeset{}} it does today:

def register_user(attrs, client_ip \\ nil) do
  changeset =
    User.registration_changeset(%User{}, attrs, validate_email: false, validate_callsign: false)

  if changeset.valid? and client_ip != nil and RegistrationThrottle.check(client_ip) == :throttled do
    {:error, :throttled}
  else
    Repo.insert(changeset)
  end
end
**🟡 Warning — Failed-validation submits burn the registration budget** The budget is drawn before the changeset is validated, so any submit — including one that only fails local validation (short password, malformed email) — costs a unit. A normal user who mistypes the form 5 times in an hour and then submits a correct one gets `{:error, :throttled}`, the LiveView renders the "check your inbox" notice, no account exists and no mail was sent, and there is no error for them to act on. Draw the budget after the changeset is known valid (still before `Repo.insert`, so no DB work or mail happens first) — `Repo.insert/1` on an invalid changeset returns the same `{:error, %Ecto.Changeset{}}` it does today: ```elixir def register_user(attrs, client_ip \\ nil) do changeset = User.registration_changeset(%User{}, attrs, validate_email: false, validate_callsign: false) if changeset.valid? and client_ip != nil and RegistrationThrottle.check(client_ip) == :throttled do {:error, :throttled} else Repo.insert(changeset) end end ```
skippy-bot marked this conversation as resolved
@ -0,0 +15,4 @@
alias Aprsme.RateLimiter
@window to_timeout(hour: 1)
@ip_limit 5
First-time contributor

🟡 Warning — 5/hour per IP is a shared-egress lockout, and the refusal is silent

The bucket is the resolved client IP, and mobile carriers (CGNAT) or an office NAT put thousands of people behind one address: the sixth person to register from that egress within the hour gets the success-shaped notice, no account, no error, and no hint that retrying is futile. Because the deny is deliberately rendered like a success, that lockout is invisible to both the user and the operator. 5/hour is also a weak bound for the W3 threat — each relaying attempt costs the attacker a distinct address anyway — so something like 20–50/hour per IP still bounds the mail a loop can cause while leaving room for shared egress. Either way, keep a knob (config/Application.get_env) for this instead of a module attribute so it can be raised without a release when a carrier subnet gets locked out.

**🟡 Warning — 5/hour per IP is a shared-egress lockout, and the refusal is silent** The bucket is the resolved client IP, and mobile carriers (CGNAT) or an office NAT put thousands of people behind one address: the sixth person to register from that egress within the hour gets the success-shaped notice, no account, no error, and no hint that retrying is futile. Because the deny is deliberately rendered like a success, that lockout is invisible to both the user and the operator. 5/hour is also a weak bound for the W3 threat — each relaying attempt costs the attacker a distinct address anyway — so something like 20–50/hour per IP still bounds the mail a loop can cause while leaving room for shared egress. Either way, keep a knob (config/`Application.get_env`) for this instead of a module attribute so it can be raised without a release when a carrier subnet gets locked out.
skippy-bot marked this conversation as resolved
@ -0,0 +29,4 @@
def check(client_ip) do
case RateLimiter.hit(ip_key(normalize_ip(client_ip)), @window, @ip_limit) do
{:allow, _count} -> :ok
{:deny, _retry_after_ms} -> :throttled
First-time contributor

🔵 Suggestion — Denials leave no trace anywhere

{:deny, _retry_after_ms} is collapsed to :throttled, the caller renders the accepted-registration notice, and nothing logs or emits telemetry — so a throttle that is refusing every real user (e.g. a trusted-proxy misconfiguration collapsing all clients onto one ingress IP) looks exactly like a quiet hour. One Logger.warning or telemetry event on this branch, carrying the bucket key only (Logger.metadata already has remote_ip), makes that visible in prod.

**🔵 Suggestion — Denials leave no trace anywhere** `{:deny, _retry_after_ms}` is collapsed to `:throttled`, the caller renders the accepted-registration notice, and nothing logs or emits telemetry — so a throttle that is refusing every real user (e.g. a trusted-proxy misconfiguration collapsing all clients onto one ingress IP) looks exactly like a quiet hour. One `Logger.warning` or telemetry event on this branch, carrying the bucket key only (`Logger.metadata` already has `remote_ip`), makes that visible in prod.
skippy-bot marked this conversation as resolved
fix: throttle valid registrations only, raise and configure the limit, log denials
Some checks failed
Elixir CI / Build and test (pull_request) Successful in 59s
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
Elixir CI / Dialyzer (pull_request) Successful in 2m48s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
5917ed3c64
skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity Location Issue
🟡 Warning lib/aprsme/accounts/registration_throttle.ex:42 The denial log floods on a loop, and the bucket key embeds the raw IP

Resolved 3 earlier findings fixed in 5917ed3: the budget is now drawn only once changeset.valid? (so a mistyped form no longer spends the allowance), the limit is raised to 20 and moved behind :registration_ip_limit (the LiveView test drives the knob), and denials now log. The new commit moved the budget draw in accounts.ex and rewrote the throttle's check/1, which is where the one new finding sits.

Reviewed 5917ed3c6470. Comment skippy review to re-run.

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `lib/aprsme/accounts/registration_throttle.ex:42` | The denial log floods on a loop, and the bucket key embeds the raw IP | Resolved 3 earlier findings fixed in 5917ed3: the budget is now drawn only once `changeset.valid?` (so a mistyped form no longer spends the allowance), the limit is raised to 20 and moved behind `:registration_ip_limit` (the LiveView test drives the knob), and denials now log. The new commit moved the budget draw in accounts.ex and rewrote the throttle's check/1, which is where the one new finding sits. <sub>Reviewed `5917ed3c6470`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -0,0 +39,4 @@
# trusted-proxy regression collapsing all clients onto one ingress IP
# looks exactly like a quiet hour otherwise). Bucket key only — the
# IP itself is metadata the logger already carries as remote_ip.
Logger.warning("registration throttled bucket=#{key}")
First-time contributor

🟡 Warning — The denial log floods on a loop, and the bucket key embeds the raw IP

check/1 logs a line on every denied submit, and deny is the cheap path (no DB, no mail), so the loop this throttle exists to bound can still write unbounded WARN volume from one client.

The message also carries the raw client address, because ip_key/1 is "register:ip:#{ip}" — the inline comment ("Bucket key only — the IP itself is metadata the logger already carries") is not true of the string it interpolates. ConfirmationThrottle and LoginThrottle both document the opposite rule (identifiers are hashed into bucket keys "so raw addresses never land in ETS, logs or telemetry") and log a truncated digest, gated to one line per hour per account via notify_allowed?.

Mirror that: hash the address into the key, and bound the line itself —

key = ip_key(normalize_ip(client_ip))

if match?({:allow, _}, RateLimiter.hit("register:notify:#{key}", @window, 1)) do
  Logger.warning("registration throttled bucket=#{key}")
end

or keep the raw IP deliberately and drop the claim that it is absent.

**🟡 Warning — The denial log floods on a loop, and the bucket key embeds the raw IP** `check/1` logs a line on **every** denied submit, and deny is the cheap path (no DB, no mail), so the loop this throttle exists to bound can still write unbounded WARN volume from one client. The message also carries the raw client address, because `ip_key/1` is `"register:ip:#{ip}"` — the inline comment ("Bucket key only — the IP itself is metadata the logger already carries") is not true of the string it interpolates. `ConfirmationThrottle` and `LoginThrottle` both document the opposite rule (identifiers are hashed into bucket keys "so raw addresses never land in ETS, logs or telemetry") and log a truncated digest, gated to one line per hour per account via `notify_allowed?`. Mirror that: hash the address into the key, and bound the line itself — ```elixir key = ip_key(normalize_ip(client_ip)) if match?({:allow, _}, RateLimiter.hit("register:notify:#{key}", @window, 1)) do Logger.warning("registration throttled bucket=#{key}") end ``` or keep the raw IP deliberately and drop the claim that it is absent.
skippy-bot marked this conversation as resolved
fix: register_user doc reflects validation-first budget draw
Some checks failed
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
Elixir CI / Build and test (pull_request) Successful in 54s
Elixir CI / Dialyzer (pull_request) Successful in 3m3s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
885c8b6915
fix: hash registration throttle keys and cap denial logs
All checks were successful
Elixir CI / Build and test (pull_request) Successful in 57s
skippy-bot/review Skippy review: clean — 1 non-blocking note open
Elixir CI / Dialyzer (pull_request) Successful in 2m48s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
82c9750be7
The bucket key embedded the raw client address in ETS and in the
denial log line despite the comment claiming otherwise, and check/1
logged on every denied submit — the cheap path — so one looping client
could write unbounded WARN volume. Hash the address into the key
(truncated sha256 digest, like the sibling throttles' identifiers) and
gate the log to one line per bucket per window via a RateLimiter notify
bucket, mirroring LoginThrottle.
skippy-bot left a comment

🤖 Skippy PR review

1 finding — none blocking.

Severity Location Issue
🔵 Suggestion test/aprsme/accounts/registration_throttle_test.exs:84 \b after a base64url group flakes when the digest ends in -

Resolved the one earlier finding (5917ed3's denial log): the refusals are now capped at one line per bucket per window via the RateLimiter notify gate, and ip_key hashes the source, so the log can no longer write unbounded WARN volume or carry a raw address. The log gate itself is correct - Hammer's hit/3 with a limit of 1 allows the first denial and denies the rest.

Reviewed 82c9750be730. Comment skippy review to re-run.

### 🤖 Skippy PR review **1 finding** — none blocking. | Severity | Location | Issue | | --- | --- | --- | | 🔵 Suggestion | `test/aprsme/accounts/registration_throttle_test.exs:84` | `\b` after a base64url group flakes when the digest ends in `-` | Resolved the one earlier finding (5917ed3's denial log): the refusals are now capped at one line per bucket per window via the RateLimiter notify gate, and ip_key hashes the source, so the log can no longer write unbounded WARN volume or carry a raw address. The log gate itself is correct - Hammer's hit/3 with a limit of 1 allows the first denial and denies the rest. <sub>Reviewed `82c9750be730`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -0,0 +81,4 @@
end)
refute logs =~ ip_text
assert Regex.match?(~r/bucket=register:ip:[A-Za-z0-9_-]{16}\b/, logs)
First-time contributor

🔵 Suggestion — \b after a base64url group flakes when the digest ends in -

\b requires a word character on one side, and - is not one, so this assertion fails whenever the 16th character of the digest is - (that is 1 run in 64, since the alphabet is A-Za-z0-9_-): Regex.match?(~r/bucket=register:ip:[A-Za-z0-9_-]{16}\b/, "bucket=register:ip:AbCdEfGh0123456-") is false. The key is correct in that run, so CI goes red on a passing implementation, with a message that reads like the raw address leaked. Anchor on what actually follows the key instead, e.g. ~r/bucket=register:ip:[A-Za-z0-9_-]{16}$/ with the m flag, or ~r/bucket=register:ip:\S{16}/.

**🔵 Suggestion — `\b` after a base64url group flakes when the digest ends in `-`** `\b` requires a word character on one side, and `-` is not one, so this assertion fails whenever the 16th character of the digest is `-` (that is 1 run in 64, since the alphabet is `A-Za-z0-9_-`): `Regex.match?(~r/bucket=register:ip:[A-Za-z0-9_-]{16}\b/, "bucket=register:ip:AbCdEfGh0123456-")` is `false`. The key is correct in that run, so CI goes red on a passing implementation, with a message that reads like the raw address leaked. Anchor on what actually follows the key instead, e.g. `~r/bucket=register:ip:[A-Za-z0-9_-]{16}$/` with the `m` flag, or `~r/bucket=register:ip:\S{16}/`.
skippy-bot marked this conversation as resolved
test: make registration throttle tests env-agnostic
All checks were successful
Elixir CI / Build and test (pull_request) Successful in 54s
Elixir CI / Dialyzer (pull_request) Successful in 2m49s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
skippy-bot/review Skippy review: clean — 1 non-blocking note open
a6274b0015
The limit knob is global Application env owned by the LiveView budget
test, so pinning it from a serial module was safe but heavy, and
looping a fixed count at the default would race its concurrent value.
Spend the budget by looping until the first refusal instead, keeping
the module async. Also drop a trailing \\b from the digest pattern: a
digest ending in '-' has no word boundary before the newline, so the
match flaked on 1/64 of sources.
skippy-bot left a comment

🤖 Skippy PR review

1 finding — none blocking.

Severity Location Issue
🔵 Suggestion test/aprsme/accounts/registration_throttle_test.exs:58 Exact-count log assertion now races the async LiveView budget test

Resolved the open suggestion from 82c9750: the test no longer anchors on \b, so a digest ending in - no longer fails the assertion. The one new note below is the async capture that this commit introduces.

Reviewed a6274b001542. Comment skippy review to re-run.

### 🤖 Skippy PR review **1 finding** — none blocking. | Severity | Location | Issue | | --- | --- | --- | | 🔵 Suggestion | `test/aprsme/accounts/registration_throttle_test.exs:58` | Exact-count log assertion now races the async LiveView budget test | Resolved the open suggestion from 82c9750: the test no longer anchors on \b, so a digest ending in - no longer fails the assertion. The one new note below is the async capture that this commit introduces. <sub>Reviewed `a6274b001542`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -0,0 +55,4 @@
test "a denied loop logs one line per window, not one per submit", %{ip: ip} do
logs =
capture_log(fn ->
assert :throttled = spend_budget(ip)
First-time contributor

🔵 Suggestion — Exact-count log assertion now races the async LiveView budget test

Moving spend_budget/1 inside the capture is right (the first denial is the one that logs), but the count at the end of this test is now == 1 over a capture shared with every other async test: capture_log/1 mutes the default handler and fans each Logger event out to all active captures, and Elixir's docs call this out for async: true ("messages from other tests might be captured"). AprsmeWeb.UserRegistrationLiveTest is async, pins :registration_ip_limit to 2, and its budget test drives a third registration into log_denial - emitting the byte-identical registration throttled line. If that lands inside this window the scan returns 2 and the test fails on a correct implementation, intermittently, which is the kind of red CI that gets blamed on the wrong commit. Scope the count to this bucket instead of the shared phrase (compute the digest in the test and match registration throttled bucket=register:ip:#{digest}), or keep this module async: false.

Related, same file: the follow-up asserts assume the limit is frozen, but spend_budget/1 re-reads :registration_ip_limit on every hit, so a knob flip between the halting denial and the next check/1 (the LiveView test's on_exit deleting the env back to the default 20) turns a real :throttled back into :ok - a narrow window, but the one path that makes assert :throttled = spend_budget(ip) followed by assert RegistrationThrottle.check(ip) == :throttled inconsistent.

**🔵 Suggestion — Exact-count log assertion now races the async LiveView budget test** Moving `spend_budget/1` inside the capture is right (the first denial is the one that logs), but the count at the end of this test is now `== 1` over a capture shared with every other async test: `capture_log/1` mutes the default handler and fans each Logger event out to all active captures, and Elixir's docs call this out for `async: true` ("messages from other tests might be captured"). `AprsmeWeb.UserRegistrationLiveTest` is async, pins `:registration_ip_limit` to 2, and its budget test drives a third registration into `log_denial` - emitting the byte-identical `registration throttled` line. If that lands inside this window the scan returns 2 and the test fails on a correct implementation, intermittently, which is the kind of red CI that gets blamed on the wrong commit. Scope the count to this bucket instead of the shared phrase (compute the digest in the test and match `registration throttled bucket=register:ip:#{digest}`), or keep this module `async: false`. Related, same file: the follow-up asserts assume the limit is frozen, but `spend_budget/1` re-reads `:registration_ip_limit` on every hit, so a knob flip between the halting denial and the next `check/1` (the LiveView test's `on_exit` deleting the env back to the default 20) turns a real `:throttled` back into `:ok` - a narrow window, but the one path that makes `assert :throttled = spend_budget(ip)` followed by `assert RegistrationThrottle.check(ip) == :throttled` inconsistent.
merge: resolve alias conflict with main (password reset throttle)
Some checks failed
skippy-bot/review Skippy review: clean — 1 non-blocking note open
Elixir CI / Build and test (pull_request) Failing after 58s
Elixir CI / Dialyzer (pull_request) Successful in 43s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
ce625e126f
Author
Owner

Heads-up on the suite run for ce625e12: 2271/2272, with the single failure inherited from main, not introduced here.

AprsmeWeb.StatusLive.IndexTest — "mount via HTTP renders the connected-path template when cache has a live status" (assert html =~ "N0CALL") — is deterministically red on plain origin/main at a27895f6 (repro: MIX_ENV=test mix test test/aprsme_web/live/status_live/index_test.exs; verified in a scratch worktree, since removed). This branch was green on the same test in two full pre-merge runs, and none of its changes touch the status path — the merge commit only combines the alias block (both throttles kept, alphabetized).

That break needs its own fix on main (likely the PR 31-era status sanitize vs this PR-7-era test); flagging rather than fixing it inside this PR.

Heads-up on the suite run for `ce625e12`: 2271/2272, with the single failure inherited from main, not introduced here. `AprsmeWeb.StatusLive.IndexTest` — "mount via HTTP renders the connected-path template when cache has a live status" (`assert html =~ "N0CALL"`) — is deterministically red on plain `origin/main` at a27895f6 (repro: `MIX_ENV=test mix test test/aprsme_web/live/status_live/index_test.exs`; verified in a scratch worktree, since removed). This branch was green on the same test in two full pre-merge runs, and none of its changes touch the status path — the merge commit only combines the alias block (both throttles kept, alphabetized). That break needs its own fix on main (likely the PR 31-era status sanitize vs this PR-7-era test); flagging rather than fixing it inside this PR.
graham merged commit 04c1bc003f into main 2026-09-15 17:39:20 -05:00
graham deleted branch fix/w3-registration-throttle 2026-09-15 17:39:21 -05:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
graham/aprs.me!20
No description provided.