Fix rate limiter counted every path against one bucket per IP

[aaronjae22]

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.

[lisad]

Let's discuss the log message stuff: once per "chain shape" and make sure I'm understanding

[lisad]

It seems to me that this will likely log the IP address of the first request to hit a production thread, then no other IP addresses. Is that the goal? I would assume that would not be the goal. Is there an assumption that new requests trigger a new thread? If so, that assumption could make this logging code fairly brittle and mislead us by skipping events we intend to log.

Is this more fancy than we need anyway? Why does the chain shape matter for rate limiting and not just the client IP? Or is this an indication that we should use a different approach that could be a lot less complex? E.g. maybe we should assign API keys and track those rather than try to use client IP

There may be a bunch of things I'm not understanding yet so I'd love a discussion.

[aaronjae22]

I agree this could be a bit confusing and I was planning on iterating over that once we get it tested on production.

I’ve been iterating over several approaches to do it.

I submitted the client ip approach but I also thought of other ideas. This LOLA spec part: The source may limit active OAuth tokens per account or Actor, to prevent a single user from initiating too many concurrent data copy sessions. gave me two differentes approaches. OAuthClientCredentials gives every registered destination a client_id, and TokenActorBinding binds each portability token to one Actor at issuance. Keying limits on that identity would give real destinations exact buckets and make proxy depth irrelevant.

I also thought of adding a small endpoint for testing purposes that will get us what the middleware resolved for our own request and start from there.

I agree that identity beats IP in this specific case but there are two things to mention:

Two things to mention:

1. The limiter currently runs above DRF (before auth, ddbb, and view) so a rejected request never hits the database, and moving it behind authentication gives that up since we'll have to look for the token before we can decide on it.
   • The bigger problem is coverage since /oauth/authorize/ and /oauth/token/ aren't DRF views and they come from django-oauth-toolkit. If the limiter is placed in the DRF layer it would stop protecting them.
2. And this is basically why I decided the client ip approach, since half the LOLA surface is public readable by design, so I am thinking anonymous traffic still needs an IP-based fallback tier or we could just omit this part.

I would like your input on this so we can decide which approach you think is better for us. I wouldn't mind going back to the OAuthClientCredentias approach I thought of but we might need to decide what to do with the public access of it.

---

The thing with the global log is that is meant as a one-shot setup scanning or examination instead of observability. The thing is that behind Cloud Run or AWS the user address is in the X-Forwarder-For header.

To read it we have to count from the right hand end which means we need to know how many entries Google appends. This is documented for Load Balancer but not for our normal run.app/domain-mapping path we’re currently on.

There's no thread assumption. The flag is a module-level global, it's shared across all 8 gunicorn threads, one line per process and since we run --workers 1, one line per Cloud Run instance lifetime. That was the intent that each instance reports the chain it sees.