Mark/msp_c2
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Three overlapping rate limiters with inconsistent 429 response shapes #45

New issue
Closed
opened 2026-08-11 02:49:35 +00:00 by Mark · 3 comments
Mark commented 2026-08-11 02:49:35 +00:00
Owner
Copy link

core/rate_limiter.py (RateLimiter class + rate_limit_middleware), core/middleware.py (_is_login_rate_limit_exceeded, hardcoded 10/60s login limit), and core/security_middleware.py (RateLimitMiddleware, production only) all rate-limit the same endpoints independently. They return different bodies: rate_limit_middleware sends {"detail": "Rate limit exceeded", "retry_after": N} with X-RateLimit headers, middleware.py sends {"detail": "Too many requests"}. Attackers get the strictest limit (5 sensitive logins) while legitimate users hit whichever fires first, and the duplicated sliding-window state is hard to reason about. Consolidate on core/rate_limiter.py and delete the other two.

core/rate_limiter.py (RateLimiter class + rate_limit_middleware), core/middleware.py (_is_login_rate_limit_exceeded, hardcoded 10/60s login limit), and core/security_middleware.py (RateLimitMiddleware, production only) all rate-limit the same endpoints independently. They return different bodies: rate_limit_middleware sends {"detail": "Rate limit exceeded", "retry_after": N} with X-RateLimit headers, middleware.py sends {"detail": "Too many requests"}. Attackers get the strictest limit (5 sensitive logins) while legitimate users hit whichever fires first, and the duplicated sliding-window state is hard to reason about. Consolidate on core/rate_limiter.py and delete the other two.
Mark 2026-08-14 00:45:36 +00:00
Mark commented 2026-08-14 07:04:50 +00:00
Author
Owner
Copy link

Design spec — Unified Rate Limiter Interface (kanban t_aa478233, audit at origin/main 69e5b4b2). No code changes in this card; this spec drives the implementation card.

