Review: Rate Limiter Written by an AI Assistant

Your team asked an AI coding assistant to add rate limiting to a public Flask API. It opened this pull request. You are the reviewer, and nothing merges without your approval.

The assistant's description:

Adds a token-bucket rate limiter: 5 requests per second per client, with bursts of up to 10. Uses the real client IP from the load balancer. Drop-in @rate_limited decorator for any route.

Where it runs: Gunicorn with 4 worker processes and 8 threads each, behind a cloud load balancer. The decorator will be added to about 30 routes.

Your task

• Flag every problem you would block the merge for. Click a line number to comment, and Shift-click to cover a range.
• For each one, choose the issue type and severity, and explain what fails and how to fix it.
• You are scored on the problems you catch, the right issue type and a clear explanation. Missing a critical problem fails the round.

Aim for 15 minutes, as in a real review round.

Who controls each input? Look at where the client id comes from.

Trace the bucket when it holds 0.5 tokens. Is the request allowed?

32 threads in 4 processes call allow at the same moment. What do they each see?

What happens when Flask registers the second route that uses this decorator?

A strong reviewer blocks this change on two grounds before anything else: the limit can be bypassed by anyone, and the app will not start once the decorator is on a second route.

1. Spoofable client id (line 12, critical). X-Forwarded-For is set by the client unless your own proxy overwrites it. Any caller can send a new value per request and get a fresh bucket, so the limiter does nothing. The header can also hold a list such as "1.2.3.4, 10.0.0.1". Use the address added by your own load balancer, for example with Werkzeug's ProxyFix(x_for=1).
2. One request too many (lines 19–20, major). tokens >= 0 lets a request through with 0.4 tokens, and the bucket goes negative. The check must be tokens >= 1.
3. Race and per-process state (lines 17–22, major). The read, compute and write are not atomic, so concurrent threads can all pass with the same tokens. Each of the 4 processes also has its own dictionary, so the real limit is four times higher. Use a shared store with an atomic update, such as a Redis Lua script.
4. Endpoint collision (lines 27–31, major). Without functools.wraps, every decorated view is named wrapper, and Flask raises "View function mapping is overwriting an existing endpoint" on the second route.
5. Unbounded memory (line 7, major). Every client id stays in buckets forever. Combined with the spoofable header, an attacker can grow it without limit. Expire idle entries or use a store with TTLs.
6. Wall-clock time (line 16, minor). time.time() can jump when the clock is adjusted. Use time.monotonic() for intervals.