Merge 08cd9c2d8a30109c03bb9ead4a30fef100e54700 into 74158715cde6d7b767ead23d9a2af64b7b58a588

common/verification.go

@@ -1,10 +1,15 @@
 package common
 
 import (
+	"context"
+	"crypto/sha256"
+	"crypto/subtle"
+	"fmt"
 	"strings"
 	"sync"
 	"time"
 
+	"github.com/go-redis/redis/v8"
 	"github.com/google/uuid"
 )
 
@@ -16,6 +21,7 @@ type verificationValue struct {
 const (
 	EmailVerificationPurpose = "v"
 	PasswordResetPurpose     = "r"
+	verificationRedisPrefix  = "verification:"
 )
 
 var verificationMutex sync.Mutex
@@ -23,6 +29,24 @@ var verificationMap map[string]verificationValue
 var verificationMapMaxSize = 10
 var VerificationValidMinutes = 10
 
+var consumeVerificationCodeScript = redis.NewScript(`
+local stored = redis.call("GET", KEYS[1])
+if not stored or stored ~= ARGV[1] then
+  return -1
+end
+local ttl = redis.call("PTTL", KEYS[1])
+redis.call("DEL", KEYS[1])
+return ttl
+`)
+
+var restoreVerificationCodeScript = redis.NewScript(`
+if redis.call("EXISTS", KEYS[1]) == 1 then
+  return 0
+end
+redis.call("SET", KEYS[1], ARGV[1], "PX", ARGV[2])
+return 1
+`)
+
 func GenerateVerificationCode(length int) string {
 	code := uuid.New().String()
 	code = strings.Replace(code, "-", "", -1)
@@ -32,7 +56,28 @@ func GenerateVerificationCode(length int) string {
 	return code[:length]
 }
 
-func RegisterVerificationCodeWithKey(key string, code string, purpose string) {
+func verificationStorageKey(key string, purpose string) string {
+	digest := sha256.Sum256([]byte(key))
+	return fmt.Sprintf("%s%s:%x", verificationRedisPrefix, purpose, digest)
+}
+
+func verificationTTL() time.Duration {
+	return time.Duration(VerificationValidMinutes) * time.Minute
+}
+
+func RegisterVerificationCodeWithKey(key string, code string, purpose string) error {
+	if RedisEnabled {
+		if RDB == nil {
+			return fmt.Errorf("verification code storage: Redis is enabled but unavailable")
+		}
+		// Do not use RedisSet here: its debug logging includes the value, which
+		// would disclose the verification code in application logs.
+		if err := RDB.Set(context.Background(), verificationStorageKey(key, purpose), code, verificationTTL()).Err(); err != nil {
+			return fmt.Errorf("store verification code in Redis: %w", err)
+		}
+		return nil
+	}
+
 	verificationMutex.Lock()
 	defer verificationMutex.Unlock()
 	verificationMap[purpose+key] = verificationValue{
@@ -42,23 +87,134 @@ func RegisterVerificationCodeWithKey(key string, code string, purpose string) {
 	if len(verificationMap) > verificationMapMaxSize {
 		removeExpiredPairs()
 	}
+	return nil
 }
 
-func VerifyCodeWithKey(key string, code string, purpose string) bool {
+func VerifyCodeWithKey(key string, code string, purpose string) (bool, error) {
+	if RedisEnabled {
+		if RDB == nil {
+			return false, fmt.Errorf("verification code storage: Redis is enabled but unavailable")
+		}
+		storedCode, err := RedisGet(verificationStorageKey(key, purpose))
+		if err != nil {
+			// A missing or expired code is an ordinary failed verification. Other
+			// Redis errors must remain distinguishable from an invalid code.
+			if err == redis.Nil {
+				return false, nil
+			}
+			return false, fmt.Errorf("read verification code from Redis: %w", err)
+		}
+		return secureCodeEqual(storedCode, code), nil
+	}
+
 	verificationMutex.Lock()
 	defer verificationMutex.Unlock()
 	value, okay := verificationMap[purpose+key]
 	now := time.Now()
 	if !okay || int(now.Sub(value.time).Seconds()) >= VerificationValidMinutes*60 {
-		return false
+		return false, nil
 	}
-	return code == value.code
+	return secureCodeEqual(value.code, code), nil
 }
 
-func DeleteKey(key string, purpose string) {
+// ConsumeVerificationCodeWithKey atomically validates and deletes a code.
+// Use it when replay must be prevented before performing the protected action.
+func ConsumeVerificationCodeWithKey(key string, code string, purpose string) (bool, error) {
+	valid, _, err := ConsumeVerificationCodeWithTTL(key, code, purpose)
+	return valid, err
+}
+
+// ConsumeVerificationCodeWithTTL atomically validates and deletes a code and
+// returns its remaining validity for failure recovery.
+func ConsumeVerificationCodeWithTTL(key string, code string, purpose string) (bool, time.Duration, error) {
+	if RedisEnabled {
+		if RDB == nil {
+			return false, 0, fmt.Errorf("verification code storage: Redis is enabled but unavailable")
+		}
+		remainingMilliseconds, err := consumeVerificationCodeScript.Run(
+			context.Background(),
+			RDB,
+			[]string{verificationStorageKey(key, purpose)},
+			code,
+		).Int()
+		if err != nil {
+			return false, 0, fmt.Errorf("consume verification code from Redis: %w", err)
+		}
+		if remainingMilliseconds < 0 {
+			return false, 0, nil
+		}
+		return true, time.Duration(remainingMilliseconds) * time.Millisecond, nil
+	}
+
+	verificationMutex.Lock()
+	defer verificationMutex.Unlock()
+	storageKey := purpose + key
+	value, okay := verificationMap[storageKey]
+	remaining := verificationTTL() - time.Since(value.time)
+	if !okay || remaining <= 0 {
+		return false, 0, nil
+	}
+	if !secureCodeEqual(value.code, code) {
+		return false, 0, nil
+	}
+	delete(verificationMap, storageKey)
+	return true, remaining, nil
+}
+
+// RestoreVerificationCodeIfAbsent restores a consumed code after the protected
+// operation fails, without overwriting a newer code issued concurrently.
+func RestoreVerificationCodeIfAbsent(key string, code string, purpose string, remaining time.Duration) error {
+	if remaining <= 0 {
+		return nil
+	}
+	if RedisEnabled {
+		if RDB == nil {
+			return fmt.Errorf("verification code storage: Redis is enabled but unavailable")
+		}
+		_, err := restoreVerificationCodeScript.Run(
+			context.Background(),
+			RDB,
+			[]string{verificationStorageKey(key, purpose)},
+			code,
+			remaining.Milliseconds(),
+		).Int()
+		if err != nil {
+			return fmt.Errorf("restore verification code in Redis: %w", err)
+		}
+		return nil
+	}
+
+	verificationMutex.Lock()
+	defer verificationMutex.Unlock()
+	storageKey := purpose + key
+	if _, exists := verificationMap[storageKey]; !exists {
+		verificationMap[storageKey] = verificationValue{
+			code: code,
+			time: time.Now().Add(remaining - verificationTTL()),
+		}
+	}
+	return nil
+}
+
+func secureCodeEqual(expected string, actual string) bool {
+	return subtle.ConstantTimeCompare([]byte(expected), []byte(actual)) == 1
+}
+
+func DeleteKey(key string, purpose string) error {
+	if RedisEnabled {
+		if RDB == nil {
+			return fmt.Errorf("verification code storage: Redis is enabled but unavailable")
+		}
+		if err := RedisDel(verificationStorageKey(key, purpose)); err != nil {
+			return fmt.Errorf("delete verification code from Redis: %w", err)
+		}
+		return nil
+	}
+
 	verificationMutex.Lock()
 	defer verificationMutex.Unlock()
 	delete(verificationMap, purpose+key)
+	return nil
 }
 
 // no lock inside, so the caller must lock the verificationMap before calling!

common/verification_test.go

@@ -0,0 +1,241 @@
+package common
+
+import (
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/alicebob/miniredis/v2"
+	"github.com/go-redis/redis/v8"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func useVerificationTestState(t *testing.T) {
+	t.Helper()
+	previousRedisEnabled := RedisEnabled
+	previousRDB := RDB
+	previousValidMinutes := VerificationValidMinutes
+	verificationMutex.Lock()
+	previousMap := verificationMap
+	verificationMap = make(map[string]verificationValue)
+	verificationMutex.Unlock()
+	t.Cleanup(func() {
+		RedisEnabled = previousRedisEnabled
+		RDB = previousRDB
+		VerificationValidMinutes = previousValidMinutes
+		verificationMutex.Lock()
+		verificationMap = previousMap
+		verificationMutex.Unlock()
+	})
+}
+
+func TestVerificationCodeUsesMemoryWithoutRedis(t *testing.T) {
+	useVerificationTestState(t)
+	RedisEnabled = false
+
+	require.NoError(t, RegisterVerificationCodeWithKey("user@example.com", "123456", EmailVerificationPurpose))
+	valid, err := VerifyCodeWithKey("user@example.com", "123456", EmailVerificationPurpose)
+	require.NoError(t, err)
+	assert.True(t, valid)
+
+	valid, err = VerifyCodeWithKey("user@example.com", "wrong", EmailVerificationPurpose)
+	require.NoError(t, err)
+	assert.False(t, valid)
+
+	require.NoError(t, DeleteKey("user@example.com", EmailVerificationPurpose))
+	valid, err = VerifyCodeWithKey("user@example.com", "123456", EmailVerificationPurpose)
+	require.NoError(t, err)
+	assert.False(t, valid)
+}
+
+func TestVerificationCodeIsSharedAcrossRedisClients(t *testing.T) {
+	useVerificationTestState(t)
+	server := miniredis.RunT(t)
+	clientA := redis.NewClient(&redis.Options{Addr: server.Addr()})
+	clientB := redis.NewClient(&redis.Options{Addr: server.Addr()})
+	t.Cleanup(func() {
+		_ = clientA.Close()
+		_ = clientB.Close()
+	})
+	RedisEnabled = true
+	RDB = clientA
+
+	require.NoError(t, RegisterVerificationCodeWithKey("cluster@example.com", "654321", EmailVerificationPurpose))
+	RDB = clientB
+	valid, err := VerifyCodeWithKey("cluster@example.com", "654321", EmailVerificationPurpose)
+	require.NoError(t, err)
+	assert.True(t, valid, "a code created by one node must be readable by another node")
+}
+
+func TestVerificationCodeRedisTTLAndPurposeIsolation(t *testing.T) {
+	useVerificationTestState(t)
+	server := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: server.Addr()})
+	t.Cleanup(func() { _ = client.Close() })
+	RedisEnabled = true
+	RDB = client
+	VerificationValidMinutes = 10
+
+	require.NoError(t, RegisterVerificationCodeWithKey("same@example.com", "email-code", EmailVerificationPurpose))
+	require.NoError(t, RegisterVerificationCodeWithKey("same@example.com", "reset-code", PasswordResetPurpose))
+
+	valid, err := VerifyCodeWithKey("same@example.com", "email-code", PasswordResetPurpose)
+	require.NoError(t, err)
+	assert.False(t, valid)
+	valid, err = VerifyCodeWithKey("same@example.com", "reset-code", PasswordResetPurpose)
+	require.NoError(t, err)
+	assert.True(t, valid)
+
+	server.FastForward(10 * time.Minute)
+	valid, err = VerifyCodeWithKey("same@example.com", "email-code", EmailVerificationPurpose)
+	require.NoError(t, err)
+	assert.False(t, valid)
+	valid, err = VerifyCodeWithKey("same@example.com", "reset-code", PasswordResetPurpose)
+	require.NoError(t, err)
+	assert.False(t, valid)
+}
+
+func TestConsumeVerificationCodeIsAtomicInMemory(t *testing.T) {
+	useVerificationTestState(t)
+	RedisEnabled = false
+	require.NoError(t, RegisterVerificationCodeWithKey("consume@example.com", "123456", EmailVerificationPurpose))
+
+	assertExactlyOneVerificationConsumer(t, func() (bool, error) {
+		return ConsumeVerificationCodeWithKey("consume@example.com", "123456", EmailVerificationPurpose)
+	})
+}
+
+func TestConsumeVerificationCodeIsAtomicInRedis(t *testing.T) {
+	useVerificationTestState(t)
+	server := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: server.Addr()})
+	t.Cleanup(func() { _ = client.Close() })
+	RedisEnabled = true
+	RDB = client
+	require.NoError(t, RegisterVerificationCodeWithKey("consume@example.com", "123456", PasswordResetPurpose))
+
+	assertExactlyOneVerificationConsumer(t, func() (bool, error) {
+		return ConsumeVerificationCodeWithKey("consume@example.com", "123456", PasswordResetPurpose)
+	})
+}
+
+func TestConsumeVerificationCodeDoesNotDeleteReplacement(t *testing.T) {
+	useVerificationTestState(t)
+	server := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: server.Addr()})
+	t.Cleanup(func() { _ = client.Close() })
+	RedisEnabled = true
+	RDB = client
+
+	require.NoError(t, RegisterVerificationCodeWithKey("replace@example.com", "old-code", EmailVerificationPurpose))
+	require.NoError(t, RegisterVerificationCodeWithKey("replace@example.com", "new-code", EmailVerificationPurpose))
+	valid, err := ConsumeVerificationCodeWithKey("replace@example.com", "old-code", EmailVerificationPurpose)
+	require.NoError(t, err)
+	assert.False(t, valid)
+	valid, err = ConsumeVerificationCodeWithKey("replace@example.com", "new-code", EmailVerificationPurpose)
+	require.NoError(t, err)
+	assert.True(t, valid)
+}
+
+func TestRestoreVerificationCodeDoesNotOverwriteReplacement(t *testing.T) {
+	useVerificationTestState(t)
+	server := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: server.Addr()})
+	t.Cleanup(func() { _ = client.Close() })
+	RedisEnabled = true
+	RDB = client
+
+	require.NoError(t, RegisterVerificationCodeWithKey("restore@example.com", "old-code", PasswordResetPurpose))
+	valid, remaining, err := ConsumeVerificationCodeWithTTL("restore@example.com", "old-code", PasswordResetPurpose)
+	require.NoError(t, err)
+	require.True(t, valid)
+	require.NoError(t, RestoreVerificationCodeIfAbsent("restore@example.com", "old-code", PasswordResetPurpose, remaining))
+	valid, err = VerifyCodeWithKey("restore@example.com", "old-code", PasswordResetPurpose)
+	require.NoError(t, err)
+	assert.True(t, valid)
+
+	require.NoError(t, RegisterVerificationCodeWithKey("restore@example.com", "new-code", PasswordResetPurpose))
+	require.NoError(t, RestoreVerificationCodeIfAbsent("restore@example.com", "old-code", PasswordResetPurpose, remaining))
+	valid, err = VerifyCodeWithKey("restore@example.com", "new-code", PasswordResetPurpose)
+	require.NoError(t, err)
+	assert.True(t, valid)
+	valid, err = VerifyCodeWithKey("restore@example.com", "old-code", PasswordResetPurpose)
+	require.NoError(t, err)
+	assert.False(t, valid)
+}
+
+func TestRestoreVerificationCodePreservesRemainingTTL(t *testing.T) {
+	useVerificationTestState(t)
+	server := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: server.Addr()})
+	t.Cleanup(func() { _ = client.Close() })
+	RedisEnabled = true
+	RDB = client
+	VerificationValidMinutes = 10
+
+	require.NoError(t, RegisterVerificationCodeWithKey("ttl@example.com", "reset-code", PasswordResetPurpose))
+	server.FastForward(9 * time.Minute)
+	valid, remaining, err := ConsumeVerificationCodeWithTTL("ttl@example.com", "reset-code", PasswordResetPurpose)
+	require.NoError(t, err)
+	require.True(t, valid)
+	assert.InDelta(t, time.Minute.Milliseconds(), remaining.Milliseconds(), float64(time.Second.Milliseconds()))
+	require.NoError(t, RestoreVerificationCodeIfAbsent("ttl@example.com", "reset-code", PasswordResetPurpose, remaining))
+	server.FastForward(61 * time.Second)
+	valid, err = VerifyCodeWithKey("ttl@example.com", "reset-code", PasswordResetPurpose)
+	require.NoError(t, err)
+	assert.False(t, valid)
+}
+
+func assertExactlyOneVerificationConsumer(t *testing.T, consume func() (bool, error)) {
+	t.Helper()
+	const attempts = 8
+	results := make(chan bool, attempts)
+	errors := make(chan error, attempts)
+	var wg sync.WaitGroup
+	for range attempts {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			valid, err := consume()
+			results <- valid
+			errors <- err
+		}()
+	}
+	wg.Wait()
+	close(results)
+	close(errors)
+
+	consumed := 0
+	for err := range errors {
+		require.NoError(t, err)
+	}
+	for valid := range results {
+		if valid {
+			consumed++
+		}
+	}
+	assert.Equal(t, 1, consumed)
+}
+
+func TestVerificationCodeDoesNotSilentlyFallBackWhenRedisFails(t *testing.T) {
+	useVerificationTestState(t)
+	server := miniredis.RunT(t)
+	client := redis.NewClient(&redis.Options{Addr: server.Addr()})
+	t.Cleanup(func() { _ = client.Close() })
+	RedisEnabled = true
+	RDB = client
+	server.Close()
+
+	err := RegisterVerificationCodeWithKey("user@example.com", "123456", EmailVerificationPurpose)
+	require.Error(t, err)
+
+	_, err = VerifyCodeWithKey("user@example.com", "123456", EmailVerificationPurpose)
+	require.Error(t, err)
+}
+
+func TestVerificationStorageKeyDoesNotExposeEmail(t *testing.T) {
+	storageKey := verificationStorageKey("private@example.com", EmailVerificationPurpose)
+	assert.NotContains(t, storageKey, "private@example.com")
+	assert.Contains(t, storageKey, verificationRedisPrefix+EmailVerificationPurpose+":")
+}

