fix: 15 review findings -- cookie validators, persist-before-cookie, lockout TTL, audit logs

Aureliolo · Aureliolo · commit 63f0a438fde4 · 2026-04-07T10:05:31.000+02:00
Config validators:
- Reject SameSite=None without Secure (browser requirement)
- Reject colliding cookie names (session/csrf/refresh must be distinct)

Controller:
- Persist refresh token before appending cookie (skip cookie on failure)
- Fix CSRF cookie lifetime note in docs/security.md (cascade revocation)

Security hardening:
- Wrap SimpleCookie parsing in try/except (malformed cookies = absent)
- Lockout load_locked() restores remaining TTL, not fresh full duration
- Log warning on missing authentication before raising 401
- Fix retry_after=0 truthiness (use 'is not None' check)
- Wrap lockout store init in try/except (non-fatal on failure)
- CHECK(used IN (0,1)) constraint on refresh_tokens
- Gate SSE 401 handler with dev bypass check

Audit logging:
- Add INFO logs to RefreshStore create/revoke_by_session/revoke_by_user
- New event constants: API_AUTH_REFRESH_CREATED, API_AUTH_REFRESH_REVOKED

Tests:
- Assert both session + CSRF cookies in integration test
- Validate _extract_auth_cookies returns both cookies
- Fix fetchUser test for new short-circuit logic
- Wrap getCsrfToken decodeURIComponent in try/catch
diff --git a/docs/security.md b/docs/security.md
@@ -70,7 +70,7 @@ on resolution.
 - **HttpOnly cookie sessions** -- JWTs are delivered via HttpOnly, Secure, SameSite=Strict cookies (never exposed to JavaScript). Password changes rotate the session cookie so the embedded `pwd_sig` stays current.
 - **CSRF protection** -- double-submit cookie pattern: a non-HttpOnly CSRF cookie is set alongside the session cookie; JavaScript reads it and sends it as the `X-CSRF-Token` header on mutating requests. The middleware validates header-vs-cookie match using constant-time comparison.
 - **Account lockout** -- after exceeding a configurable threshold of failed login attempts within a sliding window, the account is temporarily locked (HTTP 429 with `Retry-After` header). Lockout state is restored from SQLite on restart.
-- **Refresh token rotation** -- optional single-use refresh tokens with replay detection (reuse of a consumed token triggers a warning).
+- **Refresh token rotation** -- optional single-use refresh tokens with replay detection; reuse of a consumed token logs the event and the affected session's refresh tokens are cascade-revoked.
 - **Concurrent session limits** -- configurable maximum active sessions per user; oldest sessions are revoked when the limit is exceeded.
 - **JWT bearer tokens** with password-change detection (`pwd_sig` claim, skipped for system user)
 - **System user (CLI)** -- internal identity bootstrapped at startup with a random Argon2id password hash. CLI tokens use `sub: "system"` with `iss: "synthorg-cli"` and skip `pwd_sig` validation (JWT HMAC signature is the sole authentication gate). The system user cannot log in, change its password, or be modified through the API.
diff --git a/src/synthorg/api/auth/config.py b/src/synthorg/api/auth/config.py
@@ -186,6 +186,33 @@ def _validate_refresh_expiry(self) -> Self:
             raise ValueError(msg)
         return self
 
+    @model_validator(mode="after")
+    def _validate_cookie_settings(self) -> Self:
+        """Reject invalid cookie configuration combinations.
+
+        - ``SameSite=None`` requires ``Secure=True`` (browser
+          requirement).
+        - Cookie names must be distinct to avoid collisions.
+        """
+        if self.cookie_samesite == "none" and not self.cookie_secure:
+            msg = (
+                "cookie_secure must be True when "
+                "cookie_samesite is 'none' (browser requirement)"
+            )
+            raise ValueError(msg)
+        names = [
+            self.cookie_name,
+            self.csrf_cookie_name,
+            self.refresh_cookie_name,
+        ]
+        if len(set(names)) != len(names):
+            msg = (
+                "cookie_name, csrf_cookie_name, and "
+                "refresh_cookie_name must all be distinct"
+            )
+            raise ValueError(msg)
+        return self
+
     def with_secret(self, secret: str) -> AuthConfig:
         """Return a copy with the JWT secret set.
 
diff --git a/src/synthorg/api/auth/controller.py b/src/synthorg/api/auth/controller.py
@@ -262,10 +262,9 @@ async def _make_session_cookies(  # noqa: PLR0913
     if config.jwt_refresh_enabled:
         refresh_token = secrets.token_urlsafe(32)
         refresh_max_age = config.jwt_refresh_expiry_minutes * 60
-        cookies.append(
-            make_refresh_cookie(refresh_token, refresh_max_age, config),
-        )
-        # Persist hashed refresh token for single-use validation.
+        # Persist hashed refresh token BEFORE setting the cookie.
+        # Only append the cookie if persistence succeeds.
+        refresh_persisted = False
         if app_state is not None and session_id and user_id:
             try:
                 from synthorg.api.auth.refresh_store import (  # noqa: PLC0415, TC001
@@ -286,6 +285,12 @@ async def _make_session_cookies(  # noqa: PLR0913
                         user_id=user_id,
                         expires_at=refresh_expiry,
                     )
+                    refresh_persisted = True
+                else:
+                    logger.warning(
+                        API_AUTH_FAILED,
+                        reason="refresh_store_not_available",
+                    )
             except MemoryError, RecursionError:
                 raise
             except Exception:
@@ -294,6 +299,18 @@ async def _make_session_cookies(  # noqa: PLR0913
                     reason="refresh_token_persist_failed",
                     exc_info=True,
                 )
+        else:
+            logger.warning(
+                API_AUTH_FAILED,
+                reason="refresh_token_persist_skipped",
+                has_app_state=app_state is not None,
+                has_session_id=bool(session_id),
+                has_user_id=bool(user_id),
+            )
+        if refresh_persisted:
+            cookies.append(
+                make_refresh_cookie(refresh_token, refresh_max_age, config),
+            )
     return cookies
 
 
diff --git a/src/synthorg/api/auth/csrf.py b/src/synthorg/api/auth/csrf.py
@@ -174,7 +174,11 @@ def _parse_cookies(
     result: dict[str, str] = {}
     for name, value in headers:
         if name.lower() == b"cookie":
-            morsel = SimpleCookie(value.decode("latin-1"))
+            try:
+                morsel = SimpleCookie(value.decode("latin-1"))
+            except Exception:  # noqa: S112
+                # Malformed cookie header -- treat as absent.
+                continue
             for key, m in morsel.items():
                 result[key] = m.value
     return result
diff --git a/src/synthorg/api/auth/lockout_store.py b/src/synthorg/api/auth/lockout_store.py
@@ -87,21 +87,29 @@ async def load_locked(self) -> int:
         now = datetime.now(UTC)
         window_start = (now - self._window).isoformat()
         cursor = await self._db.execute(
-            "SELECT username, COUNT(*) AS cnt "
+            "SELECT username, COUNT(*) AS cnt, "
+            "MAX(attempted_at) AS max_attempted_at "
             "FROM login_attempts "
             "WHERE attempted_at >= ? "
             "GROUP BY username "
             "HAVING cnt >= ?",
             (window_start, self._threshold),
         )
         rows = await cursor.fetchall()
+        mono_now = time.monotonic()
         restored = 0
         for row in rows:
             uname = row["username"]
             uname = uname.lower()
             if uname not in self._locked:
-                self._locked[uname] = time.monotonic() + self._duration_seconds
-                restored += 1
+                max_at = datetime.fromisoformat(
+                    row["max_attempted_at"],
+                )
+                locked_until = max_at + self._duration
+                remaining = (locked_until - now).total_seconds()
+                if remaining > 0:
+                    self._locked[uname] = mono_now + remaining
+                    restored += 1
         if restored:
             logger.info(
                 API_AUTH_ACCOUNT_LOCKED,
diff --git a/src/synthorg/api/auth/middleware.py b/src/synthorg/api/auth/middleware.py
@@ -127,6 +127,11 @@ async def authenticate_request(
                 raise NotAuthorizedException(
                     detail="Invalid session cookie",
                 )
+            logger.warning(
+                API_AUTH_FAILED,
+                reason="missing_authentication",
+                path=path,
+            )
             raise NotAuthorizedException(
                 detail="Missing authentication",
             )
diff --git a/src/synthorg/api/auth/refresh_store.py b/src/synthorg/api/auth/refresh_store.py
@@ -18,7 +18,9 @@
 from synthorg.observability.events.api import (
     API_AUTH_REFRESH_CLEANUP,
     API_AUTH_REFRESH_CONSUMED,
+    API_AUTH_REFRESH_CREATED,
     API_AUTH_REFRESH_REJECTED,
+    API_AUTH_REFRESH_REVOKED,
 )
 
 logger = get_logger(__name__)
@@ -96,6 +98,11 @@ async def create(
             ),
         )
         await self._db.commit()
+        logger.info(
+            API_AUTH_REFRESH_CREATED,
+            session_id=session_id,
+            user_id=user_id,
+        )
 
     async def consume(
         self,
@@ -193,7 +200,14 @@ async def revoke_by_session(self, session_id: str) -> int:
             (session_id,),
         )
         await self._db.commit()
-        return cursor.rowcount
+        count = cursor.rowcount
+        if count:
+            logger.info(
+                API_AUTH_REFRESH_REVOKED,
+                session_id=session_id,
+                revoked=count,
+            )
+        return count
 
     async def revoke_by_user(self, user_id: str) -> int:
         """Mark all refresh tokens for a user as used.
@@ -209,7 +223,14 @@ async def revoke_by_user(self, user_id: str) -> int:
             (user_id,),
         )
         await self._db.commit()
-        return cursor.rowcount
+        count = cursor.rowcount
+        if count:
+            logger.info(
+                API_AUTH_REFRESH_REVOKED,
+                user_id=user_id,
+                revoked=count,
+            )
+        return count
 
     async def cleanup_expired(self) -> int:
         """Remove expired and used tokens.
diff --git a/src/synthorg/api/exception_handlers.py b/src/synthorg/api/exception_handlers.py
@@ -348,9 +348,12 @@ def handle_api_error(
         msg = type(exc).default_message
     else:
         msg = str(exc)
-    retry_after_val: int | None = getattr(exc, "retry_after", None) or None
+    retry_after_raw = getattr(exc, "retry_after", None)
+    retry_after_val: int | None = (
+        retry_after_raw if retry_after_raw is not None else None
+    )
     headers: dict[str, str] | None = None
-    if retry_after_val:
+    if retry_after_val is not None:
         headers = {"Retry-After": str(retry_after_val)}
     return _build_response(
         request,
diff --git a/src/synthorg/api/lifecycle.py b/src/synthorg/api/lifecycle.py
@@ -239,13 +239,22 @@ async def _safe_startup(  # noqa: PLR0913, PLR0912, PLR0915, C901
                     app_state.config.api.auth if app_state.config is not None else None
                 )
                 if auth_cfg is not None:
-                    lockout_store = LockoutStore(db, auth_cfg)
-                    await lockout_store.load_locked()
-                    app_state.set_lockout_store(lockout_store)
-                    logger.info(
-                        API_APP_STARTUP,
-                        note="Lockout store initialized",
-                    )
+                    try:
+                        lockout_store = LockoutStore(db, auth_cfg)
+                        await lockout_store.load_locked()
+                        app_state.set_lockout_store(lockout_store)
+                        logger.info(
+                            API_APP_STARTUP,
+                            note="Lockout store initialized",
+                        )
+                    except MemoryError, RecursionError:
+                        raise
+                    except Exception:
+                        logger.error(
+                            API_APP_STARTUP,
+                            note="Lockout store initialization failed",
+                            exc_info=True,
+                        )
 
         if message_bus is not None:
             try:
diff --git a/src/synthorg/observability/events/api.py b/src/synthorg/observability/events/api.py
@@ -107,8 +107,10 @@
 API_AUTH_LOCKOUT_CLEANUP: Final[str] = "api.auth.lockout_cleanup"
 
 # Refresh tokens
+API_AUTH_REFRESH_CREATED: Final[str] = "api.auth.refresh_created"
 API_AUTH_REFRESH_CONSUMED: Final[str] = "api.auth.refresh_consumed"
 API_AUTH_REFRESH_REJECTED: Final[str] = "api.auth.refresh_rejected"
+API_AUTH_REFRESH_REVOKED: Final[str] = "api.auth.refresh_revoked"
 API_AUTH_REFRESH_CLEANUP: Final[str] = "api.auth.refresh_cleanup"
 
 # Cookie auth
diff --git a/src/synthorg/persistence/sqlite/schema.sql b/src/synthorg/persistence/sqlite/schema.sql
@@ -519,7 +519,7 @@ CREATE TABLE IF NOT EXISTS refresh_tokens (
     user_id TEXT NOT NULL
         REFERENCES users(id) ON DELETE CASCADE,
     expires_at TEXT NOT NULL,
-    used INTEGER NOT NULL DEFAULT 0,
+    used INTEGER NOT NULL DEFAULT 0 CHECK(used IN (0, 1)),
     created_at TEXT NOT NULL
 );
 
diff --git a/tests/integration/api/test_first_run_flow.py b/tests/integration/api/test_first_run_flow.py
@@ -154,7 +154,8 @@ def test_full_first_run_flow(
         assert resp.status_code == 201
         assert resp.json()["data"]["expires_in"] > 0
         session_token, csrf_token = _extract_auth_cookies(resp)
-        assert session_token
+        assert session_token, "missing session cookie after setup"
+        assert csrf_token, "missing CSRF cookie after setup"
 
         # ── 3. POST /auth/login -- verify credentials work ──
         resp = client.post(
diff --git a/tests/unit/api/auth/test_controller.py b/tests/unit/api/auth/test_controller.py
@@ -29,6 +29,8 @@ def _extract_auth_cookies(response: Any) -> dict[str, str]:
             result["session"] = v.split("session=")[1].split(";")[0]
         elif v.startswith("csrf_token="):
             result["csrf_token"] = v.split("csrf_token=")[1].split(";")[0]
+    assert "session" in result, "missing session cookie"
+    assert "csrf_token" in result, "missing csrf_token cookie"
     return result
 
 
diff --git a/web/src/__tests__/stores/auth.test.ts b/web/src/__tests__/stores/auth.test.ts
@@ -124,9 +124,9 @@ describe('auth store', () => {
   })
 
   describe('fetchUser', () => {
-    it('does nothing when unauthenticated', async () => {
+    it('skips when already authenticated with user loaded', async () => {
       const authApi = await import('@/api/endpoints/auth')
-      useAuthStore.setState({ authStatus: 'unauthenticated' })
+      useAuthStore.setState({ authStatus: 'authenticated', user: mockUser })
 
       await useAuthStore.getState().fetchUser()
 
diff --git a/web/src/api/endpoints/providers.ts b/web/src/api/endpoints/providers.ts
@@ -1,5 +1,6 @@
 import { createLogger } from '@/lib/logger'
 import { getCsrfToken } from '@/utils/csrf'
+import { IS_DEV_AUTH_BYPASS } from '@/utils/dev'
 import { apiClient, unwrap, unwrapVoid } from '../client'
 import type {
   AddAllowlistEntryRequest,
@@ -184,7 +185,7 @@ export async function pullModel(
   })
 
   if (!response.ok || !response.body) {
-    if (response.status === 401) {
+    if (response.status === 401 && !IS_DEV_AUTH_BYPASS) {
       // Server clears the session cookie. Sync Zustand auth state.
       import('@/stores/auth').then(({ useAuthStore }) => {
         useAuthStore.getState().handleUnauthorized()
diff --git a/web/src/stores/auth.ts b/web/src/stores/auth.ts
@@ -101,7 +101,7 @@ export const useAuthStore = create<AuthState>()((set, get) => {
     },
 
     async fetchUser() {
-      if (get().authStatus === 'unauthenticated' && !IS_DEV_AUTH_BYPASS) return
+      if (get().authStatus === 'authenticated' && get().user && !IS_DEV_AUTH_BYPASS) return
       try {
         const user = await authApi.getMe()
         set({ user, authStatus: 'authenticated' })
diff --git a/web/src/utils/csrf.ts b/web/src/utils/csrf.ts
@@ -18,5 +18,10 @@ export function getCsrfToken(): string | null {
     .find((row) => row.startsWith('csrf_token='))
   if (!match) return null
   const eqIdx = match.indexOf('=')
-  return eqIdx === -1 ? null : decodeURIComponent(match.slice(eqIdx + 1))
+  if (eqIdx === -1) return null
+  try {
+    return decodeURIComponent(match.slice(eqIdx + 1))
+  } catch {
+    return null
+  }
 }

Original file line number	Diff line number	Diff line change
`@@ -127,6 +127,11 @@ async def authenticate_request(`
`127`	`127`	`raise NotAuthorizedException(`
`128`	`128`	`detail="Invalid session cookie",`
`129`	`129`	`)`
	`130`	`+ logger.warning(`
	`131`	`+ API_AUTH_FAILED,`
	`132`	`+ reason="missing_authentication",`
	`133`	`+ path=path,`
	`134`	`+ )`
`130`	`135`	`raise NotAuthorizedException(`
`131`	`136`	`detail="Missing authentication",`
`132`	`137`	`)`