Skip to content

fix(rate_limit): eviction fires after new entry insertion, allowing attacker to escape rate-limit cap #243

Closed

Description

@piyushmakhija28

Summary

The _windows eviction in rate_limit.py fires after the new entry is already inserted into the dict. When eviction is triggered, the sorted key list includes the newly added entry, and the eviction routine may delete that new entry immediately — resetting the caller's counter to zero and allowing them to bypass the limit entirely.

Root Cause

backend/app/security/rate_limit.py around line 67:

self._windows[key] = window       # ← new entry inserted first
if len(self._windows) > self._MAX_WINDOW_ENTRIES:
    # sorted_keys includes the brand-new entry
    sorted_keys = sorted(self._windows, key=lambda k: self._windows[k].last_seen)
    for k in sorted_keys[: len(sorted_keys) // 2]:
        del self._windows[k]       # ← may delete the entry just added above

If the new entry's last_seen is the oldest (e.g., first-ever request from this credential during an eviction triggered by the 25,001st unique key), it lands at the front of sorted_keys and is evicted immediately, resetting the caller's rate-limit window to nothing.

Failure Scenario

  1. Attacker creates 25,000 unique credentials (or tenant keys), filling _MAX_WINDOW_ENTRIES.
  2. Attacker sends request [SECURITY] resolve_principal raises 401/503 from X-API-Key check before JWT fallback — valid Bearer tokens rejected when API_KEY_SALT unconfigured #25,001 using a fresh credential → entry is inserted → eviction fires → the new entry is the least-recently-seen → it is deleted.
  3. The attacker's counter is reset to 0. Next request starts a clean window.
  4. Repeat indefinitely: attacker can send unlimited requests at the rate of one per eviction cycle.

Proposed Fix

# Insert AFTER eviction, not before:
if len(self._windows) >= self._MAX_WINDOW_ENTRIES:
    sorted_keys = sorted(self._windows, key=lambda k: self._windows[k].last_seen)
    for k in sorted_keys[: len(sorted_keys) // 2]:
        del self._windows[k]

self._windows[key] = window   # ← insert after eviction

Also add a test that verifies the newly inserted key survives an eviction cycle:

def test_eviction_does_not_delete_new_entry():
    rl = RateLimiter(...)
    # fill to MAX_WINDOW_ENTRIES - 1 with dummy keys
    # trigger eviction by adding the MAX+1 key
    # assert the MAX+1 key still exists and its count is correct

Activity

  1. added
    bugSomething isn't working
    priority: criticalBlocks all requests or crashes startup
    securitySecurity or IDOR concern
    code-reviewFound via automated code review
    on Jun 21, 2026
  2. piyushmakhija28 commented on Jun 21, 2026

    @piyushmakhija28
    ContributorAuthor

    Fixed in PR #280 (Sprint R3/R4: fix(sprint-r3): root-cause fixes for all 39 bugs from code review round 3 + Sprint R4 round 4)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingcode-reviewFound via automated code reviewpriority: criticalBlocks all requests or crashes startupsecuritySecurity or IDOR concern

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions