Stop the session secret and the bundle cache from logging people out

CLAUDE.md

@@ -134,6 +134,30 @@ quizbinf/
 - For local development there must be a **mock login mode** (env-flag
   controlled, hard-disabled in production builds) so the app can be developed
   without reaching KTH's IdP.
+- **The session window is an *idle* timeout, and it slides.** A cookie lasts a
+  week unused (`SESSION_MAX_AGE`), and any request that uses it re-issues it
+  once it is older than a day (`SESSION_RENEW_AFTER`), so a session cannot
+  lapse mid-lecture. The renewal lives in a middleware in `app/main.py`, not
+  in the `current_user` dependency: FastAPI merges a dependency's response
+  headers only when the endpoint returns data to serialise, so a cookie set
+  there is silently dropped by anything returning a `Response` directly —
+  `qr.svg`, the SPA fallback. `tests/test_session_cookie.py` pins that case.
+- **The session secret must be identical in every process.** It is generated
+  once and stored at `<data>/session_secret`, created with `O_EXCL` so that
+  concurrent cold starts adopt one value instead of each writing its own, and
+  cached per process. Both properties matter: a check-then-write race produced
+  six different secrets from one cold start, and as a plain property it was
+  re-evaluated on *every request*, so a cookie handed out by one request was
+  rejected by the next. `GET /api/health` reports `instance` and a truncated
+  hash of the secret — if either changes between two calls to the same URL,
+  processes disagree and logins fail at random. Setting `SESSION_SECRET`
+  explicitly sidesteps all of it.
+- **Why a cookie was rejected is worth distinguishing.** `SignatureExpired`
+  subclasses `BadSignature`, so catching only the latter reports every routine
+  expiry as a forged cookie — which cost real debugging time. Expiry says
+  *Session expired*; *Invalid session* means specifically that the server
+  could not verify an in-date cookie, i.e. the session secret is not what
+  signed it.
 
 ## Deployment: SciLifeLab Serve
 

backend/app/auth.py

@@ -1,5 +1,7 @@
+from datetime import datetime, timedelta, timezone
+
 from fastapi import Depends, HTTPException, Request, Response, status
-from itsdangerous import BadSignature, URLSafeTimedSerializer
+from itsdangerous import BadSignature, SignatureExpired, URLSafeTimedSerializer
 from sqlalchemy import select
 from sqlalchemy.orm import Session
 
@@ -8,7 +10,23 @@
 from .models import Role, User
 
 COOKIE_NAME = "quizbinf_session"
-SESSION_MAX_AGE = 12 * 3600  # one lecture day
+
+# How long a cookie stays valid without being used. Renewal below means this
+# is an *idle* timeout: someone who keeps using the app is never signed out,
+# so a session cannot lapse in the middle of a lecture.
+SESSION_MAX_AGE = 7 * 24 * 3600  # a week away from the app
+
+# Re-issue a cookie once it is older than this. Well short of the window, so
+# an active session is always far from expiring, and rare enough that it costs
+# one Set-Cookie per client per day rather than one per request.
+SESSION_RENEW_AFTER = 24 * 3600
+
+# Where current_user records that the cookie it accepted is due for renewal.
+# A dependency cannot set the cookie itself: FastAPI merges a dependency's
+# response headers only when the endpoint returns data to serialise, so
+# anything returning a Response directly — qr.svg, the SPA fallback — would
+# silently drop it. The middleware in main.py applies this to every response.
+RENEW_FLAG = "renew_session_for"
 
 
 def _serializer(settings: Settings) -> URLSafeTimedSerializer:
