diff --git a/internal/api/mfa.go b/internal/api/mfa.go index 2d9f2fa7b2..accf04049c 100644 --- a/internal/api/mfa.go +++ b/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 { diff --git a/internal/api/mfa_test.go b/internal/api/mfa_test.go index 48746b7736..ed5446fa78 100644 --- a/internal/api/mfa_test.go +++ b/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") +} diff --git a/internal/api/recovery_codes.go b/internal/api/recovery_codes.go index bd39ee9d93..7282566b2e 100644 --- a/internal/api/recovery_codes.go +++ b/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) } diff --git a/internal/api/token.go b/internal/api/token.go index 86dcc27de8..00fae3b12a 100644 --- a/internal/api/token.go +++ b/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 +} diff --git a/internal/api/token_test.go b/internal/api/token_test.go index 5b34a0892c..2bd4712d6d 100644 --- a/internal/api/token_test.go +++ b/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)) + }) +}