From e627f50f66f6a491ec38f5621a2dce068c27e9df Mon Sep 17 00:00:00 2001 From: root Date: Tue, 8 Sep 2026 12:48:47 +0000 Subject: [PATCH] no-mistakes(document): Document P0 auth batch; reconcile HARDENING statuses; lint fixes --- AGENTS.md | 8 ++- HARDENING.md | 99 +++++++++++++++++-------------- app/routers/tickets.py | 2 +- app/schemas/auth.py | 2 +- tests/conftest.py | 2 +- tests/test_p0_auth_admin_batch.py | 3 +- 6 files changed, 67 insertions(+), 49 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0b6ea6e..15fd99c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -105,8 +105,12 @@ Frontend: Alpine.js (CDN) + Tailwind CSS (CDN). Auth state in localStorage. Role - Role checks, admin user creation, login, and JWT validation all derive from the role module; unknown/junk roles (e.g. lowercase `admin`/`superadmin` from the old open register) fail closed at login/JWT and can never be recreated via the - API (422). `seed_users` normalizes unambiguous legacy aliases to canonical on - every startup (`normalize_legacy_user_roles`). + API (422). Startup self-heals (lifespan in `app/main.py`, helpers in + `app/services/seed.py`) converge legacy rows: `normalize_legacy_user_roles` + maps unambiguous alias nicknames onto canonical roles, and + `normalize_legacy_user_emails` lowercases stored emails — login and the admin + create-user duplicate check both compare on the lowercased form, so pre-P0 + mixed-case emails are never silently locked out. - Use `require_roles(*ADMIN_ROLES)` for admin gates; `sub` claim holds string user ID - Security headers middleware in `app/main.py`: X-Frame-Options DENY + nosniff on everything, CSP on HTML pages, HSTS when `X-Forwarded-Proto: https` diff --git a/HARDENING.md b/HARDENING.md index 923aaec..27a7b5c 100644 --- a/HARDENING.md +++ b/HARDENING.md @@ -19,15 +19,18 @@ legacy-schema self-heal. **32 pytest tests pass.** - **Live:** container `denya-onecare` on LXC `scottdenya` (192.168.68.75:8000), image built 2026-08-02, `restart: unless-stopped`. -- **Known demo-only posture (must change):** `SECRET_KEY=change-me-in-production`, - `CORS_ORIGINS=*`, open `/api/auth/register`, SQLite backend, WhatsApp webhook +- **Demo-only posture resolved (PR #10 + P0 batch):** `SECRET_KEY` now fails + closed without a real key, CORS is an explicit origin allow-list, and open + self-registration was removed — `POST /api/auth/register` → 404, users are + admin-managed only (see AGENTS.md). +- **Known demo-only posture (must change):** SQLite backend; WhatsApp webhook code is present but **no real credentials wired**. --- ## 1. P0 — Security blockers (do these FIRST, before any real data) -### P0.1 Hardcode-safe secrets; never ship the default key +### P0.1 Hardcode-safe secrets; never ship the default key — **DONE (PR #10)** - **Files:** `docker-compose.yml`, `app/core/config.py` - Replace the hardcoded `SECRET_KEY=change-me-in-production` default with a fail-closed default: if `SECRET_KEY` is unset/empty or still the well-known @@ -37,7 +40,7 @@ - **Acceptance:** starting the app without a real key fails loudly; container env contains a strong random key (≥32 bytes, e.g. `openssl rand -hex 32`). -### P0.2 Lock down CORS +### P0.2 Lock down CORS — **DONE (PR #10)** - **Files:** `app/main.py`, `docker-compose.yml` - `CORS_ORIGINS=*` + `allow_credentials=True` is an invalid/unsafe combo (browsers reject `*` with credentials anyway). Replace with an explicit @@ -47,18 +50,21 @@ - **Acceptance:** `settings.CORS_ORIGINS` is a comma-separated explicit list; the middleware builds an allow-list, not `["*"]`. -### P0.3 Gate user registration -- **File:** `app/routers/auth.py` (`POST /api/auth/register`) -- Today anyone on the network can self-register. Decide the model: - - **Recommended:** require an admin-issued invitation token, or restrict - registration to a seed/allowed list, or remove the open route and create - users only via seed/admin. - - If a public self-service resident/tenant signup is genuinely required - (Phase 2 QR/self-service), it must be a SEPARATE endpoint with a **role - default of the least-privilege role** and rate-limiting — never able to mint - admin/FM roles. -- **Acceptance:** a raw, unauthenticated register call can no longer mint an - `Admin/*` or `Director` account. +### P0.3 Gate user registration — **DONE (P0 batch, 2026-09)** +- Open `POST /api/auth/register` was removed entirely (404) — there is no sign-up UI. +- Users are admin-managed: `POST /api/auth/users` (Admin/Jerome + Admin/Wahab only) + creates users with a **forced canonical role** (unknown roles → 422); + `PATCH /api/auth/users/{id}` changes role / deactivates (self-modification → 400); + `DELETE /api/auth/users/{id}` is guarded (users referenced by + tickets/timeline/escalations → 409); duplicate email → 409. +- Emails are normalized (strip + lowercase) on every write path; a startup + self-heal lowercases legacy mixed-case rows so pre-P0 accounts can't be locked + out of login. Unified role model lives in `app/core/roles.py`. +- **Regression tests:** `tests/test_p0_auth_admin_batch.py`. +- If a public self-service resident/tenant signup is genuinely required later + (Phase 2 QR/self-service), it must be a SEPARATE endpoint with a **role + default of the least-privilege role** and rate-limiting — never able to mint + admin/FM roles. ### P0.4 Reconsider SQLite for the final product - **Files:** `docker-compose.yml`, `app/core/database.py`, `app/core/config.py`, PRD §16 @@ -71,21 +77,21 @@ - **Acceptance:** `pytest` green against Postgres (tests param via conftest), Alembic applies cleanly on a fresh Postgres DB. -### P0.5 WhatsApp webhook auth + hardening (finish wiring, then lock it) +### P0.5 WhatsApp webhook auth + hardening — **PARTIAL (P0 batch, 2026-09)** - **File:** `app/routers/whatsapp.py` -- The handler exists but no credentials are set. When wiring this week: - - Verify the `hub.verify_token` check is constant-time (compare with - `secrets.compare_digest`). **The GET verification path currently returns - `{"error": ...}` with HTTP 200** — flip to `403` on token mismatch. - - Validate **inbound messages only from Meta** — the webhook MUST authenticate - Meta's request signature (X-Hub-Signature-256 HMAC over the raw body with your - app secret) before processing, otherwise anyone who discovers the endpoint can - forge tickets. This is the single most important WhatsApp hardening item. - - Add per-sender rate limiting / dedupe on `wa_message_id` (webhook retries can - double-create tickets). Create an idempotency guard keyed on `wa_message_id`. - - Never log the raw access token; redact in `send_whatsapp_reply` error paths. -- **Acceptance:** a forged POST without the Meta signature is rejected; duplicate - `wa_message_id` does not create a second ticket; verify-token mismatch returns 403. +- **Done:** the `GET` handshake validates `hub.verify_token` with a constant-time + compare and returns **403 on mismatch**; inbound `POST`s are gated by the + `X-Webhook-Secret` header matching `WHATSAPP_WEBHOOK_SECRET` (fail-closed 403 + when the env var is unset — see `.env.example`); the debug + `GET /api/whatsapp/mock-log` now requires Bearer auth. +- **Still open:** Meta request-signature validation (`X-Hub-Signature-256` HMAC + over the raw body with the app secret — the shared-secret header above is the + interim gate); per-sender rate limiting / dedupe idempotency keyed on + `wa_message_id` (retries can still double-create tickets); redacting the raw + access token in `send_whatsapp_reply` error paths. +- **Acceptance (open items):** a forged POST without the Meta signature is + rejected; duplicate `wa_message_id` does not create a second ticket; + verify-token mismatch returns 403 (done). --- @@ -130,10 +136,13 @@ `X-Content-Type-Options: nosniff`. ### P1.6 API hardening & rate limiting -- Add rate limiting on `POST /api/auth/login` (brute-force) — per-IP/IP+account. -- Consider rate limits on ticket creation (spam / mass-creation). -- Normalize/validate `page_size` (already capped `le=200`) and pagination - tie-breaker (`id DESC` present — good). +- **Done (P0 batch):** `POST /api/auth/login` is rate-limited in-process — + ~5 failures / 15 min per IP+email → 429 (env-tunable `LOGIN_RATE_LIMIT_*`, + a successful login resets the window). **Open:** ticket-creation rate limiting + (spam / mass-creation). +- **Done (P0 batch):** ticket-list pagination — `page`/`page_size` (default 50, + cap 200), `limit` alias for `page_size` also capped; passing both with + different values → 422. Tie-breaker `id DESC` present. --- @@ -145,9 +154,9 @@ Assumes Denya provides: **phone number ID, access token, verify token, app secre `WHATSAPP_VERIFY_TOKEN`, `WHATSAPP_APP_SECRET`, `META_GRAPH_BASE`) to `.env` (git-ignored) and inject at runtime. Never commit. 2. **Webhook handshake:** in Meta dashboard point the webhook URL at - `/api/whatsapp/webhook`. The GET verify path currently echoes - `hub.challenge` when the verify token matches — confirm this works, then apply - P0.5 (403 on mismatch, HMAC signature validation). + `/api/whatsapp/webhook`. The GET verify path echoes `hub.challenge` + when the verify token matches (constant-time compare; mismatch → 403). The + still-open P0.5 work is the Meta signature validation in step 3. 3. **Verify incoming signature** (P0.5) — use `X-Hub-Signature-256` = HMAC-SHA256 of the raw body with your app secret, compared with `compare_digest`. 4. **Reply flow:** confirm `send_whatsapp_reply` posts correctly to @@ -194,14 +203,18 @@ Assumes Denya provides: **phone number ID, access token, verify token, app secre ## 5. Definition of Done (production-ready) -- [ ] No default `SECRET_KEY`; app fails closed without a real key -- [ ] CORS is an explicit origin allow-list -- [ ] Self-registration cannot mint privileged roles (or is admin-gated/removed) +- [x] No default `SECRET_KEY`; app fails closed without a real key (PR #10) +- [x] CORS is an explicit origin allow-list (PR #10) +- [x] Self-registration removed (register → 404); users are admin-managed only + with forced canonical roles (P0 batch) - [ ] Postgres backend; Alembic applies cleanly on fresh DB; nightly backups - [ ] TLS-terminated reverse proxy with real domain; no raw :8000 on WAN -- [ ] WhatsApp webhook: Meta signature validated, verify-token mismatch → 403, - idempotent on `wa_message_id`, real credentials injected at runtime -- [ ] Login/ticket rate limiting in place +- [x] Webhook POST gated by `X-Webhook-Secret` (fail-closed); verify-token + mismatch → 403; `mock-log` requires auth +- [ ] WhatsApp webhook: Meta `X-Hub-Signature-256` HMAC validated; idempotent on + `wa_message_id`; real credentials injected at runtime +- [x] Login rate limiting in place (~5 fails / 15 min per IP+email → 429) +- [ ] Ticket-creation rate limiting in place - [ ] Photo uploads size-limited and content-sniffed - [ ] RBAC audited per-route; phone data access controlled - [ ] Expanded test suite (auth, WhatsApp, SLA boundary, uploads) — all green diff --git a/app/routers/tickets.py b/app/routers/tickets.py index 808e925..9a60eee 100644 --- a/app/routers/tickets.py +++ b/app/routers/tickets.py @@ -226,7 +226,7 @@ async def list_tickets( """ if page_size is not None and limit is not None and page_size != limit: raise HTTPException( - status_code=422, # noqa: PLR2004 — param ``status`` shadows fastapi.status here + status_code=422, # ``status`` query param shadows fastapi.status in this scope detail="Provide either 'page_size' or 'limit', not both", ) effective_page_size = page_size if page_size is not None else (limit or DEFAULT_PAGE_SIZE) diff --git a/app/schemas/auth.py b/app/schemas/auth.py index d776e84..6f2e089 100644 --- a/app/schemas/auth.py +++ b/app/schemas/auth.py @@ -92,7 +92,7 @@ class AdminUpdateUserRequest(BaseModel): return _validate_canonical_role(value) @model_validator(mode="after") - def _at_least_one_field(self) -> "AdminUpdateUserRequest": + def _at_least_one_field(self) -> AdminUpdateUserRequest: if self.role is None and self.active is None: raise ValueError("Provide at least one of 'role' or 'active'") return self diff --git a/tests/conftest.py b/tests/conftest.py index 28386d2..87fd46a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -17,8 +17,8 @@ os.environ["DATABASE_URL"] = f"sqlite+aiosqlite:///{_TMP_DIR}/test.db" os.environ.setdefault("SECRET_KEY", "test-secret-key-not-for-production-0123456789abcdef") os.environ.setdefault("CORS_ORIGINS", "http://test") -import pytest_asyncio # noqa: E402 (DATABASE_URL must be set before app imports) import pytest # noqa: E402 +import pytest_asyncio # noqa: E402 (DATABASE_URL must be set before app imports) from httpx import ASGITransport, AsyncClient # noqa: E402 from app.core.database import Base, async_session_factory, engine # noqa: E402 diff --git a/tests/test_p0_auth_admin_batch.py b/tests/test_p0_auth_admin_batch.py index c46b20e..6448362 100644 --- a/tests/test_p0_auth_admin_batch.py +++ b/tests/test_p0_auth_admin_batch.py @@ -429,9 +429,10 @@ async def test_rate_limit_is_per_email(client: AsyncClient): async def test_rate_limit_window_expires(client: AsyncClient, monkeypatch): """After the 15-minute window passes, the account can log in again.""" - import app.core.ratelimit as ratelimit_mod import time as _time + import app.core.ratelimit as ratelimit_mod + email, password = "window@example.com", "password1" token = await _login(client) await client.post(