fix(auth): the admin MFA reset accepted the caller's own account (BACKLOG #1022) - #590
Open
wshallwshall wants to merge 15 commits into
Open
fix(auth): the admin MFA reset accepted the caller's own account (BACKLOG #1022)#590wshallwshall wants to merge 15 commits into
wshallwshall wants to merge 15 commits into
Conversation
…KLOG #1022)
`POST /users/{user_id}/reset-mfa` had no self-exclusion. Its sibling `reset_user_password` has
refused `user_id == identity.user_id` since ASVS 6.4.6, twenty-five lines above in the same file --
and reset-MFA is the sharper of the two: one call clears the TOTP secret, every recovery code and
every passkey.
WHY IT MATTERS, and it is not "an admin resetting themselves is odd". `AuthService.disable_mfa` and
`delete_webauthn_credential` BOTH refuse when they would drop the caller to zero factors while MFA
is required (ADR 0068 decision 5). Pointing the ADMIN reset at your own account was a THIRD route to
zero factors that skipped both. `disable_mfa`'s own docstring names this item for exactly that
reason: two self-service routes to zero factors behind one step-up gate, and only one of them asked.
*** THE CONSTRAINT I WAS GIVEN DID NOT HOLD AS STATED, AND CHECKING IT CHANGED WHAT I WROTE. ***
The brief said to put the guard at the ROUTE and never inside `admin_reset_mfa`, because that
function is the always-available recovery for a locked-out passkey user. The second half is true and
I honoured it. The stated REASON is not: `grep -rn admin_reset_mfa` shows the route is its ONLY
caller in the engine, and there is no CLI path, so guarding the route IS guarding the only path --
the distinction cannot be about which function holds the line.
It survives on a different and better argument, which the comment now records: SELF-TARGETING IS
NEVER RECOVERY. Reaching the route requires passing `require_step_up_action(... ADMIN_RESET_MFA
...)`, which is itself MFA-gated, so an operator who has genuinely lost every factor cannot call it
at all. What the refusal removes is the zero-factor bypass; what it leaves untouched is cross-user
reset, which is the actual recovery path. Had the relayed reason been the real one, this change
would have stranded a sole administrator.
TEST: one case, both halves. The self-target must 400, AND a DIFFERENT user must still reset
successfully -- without the second assertion the test passes just as well if the route were broken
outright, which is the shape that reads as coverage and is not.
MUTATION: guard removed -> the test fails. Restored from a byte copy, hash-verified identical.
DOCS -- BOTH SITES WERE ASSERTING THE OPPOSITE OF THE CODE, which is the SDS-3.7 shape:
docs/SECURITY.md said `DELETE /me/mfa` has "No last-factor guard ... it does not refuse when it
would leave the account with zero enrolled factors", and pointed at this item for the asymmetry.
FALSE: `AuthService.disable_mfa` raises on exactly that condition at service.py:2375, with the
same message as the passkey path. The asymmetry is closed; the row now records the refusal.
docs/adr/0068 said "TOTP-disable keeps its existing behavior this lane (parity follow-up
recorded)". That follow-up has landed. Corrected in place, with the retraction kept rather than
the sentence quietly swapped.
The reset-mfa row in SECURITY.md's route table now carries the self-exclusion and, beside it, the
fact that cross-user reset is untouched -- so a reader cannot take the new refusal as a
narrowing of the recovery path.
NON-GOALS HELD, all three named in the brief: no hunt for the AC-10/AC-11 contradiction (the row
proves it is not on main), no change to login-time MFA enforcement, no attempt at the check-then-act
race (ruled out of scope).
VERIFIED, scope named:
pytest 158 passed test_api_auth plus the gates asserting on these two documents
(test_docs_security_pathways, test_security_doc_rate_limits, test_asvs_residual_lint,
test_cutover_slug_rot)
ruff format --check, ruff check, mypy -- run separately, each with its own exit code, all clean
NOT a full-suite run
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
wshallwshall
enabled auto-merge (squash)
August 25, 2026 18:46
…has something to find This PR's own body already said "ledger progress note in a companion PR" -- that companion never arrived, which is exactly why the required "must update BACKLOG.md" check has been failing. Also corrects the note's own wording: it described records (1) and (2) as already fixed "on main", which was never true and became actively false once this PR's ledger-less state landed without it -- corrected to say what's actually true, that this same PR is what fixes them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
reset-MFA had no self-exclusion while its sibling reset-password has had one since ASVS 6.4.6. Targeting yourself was a third route to zero factors that skipped the last-factor refusal both self-service paths make.
Item #1022 stays OPEN -- per standing guidance this is one limb of a multi-limb row, not a closure. Ledger progress note in a companion PR.
Two things a reviewer may ask, both answered in the commit:
admin_reset_mfadoesn't hold -- the route is that function's only caller, so guarding the route already guards the only path. It survives on a different argument: self-targeting is never recovery, because reaching the route requires an MFA-gated step-up a locked-out operator can't pass. Cross-user reset is untouched, and the test pins that explicitly.DELETE /me/mfahas no last-factor guard (it does, service.py:2375), and ADR 0068 said the parity follow-up was outstanding (it landed). Retractions kept in place rather than sentences silently swapped.Verification, explicitly scoped: 158 passed over test_api_auth plus the four gates asserting on those documents. ruff/mypy strict clean, each run separately. Guard mutation-proved: removing it reds the test; restored and hash-verified. Full suite not run.