fix(oidc): address review findings — PKCE, iat, TLS, sub, cookies, sessions

Addresses RaresKeY's review (5 findings) and follow-up Basic RP
validation comment on PR #3508:

- Add PKCE (RFC 7636, S256): code_challenge in the authorization
  request, verifier carried in the Fernet-encrypted state, and
  code_verifier sent to the token endpoint.
- Require the iat claim in id_tokens (OIDC Core §2); tokens without
  iat are now rejected.
- Prefer client_secret_basic at the token endpoint per discovery
  (OIDC default), falling back to client_secret_post only when the
  provider excludes basic.
- Require HTTPS for the issuer and authorization endpoint, not just
  the back-channel endpoints.
- Preserve OIDC subs exactly (no strip) so distinct whitespace-bearing
  subjects can never collapse into one local account; same for the
  UserInfo sub-binding comparison.
- Sync admin state only on a well-formed groups claim; UserInfo
  availability alone (or a malformed groups value) no longer demotes
  an existing admin.
- OIDC session/CSRF cookies are Secure by default regardless of
  SECURE_COOKIES; explicit OIDC_ALLOW_INSECURE_COOKIES=true is the
  only (documented, dev-only) opt-out.
- Make sessions issued by one uvicorn worker validate on others via
  an mtime-gated read-through reload of sessions.json.

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

Author: holden093

.env.example

@@ -130,6 +130,11 @@ SEARXNG_INSTANCE=http://localhost:8080
 # TRUST_PROXY_HEADERS=true only when the app is behind a trusted proxy that
 # strips/replaces inbound X-Forwarded-Proto; otherwise client-supplied
 # forwarded headers are ignored for cookie security decisions.
+#
+# OIDC session and CSRF cookies always carry the Secure flag, regardless
+# of SECURE_COOKIES — SSO implies a TLS deployment.  The only opt-out is
+# the development-only override below; never enable it in production.
+# OIDC_ALLOW_INSECURE_COOKIES=false
 
 # ============================================================
 # ChromaDB (vector store)

core/auth.py

@@ -136,6 +136,10 @@ def __init__(self, auth_path: str = DEFAULT_AUTH_PATH):
         # all uvicorn workers so first-admin bootstrap and auth.json mutations
         # are serialised across processes, not just threads within one worker.
         self._ipc_lock_path = auth_path + ".lock"
+        # mtime of sessions.json at last load — lets validate_token cheaply
+        # detect sessions written by other uvicorn workers (see
+        # _reload_sessions_if_changed).
+        self._sessions_mtime_ns = -1
         self._load()
         self._load_sessions()
         self._migrate_single_user()
@@ -168,6 +172,7 @@ def _load_sessions(self):
         """Load persisted session tokens from disk, pruning expired ones."""
         try:
             if os.path.exists(self._sessions_path):
+                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()
@@ -180,6 +185,46 @@ def _load_sessions(self):
             logger.error(f"Failed to load sessions: {e}")
             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.
+        """
+        try:
+            stat = os.stat(self._sessions_path)
+        except OSError:
+            return
+        with self._sessions_lock:
+            if stat.st_mtime_ns == self._sessions_mtime_ns:
+                return
+            try:
+                with open(self._sessions_path, "r", encoding="utf-8") as f:
+                    data = json.load(f)
+            except Exception as e:
+                logger.error(f"Failed to reload sessions: {e}")
+                return
+            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
+
     def _save_sessions(self):
         """Persist session tokens to disk (atomic, lock-guarded)."""
         try:
@@ -878,6 +923,11 @@ 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()
         expired = False
         deleted_user = False
         with self._sessions_lock:
@@ -904,6 +954,11 @@ 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()
         expired = False
         deleted_user = False
         with self._sessions_lock:

core/oidc.py

@@ -26,6 +26,8 @@
 60-second cooldown throttles both successful and failed refreshes.
 """
 
+import base64
+import hashlib
 import json
 import logging
 import math
@@ -74,12 +76,13 @@ def _get_state_fernet():
         return _state_fernet
 
 
-def _encode_state(nonce: str, redirect_uri: str) -> str:
+def _encode_state(nonce: str, redirect_uri: str, code_verifier: str) -> str:
     """Return a Fernet-encrypted state token containing nonce + metadata."""
     fernet = _get_state_fernet()
     payload = json.dumps({
         "nonce": nonce,
         "redirect_uri": redirect_uri,
+        "code_verifier": code_verifier,
         "created": time.time(),
     })
     return fernet.encrypt(payload.encode()).decode()
@@ -97,11 +100,14 @@ def _decode_state(state: str) -> Optional[Dict[str, Any]]:
         return None
     nonce = data.get("nonce")
     redirect_uri = data.get("redirect_uri")
