fix: throttle password reset email requests #19
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/w2-reset-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 W2 from bugs.md.
Adds
Accounts.request_password_reset/3with a two-bucket fixed-window throttle (per-source and per-address, hashed email keys), consulted before any database lookup. The LiveView resolves the client IP viaRateLimiter.extract_socket_ip/1at mount and renders one fixed response for every address, closing the mail-flood and enumeration vectors on the WebSocket path. Reset token minting now replaces outstanding tokens instead of accumulating them, and mail delivery moved into an unlinked task.Verification: focused tests passed (53), full suite passed (2237),
mix credo --strict, and commit hooks including Dialyzer passed.🤖 Skippy PR review
2 findings — none blocking.
lib/aprsme/accounts/password_reset_throttle.ex:53lib/aprsme/accounts.ex:570Reviewed
53c16b149c5c. Commentskippy reviewto re-run.@ -533,0 +567,4 @@case get_user_by_email(email) do%User{} = user ->url = mint_reset_password_url(user, reset_password_url_fun)_task = Task.start(fn -> UserNotifier.deliver_reset_password_instructions(user, url) end)🔵 Suggestion — A failed reset mail is now completely unobservable
UserNotifier.deliver_reset_password_instructions/2returns{:error, reason}whenMailer.deliver/1fails, and the unlinked task drops it: no flash (correct — the old error flash only fired for registered addresses, so it was itself the enumeration oracle) and now no log either. During a mail-provider outage every reset request looks successful to the user and leaves no trace for ops. Log the failure inside the task — this module has norequire Loggeryet — keyed on the user id or the address hash rather than the raw address, to keep the throttle's no-PII-in-logs stance. The confirmation path has the same shape (line 449), so ignore if that silence was a deliberate call there too.@ -0,0 +50,4 @@enddefp check_address(hash) docase RateLimiter.hit(email_key(hash), @window, @email_limit) do🔵 Suggestion — Per-address bucket is never exercised from a second source
The address bucket is only ever hit from the one source the added LiveView test drives, so nothing in the suite fails if the address key later becomes source-scoped (e.g.
"pwreset:email:#{ip}:#{hash}") — a realistic change, since scoping it per source would remove the "any client can silence a victim's reset mail" trade-off. That change silently restores the mail-flood vector this PR closes and every added test still goes green.resend_confirmation_instructions/3has dedicated coverage for exactly this intest/aprsme/accounts_test.exs("the cooldown follows the address across sources", "the per-source budget refuses a client spraying many addresses") using the privateunique_ip/0helper. Port both forrequest_password_reset/3: after1..3requests foruser.emailfrom oneunique_ip(), a 4th from a freshunique_ip()must return:okand leave the minted token unchanged.Resolved 2 of 2 earlier findings:
PasswordResetThrottleper-address bucket is now exercised from a second source:the address bucket follows the address across sourcesfails if the address key ever becomes source-scoped (it would mint a 4th token).deliver_reset_instructions/2logs{:error, reason}from the unlinked task, keyed onuser.id, so a provider outage is no longer invisible.0 still open. Nothing new in
53c16b1..359151b(comment reflow plus those two changes only).Reviewed
359151b1e450.