No rate limiting on authentication or expensive endpoints #68
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Severity: MEDIUM
The bug
No throttling anywhere. Grepped for
ratelimit,slowapi,limiter,throttle— zeromatches. Nothing limits login attempts, the OAuth callback, uploads, or reads.
POST /api/ingestaccepts 100 MB per file with no per-user quota and no concurrency cap, andeach request triggers a full SHA-256 read plus a Pillow parse.
Impact
An authenticated user (or a script with a stolen cookie) uploads 100 MB files in a loop, filling
the disk that holds both the photo archive and the database. Or hammers
GET /api/photos?limit=200concurrently to saturate CPU — SQLite serializes writers, sosustained load degrades everything.
Contradicts the spec
docs/circa-spec.md§4.1: "Failed login attempts are rate-limited and logged."Fix
slowapior equivalent with per-user and per-IP limits, stricter on/api/ingestand theauth routes.
Done when
References
backend/app/main.py(middleware stack)docs/circa-spec.md§4.1Done in
0b8e5f0. All three "done when" items covered, 27 tests inbackend/tests/test_rate_limiting.py.I did not use
slowapi, and I want to be explicit about why since the issue named it. Its default storage backend is in-process memory — exactly what's implemented here — so on a single-instance deployment the dependency buys an API, not a capability. Adding a third-party package to the pre-authentication request path has a real cost, and it's the same call already made for CSRF and body limits (middleware.pyrecords the reasoning). If Circa ever runs multi-process, this needs a shared backend andslowapi+ Redis becomes the right answer; the limiter is a single class behind one middleware, so that swap is contained. Push back if you'd rather take the dependency now.Budgets shipped (all configurable):
/api/auth/*/api/ingest/api/healthAuth keys on address, not session, because the caller has no session yet — otherwise an attacker gets a fresh budget per attempt just by discarding cookies. Everything else keys on the session id from the signed cookie, which is only a bucket label and never a lookup, so a forged one costs an attacker a bucket rather than access.
Two design points that are easy to get wrong, both tested directly:
Refused requests are not charged. If they were, a client already over its limit would hold itself over indefinitely by continuing to send — the window would never drain, turning a brief burst into a permanent lockout.
Tracked identities are LRU-capped. Without it the limiter is itself a memory-exhaustion vector: an attacker cycling source addresses grows the map without bound, so the thing meant to bound resource use becomes the leak.
Uploads need both budgets. Request count alone doesn't bound disk — at 100 MB per file, 60 requests is 6 GB. The byte quota charges declared
Content-Lengthbefore the body is read; an undeclared length is charged the per-file maximum, since refusing to guess low is the safe direction.Two things needing your attention:
X-Forwarded-Foris not trusted by default (CIRCA_TRUST_PROXY_HEADERS=false). Believing it without a proxy that overwrites it is worse than no per-address limit at all. But once #50 lands you must turn it on, or every request will appear to come from the proxy and the per-address auth budget becomes a single global one. Worth a note on #50.Per-user storage quota not implemented — only a byte rate. A cumulative "this user may store at most N GB" needs usage accounting that doesn't exist yet and interacts with the backup work (#117). The rate limit bounds how fast the disk can fill, which is the DoS the issue describes; a standing quota is a capacity-planning control. Say if you want it and I'll open an issue.
Refused logins and allowlist rejections are now logged (§4.1) — previously the callback logged only OAuth exceptions, so an unverified-email or not-on-the-allowlist refusal was silent.
Also fixed a flaky test I'd introduced in
f745175while here — details in the commit message. Suite run four times clean at 284 tests.