6 plans across 4 waves covering structlog/Loki observability, Locust load testing, multi-stage Dockerfile hardening, trusted-proxy rate limiting, and RUNBOOK.md. Verification passed (0 blockers). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
29 KiB
phase, plan, type, wave, depends_on, files_modified, autonomous, requirements, user_setup, must_haves
| phase | plan | type | wave | depends_on | files_modified | autonomous | requirements | user_setup | must_haves | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 06-performance-production-hardening | 05 | execute | 2 |
|
|
true |
|
|
Purpose: Header-spoofing currently lets any external client claim an arbitrary IP and bypass the IP-based rate limiter. D-11 closes that. D-12 adds a second axis (per-account) so a single compromised account cannot exhaust the application's request capacity by rotating IPs. Together they form the rate-limit half of Phase 6's hardening.
Output: One body-replacement in deps/utils.py (NO new function, NO new public API), two lines changed in auth.py, a new ~30-line rate_limiting.py service module, and decorator/first-line additions across every authenticated endpoint in documents.py and cloud.py.
<execution_context> @$HOME/.claude/get-shit-done/workflows/execute-plan.md @$HOME/.claude/get-shit-done/templates/summary.md </execution_context>
@.planning/phases/06-performance-production-hardening/06-CONTEXT.md @.planning/phases/06-performance-production-hardening/06-RESEARCH.md @.planning/phases/06-performance-production-hardening/06-PATTERNS.md @CLAUDE.md @backend/deps/utils.py @backend/api/auth.py @backend/api/documents.py @backend/api/cloud.py @backend/main.py @backend/tests/test_rate_limiting.py @backend/tests/conftest.pyExisting get_client_ip (backend/deps/utils.py:10):
- Signature:
def get_client_ip(request: Request) -> Optional[str]: - Currently returns
request.headers.get("X-Forwarded-For") or (request.client.host if request.client else None) - TASK: replace BODY only. Signature is unchanged. Add no new public functions.
ipaddressis stdlib — no new package dependency. - Existing callers: backend/api/auth.py, audit log helpers, etc. They import by name from deps.utils; the import chain stays identical.
Existing slowapi limiter (backend/api/auth.py:37–44):
from slowapi.util import get_remote_addresslimiter = Limiter(key_func=get_remote_address)- TASK: drop the slowapi.util import; add
from deps.utils import get_client_ip; changekey_func=get_remote_addresstokey_func=get_client_ip. Two-line patch. Existing @limiter.limit decorators stay exactly as written.
New shared module (backend/services/rate_limiting.py):
- Exports:
account_limiter: Limiter,_account_key(request: Request) -> str(module-private; reused only inside this file). _account_keyreadsgetattr(request.state, "current_user", None)and returnsstr(user.id)when set; falls back torequest.client.host or "anonymous"to avoid raising in untested code paths (Pitfall 3).- Why a separate module: CLAUDE.md mandates one canonical definition for shared utilities; both documents.py and cloud.py will import the same
account_limiterinstance so rate-limit counters are shared across routers.
Per-account decorator contract on each authenticated route:
request: RequestMUST appear in the signature (slowapi looks it up by name on the function's first parameters).@account_limiter.limit("100/minute")decorates the handler.- First line of handler body:
request.state.current_user = current_user(so _account_key has the user object available when slowapi evaluates the key — A1 assumption). - Apply to every route in documents.py and cloud.py that uses
Depends(get_regular_user).
Routes in documents.py to decorate (verified at planning time, lines 88, 129, 290, 399, 521, 565, 616, 693, 746):
POST /api/documents/upload-url
POST /api/documents/upload
POST /api/documents/{doc_id}/confirm
GET /api/documents
GET /api/documents/{doc_id}
PATCH /api/documents/{doc_id}
DELETE /api/documents/{doc_id}
POST /api/documents/{doc_id}/classify
GET /api/documents/{doc_id}/content
That's 9 endpoints. All currently take current_user: User = Depends(get_regular_user).
Routes in cloud.py to decorate (verified at planning time, those using get_regular_user — at lines 315, 407, 553, 644, 677, 728, 773, 928 and others): GET /api/cloud/oauth/initiate/{provider} GET /api/cloud/oauth/callback/{provider} POST /api/cloud/connections/webdav GET /api/cloud/connections GET /api/cloud/connections/{connection_id}/config DELETE /api/cloud/connections/{connection_id} GET /api/cloud/folders/{provider}/{folder_id:path} (… and any other route in cloud.py that injects get_regular_user — apply uniformly) Note: oauth_callback returns a RedirectResponse — still safe to apply the limit; per-account is rate-limit on internal users, not OAuth providers.
Wave 0 stubs to promote (backend/tests/test_rate_limiting.py — 8 xfails):
- test_get_client_ip_untrusted_returns_direct_peer
- test_get_client_ip_trusted_proxy_reads_xff_leftmost
- test_get_client_ip_trusted_proxy_no_xff_falls_back
- test_get_client_ip_invalid_peer_returns_none_or_string
- test_account_limiter_key_uses_user_id
- test_account_limiter_key_falls_back_to_ip_when_no_user
- test_account_limiter_key_ordering_assumption (A1 verification — gates D-12 rollout)
- test_authenticated_endpoint_429_after_100_per_minute
Create backend/services/rate_limiting.py (new file). Module docstring: "Per-account rate limiter shared across document and cloud routers (D-12)." Pattern: `from __future__ import annotations`, `from fastapi import Request`, `from slowapi import Limiter`. Define private function `_account_key(request: Request) -> str` per the behaviour spec — read `getattr(request.state, "current_user", None)`; when set return `str(user.id)`; when None fall back to `request.client.host if request.client else "anonymous"`. Define module-level singleton `account_limiter = Limiter(key_func=_account_key)`. Export both symbols (no __all__ required — they are top-level names).
Edit backend/tests/test_rate_limiting.py:
For tests 1–7 (the unit-level tests — get_client_ip variants, _account_key key selection, A1 ordering), REMOVE the `@pytest.mark.xfail(...)` decorators and REPLACE the single-line `pytest.xfail(...)` bodies with real assertions per the behaviour spec above. Use a lightweight mock Request (or starlette.requests.Request constructed from a minimal scope dict) — do NOT spin up the full app for unit tests 1–6.
For test 7 (A1 ordering): build a minimal FastAPI app inline (within the test), register one GET endpoint that sets `request.state.current_user = SimpleNamespace(id=uuid.uuid4())` as its first line, decorate it with `@account_limiter.limit("100/minute")`, mount via TestClient. Send 101 requests in a tight loop with the SAME peer IP but VARYING X-Forwarded-For; assert the 101st response is 429 (proves the key is the constant user.id, NOT the per-request IP). Skip integration test #8 in this task — Task 2 handles it.
Edit backend/main.py:
(a) Add an import line near the other service imports: `from services.rate_limiting import account_limiter`.
(b) Leave `app.state.limiter = auth_limiter` (or whatever name is currently used) UNCHANGED — SlowAPIMiddleware drives only the limiter assigned to app.state. The account_limiter is invoked directly via its decorators.
Edit backend/api/documents.py:
For EACH of the 9 endpoints listed in the <interfaces> block (request_upload_url, upload_document, confirm_upload, list_documents, get_document, patch_document, delete_document, classify_document, stream_document_content):
(a) Add at the top of the file (alongside the existing imports): `from services.rate_limiting import account_limiter`.
(b) Above each route function definition, immediately under the `@router.<verb>(...)` decorator, add `@account_limiter.limit("100/minute")`.
(c) Ensure the function signature has `request: Request` as a parameter (some handlers may already have it — keep). If a handler doesn't, ADD `request: Request` as the first parameter after any path/body params (FastAPI parameter order rules allow Request anywhere; slowapi finds it by type/name).
(d) Make `request.state.current_user = current_user` the FIRST executable statement of the handler body — before any other logic. This is the line that satisfies the A1 contract proven by Task 1's ordering test.
Edit backend/api/cloud.py:
Apply the same three changes (import account_limiter; decorate with @account_limiter.limit("100/minute"); first-line `request.state.current_user = current_user`) to EVERY endpoint in cloud.py whose signature contains `Depends(get_regular_user)`. Use `grep -n "Depends(get_regular_user)" backend/api/cloud.py` to enumerate the targets and ensure 100% coverage. The endpoint at /api/cloud/oauth/callback/{provider} returns a RedirectResponse — that's fine; rate-limit applies to the authenticated user regardless of response type.
Edit backend/tests/test_rate_limiting.py:
Remove the `@pytest.mark.xfail(...)` decorator from test_authenticated_endpoint_429_after_100_per_minute. Replace its single-line body with real assertions: use the async_client fixture from conftest.py and the auth_user fixture, hit GET /api/documents/ 101 times in a tight loop with the same Bearer token, assert the 101st response is 429. Implementation note: because slowapi's default in-memory storage is per-process, the test must run within a single test client lifetime; reset isn't required because each test gets a fresh app from conftest.
Do NOT modify any endpoint signature beyond adding `request: Request` where missing. Do NOT change @router.<verb>(...) URL paths, body models, response models, or dependency lists.
<threat_model>
Trust Boundaries
| Boundary | Description |
|---|---|
| External client → FastAPI | Client controls all HTTP headers, including X-Forwarded-For; FastAPI must distinguish trusted-proxy origin from arbitrary internet origin |
| Reverse proxy → FastAPI (intra-cluster) | Trusted CIDR; X-Forwarded-For is authoritative for the originating client |
| Authenticated user → application capacity | A compromised account rotating IPs bypasses the IP limiter; per-account limit closes that hole |
STRIDE Threat Register
| Threat ID | Category | Component | Disposition | Mitigation Plan |
|---|---|---|---|---|
| T-06-05-01 | Tampering | Rate-limit bypass via X-Forwarded-For spoofing from external clients | mitigate | get_client_ip body replaced with trusted-proxy CIDR check (D-11); external clients can no longer claim arbitrary IPs |
| T-06-05-02 | Denial of Service | Compromised account exhausts request capacity by rotating IPs (defeats per-IP limit) | mitigate | account_limiter keyed by str(current_user.id) caps each user at 100 req/min across all decorated endpoints (D-12) |
| T-06-05-03 | Tampering | account_limiter key_func runs BEFORE request.state.current_user is set → silently falls back to IP, defeats D-12 | mitigate | A1 verification test (test_account_limiter_key_ordering_assumption) in Task 1 — the first-line request.state.current_user = current_user contract is verified before applying decorators to all endpoints in Task 2 |
| T-06-05-04 | Denial of Service | _account_key raises when request.state.current_user is unset → crashes request handling | mitigate | _account_key returns IP fallback or "anonymous" string instead of raising (Pitfall 3) |
| T-06-05-05 | Information Disclosure | Per-account 429 reveals authenticated user enumeration to attacker | accept | The 429 only fires for already-authenticated users; the attacker already proved knowledge of credentials. Information leak is bounded by the existing per-IP limit on login endpoints (preserved by D-13). |
| T-06-05-06 | Tampering | Duplicate get_client_ip definitions drift apart over time (CLAUDE.md anti-pattern) | mitigate | Replace BODY in-place; no new function created; grep gate verifies single canonical definition |
| </threat_model> |
<success_criteria>
- D-11 satisfied: get_client_ip uses trusted-proxy CIDR validation; external clients can no longer spoof XFF.
- D-12 satisfied: account_limiter Limiter keyed by user.id; all documents.py + cloud.py authenticated endpoints decorated; per-account 100/minute enforced.
- D-13 satisfied: existing per-IP limits on auth endpoints (10/min, 5/hour) preserved verbatim; only the key_func changed.
- All 8 Wave 0 xfails promoted to PASS — including A1 ordering verification.
- Zero new failures in the full backend pytest suite. </success_criteria>