controller/misc.go

@@ -284,7 +284,11 @@ func SendEmailVerification(c *gin.Context) {
 		return
 	}
 	code := common.GenerateVerificationCode(6)
-	common.RegisterVerificationCodeWithKey(email, code, common.EmailVerificationPurpose)
+	if err := common.RegisterVerificationCodeWithKey(email, code, common.EmailVerificationPurpose); err != nil {
+		logger.LogError(c.Request.Context(), fmt.Sprintf("failed to store email verification code for %s: %s", email, err.Error()))
+		common.ApiErrorI18n(c, i18n.MsgRetryLater)
+		return
+	}
 	subject := fmt.Sprintf("%s邮箱验证邮件", common.SystemName)
 	content := fmt.Sprintf("<p>您好，你正在进行%s邮箱验证。</p>"+
 		"<p>您的验证码为: <strong>%s</strong></p>"+
@@ -309,7 +313,11 @@ func SendPasswordResetEmail(c *gin.Context) {
 	}
 	if _, err := model.GetUniqueUserByEmail(email); err == nil {
 		code := common.GenerateVerificationCode(0)
-		common.RegisterVerificationCodeWithKey(email, code, common.PasswordResetPurpose)
+		if err := common.RegisterVerificationCodeWithKey(email, code, common.PasswordResetPurpose); err != nil {
+			logger.LogError(c.Request.Context(), fmt.Sprintf("failed to store password reset code for %s: %s", email, err.Error()))
+			c.JSON(http.StatusOK, gin.H{"success": true, "message": ""})
+			return
+		}
 		link := fmt.Sprintf("%s/user/reset?email=%s&token=%s", system_setting.ServerAddress, email, code)
 		subject := fmt.Sprintf("%s密码重置", common.SystemName)
 		content := fmt.Sprintf("<p>您好，你正在进行%s密码重置。</p>"+
@@ -346,13 +354,22 @@ func ResetPassword(c *gin.Context) {
 		common.ApiErrorI18n(c, i18n.MsgInvalidParams)
 		return
 	}
-	if !common.VerifyCodeWithKey(req.Email, req.Token, common.PasswordResetPurpose) {
+	valid, remaining, err := common.ConsumeVerificationCodeWithTTL(req.Email, req.Token, common.PasswordResetPurpose)
+	if err != nil {
+		logger.LogError(c.Request.Context(), fmt.Sprintf("failed to consume password reset code for %s: %s", req.Email, err.Error()))
+		common.ApiErrorI18n(c, i18n.MsgRetryLater)
+		return
+	}
+	if !valid {
 		common.ApiErrorI18n(c, i18n.MsgUserPasswordResetLinkInvalid)
 		return
 	}
 	password := common.GenerateVerificationCode(12)
 	err = model.ResetUserPasswordByEmail(req.Email, password)
 	if err != nil {
+		if restoreErr := common.RestoreVerificationCodeIfAbsent(req.Email, req.Token, common.PasswordResetPurpose, remaining); restoreErr != nil {
+			logger.LogError(c.Request.Context(), fmt.Sprintf("failed to restore password reset code for %s after reset failure: %s", req.Email, restoreErr.Error()))
+		}
 		if errors.Is(err, model.ErrEmailNotFound) || errors.Is(err, model.ErrEmailAmbiguous) {
 			common.ApiErrorI18n(c, i18n.MsgUserPasswordResetLinkInvalid)
 			return
@@ -360,7 +377,6 @@ func ResetPassword(c *gin.Context) {
 		common.ApiError(c, err)
 		return
 	}
-	common.DeleteKey(req.Email, common.PasswordResetPurpose)
 	c.JSON(http.StatusOK, gin.H{
 		"success": true,
 		"message": "",

controller/user.go

@@ -263,7 +263,13 @@ func Register(c *gin.Context) {
 			common.ApiErrorI18n(c, i18n.MsgUserEmailVerificationRequired)
 			return
 		}
-		if !common.VerifyCodeWithKey(user.Email, user.VerificationCode, common.EmailVerificationPurpose) {
+		valid, err := common.ConsumeVerificationCodeWithKey(user.Email, user.VerificationCode, common.EmailVerificationPurpose)
+		if err != nil {
+			logger.LogError(c.Request.Context(), fmt.Sprintf("failed to consume registration verification code for %s: %s", user.Email, err.Error()))
+			common.ApiErrorI18n(c, i18n.MsgRetryLater)
+			return
+		}
+		if !valid {
 			common.ApiErrorI18n(c, i18n.MsgUserVerificationCodeError)
 			return
 		}
@@ -345,7 +351,6 @@ func Register(c *gin.Context) {
 			return
 		}
 	}
-
 	c.JSON(http.StatusOK, gin.H{
 		"success": true,
 		"message": "",
@@ -1313,11 +1318,6 @@ func EmailBind(c *gin.Context) {
 	}
 	email := req.Email
 	email = model.NormalizeEmail(email)
-	code := req.Code
-	if !common.VerifyCodeWithKey(email, code, common.EmailVerificationPurpose) {
-		common.ApiErrorI18n(c, i18n.MsgUserVerificationCodeError)
-		return
-	}
 	user := model.User{
 		Id: c.GetInt("id"),
 	}
@@ -1325,7 +1325,18 @@ func EmailBind(c *gin.Context) {
 		c.JSON(http.StatusUnauthorized, gin.H{"success": false, "message": "not authenticated"})
 		return
 	}
-	err := user.FillUserById()
+	code := req.Code
+	valid, err := common.ConsumeVerificationCodeWithKey(email, code, common.EmailVerificationPurpose)
+	if err != nil {
+		logger.LogError(c.Request.Context(), fmt.Sprintf("failed to consume email binding verification code for %s: %s", email, err.Error()))
+		common.ApiErrorI18n(c, i18n.MsgRetryLater)
+		return
+	}
+	if !valid {
+		common.ApiErrorI18n(c, i18n.MsgUserVerificationCodeError)
+		return
+	}
+	err = user.FillUserById()
 	if err != nil {
 		common.ApiError(c, err)
 		return