Concurrent MFA verify for the same user returns 500 (FK violation / deadlock) instead of a 4xx conflict

[egrish]

Summary

Two concurrent POST /factors/{id}/verify requests for the same user make one of them fail with
a 500 unexpected_failure, where the honest answer is a 4xx conflict. The cause is that a
successful TOTP verification unconditionally deletes every aal1 session of that user, in the
same transaction that another in-flight verification is still building its session in.

This is not a "don't do that" case: signing the same account in from two places at once is ordinary
(a browser tab and a CI job, two devices, a test harness and a human). The security behaviour is
correct and I am not asking for it to change — only for the failure to be reported as a conflict
rather than as an internal error, and ideally to be avoided where it is avoidable.

Mechanism

internal/api/mfa.go, TOTP verify transaction (current master):

token, terr = a.updateMFASessionAndClaims(r, tx, user, models.TOTPSignIn, ...)
if terr != nil {
    return terr
}
if terr = models.InvalidateSessionsWithAALLessThan(tx, user.ID, models.AAL2.String()); terr != nil {
    return apierrors.NewInternalServerError("Failed to update sessions. %s", terr)
}

and internal/models/sessions.go:

func InvalidateSessionsWithAALLessThan(tx *storage.Connection, userID uuid.UUID, level string) error {
	return tx.RawQuery("DELETE FROM "+... +" WHERE user_id = ? AND aal < ?", userID, level).Exec()
}

Two details make this observable:

1. The DELETE is user-scoped, not session-scoped. Request A's successful verify deletes request
   B's aal1 session, which B is at that moment mid-way through upgrading.

2. updateMFASessionAndClaims writes before it checks. In internal/api/token.go:

   if terr := models.AddClaimToSession(tx, sessionId, authenticationMethod); terr != nil {
       return terr
   }
   session, terr := models.FindSessionByID(tx, sessionId, true)   // existence check + FOR UPDATE

   The mfa_amr_claims INSERT — which carries an FK to sessions.id — happens before the
   existence check and before the row lock. If A's DELETE commits in that window, B's INSERT hits
   the foreign key.

Observed

GoTrue v2.189.0, Postgres 15. Three distinct outcomes, all on POST /factors/{id}/verify, all from
the same underlying race:

403  session_not_found      "Session from session_id claim in JWT does not exist"
500  unexpected_failure     "Failed to update sessions. ERROR: deadlock detected (SQLSTATE 40P01)"
500  unexpected_failure     insert or update on table "mfa_amr_claims" violates foreign key
                            constraint "mfa_amr_claims_session_id_fkey" (SQLSTATE 23503)

The 403 is arguably fine. The two 500s are the report: a client that lost a race against another
client of the same account gets an internal-error shape it cannot distinguish from a server fault,
and every retry policy treats it accordingly.

Deadlock detail: the DELETE takes row locks in arbitrary order (no ORDER BY, no SKIP LOCKED), so
two concurrent invalidations for the same user can deadlock against each other rather than serialise.

Reproduction

Reproduced deliberately, twice, ~2 minutes apart:

• one client repeatedly completes a full password-grant → challenge → verify cycle for account X;
• a second client does the same for account X, concurrently;
• within ~2 minutes at a few requests per second, one side produces 40P01 and/or
  session_not_found.

Control: pointing the second client at a different account, with everything else identical and
in the same minute, produced 5/5 clean runs against 29 successful step-ups of the first account —
so the trigger is specifically same-user concurrency, not load.

Two request IDs from an earlier, unprompted occurrence in CI, if they are useful:
436d595c-f316-4e7a-8c4f-88f128164190 (won the race) and
3f39394b-af5d-43ea-b858-632631cca7cd (the 500). Error ID from the reproduction:
296da493-678f-4f6e-8d1d-e0a66075a8f1.

Suggested directions

Roughly in increasing order of ambition — any one of them would resolve the reported problem:

1. Map the conflict to a 4xx. Translate 23503 on mfa_amr_claims_session_id_fkey and 40P01
   from this transaction into a conflict-shaped error (e.g. 409 with a dedicated error code)
   rather than NewInternalServerError. This is the minimal fix and the one I would want.
2. Check before writing. Move FindSessionByID(tx, sessionId, true) ahead of
   AddClaimToSession in updateMFASessionAndClaims, so the session is locked and proven to exist
   before a row referencing it is inserted. That converts the FK violation into the existing, clean
   session_not_found path.
3. Make the invalidation deadlock-resistant — a deterministic lock order (... WHERE user_id = ? AND aal < ? ORDER BY id FOR UPDATE before the delete) or an explicit advisory lock on the user
   for the duration of the verify.
4. Exclude the caller's own session from the invalidation where the flow already knows it, so
   the common "same user, two verifies" case stops being destructive at all. (This one changes
   behaviour, so it is the least safe suggestion of the four.)

I am happy to open a PR for (1) or (2) if that would be welcome — say which shape you would accept.

Not the issue

• Not a version regression: sessions.go, amr.go and the relevant part of mfa.go are unchanged
  from v2.189.0 through current master, so upgrading does not affect it. I re-read master before
  filing.
• Not a bad TOTP secret, not clock skew, and not a replayed code — the losing request's own code was
  accepted in the same window on retry.

[cekuu35]

The race is exactly as described, and the fix direction is clear. Two things that may help whoever picks this up:

1. The DELETE scope is the root cause, not the TOTP flow. InvalidateSessionsWithAALLessThan takes a userID and deletes every session below the target AAL. Any two concurrent verifications — same method or different — hit the same window. This means the fix should be in the invalidation function itself, not in the TOTP verify path. If the invalidation targeted the specific session being upgraded (or all sessions except the current one via session_id), the race disappears for every MFA method at once.

2. The INSERT-before-lock in AddClaimToSession is the FK violation trigger. The mfa_amr_claims INSERT fires before FindSessionByID takes FOR UPDATE. Even if the DELETE scope is narrowed, any future session-deletion race in the same transaction will hit the same FK. A SELECT ... FOR UPDATE SKIP LOCKED on the session row at the top of the verify transaction — before any writes — would serialize the two concurrent verifications cleanly: one proceeds, the other either waits or gets 409 Conflict if the session was already promoted to AAL2.

For the 500 → 4xx mapping: the deadlock case (SQLSTATE 40P01) and the FK violation (SQLSTATE 23503) should both map to the same 409 with a message like "mfa verification in progress for this account". The 403 session_not_found is already correct — that's the loser of the race and it should see it.