fix panic in rate limiter and missing error in MFA

Signed-off-by: Ronni Skansing <rskansing@gmail.com>

backend/middleware/ratelimiter.go

@@ -3,12 +3,21 @@ package middleware
 import (
 	"net/http"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/gin-gonic/gin"
 	"golang.org/x/time/rate"
 )
 
+// limiterEntry holds a rate limiter and the last time it was accessed as
+// unix nanoseconds. lastAccess is accessed atomically to avoid data races
+// between GetLimiter (writer) and cleanup (reader) under concurrent requests.
+type limiterEntry struct {
+	limiter    *rate.Limiter
+	lastAccess atomic.Int64
+}
+
 // NewIPRateLimiterMiddleware creates a middleware that limits the number of requests per IP
 // limit is the number of requests per second
 // burst is the maximum burst size, the maximum number of requests that can be made in a burst without being limited
@@ -24,23 +33,20 @@ func NewIPRateLimiterMiddleware(limit float64, burst int) gin.HandlerFunc {
 	}
 }
 
-//const cleanupInterval = 1 * time.Minute
-//const entryExpiration = 10 * time.Minute
-
 // KeyRateLimiter is a rate limiter for key such as username, email or IP
 type KeyRateLimiter struct {
-	// ips is a map of key to rate limit
+	// key is a map of key to limiterEntry
 	key sync.Map
-	// limiter is the rate limit, e.g. 1 request per seconds
+	// limiter is the rate limit, e.g. 1 request per second
 	limiter rate.Limit
 	// burst is the maximum burst size, the maximum number of requests that can be made in a burst without being limited
 	burst int
-	// cleanupInterval is the interval at which the expired keys are cleaned up
+	// cleanupInterval is the interval at which idle entries are evicted
 	cleanupInterval time.Duration
 }
 
 // NewKeyRateLimiter creates a new key rate limiter
-// limiter is the rate limit, e.g. 1 request per seconds
+// limiter is the rate limit, e.g. 1 request per second
 // burst is the maximum burst size, the maximum number of requests that can be made in a burst without being limited
 func NewKeyRateLimiter(
 	limiter rate.Limit,
@@ -48,21 +54,27 @@ func NewKeyRateLimiter(
 	cleanupInterval time.Duration,
 ) *KeyRateLimiter {
 	rl := &KeyRateLimiter{
-		limiter: limiter,
-		burst:   burst,
+		limiter:         limiter,
+		burst:           burst,
+		cleanupInterval: cleanupInterval,
 	}
 	go rl.cleanup()
 	return rl
 }
 
-// cleanup cleans up the expired keys, this is to avoid
-// memory leaking through the sync.Map when the key is not used anymore
+// cleanup evicts entries that have not been accessed within the cleanup interval,
+// preventing unbounded memory growth from the sync.Map
 func (r *KeyRateLimiter) cleanup() {
 	for range time.Tick(r.cleanupInterval) {
-		now := time.Now()
+		threshold := time.Now().Add(-r.cleanupInterval).UnixNano()
 		r.key.Range(func(key, value interface{}) bool {
-			expirationTime := value.(time.Time)
-			if now.After(expirationTime) {
+			entry, ok := value.(*limiterEntry)
+			if !ok {
+				// remove any entry with an unexpected type
+				r.key.Delete(key)
+				return true
+			}
+			if entry.lastAccess.Load() < threshold {
 				r.key.Delete(key)
 			}
 			return true
@@ -70,14 +82,22 @@ func (r *KeyRateLimiter) cleanup() {
 	}
 }
 
-// GetLimiter gets the limiter for an key or creates one if it does not exist
+// GetLimiter gets the limiter for a key or creates one if it does not exist
 func (r *KeyRateLimiter) GetLimiter(key string) *rate.Limiter {
-	value, exists := r.key.Load(key)
-	if exists {
-		return value.(*rate.Limiter)
+	entry := &limiterEntry{
+		limiter: rate.NewLimiter(r.limiter, r.burst),
+	}
+	entry.lastAccess.Store(time.Now().UnixNano())
+
+	// LoadOrStore atomically either stores our new entry or returns the
+	// existing one — correctly handles both the common case and concurrent
+	// goroutines racing to create an entry for the same key
+	actual, loaded := r.key.LoadOrStore(key, entry)
+	if loaded {
+		existing := actual.(*limiterEntry)
+		existing.lastAccess.Store(time.Now().UnixNano())
+		return existing.limiter
 	}
 
-	limiter := rate.NewLimiter(r.limiter, r.burst)
-	r.key.Store(key, limiter)
-	return limiter
+	return entry.limiter
 }

backend/service/user.go

@@ -1101,17 +1101,17 @@ func (u *User) CheckMFARecoveryCode(
 	userID *uuid.UUID,
 	recoveryCode *vo.String64,
 ) (bool, error) {
-	dbRecoveryCodeHash, err := u.UserRepository.GetMFARecoveryCode(
+	dbRecoveryCode, err := u.UserRepository.GetMFARecoveryCode(
 		ctx,
 		userID,
 	)
-	if subtle.ConstantTimeCompare([]byte(recoveryCode.String()), []byte(dbRecoveryCodeHash)) != 1 {
-		u.Logger.Info("invalid recovery code")
-		return false, errs.ErrUserWrongRecoveryCode
-	}
 	if err != nil {
 		u.Logger.Errorw("failed to get recovery code", "error", err)
 		return false, errs.Wrap(err)
 	}
+	if subtle.ConstantTimeCompare([]byte(recoveryCode.String()), []byte(dbRecoveryCode)) != 1 {
+		u.Logger.Info("invalid recovery code")
+		return false, errs.ErrUserWrongRecoveryCode
+	}
 	return true, nil
 }