Skip to content

feat(middleware): double-submit CSRF protection for cookie auth (#443) [stacked on #478] - #479

Open
olegbrok wants to merge 1 commit into
feat/rate-limit-stackedfrom
feat/csrf-stacked
Open

feat(middleware): double-submit CSRF protection for cookie auth (#443) [stacked on #478]#479
olegbrok wants to merge 1 commit into
feat/rate-limit-stackedfrom
feat/csrf-stacked

Conversation

@olegbrok

Copy link
Copy Markdown
Collaborator

Summary

PR-8 of the #433 web-exposed hardening track. Defense-in-depth on top of `SameSite=Strict` for cookie-authenticated state-changing routes in lan/web modes.

Stacked on #478#477#476#475#474#473#472.

Closes part of #433. Refs #443.

Pattern

Double-submit cookie + header:

  1. Server issues `pinky_csrf` cookie alongside any session cookie that doesn't already have one. Value = 32-byte URL-safe random, `SameSite=Strict`, NOT HttpOnly (SPA reads it).
  2. Frontend reads cookie, sends as `X-Pinky-CSRF` header on every state-changing request.
  3. Middleware on unsafe verbs (POST/PUT/PATCH/DELETE) compares header vs cookie via `secrets.compare_digest`. Mismatch → `HTTP 403 {"error":"csrf_check_failed"}`.

Mode behaviour

Mode CSRF enforcement Cookie `Secure` flag
trusted off (back-compat) n/a
lan on False (localhost dev has no TLS)
web on True

Exemptions

  • Bearer-token auth — no ambient credential vulnerability
  • Internal MCP-signed requests (`X-Pinky-Agent` + signature headers)
  • `/triggers/webhook/*` — token-gated upstream
  • `/twilio/*` — Twilio signature gated
  • Anonymous requests (no session cookie → nothing to forge)

Token rotation

Session-bound. Whenever a request carries a session cookie but no CSRF cookie, the response sets a fresh CSRF token. Login (which sets a new session) naturally rotates. Explicit `/auth/csrf/refresh` is out of scope for this PR.

Audit

Every CSRF rejection emits `auth.csrf.rejected` into #440's security audit log with ip / via_proxy / forwarded_chain / method / target / user_agent / `detail={"reason": "missing"|"mismatch"}`. The redaction allowlist was extended in `security_audit.py` to register the new event type.

Frontend follow-up (not in this PR)

`src/lib/csrf.ts` — fetch wrapper that reads `pinky_csrf` cookie and injects `X-Pinky-CSRF`. Lands when the SPA next gets a substantive update; in the meantime, the trusted-mode default on the Mac Mini is unaffected, and any new web deployment can wire CSRF in via env config.

Tests (20 new)

  • Pure helpers: `UNSAFE_METHODS`, `generate_csrf_token` randomness, `is_exempt_path`
  • Middleware enforcement: trusted pass-through, safe methods pass, unsafe POST without/mismatch/match, bearer exempts, internal-signed exempts, webhook + twilio paths exempt, no-session no-enforcement
  • Token issuance: session-no-cookie gets one (samesite=strict, httponly=false), session-with-cookie doesn't reissue, no session no token, Secure flag mode-specific
  • Audit emission: reject with reason=missing, mismatch reason=mismatch
  • E2E `create_api`: trusted mode = no enforcement, no token issuance

523 tests green across the full hardening suite + existing api tests.

Test plan

  • `pytest tests/test_csrf.py -v` (20 passing)
  • `pytest tests/test_*.py` (no regression)
  • `ruff check src/pinky_daemon/middleware/csrf.py src/pinky_daemon/api.py tests/test_csrf.py` (clean)
  • Manual: `PINKY_DEPLOYMENT_MODE=web`; login, then POST /admin/... without X-Pinky-CSRF → 403; with the cookie value → 200
  • Manual: confirm `auth.csrf.rejected` event appears in security_audit_log

🤖 Opened by Barsik

PR-8 of the #433 web-exposed hardening track. Defense-in-depth on top
of SameSite=Strict for cookie-authenticated state-changing routes in
lan/web modes. Stacked on #478#477#476#475#474#473#472.

Pattern
=======
Double-submit cookie + header:

1. Server issues `pinky_csrf` cookie alongside any session cookie that
   doesn't already have one. Value = 32-byte URL-safe random,
   SameSite=Strict, NOT HttpOnly (frontend reads it).
2. Frontend reads cookie, sends as `X-Pinky-CSRF` header on every
   state-changing request.
3. Middleware on unsafe verbs (POST/PUT/PATCH/DELETE) compares header
   vs cookie via secrets.compare_digest. Mismatch → 403
   {"error":"csrf_check_failed"}.

Why not just SameSite=Strict
============================
* Older browsers without enforcement
* Multi-window cross-tab leakage
* Subdomain-trust attacks if PinkyBot ever shares a parent domain
* OAuth/redirect flows that intentionally use SameSite=Lax

#439's `/auth/elevate` and other admin POSTs are exactly the
high-value targets CSRF protects.

Mode behaviour
==============
* trusted — disabled (back-compat)
* lan + web — enforced

In LAN the cookie is set with Secure=False (localhost dev doesn't have
TLS); in WEB it's Secure=True.

Exemptions
==========
* Bearer-token auth (Authorization: Bearer ...) — no ambient
  credential vulnerability
* Internal MCP-signed requests (X-Pinky-Agent + X-Pinky-Signature)
* `/triggers/webhook/*` (token-gated upstream)
* `/twilio/*` (Twilio signature gated)
* Anonymous requests (no session cookie → nothing to forge)

Token rotation
==============
Session-bound. Whenever a request carries a session cookie but no CSRF
cookie, the response sets a fresh CSRF token. Login (which sets a new
session) naturally rotates: the next response sees a session without
CSRF and writes a new token. Explicit `/auth/csrf/refresh` is out of
scope for this PR.

Audit
=====
Every CSRF rejection emits `auth.csrf.rejected` into the security
audit log (#440) with ip, via_proxy, forwarded_chain, method, target,
user_agent, and a redacted detail blob {"reason": "missing"|"mismatch"}.
The redaction allowlist was extended in security_audit.py to register
the new event type.

Tests (20 new)
==============
* Pure helpers: UNSAFE_METHODS, generate_csrf_token randomness,
  is_exempt_path.
* Middleware enforcement: trusted pass-through, safe methods
  (GET/HEAD/OPTIONS) pass, unsafe POST without/with mismatch/with
  match, bearer auth exempts, internal-signed exempts, webhook + twilio
  paths exempt, no-session no-enforcement.
* Token issuance: session without CSRF gets one (samesite=strict,
  httponly=false), session with CSRF doesn't reissue, no session no
  token, Secure flag only in web mode.
* Audit: reject emits event with method/target/reason; mismatch
  distinguishable from missing.
* E2E create_api: trusted mode no token issuance + no enforcement.

523 tests green across the full hardening suite + existing api tests.

Closes part of #433. Refs #443.

🤖 Authored by Barsik
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant