From bd3ef269d4dd88cd7144fcd9d513fcbf6ac147fb Mon Sep 17 00:00:00 2001 From: Ben Stull Date: Thu, 28 May 2026 18:19:41 -0700 Subject: [PATCH] Release v0.27.0: security hardening (audit 0026) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remediates the rfc-app application + deploy-config findings from the Session 0026 security audit. Cut as the "v0.25.0-security-hardening" branch (from v0.24.0); reversioned to 0.27.0 on rebase onto main since v0.26.0 (#28) shipped while this was in flight. - C1 (Critical): single sanitizeHtml.js chokepoint (DOMPurify) for every marked→innerHTML / dangerouslySetInnerHTML sink (MarkdownPreview, ProposalView x2, Editor); rel=noopener hook on target=_blank links. - H1: per-account OTC-verify lockout (migration 023, auto-applied) + per-IP throttle via new ratelimit.py; wired on otc verify/request + passcode check/verify. - M1: device_trust.lookup() single indexed-row read — cookie value is now "."; bcrypt-checks one row, not a global table scan. (Behavior change: existing device-trust cookies re-prompt once.) - M2: HTTP security headers (CSP/HSTS/XFO/XCTO/Referrer-Policy) at nginx. - M4: session cookie Secure-by-default (SESSION_COOKIE_SECURE opt-out). - M5: bounce webhook fails CLOSED (503) when secret unset, instead of open; RFC_APP_INSECURE_BOUNCE_WEBHOOK=1 dev opt-in. - L2/L3: per-IP cooldown + check-endpoint throttle. - L4: systemd sandbox knobs. L8/I1: nginx server_tokens off + TLS1.0/1.1 out. VERSION + frontend/package.json → 0.27.0; CHANGELOG documents the upgrade steps (incl. the out-of-band nginx + systemd apply, which the flotilla deploy gesture does not perform). Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 62 +++++++++++++ VERSION | 2 +- backend/app/api_notifications.py | 21 ++++- backend/app/device_trust.py | 53 ++++++----- backend/app/main.py | 78 ++++++++++++---- backend/app/otc.py | 76 +++++++++++++++ backend/app/ratelimit.py | 92 +++++++++++++++++++ backend/migrations/023_otc_verify_lockout.sql | 18 ++++ backend/tests/conftest.py | 19 ++++ backend/tests/test_propose_vertical.py | 12 +++ deploy/nginx/ohm.wiggleverse.org.conf | 50 ++++++++++ deploy/systemd/rfc-app.service | 27 ++++++ frontend/package-lock.json | 5 +- frontend/package.json | 3 +- frontend/src/components/Editor.jsx | 4 +- frontend/src/components/MarkdownPreview.jsx | 3 +- frontend/src/components/ProposalView.jsx | 6 +- frontend/src/lib/sanitizeHtml.js | 47 ++++++++++ 18 files changed, 528 insertions(+), 50 deletions(-) create mode 100644 backend/app/ratelimit.py create mode 100644 backend/migrations/023_otc_verify_lockout.sql create mode 100644 backend/tests/conftest.py create mode 100644 frontend/src/lib/sanitizeHtml.js diff --git a/CHANGELOG.md b/CHANGELOG.md index fcd0abf..7ad846c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,68 @@ skip versions are the composition of each intervening adjacent release's steps in order — no A-to-B path is pre-computed beyond that. +## 0.27.0 — 2026-05-28 + +**Minor — security-hardening release (Session-0026 audit remediation). +One auto-applied migration (023); one behavior change that re-prompts +device-trust; deployments MUST re-apply the nginx + systemd files.** +This is the work cut as the "v0.25.0 security-hardening" branch; it +reversioned to 0.27.0 because v0.26.0 (#28) took the next slot while it +was in flight. Shipped from driver session 0030.0. + +- **C1 (Critical) — stored-XSS closed.** Every markdown→HTML sink now + routes through one chokepoint, `frontend/src/lib/sanitizeHtml.js` + (DOMPurify), before any `innerHTML` / `dangerouslySetInnerHTML` write: + `MarkdownPreview`, both `ProposalView` sinks (entry body + + `proposed_use_case`), and `Editor`. A hook adds + `rel="noopener noreferrer"` to `target=_blank` links. `marked` no + longer passes raw HTML / `javascript:` URIs to the DOM, so a + contributor can no longer plant a payload that runs in an admin/owner + session during review. +- **H1 — OTC verify is rate-limited.** New `backend/app/ratelimit.py` + (per-IP token buckets) gates `/auth/otc/verify`, `/auth/otc/request`, + and the passcode check/verify paths; a per-account OTC-verify lockout + (migration `023_otc_verify_lockout.sql`) mirrors the passcode lockout. +- **M1 — device-trust lookup no longer table-scans.** The device-trust + cookie value is now `"."`; `device_trust.lookup` + reads the one indexed row and bcrypt-checks only it, instead of + bcrypt-checking every row in the table on each unauthenticated + `/auth/device-trust/start`. +- **M2 — HTTP security headers** (CSP, HSTS, X-Frame-Options, + X-Content-Type-Options, Referrer-Policy) added to the nginx server + block. **L8/I1**: `server_tokens off` + legacy TLS1.0/1.1 removed. +- **M4 — session cookie `Secure` by default** (`SESSION_COOKIE_SECURE`, + defaults on; a dev box on plain http sets it `false`). +- **M5 — bounce webhook fails closed.** An unset + `WEBHOOK_EMAIL_BOUNCE_SECRET` now **disables** `/api/webhooks/email-bounce` + (503) instead of leaving it open; a dev opts back in with + `RFC_APP_INSECURE_BOUNCE_WEBHOOK=1`. +- **L2/L3** per-IP cooldown + check-endpoint throttle. **L4** systemd + sandbox knobs (`CapabilityBoundingSet=`, `ProtectKernel*`, + `RestrictAddressFamilies`, `SystemCallFilter`, …). + +Upgrade steps: + +1. **Migration** — none manual; `023_otc_verify_lockout.sql` auto-applies + at startup via `db.run_migrations`. +2. **Device trust (MUST expect re-prompt)** — the cookie format changed, + so existing "trusted device" cookies no longer match; affected users + are re-prompted for device verification once. No data migration; old + rows are simply never matched and age out. +3. **nginx + systemd (MUST apply out-of-band)** — the deploy gesture does + **not** install `deploy/nginx/ohm.wiggleverse.org.conf` or + `deploy/systemd/rfc-app.service`. After deploying the code, copy both + to their system locations, then `nginx -t && systemctl reload nginx` + and `systemctl daemon-reload && systemctl restart `. (M2 headers + and L4 sandboxing do not take effect until this is done.) +4. **Bounce webhook (SHOULD)** — bind `WEBHOOK_EMAIL_BOUNCE_SECRET` (or + set `RFC_APP_INSECURE_BOUNCE_WEBHOOK=1` for dev). Unset → the endpoint + returns 503 (closed). No legitimate bounce source is wired today, so + 503 is the safe default. +5. **Session cookie (SHOULD, dev only)** — a deployment served over plain + http MUST set `SESSION_COOKIE_SECURE=false` or the session cookie + won't be sent. Production over HTTPS leaves it unset (Secure on). + ## 0.26.0 — 2026-05-28 **Minor — no schema migration, no new secret, no config, no upgrade diff --git a/VERSION b/VERSION index 4e8f395..1b58cc1 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.26.0 +0.27.0 diff --git a/backend/app/api_notifications.py b/backend/app/api_notifications.py index a97558d..01f6adc 100644 --- a/backend/app/api_notifications.py +++ b/backend/app/api_notifications.py @@ -557,7 +557,26 @@ def make_router(config: Config) -> APIRouter: # stays unauthenticated for dev (the v1 contract). import os as _os expected = _os.environ.get("WEBHOOK_EMAIL_BOUNCE_SECRET", "").strip() - if expected: + # v0.25.0 (audit 0026 M5): fail closed. An unset secret used to + # leave this endpoint fully unauthenticated — anyone could suppress + # any user's mail by POSTing their address (email_opt_out_all flip + # below). Now an unset secret DISABLES the endpoint (503) instead + # of opening it. A dev that genuinely wants it open opts in + # explicitly with RFC_APP_INSECURE_BOUNCE_WEBHOOK=1, mirroring the + # RFC_APP_INSECURE_WEBHOOKS dev-bypass on the Gitea hook. + if not expected: + if _os.environ.get("RFC_APP_INSECURE_BOUNCE_WEBHOOK", "").strip() == "1": + log.warning( + "email-bounce webhook running UNAUTHENTICATED " + "(RFC_APP_INSECURE_BOUNCE_WEBHOOK=1) — never set this in production" + ) + else: + log.error( + "email-bounce webhook refused: WEBHOOK_EMAIL_BOUNCE_SECRET is unset " + "(set the secret to enable, or RFC_APP_INSECURE_BOUNCE_WEBHOOK=1 for dev)" + ) + raise HTTPException(503, "Bounce webhook not configured") + else: received = request.headers.get("X-Webhook-Secret", "") import hmac as _hmac if not received or not _hmac.compare_digest(expected, received): diff --git a/backend/app/device_trust.py b/backend/app/device_trust.py index 3ffdc1c..f70c6ea 100644 --- a/backend/app/device_trust.py +++ b/backend/app/device_trust.py @@ -97,6 +97,13 @@ class IssueOutcome: raw_token: str row_id: int + @property + def cookie_value(self) -> str: + """The value to put in the `rfc_device_trust` cookie: the row-id + selector joined to the raw token (v0.25.0 / audit 0026 M1). The + selector lets `lookup` read one indexed row instead of scanning.""" + return f"{self.row_id}.{self.raw_token}" + def _new_token() -> str: return secrets.token_urlsafe(TOKEN_BYTES) @@ -173,33 +180,37 @@ def lookup(raw_token: str) -> LookupOutcome: if not raw: return LookupOutcome(ok=False, user=None, reason="invalid") - # The unique index on `device_token_hash` would let us SELECT by - # hash if bcrypt were a stable hash, but bcrypt incorporates a - # per-row salt — equal tokens produce different hashes. We walk - # the candidate set instead. In practice the set is small (a - # human has a handful of trusted devices) and bcrypt is cheap on - # the order of milliseconds; the walk is bounded by the user's - # active device count. + # v0.25.0 (audit 0026 M1): the cookie is ".". We + # parse the row-id selector and read exactly ONE row by its indexed + # primary key, then bcrypt-check the token against that single row. # - # We don't pre-filter by `revoked_at IS NULL` here so that a - # token presented for a recently-revoked row produces a - # 'revoked' outcome (the endpoint surfaces a different shape). - # Same for expired: we let the walk hit and classify after. - rows = db.conn().execute( + # The previous shape read EVERY device_trust row (all users, including + # revoked/expired) and bcrypt-checked each — an unauthenticated + # CPU-amplification DoS reachable at /auth/device-trust/start that + # grew without bound as the table accumulated. bcrypt's per-row salt + # is why we can't SELECT by hash; carrying the row-id in the cookie is + # the standard fix (the id is not secret; the token still is). + selector, sep, token = raw.partition(".") + if not sep or not selector.isdigit() or not token: + # Legacy bare-token cookies (pre-v0.25.0) and malformed values land + # here. We refuse rather than fall back to a full-table scan, so + # the amplification path is fully closed; affected users simply + # re-authenticate once via OTC/passcode and get a new cookie. + return LookupOutcome(ok=False, user=None, reason="invalid") + + matched = db.conn().execute( """ SELECT id, user_id, device_token_hash, expires_at, revoked_at FROM device_trust - ORDER BY id DESC + WHERE id = ? """, - ).fetchall() + (int(selector),), + ).fetchone() - matched = None - for row in rows: - if _check(raw, row["device_token_hash"]): - matched = row - break - - if matched is None: + # One bcrypt check, against the selected row only. A wrong/forged token + # for a real id reads as 'unknown' (cookie cleared), same as a missing + # row — a probing client can't distinguish the two. + if matched is None or not _check(token, matched["device_token_hash"]): return LookupOutcome(ok=False, user=None, reason="unknown") if matched["revoked_at"] is not None: diff --git a/backend/app/main.py b/backend/app/main.py index faa0a7d..18d99c2 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -7,6 +7,7 @@ no need for a separate worker. from __future__ import annotations import logging +import os import secrets from contextlib import asynccontextmanager @@ -28,6 +29,7 @@ from . import ( otc, passcode as passcode_mod, providers as providers_mod, + ratelimit, turnstile, webhooks, ) @@ -142,12 +144,20 @@ def create_app() -> FastAPI: # eagerly via load_config(). Everything else waits for lifespan. config = load_config() app = FastAPI(lifespan=lifespan) + # v0.25.0 (audit 0026 M4): the session cookie is the primary 30-day + # auth credential and must carry `Secure` in production so it never + # travels cleartext. Default to Secure; a dev box serving over plain + # http opts out with SESSION_COOKIE_SECURE=false. Production (OHM is + # HTTPS-only with an HTTP->HTTPS 301) leaves this unset → Secure on. + session_secure = os.environ.get("SESSION_COOKIE_SECURE", "true").strip().lower() not in ( + "0", "false", "no", "off", + ) app.add_middleware( SessionMiddleware, secret_key=config.secret_key, session_cookie="rfc_session", max_age=60 * 60 * 24 * 30, - https_only=False, + https_only=session_secure, ) return app @@ -155,24 +165,25 @@ def create_app() -> FastAPI: app = create_app() -def _set_device_trust_cookie(response: Response, raw_token: str) -> None: +def _set_device_trust_cookie(response: Response, cookie_value: str) -> None: """Attach the v0.11.0 device-trust cookie to the response. - HttpOnly + Secure + SameSite=Lax + 30-day Max-Age + Path=/. The - cookie value is the raw token; server-side storage is the hash. - The cookie is "essential" per the v0.13.0 cookie-consent contract - (it is part of authentication), so we set it regardless of the - user's analytics / other-cookies choice. + HttpOnly + Secure + SameSite=Lax + 30-day Max-Age + Path=/. As of + v0.25.0 (audit 0026 M1) the value is `IssueOutcome.cookie_value` — + "." — so `device_trust.lookup` can read one indexed + row instead of scanning; server-side storage remains the bcrypt hash + of the token half only. The cookie is "essential" per the v0.13.0 + cookie-consent contract (it is part of authentication), so we set it + regardless of the user's analytics / other-cookies choice. - Secure=True means the cookie is only ever sent over HTTPS. The - SessionMiddleware in `create_app` keeps `https_only=False` for - dev parity, but the device-trust cookie holds a 30-day credential - and must not travel cleartext — production deployments serve over - HTTPS, so Secure on the device-trust cookie is non-negotiable. + Secure=True means the cookie is only ever sent over HTTPS — the + device-trust cookie holds a 30-day credential and must never travel + cleartext. (The session cookie now also defaults to Secure; see M4 in + `create_app`.) """ response.set_cookie( key=device_trust_mod.COOKIE_NAME, - value=raw_token, + value=cookie_value, max_age=device_trust_mod.COOKIE_MAX_AGE_SECONDS, path="/", secure=True, @@ -245,6 +256,10 @@ def _oauth_router(config) -> APIRouter: @router.post("/auth/otc/request") async def otc_request(body: OtcRequestBody, request: Request): + # v0.25.0 (audit 0026 H1/L2): per-IP brake at the cheapest point, + # before the Turnstile network call or any bcrypt/SMTP work. + if not ratelimit.otc_request_limiter.allow(ratelimit.client_key(request)): + raise HTTPException(429, "Too many requests; please wait a few minutes") # v0.12.0 / roadmap item #10: gate the request on a successful # Turnstile siteverify before the bcrypt hash + SMTP send. The # check runs first so a failed challenge spends no rate budget @@ -278,9 +293,25 @@ def _oauth_router(config) -> APIRouter: @router.post("/auth/otc/verify") async def otc_verify(body: OtcVerifyBody, request: Request, response: Response): + # v0.25.0 (audit 0026 H1): per-IP brake against fan-out guessing, + # plus the per-email lockout enforced inside otc.verify_code. + ip = ratelimit.client_key(request) + if not ratelimit.verify_limiter.allow(ip): + raise HTTPException(429, "Too many attempts; please wait a few minutes") result = otc.verify_code(body.email, body.code) + if result.reason == "locked": + raise HTTPException( + 423, + { + "detail": "Too many failed attempts; wait a few minutes or request a new code", + "locked_until": result.locked_until, + }, + ) if not result.ok or result.user is None: raise HTTPException(400, "Invalid or expired code") + # Legit sign-in: clear this IP's window so a user who fat-fingered + # a couple of codes isn't left throttled. + ratelimit.verify_limiter.reset(ip) auth.store_session(request, result.user) # v0.8.0: surface `needs_profile` so the Login.jsx surface can # decide whether to advance to the first/last/why capture step @@ -313,7 +344,7 @@ def _oauth_router(config) -> APIRouter: if body.trust_device: ua = request.headers.get("user-agent", "") outcome = device_trust_mod.issue(result.user.user_id, ua) - _set_device_trust_cookie(response, outcome.raw_token) + _set_device_trust_cookie(response, outcome.cookie_value) return { "ok": True, "user": { @@ -337,12 +368,17 @@ def _oauth_router(config) -> APIRouter: # --------------------------------------------------------------- @router.get("/auth/passcode/check") - async def passcode_check(email: str = ""): + async def passcode_check(request: Request, email: str = ""): """Does this email have a passcode set? Anonymous endpoint — the Login.jsx flow calls this after the user types their email to decide whether to render a passcode input or fall back to OTC. We surface only the boolean; lockout state, the hash, and - the set-at stamp are not leaked here.""" + the set-at stamp are not leaked here. + + v0.25.0 (audit 0026 L3): per-IP rate limit so the has-passcode + boolean can't be bulk-harvested to enumerate accounts.""" + if not ratelimit.check_limiter.allow(ratelimit.client_key(request)): + raise HTTPException(429, "Too many requests; please wait a few minutes") status = passcode_mod.passcode_status(email) return {"has_passcode": status.has_passcode} @@ -376,6 +412,11 @@ def _oauth_router(config) -> APIRouter: v0.11.0: the body's `trust_device` flag, if true, mints a fresh device-trust row and sets the long-lived cookie. Same opt-in contract as `/auth/otc/verify`.""" + # v0.25.0 (audit 0026 H1): per-IP brake in front of the per-account + # passcode lockout, so fan-out across emails is throttled too. + ip = ratelimit.client_key(request) + if not ratelimit.verify_limiter.allow(ip): + raise HTTPException(429, "Too many attempts; please wait a few minutes") result = passcode_mod.verify_passcode(body.email, body.passcode) if result.reason == "locked": raise HTTPException( @@ -387,11 +428,12 @@ def _oauth_router(config) -> APIRouter: ) if not result.ok or result.user is None: raise HTTPException(400, "Invalid passcode") + ratelimit.verify_limiter.reset(ip) auth.store_session(request, result.user) if body.trust_device: ua = request.headers.get("user-agent", "") outcome = device_trust_mod.issue(result.user.user_id, ua) - _set_device_trust_cookie(response, outcome.raw_token) + _set_device_trust_cookie(response, outcome.cookie_value) return { "ok": True, "user": { @@ -459,7 +501,7 @@ def _oauth_router(config) -> APIRouter: if body.trust_device: ua = request.headers.get("user-agent", "") outcome = device_trust_mod.issue(result.user.user_id, ua) - _set_device_trust_cookie(response, outcome.raw_token) + _set_device_trust_cookie(response, outcome.cookie_value) # Has the user already set a passcode? (Could only happen via # an admin pre-population path that doesn't exist yet, but diff --git a/backend/app/otc.py b/backend/app/otc.py index 000eb6b..e02d146 100644 --- a/backend/app/otc.py +++ b/backend/app/otc.py @@ -85,6 +85,15 @@ def _cooldown_seconds() -> int: return 60 +# v0.25.0 / security audit 0026 (H1): per-email OTC verify lockout, +# mirroring the passcode path (passcode.py). Five consecutive wrong codes +# for an email lock its OTC verify for 15 minutes. The per-IP limiter in +# ratelimit.py is the primary brute-force brake; this is the durable, +# passcode-parity layer. +LOCKOUT_AFTER_FAILED_ATTEMPTS = 5 +LOCKOUT_DURATION_MINUTES = 15 + + # --------------------------------------------------------------------------- # Code generation + hashing # --------------------------------------------------------------------------- @@ -206,6 +215,61 @@ class VerifyOutcome: ok: bool user: SessionUser | None reason: str + # v0.25.0 (H1): ISO-8601 stamp when reason == 'locked'. + locked_until: str | None = None + + +def _verify_lockout_until(email: str) -> str | None: + """Return the active lockout stamp for `email`, or None if not locked. + + Clears an elapsed lockout (and resets the counter) as a side effect so + the next failure starts a fresh budget — mirrors passcode.verify_passcode. + """ + row = db.conn().execute( + "SELECT failed_attempts, locked_until FROM otc_verify_state WHERE email = ?", + (email,), + ).fetchone() + if row is None or not row["locked_until"]: + return None + still_locked = db.conn().execute( + "SELECT datetime(?) > datetime('now') AS locked", (row["locked_until"],), + ).fetchone()["locked"] + if still_locked: + return row["locked_until"] + db.conn().execute( + "UPDATE otc_verify_state SET failed_attempts = 0, locked_until = NULL WHERE email = ?", + (email,), + ) + return None + + +def _record_verify_failure(email: str) -> None: + """Increment the per-email failure counter; stamp a lockout once it + crosses the threshold. Mirrors the passcode lockout shape.""" + db.conn().execute( + """ + INSERT INTO otc_verify_state (email, failed_attempts) + VALUES (?, 1) + ON CONFLICT(email) DO UPDATE SET failed_attempts = failed_attempts + 1 + """, + (email,), + ) + count = db.conn().execute( + "SELECT failed_attempts FROM otc_verify_state WHERE email = ?", (email,), + ).fetchone()["failed_attempts"] + if count >= LOCKOUT_AFTER_FAILED_ATTEMPTS: + db.conn().execute( + f""" + UPDATE otc_verify_state + SET locked_until = datetime('now', '+{LOCKOUT_DURATION_MINUTES} minutes') + WHERE email = ? + """, + (email,), + ) + + +def _clear_verify_state(email: str) -> None: + db.conn().execute("DELETE FROM otc_verify_state WHERE email = ?", (email,)) def verify_code(email: str, code: str) -> VerifyOutcome: @@ -214,6 +278,13 @@ def verify_code(email: str, code: str) -> VerifyOutcome: if not email or not code: return VerifyOutcome(ok=False, user=None, reason="invalid") + # v0.25.0 (H1): refuse before spending any bcrypt if this email is in + # its OTC-verify lockout window. The passcode path is unaffected — a + # locked-out OTC user can still set/use a passcode, and vice versa. + locked_until = _verify_lockout_until(email) + if locked_until: + return VerifyOutcome(ok=False, user=None, reason="locked", locked_until=locked_until) + rows = db.conn().execute( """ SELECT id, code_hash, expires_at, consumed_at @@ -237,6 +308,9 @@ def verify_code(email: str, code: str) -> VerifyOutcome: break if matched is None: + # A genuine wrong guess against this email — the brute-force + # signal. Count it toward the lockout threshold (H1). + _record_verify_failure(email) return VerifyOutcome(ok=False, user=None, reason="wrong") if matched["consumed_at"] is not None: @@ -255,6 +329,8 @@ def verify_code(email: str, code: str) -> VerifyOutcome: "UPDATE otc_codes SET consumed_at = datetime('now') WHERE id = ?", (matched["id"],), ) + # Success wipes the per-email failure counter (H1). + _clear_verify_state(email) user = provision_or_link_user(email) return VerifyOutcome(ok=True, user=user, reason="ok") diff --git a/backend/app/ratelimit.py b/backend/app/ratelimit.py new file mode 100644 index 0000000..41850f3 --- /dev/null +++ b/backend/app/ratelimit.py @@ -0,0 +1,92 @@ +"""In-process per-IP sliding-window rate limiter (security audit 0026, H1). + +The auth verify endpoints (`/auth/otc/verify`, `/auth/passcode/verify`) +had no per-IP brake, so an attacker could fan out guesses against a +target identity bounded only by bcrypt cost. This module is the brake. + +It is deliberately tiny: §4.2 says the app is a single process with a +colocated SQLite file, so an in-memory dict of `key -> deque[timestamps]` +is sufficient and needs no shared store. State resets on restart, which +fails *open* for a brief window — acceptable because the per-email OTC +lockout (`otc_verify_state`) and the passcode lockout both persist in the +database and carry the durable guarantee; this limiter is the +anti-fan-out layer on top. + +Chosen over a per-identity lockout *as the primary control* because a +per-IP window throttles the attacker without letting them grief a victim +by locking that victim's account (the known downside of identity +lockouts). Both layers run together. +""" +from __future__ import annotations + +import threading +import time +from collections import defaultdict, deque + + +class SlidingWindowLimiter: + """Allow at most `max_events` per `window_seconds` per key. + + `allow(key)` records an event and returns True if the key is still + within budget, False if it has exceeded it. Timestamps use a + monotonic clock so the limiter is immune to wall-clock jumps. + """ + + def __init__(self, max_events: int, window_seconds: float) -> None: + self.max_events = max_events + self.window_seconds = window_seconds + self._events: dict[str, deque[float]] = defaultdict(deque) + self._lock = threading.Lock() + + def allow(self, key: str) -> bool: + now = time.monotonic() + cutoff = now - self.window_seconds + with self._lock: + q = self._events[key] + while q and q[0] < cutoff: + q.popleft() + if len(q) >= self.max_events: + return False + q.append(now) + # Opportunistic cleanup so idle keys don't accumulate forever. + if not q: + self._events.pop(key, None) + return True + + def reset(self, key: str) -> None: + """Drop a key's window — e.g. after a successful sign-in so a + legitimate user who fat-fingered a few times isn't throttled.""" + with self._lock: + self._events.pop(key, None) + + +# Module-level limiters shared across requests (one process, so module +# state is the natural home). Tunables are intentionally generous enough +# not to bother a human retyping a code, tight enough to kill fan-out: +# * verify: 10 attempts / 5 min / IP across the auth verify surfaces. +# * otc request: 5 sends / 5 min / IP (Turnstile is the primary gate; +# this is defense in depth against a solved-challenge replay loop). +verify_limiter = SlidingWindowLimiter(max_events=10, window_seconds=300) +otc_request_limiter = SlidingWindowLimiter(max_events=5, window_seconds=300) +# /auth/passcode/check is an anonymous has-passcode oracle (audit 0026 L3). +# It's a legitimate Login-flow affordance, so the budget is generous — +# enough for a human typing emails, tight enough to stop bulk scraping. +check_limiter = SlidingWindowLimiter(max_events=30, window_seconds=300) + + +def _reset_all_for_tests() -> None: + """Clear every module-level limiter's window. Test support only — the + limiters are process-global singletons, so without a per-test reset + one test's requests bleed into the next and later tests trip the + budget (429). Not called in production.""" + for lim in (verify_limiter, otc_request_limiter, check_limiter): + with lim._lock: + lim._events.clear() + + +def client_key(request) -> str: + """Best-effort client identity for limiting. Behind nginx the app is + started with `--forwarded-allow-ips 127.0.0.1`, so `request.client.host` + reflects the real client IP via Uvicorn's ProxyHeaders handling.""" + client = getattr(request, "client", None) + return client.host if client and client.host else "unknown" diff --git a/backend/migrations/023_otc_verify_lockout.sql b/backend/migrations/023_otc_verify_lockout.sql new file mode 100644 index 0000000..be8b6e2 --- /dev/null +++ b/backend/migrations/023_otc_verify_lockout.sql @@ -0,0 +1,18 @@ +-- v0.25.0 / security audit 0026, finding H1. +-- +-- The OTC verify path had no attempt-limit or lockout, unlike the +-- passcode path (015_passcode.sql gave users.passcode_failed_attempts + +-- passcode_locked_until). This table gives the OTC verify endpoint the +-- same per-identity lockout shape. It is keyed by email rather than +-- user_id because an OTC sign-in may not have a users row yet — the row +-- is provisioned only on a *successful* verify, so the lockout state has +-- to survive independently of it. +-- +-- The per-IP rate limiter (app/ratelimit.py) is the primary brute-force +-- defense; this table is the parity layer that mirrors the passcode +-- lockout and persists across restarts. +CREATE TABLE IF NOT EXISTS otc_verify_state ( + email TEXT PRIMARY KEY, + failed_attempts INTEGER NOT NULL DEFAULT 0, + locked_until TEXT +); diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py new file mode 100644 index 0000000..33fe961 --- /dev/null +++ b/backend/tests/conftest.py @@ -0,0 +1,19 @@ +"""Shared pytest fixtures for the backend suite. + +Added in v0.27.0 (security audit 0026) alongside the new per-IP rate +limiter. The limiters in `app.ratelimit` are process-global singletons, +so their state survives across tests within a run; without a reset, the +accumulated requests from earlier tests exhaust the budget and later +tests see spurious 429s. This autouse fixture gives every test a clean +limiter window. +""" +import pytest + +from app import ratelimit + + +@pytest.fixture(autouse=True) +def _reset_rate_limiters(): + ratelimit._reset_all_for_tests() + yield + ratelimit._reset_all_for_tests() diff --git a/backend/tests/test_propose_vertical.py b/backend/tests/test_propose_vertical.py index 5bcf73a..c55f2f4 100644 --- a/backend/tests/test_propose_vertical.py +++ b/backend/tests/test_propose_vertical.py @@ -444,6 +444,18 @@ def tmp_env(monkeypatch): # the dev-bypass path monkeypatch `RFC_APP_INSECURE_WEBHOOKS=1`. "GITEA_WEBHOOK_SECRET": "test-webhook-secret-for-signature-verification", "ENABLED_MODELS": "claude", + # v0.27.0 (audit 0026 M4): the session cookie now defaults to + # Secure. The TestClient talks plain http://testserver, so a + # Secure cookie is never sent back and every authenticated flow + # would fail. Tests opt out explicitly, exactly as a dev box on + # plain http does. + "SESSION_COOKIE_SECURE": "false", + # v0.27.0 (audit 0026 M5): the bounce webhook fails closed (503) + # when its secret is unset. Tests exercise the legacy behavioral + # path via the documented dev opt-in, mirroring the + # RFC_APP_INSECURE_WEBHOOKS bypass above. Tests that assert the + # fail-closed default delenv this key themselves. + "RFC_APP_INSECURE_BOUNCE_WEBHOOK": "1", } for k, v in env.items(): monkeypatch.setenv(k, v) diff --git a/deploy/nginx/ohm.wiggleverse.org.conf b/deploy/nginx/ohm.wiggleverse.org.conf index 3b108c1..83e1df3 100644 --- a/deploy/nginx/ohm.wiggleverse.org.conf +++ b/deploy/nginx/ohm.wiggleverse.org.conf @@ -18,6 +18,56 @@ server { listen [::]:80; server_name ohm.wiggleverse.org; + # v0.25.0 security hardening (audit 0026 M2/L8) + # + # NOTE: certbot promotes THIS server block to the HTTPS listener + # (`listen 443 ssl`) and adds a separate port-80 → 443 redirect + # block (see the install comment above). These response headers + # therefore ride into the HTTPS server block on the VM. They use + # `add_header ... always` so they also apply to nginx-generated + # error responses (4xx/5xx), not just 200s. + # + # `server_tokens off` (L8) — suppress the nginx version in the + # Server header and on error pages so we don't advertise the + # build to scanners. + server_tokens off; + + add_header Strict-Transport-Security "max-age=31536000; includeSubDomains" always; + add_header X-Frame-Options "DENY" always; + add_header X-Content-Type-Options "nosniff" always; + add_header Referrer-Policy "strict-origin-when-cross-origin" always; + + # Content-Security-Policy (M2). Tuned to what the SPA actually loads: + # - default-src 'self': everything not called out below is same-origin. + # - script-src 'self' + challenges.cloudflare.com: the only external + #