fix(auth): accept each TOTP code once

internal/auth/interfaces.go

@@ -23,6 +23,9 @@ type StoreInterface interface {
 	ReplaceMFARecoveryCodes(ctx context.Context, userID, readSecret string, readCodes, newHashes []string) error
 	DisableMFA(ctx context.Context, userID, readSecret string, readCodes []string) error
 	ClearPendingMFASecret(ctx context.Context, userID string) error
+	// ClaimTOTPCounter records counter as the user's last accepted TOTP step and
+	// reports whether it was above the previous one; false means a replay.
+	ClaimTOTPCounter(ctx context.Context, userID string, counter int64) (bool, error)
 	RecordFailedLogin(ctx context.Context, userID string) error
 	RecordSuccessfulLogin(ctx context.Context, userID string) error
 	DeleteUser(ctx context.Context, userID string) error

internal/auth/service.go

@@ -246,7 +246,12 @@ func (s *Service) verifyPasswordAndMFA(ctx context.Context, user *User, req Logi
 		}
 		// verifyTOTP fails closed on empty or malformed inputs: empty code, empty
 		// secret, and base32-decode errors all return false rather than a match.
-		if verifyTOTP(user.MFASecret, req.MFACode) {
+		claimed, err := s.verifyAndClaimTOTP(ctx, user.ID, user.MFASecret, req.MFACode)
+		if err != nil {
+			logging.Warnf("Failed to record TOTP use for user %s: %v", user.ID, err)
+			return ErrInvalidMFACode
+		}
+		if claimed {
 			return nil
 		}
 		// TOTP miss — try a recovery code. consumeRecoveryCode mutates

internal/auth/service_credentials_db_test.go

@@ -142,7 +142,7 @@ func (f *credentialRaceFixture) requireFactorEnforced(email, password string, en
 	_, err := f.svc.Login(f.t.Context(), LoginRequest{Email: email, Password: password})
 	require.ErrorIs(f.t, err, ErrMFARequired)
 	_, err = f.svc.Login(f.t.Context(), LoginRequest{Email: email, Password: password,
-		MFACode: generateTOTP(enrolled.MFASecret, time.Now().Unix()/30)})
+		MFACode: generateTOTP(enrolled.MFASecret, time.Now().Unix()/30+1)})
 	require.NoError(f.t, err)
 }
 

internal/auth/service_mfa.go

