Skip to content

fix(throttling): client-IP identity and atomic counting for per-IP limits - #1028

Open
nimish-ks wants to merge 3 commits into
feat--unify-rate-limitsfrom
feat--unify-rate-limits-per-ip
Open

nimish-ks wants to merge 3 commits into
feat--unify-rate-limitsfrom
feat--unify-rate-limits-per-ip

Conversation

@nimish-ks

Copy link
Copy Markdown
Member

Builds on #1019. Per-IP throttles now identify the client with get_client_ip(), the same resolution audit logs and network access policies use. DRF's default get_ident() keys on the raw X-Forwarded-For chain, so a proxy hop that changes between requests could spread one client across several buckets. IPv6 clients are now grouped per /64.

The sign-in throttles (password login, signup, email check, resend verification, MFA verify) move onto the atomic fixed-window counter from #1019, each with its own scope. Previously most of them shared DRF's anon scope, one budget across different rates, and DRF's timestamp list could admit concurrent requests past the limit. Email check and invite lookup share a scope by design.

Counters also outlive their window, so they can't lose their TTL mid-count. Retry-After is never 0, and the first rejected request per bucket per window is logged.

Test plan

  • pytest tests/ passes, including new tests in tests/api/test_throttling.py for client identity, scope isolation, concurrent requests, counter TTL, Retry-After and logging
  • Guard tests fail if a throttle attached to a route isn't built on the shared base, or if sign-in throttles share a scope unintentionally
  • Once merged into feat: apply API rate limits org-wide via an atomic fixed-window counter #1019, verify on a test environment that a sign-in limit holds for one client across varying proxy hops and under concurrent requests

- Add FixedWindowRateThrottle as the base for every throttle: the atomic
  add+incr fixed-window counter from this PR, with get_ident() resolving the
  client through get_client_ip(), so throttling identifies a client the same
  way audit logging and network access policies do rather than by the raw
  X-Forwarded-For chain. IPv6 clients are bucketed per /64.
- Add AnonIPRateThrottle for unauthenticated endpoints and move the sign-in
  throttles (password login, signup, email check, resend verification, MFA
  verify) onto it with their own scopes. Email check and invite lookup share
  one scope by design. Most of these inherited DRF's "anon" scope, so they
  shared one budget per client across different rates.
- Counter TTL spans two windows, so a counter can't lose its TTL between
  Django's EXISTS and INCR. wait() never returns 0, so Retry-After is always
  sent.
- Tests for client identity, scope isolation and concurrency, plus a guard
  that every throttle attached to a route is built on the shared base and that
  sign-in throttles don't share a scope unintentionally.
One warning per bucket per window, naming the bucket key (the org, or scope and client for per-IP limits) and the limit, so throttled orgs and clients can be identified from the logs.
@nimish-ks
nimish-ks marked this pull request as draft September 21, 2026 13:56
@nimish-ks nimish-ks self-assigned this Sep 21, 2026
@nimish-ks
nimish-ks marked this pull request as ready for review September 21, 2026 14:46
When a proxy in front of nginx doesn't pass the client address on (for example a tunnel or serve sidecar sharing nginx's network namespace), X-Real-IP is the proxy's own loopback or private address for every client, and keying on it would put all clients in one bucket.

throttle_ident() now treats a loopback, private or link-local address as unresolved, so the throttle falls back to DRF's X-Forwarded-For chain identity, which still separates clients. The docstring also states the trust assumption: the edge must overwrite or strip client-supplied X-Real-IP and X-Forwarded-For.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant