From f565139abcca37a5cbc17d2c4ccf0fe77fa07c3d Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 25 Aug 2026 13:41:57 -0500 Subject: [PATCH 1/2] fix(auth): the admin MFA reset accepted the caller's own account (BACKLOG #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 --- docs/SECURITY.md | 4 +- ...8-browser-webauthn-passkeys-offloopback.md | 8 ++- messagefoundry/api/auth_routes.py | 26 ++++++++ tests/test_api_auth.py | 64 +++++++++++++++++++ 4 files changed, 98 insertions(+), 4 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 85fdec2ae..dec4ba74f 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -341,7 +341,7 @@ tuple: they act only on the caller's own account. | `GET` | `/me/mfa` | `require` | | | `POST` | `/me/mfa/enroll` | `require_reauth_only_action` (action `mfa_enroll`) | password-only step-up — the MFA gate is skipped so a required-but-unenrolled user cannot deadlock | | `POST` | `/me/mfa/confirm` | `require_reauth_only_action` (action `mfa_confirm`) | per-actor ceremony limiter; password-only step-up | -| `DELETE` | `/me/mfa` | `require_step_up_action` (action `mfa_disable`) | step-up bound to the disable action (current factor + a fresh password). ⚠️ **No last-factor guard** — this is the TOTP path (`disable_mfa`), and it does **not** refuse when it would leave the account with zero enrolled factors. The passkey removal path does refuse; see BACKLOG #1022 for the asymmetry | +| `DELETE` | `/me/mfa` | `require_step_up_action` (action `mfa_disable`) | step-up bound to the disable action (current factor + a fresh password). **Refuses (400) when TOTP is your last second factor and MFA is required for your account** — the same refusal, on the same condition, as the passkey removal path (`AuthService.disable_mfa`, ADR 0068 decision 5). The asymmetry this row used to record is closed (BACKLOG #1022) | | `GET` | `/me/sessions` | `require` | | | `GET` | `/me/security-events` | `require` | | | `DELETE` | `/me/sessions/{session_id}` | `require_reauth_only_action` (action `session_terminate`) | password-only step-up, bound to the action (ASVS 7.5.2): a login-seeded window does not unlock a terminate | @@ -364,7 +364,7 @@ tuple: they act only on the caller's own account. | `DELETE` | `/users/{user_id}/sessions` | `users:manage` | `require_step_up` | | `PUT` | `/users/{user_id}/roles` | `users:manage` | `require_step_up` | | `POST` | `/users/{user_id}/reset-password` | `users:manage` | `require_step_up_action` (action `admin_reset_password`) | -| `POST` | `/users/{user_id}/reset-mfa` | `users:manage` | `require_step_up_action` (action `admin_reset_mfa`) | +| `POST` | `/users/{user_id}/reset-mfa` | `users:manage` | `require_step_up_action` (action `admin_reset_mfa`); **refuses (400) when `user_id` is the caller's own** — use the self-service MFA settings instead. Targeting yourself here was a third route to zero factors that skipped the last-factor refusal both self-service paths make (BACKLOG #1022). Cross-user reset is untouched: it is the always-available recovery for a locked-out passkey user (ADR 0068 §2) | | `GET` | `/users/{user_id}/channel-scope` | `users:manage` | `require` (a read on the `users:manage` tier, not `users:read`) | | `PUT` | `/users/{user_id}/channel-scope` | `users:manage` | `require_step_up` | | `GET` | `/ad-group-map` | `users:manage` | `require` | diff --git a/docs/adr/0068-browser-webauthn-passkeys-offloopback.md b/docs/adr/0068-browser-webauthn-passkeys-offloopback.md index f6f5ed091..285053f8f 100644 --- a/docs/adr/0068-browser-webauthn-passkeys-offloopback.md +++ b/docs/adr/0068-browser-webauthn-passkeys-offloopback.md @@ -136,8 +136,12 @@ TOTP-specific (a WebAuthn-only user's TOTP code gets "not enrolled", never a loc **No WebAuthn recovery codes** — they are phishable knowledge secrets that undercut the phishing-resistant tier. Recovery = enroll ≥2 passkeys (UI nudge) / keep TOTP alongside / `admin_reset_mfa`, which is **extended to also delete all WebAuthn credentials**. Deleting the -**last remaining second factor while MFA is required is refused** ("enroll another factor first"); -TOTP-disable keeps its existing behavior this lane (parity follow-up recorded). Documented +**last remaining second factor while MFA is required is refused** ("enroll another factor first"). +*The parity follow-up this lane deferred has since landed: `AuthService.disable_mfa` now refuses on +the same condition and with the same message, so the two self-service paths to zero factors are +symmetric. The ADMIN reset additionally refuses when an operator targets their own account, which +was a third route to zero factors that skipped both (BACKLOG #1022) — `admin_reset_mfa` itself stays +unguarded and cross-user reset is unchanged, so the recovery path below is intact.* Documented consequence: a passkey-only local user cannot satisfy the TOTP-shaped JSON `/auth/mfa-verify`, so desktop-console step-up actions become unavailable to them (enroll-page warning; owner-accepted). An extra-less install with enrolled credentials gets a **startup advisory** naming diff --git a/messagefoundry/api/auth_routes.py b/messagefoundry/api/auth_routes.py index ff1417c14..0be44ee9d 100644 --- a/messagefoundry/api/auth_routes.py +++ b/messagefoundry/api/auth_routes.py @@ -812,6 +812,32 @@ async def reset_user_mfa( ) -> SimpleMessage: """Admin MFA reset (lost authenticator + no recovery codes): clear the user's TOTP enrollment and revoke their sessions so they re-enroll. The acting admin is itself step-up + MFA gated.""" + # SELF-EXCLUSION, AND IT BELONGS HERE RATHER THAN IN THE SERVICE (BACKLOG #1022). + # + # `AuthService.admin_reset_mfa` is the ALWAYS-AVAILABLE RECOVERY for a locked-out passkey + # user and is deliberately unguarded (its own docstring, and ADR 0068 §2 via + # auth/webauthn.py). An actor check inside it would narrow the one path that exists when a + # user has lost every factor. This route is the only caller today, so the distinction is + # about WHICH CASE is refused, not about which function holds the line: cross-user recovery + # still reaches the service untouched, and only self-targeting is refused here. + # + # SELF-TARGETING IS NEVER RECOVERY, which is what makes the refusal safe. Reaching this + # route at all requires passing `require_step_up_action(... ADMIN_RESET_MFA ...)`, which is + # itself MFA-gated -- so a genuinely locked-out operator cannot call it in the first place. + # What the refusal removes is a THIRD ROUTE TO ZERO FACTORS: `disable_totp` and + # `delete_webauthn_credential` both refuse when they would drop the caller to zero factors + # while MFA is required (ADR 0068 decision 5), and using the ADMIN reset on yourself skipped + # that check entirely. `AuthService.disable_totp`'s 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. + # + # Copied in shape from reset_user_password above, which has carried the same guard since + # ASVS 6.4.6 -- the two admin routes now refuse the same case the same way. + if user_id == identity.user_id: + raise HTTPException( + status.HTTP_400_BAD_REQUEST, + "use the self-service MFA settings for your own account", + ) try: await service.admin_reset_mfa(user_id, actor=identity.username) except ValueError as exc: diff --git a/tests/test_api_auth.py b/tests/test_api_auth.py index a39be93cc..4c67231aa 100644 --- a/tests/test_api_auth.py +++ b/tests/test_api_auth.py @@ -1568,3 +1568,67 @@ async def test_admin_reset_mfa_requires_an_action_bound_proof_and_keeps_the_mfa_ "the MFA gate is gone from admin reset-MFA -- require_step_up_action was replaced with a " "reauth-only factory, which is the weakening #1148's research recommended" ) + + +async def test_admin_reset_mfa_refuses_to_target_the_caller(engine: Engine) -> None: + """BACKLOG #1022: the admin MFA reset was a THIRD, unasked route to zero factors. + + `disable_totp` 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 skipped that check entirely -- it clears TOTP, every recovery code and every passkey in + one call. `AuthService.disable_totp`'s own docstring names this item: two self-service routes to + zero factors behind one step-up gate, and only one of them asked. + + THE SIBLING ROUTE HAS CARRIED THIS GUARD SINCE ASVS 6.4.6. `reset_user_password` refuses + `user_id == identity.user_id` twenty-five lines above; reset-MFA, the sharper of the two, did not. + + WHY REFUSING SELF-TARGETING DOES NOT COST A RECOVERY PATH, which is the thing to get right: + `AuthService.admin_reset_mfa` stays unguarded and is still the always-available recovery for a + locked-out passkey user (ADR 0068 section 2). Cross-user reset is untouched -- the assertion + below pins that. And self-targeting was never recovery: reaching this 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. + """ + service = await _service( + engine, AuthSettings(require_mfa=False, login_rate_limit_enabled=False) + ) + # `_add` discards the id it creates, and this test is specifically ABOUT the caller's own id -- + # so root is created the long way, exactly as `_add` does internally, to keep it. + root_id = await service.create_local_user( + username="root", + password=PW, + display_name=None, + email=None, + roles=[Role.ADMINISTRATOR.value], + actor="test", + ) + await _clear_must_change(service, root_id) + target = await service.create_local_user( + username="mallory", + password=PW, + display_name=None, + email=None, + roles=["viewer"], + actor="root", + ) + async with _client(engine, service) as c: + tok = (await _login(c, "root")).json()["token"] + h = _auth(tok) + + # A grant bound to THIS action, so the request reaches the route body rather than the gate. + assert (await _reauth(c, tok, purpose="admin_reset_mfa")).status_code == 200 + mine = await c.post(f"/users/{root_id}/reset-mfa", headers=h) + assert mine.status_code == 400, ( + "the admin MFA reset accepted the caller's own id. That is a route to zero factors " + "which bypasses the last-factor refusal both self-service paths make." + ) + assert "own account" in mine.json()["detail"] + + # THE OTHER HALF, and without it this test would pass just as well if the route were broken + # outright: a DIFFERENT user is still resettable, so recovery is intact. + assert (await _reauth(c, tok, purpose="admin_reset_mfa")).status_code == 200 + other = await c.post(f"/users/{target}/reset-mfa", headers=h) + assert other.status_code == 200, ( + "cross-user admin MFA reset broke. That is the always-available recovery for a " + "locked-out passkey user (ADR 0068 section 2) and the self-exclusion must not touch it." + ) From 35f4e88d2a28a121f8a92b9a3ab18e553b13571d Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 26 Aug 2026 08:50:48 -0500 Subject: [PATCH 2/2] backlog: #1022 -- the ledger update this PR always owed, so the gate 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 --- docs/BACKLOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 83bef1c4d..e04a26002 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -4815,7 +4815,7 @@ Retiring the tree costs the engine nothing operationally: **`tests/test_ech_egre > **One bounded residual, stated so nobody reports it as worse than it is:** the guard is **check-then-act** (read then write, both `await`s), so it is not fail-closed under concurrency. Two racing requests could each see a surviving factor and both remove one. The consequence is **forced re-enrollment, not single-factor access**, which is why it is a residual rather than a blocker. > **RESIDUAL SCOPE, written 2026-08-21 (dispatcher), verified on the code at `origin/main` `75e20d43`. THE TITLED DEFECT IS BUILT -- what remains is one small code limb and three records the fix made FALSE.** > **BUILT, verified at the definition, at BOTH call sites, and in both polarities of test.** `disable_mfa` (`auth/service.py:2253`) reads `list_webauthn_credentials` then raises when `_mfa_required_for` holds and no factor would survive -- the exact guard shape this item specified, keyed on the Identity's roles with no extra store read. Call sites: `api/auth_routes.py:443-446` maps it to a 400 with a `#1022` comment, so the 500 this item predicted does not occur; `messagefoundry_webconsole/routes/account.py:282-294` renders it as a page and re-raises 429 to preserve Retry-After. Tests: `tests/test_mfa.py:369` asserts the refusal AND that the store was not mutated anyway; `:392` is the labelled POSITIVE CONTROL asserting the disable is ALLOWED when `require_mfa=False`; `:407` asserts the admin recovery path was NOT narrowed. Route level: `tests/test_api_auth.py:1444` and the web console suite. -> **BUILT 2026-08-25, self-exclusion limb.** `POST /users/{user_id}/reset-mfa` now refuses (400) when `user_id` is the caller's own; cross-user reset is untouched, pinned by its own test. **Stays OPEN regardless** -- per standing dispatcher guidance this row is not a clean closure. Of the three OWED records below, (1) and (2) are corrected on main; (3) is NOT -- the ADR gained retraction prose at the same spot but no new formal `AC-N` entry (checked directly: the file's ACs run AC-10 through AC-16, none added for this refusal). Whoever eventually closes this row still owes that one line. +> **BUILT (PR #590), self-exclusion limb.** `POST /users/{user_id}/reset-mfa` now refuses (400) when `user_id` is the caller's own; cross-user reset is untouched, pinned by its own test. **Stays OPEN regardless** -- per standing dispatcher guidance this row is not a clean closure. Of the three OWED records below, (1) and (2) are corrected **in this same PR** -- not, as an earlier draft of this note said, already on main; that draft was written before this PR's own ledger edit existed and landed without one, which is the mechanism recorded and corrected at length in `#1022`'s own entry on `main` as of 2026-08-26. (3) is NOT corrected here either -- the ADR gains retraction prose at the same spot but no new formal `AC-N` entry (checked directly: the file's ACs run AC-10 through AC-16, none added for this refusal). Whoever eventually closes this row still owes that one line. > **OWED -- code, and it is small.** `POST /users/{user_id}/reset-mfa` (`api/auth_routes.py:786-804`) still has **no self-exclusion**, while its sibling `reset_user_password` 25 lines above (`:761-784`, guard at `:770-773`) does. So an Administrator can still zero their own factors. **The fix is self-exclusion at the ROUTE, never a guard in the service method** -- `admin_reset_mfa` is deliberately unguarded as the always-available recovery for a locked-out passkey user, its docstring says so, and `tests/test_mfa.py:407` exists specifically to prove the new guard did not narrow it. > **OWED -- three records the landed fix made false.** (1) `docs/SECURITY.md` still asserts of `DELETE /me/mfa` that there is "no last-factor guard" and that it does not refuse a zero-factor state -- **a shipped security document making a false NEGATIVE claim about a control that now exists.** (2) `docs/adr/0068-*.md:140` still says TOTP-disable "keeps its existing behavior this lane (parity follow-up recorded)". (3) ADR 0068 should gain an acceptance criterion for the `disable_mfa` refusal, pointing at the named test. > **DO NOT go looking for the AC-10 / AC-11 contradiction this item alleges -- IT IS NOT THERE.** The body above says ADR 0068 "contradicts itself four lines apart" because AC-10 was amended unscoped. On `origin/main` AC-10 IS scoped -- it conditions on deleting a WebAuthn credential that would remove the user's last second factor -- so it does not collide with AC-11's mandate that `admin_reset_mfa` clears both factor types. **The real ADR defect is under-description, not contradiction.** A builder sent to resolve a contradiction that does not exist will either invent one or return empty-handed.