fix: map concurrent MFA verify races to 4xx instead of 500

internal/api/mfa.go

@@ -699,6 +699,9 @@ func (a *API) verifyTOTPFactor(w http.ResponseWriter, r *http.Request, params *V
 			return terr
 		}
 		if terr = models.InvalidateSessionsWithAALLessThan(tx, user.ID, models.AAL2.String()); terr != nil {
+			if mapped := mapMFASessionConflictError(terr); mapped != terr {
+				return mapped
+			}
 			return apierrors.NewInternalServerError("Failed to update sessions. %s", terr)
 		}
 		if terr = models.DeleteUnverifiedFactors(tx, user, factor.FactorType); terr != nil {
@@ -840,6 +843,9 @@ func (a *API) verifyPhoneFactor(w http.ResponseWriter, r *http.Request, params *
 			return terr
 		}
 		if terr = models.InvalidateSessionsWithAALLessThan(tx, user.ID, models.AAL2.String()); terr != nil {
+			if mapped := mapMFASessionConflictError(terr); mapped != terr {
+				return mapped
+			}
 			return apierrors.NewInternalServerError("Failed to update sessions. %s", terr)
 		}
 		if terr = models.DeleteUnverifiedFactors(tx, user, factor.FactorType); terr != nil {
@@ -960,6 +966,9 @@ func (a *API) verifyWebAuthnFactor(w http.ResponseWriter, r *http.Request, param
 			return terr
 		}
 		if terr = models.InvalidateSessionsWithAALLessThan(tx, user.ID, models.AAL2.String()); terr != nil {
+			if mapped := mapMFASessionConflictError(terr); mapped != terr {
+				return mapped
+			}
 			return apierrors.NewInternalServerError("Failed to update session").WithInternalError(terr)
 		}
 		if terr = models.DeleteUnverifiedFactors(tx, user, models.WebAuthn); terr != nil {

internal/api/mfa_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -1113,3 +1114,93 @@ func (ts *MFATestSuite) TestMFAFactorUnenrolledNotificationDisabled() {
 	// Assert that MFA factor unenrolled notification email was sent or not based on the config
 	require.Len(ts.T(), mockMailer.MFAFactorUnenrolledMailCalls, 0, "Expected 0 MFA factor unenrolled notification email(s) to be sent")
 }
+
+func (ts *MFATestSuite) TestUpdateMFASessionAndClaimsMissingSession() {
+	grant, err := models.GrantAuthenticatedUser(ts.API.db, ts.TestUser, models.GrantParams{})
+	require.NoError(ts.T(), err)
+	token := ts.generateAAL1Token(ts.TestUser, grant.SessionId)
+
+	require.NoError(ts.T(), models.LogoutSession(ts.API.db, *grant.SessionId))
+
+	req := httptest.NewRequest(http.MethodPost, "/factors/verify", nil)
+	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
+	ctx, err := ts.API.parseJWTClaims(token, req)
+	require.NoError(ts.T(), err)
+	req = req.WithContext(ctx)
+
+	factorID := ts.TestUser.Factors[0].ID
+	_, err = ts.API.updateMFASessionAndClaims(req, ts.API.db, ts.TestUser, models.TOTPSignIn, models.GrantParams{
+		FactorID: &factorID,
+	})
+	require.Error(ts.T(), err)
+
+	var httpErr *apierrors.HTTPError
+	require.ErrorAs(ts.T(), err, &httpErr)
+	require.Equal(ts.T(), http.StatusForbidden, httpErr.HTTPStatus)
+	require.Equal(ts.T(), apierrors.ErrorCodeSessionNotFound, httpErr.ErrorCode)
+	require.NotEqual(ts.T(), http.StatusInternalServerError, httpErr.HTTPStatus)
+}
+
+func (ts *MFATestSuite) TestConcurrentMFAVerifySameUserNoInternalError() {
+	friendlyName := uuid.Must(uuid.NewV4()).String()
+	factor := models.NewTOTPFactor(ts.TestUser, friendlyName)
+	sharedSecret := ts.TestOTPKey.Secret()
+	factor.Secret = sharedSecret
+	require.NoError(ts.T(), ts.API.db.Create(factor), "Error creating test factor")
+
+	type client struct {
+		token       string
+		challengeID uuid.UUID
+	}
+	clients := make([]client, 2)
+	for i := range clients {
+		grant, err := models.GrantAuthenticatedUser(ts.API.db, ts.TestUser, models.GrantParams{})
+		require.NoError(ts.T(), err)
+		token := ts.generateAAL1Token(ts.TestUser, grant.SessionId)
+
+		req := httptest.NewRequest(http.MethodPost, fmt.Sprintf("/factors/%s/verify", factor.ID), nil)
+		challenge := factor.CreateChallenge(utilities.GetIPAddress(req))
+		require.NoError(ts.T(), ts.API.db.Create(challenge), "Error creating challenge")
+		clients[i] = client{token: token, challengeID: challenge.ID}
+	}
+
+	code, err := totp.GenerateCode(sharedSecret, time.Now().UTC())
+	require.NoError(ts.T(), err)
+
+	type result struct {
+		code int
+		err  error
+	}
+	var wg sync.WaitGroup
+	results := make(chan result, len(clients))
+	for _, c := range clients {
+		wg.Add(1)
+		go func(c client) {
+			defer wg.Done()
+			var buffer bytes.Buffer
+			if err := json.NewEncoder(&buffer).Encode(map[string]interface{}{
+				"challenge_id": c.challengeID,
+				"code":         code,
+			}); err != nil {
+				results <- result{err: err}
+				return
+			}
+			w := httptest.NewRecorder()
+			req := httptest.NewRequest(http.MethodPost, fmt.Sprintf("/factors/%s/verify", factor.ID), &buffer)
+			req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", c.token))
+			req.Header.Set("Content-Type", "application/json")
+			ts.API.handler.ServeHTTP(w, req)
+			results <- result{code: w.Code}
+		}(c)
+	}
+	wg.Wait()
+	close(results)
+
+	statuses := make([]int, 0, len(clients))
+	for res := range results {
+		require.NoError(ts.T(), res.err)
+		statuses = append(statuses, res.code)
+		require.NotEqual(ts.T(), http.StatusInternalServerError, res.code, "concurrent MFA verify must not return 500")
+	}
+	require.Contains(ts.T(), statuses, http.StatusOK, "at least one concurrent verify should succeed")
+}

internal/api/recovery_codes.go

@@ -428,6 +428,9 @@ func (a *API) RecoveryCodesVerify(w http.ResponseWriter, r *http.Request) error
 		}
 
 		if terr := models.InvalidateSessionsWithAALLessThan(tx, user.ID, models.AAL2.String()); terr != nil {
+			if mapped := mapMFASessionConflictError(terr); mapped != terr {
+				return mapped
+			}
 			return apierrors.NewInternalServerError("Failed to update sessions. %s", terr)
 		}
 

internal/api/token.go

@@ -2,9 +2,12 @@ package api
 
 import (
 	"context"
+	"errors"
 	"net/http"
 
 	"github.com/gofrs/uuid"
+	"github.com/jackc/pgconn"
+	"github.com/jackc/pgerrcode"
 
 	"github.com/supabase/auth/internal/api/apierrors"
 	"github.com/supabase/auth/internal/crypto"
@@ -314,13 +317,22 @@ func (a *API) updateMFASessionAndClaims(r *http.Request, tx *storage.Connection,
 	}
 
 	err = tx.Transaction(func(tx *storage.Connection) error {
+		// Lock the session before inserting AMR claims so a concurrent MFA
+		// verify that invalidates aal1 sessions cannot delete this session
+		// between the claim insert and the existence check (FK 23503 / 500).
+		session, terr := models.FindSessionByID(tx, sessionId, true)
+		if terr != nil {
+			return mapMFASessionConflictError(terr)
+		}
+
 		if terr := models.AddClaimToSession(tx, sessionId, authenticationMethod); terr != nil {
-			return terr
+			return mapMFASessionConflictError(terr)
 		}
 
-		session, terr := models.FindSessionByID(tx, sessionId, true)
+		// Reload so CalculateAALAndAMR sees the claim inserted above.
+		session, terr = models.FindSessionByID(tx, sessionId, false)
 		if terr != nil {
-			return terr
+			return mapMFASessionConflictError(terr)
 		}
 
 		if err := tx.Load(user, "Identities"); err != nil {
@@ -404,3 +416,21 @@ func (a *API) updateMFASessionAndClaims(r *http.Request, tx *storage.Connection,
 		User:         user,
 	}, nil
 }
+
+// mapMFASessionConflictError turns concurrent MFA verify races into client
+// errors instead of 500s. A peer verify may delete this aal1 session
+// (InvalidateSessionsWithAALLessThan) while claims are being written, which
+// surfaces as session-not-found, FK violation (23503), or deadlock (40P01).
+func mapMFASessionConflictError(err error) error {
+	if err == nil {
+		return nil
+	}
+	if models.IsNotFoundError(err) {
+		return apierrors.NewForbiddenError(apierrors.ErrorCodeSessionNotFound, "Session from session_id claim in JWT does not exist").WithInternalError(err)
+	}
+	var pgErr *pgconn.PgError
+	if errors.As(err, &pgErr) && (pgErr.Code == pgerrcode.ForeignKeyViolation || pgErr.Code == pgerrcode.DeadlockDetected) {
+		return apierrors.NewConflictError("Session conflict during MFA verification").WithInternalError(err)
+	}
+	return err
+}

internal/api/token_test.go

@@ -5,6 +5,7 @@ import (
 	"crypto/sha256"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -14,6 +15,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/jackc/pgconn"
+	"github.com/jackc/pgerrcode"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
@@ -957,3 +960,42 @@ func TestRefreshTokenGrantParamsValidate(t *testing.T) {
 	p.RefreshToken = (&crypto.RefreshToken{}).Encode(make([]byte, 32))
 	require.NoError(t, p.Validate())
 }
+
+func TestMapMFASessionConflictError(t *testing.T) {
+	t.Parallel()
+
+	t.Run("nil", func(t *testing.T) {
+		require.NoError(t, mapMFASessionConflictError(nil))
+	})
+
+	t.Run("session not found", func(t *testing.T) {
+		err := mapMFASessionConflictError(models.SessionNotFoundError{})
+		var httpErr *apierrors.HTTPError
+		require.ErrorAs(t, err, &httpErr)
+		require.Equal(t, http.StatusForbidden, httpErr.HTTPStatus)
+		require.Equal(t, apierrors.ErrorCodeSessionNotFound, httpErr.ErrorCode)
+		require.NotEqual(t, http.StatusInternalServerError, httpErr.HTTPStatus)
+	})
+
+	t.Run("foreign key violation", func(t *testing.T) {
+		err := mapMFASessionConflictError(&pgconn.PgError{Code: pgerrcode.ForeignKeyViolation, Message: "fk"})
+		var httpErr *apierrors.HTTPError
+		require.ErrorAs(t, err, &httpErr)
+		require.Equal(t, http.StatusConflict, httpErr.HTTPStatus)
+		require.Equal(t, apierrors.ErrorCodeConflict, httpErr.ErrorCode)
+		require.Contains(t, httpErr.Message, "Session conflict during MFA verification")
+	})
+
+	t.Run("deadlock", func(t *testing.T) {
+		err := mapMFASessionConflictError(&pgconn.PgError{Code: pgerrcode.DeadlockDetected, Message: "deadlock"})
+		var httpErr *apierrors.HTTPError
+		require.ErrorAs(t, err, &httpErr)
+		require.Equal(t, http.StatusConflict, httpErr.HTTPStatus)
+		require.Equal(t, apierrors.ErrorCodeConflict, httpErr.ErrorCode)
+	})
+
+	t.Run("unrelated error unchanged", func(t *testing.T) {
+		original := errors.New("boom")
+		require.Equal(t, original, mapMFASessionConflictError(original))
+	})
+}