phase: 81_login_rate_limit
All gates verified. The phase is complete — everything was already implemented by prior task runs; this pass verified each completion criterion end-to-end and found no defects. **Phase 81 final verification report** (tasks 01–03 all present in `complete/`; no code changes needed this pass) - Verified: `app/core/rate_limit.py` (stdlib sliding window, fail-open) + both login routes' 429 pre-check/record/reset wiring + all unit/integration pins present (11th-429, blocked-success-still-429, clean-counter reset, shared counter, autouse `clean_rate_limit` fixture documented). - Live check (task 03): dev server + 11 rapid wrong logins → `401 ×10, 429`; 429 body carries `retry-after: 900` + generic detail; server restarted (per-process counter cleared by design). - `uv run pytest tests/unit/test_rate_limit.py -v --no-cov` → 10 passed - `uv run pytest --cov=app --cov-report=term-missing` → 1652 passed, **TOTAL 99%** (>90%; rate_limit.py 100%, auth.py 100%) - `uv run pytest tests/e2e/test_smoke.py -v --no-cov` (isolation) → 3 passed - `uv run ruff check . && uv run pyright` → All checks passed / 0 errors, 0 warnings - Completion criteria: all met, except commit + phase-dir move — per harness rules I left all changes uncommitted in the working tree (harness commits atomically and moves the phase). - Diff scope: exactly `app/core/rate_limit.py`, `app/api/auth.py`, `tests/unit/test_rate_limit.py`, `tests/integration/test_auth_api.py` + phase files; `pyproject.toml` / `uv.lock` / `frontend/` untouched. - Deviations: none in code; commit/move deferred to harness as instructed. - Next pending phase: `82_security_headers`.
This commit is contained in:
@@ -0,0 +1,69 @@
|
||||
# Phase 81 — Rate-limit failed sign-ins: 429 + Retry-After after repeated failures
|
||||
|
||||
**Source:** `.agents/remediation_plan.md` SEC-03 (security audit 2026-09-07, severity Medium — "No rate limiting / lockout on login + token-auth")
|
||||
**Story:** n/a (security hardening — audit-derived, no user story)
|
||||
**Context:** `app/api/auth.py` (`POST /api/login` — `check_password` then `sign_in`; `POST /api/token-auth` — `find_active_by_token` then session keys — both return one generic 401 on every failure shape, no throttling of any kind), `app/core/auth.py` (the constant-time `check_password`, `sign_in`/`sign_out` — untouched by this phase), `app/main.py` (middleware registration order — this phase adds no middleware), `frontend/assets/login.js` (the login form's error path: non-204 → `Sign-in failed (HTTP ${r.status}) — try again.` via `showError` — a 429 already renders there as a generic message, so no frontend change is required), `tests/unit/test_auth.py` + `tests/integration/test_auth_api.py` (the existing auth contract pins — every 401 shape, no enumeration).
|
||||
|
||||
## Objective
|
||||
A client IP that fails sign-in repeatedly (admin password or token) gets **429 + `Retry-After`** instead of unlimited line-rate guesses: a stdlib-only in-memory sliding-window limiter shared by both login routes, counting only failed attempts, reset on success, failing open (a limiter bug must never lock the owner out).
|
||||
|
||||
## Audit basis (read this, not the chat)
|
||||
- Neither `POST /api/login` nor `POST /api/token-auth` throttles: the password is compared with `compare_digest` (fast), so an attacker who can reach `:8000` can try thousands of guesses per second (audit PoC: 10 000 sequential logins all answered at line rate).
|
||||
- The fix is deliberately small and stateless-per-process (A10: no new service, no DB table, no Redis): a per-IP deque of failure timestamps in module memory, lost on restart (an acceptable reset — it is a friction bump, not the boundary; the homelab HTTP posture is the documented owner decision).
|
||||
- Threshold (locked in this phase): **10 failed attempts per client IP per 15-minute sliding window** → subsequent attempts get **429** `{"detail": "too many failed sign-in attempts — try again later"}` + a `Retry-After: <seconds>` header until the window clears.
|
||||
- The counter is SHARED by both routes (any auth failure from the IP counts — a token-spraying attack must not reset by alternating routes), and a SUCCESS on either route clears that IP's counter (a legitimate owner who fat-fingers twice is not poisoned).
|
||||
|
||||
## Owner decisions (chat, 2026-09-07 — recorded per AGENTS.md rule 3)
|
||||
- **A1 — in-memory, stdlib-only:** no new dependency, no DB table, per-process state (A10/A12 spirit). A restart clears the counter — accepted.
|
||||
- **A2 — 10 / 15 min, module constants:** `MAX_FAILURES = 10`, `WINDOW_SECONDS = 900` as documented module constants in the limiter module — NOT new `BOR_` settings (a security control with a fixed, tested default; no env surface to mistype).
|
||||
- **A3 — 429 contract:** one generic detail (no enumeration between login/token failures), `Retry-After` header carrying the whole-window seconds; the login page's existing `Sign-in failed (HTTP 429) — try again.` line renders it — no frontend change in this phase.
|
||||
- **A4 — client IP = `request.client.host`:** the direct peer address. ASSUMPTION (locked here): the app is served directly (homelab), no reverse proxy — if a proxy is ever put in front, the limiter needs `X-Forwarded-For` handling (noted in the module docstring, out of scope now).
|
||||
|
||||
## Design (shared by all tasks — the executor reads this, not the chat)
|
||||
- **`app/core/rate_limit.py` (new)** — the limiter, pure stdlib (`collections.deque`, `time.monotonic`, `threading.Lock` for the dict — the ASGI event loop is single-threaded but the lock costs nothing and keeps the unit tests honest under pytest-xdist if ever added):
|
||||
```python
|
||||
MAX_FAILURES = 10
|
||||
WINDOW_SECONDS = 900
|
||||
|
||||
_failures: dict[str, deque[float]] = {}
|
||||
_lock = threading.Lock()
|
||||
|
||||
def record_failure(client_ip: str) -> None: ... # append now(); prune expired; never raises
|
||||
def remaining_wait(client_ip: str) -> int: ... # 0 = allowed; else whole-window seconds (ceil) until the oldest counted failure expires
|
||||
def reset(client_ip: str) -> None: ... # drop the IP's entry (on success)
|
||||
```
|
||||
Semantics (all unit-pinned): a failure is counted when it is recorded; the window slides — only failures within the last `WINDOW_SECONDS` count; the IP is blocked while the count of in-window failures `>= MAX_FAILURES` (the 10th failure already blocked? NO — the 10th failure is recorded and the **11th** attempt is the first 429: the check happens before the attempt); `remaining_wait` returns `int(ceil(WINDOW_SECONDS - (now - oldest)))` while blocked, `0` otherwise; **fail-open**: any internal error (corrupt deque, clock skew) → `remaining_wait` returns `0` and `record_failure` no-ops — a limiter bug never denies the owner.
|
||||
- **`app/api/auth.py`** — the two route changes (identical shape):
|
||||
- `login`: first line of the handler — `ip = request.client.host; if (wait := _wait(ip)) > 0: raise HTTPException(429, detail=TOO_MANY_DETAIL, headers={"Retry-After": str(wait)})`; on password failure → `record_failure(ip)` before the 401; on success → `reset(ip)` before the 204.
|
||||
- `token_auth`: same three points (pre-check, record on the 401 path, reset on the 204 path).
|
||||
- `TOO_MANY_DETAIL = "too many failed sign-in attempts — try again later"` (one string shared by both routes — no enumeration).
|
||||
- Module docstrings updated: the 429 case joins the documented status set of each route.
|
||||
- **Not touched:** `app/core/auth.py` (password check + session helpers), the 401 contract of both routes (byte-identical on the non-throttled path), everything else in the app, all frontend files.
|
||||
|
||||
## Dependencies
|
||||
— (none; extends the completed phase-16/79 auth surface additively — the 401/204 contract is unchanged for non-throttled callers)
|
||||
|
||||
## Tasks
|
||||
1. `01_rate_limiter_core.md` — `app/core/rate_limit.py` + the unit suite for the window semantics.
|
||||
2. `02_auth_routes_wiring.md` — the 429 pre-check + failure/success bookkeeping on both login routes + integration tests.
|
||||
3. `03_verify_and_commit.md` — full gate (suite + coverage + smoke E2E in isolation) + atomic commit.
|
||||
|
||||
## Testing & Quality
|
||||
- Unit — `tests/unit/test_rate_limit.py` (new): 9 failures → allowed; the 10th failure recorded; the next attempt → `remaining_wait > 0`; two independent IPs independent; a failure older than the window expires (monkeypatch a fake clock or pre-seed timestamps) and unblocks; `reset` clears (a success unblocks immediately); `remaining_wait` is the whole-window seconds, not the per-failure age; fail-open: a pre-corrupted state (e.g. a non-deque entry injected) → `remaining_wait` returns 0, `record_failure` doesn't raise.
|
||||
- Integration — `tests/integration/test_auth_api.py` (extend): 10 wrong-password logins from the TestClient → 11th → **429** with the generic detail + a `Retry-After` header (int > 0); a correct password after 9 failures → 204 AND the counter is reset (the next 10 failures needed to block again — assert by 9 more failures still 401); token-auth failures share the counter (5 login + 5 token failures → the 11th of either → 429); the non-throttled 401 contract (existing pins) byte-identical.
|
||||
- E2E (isolation gate per AGENTS.md rule 9): `uv run pytest tests/e2e/test_smoke.py -v --no-cov` green — no UI change; the login form's existing 429 rendering is covered by the integration contract (the generic `HTTP ${status}` line).
|
||||
- Coverage: **>90%** on `app/` — `app/core/rate_limit.py` is small and fully unit-exercised; the `app/api/auth.py` branches (pre-check hit/miss, record, reset) integration-covered.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] 11 rapid failed logins from one client: the first 10 → 401 (unchanged detail), the 11th → 429 + `Retry-After`; the token-auth route participates in the same counter (integration pins).
|
||||
- [ ] A successful login/token-auth resets the IP's counter (integration pin).
|
||||
- [ ] `uv run pytest tests/unit/test_rate_limit.py -v` green; full `uv run pytest` green; `uv run pytest --cov=app --cov-report=term-missing` >90%.
|
||||
- [ ] `uv run pytest tests/e2e/test_smoke.py -v --no-cov` green in isolation.
|
||||
- [ ] `uv run ruff check . && uv run pyright` clean.
|
||||
- [ ] No dependency change (`pyproject.toml` / `uv.lock` untouched); `git diff --stat` limited to `app/core/rate_limit.py`, `app/api/auth.py`, the two test files, phase files.
|
||||
- [ ] One atomic `--no-gpg-sign` commit (e.g. `fix(auth): rate-limit failed logins and token-auth attempts with 429 + Retry-After`); phase dir moved to `.agents/phases/complete/`.
|
||||
|
||||
## Locked decisions
|
||||
- **A10 intact** — no new service/table/middleware; one module + two route pre-checks.
|
||||
- **401 contract unchanged** — the no-enumeration single-401 discipline of phase 16/79 is byte-identical for every non-throttled attempt; 429 is a new status, not a rewording.
|
||||
- **No frontend change** — `login.js` already renders any non-204 as `Sign-in failed (HTTP ${status}) — try again.` (the 429 reads correctly there).
|
||||
@@ -0,0 +1,33 @@
|
||||
# Task 01 — The sliding-window limiter module + unit suite
|
||||
|
||||
**Phase:** `81_login_rate_limit` · **Story:** n/a (security hardening — audit SEC-03)
|
||||
|
||||
## Objective
|
||||
`app/core/rate_limit.py` exists with the exact semantics from the phase design block, and `tests/unit/test_rate_limit.py` pins every one of them.
|
||||
|
||||
## Work
|
||||
1. `app/core/rate_limit.py` (new) — implement per the phase overview's design block:
|
||||
- constants `MAX_FAILURES = 10`, `WINDOW_SECONDS = 900`;
|
||||
- module state: `_failures: dict[str, deque[float]]` + a `threading.Lock`;
|
||||
- `record_failure(client_ip: str)` — under the lock: get/create the deque, append `time.monotonic()`, prune entries older than `WINDOW_SECONDS`; **never raises** (wrap the body, `except Exception: return` — the fail-open contract);
|
||||
- `remaining_wait(client_ip: str) -> int` — under the lock: prune; count in-window entries; if `< MAX_FAILURES` return `0`; else `max(1, int(-oldest - now + WINDOW_SECONDS))` as a ceiling — i.e. `int(math.ceil(WINDOW_SECONDS - (now - oldest)))` clamped to >= 1; **never raises** (fail-open → 0);
|
||||
- `reset(client_ip: str)` — under the lock: `_failures.pop(client_ip, None)`;
|
||||
- module docstring: the audit basis (SEC-03), the 10/15-min threshold, the shared-counter intent (both login routes), fail-open rationale (a limiter bug must never lock the owner out), and the `request.client.host` assumption — direct peer, no reverse proxy in the current homelab deployment (proxy deployments would need `X-Forwarded-For` handling — out of scope).
|
||||
2. `tests/unit/test_rate_limit.py` (new) — pin, in this order:
|
||||
- 9 × `record_failure("ip")` → `remaining_wait("ip") == 0`;
|
||||
- a 10th `record_failure` → `remaining_wait("ip") > 0` (and `<= WINDOW_SECONDS`);
|
||||
- a second IP is unaffected (`remaining_wait("ip2") == 0`);
|
||||
- expiry: pre-seed one IP's deque with a timestamp `WINDOW_SECONDS + 1` in the past (reach into the module state, or a small `for` loop calling `record_failure` after monkeypatching `time.monotonic` to walk the clock forward) → `remaining_wait` back to 0;
|
||||
- `reset` after 10 failures → `remaining_wait == 0`;
|
||||
- `remaining_wait` for an unknown IP → 0;
|
||||
- fail-open: inject a corrupt entry (`_failures["bad"] = object()` — reach into module state) → `remaining_wait("bad") == 0` and `record_failure("bad")` returns without raising;
|
||||
- `reset` on an unknown IP → no error.
|
||||
|
||||
## Testing & Quality
|
||||
- `uv run pytest tests/unit/test_rate_limit.py -v` green.
|
||||
- Coverage: **>90%** on `app/core/rate_limit.py` (the fail-open branches included — they are the point).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/unit/test_rate_limit.py -v` — all tests green.
|
||||
- [ ] `uv run ruff check app/core/rate_limit.py tests/unit/test_rate_limit.py && uv run pyright` clean.
|
||||
- [ ] `git diff --stat` for this task: exactly the two new files.
|
||||
@@ -0,0 +1,33 @@
|
||||
# Task 02 — Wire the limiter into both login routes + integration tests
|
||||
|
||||
**Phase:** `81_login_rate_limit` · **Story:** n/a (security hardening — audit SEC-03)
|
||||
|
||||
## Objective
|
||||
`POST /api/login` and `POST /api/token-auth` enforce the 429 pre-check and record/reset the counter, and the integration suite pins the HTTP contract (429 + `Retry-After`, shared counter, success reset, unchanged 401s).
|
||||
|
||||
## Work
|
||||
1. `app/api/auth.py` — add the module constant `TOO_MANY_DETAIL = "too many failed sign-in attempts — try again later"` and import the limiter (`from app.core import rate_limit`).
|
||||
2. `app/api/auth.py::login` — insert at the TOP of the handler (before `check_password`):
|
||||
- `ip = request.client.host or "unknown"` (the `or` guards a scope with no client — TestClient edge; fail-open spirit);
|
||||
- `if (wait := rate_limit.remaining_wait(ip)) > 0: raise HTTPException(status_code=429, detail=TOO_MANY_DETAIL, headers={"Retry-After": str(wait)})`;
|
||||
- in the failure branch: `rate_limit.record_failure(ip)` immediately before the 401;
|
||||
- in the success branch: `rate_limit.reset(ip)` immediately before the 204.
|
||||
3. `app/api/auth.py::token_auth` — the identical three insertions (same `ip` derivation, same `TOO_MANY_DETAIL`, same pre-check/record/reset points — the counter is shared by design).
|
||||
4. `app/api/auth.py` module docstring — extend the two route bullets with the 429 case (one line each: "while the per-IP failure window is exhausted → 429 + `Retry-After`, one generic detail — audit SEC-03").
|
||||
5. `tests/integration/test_auth_api.py` — extend (keep every existing pin intact; the TestClient's `request.client.host` is a fixed "testclient" value, so these tests share one counter — **add a fixture or helper that calls `rate_limit.reset("testclient")` between tests** so existing tests can't trip the limiter, and document why in the test module docstring):
|
||||
- 10 × wrong-password `POST /api/login` → 11th → **429**, `detail == TOO_MANY_DETAIL`, `Retry-After` header present and `int(...) > 0`;
|
||||
- after the 429, a CORRECT password → still 429 (the window is not bypassed by success — success only RESETS a clean counter; while blocked, the pre-check fires first; pin this: it is the intended semantics — an exhausted window stays exhausted until it slides);
|
||||
- reset: with a clean counter, 9 failures + 1 success (204) → the next 9 failures are all 401 (the counter is back to 0 after the success);
|
||||
- shared counter: 5 login failures + 5 token failures → the next token-auth attempt → 429;
|
||||
- the existing non-throttled 401 pins (generic `invalid password` / `invalid token`) still green with the clean-counter fixture.
|
||||
|
||||
## Testing & Quality
|
||||
- `uv run pytest tests/integration/test_auth_api.py -v` green (new + existing pins).
|
||||
- `uv run pytest tests/unit/test_rate_limit.py -v` still green (no regressions to task 01).
|
||||
- Coverage: **>90%** on the modified `app/api/auth.py` branches (429 hit/miss, record, reset paths all hit by the integration tests).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] The 11th rapid failed login from one client is a 429 with `Retry-After` (integration pin); the token route shares the counter (integration pin).
|
||||
- [ ] A success on a clean counter resets it (integration pin); a success while blocked does not unblock (integration pin).
|
||||
- [ ] All pre-existing `test_auth_api.py` pins green (the clean-counter fixture is documented).
|
||||
- [ ] `uv run ruff check . && uv run pyright` clean.
|
||||
@@ -0,0 +1,28 @@
|
||||
# Task 03 — Full gate + atomic commit
|
||||
|
||||
**Phase:** `81_login_rate_limit` · **Story:** n/a (security hardening — audit SEC-03)
|
||||
|
||||
## Objective
|
||||
Run the complete phase gate, land the phase as one atomic commit, and move the phase directory to `complete/`.
|
||||
|
||||
## Work
|
||||
1. **Full regression gate** (AGENTS.md rule 9):
|
||||
- `uv run pytest` — unit + integration green.
|
||||
- `uv run pytest --cov=app --cov-report=term-missing` — `app/` coverage **>90%**; confirm `app/core/rate_limit.py` and `app/api/auth.py` show no meaningful uncovered lines in the new branches.
|
||||
- `uv run pytest tests/e2e/test_smoke.py -v --no-cov` — green **in isolation** (this phase's E2E contract: no UI change — `login.js`'s existing non-204 rendering covers the 429).
|
||||
- `uv run ruff check . && uv run pyright` — clean.
|
||||
2. **Manual live check** (keep the output in the session log): start the dev server (`uv run uvicorn app.main:app`) and, from the shell, 11 rapid wrong-password logins: `for i in $(seq 11); do curl -s -o /dev/null -w "%{http_code} " -X POST localhost:8000/api/login -H 'Content-Type: application/json' -d '{"password":"wrong"}'; done` → expect `401 ×10, 429` (and the 429 body carries `Retry-After`). Restart the server to clear the counter (the per-process state is by design).
|
||||
3. **Commit** (AGENTS.md rule 8 — one atomic, Conventional-Commits commit, always `--no-gpg-sign`), staging `app/core/rate_limit.py`, `app/api/auth.py`, `tests/unit/test_rate_limit.py`, `tests/integration/test_auth_api.py`, and the phase files:
|
||||
`fix(auth): rate-limit failed logins and token-auth attempts with 429 + Retry-After`
|
||||
— body: security audit SEC-03 (2026-09-07) — neither login route throttled, so a reachable deployment took unlimited line-rate guesses; a stdlib-only sliding-window limiter (10 failures / IP / 15 min, shared by both routes, success resets, fail-open) now answers the 11th attempt with 429 + `Retry-After` while every non-throttled 401/204 is byte-identical. No dependency or frontend change.
|
||||
4. Move the phase directory: `mv .agents/phases/todo/81_login_rate_limit .agents/phases/complete/` and include the move in the same commit.
|
||||
|
||||
## Testing & Quality
|
||||
- This task IS the phase-level gate — the commands above are the completion evidence.
|
||||
- Coverage: >90% held.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] The live check shows `401 ×10, 429` (output kept in the session log); the 429 response carries `Retry-After`.
|
||||
- [ ] `uv run pytest` green; coverage >90%; `tests/e2e/test_smoke.py` green in isolation; ruff + pyright clean.
|
||||
- [ ] Exactly one new commit; `git show --stat HEAD` lists the five files above + the phase files (todo → complete move) — nothing else (in particular `pyproject.toml` / `uv.lock` / `frontend/` untouched).
|
||||
- [ ] `.agents/phases/complete/81_login_rate_limit/` exists; `todo/` no longer contains it.
|
||||
Reference in New Issue
Block a user