From 5dcce313b31ba0109b20d8ea0ed1ba76e00b2c54 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 6 Oct 2026 12:16:14 +0200 Subject: [PATCH 1/2] fix(auth): accept each TOTP code once verifyTOTP accepted counters -1..+1 with no state, so an observed code could be spent again by Login, MFADisable or recovery-code regeneration for up to 90 seconds (RFC 6238 section 5.2). Record the last accepted counter in users.mfa_last_totp_counter (migration 000104) and claim it with a single conditional UPDATE, so a replayed or racing code is rejected as an invalid code. Refs #442 --- internal/auth/interfaces.go | 3 + internal/auth/service.go | 7 +- internal/auth/service_credentials_db_test.go | 2 +- internal/auth/service_mfa.go | 58 ++++++++-- internal/auth/service_mfa_cas_db_test.go | 5 +- internal/auth/service_mfa_test.go | 65 +++++++++++ internal/auth/service_test.go | 1 + internal/auth/service_totp_replay_db_test.go | 107 ++++++++++++++++++ internal/auth/store_postgres_credentials.go | 13 +++ internal/auth/test_helpers.go | 5 + ...000104_user_mfa_last_totp_counter.down.sql | 2 + .../000104_user_mfa_last_totp_counter.up.sql | 4 + .../000104_user_mfa_last_totp_counter_test.go | 37 ++++++ internal/mocks/stores.go | 5 + internal/server/health_test.go | 4 + 15 files changed, 302 insertions(+), 16 deletions(-) create mode 100644 internal/auth/service_totp_replay_db_test.go create mode 100644 internal/database/postgres/migrations/000104_user_mfa_last_totp_counter.down.sql create mode 100644 internal/database/postgres/migrations/000104_user_mfa_last_totp_counter.up.sql create mode 100644 internal/database/postgres/migrations/000104_user_mfa_last_totp_counter_test.go diff --git a/internal/auth/interfaces.go b/internal/auth/interfaces.go index abe409647..f941db33b 100644 --- a/internal/auth/interfaces.go +++ b/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 diff --git a/internal/auth/service.go b/internal/auth/service.go index 8acc62e08..edaa3795d 100644 --- a/internal/auth/service.go +++ b/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 diff --git a/internal/auth/service_credentials_db_test.go b/internal/auth/service_credentials_db_test.go index 0692f26ed..c0c2dc6cd 100644 --- a/internal/auth/service_credentials_db_test.go +++ b/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) } diff --git a/internal/auth/service_mfa.go b/internal/auth/service_mfa.go index 784e312cf..10fd1de53 100644 --- a/internal/auth/service_mfa.go +++ b/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) } diff --git a/internal/auth/service_mfa_cas_db_test.go b/internal/auth/service_mfa_cas_db_test.go index c03475918..81c7f91cb 100644 --- a/internal/auth/service_mfa_cas_db_test.go +++ b/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) }) diff --git a/internal/auth/service_mfa_test.go b/internal/auth/service_mfa_test.go index 3c3a14ee5..66a31a376 100644 --- a/internal/auth/service_mfa_test.go +++ b/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,66 @@ 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) +} diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 846323424..7ce63f50b 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -421,6 +421,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) diff --git a/internal/auth/service_totp_replay_db_test.go b/internal/auth/service_totp_replay_db_test.go new file mode 100644 index 000000000..27d81e3f3 --- /dev/null +++ b/internal/auth/service_totp_replay_db_test.go @@ -0,0 +1,107 @@ +//go:build integration + +package auth + +import ( + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Issue #442: a TOTP code is accepted once, whichever endpoint spends it. +func TestIntegration_TOTPCodeIsAcceptedOnce(t *testing.T) { + store := NewPostgresStore(setupAuthTestDB(t)) + ctx := t.Context() + step := func() int64 { return time.Now().Unix() / 30 } + + login := func(f *credentialRaceFixture, svc *Service, code string) error { + _, err := svc.Login(ctx, LoginRequest{Email: f.user.Email, Password: credentialRacePassword, MFACode: code}) + return err + } + + t.Run("login-replay-rejected-next-step-accepted", func(t *testing.T) { + f := newCredentialRaceFixture(t, store, "totp-replay-login@example.com") + f.enrollMFA() + secret := f.stored().MFASecret + code := generateTOTP(secret, step()) + + require.NoError(t, login(f, f.svc, code)) + require.ErrorIs(t, login(f, f.svc, code), ErrInvalidMFACode, "an accepted code must not log in twice") + require.NoError(t, login(f, f.svc, generateTOTP(secret, step()+1))) + }) + + t.Run("older-step-than-last-accepted-rejected", func(t *testing.T) { + f := newCredentialRaceFixture(t, store, "totp-replay-older@example.com") + f.enrollMFA() + secret := f.stored().MFASecret + + require.NoError(t, login(f, f.svc, generateTOTP(secret, step()+1))) + require.ErrorIs(t, login(f, f.svc, generateTOTP(secret, step())), ErrInvalidMFACode) + }) + + t.Run("wrong-code-does-not-spend-the-step", func(t *testing.T) { + f := newCredentialRaceFixture(t, store, "totp-replay-wrong@example.com") + f.enrollMFA() + secret := f.stored().MFASecret + + require.ErrorIs(t, login(f, f.svc, "000000"), ErrInvalidMFACode) + require.NoError(t, login(f, f.svc, generateTOTP(secret, step()))) + }) + + t.Run("login-code-cannot-disable-mfa", func(t *testing.T) { + f := newCredentialRaceFixture(t, store, "totp-replay-disable@example.com") + f.enrollMFA() + secret := f.stored().MFASecret + code := generateTOTP(secret, step()) + + require.NoError(t, login(f, f.svc, code)) + require.ErrorIs(t, f.svc.MFADisable(ctx, f.user.ID, credentialRacePassword, code), ErrMFAInvalidCode) + assert.True(t, f.stored().MFAEnabled) + require.NoError(t, f.svc.MFADisable(ctx, f.user.ID, credentialRacePassword, generateTOTP(secret, step()+1))) + assert.False(t, f.stored().MFAEnabled) + }) + + t.Run("login-code-cannot-regenerate-recovery-codes", func(t *testing.T) { + f := newCredentialRaceFixture(t, store, "totp-replay-regen@example.com") + codes := f.enrollMFA() + secret := f.stored().MFASecret + code := generateTOTP(secret, step()) + + require.NoError(t, login(f, f.svc, code)) + _, err := f.svc.MFARegenerateRecoveryCodes(ctx, f.user.ID, code) + require.ErrorIs(t, err, ErrMFAInvalidCode) + assert.Len(t, f.stored().MFARecoveryCodes, len(codes), "a replayed code must not replace the recovery codes") + fresh, err := f.svc.MFARegenerateRecoveryCodes(ctx, f.user.ID, generateTOTP(secret, step()+1)) + require.NoError(t, err) + assert.Len(t, fresh, recoveryCodeCount) + }) + + t.Run("concurrent-logins-with-one-code-accept-one", func(t *testing.T) { + f := newCredentialRaceFixture(t, store, "totp-replay-race@example.com") + f.enrollMFA() + code := generateTOTP(f.stored().MFASecret, step()) + + const racers = 8 + var accepted atomic.Int32 + var wg sync.WaitGroup + start := make(chan struct{}) + for range racers { + svc := f.newService(store, f.mail) + wg.Add(1) + go func() { + defer wg.Done() + <-start + if login(f, svc, code) == nil { + accepted.Add(1) + } + }() + } + close(start) + wg.Wait() + assert.Equal(t, int32(1), accepted.Load(), "one code must be accepted at most once under concurrency") + }) +} diff --git a/internal/auth/store_postgres_credentials.go b/internal/auth/store_postgres_credentials.go index 0061ea245..6e0ead3fa 100644 --- a/internal/auth/store_postgres_credentials.go +++ b/internal/auth/store_postgres_credentials.go @@ -170,6 +170,19 @@ func (s *PostgresStore) DisableMFA(ctx context.Context, userID, readSecret strin return nil } +// ClaimTOTPCounter advances the last accepted TOTP counter only while the stored +// one is lower, so a code is accepted once even when two requests race (issue #442). +func (s *PostgresStore) ClaimTOTPCounter(ctx context.Context, userID string, counter int64) (bool, error) { + result, err := s.db.Exec(ctx, ` + UPDATE users SET mfa_last_totp_counter = $2 + WHERE id = $1 AND mfa_last_totp_counter < $2 + `, userID, counter) + if err != nil { + return false, fmt.Errorf("failed to record TOTP counter: %w", err) + } + return result.RowsAffected() == 1, nil +} + // ClearPendingMFASecret cancels a pending enrollment, only while MFA is off, so // a password-only cancel cannot erase a concurrent enrollment (issue #227). func (s *PostgresStore) ClearPendingMFASecret(ctx context.Context, userID string) error { diff --git a/internal/auth/test_helpers.go b/internal/auth/test_helpers.go index 00e422cd3..cdafac98a 100644 --- a/internal/auth/test_helpers.go +++ b/internal/auth/test_helpers.go @@ -94,6 +94,11 @@ func (m *MockStore) ClearPendingMFASecret(ctx context.Context, userID string) er return m.Called(ctx, userID).Error(0) } +func (m *MockStore) ClaimTOTPCounter(ctx context.Context, userID string, counter int64) (bool, error) { + args := m.Called(ctx, userID, counter) + return args.Bool(0), args.Error(1) +} + func (m *MockStore) RecordFailedLogin(ctx context.Context, userID string) error { return m.Called(ctx, userID).Error(0) } diff --git a/internal/database/postgres/migrations/000104_user_mfa_last_totp_counter.down.sql b/internal/database/postgres/migrations/000104_user_mfa_last_totp_counter.down.sql new file mode 100644 index 000000000..87593ad66 --- /dev/null +++ b/internal/database/postgres/migrations/000104_user_mfa_last_totp_counter.down.sql @@ -0,0 +1,2 @@ +-- 000104 down: drop the last accepted TOTP step. +ALTER TABLE users DROP COLUMN IF EXISTS mfa_last_totp_counter; diff --git a/internal/database/postgres/migrations/000104_user_mfa_last_totp_counter.up.sql b/internal/database/postgres/migrations/000104_user_mfa_last_totp_counter.up.sql new file mode 100644 index 000000000..a2bad0846 --- /dev/null +++ b/internal/database/postgres/migrations/000104_user_mfa_last_totp_counter.up.sql @@ -0,0 +1,4 @@ +-- Migration 000104: remember the last accepted TOTP step per user (issue #442). +-- A code is accepted only for a step above this value, so an observed code +-- cannot be spent again inside its validity window (RFC 6238 section 5.2). +ALTER TABLE users ADD COLUMN mfa_last_totp_counter BIGINT NOT NULL DEFAULT 0; diff --git a/internal/database/postgres/migrations/000104_user_mfa_last_totp_counter_test.go b/internal/database/postgres/migrations/000104_user_mfa_last_totp_counter_test.go new file mode 100644 index 000000000..e08aa99ac --- /dev/null +++ b/internal/database/postgres/migrations/000104_user_mfa_last_totp_counter_test.go @@ -0,0 +1,37 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Migration 000104 adds users.mfa_last_totp_counter. Existing rows start at 0 +// (no code accepted yet), and the down migration removes the column. +func TestMigration_UserMFALastTOTPCounter(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 104))) + userID := seedDeactivatedAtUser(ctx, t, pool, "totp-counter@example.com", true, nil, nil) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 104)) + var counter int64 + require.NoError(t, pool.QueryRow(ctx, `SELECT mfa_last_totp_counter FROM users WHERE id = $1`, userID).Scan(&counter)) + assert.Zero(t, counter, "an existing user must start with no accepted code") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 104))) + err = pool.QueryRow(ctx, `SELECT mfa_last_totp_counter FROM users WHERE id = $1`, userID).Scan(&counter) + require.Error(t, err, "the down migration must drop the column") +} diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index f98ab3383..cc6d388a5 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -841,6 +841,11 @@ func (m *MockAuthStore) ClearPendingMFASecret(ctx context.Context, userID string return m.Called(ctx, userID).Error(0) } +func (m *MockAuthStore) ClaimTOTPCounter(ctx context.Context, userID string, counter int64) (bool, error) { + args := m.Called(ctx, userID, counter) + return args.Bool(0), args.Error(1) +} + func (m *MockAuthStore) RecordFailedLogin(ctx context.Context, userID string) error { return m.Called(ctx, userID).Error(0) } diff --git a/internal/server/health_test.go b/internal/server/health_test.go index 962991077..ff2c55263 100644 --- a/internal/server/health_test.go +++ b/internal/server/health_test.go @@ -76,6 +76,10 @@ func (m *mockAuthStoreForHealth) ClearPendingMFASecret(context.Context, string) return nil } +func (m *mockAuthStoreForHealth) ClaimTOTPCounter(context.Context, string, int64) (bool, error) { + return true, nil +} + func (m *mockAuthStoreForHealth) RecordFailedLogin(context.Context, string) error { return nil } From c5a0143c2612a6877d520ee5e45923089c2bfbdf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 6 Oct 2026 12:36:48 +0200 Subject: [PATCH 2/2] test(auth): cover TOTP claim failure in login and MFA disable --- internal/auth/service_mfa_test.go | 17 +++++++++++++++++ internal/auth/service_test.go | 23 +++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/internal/auth/service_mfa_test.go b/internal/auth/service_mfa_test.go index 66a31a376..a807435ab 100644 --- a/internal/auth/service_mfa_test.go +++ b/internal/auth/service_mfa_test.go @@ -750,3 +750,20 @@ func TestMFARegenerateRecoveryCodes_ClaimFailureFailsClosed(t *testing.T) { 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) +} diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 7ce63f50b..2dfc78ed8 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -2,6 +2,7 @@ package auth import ( "context" + "errors" "fmt" "testing" "time" @@ -437,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)