Skip to content

Fix rate limiter counted every path against one bucket per IP - #276

Open
aaronjae22 wants to merge 5 commits into
mainfrom
fix-rate-limiter-per-rule-buckets
Open

Fix rate limiter counted every path against one bucket per IP#276
aaronjae22 wants to merge 5 commits into
mainfrom
fix-rate-limiter-per-rule-buckets

Conversation

@aaronjae22

@aaronjae22 aaronjae22 commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Closes #275

Previously the middleware picked its limit per path but counted requests in a single bucket per IP across all paths:

rate_limit    = self.get_rate_limit_for_path(request.path)   # limit is PER-PATH
request_times = self.request_counts[client_ip]               # counter is PER-IP, ALL PATHS

So basically traffic to any URL was actually using / draining every other URL's budget. Also, DEBUG=True in local was making Django serve static files, so one demo page load spent ~12 requests, and the first click on
"Test Authorization Flow" came back 429. Staging and production had the same bug but slower, since static comes from Cloud Storage there.


Now we are using a one fixed-window counter per (rule, client) pair that's being held in Django's cache:

ratelimit:<rule_name>:<client_ip>

Each request resolves to exactly one rule by longest matching path prefix (so declaration order in settings doesn't matter, specificity decides).

The counter is a single integer with a TTL equal to the rule's window. The first request of a window creates it with
cache.add(); later ones use cache.incr(). Expiry is the cache's job, which is why there's no cleanup pass any more, the old one scanned every tracked IP on every request.

A companion <key>:reset entry records when the window ends, so Retry-After reports real remaining seconds instead of a flat guess.

Two things worth knowing:

  • Exactly limit requests get through. The check is count > limit after incrementing, so request 60 passes and 61 doesn't.
  • Hammering while blocked can't extend own lockout. incr() preserves the TTL, so Retry-After only ever counts down.
    • This is load-bearing: Django's database cache backend would break it (its incr() does get() then set() without a timeout, resetting the TTL), which would silently turn this into a sliding window with no test failing.

Current limits are a bit generous, since an interactive OAuth flow is several requests and this testbed exists for people to exercise it repeatedly:

Rule Prefix Limit Window
oauth_authorize /oauth/authorize/ 60 300s
oauth_token /oauth/token/ 120 300s
lola_discovery /.well-known/oauth-authorization-server 60 60s
lola_api /api/actors/ 120 60s
(fallback) everything else 300 60s

Static, media, health checks and favicon are exempt entirely. Rate limiting is off in development and test, env-flippable via DJANGO_RATE_LIMIT_ENABLED=1.

The 429 body is now JSON on the same error contract as every other endpoint, instead of text/plain that clients couldn't parse.


RATE_LIMIT_TRUSTED_PROXY_DEPTH

The old code read X-Forwarded-For[0]. That header is appended to by each proxy, so the leftmost entry is whatever the caller sent, anyone could mint a fresh bucket per request just by varying it. The limiter constrained honest clients and not abusive ones.

The new setting says how many trailing entries came from infrastructure we trust; the client is the one just before them. Anything injected lands further left and is ignored.
Default is 0, meaning the header isn't trusted at all. Each deployment opts in.

Production sets 1, and I want to be honest about that this is a bet rather than a documented fact. I went looking for the guarantee and official docs and it isn't there: Google specifies the <client-ip>, <load-balancer-ip> format only for external Application Load Balancers, which we don't use yet, and the Cloud Run container contract documents no X-Forwarded-For behaviour at all. Nothing published covers the run.app / domain-mapping path.

I settled on 1 because for honest callers it's never worse than 0. If the layout is what we expect, 1 identifies each caller correctly; if it isn't, it falls back to REMOTE_ADDR and behaves exactly like 0 would. The risk is that a caller could pad the header to get a fresh bucket per request. It's env-overridable, so a wrong value is a gcloud run services update away from being fixed.


The global in _log_chain_shape_once

This is the part I'd most like a second opinion on.

The problem is that the depth above can only be validated against a real request through the real proxy chain, and a value that's too low fails silently; the existing warning only fires when the chain is shorter than configured, never longer. Without something new, the only way to see what the middleware resolved was to deliberately trigger a 429 on
production, which risks causing the exact outage you're testing for.

So the middleware now logs the chain shape once per process, on the first counted request:

Rate limit client resolution: xff_entries=2 configured_depth=1 resolved=203.0.113.5

Verification becomes one ordinary request plus one log read, and the entries are parsed even at depth 0 so it works before we have trusted the header at all.

The global _chain_shape_logged is deliberate but I know its a bit weird probably

The test resets it with monkeypatch.setattr rather than a bare assignment, so it doesn't leak into whatever runs after.


Cloud Run and instances

Counters live in Django's cache, which under the default LocMemCache is per-process. Cloud Run autoscales. So the real ceiling is:

limit × gunicorn_workers × running_cloud_run_instances

Threads don't multiply it; they share one process's memory, which is exactly why incr()
has to be atomic. Worker processes and instances do. The container runs --workers 1, so currently it reduces to limit × instances.

In practice the testbed usually runs one instance, where counters are exactly right. Cloud Run scales on concurrency, and a destination doing sequential paged fetches generates a concurrency of about 1 so a normal migration stays on one instance and sees the configured limit. The multiplier only shows up under genuine concurrent load, which is also when letting a bit of extra traffic through matters least.


This needs revisiting after deploy. I left docstring as a guide when we verify the implementation after deployment.

@aaronjae22 aaronjae22 self-assigned this Jul 31, 2026
@aaronjae22 aaronjae22 changed the title refactor: Extracting build_error_payload for middleware JSON errors Iterating over rate limiter implementation Aug 4, 2026
@aaronjae22
aaronjae22 force-pushed the fix-rate-limiter-per-rule-buckets branch from 7850d30 to bd255b5 Compare August 4, 2026 02:50
Base automatically changed from stacked-1/feedback-from-validate_lola_access_review-pr to main August 4, 2026 02:55
@aaronjae22
aaronjae22 force-pushed the fix-rate-limiter-per-rule-buckets branch from bd255b5 to 0c01d9e Compare August 4, 2026 02:55
@aaronjae22
aaronjae22 marked this pull request as ready for review August 4, 2026 15:52
@aaronjae22
aaronjae22 requested a review from lisad August 4, 2026 15:53
@aaronjae22 aaronjae22 changed the title Iterating over rate limiter implementation Fix rate limiter counted every path against one bucket per IP Aug 5, 2026
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.

Rate limiter counts all traffic in one per-IP bucket, causing 429s

1 participant