feat(security): OAuth CSRF nonce + redirect allowlist [needs user_id review] (#11)

[ianpatrickhines]

Addresses #11 — CSRF protection + redirect_uri allowlist for the NationBuilder OAuth flow. Flagged for review, not auto-merged: one open security question + deploy-packaging gaps (below).

What's implemented (committed)

• src/lambdas/shared/oauth_state.py — single-use CSRF nonce: server-generated (secrets.token_urlsafe(32)), persisted in a new OAuthStateTable (DynamoDB, TTL, DeletionPolicy: Retain), bound to (user_id, nb_slug, redirect_uri).
• New src/lambdas/nb_oauth_init/handler.py (+ API route GET /auth/nationbuilder) — issuance side: mints the nonce, then 302s to NB authorize. redirect_uri is fixed server-side from OAUTH_CALLBACK_URL (can't be client-influenced).
• nb_oauth_callback — validates + atomically consumes the nonce and enforces a redirect_uri allowlist before exchanging the code.
• nb_slug format validation before the authorize redirect.

Verified independently

• pytest -q → 573 passed (+31); mypy clean (25 files); cfn-lint clean
• CSRF core is sound: nonce is consumed via delete_item + ConditionExpression=attribute_exists(nonce) + ReturnValues=ALL_OLD — atomic single-use, no replay/TOCTOU. redirect_uri fixed server-side. Tests cover forged/missing/replayed state.
• Extension already targets /auth/nationbuilder (the new init route) — no extension change needed (the example.com host is the Consolidate to one domain, replace placeholder URLs, load real secrets #14 placeholder).

⚠️ Open security question (why this is needs-ian-judgment, not ready-to-merge)

nb_oauth_init accepts a caller-supplied user_id (query_params.get("user_id") or generate). Classic OAuth session-fixation: an attacker could mint a nonce bound to their own user_id, lure a victim through authorize, and link the victim's nation under the attacker's user_id. My analysis is this is likely inert given #10 (tokens are per-nation; the session token that grants access is minted at the callback and delivered to the victim's browser; agent authz derives nation from the session-token claims, not the UsersTable link) — but I could not conclusively clear it, and the adversarial reviewer stalled before reaching a verdict. Recommended: harden nb_oauth_init to never trust an unverified user_id (generate server-side for fresh connects; for re-auth derive user_id from a verified existing session token), then merge.

Deploy-packaging gaps (tracked under deploy readiness, not blockers for code review)

• deploy.yml doesn't package the new nb_oauth_init Lambda.
• Since Add API authorization to public endpoints (AuthorizationType: NONE) #10/Add CSRF protection and redirect_uri allowlist to NB OAuth callback #11, nb_oauth_callback imports shared/ but its zip step doesn't bundle shared/ (the agent functions do). Both oauth Lambdas need the shared-bundling pattern before first deploy. Noted on Consolidate to one domain, replace placeholder URLs, load real secrets #14.

Net: this is strictly more secure than main (which had no CSRF), but I'm holding it for the user_id call.

[ianpatrickhines]

🧭 Needs Ian's judgment — security code is airtight; deploy packaging is a cross-cutting blocker

TL;DR: The actual security work in this PR (issue #11's acceptance criteria) is complete and independently verified as sound. The reason this isn't a clean "merge it" is a systemic Lambda-packaging bug that would crash these (and already-merged) functions at deploy time. That fix touches code outside #11's scope and is an architecture decision I think is yours to make.

✅ What's verified good (issue #11 acceptance criteria — all met)

• CSRF nonce, single-use: server-minted secrets.token_urlsafe(32), persisted in a new OAuthStateTable (DynamoDB, TTL, DeletionPolicy: Retain), consumed via an atomic conditional delete_item (attribute_exists(nonce) + ReturnValues=ALL_OLD). No TOCTOU, no replay.
• Tamper check: the client copy of (user_id, nb_slug, redirect_uri) must match the authoritative server-side record.
• Expiry: enforced against the stored created_at, not the forgeable client copy.
• redirect_uri allowlist: exact-match, enforced at both issuance and callback; empty allowlist = deny-all.
• Tests: forged / replayed / expired / tampered state and redirect_uri rejection are all exercised.

Evidence: pytest → 53/53 OAuth tests pass (564 pass overall; the 9 "failures" are Python-3.14 async-mock artifacts on my local box — CI on 3.11 is green). mypy clean (25 files). cfn-lint clean. A fresh adversarial reviewer with no context, told to refute, concluded the CSRF/redirect design is "genuinely solid… airtight" and could not break it.

➕ What I added this run (in scope, verified)

Closed the open user_id session-fixation question the prior run flagged. nb_oauth_init now always mints user_id server-side and ignores any caller-supplied value — trusting it was a login-CSRF vector. Zero compatibility cost: the extension never sends user_id. New test test_supplied_user_id_is_ignored proves it. (This commit is staged locally on the branch; I was unable to push it in this run — see note at bottom.)

⛔ Why this needs your call: a systemic flattened-Lambda import bug

Both OAuth handlers (and the already-merged nat_agent / nat_agent_streaming) import shared code via from src.lambdas.shared.…. The deploy pipeline flattens the zip so shared/ sits at the root with no src/ tree, so that import resolves to ModuleNotFoundError at cold start. The handlers' except ModuleNotFoundError → from shared.… fallback does not save it, because importing the shared package runs src/lambdas/shared/__init__.py, which itself does an unconditional from src.lambdas.shared.subscription_middleware import … and throws uncaught.

Reproduced against the flattened layout:

from shared.oauth_state import issue_oauth_state
  File ".../shared/__init__.py", line 3, in <module>
    from src.lambdas.shared.subscription_middleware import (...)
ModuleNotFoundError: No module named 'src'

This is pre-existing and not created by #11 — nat_agent/nat_agent_streaming (already merged) have the same latent break (from src.lambdas.shared.usage_tracking at top level, no fallback). The unit tests pass only because pytest runs from repo root where src is a real package; nothing exercises the flattened import path the Lambda actually uses.

🔧 Recommended decision

1. Treat Add CSRF protection and redirect_uri allowlist to NB OAuth callback #11's security code as correct and mergeable on its own merits — the acceptance criteria are met and verified.
2. Fix the packaging/import issue holistically under its own issue (deploy-readiness, Consolidate to one domain, replace placeholder URLs, load real secrets #14) before any deploy, since it spans nat_agent, nat_agent_streaming, and both OAuth lambdas. Cleanest fix: switch shared/__init__.py (and the agent handlers' top-level imports) to relative imports (from .subscription_middleware import …) so they resolve in both layouts, plus a smoke test that imports each handler from the flattened zip layout so this can't regress silently. Also: deploy.yml doesn't yet bundle shared/ into the OAuth zips or package the new nb_oauth_init function — both needed before first deploy.
3. Minor, non-blocking: admin_email is no longer captured on the nation record (now None); restoring it from NB userinfo is a noted follow-up.

I did not fold the packaging fix into this PR on purpose: bundling a cross-cutting infra fix for already-merged functions into a CSRF PR would muddy review and make it hard to revert independently. Happy to take that on as a dedicated PR if you'd like — just say the word.

— autonomous pr-driver

[ianpatrickhines]

Update (2026-06-22): the recommended hardening is now implemented and pushed — nb_oauth_init no longer trusts a caller-supplied user_id (generated server-side for fresh connects; derived from a verified session token for re-auth), which closes the session-fixation question this PR was held on. 573 tests pass, mypy clean. Ready for your review to clear the needs-ian-judgment hold.