fix(auth): proactive hardening — cross-worker revocation, file perms, cookie policy

Follow-up hardening beyond the explicit review findings:

- Propagate session revocation across uvicorn workers: token
  validation now syncs issuance AND revocation from sessions.json
  (mtime-gated), _save_sessions merges on-disk state under an
  inter-process flock so concurrent workers can't lose each other's
  sessions, and revocation tombstones prevent a just-revoked token
  from being re-merged.
- Restrict sessions.json and auth.json to 0600 (bearer tokens and
  password hashes; same policy as data/app.db, #4420), applied
  atomically at write time and retroactively at load.
- Password-login session cookie: SECURE_COOKIES=false can no longer
  downgrade the cookie when the request arrived over HTTPS (spoofable
  X-Forwarded-Proto still requires TRUST_PROXY_HEADERS opt-in).
- Document why OIDC state tokens are deliberately not single-use and
  which mechanisms bound the replay window.
- Warn once per process (not twice per login) when
  OIDC_ALLOW_INSECURE_COOKIES is enabled; pass the variable through
  the Compose files so the documented dev override actually reaches
  containers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GRiLb12nnLnBnYsg14oSWd

Author: holden093

core/atomic_io.py

@@ -18,15 +18,27 @@
 from typing import Any, Optional
 
 
-def atomic_write_json(path: str, data: Any, *, indent: Optional[int] = None) -> None:
+def atomic_write_json(
+    path: str, data: Any, *, indent: Optional[int] = None, mode: Optional[int] = None
+) -> None:
     """Atomically persist `data` as JSON at `path`.
 
     The temp file uses the live PID as a suffix so two processes saving the
     same file (e.g. unit tests) don't collide on the rename target.
+
+    When *mode* is given (e.g. ``0o600`` for files holding secrets), the
+    temp file is chmod'ed before the rename so the restricted permissions
+    are in place atomically with the content — there is no window where
+    the target exists with default-umask permissions.
     """
     os.makedirs(os.path.dirname(path) or ".", exist_ok=True)
     tmp = f"{path}.tmp.{os.getpid()}"
     with open(tmp, "w", encoding="utf-8") as f:
+        if mode is not None:
+            try:
+                os.fchmod(f.fileno(), mode)
+            except AttributeError:  # Windows has no fchmod
+                os.chmod(tmp, mode)
         json.dump(data, f, indent=indent)
         f.flush()
         os.fsync(f.fileno())

core/auth.py

@@ -140,6 +140,15 @@ def __init__(self, auth_path: str = DEFAULT_AUTH_PATH):
         # detect sessions written by other uvicorn workers (see
         # _reload_sessions_if_changed).
         self._sessions_mtime_ns = -1
+        # Tokens present in sessions.json at the last disk sync.  Used to
+        # distinguish "revoked by another worker" (was on disk, now gone —
+        # drop it) from "issued locally moments ago, racing its own save"
+        # (never seen on disk — keep it).
+        self._disk_tokens: set = set()
+        # Tokens this worker revoked whose removal may not yet be visible
+        # on disk.  A disk sync must never re-add these; pruned once the
+        # on-disk file no longer contains them.
+        self._revoked_tokens: set = set()
         self._load()
         self._load_sessions()
         self._migrate_single_user()
@@ -149,6 +158,12 @@ def __init__(self, auth_path: str = DEFAULT_AUTH_PATH):
     def _load(self):
         try:
             if os.path.exists(self.auth_path):
+                # Contains password hashes — restrict pre-existing files
+                # written before the 0600 policy.
+                try:
+                    os.chmod(self.auth_path, 0o600)
+                except OSError:
+                    pass
                 with open(self.auth_path, "r", encoding="utf-8") as f:
                     self._config = json.load(f)
                 # Normalize all stored usernames to lowercase so they match
@@ -172,11 +187,19 @@ def _load_sessions(self):
         """Load persisted session tokens from disk, pruning expired ones."""
         try:
             if os.path.exists(self._sessions_path):
+                # Session tokens are bearer credentials — never leave the
+                # file readable by other local users (same policy as
+                # data/app.db, #4420).
+                try:
+                    os.chmod(self._sessions_path, 0o600)
+                except OSError:
+                    pass
                 self._sessions_mtime_ns = os.stat(self._sessions_path).st_mtime_ns
                 with open(self._sessions_path, "r", encoding="utf-8") as f:
                     data = json.load(f)
                 now = time.time()
                 self._sessions = {k: v for k, v in data.items() if v.get("expiry", 0) > now}
+                self._disk_tokens = set(data)
                 pruned = len(data) - len(self._sessions)
                 if pruned > 0:
                     self._save_sessions()
@@ -186,19 +209,23 @@ def _load_sessions(self):
             self._sessions = {}
 
     def _reload_sessions_if_changed(self):
-        """Merge sessions written by other uvicorn workers.
-
-        The OIDC callback (or a password login) may run on one worker while
-        the browser's next request lands on another; each worker loads
-        sessions.json only at startup, so the new token would be rejected.
-        Called on a token miss: when the file's mtime has changed since the
-        last load, re-read it and add unknown unexpired tokens to the
-        in-memory map.  The mtime gate keeps unknown-token spam at one
-        os.stat per request, not a JSON parse.
-
-        Additive only — tokens missing from disk are NOT dropped from
-        memory, so a token issued moments ago on this worker can't be lost
-        to a reload racing its own _save_sessions.
+        """Sync session state written by other uvicorn workers.
+
+        The OIDC callback (or a password login/logout) may run on one
+        worker while the browser's next request lands on another; each
+        worker loads sessions.json only at startup, so cross-worker
+        issuance and revocation would otherwise be invisible.  Called on
+        every token validation: when the file's mtime has changed since
+        the last sync, re-read it and
+
+        - add unknown unexpired tokens (issued by another worker), and
+        - drop in-memory tokens that were on disk at the last sync but
+          are gone now (revoked by another worker).
+
+        A token never yet seen on disk is kept — it was issued locally
+        moments ago and may be racing its own _save_sessions.  The mtime
+        gate keeps the steady-state cost at one os.stat per validation,
+        not a JSON parse.
         """
         try:
             stat = os.stat(self._sessions_path)
@@ -216,21 +243,76 @@ def _reload_sessions_if_changed(self):
             self._sessions_mtime_ns = stat.st_mtime_ns
             if not isinstance(data, dict):
                 return
-            now = time.time()
-            for tok, sess in data.items():
-                if (
-                    tok not in self._sessions
-                    and isinstance(sess, dict)
-                    and sess.get("expiry", 0) > now
-                ):
-                    self._sessions[tok] = sess
+            self._apply_disk_sessions(data)
+
+    def _apply_disk_sessions(self, data: Dict[str, Any]) -> None:
+        """Merge parsed sessions.json content into memory.
+
+        Caller must hold ``_sessions_lock``.  Adds unknown unexpired
+        tokens (unless this worker revoked them and the removal hasn't
+        reached disk yet), drops tokens revoked by other workers, and
+        refreshes the disk-snapshot bookkeeping.
+        """
+        now = time.time()
+        for tok, sess in data.items():
+            if (
+                tok not in self._sessions
+                and tok not in self._revoked_tokens
+                and isinstance(sess, dict)
+                and sess.get("expiry", 0) > now
+            ):
+                self._sessions[tok] = sess
+        revoked_elsewhere = [
+            tok for tok in self._sessions
+            if tok not in data and tok in self._disk_tokens
+        ]
+        for tok in revoked_elsewhere:
+            self._sessions.pop(tok, None)
+        self._disk_tokens = set(data)
+        # A tombstone is only needed while the token is still on disk.
+        self._revoked_tokens &= self._disk_tokens
+
+    @contextmanager
+    def _interprocess_sessions_lock(self):
+        """Serialise sessions.json read-merge-write cycles across uvicorn
+        workers.  Separate lock file from the auth.json IPC lock so a
+        session save can never deadlock a caller already holding the auth
+        lock (flock is not re-entrant across file descriptors)."""
+        if not HAS_FCNTL:
+            yield
+            return
+        fd = os.open(self._sessions_path + ".lock", os.O_CREAT | os.O_RDWR, 0o600)
+        try:
+            fcntl.flock(fd, fcntl.LOCK_EX)
+            yield
+        finally:
+            fcntl.flock(fd, fcntl.LOCK_UN)
+            os.close(fd)
 
     def _save_sessions(self):
-        """Persist session tokens to disk (atomic, lock-guarded)."""
+        """Persist session tokens to disk (atomic, merge-on-write).
+
+        Merges the current on-disk state before writing, under an
+        inter-process flock — a plain overwrite would clobber sessions
+        issued by other workers since this worker's last sync (lost
+        update).  Tombstones in ``_revoked_tokens`` keep just-revoked
+        tokens from being re-merged and resurrected.
+        """
         try:
-            with self._sessions_lock:
+            with self._interprocess_sessions_lock(), self._sessions_lock:
+                try:
+                    with open(self._sessions_path, "r", encoding="utf-8") as f:
+                        data = json.load(f)
+                    if isinstance(data, dict):
+                        self._apply_disk_sessions(data)
+                except OSError:
+                    pass  # first save — no file yet
+                except Exception as e:
+                    logger.error(f"Failed to merge sessions before save: {e}")
                 snapshot = dict(self._sessions)
-            _atomic_write_json(self._sessions_path, snapshot)
+                _atomic_write_json(self._sessions_path, snapshot, mode=0o600)
+                self._disk_tokens = set(snapshot)
+                self._revoked_tokens &= self._disk_tokens
         except Exception as e:
             logger.error(f"Failed to save sessions: {e}")
 
@@ -295,7 +377,8 @@ def _migrate_legacy_admin_role(self):
             self._save()
 
     def _save(self):
-        _atomic_write_json(self.auth_path, self._config, indent=2)
+        # Password hashes — owner-only, same policy as sessions.json.
+        _atomic_write_json(self.auth_path, self._config, indent=2, mode=0o600)
 
     @property
     def users(self) -> Dict[str, Any]:
@@ -622,6 +705,7 @@ def delete_user(self, username: str, requesting_user: str) -> bool:
                        if (sess or {}).get("username") == username]
             for tok in to_drop:
                 self._sessions.pop(tok, None)
+                self._revoked_tokens.add(tok)
                 revoked += 1
         if revoked:
             self._save_sessions()
@@ -923,11 +1007,8 @@ def create_session_trusted(self, username: str) -> Optional[str]:
     def validate_token(self, token: Optional[str]) -> bool:
         if not token:
             return False
-        with self._sessions_lock:
-            known = token in self._sessions
-        if not known:
-            # May have been issued by another worker — read through to disk.
-            self._reload_sessions_if_changed()
+        # Sync issuance/revocation from other workers (mtime-gated).
+        self._reload_sessions_if_changed()
         expired = False
         deleted_user = False
         with self._sessions_lock:
@@ -944,6 +1025,7 @@ def validate_token(self, token: Optional[str]) -> bool:
                 # silently authenticating against a non-existent account.
                 if session.get("username") not in self.users:
                     self._sessions.pop(token, None)
+                    self._revoked_tokens.add(token)
                     deleted_user = True
         if expired or deleted_user:
             self._save_sessions()
@@ -954,11 +1036,8 @@ def get_username_for_token(self, token: Optional[str]) -> Optional[str]:
         """Return the username associated with a valid token."""
         if not token:
             return None
-        with self._sessions_lock:
-            known = token in self._sessions
-        if not known:
-            # May have been issued by another worker — read through to disk.
-            self._reload_sessions_if_changed()
+        # Sync issuance/revocation from other workers (mtime-gated).
+        self._reload_sessions_if_changed()
         expired = False
         deleted_user = False
         with self._sessions_lock:
@@ -973,6 +1052,7 @@ def get_username_for_token(self, token: Optional[str]) -> Optional[str]:
                 # SECURITY: orphan check — same rationale as validate_token.
                 if _u not in self.users:
                     self._sessions.pop(token, None)
+                    self._revoked_tokens.add(token)
                     deleted_user = True
                 else:
                     return _u
@@ -983,6 +1063,7 @@ def get_username_for_token(self, token: Optional[str]) -> Optional[str]:
     def revoke_token(self, token: str):
         with self._sessions_lock:
             self._sessions.pop(token, None)
+            self._revoked_tokens.add(token)
         self._save_sessions()
 
     def revoke_user_sessions(self, username: str, except_token: Optional[str] = None) -> int:
@@ -996,9 +1077,13 @@ def revoke_user_sessions(self, username: str, except_token: Optional[str] = None
             ]
             for token in to_drop:
                 self._sessions.pop(token, None)
+                self._revoked_tokens.add(token)
                 revoked += 1
-            if revoked:
-                self._save_sessions()
+        # Save outside _sessions_lock: _save_sessions acquires the
+        # inter-process flock before _sessions_lock, and taking them in
+        # the opposite order here could deadlock two threads.
+        if revoked:
+            self._save_sessions()
         return revoked
 
     def status(self, token: Optional[str]) -> Dict[str, Any]:

core/oidc.py

@@ -51,6 +51,16 @@
 
 _STATE_TTL = 600  # 10 minutes
 
+# DESIGN NOTE — state tokens are deliberately NOT single-use.  Enforcing
+# one-time consumption would require shared server-side storage, which
+# this stateless design intentionally avoids (multi-worker support with
+# no session store).  Replay of a state within its TTL is mitigated by:
+#   - the authorization code being single-use at the IdP (a replayed
+#     callback fails the token exchange),
+#   - the nonce being bound into the signed id_token and verified,
+#   - the PKCE verifier being bound to the same encrypted state, and
+#   - the CSRF cookie requiring the completing browser to hold the state.
+
 _state_fernet_lock = threading.Lock()
 _state_fernet = None
 

docker-compose.gpu-amd.yml

@@ -58,6 +58,8 @@ services:
       - OIDC_ADMIN_GROUPS=${OIDC_ADMIN_GROUPS:-}
       - OIDC_REDIRECT_URI=${OIDC_REDIRECT_URI:-}
       - OIDC_FIRST_USER_IS_ADMIN=${OIDC_FIRST_USER_IS_ADMIN:-true}
+      # Dev-only: allow OIDC cookies without the Secure flag (plain-HTTP testing).
+      - OIDC_ALLOW_INSECURE_COOKIES=${OIDC_ALLOW_INSECURE_COOKIES:-false}
       - EMBEDDING_URL=${EMBEDDING_URL:-}
       - EMBEDDING_MODEL=${EMBEDDING_MODEL:-}
       - EMBEDDING_API_KEY=${EMBEDDING_API_KEY:-}

docker-compose.gpu-nvidia.yml

@@ -57,6 +57,8 @@ services:
       - OIDC_ADMIN_GROUPS=${OIDC_ADMIN_GROUPS:-}
       - OIDC_REDIRECT_URI=${OIDC_REDIRECT_URI:-}
       - OIDC_FIRST_USER_IS_ADMIN=${OIDC_FIRST_USER_IS_ADMIN:-true}
+      # Dev-only: allow OIDC cookies without the Secure flag (plain-HTTP testing).
+      - OIDC_ALLOW_INSECURE_COOKIES=${OIDC_ALLOW_INSECURE_COOKIES:-false}
       - EMBEDDING_URL=${EMBEDDING_URL:-}
       - EMBEDDING_MODEL=${EMBEDDING_MODEL:-}
       - EMBEDDING_API_KEY=${EMBEDDING_API_KEY:-}

docker-compose.yml

@@ -46,6 +46,8 @@ services:
       - OIDC_ADMIN_GROUPS=${OIDC_ADMIN_GROUPS:-}
       - OIDC_REDIRECT_URI=${OIDC_REDIRECT_URI:-}
       - OIDC_FIRST_USER_IS_ADMIN=${OIDC_FIRST_USER_IS_ADMIN:-true}
+      # Dev-only: allow OIDC cookies without the Secure flag (plain-HTTP testing).
+      - OIDC_ALLOW_INSECURE_COOKIES=${OIDC_ALLOW_INSECURE_COOKIES:-false}
       - EMBEDDING_URL=${EMBEDDING_URL:-}
       - EMBEDDING_MODEL=${EMBEDDING_MODEL:-}
       - EMBEDDING_API_KEY=${EMBEDDING_API_KEY:-}

routes/auth_routes.py

@@ -84,6 +84,26 @@ class SetOpenRegistrationRequest(BaseModel):
 SESSION_COOKIE = "odysseus_session"
 
 
+def _session_cookie_secure(request: Request) -> bool:
+    """Secure flag for the password-login session cookie.
+
+    SECURE_COOKIES=true always wins.  Unlike the historical behaviour,
+    SECURE_COOKIES=false (the bundled Compose default) can no longer
+    downgrade the cookie when the request itself arrived over HTTPS —
+    a stock TLS deployment must not issue a bearer cookie eligible for
+    cleartext transmission.  X-Forwarded-Proto is honoured only when the
+    deployment explicitly opts in via TRUST_PROXY_HEADERS, so a client
+    cannot influence cookie policy with a spoofed header.
+    """
+    if os.getenv("SECURE_COOKIES", "").strip().lower() in ("true", "1", "yes"):
+        return True
+    forwarded = ""
+    if os.getenv("TRUST_PROXY_HEADERS", "").strip().lower() in ("true", "1", "yes"):
+        forwarded = getattr(request, "headers", {}).get("x-forwarded-proto", "")
+    scheme = forwarded or getattr(getattr(request, "url", None), "scheme", "") or "http"
+    return scheme == "https"
+
+
 def setup_auth_routes(auth_manager: AuthManager) -> APIRouter:
     router = APIRouter(prefix="/api/auth", tags=["auth"])
 
@@ -157,7 +177,7 @@ async def login(body: LoginRequest, request: Request, response: Response):
             value=token,
             httponly=True,
             samesite="lax",
-            secure=os.getenv("SECURE_COOKIES", "false").lower() == "true",
+            secure=_session_cookie_secure(request),
             path="/",
         )
         if body.remember:

routes/oidc_routes.py

@@ -1,6 +1,7 @@
 """OpenID Connect authentication routes — login, callback, config."""
 
 import asyncio
+import functools
 import logging
 import os
 import secrets
@@ -319,10 +320,17 @@ def _oidc_cookie_secure() -> bool:
     downgrade OIDC cookies.
     """
     if os.getenv("OIDC_ALLOW_INSECURE_COOKIES", "").strip().lower() in ("true", "1", "yes"):
-        logger.warning(
-            "OIDC_ALLOW_INSECURE_COOKIES=true — OIDC session and CSRF "
-            "cookies are issued without the Secure flag. Never use this "
-            "outside plain-HTTP local development."
-        )
+        _warn_insecure_cookies_once()
         return False
     return True
+
+
+@functools.lru_cache(maxsize=1)
+def _warn_insecure_cookies_once() -> None:
+    # Once per process, not once per login — the flag doesn't change at
+    # runtime and repeating the warning twice per flow is pure log spam.
+    logger.warning(
+        "OIDC_ALLOW_INSECURE_COOKIES=true — OIDC session and CSRF "
+        "cookies are issued without the Secure flag. Never use this "
+        "outside plain-HTTP local development."
+    )