[API Policies] Advanced Rate Limit Policy Issues #689
Description
Overview
This document outlines critical security and reliability issues found in the Advanced Rate Limit Policy implementation affecting Redis operations, GCRA algorithms, and thread safety.
Issues
1. Race Condition in Fixed Window Redis Operations
File: algorithms/fixedwindow/redis.go (lines 92-113)
Description:
The INCRBY and EXPIRE operations are not atomic. If INCRBY succeeds but EXPIRE fails, the key persists without a TTL, causing permanent rate limiting for that key.
Proposed Fix:
Use a Lua script to make both operations atomic:
local key = KEYS[1]
local ttl = ARGV[1]
local increment = ARGV[2]
local value = redis.call('INCRBY', key, increment)
redis.call('EXPIRE', key, ttl)
return valueImpact: Keys without TTL cause incorrect rate limiting behavior indefinitely.
2. Division by Zero in GCRA Policy
File: algorithms/gcra/policy.go (lines 47-51)
Description:
The EmissionInterval() function will panic with division by zero if p.Limit is 0.
Proposed Fix:
Add validation in NewPolicy() to ensure Limit > 0, or guard the division.
Impact: Panic when rate limit is configured with zero limit.
3. Unchecked Type Assertions in GCRA Redis
File: algorithms/gcra/redis.go (lines 117-136)
Description:
Type assertions are performed without checking, which will panic if the Lua script returns unexpected data types or fewer elements.
Proposed Fix:
Add defensive type checking before assertions, with proper error handling.
Impact: Unexpected Redis responses cause application panics.
4. Thread-Safety Issue in GCRA Memory
File: algorithms/gcra/memory.go (lines 66-70)
Description:
The clock field can be modified via WithClock while AllowN or cleanupLoop are reading it, creating a data race.
Proposed Fix:
Document that WithClock must be called before any operations, or protect the clock field with the existing mutex.
Impact: Concurrent access leads to undefined behavior and potential crashes.
5. Unassigned API Metadata Fields
File: ratelimit.go (lines 97-100)
Description:
Variables apiId, apiName, and apiVersion are declared but never assigned. They remain empty and are passed to getBaseCacheKey(), leaving cache keys and stored metadata incomplete.
Proposed Fix:
Populate these fields from appropriate sources (metadata, params, context) or remove if not needed.
Impact: Cache keys are incomplete, potentially causing incorrect rate limiting isolation.
6. Race Conditions in Test Clock Access
File: Test files for both fixedwindow and gcra algorithms
Description:
Data races detected in TestMemoryLimiter_CleanupExpired tests. Concurrent access to FixedClock without proper synchronization:
- Test goroutine writes to the clock
- Cleanup goroutine reads from the clock
Proposed Fix:
Add proper synchronization to the test clock implementation, or ensure clock is not modified after limiter initialization in tests.
Impact: Race detector failures in tests, potential undefined behavior in concurrent scenarios.