fix: throttle user registration email requests #20
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/w3-registration-throttle"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes W3 from bugs.md.
Adds
Aprsme.Accounts.RegistrationThrottle— a per-IP fixed-window budget (5/hour) checked before any insert or mail inAccounts.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.🤖 Skippy PR review
3 findings — 2 blocking before merge.
lib/aprsme/accounts.ex:126lib/aprsme/accounts/registration_throttle.ex:18lib/aprsme/accounts/registration_throttle.ex:32Reviewed
e98a2ca5ac24. Commentskippy reviewto re-run.@ -120,2 +125,2 @@|> User.registration_changeset(attrs, validate_email: false, validate_callsign: false)|> Repo.insert()def register_user(attrs, client_ip \\ nil) doif client_ip != nil and RegistrationThrottle.check(client_ip) == :throttled do🟡 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 beforeRepo.insert, so no DB work or mail happens first) —Repo.insert/1on an invalid changeset returns the same{:error, %Ecto.Changeset{}}it does today:@ -0,0 +15,4 @@alias Aprsme.RateLimiter@window to_timeout(hour: 1)@ip_limit 5🟡 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.@ -0,0 +29,4 @@def check(client_ip) docase RateLimiter.hit(ip_key(normalize_ip(client_ip)), @window, @ip_limit) do{:allow, _count} -> :ok{:deny, _retry_after_ms} -> :throttled🔵 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. OneLogger.warningor telemetry event on this branch, carrying the bucket key only (Logger.metadataalready hasremote_ip), makes that visible in prod.🤖 Skippy PR review
1 finding — 1 blocking before merge.
lib/aprsme/accounts/registration_throttle.ex:42Resolved 3 earlier findings fixed in
5917ed3: the budget is now drawn only oncechangeset.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. Commentskippy reviewto re-run.@ -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}")🟡 Warning — The denial log floods on a loop, and the bucket key embeds the raw IP
check/1logs 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/1is"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.ConfirmationThrottleandLoginThrottleboth 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 vianotify_allowed?.Mirror that: hash the address into the key, and bound the line itself —
or keep the raw IP deliberately and drop the claim that it is absent.
🤖 Skippy PR review
1 finding — none blocking.
test/aprsme/accounts/registration_throttle_test.exs:84\bafter 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. Commentskippy reviewto re-run.@ -0,0 +81,4 @@end)refute logs =~ ip_textassert Regex.match?(~r/bucket=register:ip:[A-Za-z0-9_-]{16}\b/, logs)🔵 Suggestion —
\bafter a base64url group flakes when the digest ends in-\brequires 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 isA-Za-z0-9_-):Regex.match?(~r/bucket=register:ip:[A-Za-z0-9_-]{16}\b/, "bucket=register:ip:AbCdEfGh0123456-")isfalse. 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 themflag, or~r/bucket=register:ip:\S{16}/.🤖 Skippy PR review
1 finding — none blocking.
test/aprsme/accounts/registration_throttle_test.exs:58Resolved 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. Commentskippy reviewto re-run.@ -0,0 +55,4 @@test "a denied loop logs one line per window, not one per submit", %{ip: ip} dologs =capture_log(fn ->assert :throttled = spend_budget(ip)🔵 Suggestion — Exact-count log assertion now races the async LiveView budget test
Moving
spend_budget/1inside the capture is right (the first denial is the one that logs), but the count at the end of this test is now== 1over a capture shared with every other async test:capture_log/1mutes the default handler and fans each Logger event out to all active captures, and Elixir's docs call this out forasync: true("messages from other tests might be captured").AprsmeWeb.UserRegistrationLiveTestis async, pins:registration_ip_limitto 2, and its budget test drives a third registration intolog_denial- emitting the byte-identicalregistration throttledline. 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 matchregistration throttled bucket=register:ip:#{digest}), or keep this moduleasync: false.Related, same file: the follow-up asserts assume the limit is frozen, but
spend_budget/1re-reads:registration_ip_limiton every hit, so a knob flip between the halting denial and the nextcheck/1(the LiveView test'son_exitdeleting the env back to the default 20) turns a real:throttledback into:ok- a narrow window, but the one path that makesassert :throttled = spend_budget(ip)followed byassert RegistrationThrottle.check(ip) == :throttledinconsistent.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 plainorigin/mainata27895f6(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.