Fix auth throttle crash, session disconnect on password change, TOTP gating #24

graham · 2026-09-22T11:23:18-05:00

graham commented

2026-09-22 11:23:18 -05:00

Audit auth/security group.

AuthThrottle 500'd on non-map user param — now reaches the controller's 400.
Password change/reset broadcasts disconnect on live_socket_id topics — open LiveView sessions are evicted.
TOTP enrollment behind require_sudo_mode (moduledoc claimed it); password reset clears TOTP + recovery codes.
user_return_to survives the pending-2FA session renewal.
Email-change mailer failure logged + honest flash.

Verified: 137 targeted tests pass; xref under threshold.

Audit auth/security group. - AuthThrottle 500'd on non-map user param — now reaches the controller's 400. - Password change/reset broadcasts disconnect on live_socket_id topics — open LiveView sessions are evicted. - TOTP enrollment behind require_sudo_mode (moduledoc claimed it); password reset clears TOTP + recovery codes. - user_return_to survives the pending-2FA session renewal. - Email-change mailer failure logged + honest flash. Verified: 137 targeted tests pass; xref under threshold.

graham added 1 commit

2026-09-22 11:23:19 -05:00

Fix auth throttle crash, session disconnect on password change, TOTP gating

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

d341b46ea1

- AuthThrottle no longer 500s when the 'user' form param is not a map
- Password change/reset now broadcasts disconnect to live_socket_id
  sessions so open LiveViews are signed out
- TOTP enrollment (create/confirm) is behind require_sudo_mode as its
  moduledoc claimed; password reset clears TOTP + recovery codes
- user_return_to survives the pending-2FA session renewal
- Email-change mailer failure is logged and flashed instead of reported
  as sent

skippy-bot reviewed

2026-09-22 11:29:00 -05:00

skippy-bot left a comment

🤖 Skippy PR review

2 findings — 2 blocking before merge.

Severity	Location	Issue
🟠 High	`lib/microwaveprop/accounts.ex:467`	Password reset silently disables the second factor
🟡 Warning	`lib/microwaveprop_web/controllers/user_totp_controller.ex:26`	Teardown is an unthrottled password-verification endpoint

_{Reviewed d341b46ea10e. Comment skippy review to re-run.}

### 🤖 Skippy PR review **2 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `lib/microwaveprop/accounts.ex:467` | Password reset silently disables the second factor | | 🟡 Warning | `lib/microwaveprop_web/controllers/user_totp_controller.ex:26` | Teardown is an unthrottled password-verification endpoint | <sub>Reviewed `d341b46ea10e`. Comment `skippy review` to re-run.</sub>

lib/microwaveprop/accounts.ex Outdated

					
				@ -453,3 +465,3 @@

				    user

				    |> User.password_changeset(attrs)

				    |> update_user_and_delete_all_tokens()

				    |> Ecto.Changeset.change(totp_secret: nil, totp_confirmed_at: nil, totp_last_used_at: nil)

🟠 High — Password reset silently disables the second factor

reset_user_password/2 nils the TOTP secret and clear_totp: true deletes the recovery codes, so the factor that exists to survive a mailbox compromise is removed by a mailbox compromise: an attacker who can read (or intercept, or forward) the reset mail completes the reset and logs in with only the new password. The docstring's "attacker-enrolled factor" case needs the account password inside the sudo window to reach enrollment at all (this PR's own require_sudo_mode), so it is a much rarer path than mailbox access. Require the second factor on the reset form instead: when totp_enabled?(user) is true, add a code field to edit/2 and verify it (or a recovery code) in update/2 before calling this function, then rotate the secret and mail the owner a notice. At minimum keep the secret and say in the post-reset flash that two-factor was turned off.

**🟠 High — Password reset silently disables the second factor** `reset_user_password/2` nils the TOTP secret and `clear_totp: true` deletes the recovery codes, so the factor that exists to survive a mailbox compromise is removed *by* a mailbox compromise: an attacker who can read (or intercept, or forward) the reset mail completes the reset and logs in with only the new password. The docstring's "attacker-enrolled factor" case needs the account password inside the sudo window to reach enrollment at all (this PR's own `require_sudo_mode`), so it is a much rarer path than mailbox access. Require the second factor on the reset form instead: when `totp_enabled?(user)` is true, add a code field to `edit/2` and verify it (or a recovery code) in `update/2` before calling this function, then rotate the secret and mail the owner a notice. At minimum keep the secret and say in the post-reset flash that two-factor was turned off.

skippy-bot marked this conversation as resolved

lib/microwaveprop_web/controllers/user_totp_controller.ex Outdated

					
				@ -20,2 +23,4 @@

				  alias Microwaveprop.Accounts

				  plug :require_sudo_mode when action in [:create, :confirm]

🟡 Warning — Teardown is an unthrottled password-verification endpoint

delete/2 spends a full bcrypt Accounts.disable_totp/2 password check per request, and it is the one password-checking action here that is neither sudo-gated nor throttled: the :browser pipeline has no AuthThrottle and this plug covers only [:create, :confirm], while login, two-factor and the reset forms all wire one. The response is a clean oracle (302 with "That password is not correct." versus "Two-factor authentication is off..."), so the hijacked session this PR is hardening against can grind the account password at one bcrypt per request with no limit and no lockout, then re-login through the (per-IP/per-identifier) login form and use the sudo window to change the email. Either add :delete to the sudo-gated set so the guess has to pass the throttled login form, or throttle it directly: plug AuthThrottle, [scope: :totp_teardown, id_path: nil, ip_limit: 10] when action in [:delete] (the account lives in the session, so there is no identifier to bucket on — same shape as the :two_factor scope).

**🟡 Warning — Teardown is an unthrottled password-verification endpoint** `delete/2` spends a full bcrypt `Accounts.disable_totp/2` password check per request, and it is the one password-checking action here that is neither sudo-gated nor throttled: the `:browser` pipeline has no `AuthThrottle` and this plug covers only `[:create, :confirm]`, while login, two-factor and the reset forms all wire one. The response is a clean oracle (302 with "That password is not correct." versus "Two-factor authentication is off..."), so the hijacked session this PR is hardening against can grind the account password at one bcrypt per request with no limit and no lockout, then re-login through the (per-IP/per-identifier) login form and use the sudo window to change the email. Either add `:delete` to the sudo-gated set so the guess has to pass the throttled login form, or throttle it directly: `plug AuthThrottle, [scope: :totp_teardown, id_path: nil, ip_limit: 10] when action in [:delete]` (the account lives in the session, so there is no identifier to bucket on — same shape as the `:two_factor` scope).

skippy-bot marked this conversation as resolved

graham force-pushed fix/auth-security from d341b46ea1

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

to d35fb328aa

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

2026-09-22 12:04:18 -05:00

Compare

graham force-pushed fix/auth-security from d35fb328aa

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

to 77b4649dcb

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

2026-09-22 12:53:17 -05:00

Compare

graham commented

2026-09-22 12:53:26 -05:00

Addressed in 77b4649d:

Reset strips 2FA — update/2 now verifies a TOTP or recovery code via Accounts.verify_totp/2 before calling reset_user_password/2 when totp_enabled?(user). The form renders a code field only for factor-enabled accounts, and PUT /users/reset-password/:token is AuthThrottle'd per token. Clearing the factor after a verified reset is kept so the owner re-enrolls cleanly (and an attacker-enrolled factor can't survive).
Unthrottled teardown oracle — :delete now sits behind require_sudo_mode alongside :create/:confirm, so the password re-check runs inside the throttled re-authentication path.

