🐛 bug: Issues with limiter middleware

[coldsmirk]

Description

I've identified three critical bugs in the sliding window rate limiter implementation (middleware/limiter/limiter_sliding.go). These bugs can cause incorrect rate limiting behavior, especially under high concurrency.

Bug 1: Race Condition in SkipFailedRequests/SkipSuccessfulRequests

Location: Lines 117-133

Problem: When SkipFailedRequests or SkipSuccessfulRequests is enabled, the code releases the lock after incrementing currHits, executes the handler, then re-acquires the lock and reads the entry again from storage. This creates a race condition where concurrent requests can interfere with each other's counts.

Code:

// First: increment and save
e.currHits++
manager.set(c, key, e, ...)
mux.Unlock()  // ⚠️ Lock released

err = c.Next()  // Handler execution (time-consuming)

// Second: re-acquire and decrement
mux.Lock()
entry, getErr := manager.get(c, key)  // ⚠️ Re-fetch from storage
e = entry
e.currHits--  // ⚠️ May decrement wrong value

Scenario:

1. Request A: currHits = 5 → 6, saves, unlocks, executes handler
2. 2. Request B: Concurrently enters, reads currHits = 6 → 7, saves
3. 3. Request A: Handler completes, needs to skip, re-fetches currHits = 7 → 6
4. 4. Result: Request B's increment is incorrectly canceled!

Bug 2: Inconsistent Variable Usage for Remaining Calculation

Location: Line 91

Problem: The code uses cfg.Max instead of maxRequests when calculating remaining, but maxRequests is the dynamically generated limit that should be used.

Code:

// Line 33: Dynamic max value
maxRequests := cfg.MaxFunc(c)  // e.g., returns 100 for VIP users

// Line 91: Uses wrong variable ❌
remaining := cfg.Max - rate  // Uses fixed cfg.Max instead of maxRequests

// Line 99: Rate limit check
if remaining < 0 {
    return cfg.LimitReached(c)
}

// Line 137: Response header uses correct variable
c.Set(xRateLimitLimit, strconv.Itoa(maxRequests))  // ✅
c.Set(xRateLimitRemaining, strconv.Itoa(remaining))  // ❌ Wrong value

Scenario:

• cfg.Max = 10 (default)
• • maxRequests = 100 (VIP user)
• • • rate = 50
• • • • remaining = 10 - 50 = -40 ❌ Should be 100 - 50 = 50
• • • • • Result: VIP users are incorrectly rate-limited!

Bug 3: Incorrect Remaining Recalculation in Skip Logic

Location: Line 130

Problem: When skipping a request, the code simply does remaining++, but this ignores the sliding window weight calculation. The correct remaining depends on both prevHits * weight and currHits.

Code:

e.currHits--
remaining++  // ⚠️ Oversimplified calculation

Correct calculation:

weight := float64(resetInSec) / float64(expiration)
rate := int(float64(e.prevHits)*weight) + e.currHits
remaining = maxRequests - rate

Impact

1. Race condition: Inaccurate request counting under high concurrency
2. 2. Variable inconsistency: Dynamic rate limits (e.g., per-user quotas) don't work correctly
3. 3. Incorrect headers: X-RateLimit-Remaining shows wrong values to clients
4. 4. Security risk: Attackers could exploit these bugs to bypass rate limiting

Proposed Fixes

Fix 1: Eliminate Race Condition

Don't re-fetch from storage when skipping; use the original entry reference:

originalEntry := e
manager.set(c, key, e, ...)
mux.Unlock()

err = c.Next()
statusCode := getEffectiveStatusCode(c, err)

if shouldSkip {
    mux.Lock()
    originalEntry.currHits--  // Use original reference
    // Recalculate rate and remaining properly
    weight := float64(resetInSec) / float64(expiration)
    rate := int(float64(originalEntry.prevHits)*weight) + originalEntry.currHits
    remaining = maxRequests - rate
    
    manager.set(c, key, originalEntry, ...)
    mux.Unlock()
}

Fix 2: Use Consistent Variable

// Line 91 should be:
remaining := maxRequests - rate  // Use maxRequests instead of cfg.Max

Fix 3: Proper Remaining Recalculation

Already covered in Fix 1 - recalculate using the full formula instead of remaining++.

Environment

• Fiber version: v3.0.0-rc.2
• • Go version: 1.25.3
• • • File: middleware/limiter/limiter_sliding.go

Additional Notes

These bugs affect the core correctness of rate limiting. I recommend:

1. Adding comprehensive concurrent test cases
2. 2. Reviewing all paths where mux.Lock()/Unlock() is used
3. 3. Ensuring consistency between cfg.Max and maxRequests throughout the codebase

[gaby]

@ilxqx Which AI model wrote this?

[coldsmirk]

@ilxqx Which AI model wrote this?

Haha, because human writing couldn't possibly be this thoughtful, right? This middleware does indeed have the above issues. At least the cfg.Max and maxRequests problem is an error I reviewed, because I found that in my project, regardless of what the MaxFunc function returns, it couldn't have any effect, since the middleware code ultimately always uses cfg.Max. So I took a quick look at the middleware code and indeed found this obvious error. Then, being a bit lazy, I had Claude 4.5 help me write the issue, and it actually spotted the concurrent race condition problem as well. This seems quite reasonable to me, but whether this problem actually exists still needs to be evaluated by you guys.

[gaby]

Analysis using OpenAI GPT5-Codex:

Evaluation:

• Bug 1 (race when skipping requests) – Not reproducible. The middleware serializes all mutations with mux.Lock()/Unlock() both before the handler runs and again inside the skip branch, so concurrent requests cannot interfere while the count is adjusted. Re-fetching the entry is necessary because manager.set releases pooled items for external storages, meaning the prior pointer is no longer valid.

• Bug 2 (ignoring MaxFunc) – Valid. The allowance is still computed with the static cfg.Max even though a per-request limit was obtained via maxRequests := cfg.MaxFunc(c), so dynamic limits are never honored.

• Bug 3 (incorrect remaining recomputation) – Not reproducible. Decrementing currHits reduces the rate by exactly one request in the sliding-window formula, so incrementing remaining keeps the values consistent with the earlier calculation.