+    code_verifier = data.get("code_verifier")
     created = data.get("created")
     if not isinstance(nonce, str) or not nonce:
         return None
     if not isinstance(redirect_uri, str):
         return None
+    if not isinstance(code_verifier, str) or not code_verifier:
+        return None
     if not _is_numericdate(created):
         return None
     now = time.time()
@@ -164,8 +170,19 @@ def __init__(
         self._jwks_cache: Dict[str, Dict[str, Any]] = {}
         self._jwks_cache_lock = threading.Lock()
         self._allowed_algs: Optional[List[str]] = None
+        self._token_auth_methods: List[str] = ["client_secret_basic"]
         self._discover()
 
+    def _use_basic_auth(self) -> bool:
+        """True when the token endpoint should use client_secret_basic."""
+        if "client_secret_basic" in self._token_auth_methods:
+            return True
+        if "client_secret_post" in self._token_auth_methods:
+            return False
+        # Provider advertises neither shared-secret method — use the OIDC
+        # default rather than silently leaking the secret in the body.
+        return True
+
     # -- discovery -----------------------------------------------------------
 
     def _discover(self) -> None:
@@ -176,6 +193,15 @@ def _discover(self) -> None:
         well_known_url = self.issuer + "/.well-known/openid-configuration"
         if not well_known_url.startswith(("http://", "https://")):
             well_known_url = f"https://{well_known_url}"
+        # The issuer (and therefore the discovery document) must be HTTPS:
+        # the authorization redirect carries state/nonce, and an http://
+        # issuer lets an active network attacker substitute authorization
+        # codes or rewrite the discovery document entirely.
+        if not well_known_url.startswith("https://"):
+            raise OidcError(
+                f"OIDC issuer must use HTTPS, got {self.issuer!r}. "
+                "Configure the IdP with TLS or set OIDC_ENABLED=false."
+            )
 
         try:
             resp = httpx.get(well_known_url, timeout=15.0)
@@ -204,10 +230,12 @@ def _discover(self) -> None:
                 f"discovery doc returned {doc_issuer!r}"
             )
 
-        # Client credentials and bearer tokens must never be sent over
-        # cleartext transport. Authorization is browser-facing and is not
-        # included because it does not carry those secrets.
-        for name in ("token_endpoint", "jwks_uri", "userinfo_endpoint"):
+        # No OIDC endpoint may use cleartext transport.  The back-channel
+        # endpoints carry client credentials and bearer tokens; the browser-
+        # facing authorization endpoint carries state/nonce and returns the
+        # authorization code, so an http:// endpoint enables code
+        # substitution by an active network observer.
+        for name in ("authorization_endpoint", "token_endpoint", "jwks_uri", "userinfo_endpoint"):
             url = self._config.get(name)
             if url and not isinstance(url, str):
                 raise OidcError(f"OIDC {name} must be a URL string")
@@ -224,6 +252,13 @@ def _discover(self) -> None:
         safe = [a for a in supported if a in ("RS256", "RS384", "RS512", "ES256", "ES384", "ES512", "PS256", "PS384", "PS512")]
         self._allowed_algs = safe or ["RS256"]
 
