PST-027: Rate-limit counters live inside the request transaction, so a charge made before an exception is rolled back #756

New issue

Closed

opened 2026-10-10 10:56:03 +00:00 by tiagoagueda · 1 comment

tiagoagueda commented

2026-10-10 10:56:03 +00:00

Owner

Found in the security audit of 2026-10-10 (SECURITY-AUDIT-2026-10-10.md), run against main at c30851b from 8 to 10 October 2026, as PST-027. Severity: Low.

The text below is the audit's entry, unchanged apart from links to the other issues it names. The finding was produced by automated review and checked by a second automated pass; re-check it before acting on it.

Severity: Low
Category: rate-limiting / cache design
Location: src/postulo/core/throttle.py (112-136)
Found by: configuration and dependencies
Related: PST-005 (#734)

Who can do it, and what they need. An authenticated account or API token holder who wants more of a bounded action than the allowance. Precondition: a view that runs under ATOMIC_REQUESTS, calls throttle.consume() and can then be made to raise an exception that escapes the view (Http404, PermissionDenied, an unhandled error). …

How it works.

ATOMIC_REQUESTS is on for every view (base.py:154) and the default cache is a table in the same database (base.py:189-191), so cache.add/incr in consume() are statements inside the request's transaction.
The attacker sends a request to a throttled action that fails after consume() with an exception leaving the view.
Django rolls the transaction back, which also discards the counter increment.
Repeating the request never spends the allowance. Verified mechanism with a scratch test ([scratch test]): five consume()+raise cycles inside transaction.atomic() left the counter at zero; two normal returns were persisted and the third call raised TooOften.

Impact. A per-account or per-address limit stops counting for any request shape that raises after being charged. Today that is only the undercounting of failed requests; a future view that looks up a record after spending a limit (the recovery-link view already does the 404 lookup before consume, which is why it is safe) would hand an attacker an unbounded allowance. Same mechanism weakens the /logs and /metrics wrong-token counter if a refusal ever becomes an exception instead of a returned 401.

Evidence.

base.py:154 `DATABASES["default"].setdefault("ATOMIC_REQUESTS", True)`; base.py:190 `"default": env.cache_url("POSTULO_CACHE_URL", default=f"dbcache://{CACHE_TABLE}")`; throttle.py:125-131 `if cache.add(key, 1, timeout=rate.seconds): used = 1 else: try: used = cache.incr(key)`; throttle.py docstring lines 21-24 acknowledges incr is non-atomic but not that it is transactional.

Verification (verdict: confirmed; severity after verification: Low). Attacker/precondition as claimed. Correction to the call-site list: the outbox charge at correspondence.py:131 runs under ATOMIC_REQUESTS from SendDocumentsView (documents/views.py:934), and the API per-token charge at api/auth.py:29 runs inside the transaction on every API route except the three in OUTSIDE_A_TRANSACTION; neither is exploitable because their refusals are returned, not raised (ninja handlers at api/problems.py:203-253). Today the only thing that un-charges is an unhandled 500 or an unhandled SuspiciousOperation subclass (TooManyFieldsSent and the like) after the charge, which gains nothing. Remediation (describe only): make the counter independent of the request transaction — e.g. give the throttle its own cache alias on a second connection to the same database (a DATABASES alias routed for CacheEntry) or recommend a non-database cache for counting in the docs/check; or at minimum state in throttle.py's docstring and in tests/security/test_rate_limits.py that a charge must be followed by a return, never a raise, so a future view (one that looks up a record after spending the allowance) cannot reopen this.

Suggested remediation. Spend counters outside the request transaction: route the cache table to a second database alias of the same database through a router so the DB cache uses its own connection, or recommend Redis in production and make the dbcache default fail-safe by running consume() under transaction.atomic(using=...) on that alias; alternatively mark every view that spends an allowance with non_atomic_requests and add a security test asserting that a charge survives a view that raises Http404.

Found in the security audit of 2026-10-10 (`SECURITY-AUDIT-2026-10-10.md`), run against `main` at `c30851b` from 8 to 10 October 2026, as **PST-027**. Severity: **Low**. The text below is the audit's entry, unchanged apart from links to the other issues it names. The finding was produced by automated review and checked by a second automated pass; re-check it before acting on it. --- - **Severity:** Low - **Category:** rate-limiting / cache design - **Location:** `src/postulo/core/throttle.py` (112-136) - **Found by:** configuration and dependencies - **Related:** PST-005 (#734) **Who can do it, and what they need.** An authenticated account or API token holder who wants more of a bounded action than the allowance. Precondition: a view that runs under ATOMIC_REQUESTS, calls throttle.consume() and can then be made to raise an exception that escapes the view (Http404, PermissionDenied, an unhandled error). … **How it works.** 1. ATOMIC_REQUESTS is on for every view (base.py:154) and the default cache is a table in the same database (base.py:189-191), so cache.add/incr in consume() are statements inside the request's transaction. 2. The attacker sends a request to a throttled action that fails after consume() with an exception leaving the view. 3. Django rolls the transaction back, which also discards the counter increment. 4. Repeating the request never spends the allowance. Verified mechanism with a scratch test ([scratch test]): five consume()+raise cycles inside transaction.atomic() left the counter at zero; two normal returns were persisted and the third call raised TooOften. **Impact.** A per-account or per-address limit stops counting for any request shape that raises after being charged. Today that is only the undercounting of failed requests; a future view that looks up a record after spending a limit (the recovery-link view already does the 404 lookup before consume, which is why it is safe) would hand an attacker an unbounded allowance. Same mechanism weakens the /logs and /metrics wrong-token counter if a refusal ever becomes an exception instead of a returned 401. **Evidence.** ```text base.py:154 `DATABASES["default"].setdefault("ATOMIC_REQUESTS", True)`; base.py:190 `"default": env.cache_url("POSTULO_CACHE_URL", default=f"dbcache://{CACHE_TABLE}")`; throttle.py:125-131 `if cache.add(key, 1, timeout=rate.seconds): used = 1 else: try: used = cache.incr(key)`; throttle.py docstring lines 21-24 acknowledges incr is non-atomic but not that it is transactional. ``` **Verification** (verdict: confirmed; severity after verification: Low). Attacker/precondition as claimed. Correction to the call-site list: the outbox charge at correspondence.py:131 runs under ATOMIC_REQUESTS from SendDocumentsView (documents/views.py:934), and the API per-token charge at api/auth.py:29 runs inside the transaction on every API route except the three in OUTSIDE_A_TRANSACTION; neither is exploitable because their refusals are returned, not raised (ninja handlers at api/problems.py:203-253). Today the only thing that un-charges is an unhandled 500 or an unhandled SuspiciousOperation subclass (TooManyFieldsSent and the like) after the charge, which gains nothing. Remediation (describe only): make the counter independent of the request transaction — e.g. give the throttle its own cache alias on a second connection to the same database (a DATABASES alias routed for CacheEntry) or recommend a non-database cache for counting in the docs/check; or at minimum state in throttle.py's docstring and in tests/security/test_rate_limits.py that a charge must be followed by a return, never a raise, so a future view (one that looks up a record after spending the allowance) cannot reopen this. **Suggested remediation.** Spend counters outside the request transaction: route the cache table to a second database alias of the same database through a router so the DB cache uses its own connection, or recommend Redis in production and make the dbcache default fail-safe by running consume() under transaction.atomic(using=...) on that alias; alternatively mark every view that spends an allowance with non_atomic_requests and add a security test asserting that a charge survives a view that raises Http404.

tiagoagueda added the

bug

security

labels

2026-10-10 10:56:04 +00:00

tiagoagueda referenced this issue

2026-10-10 10:57:19 +00:00

PST-005: Rate limits longer than five minutes collapse to five minutes on the default database cache #734

tiagoagueda added this to the 0.6.0 milestone

2026-10-10 10:57:35 +00:00

tiagoagueda referenced this issue

2026-10-10 10:58:39 +00:00