Unified Rate Limiter Interface — Design Spec (Mark/msp_c2#45)

Status: spec (no code changes). Source audit at origin/main 69e5b4b2.

1. Current state (audit of the three implementations)

File Component Limits 429 body 429 headers
core/rate_limiter.py RateLimiter + rate_limit_middleware PER_USER 100/60s, PER_IP 20/60s, SENSITIVE 5/60s (env-driven via core/config.py) {"detail": "Rate limit exceeded", "retry_after": N} X-RateLimit-Limit, X-RateLimit-Remaining(0), X-RateLimit-Reset, X-Request-Id — no Retry-After
core/middleware.py _is_login_rate_limit_exceeded inside request_context_middleware login POST /api/v1/auth/login: 10/60s per-IP (1000 in test) — hardcoded {"detail": "Too many requests"} X-Request-Id only
core/security_middleware.py RateLimitMiddleware (registered only when ENVIRONMENT == "production") per-IP 100/60s — hardcoded at registration plain text "Rate limit exceeded. Please try again later." Retry-After only

Registration order in backend/main.py: CORS → register_middleware (request-id/logging + login rate-limit branch) → app.middleware("http")(rate_limit_middleware) → GZip → setup_security_middleware (prod: SecurityHeaders + RateLimit + HTTPSRedirect).

Findings:

  • The login endpoint is rate-limited twice: middleware.py 10/60s and rate_limiter.py SENSITIVE 5/60s. The 5/min scope fires first in practice, so attackers see 5/min while the 10/60 path is a redundant second net with independent sliding-window state.
  • rate_limit_middleware computes retry_after for the body but never emits the Retry-After header — must be added.
  • RateLimitMiddleware returns a plain-text body and no X-RateLimit-* headers, inconsistent with both other emitters.
  • .env.production carries RATE_LIMIT_REQUESTS=100 / RATE_LIMIT_WINDOW=60, which nothing reads (core/config.py uses the per-scope RATE_LIMIT_*_REQUESTS / RATE_LIMIT_*_WINDOW names). Stale env, cleanup candidate.
  • No in-repo consumer reads the 429 body: frontend/src has zero references to "Too many requests" / retry_after / X-RateLimit. The string exists only in backend/core/middleware.py (RATE_LIMIT_DETAIL).
  • backend/tests/test_rate_limiting.py already asserts the canonical body ("Rate limit exceeded" + retry_after) at the 6th login request — that assertion is satisfied by the SENSITIVE scope, not by middleware.py (whose test limit is 1000). conftest.py resets rate_limiter.rate_limiter._windows between tests — that fixture stays valid.

2. Target design

2.1 Single canonical entry point

core/rate_limiter.py (RateLimiter class + rate_limit_middleware) is the sole rate-limiting mechanism. main.py keeps app.middleware("http")(rate_limit_middleware). The login branch in core/middleware.py and RateLimitMiddleware in core/security_middleware.py are deleted.

2.2 Unified 429 response (every rate-limit rejection)

  • Status: 429
  • Body: {"detail": "Rate limit exceeded", "retry_after": <int seconds>} — the "Too many requests" and plain-text variants are dropped.
  • Headers on every 429:
    • Retry-After: <int seconds> (same value as body retry_after; missing today — add to rate_limit_middleware)
    • X-RateLimit-Limit: <limit>
    • X-RateLimit-Remaining: 0
    • X-RateLimit-Reset: <unix epoch seconds>
    • X-Request-Id (existing behavior, keep)
  • Non-429 responses keep the X-RateLimit-* headers (existing behavior in rate_limit_middleware).

2.3 Data-driven limits table (endpoint family → (count, window))

Replace every hardcoded limit with one table in core/config.py, env-overridable, keyed by endpoint family. Suggested shape:

RATE_LIMITS = {
  "sensitive-login": {"count": 5,   "window": 60, "scope": "ip"},    # POST /api/v1/auth/login
  "general-auth":    {"count": 100, "window": 60, "scope": "user"},  # authenticated API
  "general-anon":    {"count": 20,  "window": 60, "scope": "ip"},    # unauthenticated API
}
  • Keep the existing per-scope env vars (RATE_LIMIT_PER_USER_REQUESTS, RATE_LIMIT_PER_IP_REQUESTS, RATE_LIMIT_SENSITIVE_REQUESTS, and *_WINDOW counterparts in core/config.py) as the value source so deployed envs keep working; the class maps family → (count, window, scope) instead of the current fixed enum→tuple dict.
  • The sensitive endpoint set must be data-driven too: today rate_limit_middleware hardcodes is_sensitive_endpoint = (POST, /api/v1/auth/login). Move it to a config list (e.g. RATE_LIMIT_SENSITIVE_PATHS) so adding sensitive endpoints is a config change.
  • Scope of "10/min general": the 10/60s figure in the issue body is the deleted middleware.py login limit. Under unification the login endpoint is governed by the sensitive-login scope at 5/min (matches issue text: "Attackers get the strictest limit (5 sensitive logins)"). The general scope defaults stay at the current data-driven values (100/min per-user, 20/min per-IP); moving any family to 10/min is a one-line env change, not code.

2.4 Deletion list

core/middleware.py:

  • RATE_LIMIT_DETAIL constant, _TEST_MODE/_LOGIN_RATE_LIMIT_MAX_REQUESTS/_LOGIN_RATE_LIMIT_WINDOW_SECONDS/_LOGIN_RATE_LIMIT_ROUTE constants
  • _rate_limit_windows deque
  • _get_client_ip (used only by the login rate-limit branch)
  • _is_login_rate_limit_exceeded()
  • the login rate-limit branch inside request_context_middleware (the if _is_login_rate_limit_exceeded(request): block)
  • now-dead imports: os, time, defaultdict, deque (and the inline HTTP_429_TOO_MANY_REQUESTS/JSONResponse imports used only by the branch)
  • KEEP: REQUEST_ID_HEADER, _resolve_request_id, _extract_user_context, request logging, both exception handlers, register_middleware

core/security_middleware.py:

  • RateLimitMiddleware class entirely
  • app.add_middleware(RateLimitMiddleware, max_requests=100, window=60) in setup_security_middleware
  • now-dead import: time
  • KEEP: SecurityHeadersMiddleware, HTTPSRedirectMiddleware, setup_security_middleware minus the RateLimit registration (the production-only gating for headers/HTTPS-redirect is unrelated to rate limiting and stays)

backend/main.py: no import/registration changes needed — rate_limit_middleware (canonical) stays registered, register_middleware and setup_security_middleware still exist.

2.5 Backward compatibility

  • Clients previously reading {"detail": "Too many requests"} (login 429) will now receive {"detail": "Rate limit exceeded", "retry_after": N} — same 429 status, different body.
  • No in-repo client parses the string (verified: zero matches in frontend/src). External API consumers should key on status_code == 429 and the Retry-After header, not the detail string.
  • test_rate_limiting.py already asserts the new body and passes via the SENSITIVE scope; conftest.py's window-reset fixture still targets the canonical limiter. No test changes required.

2.6 Verification gate (for the implementer)

  1. git grep -nE "_is_login_rate_limit_exceeded|RateLimitMiddleware|Too many requests" → no hits in backend/core/
  2. git grep -n "Rate limit exceeded" → exactly one emitter, backend/core/rate_limiter.py
  3. Live 429 (login, 6th request) returns body {"detail": "Rate limit exceeded", "retry_after": N} plus Retry-After, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset
  4. Sensitive login scope still 5/min (test asserts request 6 → 429)
  5. pytest backend/tests passes (rate-limiter + security suites unaffected otherwise)
**Design spec — Unified Rate Limiter Interface (kanban t_aa478233, audit at origin/main 69e5b4b2). No code changes in this card; this spec drives the implementation card.** # Unified Rate Limiter Interface — Design Spec (Mark/msp_c2#45) Status: spec (no code changes). Source audit at origin/main 69e5b4b2. ## 1. Current state (audit of the three implementations) | File | Component | Limits | 429 body | 429 headers | |---|---|---|---|---| | `core/rate_limiter.py` | `RateLimiter` + `rate_limit_middleware` | PER_USER 100/60s, PER_IP 20/60s, SENSITIVE 5/60s (env-driven via `core/config.py`) | `{"detail": "Rate limit exceeded", "retry_after": N}` | X-RateLimit-Limit, X-RateLimit-Remaining(0), X-RateLimit-Reset, X-Request-Id — **no Retry-After** | | `core/middleware.py` | `_is_login_rate_limit_exceeded` inside `request_context_middleware` | login `POST /api/v1/auth/login`: 10/60s per-IP (1000 in test) — **hardcoded** | `{"detail": "Too many requests"}` | X-Request-Id only | | `core/security_middleware.py` | `RateLimitMiddleware` (registered only when `ENVIRONMENT == "production"`) | per-IP 100/60s — hardcoded at registration | plain text `"Rate limit exceeded. Please try again later."` | Retry-After only | Registration order in `backend/main.py`: CORS → `register_middleware` (request-id/logging + login rate-limit branch) → `app.middleware("http")(rate_limit_middleware)` → GZip → `setup_security_middleware` (prod: SecurityHeaders + **RateLimit** + HTTPSRedirect). Findings: - The login endpoint is rate-limited twice: `middleware.py` 10/60s and `rate_limiter.py` SENSITIVE 5/60s. The 5/min scope fires first in practice, so attackers see 5/min while the 10/60 path is a redundant second net with independent sliding-window state. - `rate_limit_middleware` computes `retry_after` for the body but never emits the `Retry-After` header — must be added. - `RateLimitMiddleware` returns a plain-text body and no X-RateLimit-* headers, inconsistent with both other emitters. - `.env.production` carries `RATE_LIMIT_REQUESTS=100` / `RATE_LIMIT_WINDOW=60`, which nothing reads (`core/config.py` uses the per-scope `RATE_LIMIT_*_REQUESTS` / `RATE_LIMIT_*_WINDOW` names). Stale env, cleanup candidate. - No in-repo consumer reads the 429 body: `frontend/src` has zero references to "Too many requests" / `retry_after` / X-RateLimit. The string exists only in `backend/core/middleware.py` (RATE_LIMIT_DETAIL). - `backend/tests/test_rate_limiting.py` already asserts the canonical body (`"Rate limit exceeded"` + `retry_after`) at the 6th login request — that assertion is satisfied by the SENSITIVE scope, not by `middleware.py` (whose test limit is 1000). `conftest.py` resets `rate_limiter.rate_limiter._windows` between tests — that fixture stays valid. ## 2. Target design ### 2.1 Single canonical entry point `core/rate_limiter.py` (`RateLimiter` class + `rate_limit_middleware`) is the sole rate-limiting mechanism. `main.py` keeps `app.middleware("http")(rate_limit_middleware)`. The login branch in `core/middleware.py` and `RateLimitMiddleware` in `core/security_middleware.py` are deleted. ### 2.2 Unified 429 response (every rate-limit rejection) - Status: `429` - Body: `{"detail": "Rate limit exceeded", "retry_after": <int seconds>}` — the "Too many requests" and plain-text variants are dropped. - Headers on every 429: - `Retry-After: <int seconds>` (same value as body `retry_after`; **missing today — add to `rate_limit_middleware`**) - `X-RateLimit-Limit: <limit>` - `X-RateLimit-Remaining: 0` - `X-RateLimit-Reset: <unix epoch seconds>` - `X-Request-Id` (existing behavior, keep) - Non-429 responses keep the `X-RateLimit-*` headers (existing behavior in `rate_limit_middleware`). ### 2.3 Data-driven limits table (endpoint family → (count, window)) Replace every hardcoded limit with one table in `core/config.py`, env-overridable, keyed by endpoint family. Suggested shape: ``` RATE_LIMITS = { "sensitive-login": {"count": 5, "window": 60, "scope": "ip"}, # POST /api/v1/auth/login "general-auth": {"count": 100, "window": 60, "scope": "user"}, # authenticated API "general-anon": {"count": 20, "window": 60, "scope": "ip"}, # unauthenticated API } ``` - Keep the existing per-scope env vars (`RATE_LIMIT_PER_USER_REQUESTS`, `RATE_LIMIT_PER_IP_REQUESTS`, `RATE_LIMIT_SENSITIVE_REQUESTS`, and `*_WINDOW` counterparts in `core/config.py`) as the value source so deployed envs keep working; the class maps family → (count, window, scope) instead of the current fixed enum→tuple dict. - The sensitive endpoint set must be data-driven too: today `rate_limit_middleware` hardcodes `is_sensitive_endpoint = (POST, /api/v1/auth/login)`. Move it to a config list (e.g. `RATE_LIMIT_SENSITIVE_PATHS`) so adding sensitive endpoints is a config change. - Scope of "10/min general": the 10/60s figure in the issue body is the deleted `middleware.py` login limit. Under unification the login endpoint is governed by the sensitive-login scope at **5/min** (matches issue text: "Attackers get the strictest limit (5 sensitive logins)"). The general scope defaults stay at the current data-driven values (100/min per-user, 20/min per-IP); moving any family to 10/min is a one-line env change, not code. ### 2.4 Deletion list `core/middleware.py`: - `RATE_LIMIT_DETAIL` constant, `_TEST_MODE`/`_LOGIN_RATE_LIMIT_MAX_REQUESTS`/`_LOGIN_RATE_LIMIT_WINDOW_SECONDS`/`_LOGIN_RATE_LIMIT_ROUTE` constants - `_rate_limit_windows` deque - `_get_client_ip` (used only by the login rate-limit branch) - `_is_login_rate_limit_exceeded()` - the login rate-limit branch inside `request_context_middleware` (the `if _is_login_rate_limit_exceeded(request):` block) - now-dead imports: `os`, `time`, `defaultdict`, `deque` (and the inline `HTTP_429_TOO_MANY_REQUESTS`/`JSONResponse` imports used only by the branch) - KEEP: `REQUEST_ID_HEADER`, `_resolve_request_id`, `_extract_user_context`, request logging, both exception handlers, `register_middleware` `core/security_middleware.py`: - `RateLimitMiddleware` class entirely - `app.add_middleware(RateLimitMiddleware, max_requests=100, window=60)` in `setup_security_middleware` - now-dead import: `time` - KEEP: `SecurityHeadersMiddleware`, `HTTPSRedirectMiddleware`, `setup_security_middleware` minus the RateLimit registration (the production-only gating for headers/HTTPS-redirect is unrelated to rate limiting and stays) `backend/main.py`: no import/registration changes needed — `rate_limit_middleware` (canonical) stays registered, `register_middleware` and `setup_security_middleware` still exist. ### 2.5 Backward compatibility - Clients previously reading `{"detail": "Too many requests"}` (login 429) will now receive `{"detail": "Rate limit exceeded", "retry_after": N}` — same 429 status, different body. - No in-repo client parses the string (verified: zero matches in `frontend/src`). External API consumers should key on `status_code == 429` and the `Retry-After` header, not the detail string. - `test_rate_limiting.py` already asserts the new body and passes via the SENSITIVE scope; `conftest.py`'s window-reset fixture still targets the canonical limiter. No test changes required. ### 2.6 Verification gate (for the implementer) 1. `git grep -nE "_is_login_rate_limit_exceeded|RateLimitMiddleware|Too many requests"` → no hits in `backend/core/` 2. `git grep -n "Rate limit exceeded"` → exactly one emitter, `backend/core/rate_limiter.py` 3. Live 429 (login, 6th request) returns body `{"detail": "Rate limit exceeded", "retry_after": N}` plus `Retry-After`, `X-RateLimit-Limit`, `X-RateLimit-Remaining`, `X-RateLimit-Reset` 4. Sensitive login scope still 5/min (test asserts request 6 → 429) 5. `pytest backend/tests` passes (rate-limiter + security suites unaffected otherwise)
Mark commented 2026-08-14 07:18:59 +00:00
Author
Owner
Copy link

Fix landed at 8199e11 on main (2026-08-14) per the unified-interface spec (comment 52184). Deleted middleware.py login rate-limit branch (_is_login_rate_limit_exceeded, RATE_LIMIT_DETAIL, _get_client_ip, _rate_limit_windows) and security_middleware.py RateLimitMiddleware; rate limiting is now solely core/rate_limiter.py RateLimiter + rate_limit_middleware. Limits table data-driven via core/config.py RATE_LIMITS (endpoint family -> count/window/scope); sensitive-login 5/60s; RATE_LIMIT_SENSITIVE_PATHS configurable. All 429s use canonical body {"detail":"Rate limit exceeded","retry_after":} plus Retry-After and X-RateLimit-Limit/Remaining/Reset. Verification: pytest backend/tests 217 passed; 6th login -> 429 with full canonical headers; git grep for deleted symbols: no hits in backend/core/. This commit supersedes the earlier speedrun closeout (no code).

Fix landed at 8199e11 on main (2026-08-14) per the unified-interface spec (comment 52184). Deleted middleware.py login rate-limit branch (_is_login_rate_limit_exceeded, RATE_LIMIT_DETAIL, _get_client_ip, _rate_limit_windows) and security_middleware.py RateLimitMiddleware; rate limiting is now solely core/rate_limiter.py RateLimiter + rate_limit_middleware. Limits table data-driven via core/config.py RATE_LIMITS (endpoint family -> count/window/scope); sensitive-login 5/60s; RATE_LIMIT_SENSITIVE_PATHS configurable. All 429s use canonical body {"detail":"Rate limit exceeded","retry_after":<int>} plus Retry-After and X-RateLimit-Limit/Remaining/Reset. Verification: pytest backend/tests 217 passed; 6th login -> 429 with full canonical headers; git grep for deleted symbols: no hits in backend/core/. This commit supersedes the earlier speedrun closeout (no code).
Mark commented 2026-08-14 07:29:48 +00:00
Author
Owner
Copy link

Regression tests for the unified rate limiter landed in commit f9aa117 (backend/tests/test_rate_limiter.py, 6 tests):

  • 6th sensitive-login attempt within 60s returns canonical 429 body {"detail":"Rate limit exceeded","retry_after":int} plus Retry-After/X-RateLimit-Limit/Remaining/Reset/X-Request-Id headers
  • general endpoint (general-anon family) exceeding its limit returns the same canonical shape
  • window elapse resets the counter (fake-clock, no freezegun dep)
  • independent sliding windows per family (login traffic does not exhaust general endpoint)
  • legacy "Too many requests" body never appears (runtime + source-scan guard)
    Verified: pytest tests/test_rate_limiter.py -q 6 passed; full backend suite 208 passed (15 deselected by CI markers). Note: repo Actions CI has no live runner (localhost-runner stopped, zsus dead) so CI runs cannot execute.
Regression tests for the unified rate limiter landed in commit f9aa117 (backend/tests/test_rate_limiter.py, 6 tests): - 6th sensitive-login attempt within 60s returns canonical 429 body {"detail":"Rate limit exceeded","retry_after":int} plus Retry-After/X-RateLimit-Limit/Remaining/Reset/X-Request-Id headers - general endpoint (general-anon family) exceeding its limit returns the same canonical shape - window elapse resets the counter (fake-clock, no freezegun dep) - independent sliding windows per family (login traffic does not exhaust general endpoint) - legacy "Too many requests" body never appears (runtime + source-scan guard) Verified: `pytest tests/test_rate_limiter.py -q` 6 passed; full backend suite 208 passed (15 deselected by CI markers). Note: repo Actions CI has no live runner (localhost-runner stopped, zsus dead) so CI runs cannot execute.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
No results found.
Labels
Clear labels   
blocked:gate-fetch

   
blocked:gate-fetch

   
blocked:gate-implement

   
blocked:gate-plan

   
blocked:needs-attention

   
blocked:needs-decision

   
blocked:needs-human

   
blocked:no-procedure

   
blocked:preflight

   
bug

Something is not working correctly

  
feature-request

   
in-progress

   
lint-cleanup

   
needs-decompose

Ready to be decomposed into sub-issues

  
needs-triage

   
ops-task

   
quickfix

   
security/medium

Medium severity security issue

  
triage-approved
No labels
blocked:gate-fetch
blocked:gate-fetch
blocked:gate-implement
blocked:gate-plan
blocked:needs-attention
blocked:needs-decision
blocked:needs-human
blocked:no-procedure
blocked:preflight
bug
feature-request
in-progress
lint-cleanup
needs-decompose
needs-triage
ops-task
quickfix
security/medium
triage-approved
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Mark/msp_c2#45
No description provided.