+        # Token-endpoint auth methods.  Per OIDC Discovery §3, an omitted
+        # token_endpoint_auth_methods_supported means client_secret_basic.
+        methods = self._config.get("token_endpoint_auth_methods_supported")
+        if not isinstance(methods, list) or not methods:
+            methods = ["client_secret_basic"]
+        self._token_auth_methods = methods
+
         logger.info(
             "OIDC provider discovered: issuer=%r auth=%r token=%r algs=%s",
             self.issuer,
@@ -264,9 +299,19 @@ def get_authorization_url(self, redirect_uri: str) -> Tuple[str, str, str]:
         """
         nonce = secrets.token_hex(32)
 
+        # PKCE (RFC 7636, S256).  The verifier travels inside the encrypted
+        # state token, so the callback can recover it without server-side
+        # storage — same carrier as the nonce.
+        code_verifier = secrets.token_urlsafe(64)
+        code_challenge = (
+            base64.urlsafe_b64encode(hashlib.sha256(code_verifier.encode("ascii")).digest())
+            .rstrip(b"=")
+            .decode("ascii")
+        )
+
         # Encode the nonce + metadata into the state parameter (Fernet-
         # encrypted, stateless — works across multiple workers/processes).
-        state = _encode_state(nonce, redirect_uri)
+        state = _encode_state(nonce, redirect_uri, code_verifier)
 
         from urllib.parse import urlencode
         params = {
@@ -276,6 +321,8 @@ def get_authorization_url(self, redirect_uri: str) -> Tuple[str, str, str]:
             "scope": self.scopes,
             "state": state,
             "nonce": nonce,
+            "code_challenge": code_challenge,
+            "code_challenge_method": "S256",
         }
         # Request forced re-authentication when OIDC_MAX_AGE is configured.
         # The claim is later verified in _verify_id_token against auth_time.
@@ -300,6 +347,7 @@ def exchange_code(
             raise OidcError("OIDC state not found — may be expired, reused, or from a different worker")
         nonce = stored.get("nonce", "")
         stored_redirect_uri = stored.get("redirect_uri", "")
+        code_verifier = stored.get("code_verifier", "")
 
         # Bind the token exchange to the redirect_uri that was used in the
         # authorization request (carried in the signed state token).  Reject
@@ -312,7 +360,9 @@ def exchange_code(
             )
 
         # 2. Exchange code for tokens (using the stored redirect_uri)
-        token_data = self._token_request(code, stored_redirect_uri or redirect_uri)
+        token_data = self._token_request(
+            code, stored_redirect_uri or redirect_uri, code_verifier
+        )
 
         # 3. Verify id_token
         id_token = token_data.get("id_token")
@@ -349,9 +399,13 @@ def exchange_code(
                     )
                     userinfo = {}
                 else:
-                    # Require a non-empty sub that matches the verified
-                    # id_token subject before trusting any UserInfo claims.
-                    ui_sub = (userinfo.get("sub") or "").strip()
+                    # Require a non-empty sub that exactly matches the
+                    # verified id_token subject before trusting any UserInfo
+                    # claims.  No normalization: subs are opaque identifiers
+                    # and trimming could equate two distinct subjects.
+                    ui_sub = userinfo.get("sub")
+                    if not isinstance(ui_sub, str):
+                        ui_sub = ""
                     if not ui_sub:
                         logger.warning(
                             "UserInfo response missing sub claim — "
@@ -388,18 +442,34 @@ def exchange_code(
         claims["_userinfo_available"] = userinfo_available
         return claims
 
-    def _token_request(self, code: str, redirect_uri: str) -> Dict[str, Any]:
+    def _token_request(
+        self, code: str, redirect_uri: str, code_verifier: str
+    ) -> Dict[str, Any]:
         """POST the token endpoint to exchange code for tokens."""
         token_endpoint = self._config["token_endpoint"]
         payload = {
             "grant_type": "authorization_code",
             "code": code,
             "redirect_uri": redirect_uri,
-            "client_id": self.client_id,
-            "client_secret": self.client_secret,
+            "code_verifier": code_verifier,
         }
+        # client_secret_basic is the OIDC default and the method the
+        # conformance suite expects; use it whenever the provider supports
+        # it (or doesn't advertise methods at all, which per Discovery §3
+        # means client_secret_basic).  Fall back to client_secret_post only
+        # when the provider explicitly excludes basic.
+        auth = None
+        if self._use_basic_auth():
+            from urllib.parse import quote
+            auth = (
+                quote(self.client_id, safe=""),
+                quote(self.client_secret, safe=""),
+            )
+        else:
+            payload["client_id"] = self.client_id
+            payload["client_secret"] = self.client_secret
         try:
-            resp = httpx.post(token_endpoint, data=payload, timeout=15.0)
+            resp = httpx.post(token_endpoint, data=payload, auth=auth, timeout=15.0)
             resp.raise_for_status()
             data = resp.json()
         except httpx.HTTPStatusError as exc:
@@ -583,15 +653,17 @@ def _verify_id_token(self, id_token: str, nonce: str) -> Dict[str, Any]:
                     f"{self.max_age} s (now={now:.0f}, age={now - auth_time:.0f} s)"
                 )
 
-        # Verify iat (issued-at) is not in the far future.
+        # Verify iat (issued-at): required by OIDC Core §2, must be numeric
+        # and not in the far future.
         iat = claims.get("iat")
-        if iat is not None:
-            if not _is_numericdate(iat):
-                raise OidcError(f"id_token iat claim is non-numeric: {iat!r}")
-            if iat > time.time() + 60:
-                raise OidcError(
-                    f"id_token iat {iat} is more than 60 s in the future"
-                )
+        if iat is None:
+            raise OidcError("id_token missing iat claim")
+        if not _is_numericdate(iat):
+            raise OidcError(f"id_token iat claim is non-numeric: {iat!r}")
+        if iat > time.time() + 60:
+            raise OidcError(
+                f"id_token iat {iat} is more than 60 s in the future"
+            )
 
         return claims