diff --git a/backend/internal/handler/auth_oauth_pending_flow_test.go b/backend/internal/handler/auth_oauth_pending_flow_test.go index 91a65d5f11..dd8e1c03fe 100644 --- a/backend/internal/handler/auth_oauth_pending_flow_test.go +++ b/backend/internal/handler/auth_oauth_pending_flow_test.go @@ -3593,3 +3593,20 @@ func (oauthPendingFlowTotpEncryptorStub) Encrypt(plaintext string) (string, erro func (oauthPendingFlowTotpEncryptorStub) Decrypt(ciphertext string) (string, error) { return ciphertext, nil } + +func (s *oauthPendingFlowEmailCacheStub) IncrVerificationCodeAttempts(_ context.Context, email string) (int, error) { + data := s.verificationCodes[email] + if data == nil { + return 0, errors.New("verification code not found") + } + data.Attempts++ + return data.Attempts, nil +} + +func (s *oauthPendingFlowEmailCacheStub) IncrNotifyVerifyCodeAttempts(context.Context, string) (int, error) { + return 0, errors.New("notify verification code not found") +} + +func (s *oauthPendingFlowEmailCacheStub) ConsumePasswordResetToken(context.Context, string, string) (bool, error) { + return false, nil +} diff --git a/backend/internal/handler/user_handler_test.go b/backend/internal/handler/user_handler_test.go index a40755c920..796c64f339 100644 --- a/backend/internal/handler/user_handler_test.go +++ b/backend/internal/handler/user_handler_test.go @@ -6,6 +6,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "net/http" "net/http/httptest" "testing" @@ -811,3 +812,19 @@ func TestUserHandlerStartIdentityBindingReturnsAuthorizeURL(t *testing.T) { require.Contains(t, resp.Data.AuthorizeURL, "intent=bind_current_user") require.Contains(t, resp.Data.AuthorizeURL, "redirect=%2Fsettings%2Fprofile") } + +func (s *userHandlerEmailCacheStub) IncrVerificationCodeAttempts(context.Context, string) (int, error) { + if s.data == nil { + return 0, errors.New("verification code not found") + } + s.data.Attempts++ + return s.data.Attempts, nil +} + +func (s *userHandlerEmailCacheStub) IncrNotifyVerifyCodeAttempts(context.Context, string) (int, error) { + return 0, errors.New("notify verification code not found") +} + +func (s *userHandlerEmailCacheStub) ConsumePasswordResetToken(context.Context, string, string) (bool, error) { + return false, nil +} diff --git a/backend/internal/repository/email_cache.go b/backend/internal/repository/email_cache.go index 96a23a8ed6..9f59ff05b2 100644 --- a/backend/internal/repository/email_cache.go +++ b/backend/internal/repository/email_cache.go @@ -17,8 +17,42 @@ const ( passwordResetKeyPrefix = "password_reset:" passwordResetSentAtKeyPrefix = "password_reset_sent:" notifyCodeUserRateKeyPrefix = "notify_code_user_rate:" + + // attemptsKeySuffix stores the failed-attempt counter next to a verification code. + // Kept in a separate key so it can be incremented atomically with INCR. + attemptsKeySuffix = ":attempts" ) +// incrAttemptsScript atomically increments the attempt counter for an existing +// verification code and aligns the counter TTL with the code TTL. +// KEYS[1] = code key, KEYS[2] = attempts key. Returns -1 when the code is missing. +var incrAttemptsScript = redis.NewScript(` +if redis.call('EXISTS', KEYS[1]) == 0 then + return -1 +end +local n = redis.call('INCR', KEYS[2]) +local ttl = redis.call('PTTL', KEYS[1]) +if ttl > 0 then + redis.call('PEXPIRE', KEYS[2], ttl) +end +return n +`) + +// consumeResetTokenScript atomically compares the stored token hash and deletes it. +// KEYS[1] = reset key, ARGV[1] = expected token hash. Returns 1 on success, 0 otherwise. +var consumeResetTokenScript = redis.NewScript(` +local v = redis.call('GET', KEYS[1]) +if not v then + return 0 +end +local ok, d = pcall(cjson.decode, v) +if not ok or type(d) ~= 'table' or d['Token'] ~= ARGV[1] then + return 0 +end +redis.call('DEL', KEYS[1]) +return 1 +`) + // verifyCodeKey generates the Redis key for email verification code. // Email is lowercased for case-insensitive consistency. func verifyCodeKey(email string) string { @@ -50,8 +84,7 @@ func NewEmailCache(rdb *redis.Client) service.EmailCache { return &emailCache{rdb: rdb} } -func (c *emailCache) GetVerificationCode(ctx context.Context, email string) (*service.VerificationCodeData, error) { - key := verifyCodeKey(email) +func (c *emailCache) getCode(ctx context.Context, key string) (*service.VerificationCodeData, error) { val, err := c.rdb.Get(ctx, key).Result() if err != nil { return nil, err @@ -60,21 +93,56 @@ func (c *emailCache) GetVerificationCode(ctx context.Context, email string) (*se if err := json.Unmarshal([]byte(val), &data); err != nil { return nil, err } + if n, err := c.rdb.Get(ctx, key+attemptsKeySuffix).Int(); err == nil && n > data.Attempts { + data.Attempts = n + } return &data, nil } -func (c *emailCache) SetVerificationCode(ctx context.Context, email string, data *service.VerificationCodeData, ttl time.Duration) error { - key := verifyCodeKey(email) +func (c *emailCache) setCode(ctx context.Context, key string, data *service.VerificationCodeData, ttl time.Duration) error { val, err := json.Marshal(data) if err != nil { return err } - return c.rdb.Set(ctx, key, val, ttl).Err() + pipe := c.rdb.TxPipeline() + pipe.Set(ctx, key, val, ttl) + pipe.Del(ctx, key+attemptsKeySuffix) + if data.Attempts > 0 { + pipe.Set(ctx, key+attemptsKeySuffix, data.Attempts, ttl) + } + _, err = pipe.Exec(ctx) + return err +} + +func (c *emailCache) incrCodeAttempts(ctx context.Context, key string) (int, error) { + n, err := incrAttemptsScript.Run(ctx, c.rdb, []string{key, key + attemptsKeySuffix}).Int() + if err != nil { + return 0, err + } + if n < 0 { + return 0, redis.Nil + } + return n, nil +} + +func (c *emailCache) deleteCode(ctx context.Context, key string) error { + return c.rdb.Del(ctx, key, key+attemptsKeySuffix).Err() +} + +func (c *emailCache) GetVerificationCode(ctx context.Context, email string) (*service.VerificationCodeData, error) { + return c.getCode(ctx, verifyCodeKey(email)) +} + +func (c *emailCache) SetVerificationCode(ctx context.Context, email string, data *service.VerificationCodeData, ttl time.Duration) error { + return c.setCode(ctx, verifyCodeKey(email), data, ttl) +} + +func (c *emailCache) IncrVerificationCodeAttempts(ctx context.Context, email string) (int, error) { + return c.incrCodeAttempts(ctx, verifyCodeKey(email)) } func (c *emailCache) DeleteVerificationCode(ctx context.Context, email string) error { - key := verifyCodeKey(email) - return c.rdb.Del(ctx, key).Err() + return c.deleteCode(ctx, verifyCodeKey(email)) } // Password reset token methods @@ -101,6 +169,16 @@ func (c *emailCache) SetPasswordResetToken(ctx context.Context, email string, da return c.rdb.Set(ctx, key, val, ttl).Err() } +// ConsumePasswordResetToken atomically deletes the stored reset token when its +// stored hash equals tokenHash. Returns true only for the single winning caller. +func (c *emailCache) ConsumePasswordResetToken(ctx context.Context, email, tokenHash string) (bool, error) { + n, err := consumeResetTokenScript.Run(ctx, c.rdb, []string{passwordResetKey(email)}, tokenHash).Int() + if err != nil { + return false, err + } + return n == 1, nil +} + func (c *emailCache) DeletePasswordResetToken(ctx context.Context, email string) error { key := passwordResetKey(email) return c.rdb.Del(ctx, key).Err() @@ -122,30 +200,19 @@ func (c *emailCache) SetPasswordResetEmailCooldown(ctx context.Context, email st // Notify email verification code methods func (c *emailCache) GetNotifyVerifyCode(ctx context.Context, email string) (*service.VerificationCodeData, error) { - key := notifyVerifyKey(email) - val, err := c.rdb.Get(ctx, key).Result() - if err != nil { - return nil, err - } - var data service.VerificationCodeData - if err := json.Unmarshal([]byte(val), &data); err != nil { - return nil, err - } - return &data, nil + return c.getCode(ctx, notifyVerifyKey(email)) } func (c *emailCache) SetNotifyVerifyCode(ctx context.Context, email string, data *service.VerificationCodeData, ttl time.Duration) error { - key := notifyVerifyKey(email) - val, err := json.Marshal(data) - if err != nil { - return err - } - return c.rdb.Set(ctx, key, val, ttl).Err() + return c.setCode(ctx, notifyVerifyKey(email), data, ttl) +} + +func (c *emailCache) IncrNotifyVerifyCodeAttempts(ctx context.Context, email string) (int, error) { + return c.incrCodeAttempts(ctx, notifyVerifyKey(email)) } func (c *emailCache) DeleteNotifyVerifyCode(ctx context.Context, email string) error { - key := notifyVerifyKey(email) - return c.rdb.Del(ctx, key).Err() + return c.deleteCode(ctx, notifyVerifyKey(email)) } // User-level rate limiting for notify email verification codes diff --git a/backend/internal/repository/email_cache_atomic_test.go b/backend/internal/repository/email_cache_atomic_test.go new file mode 100644 index 0000000000..0086bbe460 --- /dev/null +++ b/backend/internal/repository/email_cache_atomic_test.go @@ -0,0 +1,150 @@ +package repository + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/Wei-Shaw/sub2api/internal/service" + "github.com/alicebob/miniredis/v2" + "github.com/redis/go-redis/v9" + "github.com/stretchr/testify/require" +) + +func newMiniredisEmailCache(t *testing.T) (service.EmailCache, *miniredis.Miniredis, *redis.Client) { + t.Helper() + mr := miniredis.RunT(t) + rdb := redis.NewClient(&redis.Options{Addr: mr.Addr()}) + t.Cleanup(func() { _ = rdb.Close() }) + return NewEmailCache(rdb), mr, rdb +} + +func TestEmailCache_ConcurrentWrongCodesCannotExceedAttemptCap(t *testing.T) { + cache, _, _ := newMiniredisEmailCache(t) + ctx := context.Background() + email := "user@example.com" + + svc := service.NewEmailService(nil, cache) + require.NoError(t, cache.SetVerificationCode(ctx, email, &service.VerificationCodeData{ + Code: "123456", + CreatedAt: time.Now(), + ExpiresAt: time.Now().Add(15 * time.Minute), + }, 15*time.Minute)) + + const workers = 50 + var invalid, maxed atomic.Int32 + var wg sync.WaitGroup + for i := 0; i < workers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + err := svc.VerifyCode(ctx, email, "000000") + switch { + case errors.Is(err, service.ErrInvalidVerifyCode): + invalid.Add(1) + case errors.Is(err, service.ErrVerifyCodeMaxAttempts): + maxed.Add(1) + default: + t.Errorf("unexpected result: %v", err) + } + }() + } + wg.Wait() + + // Only attempts 1..4 may return "invalid"; every other guess is rejected by the cap. + require.LessOrEqual(t, int(invalid.Load()), 4) + require.Equal(t, workers, int(invalid.Load()+maxed.Load())) + + // Even the correct code is now rejected. + require.ErrorIs(t, svc.VerifyCode(ctx, email, "123456"), service.ErrVerifyCodeMaxAttempts) + + data, err := cache.GetVerificationCode(ctx, email) + require.NoError(t, err) + require.GreaterOrEqual(t, data.Attempts, 5) +} + +func TestEmailCache_AttemptsResetOnNewCodeAndTTLFollowsCode(t *testing.T) { + cache, mr, _ := newMiniredisEmailCache(t) + ctx := context.Background() + email := "User@Example.com" + + require.NoError(t, cache.SetVerificationCode(ctx, email, &service.VerificationCodeData{Code: "1"}, time.Minute)) + n, err := cache.IncrVerificationCodeAttempts(ctx, email) + require.NoError(t, err) + require.Equal(t, 1, n) + require.Greater(t, mr.TTL(verifyCodeKey(email)+attemptsKeySuffix), time.Duration(0)) + + require.NoError(t, cache.SetVerificationCode(ctx, email, &service.VerificationCodeData{Code: "2"}, time.Minute)) + data, err := cache.GetVerificationCode(ctx, email) + require.NoError(t, err) + require.Equal(t, 0, data.Attempts) + + require.NoError(t, cache.DeleteVerificationCode(ctx, email)) + _, err = cache.IncrVerificationCodeAttempts(ctx, email) + require.Error(t, err) + require.False(t, mr.Exists(verifyCodeKey(email)+attemptsKeySuffix)) +} + +func TestEmailCache_PasswordResetTokenHashedAndSingleUse(t *testing.T) { + cache, mr, _ := newMiniredisEmailCache(t) + ctx := context.Background() + email := "reset@example.com" + + svc := service.NewEmailService(nil, cache) + + // Seed the token the same way SendPasswordResetEmail does (hash only). + token, err := svc.GeneratePasswordResetToken() + require.NoError(t, err) + sum := sha256.Sum256([]byte(token)) + require.NoError(t, cache.SetPasswordResetToken(ctx, email, &service.PasswordResetTokenData{ + Token: hex.EncodeToString(sum[:]), CreatedAt: time.Now(), + }, 30*time.Minute)) + + raw, err := mr.Get(passwordResetKey(email)) + require.NoError(t, err) + require.False(t, strings.Contains(raw, token), "plaintext token must not be stored") + + require.NoError(t, svc.VerifyPasswordResetToken(ctx, email, token)) + require.ErrorIs(t, svc.ConsumePasswordResetToken(ctx, email, "wrong"), service.ErrInvalidResetToken) + + const workers = 30 + var ok atomic.Int32 + var wg sync.WaitGroup + for i := 0; i < workers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + if svc.ConsumePasswordResetToken(ctx, email, token) == nil { + ok.Add(1) + } + }() + } + wg.Wait() + require.Equal(t, int32(1), ok.Load()) + require.False(t, mr.Exists(passwordResetKey(email))) +} + +func TestEmailCache_ConsumePasswordResetTokenMismatchKeepsToken(t *testing.T) { + cache, mr, _ := newMiniredisEmailCache(t) + ctx := context.Background() + email := "keep@example.com" + require.NoError(t, cache.SetPasswordResetToken(ctx, email, &service.PasswordResetTokenData{Token: "abc"}, time.Minute)) + + ok, err := cache.ConsumePasswordResetToken(ctx, email, "xyz") + require.NoError(t, err) + require.False(t, ok) + require.True(t, mr.Exists(passwordResetKey(email))) + + ok, err = cache.ConsumePasswordResetToken(ctx, email, "abc") + require.NoError(t, err) + require.True(t, ok) + ok, err = cache.ConsumePasswordResetToken(ctx, email, "abc") + require.NoError(t, err) + require.False(t, ok) +} diff --git a/backend/internal/service/auth_service_email_bind_test.go b/backend/internal/service/auth_service_email_bind_test.go index 9bdca02f2d..c41734c40b 100644 --- a/backend/internal/service/auth_service_email_bind_test.go +++ b/backend/internal/service/auth_service_email_bind_test.go @@ -1165,3 +1165,19 @@ func cloneEmailBindUser(user *service.User) *service.User { cloned := *user return &cloned } + +func (s *emailBindCacheStub) IncrVerificationCodeAttempts(context.Context, string) (int, error) { + if s.data == nil { + return 0, errors.New("verification code not found") + } + s.data.Attempts++ + return s.data.Attempts, nil +} + +func (s *emailBindCacheStub) IncrNotifyVerifyCodeAttempts(context.Context, string) (int, error) { + return 0, errors.New("notify verification code not found") +} + +func (s *emailBindCacheStub) ConsumePasswordResetToken(context.Context, string, string) (bool, error) { + return false, nil +} diff --git a/backend/internal/service/auth_service_register_test.go b/backend/internal/service/auth_service_register_test.go index aa7519f3d1..076cd025d3 100644 --- a/backend/internal/service/auth_service_register_test.go +++ b/backend/internal/service/auth_service_register_test.go @@ -1010,3 +1010,19 @@ func TestCanBypassRegistrationDisabledForOAuth(t *testing.T) { }) } } + +func (s *emailCacheStub) IncrVerificationCodeAttempts(context.Context, string) (int, error) { + if s.data == nil { + return 0, errors.New("verification code not found") + } + s.data.Attempts++ + return s.data.Attempts, nil +} + +func (s *emailCacheStub) IncrNotifyVerifyCodeAttempts(context.Context, string) (int, error) { + return 0, errors.New("notify verification code not found") +} + +func (s *emailCacheStub) ConsumePasswordResetToken(context.Context, string, string) (bool, error) { + return false, nil +} diff --git a/backend/internal/service/email_service.go b/backend/internal/service/email_service.go index 8e60d9e73d..794dad5258 100644 --- a/backend/internal/service/email_service.go +++ b/backend/internal/service/email_service.go @@ -3,6 +3,7 @@ package service import ( "context" "crypto/rand" + "crypto/sha256" "crypto/subtle" "crypto/tls" "crypto/x509" @@ -37,16 +38,23 @@ type EmailCache interface { GetVerificationCode(ctx context.Context, email string) (*VerificationCodeData, error) SetVerificationCode(ctx context.Context, email string, data *VerificationCodeData, ttl time.Duration) error DeleteVerificationCode(ctx context.Context, email string) error + // IncrVerificationCodeAttempts atomically increments the attempt counter of an + // existing code and returns the new count. Returns an error if the code is missing. + IncrVerificationCodeAttempts(ctx context.Context, email string) (int, error) // Notify email verification code methods GetNotifyVerifyCode(ctx context.Context, email string) (*VerificationCodeData, error) SetNotifyVerifyCode(ctx context.Context, email string, data *VerificationCodeData, ttl time.Duration) error DeleteNotifyVerifyCode(ctx context.Context, email string) error + IncrNotifyVerifyCodeAttempts(ctx context.Context, email string) (int, error) // Password reset token methods GetPasswordResetToken(ctx context.Context, email string) (*PasswordResetTokenData, error) SetPasswordResetToken(ctx context.Context, email string, data *PasswordResetTokenData, ttl time.Duration) error DeletePasswordResetToken(ctx context.Context, email string) error + // ConsumePasswordResetToken atomically compares the stored token hash with + // tokenHash and deletes it on match. Only one concurrent caller can succeed. + ConsumePasswordResetToken(ctx context.Context, email, tokenHash string) (bool, error) // Password reset email cooldown methods // Returns true if in cooldown period (email was sent recently) @@ -68,6 +76,7 @@ type VerificationCodeData struct { // PasswordResetTokenData represents password reset token data type PasswordResetTokenData struct { + // Token holds the hex-encoded SHA-256 hash of the reset token (never the plaintext). Token string CreatedAt time.Time } @@ -376,35 +385,49 @@ func (s *EmailService) SendVerifyCode(ctx context.Context, email, siteName strin // VerifyCode 验证验证码 func (s *EmailService) VerifyCode(ctx context.Context, email, code string) error { - data, err := s.cache.GetVerificationCode(ctx, email) + return verifyCodeWithAttempts(ctx, email, code, + s.cache.GetVerificationCode, s.cache.IncrVerificationCodeAttempts, + func() { + if err := s.cache.DeleteVerificationCode(ctx, email); err != nil { + slog.Error("failed to delete verification code after success", "email", email, "error", err) + } + }) +} + +// verifyCodeWithAttempts checks a verification code while enforcing the attempt cap +// atomically: each check first reserves an attempt via an atomic increment, so +// concurrent guesses can never evaluate more than maxVerifyCodeAttempts codes. +func verifyCodeWithAttempts( + ctx context.Context, email, code string, + get func(context.Context, string) (*VerificationCodeData, error), + incr func(context.Context, string) (int, error), + onSuccess func(), +) error { + data, err := get(ctx, email) if err != nil || data == nil { return ErrInvalidVerifyCode } - - // 检查是否已达到最大尝试次数 if data.Attempts >= maxVerifyCodeAttempts { return ErrVerifyCodeMaxAttempts } + attempts, err := incr(ctx, email) + if err != nil { + return ErrInvalidVerifyCode + } + if attempts > maxVerifyCodeAttempts { + return ErrVerifyCodeMaxAttempts + } - // 验证码不匹配 (constant-time comparison to prevent timing attacks) + // constant-time comparison to prevent timing attacks if subtle.ConstantTimeCompare([]byte(data.Code), []byte(code)) != 1 { - data.Attempts++ - remaining := time.Until(data.ExpiresAt) - if remaining <= 0 { - return ErrInvalidVerifyCode - } - if err := s.cache.SetVerificationCode(ctx, email, data, remaining); err != nil { - slog.Error("failed to update verification attempt count", "email", email, "error", err) - } - if data.Attempts >= maxVerifyCodeAttempts { + if attempts >= maxVerifyCodeAttempts { return ErrVerifyCodeMaxAttempts } return ErrInvalidVerifyCode } - // 验证成功,删除验证码 - if err := s.cache.DeleteVerificationCode(ctx, email); err != nil { - slog.Error("failed to delete verification code after success", "email", email, "error", err) + if onSuccess != nil { + onSuccess() } return nil } @@ -480,33 +503,18 @@ func (s *EmailService) GeneratePasswordResetToken() (string, error) { // SendPasswordResetEmail sends a password reset email with a reset link func (s *EmailService) SendPasswordResetEmail(ctx context.Context, email, siteName, resetURL string, locale ...string) error { - var token string - var needSaveToken bool - - // Check if token already exists - existing, err := s.cache.GetPasswordResetToken(ctx, email) - if err == nil && existing != nil { - // Token exists, reuse it (allows resending email without generating new token) - token = existing.Token - needSaveToken = false - } else { - // Generate new token - token, err = s.GeneratePasswordResetToken() - if err != nil { - return fmt.Errorf("generate token: %w", err) - } - needSaveToken = true + // Only the SHA-256 hash of the token is stored, so an existing token cannot be + // re-sent; always issue a fresh token (the email cooldown bounds resend frequency). + token, err := s.GeneratePasswordResetToken() + if err != nil { + return fmt.Errorf("generate token: %w", err) } - - // Save token to Redis (only if new token generated) - if needSaveToken { - data := &PasswordResetTokenData{ - Token: token, - CreatedAt: time.Now(), - } - if err := s.cache.SetPasswordResetToken(ctx, email, data, passwordResetTokenTTL); err != nil { - return fmt.Errorf("save reset token: %w", err) - } + data := &PasswordResetTokenData{ + Token: hashPasswordResetToken(token), + CreatedAt: time.Now(), + } + if err := s.cache.SetPasswordResetToken(ctx, email, data, passwordResetTokenTTL); err != nil { + return fmt.Errorf("save reset token: %w", err) } // Build full reset URL with URL-encoded token and email @@ -566,31 +574,40 @@ func (s *EmailService) SendPasswordResetEmailWithCooldown(ctx context.Context, e return nil } +// hashPasswordResetToken returns the hex-encoded SHA-256 of a reset token. +func hashPasswordResetToken(token string) string { + sum := sha256.Sum256([]byte(token)) + return hex.EncodeToString(sum[:]) +} + // VerifyPasswordResetToken verifies the password reset token without consuming it func (s *EmailService) VerifyPasswordResetToken(ctx context.Context, email, token string) error { data, err := s.cache.GetPasswordResetToken(ctx, email) - if err != nil || data == nil { + if err != nil || data == nil || token == "" { return ErrInvalidResetToken } // Use constant-time comparison to prevent timing attacks - if subtle.ConstantTimeCompare([]byte(data.Token), []byte(token)) != 1 { + if subtle.ConstantTimeCompare([]byte(data.Token), []byte(hashPasswordResetToken(token))) != 1 { return ErrInvalidResetToken } return nil } -// ConsumePasswordResetToken verifies and deletes the token (one-time use) +// ConsumePasswordResetToken verifies and deletes the token atomically (one-time use). func (s *EmailService) ConsumePasswordResetToken(ctx context.Context, email, token string) error { - // Verify first + // Constant-time pre-check in Go; the atomic compare-and-delete below is authoritative. if err := s.VerifyPasswordResetToken(ctx, email, token); err != nil { return err } - - // Delete after verification (one-time use) - if err := s.cache.DeletePasswordResetToken(ctx, email); err != nil { - slog.Error("failed to delete password reset token after consumption", "email", email, "error", err) + ok, err := s.cache.ConsumePasswordResetToken(ctx, email, hashPasswordResetToken(token)) + if err != nil { + slog.Error("failed to consume password reset token", "email", email, "error", err) + return ErrInvalidResetToken + } + if !ok { + return ErrInvalidResetToken } return nil } diff --git a/backend/internal/service/email_service_reset_token_test.go b/backend/internal/service/email_service_reset_token_test.go new file mode 100644 index 0000000000..15efd50d6e --- /dev/null +++ b/backend/internal/service/email_service_reset_token_test.go @@ -0,0 +1,50 @@ +//go:build unit + +package service + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "testing" + + "github.com/stretchr/testify/require" +) + +type resetTokenCacheStub struct { + emailCacheStub + stored *PasswordResetTokenData + consumedHash string +} + +func (s *resetTokenCacheStub) GetPasswordResetToken(context.Context, string) (*PasswordResetTokenData, error) { + return s.stored, nil +} + +func (s *resetTokenCacheStub) ConsumePasswordResetToken(_ context.Context, _ string, tokenHash string) (bool, error) { + s.consumedHash = tokenHash + if s.stored == nil || s.stored.Token != tokenHash { + return false, nil + } + s.stored = nil + return true, nil +} + +func TestConsumePasswordResetToken_ComparesHashNotPlaintext(t *testing.T) { + token := "deadbeef" + sum := sha256.Sum256([]byte(token)) + hash := hex.EncodeToString(sum[:]) + require.Equal(t, hash, hashPasswordResetToken(token)) + require.NotEqual(t, token, hashPasswordResetToken(token)) + + cache := &resetTokenCacheStub{stored: &PasswordResetTokenData{Token: hash}} + svc := NewEmailService(nil, cache) + + require.NoError(t, svc.ConsumePasswordResetToken(context.Background(), "a@b.c", token)) + require.Equal(t, hash, cache.consumedHash) + require.ErrorIs(t, svc.ConsumePasswordResetToken(context.Background(), "a@b.c", token), ErrInvalidResetToken) + + // A legacy plaintext value (issued before upgrade) no longer validates. + cache.stored = &PasswordResetTokenData{Token: token} + require.ErrorIs(t, svc.ConsumePasswordResetToken(context.Background(), "a@b.c", token), ErrInvalidResetToken) +} diff --git a/backend/internal/service/user_service.go b/backend/internal/service/user_service.go index ee9a7f257f..e6d4e50736 100644 --- a/backend/internal/service/user_service.go +++ b/backend/internal/service/user_service.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "crypto/sha256" - "crypto/subtle" "encoding/base64" "encoding/hex" "fmt" @@ -1318,28 +1317,7 @@ func (s *UserService) VerifyAndAddNotifyEmail(ctx context.Context, userID int64, // verifyNotifyCode validates the verification code against the cached data. func verifyNotifyCode(ctx context.Context, cache EmailCache, email, code string) error { - data, err := cache.GetNotifyVerifyCode(ctx, email) - if err != nil || data == nil { - return ErrInvalidVerifyCode - } - if data.Attempts >= maxVerifyCodeAttempts { - return ErrVerifyCodeMaxAttempts - } - if subtle.ConstantTimeCompare([]byte(data.Code), []byte(code)) != 1 { - data.Attempts++ - remaining := time.Until(data.ExpiresAt) - if remaining <= 0 { - return ErrInvalidVerifyCode - } - if err := cache.SetNotifyVerifyCode(ctx, email, data, remaining); err != nil { - slog.Error("failed to update notify verify code attempts", "email", email, "error", err) - } - if data.Attempts >= maxVerifyCodeAttempts { - return ErrVerifyCodeMaxAttempts - } - return ErrInvalidVerifyCode - } - return nil + return verifyCodeWithAttempts(ctx, email, code, cache.GetNotifyVerifyCode, cache.IncrNotifyVerifyCodeAttempts, nil) } // addOrVerifyNotifyEmail adds the email to user's extra notification emails or marks it as verified.