@@ -56,12 +74,26 @@ def current_user(
     if not token:
         raise HTTPException(status.HTTP_401_UNAUTHORIZED, "Not logged in")
     try:
-        data = _serializer(settings).loads(token, max_age=SESSION_MAX_AGE)
+        data, issued_at = _serializer(settings).loads(
+            token, max_age=SESSION_MAX_AGE, return_timestamp=True
+        )
+    except SignatureExpired:
+        # Ordinary and expected after SESSION_MAX_AGE. Reported separately
+        # because SignatureExpired subclasses BadSignature, so folding the two
+        # together makes a routine expiry look like a forged cookie — which
+        # sent us hunting a session-secret bug that did not exist.
+        raise HTTPException(status.HTTP_401_UNAUTHORIZED, "Session expired")
     except BadSignature:
+        # Signed with a different secret, or tampered with.
         raise HTTPException(status.HTTP_401_UNAUTHORIZED, "Invalid session")
     user = db.scalar(select(User).where(User.username == data["username"]))
     if user is None:
         raise HTTPException(status.HTTP_401_UNAUTHORIZED, "Unknown user")
+
+    # Push the expiry back while the session is in use, so the window measures
+    # time away from the app rather than time since logging in.
+    if datetime.now(timezone.utc) - issued_at > timedelta(seconds=SESSION_RENEW_AFTER):
+        setattr(request.state, RENEW_FLAG, user.username)
     return user
 
 

backend/app/config.py

@@ -1,13 +1,51 @@
+import logging
 import os
 import secrets
-from functools import lru_cache
+from functools import cached_property, lru_cache
 from pathlib import Path
 
 from pydantic_settings import BaseSettings, SettingsConfigDict
 
+log = logging.getLogger("quizbinf")
+
 # Placeholder that must never be used to sign real cookies.
 DEV_SECRET = "dev-only-secret"
 
+
+def _read_secret(path: Path) -> str | None:
+    try:
+        return path.read_text().strip() or None
+    except FileNotFoundError:
+        return None
+    except OSError as exc:
+        # Typically a file left by an earlier image that ran as another uid.
+        log.error("cannot read the session secret at %s: %s", path, exc)
+        return None
+
+
+def _create_secret(path: Path) -> str | None:
+    """Create the secret file, or adopt one another process created first.
+
+    O_EXCL makes exactly one caller the creator. The obvious alternative —
+    check whether the file exists, then write it — is wrong in two ways that
+    both end in mismatched cookies: several callers starting together each
+    generate their own and the last write wins, and a plain write truncates,
+    so a concurrent reader sees an empty file and generates yet another.
+    """
+    generated = secrets.token_urlsafe(48)
+    try:
+        fd = os.open(path, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o600)
+    except FileExistsError:
+        # Lost the race, and that is fine: the winner's secret is the one
+        # every other process will read.
+        return _read_secret(path)
+    except OSError as exc:
+        log.error("cannot create a session secret at %s: %s", path, exc)
+        return None
+    with os.fdopen(fd, "w") as fh:
+        fh.write(generated)
+    return generated
+
 # SciLifeLab Serve offers no way to set environment variables on an app, so
 # settings are also read from a file on the mounted persistent volume. Upload
 # `quizbinf.env` into the project storage to configure a Serve deployment.
@@ -93,34 +131,39 @@ def resolved_database_url(self) -> str:
                 return f"sqlite:///{db_file}"
         return "sqlite:///./quizbinf.db"
 
-    @property
+    @cached_property
     def resolved_session_secret(self) -> str:
         """Explicit setting, else a random secret persisted on the volume.
 
         The image is public, so a hardcoded default would let anyone forge a
         session cookie. Generating once and storing it next to the database
         keeps logins valid across restarts without any configuration.
+
+        Cached, because every request verifies a cookie against this: as a
+        plain property it was recomputed per request, so anything that made
+        two evaluations disagree logged people out between one request and
+        the next.
         """
         if self.session_secret and self.session_secret != DEV_SECRET:
             return self.session_secret
         data = self._writable_data_dir()
         if data is not None:
             secret_file = data / "session_secret"
-            try:
-                if secret_file.exists():
-                    stored = secret_file.read_text().strip()
-                    if stored:
-                        return stored
-                generated = secrets.token_urlsafe(48)
-                secret_file.write_text(generated)
-                secret_file.chmod(0o600)
-                return generated
-            except OSError:
-                pass
+            stored = _read_secret(secret_file) or _create_secret(secret_file)
+            if stored:
+                return stored
         if self.session_secret:
             return self.session_secret
-        # No persistence available: random per process. Sessions do not
-        # survive a restart, which is safer than a known constant.
+        # Nothing readable and nothing writable. A per-process random secret
+        # is safer than a known constant, but it rejects every cookie issued
+        # by any other process — so say so rather than failing as a 401.
+        log.error(
+            "no session secret could be read or stored under %s: signing cookies "
+            "with a per-process random value. Logins will break on restart and "
+            "whenever more than one worker runs. Set SESSION_SECRET in %s.",
+            self.data_dir,
+            VOLUME_ENV_FILE,
+        )
         return secrets.token_urlsafe(48)
 
 

backend/app/main.py

@@ -1,11 +1,15 @@
+import hashlib
 import logging
+import re
+import secrets
 from contextlib import asynccontextmanager
 from pathlib import Path
 
-from fastapi import FastAPI
+from fastapi import FastAPI, HTTPException, Request, status
 from fastapi.middleware.cors import CORSMiddleware
 from starlette.responses import FileResponse
 
+from .auth import RENEW_FLAG, set_session_cookie
 from .config import VOLUME_ENV_FILE, get_settings
 from .db import Base, engine
 from .routers import auth, images, markdown, quizzes, sessions
@@ -39,6 +43,11 @@ def log_startup_summary() -> None:
     else:
         log.info("data dir %s is writable", data)
 
+    # A truncated hash, never the secret. Two instances printing different
+    # values will reject each other's cookies.
+    digest = hashlib.sha256(s.resolved_session_secret.encode()).hexdigest()[:8]
+    log.info("instance %s signing cookies with secret %s…", INSTANCE_ID, digest)
+
     url = s.resolved_database_url
     # Only the scheme and, for SQLite, the path — a Postgres URL holds a password.
     log.info(
@@ -84,30 +93,82 @@ async def lifespan(app: FastAPI):
         allow_headers=["*"],
     )
 
+@app.middleware("http")
+async def renew_session_cookie(request: Request, call_next):
+    """Slide the session window forward on a request that used the cookie.
+
+    Applied here rather than in the `current_user` dependency because FastAPI
+    merges a dependency's response headers only when the endpoint returns data
+    to serialise (routing.py: a returned Response is used as-is). An endpoint
+    like qr.svg returns a FileResponse, so a cookie set from the dependency
+    would be dropped without a trace. Middleware sees the final response
+    whatever produced it.
+    """
+    response = await call_next(request)
+    username = getattr(request.state, RENEW_FLAG, None)
+    if username:
+        set_session_cookie(response, username, get_settings())
+    return response
+
+
 app.include_router(auth.router)
 app.include_router(images.router)
 app.include_router(markdown.router)
 app.include_router(quizzes.router)
 app.include_router(sessions.router)
 
 
+# Identifies this process across requests. Two different values coming back
+# from the same URL mean more than one instance is serving it.
+INSTANCE_ID = secrets.token_hex(4)
+
+
 @app.get("/api/health")
 def health() -> dict:
-    """Liveness, plus whether anything written here will survive a restart.
+    """Liveness, plus the two things that silently break logins.
+
+    Neither announces itself. Non-persistent storage means the database is
+    thrown away on every restart; a session secret that differs between
+    processes means a cookie issued by one is rejected by the next, which
+    shows up as an unexplained 401 rather than as anything about secrets.
 
-    Reported because non-persistent storage does not announce itself: the app
-    runs, but the database is thrown away on every restart and the session
-    secret is regenerated, which invalidates everyone's cookie. That surfaces
-    as unrelated-looking failures — vanished sessions, "invalid session" —
-    so it is worth being able to check directly.
+    `secret` is a truncated hash, never the secret: enough to compare two
+    instances, useless for forging a cookie. Repeat the request a few times —
+    if `instance` or `secret` changes between calls, requests are being served
+    by processes that do not agree, and logins will fail at random.
     """
     settings = get_settings()
+    fingerprint = hashlib.sha256(settings.resolved_session_secret.encode()).hexdigest()
     return {
         "status": "ok",
         "storage": "persistent" if settings._writable_data_dir() else "ephemeral",
+        "instance": INSTANCE_ID,
+        "secret": fingerprint[:8],
     }
 
 
+def looks_like_asset(full_path: str) -> bool:
+    """Whether a path is asking for a build artefact rather than an SPA route.
+
+    Angular's routes never contain a dot ("/s/<code>", "/teacher/session/…"),
+    while every emitted artefact has an extension, so the last segment having
+    one separates them.
+    """
+    return "." in full_path.rsplit("/", 1)[-1]
+
+
+# Angular fingerprints its output ("main-3WBHVWMP.js"). A fingerprinted name
+# describes exactly one build, so it can be cached forever; anything else may
+# be replaced in place by the next deploy and has to be revalidated.
+_FINGERPRINTED = re.compile(r"-[A-Z0-9]{8,}\.[a-z0-9]+$")
+
+
+def cache_control_for(path: Path) -> str:
+    if _FINGERPRINTED.search(path.name):
+        return "public, max-age=31536000, immutable"
+    return "no-cache"
+
+
 def static_file_for(full_path: str) -> Path | None:
     """The built asset a request refers to, or None to fall back to the SPA.
 
@@ -136,5 +197,17 @@ def spa(full_path: str) -> FileResponse:
         # index.html so Angular's router handles /s/<code> etc.
         asset = static_file_for(full_path)
         if asset is not None:
-            return FileResponse(asset)
-        return FileResponse(STATIC_DIR / "index.html")
+            return FileResponse(asset, headers={"Cache-Control": cache_control_for(asset)})
+
+        # A missing artefact must 404 rather than fall through to the SPA.
+        # Answering a ".js" URL with index.html produces "Expected a
+        # JavaScript-or-Wasm module script but the server responded with a MIME
+        # type of text/html" instead of a plain 404, and the browser may then
+        # cache that HTML under the script's URL.
+        if looks_like_asset(full_path):
+            raise HTTPException(status.HTTP_404_NOT_FOUND, "No such file")
+
+        # index.html names the fingerprinted bundles of one specific build, so
+        # a cached copy outlives the deploy that produced it and asks for chunks
+        # that no longer exist. It must be revalidated every time.
+        return FileResponse(STATIC_DIR / "index.html", headers={"Cache-Control": "no-cache"})