feat: MFA challenge + trusted-device grants (#1005 slice 3) - #1009
Merged
Conversation
milkway
marked this pull request as ready for review
July 18, 2026 03:50
milkway
force-pushed
the
feat/1005-mfa-enroll
branch
from
July 18, 2026 03:50
555f3d3 to
07d3fb0
Compare
The proof machinery the slice-4 proxy guard will consume. No proxy
changes yet.
- Migration 0030 (dual-dialect): user_mfa_grants — opaque 32-byte
token stored only as salt:SHA-256, bound to user + mfa_verified_at +
expiry (30-day cap) + the factor it proved (confirmed_at binding: a
reset/re-enrollment invalidates old grants) + a session-id hash for
the 0-day semantics.
- Challenge at /admin/account/mfa/challenge: TOTP or recovery code →
device grant + cookie (__ruscker_mfa_device={id}.{token}, HttpOnly,
SameSite=Strict, base-path root so slice 4 sees it on /app/*).
Audit mfa.verify / mfa.recovery_used. Atomic try_reserve limiter.
- Decision API mfa::evaluate → Satisfied | ChallengeRequired |
EnrollmentRequired: policy → enrollment → cookie → grant → constant-
time token compare → expiry → factor binding → 0-day session binding
or proof-age vs the app's window. One proof serves all apps, each
comparing the age against its own effective_mfa_validity_days().
- TOTP replay prevention: the accepted step (current/±1, derived
explicitly) is recorded via a conditional last_used_step update —
covers challenge AND enrollment confirm.
- Revocations: password set/change and MFA reset delete grants inside
their existing transactions; user deletion cascades; explicit
"forget this device" / "sign out all devices" buttons audit
mfa.trusted_device.revoke. Normal logout keeps the device.
Implemented by a Codex (gpt-5.6, high) agent. Verified: full gate
green; postgres-it green vs real postgres:16 (grants round-trip); live
smoke — challenge with a python-computed step+1 code (the enrollment
consumed the current step; replay guard forces strictly-greater),
grant row hash-only, same-code replay 401, password change revoked the
grant, audit trail correct.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#1005 slice 3 review) Two codex-review findings: - P1: the trusted-device grant cookie is root-scoped (slice 4 needs it on /app/*), so browsers send it on every proxied request — and the #258 cookie filter didn't know it: the MFA BEARER would have reached app containers, letting a compromised app capture it and bypass MFA once it also had the password. is_ruscker_cookie now strips the whole __ruscker_mfa_ prefix (grant + ceremony) on both the HTTP path and the WS handshake. Proxy test: app cookies pass, MFA cookies never reach the upstream. - P2: "Sign out of all devices" only revokes trusted-device MFA grants, not login sessions — relabeled ×4 to "Forget all devices" with a confirm text that says exactly that (sessions stay active). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… 3 review r2) Codex review: a revocation (password set/change, forget-all) that committed while a challenge was in flight deleted the grants — and the challenge's independent INSERT then created a fresh grant AFTER, surviving the revocation. user_mfa now carries a security_epoch bumped inside every revocation transaction; grant issuance is an INSERT…SELECT conditional on the epoch read BEFORE the TOTP verification, so the revocation always wins (the in-flight insert hits 0 rows and the route asks the user to challenge again). MFA reset deletes the factor row, which refuses stale issuance the same way. Contract test: stale-epoch create is refused after a racing password change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two codex-review findings: - P1 (pg MVCC): under READ COMMITTED the conditional INSERT…SELECT can still read the pre-revocation epoch while the revoker's transaction is uncommitted, and the revoker's DELETE snapshot then misses the new grant. Each grant now RECORDS the epoch it was issued under and evaluate() validates it against the live factor row on every read — issuance-time races become harmless because an old-epoch grant is simply never accepted. Test simulates the slipped-through case. - P1 (migration immutability): the epoch column was added by editing 0029 — but 0029 belongs to slice 2 (PR #1008) and may be applied before this slice lands; editing it would trip sqlx's applied- migration checksum. 0029 restored byte-for-byte (from the slice-2 tip — first restore came from the wrong commit and dropped the ceremony column; caught by the unit suite); security_epoch moved to 0030 as ALTER TABLE. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on (#1005 slice 3 review r4) Two codex-review findings, one mechanism (db::mfa_grants::issue, one transaction per dialect: consume recovery → retire previous grant → epoch-conditional insert → audit): - P2: the recovery code was consumed BEFORE grant creation — a stale- epoch refusal (or any later failure) burned a finite code with nothing issued. The spend now lives inside the issuance transaction and rolls back with it; the route matches the code first (find_recovery_candidate, no consume) and passes the id in. Test: stale-epoch refusal leaves the code usable. - P2: every successful re-challenge left the browser's PREVIOUS grant alive while overwriting its cookie — a copied old cookie stayed a valid bearer until hard expiry. The issuance transaction now retires the cookie's old grant (scoped to the same username). Test: second challenge rotates the cookie and the grant count stays at one. The separate post-issuance audit call is gone — the audit row commits with the grant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ice 3 review r5) - P1: evaluate() read the factor BEFORE the grant — a stale-epoch grant committing between the two reads could match the cached (pre- revocation) epoch and return Satisfied. The authoritative factor read now happens AFTER the grant fetch, so its epoch is at least as fresh as the grant's; the no-cookie path fetches the factor directly (one SELECT either way — this is slice 4's hot path). - P2: record_used_step committed before issue() — a StaleEpoch refusal burned the current 30s code, locking the user out until the next step. The step consumption is now a conditional UPDATE inside the issuance transaction (new Replayed refusal), rolling back with everything else. Enrollment-confirm keeps its standalone step guard (no issuance to bundle with). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e 3 review r6) Codex review: the blanket master-key check at the top of the challenge handler also rejected kind=recovery with 503 — but recovery codes only compare salted hashes; they never decrypt the TOTP secret. A node restarted without RUSCKER_MASTER_KEY would lock out users holding valid recovery codes: exactly the emergency path that must survive misconfiguration. The check now lives inside the TOTP branch alone (which does decrypt). Test: keyless state — TOTP 503s with the hint, a valid recovery code still issues a grant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…review r7) Two codex-review concurrency findings on Postgres: - P2 deadlock: a re-challenge locks user_mfa (the TOTP-step UPDATE) then the grant row (rotation DELETE), while every revocation locked grants then user_mfa (epoch bump) — opposite order, so an overlap could deadlock and abort the SECURITY action with a 500. All revocation paths (revoke_all + the silent delete_all helpers used by password change / MFA reset) now bump the epoch BEFORE deleting grants, matching issue()'s user_mfa-then-grants order. - P2 double grant: two challenges from the same browser carry the same previous grant id; the first retired it, the second's DELETE hit 0 rows but still inserted — leaving TWO live grants for one cookie (overwritten bearer valid 30 days). The rotation DELETE is now required to affect exactly one row; a 0-row retire returns the new Superseded refusal (route → 409 "verified in another tab"). Test: concurrent rotation of the same grant leaves exactly one live grant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The r7 exclusive-rotation fix introduced a P1 lockout: after a revocation from another browser, THIS browser keeps its device cookie but its grant is gone, so the next valid challenge passed the stale id to issue(), got Superseded, and — since the cookie was never cleared — every retry looped, locking the user out. Root cause: keying rotation off the cookie's grant id. Redesign, keyed off the session instead: - Migration 0030 gains UNIQUE(username, session_binding). - issue() UPSERTs the single row for the browser-session (ON CONFLICT DO UPDATE id/token/factor/epoch/verified/expires). A re-challenge replaces its row (old cookie dies), a stale cookie after revocation simply issues fresh (no lockout), and two concurrent challenges from one browser converge to ONE row (no orphan). replace_grant_id and the Superseded refusal are gone. - Lock order is now uniformly user_mfa -> grants -> recovery across issue()/revoke_all/delete_all/reset, closing the r8 P2 deadlock between a recovery challenge and an MFA reset on Postgres (recovery consumption moved AFTER the grant UPSERT). - Tests: the concurrent-rotation test becomes "one grant per session + stale-cookie-after-revocation recovers (no lockout)"; eval/revoke tests use distinct bindings to model separate browsers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… from slice-4 review) The proxy guard (slice 4) redirects an unenrolled user to enroll; but enrollment confirmation only recorded the TOTP step — it minted no grant. The recovery page's "Continue to app" link then hit the guard, got ChallengeRequired, and the challenge rejected the just-entered code as a replay, forcing the user to wait a TOTP interval (or spend a recovery code). Now confirm() issues the browser's initial device grant (the code IS a valid proof) and sets the device cookie, so enrollment flows straight into the app. Belongs in slice 3 (issue() lives here); the fix is what slice 4's guard makes necessary. - Audited mfa.verify (the device is verified, beside the mfa.enroll already written). Non-fatal on failure — recovery codes still render. - The step was already consumed by enrollment, so issuance passes no step/recovery to consume. - Tests: enrollment asserts the initial grant + cookie; the enroll-then-manual-grant tests use distinct bindings / updated counts to account for the initial grant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
slice 3 review r9) Two codex-review findings: - P2 (pg recovery-path epoch race): the recovery-code path has no TOTP-step UPDATE, so under READ COMMITTED the epoch-conditional INSERT read a snapshot a racing revocation could pass — inserting an old-epoch (thus unusable) grant while committing the one-time recovery-code spend. issue() now reads+validates the factor row FIRST on EVERY path (Postgres: SELECT … FOR UPDATE; SQLite: a serialised read), turning a raced revocation into a clean StaleEpoch rollback that leaves the recovery code unspent. - P2 (grant retention): session-only MFA mints a row per login and expired rows were only filtered at read time. issue() now sweeps the user's expired grants (DELETE … WHERE expires_at < now) so the table can't grow unboundedly. Test covers the sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… slice 3 review r10) Two database-specific concurrency edges, both turning a rare same-user race into a transient 500/deadlock (never a security or integrity bug): - P2 SQLite WAL: pool.begin() is deferred, so reading the epoch took a read snapshot; a revocation committing before our later write would raise SQLITE_BUSY_SNAPSHOT (a 500) instead of a clean recheck. The epoch check is now a no-op conditional UPDATE that takes the WRITE lock up front — rows_affected==1 means the epoch held under the lock, 0 means a raced revocation → StaleEpoch. - P2 Postgres deadlock vs user deletion: issuance locked user_mfa first, but the grant INSERT's FK check needs a key-share lock on the parent users row, while user deletion locks users then cascades. The pg path now locks the parent `users` row (FOR KEY SHARE) BEFORE user_mfa, so both follow parent→child order. This completes the lock-ordering hardening (user→user_mfa→grants→ recovery, parent before child) across issue()/revoke/reset. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
milkway
force-pushed
the
feat/1005-mfa-challenge
branch
from
July 18, 2026 03:51
c1def60 to
1b0cfca
Compare
14 tasks
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.
Slice 3 of 4 of the step-up MFA epic (#1005). Stacked on #1008 (slice 2) — merge order 1→2→3→4. The proof machinery the slice-4 proxy guard consumes; no proxy changes here.
What changed
user_mfa_grants— opaque 32-byte token stored only as salt:SHA-256, bound to user +mfa_verified_at+ expiry (30-day cap) + the factor it proved (reset/re-enroll invalidates old grants) + a session-id hash (themfa-validity-days: 0semantics) + the security epoch it was issued under.ALTER TABLE user_mfa ADD security_epochlives here (0029 belongs to slice 2 and stays immutable)./admin/account/mfa/challenge: TOTP or recovery code → device grant + root-scoped cookie (__ruscker_mfa_device={id}.{token}) so slice 4 sees it on/app/*. One proof serves all apps — each compares the proof age against its own window; window 0 binds to the login session. Auditmfa.verify/mfa.recovery_used; atomictry_reservelimiter.mfa::evaluate → Satisfied | ChallengeRequired | EnrollmentRequired— exactly what the slice-4 guard calls.last_used_stepupdate.mfa.trusted_device.revoke; normal logout keeps the device.Review hardening — 7 rounds, 8 real findings fixed
is_ruscker_cookienow strips the whole__ruscker_mfa_prefix (HTTP + WS), with a proxy test.evaluatevalidates it against the live factor row on every read.issue(): consume recovery → retire the previous grant (cookie rotation — a copied old cookie must die) → consume TOTP step → epoch-conditional insert → audit, all-or-nothing.evaluateread factor-then-grant, leaving a stale-match window → grant first, authoritative factor read after (one SELECT per path — this is slice 4's hot path).RUSCKER_MASTER_KEYwould lock out the emergency path. Check moved into the TOTP branch; test proves recovery still issues a grant keyless.Validation
Full gate green across rounds; postgres-it green vs real
postgres:16(grants round-trip); 10 integration tests (age-window/session-binding/replay, recovery once+audit, revocations incl. racing ones, epoch-at-read rejection, cookie rotation, rate limit, keyless recovery); live smoke — challenge with a python-computed step+1 code, same-code replay 401, password change revoked the grant, audit trail correct.🤖 Generated with Claude Code