@@ -57,14 +57,19 @@ const recoveryCodeAlphabet = "ABCDEFGHJKMNPQRSTUVWXYZ23456789"
 // the correct behavior is to reject, not to accidentally match because
 // generateTOTP("","") returns "" and ConstantTimeCompare("","") == 1.
 func verifyTOTP(secret, code string) bool {
+	_, ok := matchTOTP(secret, code)
+	return ok
+}
+
+// matchTOTP is verifyTOTP that also returns the matched time-step counter, so
+// a caller can record it and refuse the same code afterwards (RFC 6238 5.2).
+// When two steps in the window produce the same code the later counter wins.
+func matchTOTP(secret, code string) (int64, bool) {
 	// Fail closed: reject empty code and empty secret up front.
 	// generateTOTP returns "" on base32-decode failure; ConstantTimeCompare("","")
 	// evaluates to 1 and would bypass MFA for any caller that passes code="".
-	if code == "" {
-		return false
-	}
-	if secret == "" {
-		return false
+	if code == "" || secret == "" {
+		return 0, false
 	}
 
 	// Allow for time skew by checking current and adjacent time windows.
@@ -73,17 +78,30 @@ func verifyTOTP(secret, code string) bool {
 
 	// Check current time step and one step before/after for clock skew tolerance.
 	// Use constant-time comparison and avoid early returns to prevent timing attacks.
-	valid := 0
+	var matched int64
+	found := 0
 	for _, offset := range []int64{-1, 0, 1} {
 		counter := (currentTime / timeStep) + offset
 		expected := generateTOTP(secret, counter)
 		// generateTOTP returns "" on base32-decode failure; a non-empty code can
 		// never equal "" so a bad secret produces no match.
 		if subtle.ConstantTimeCompare([]byte(expected), []byte(code)) == 1 {
-			valid = 1
+			matched = counter
+			found = 1
 		}
 	}
-	return valid == 1
+	return matched, found == 1
+}
+
+// verifyAndClaimTOTP accepts a TOTP code at most once per user: the matched
+// counter must be above the last one the store accepted. A replayed or
+// concurrently raced code returns false with no error.
+func (s *Service) verifyAndClaimTOTP(ctx context.Context, userID, secret, code string) (bool, error) {
+	counter, ok := matchTOTP(secret, code)
+	if !ok {
+		return false, nil
+	}
+	return s.store.ClaimTOTPCounter(ctx, userID, counter)
 }
 
 // generateTOTP generates a TOTP code for the given counter.
@@ -397,6 +415,19 @@ func (s *Service) disableMFAAlreadyOff(ctx context.Context, user *User) error {
 	return s.store.ClearPendingMFASecret(ctx, user.ID)
 }
 
+// verifyMFAProof accepts an unspent TOTP code, or else consumes a recovery code
+// in memory; the caller persists the consumed slice through its own CAS write.
+func (s *Service) verifyMFAProof(ctx context.Context, user *User, codeOrRecovery string) error {
+	matched, err := s.verifyAndClaimTOTP(ctx, user.ID, user.MFASecret, codeOrRecovery)
+	if err != nil {
+		return fmt.Errorf("failed to record TOTP use: %w", err)
+	}
+	if !matched && !s.consumeRecoveryCode(user, codeOrRecovery) {
+		return fmt.Errorf("%w", ErrMFAInvalidCode)
+	}
+	return nil
+}
+
 // MFADisable turns off MFA for a user. Requires both the current
 // password AND a fresh proof-of-possession (either a TOTP code or
 // an unused recovery code). Defense-in-depth: a stolen session
@@ -428,9 +459,8 @@ func (s *Service) MFADisable(ctx context.Context, userID, password, codeOrRecove
 	// compare, ~constant-time per slot). Either path counts as a
 	// fresh proof-of-possession.
 	readSecret, readCodes := user.MFASecret, slices.Clone(user.MFARecoveryCodes)
-	matched := verifyTOTP(user.MFASecret, codeOrRecovery) || s.consumeRecoveryCode(user, codeOrRecovery)
-	if !matched {
-		return fmt.Errorf("%w", ErrMFAInvalidCode)
+	if err := s.verifyMFAProof(ctx, user, codeOrRecovery); err != nil {
+		return err
 	}
 
 	if err := s.store.DisableMFA(ctx, user.ID, readSecret, readCodes); err != nil {
@@ -455,7 +485,11 @@ func (s *Service) MFARegenerateRecoveryCodes(ctx context.Context, userID, code s
 	if !user.MFAEnabled || user.MFASecret == "" {
 		return nil, fmt.Errorf("%w", ErrMFANotEnabled)
 	}
-	if !verifyTOTP(user.MFASecret, code) {
+	valid, err := s.verifyAndClaimTOTP(ctx, user.ID, user.MFASecret, code)
+	if err != nil {
+		return nil, fmt.Errorf("failed to record TOTP use: %w", err)
+	}
+	if !valid {
 		return nil, fmt.Errorf("%w", ErrMFAInvalidCode)
 	}
 

internal/auth/service_mfa_cas_db_test.go

@@ -60,7 +60,8 @@ func TestIntegration_MFADisableAndRegenerateAreColumnScoped(t *testing.T) {
 		fresh, err := f.svc.MFARegenerateRecoveryCodes(ctx, f.user.ID, totpNow(secret))
 		require.ErrorIs(t, err, ErrUserChanged)
 		assert.Nil(t, fresh)
-		assert.Equal(t, winner(), f.stored(), "the stale write must not replace the codes the login left")
+		assertOnlyChanged(t, winner(), f.stored(), func(*User) {})
+		assert.Equal(t, winner().MFARecoveryCodes, f.stored().MFARecoveryCodes, "the stale write must not replace the codes the login left")
 		assert.Len(t, f.stored().MFARecoveryCodes, len(codes)-1)
 	})
 
@@ -71,7 +72,7 @@ func TestIntegration_MFADisableAndRegenerateAreColumnScoped(t *testing.T) {
 		winner := f.onRead(spendRecoveryCode(f, codes[0]))
 		err := f.svc.MFADisable(ctx, f.user.ID, credentialRacePassword, totpNow(secret))
 		require.ErrorIs(t, err, ErrUserChanged)
-		assert.Equal(t, winner(), f.stored())
+		assertOnlyChanged(t, winner(), f.stored(), func(*User) {})
 		assert.True(t, f.stored().MFAEnabled)
 	})
 

internal/auth/service_mfa_test.go

@@ -320,6 +320,7 @@ func TestMFADisable_WithTOTP(t *testing.T) {
 	user.MFARecoveryCodes = []string{"$2a$04$hashedstub"} // doesn't matter, won't be tested
 
 	mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil)
+	mockStore.On("ClaimTOTPCounter", ctx, user.ID, mock.AnythingOfType("int64")).Return(true, nil).Once()
 	mockStore.On("DisableMFA", ctx, user.ID, secret, []string{"$2a$04$hashedstub"}).Return(nil).Once()
 
 	err := service.MFADisable(ctx, user.ID, "SecurePass@123", totpFor(secret))
@@ -429,6 +430,7 @@ func TestMFARegenerateRecoveryCodes_HappyPath(t *testing.T) {
 	user.MFARecoveryCodes = []string{"$2a$04$preexistingstub"}
 
 	mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil)
+	mockStore.On("ClaimTOTPCounter", ctx, user.ID, mock.AnythingOfType("int64")).Return(true, nil).Once()
 	var captured []string
 	mockStore.On("ReplaceMFARecoveryCodes", ctx, user.ID, secret, []string{"$2a$04$preexistingstub"}, mock.Anything).Run(func(args mock.Arguments) {
 		captured = append([]string{}, args.Get(4).([]string)...)
@@ -685,3 +687,83 @@ func TestMFARegenerateRecoveryCodes_WrongCode_ReturnsSentinel(t *testing.T) {
 		"MFARegenerateRecoveryCodes wrong TOTP must return ErrMFAInvalidCode, got: %v", err)
 	mockStore.AssertExpectations(t)
 }
+
+func TestMatchTOTP_ReturnsMatchedCounter(t *testing.T) {
+	secret := "JBSWY3DPEHPK3PXP"
+	current := time.Now().Unix() / 30
+	for _, offset := range []int64{-1, 0, 1} {
+		counter, ok := matchTOTP(secret, generateTOTP(secret, current+offset))
+		assert.True(t, ok)
+		assert.Equal(t, current+offset, counter)
+	}
+	_, ok := matchTOTP(secret, generateTOTP(secret, current+5))
+	assert.False(t, ok)
+}
+
+func TestMFADisable_ReplayedTOTPIsRejected(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	service := createTestService(mockStore, new(MockEmailSender))
+
+	secret := "JBSWY3DPEHPK3PXP"
+	user := createTestUser(t, "SecurePass@123")
+	user.MFAEnabled = true
+	user.MFASecret = secret
+	mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil)
+	mockStore.On("ClaimTOTPCounter", ctx, user.ID, mock.AnythingOfType("int64")).Return(false, nil).Once()
+
+	err := service.MFADisable(ctx, user.ID, "SecurePass@123", totpFor(secret))
+	require.ErrorIs(t, err, ErrMFAInvalidCode)
+	mockStore.AssertNotCalled(t, "DisableMFA", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+}
+
+func TestMFARegenerateRecoveryCodes_ReplayedTOTPIsRejected(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	service := createTestService(mockStore, new(MockEmailSender))
+
+	secret := "JBSWY3DPEHPK3PXP"
+	user := createTestUser(t, "SecurePass@123")
+	user.MFAEnabled = true
+	user.MFASecret = secret
+	mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil)
+	mockStore.On("ClaimTOTPCounter", ctx, user.ID, mock.AnythingOfType("int64")).Return(false, nil).Once()
+
+	_, err := service.MFARegenerateRecoveryCodes(ctx, user.ID, totpFor(secret))
+	require.ErrorIs(t, err, ErrMFAInvalidCode)
+	mockStore.AssertNotCalled(t, "ReplaceMFARecoveryCodes", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+}
+
+func TestMFARegenerateRecoveryCodes_ClaimFailureFailsClosed(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	service := createTestService(mockStore, new(MockEmailSender))
+
+	secret := "JBSWY3DPEHPK3PXP"
+	user := createTestUser(t, "SecurePass@123")
+	user.MFAEnabled = true
+	user.MFASecret = secret
+	mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil)
+	mockStore.On("ClaimTOTPCounter", ctx, user.ID, mock.AnythingOfType("int64")).Return(false, errors.New("db down")).Once()
+
+	_, err := service.MFARegenerateRecoveryCodes(ctx, user.ID, totpFor(secret))
+	require.ErrorContains(t, err, "db down")
+	mockStore.AssertNotCalled(t, "ReplaceMFARecoveryCodes", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+}
+
+func TestMFADisable_ClaimFailureFailsClosed(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	service := createTestService(mockStore, new(MockEmailSender))
+
+	secret := "JBSWY3DPEHPK3PXP"
+	user := createTestUser(t, "SecurePass@123")
+	user.MFAEnabled = true
+	user.MFASecret = secret
+	mockStore.On("GetUserByID", ctx, user.ID).Return(user, nil)
+	mockStore.On("ClaimTOTPCounter", ctx, user.ID, mock.AnythingOfType("int64")).Return(false, errors.New("db down")).Once()
+
+	err := service.MFADisable(ctx, user.ID, "SecurePass@123", totpFor(secret))
+	require.ErrorContains(t, err, "db down")
+	mockStore.AssertNotCalled(t, "DisableMFA", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
+}

internal/auth/service_test.go

@@ -2,6 +2,7 @@ package auth
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"testing"
 	"time"
@@ -421,6 +422,7 @@ func TestLogin_WithMFA(t *testing.T) {
 	}
 
 	mockStore.On("GetUserByEmail", ctx, "mfa@example.com").Return(user, nil)
+	mockStore.On("ClaimTOTPCounter", ctx, user.ID, counter).Return(true, nil).Once()
 	mockStore.On("CreateSession", ctx, mock.AnythingOfType("*auth.Session")).Return(nil)
 	mockStore.On("RecordSuccessfulLogin", ctx, mock.AnythingOfType("string")).Return(nil)
 
@@ -436,6 +438,28 @@ func TestLogin_WithMFA(t *testing.T) {
 	assert.NotEmpty(t, resp.Token)
 }
 
+func TestLogin_WithMFA_ClaimFailureFailsClosed(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	service := createTestService(mockStore, new(MockEmailSender))
+
+	mfaSecret := "JBSWY3DPEHPK3PXP"
+	s := newTestService()
+	hash, _ := s.hashPassword("SecurePass@123")
+	user := &User{ID: "user-123", Email: "mfa@example.com", PasswordHash: hash, Active: true, MFAEnabled: true, MFASecret: mfaSecret}
+
+	mockStore.On("GetUserByEmail", ctx, "mfa@example.com").Return(user, nil)
+	mockStore.On("ClaimTOTPCounter", ctx, user.ID, mock.AnythingOfType("int64")).Return(false, errors.New("db down")).Once()
+
+	resp, err := service.Login(ctx, LoginRequest{
+		Email: "mfa@example.com", Password: "SecurePass@123",
+		MFACode: generateTOTP(mfaSecret, time.Now().Unix()/30),
+	})
+	require.ErrorIs(t, err, ErrInvalidMFACode)
+	assert.Nil(t, resp)
+	mockStore.AssertNotCalled(t, "CreateSession", mock.Anything, mock.Anything)
+}
+
 func TestLogin_WithMFA_InvalidCode(t *testing.T) {
 	ctx := context.Background()
 	mockStore := new(MockStore)