Fix rate limiter counted every path against one bucket per IP - #276
Open
aaronjae22 wants to merge 5 commits into
Open
Fix rate limiter counted every path against one bucket per IP#276aaronjae22 wants to merge 5 commits into
aaronjae22 wants to merge 5 commits into
Conversation
aaronjae22
force-pushed
the
fix-rate-limiter-per-rule-buckets
branch
from
August 4, 2026 02:50
7850d30 to
bd255b5
Compare
Base automatically changed from
stacked-1/feedback-from-validate_lola_access_review-pr
to
main
August 4, 2026 02:55
aaronjae22
force-pushed
the
fix-rate-limiter-per-rule-buckets
branch
from
August 4, 2026 02:55
bd255b5 to
0c01d9e
Compare
aaronjae22
marked this pull request as ready for review
August 4, 2026 15:52
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #275
Previously the middleware picked its limit per path but counted requests in a single bucket per IP across all paths:
So basically traffic to any URL was actually using / draining every other URL's budget. Also,
DEBUG=Truein 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:
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 usecache.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>:resetentry records when the window ends, soRetry-Afterreports real remaining seconds instead of a flat guess.Two things worth knowing:
limitrequests get through. The check iscount > limitafter incrementing, so request 60 passes and 61 doesn't.incr()preserves the TTL, soRetry-Afteronly ever counts down.incr()doesget()thenset()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:
oauth_authorize/oauth/authorize/oauth_token/oauth/token/lola_discovery/.well-known/oauth-authorization-serverlola_api/api/actors/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/plainthat clients couldn't parse.RATE_LIMIT_TRUSTED_PROXY_DEPTHThe 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 noX-Forwarded-Forbehaviour at all. Nothing published covers therun.app/ domain-mapping path.I settled on
1because for honest callers it's never worse than0. If the layout is what we expect,1identifies each caller correctly; if it isn't, it falls back toREMOTE_ADDRand behaves exactly like0would. 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 agcloud run services updateaway from being fixed.The
globalin_log_chain_shape_onceThis 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:
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_loggedis deliberate but I know its a bit weird probablyThe test resets it with
monkeypatch.setattrrather 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
LocMemCacheis per-process. Cloud Run autoscales. So the real ceiling is: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 tolimit × 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.