Addressed in 77b4649d: 1. **Reset strips 2FA** — `update/2` now verifies a TOTP or recovery code via `Accounts.verify_totp/2` before calling `reset_user_password/2` when `totp_enabled?(user)`. The form renders a code field only for factor-enabled accounts, and `PUT /users/reset-password/:token` is AuthThrottle'd per token. Clearing the factor after a verified reset is kept so the owner re-enrolls cleanly (and an attacker-enrolled factor can't survive). 2. **Unthrottled teardown oracle** — `:delete` now sits behind `require_sudo_mode` alongside `:create`/`:confirm`, so the password re-check runs inside the throttled re-authentication path.

graham merged commit 7a6f2a939a into main

2026-09-22 12:53:41 -05:00

graham referenced this pull request from a commit

2026-09-22 12:53:42 -05:00

Merge pull request 'Fix auth throttle crash, session disconnect on password change, TOTP gating' (#24) from fix/auth-security into main

skippy-bot reviewed

2026-09-22 12:56:17 -05:00

skippy-bot left a comment

🤖 Skippy PR review

3 findings — 2 blocking before merge.

Severity	Location	Issue
🟡 Warning	`lib/microwaveprop_web/controllers/user_reset_password_controller.ex:77`	The second factor is spent before the new password is validated
🟡 Warning	`lib/microwaveprop_web/controllers/user_reset_password_html/edit.html.heex:51`	inputmode="numeric" makes the recovery-code path unusable on a phone
🔵 Suggestion	`lib/microwaveprop_web/controllers/user_reset_password_controller.ex:80`	The success flash does not say the second factor was turned off

Resolved 2 earlier findings: the reset form now verifies a live TOTP or recovery code, and teardown is behind require_sudo_mode. Nothing from the earlier review is still open.

_{Reviewed 77b4649dcb4d. Comment skippy review to re-run.}

### 🤖 Skippy PR review **3 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `lib/microwaveprop_web/controllers/user_reset_password_controller.ex:77` | The second factor is spent before the new password is validated | | 🟡 Warning | `lib/microwaveprop_web/controllers/user_reset_password_html/edit.html.heex:51` | inputmode="numeric" makes the recovery-code path unusable on a phone | | 🔵 Suggestion | `lib/microwaveprop_web/controllers/user_reset_password_controller.ex:80` | The success flash does not say the second factor was turned off | Resolved 2 earlier findings: the reset form now verifies a live TOTP or recovery code, and teardown is behind require_sudo_mode. Nothing from the earlier review is still open. <sub>Reviewed `77b4649dcb4d`. Comment `skippy review` to re-run.</sub>

lib/microwaveprop_web/controllers/user_reset_password_controller.ex

					
				@ -72,0 +74,4 @@

				    # too — otherwise mailbox access alone strips the factor that exists

				    # to survive a mailbox compromise. `reset_user_password/2` still

				    # clears TOTP afterwards so the owner re-enrolls cleanly.

				    with :ok <- require_totp(user, user_params),

🟡 Warning — The second factor is spent before the new password is validated

require_totp/2 runs before Accounts.reset_user_password/2, and verification is what consumes the factor: a live TOTP code stamps totp_last_used_at, so the replay guard then refuses that same code for the rest of its 30 s step, and a recovery code stamps used_at permanently. A password rejected by User.password_changeset (12+ chars, mixed classes, denylist: a first attempt fails often) therefore burns the code and only then re-renders the form, where the user's still-valid code is now reported as "did not match", and a recovery-code user silently spends one of their 8 on a reset that never happened. Validate the password first, verify the factor second:

changeset = Accounts.change_user_password(user, user_params)

with :ok <- validate_password(changeset),
     :ok <- require_totp(user, user_params),
     {:ok, _} <- Accounts.reset_user_password(user, user_params) do

where validate_password/1 returns {:error, changeset} when changeset.valid? is false.

**🟡 Warning — The second factor is spent before the new password is validated** `require_totp/2` runs before `Accounts.reset_user_password/2`, and verification is what consumes the factor: a live TOTP code stamps `totp_last_used_at`, so the replay guard then refuses that same code for the rest of its 30 s step, and a recovery code stamps `used_at` permanently. A password rejected by `User.password_changeset` (12+ chars, mixed classes, denylist: a first attempt fails often) therefore burns the code and only then re-renders the form, where the user's still-valid code is now reported as "did not match", and a recovery-code user silently spends one of their 8 on a reset that never happened. Validate the password first, verify the factor second: ```elixir changeset = Accounts.change_user_password(user, user_params) with :ok <- validate_password(changeset), :ok <- require_totp(user, user_params), {:ok, _} <- Accounts.reset_user_password(user, user_params) do ``` where `validate_password/1` returns `{:error, changeset}` when `changeset.valid?` is false.

lib/microwaveprop_web/controllers/user_reset_password_controller.ex

					
				@ -72,0 +77,4 @@

				    with :ok <- require_totp(user, user_params),

				         {:ok, _} <- Accounts.reset_user_password(user, user_params) do

				      conn

				      |> put_flash(:info, "Password reset successfully.")

🔵 Suggestion — The success flash does not say the second factor was turned off

reset_user_password/2 clears the secret and deletes the recovery codes, but the flash only reports the password change, so an account that had two-factor on comes back with one factor and no notice: the owner expects a code prompt at the next login, does not get one, and has no reason to open settings. Since the new code-side check means the reset can only be completed by someone holding a factor, this is now the residual of the earlier finding rather than a bypass. Say it in the flash ("Your password was reset. Two-factor authentication was turned off, so set it up again in Settings.") or mail the account.

**🔵 Suggestion — The success flash does not say the second factor was turned off** `reset_user_password/2` clears the secret and deletes the recovery codes, but the flash only reports the password change, so an account that had two-factor on comes back with one factor and no notice: the owner expects a code prompt at the next login, does not get one, and has no reason to open settings. Since the new code-side check means the reset can only be completed by someone holding a factor, this is now the residual of the earlier finding rather than a bypass. Say it in the flash ("Your password was reset. Two-factor authentication was turned off, so set it up again in Settings.") or mail the account.

lib/microwaveprop_web/controllers/user_reset_password_html/edit.html.heex

					
				@ -45,0 +48,4 @@

				        type="text"

				        label="Two-factor code (or a recovery code)"

				        autocomplete="one-time-code"

				        inputmode="numeric"

🟡 Warning — inputmode="numeric" makes the recovery-code path unusable on a phone

Recovery codes are 16-character base32 (UserTotpRecoveryCode.build_set/1 uses Base.encode32), so they contain letters, but this field asks for a numeric keypad: iOS and Android both present a digits-only keyboard for inputmode="numeric", so a phone user who lost their authenticator and is resetting with a recovery code cannot type it into the field whose label tells them to. The login prompt solves the same problem with inputmode="text" (user_session_html/two_factor.html.heex:24) while its subtitle invites recovery codes. Use inputmode="text" here as well; nothing about the field needs a digit-only keyboard, since six-digit codes type fine on the default one.

**🟡 Warning — inputmode="numeric" makes the recovery-code path unusable on a phone** Recovery codes are 16-character base32 (`UserTotpRecoveryCode.build_set/1` uses `Base.encode32`), so they contain letters, but this field asks for a numeric keypad: iOS and Android both present a digits-only keyboard for `inputmode="numeric"`, so a phone user who lost their authenticator and is resetting with a recovery code cannot type it into the field whose label tells them to. The login prompt solves the same problem with `inputmode="text"` (`user_session_html/two_factor.html.heex:24`) while its subtitle invites recovery codes. Use `inputmode="text"` here as well; nothing about the field needs a digit-only keyboard, since six-digit codes type fine on the default one.

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/prop!24

Rows
Columns