From 73bf7b4c3d069dd83543ea9cc018930eb5f227bb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 17:17:17 +0000 Subject: [PATCH 1/4] 404 missing bundles, and stop caching index.html A redeploy left browsers reporting Failed to load module script: Expected a JavaScript-or-Wasm module script but the server responded with a MIME type of "text/html" for chunk-H3QWKNOY.js and chunk-UNPCGV2O.js. Angular fingerprints its bundles and index.html names the exact set belonging to one build, so a cached index.html outlives the deploy that produced it and asks for chunks the new build does not contain. The SPA fallback answered those with index.html, turning a missing file into a MIME-type error that says nothing about the cause, and letting the browser cache HTML under a script URL. Two rules: a path whose last segment has an extension is an artefact request and 404s when absent rather than falling through, and index.html is served no-cache so it is revalidated on every load. Fingerprinted bundles are immutable by construction and now say so. The e2e specs were confirmed to fail against the previous behaviour with the reported symptom before the fix was applied. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018u3wNzXNdrPw2Z59WjGHYv --- backend/app/main.py | 41 +++++++++++++++++++++-- backend/tests/test_static_serving.py | 36 ++++++++++++++++++++ frontend/e2e/asset-serving.spec.mjs | 49 ++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 backend/tests/test_static_serving.py create mode 100644 frontend/e2e/asset-serving.spec.mjs diff --git a/backend/app/main.py b/backend/app/main.py index 78e823c..5fd34b3 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -1,8 +1,9 @@ import logging +import re from contextlib import asynccontextmanager from pathlib import Path -from fastapi import FastAPI +from fastapi import FastAPI, HTTPException, status from fastapi.middleware.cors import CORSMiddleware from starlette.responses import FileResponse @@ -108,6 +109,28 @@ def health() -> dict: } +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/", "/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 +159,17 @@ def spa(full_path: str) -> FileResponse: # index.html so Angular's router handles /s/ 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"}) diff --git a/backend/tests/test_static_serving.py b/backend/tests/test_static_serving.py new file mode 100644 index 0000000..45985f9 --- /dev/null +++ b/backend/tests/test_static_serving.py @@ -0,0 +1,36 @@ +"""How build artefacts are told apart from SPA routes, and how long they cache. + +The rules here are what keep a redeploy from stranding browsers on a cached +index.html that names bundles the new build no longer contains. +""" + +from pathlib import Path + +from app import main + + +def test_asset_paths_are_told_apart_from_routes(): + # Angular routes never contain a dot; every emitted artefact has one. + for route in ("login", "teacher", "s/abc123", "teacher/session/abc123/join"): + assert not main.looks_like_asset(route), route + + for asset in ( + "main-3WBHVWMP.js", + "chunk-H3QWKNOY.js", + "styles-5Z6IZAMC.css", + "favicon.ico", + "media/figure.png", + ): + assert main.looks_like_asset(asset), asset + + +def test_only_fingerprinted_bundles_are_cached_forever(): + # A fingerprinted name describes exactly one build, so it can never go + # stale — the next build emits a different name. + for name in ("main-3WBHVWMP.js", "chunk-H3QWKNOY.js", "styles-5Z6IZAMC.css"): + assert main.cache_control_for(Path(name)) == "public, max-age=31536000, immutable", name + + # Everything else may be replaced in place by a deploy, so it must be + # revalidated. index.html above all: it names the fingerprinted bundles. + for name in ("index.html", "favicon.ico", "manifest.webmanifest"): + assert main.cache_control_for(Path(name)) == "no-cache", name diff --git a/frontend/e2e/asset-serving.spec.mjs b/frontend/e2e/asset-serving.spec.mjs new file mode 100644 index 0000000..c2f3dd2 --- /dev/null +++ b/frontend/e2e/asset-serving.spec.mjs @@ -0,0 +1,49 @@ +import { expect, test } from '@playwright/test'; + +/** + * How the backend serves the built frontend, asserted against the real server + * rather than a unit stub — this is the seam where a deploy strands browsers. + * + * Angular fingerprints its bundles, and index.html names the exact set + * belonging to one build. A browser holding a cached index.html from the + * previous deploy asks for chunks that no longer exist; when the SPA fallback + * answers those with index.html, the browser reports + * + * Failed to load module script: Expected a JavaScript-or-Wasm module script + * but the server responded with a MIME type of "text/html" + * + * which says nothing about the actual cause. Two rules prevent it: a missing + * artefact 404s, and index.html is always revalidated. + */ +test('a missing bundle 404s instead of returning the SPA', async ({ request }) => { + const res = await request.get('/chunk-DOESNOTEXIST.js'); + + expect(res.status(), 'a missing .js must 404, not fall through to index.html').toBe(404); + expect(res.headers()['content-type'] ?? '').not.toContain('text/html'); +}); + +test('index.html is revalidated, fingerprinted bundles are cached', async ({ request }) => { + const index = await request.get('/'); + expect(index.status()).toBe(200); + // Without this a redeploy leaves clients on an index that names dead chunks. + expect(index.headers()['cache-control'] ?? '').toContain('no-cache'); + + // Find a real fingerprinted bundle from the page the server just served. + const html = await index.text(); + const bundle = html.match(/(?:src|href)="\/?([^"]*-[A-Z0-9]{8,}\.(?:js|css))"/); + expect(bundle, 'index.html should reference a fingerprinted bundle').not.toBeNull(); + + const asset = await request.get('/' + bundle[1].replace(/^\//, '')); + expect(asset.status()).toBe(200); + expect(asset.headers()['cache-control'] ?? '').toContain('immutable'); +}); + +test('SPA routes still reach the app', async ({ request }) => { + // Dotless paths are routes, not files: these must keep getting index.html + // even though nothing exists on disk under those names. + for (const route of ['/login', '/teacher', '/s/abc123', '/teacher/session/abc123/join']) { + const res = await request.get(route); + expect(res.status(), `${route} should serve the SPA`).toBe(200); + expect(res.headers()['content-type'] ?? '', route).toContain('text/html'); + } +}); From 112424a34557a73e1637a250e38c1b70f92bebc6 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 17:29:33 +0000 Subject: [PATCH 2/4] Tell an expired session apart from an unverifiable one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SignatureExpired subclasses BadSignature, so catching only the latter reported every routine 12-hour expiry as "Invalid session" — the message that means the server's session secret is not what signed the cookie. That sent us looking for a session-secret bug when the cookie had simply aged out. Expiry now reports "Session expired". "Invalid session" keeps its narrower and much more interesting meaning: a well-formed, in-date cookie this server cannot verify. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018u3wNzXNdrPw2Z59WjGHYv --- backend/app/auth.py | 9 ++++- backend/tests/test_session_cookie.py | 58 ++++++++++++++++++++++++++++ frontend/src/app/auth.interceptor.ts | 9 +++-- 3 files changed, 72 insertions(+), 4 deletions(-) create mode 100644 backend/tests/test_session_cookie.py diff --git a/backend/app/auth.py b/backend/app/auth.py index e2a30aa..39ccc0c 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -1,5 +1,5 @@ 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 @@ -57,7 +57,14 @@ def current_user( raise HTTPException(status.HTTP_401_UNAUTHORIZED, "Not logged in") try: data = _serializer(settings).loads(token, max_age=SESSION_MAX_AGE) + 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: diff --git a/backend/tests/test_session_cookie.py b/backend/tests/test_session_cookie.py new file mode 100644 index 0000000..667356d --- /dev/null +++ b/backend/tests/test_session_cookie.py @@ -0,0 +1,58 @@ +"""Why a cookie was rejected. + +An expired cookie and a cookie signed with the wrong secret both mean "log in +again" to the user, but they mean very different things to whoever is +debugging a deployment: one is routine, the other says the server's session +secret is not what it was. SignatureExpired subclasses BadSignature, so +catching only the latter reports every routine expiry as a forged cookie. +""" + +import pytest +from itsdangerous import URLSafeTimedSerializer + +from app.auth import COOKIE_NAME, SESSION_MAX_AGE +from app.config import get_settings +from tests.conftest import login + + +@pytest.fixture +def logged_in(client): + login(client, "teach") + return client + + +def test_a_valid_cookie_is_accepted(logged_in): + assert logged_in.get("/api/auth/me").status_code == 200 + + +def test_an_expired_cookie_says_so(logged_in, monkeypatch): + # Age the cookie past the window rather than waiting 12 hours for it. + monkeypatch.setattr("app.auth.SESSION_MAX_AGE", -1) + + resp = logged_in.get("/api/auth/me") + assert resp.status_code == 401 + assert resp.json()["detail"] == "Session expired" + + +def test_a_cookie_signed_with_another_secret_is_invalid(client): + # What a changed session secret actually looks like: a well-formed, in-date + # cookie that this server cannot verify. + forged = URLSafeTimedSerializer("not-the-real-secret", salt="quizbinf-auth") + client.cookies.set(COOKIE_NAME, forged.dumps({"username": "teach"})) + + resp = client.get("/api/auth/me") + assert resp.status_code == 401 + assert resp.json()["detail"] == "Invalid session" + + +def test_no_cookie_is_reported_as_not_logged_in(client): + resp = client.get("/api/auth/me") + assert resp.status_code == 401 + assert resp.json()["detail"] == "Not logged in" + + +def test_the_session_secret_is_stable_across_calls(): + # A per-process secret would invalidate every cookie on restart; this is + # the property that makes "Invalid session" meaningful as a signal. + assert get_settings().resolved_session_secret == get_settings().resolved_session_secret + assert SESSION_MAX_AGE > 0 diff --git a/frontend/src/app/auth.interceptor.ts b/frontend/src/app/auth.interceptor.ts index 089b898..0792703 100644 --- a/frontend/src/app/auth.interceptor.ts +++ b/frontend/src/app/auth.interceptor.ts @@ -11,9 +11,12 @@ import { AuthService } from './auth.service'; * * Without this a rejected cookie surfaces as whatever each view makes of a * failed request — "Session not found.", a broken QR image — while the page - * still looks logged in, which is impossible to act on. A cookie can be - * rejected because it expired, or because the server's session secret changed - * (which happens on every restart when no writable volume is mounted). + * still looks logged in, which is impossible to act on. + * + * The common case is the plainest one: the cookie is older than + * SESSION_MAX_AGE, so a teacher who logged in yesterday finds every request + * failing today. The server distinguishes that ("Session expired") from a + * cookie it cannot verify ("Invalid session"); both mean log in again here. */ export const authErrorInterceptor: HttpInterceptorFn = (req, next) => { const router = inject(Router); From 7d50200ad882602c46deda7a70a2b7c8d06b54b8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 17:39:59 +0000 Subject: [PATCH 3/4] Renew the session cookie on use, and give it a week MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The window was 12 hours from login, so a teacher who prepared the evening before was signed out by lecture time. It is now a week, and measured from last use rather than from login: a request made with a cookie older than a day re-issues it, so a session in use cannot lapse mid-lecture, while one genuinely abandoned still ages out. Renewal is a middleware rather than part of the current_user dependency. FastAPI merges a dependency's response headers only when the endpoint returns data to serialise; an endpoint returning a Response directly gets its own headers untouched (routing.py: `response = raw_response`). Setting the cookie in the dependency therefore works for every JSON endpoint and silently does nothing for qr.svg and the SPA fallback — verified against a minimal app, and pinned by a test that renews through qr.svg. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018u3wNzXNdrPw2Z59WjGHYv --- CLAUDE.md | 14 ++++++ backend/app/auth.py | 29 ++++++++++- backend/app/main.py | 21 +++++++- backend/tests/test_session_cookie.py | 73 ++++++++++++++++++++++++++-- frontend/src/app/auth.interceptor.ts | 8 +-- 5 files changed, 134 insertions(+), 11 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1c6aa44..3a3245a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -134,6 +134,20 @@ 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. +- **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 diff --git a/backend/app/auth.py b/backend/app/auth.py index 39ccc0c..6de4d65 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -1,3 +1,5 @@ +from datetime import datetime, timedelta, timezone + from fastapi import Depends, HTTPException, Request, Response, status from itsdangerous import BadSignature, SignatureExpired, URLSafeTimedSerializer from sqlalchemy import select @@ -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,7 +74,9 @@ 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 @@ -69,6 +89,11 @@ def current_user( 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 diff --git a/backend/app/main.py b/backend/app/main.py index 5fd34b3..7ec0b2e 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -3,10 +3,11 @@ from contextlib import asynccontextmanager from pathlib import Path -from fastapi import FastAPI, HTTPException, status +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 @@ -85,6 +86,24 @@ 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) diff --git a/backend/tests/test_session_cookie.py b/backend/tests/test_session_cookie.py index 667356d..42ccc51 100644 --- a/backend/tests/test_session_cookie.py +++ b/backend/tests/test_session_cookie.py @@ -7,12 +7,14 @@ catching only the latter reports every routine expiry as a forged cookie. """ +import time + import pytest from itsdangerous import URLSafeTimedSerializer -from app.auth import COOKIE_NAME, SESSION_MAX_AGE +from app.auth import COOKIE_NAME, SESSION_MAX_AGE, SESSION_RENEW_AFTER from app.config import get_settings -from tests.conftest import login +from tests.conftest import login, make_quiz_with_question @pytest.fixture @@ -21,12 +23,20 @@ def logged_in(client): return client +def _issued_at(token: str): + """When a cookie was signed, read back out of the token itself.""" + serializer = URLSafeTimedSerializer( + get_settings().resolved_session_secret, salt="quizbinf-auth" + ) + return serializer.loads(token, return_timestamp=True)[1] + + def test_a_valid_cookie_is_accepted(logged_in): assert logged_in.get("/api/auth/me").status_code == 200 def test_an_expired_cookie_says_so(logged_in, monkeypatch): - # Age the cookie past the window rather than waiting 12 hours for it. + # Age the cookie past the window rather than waiting a week for it. monkeypatch.setattr("app.auth.SESSION_MAX_AGE", -1) resp = logged_in.get("/api/auth/me") @@ -55,4 +65,59 @@ def test_the_session_secret_is_stable_across_calls(): # A per-process secret would invalidate every cookie on restart; this is # the property that makes "Invalid session" meaningful as a signal. assert get_settings().resolved_session_secret == get_settings().resolved_session_secret - assert SESSION_MAX_AGE > 0 + + +def test_a_session_lasts_at_least_a_week_when_left_alone(): + assert SESSION_MAX_AGE >= 7 * 24 * 3600 + # Renewal has to happen well inside the window, or it never gets the + # chance to run before the cookie it would renew has already expired. + assert 0 < SESSION_RENEW_AFTER < SESSION_MAX_AGE + + +def test_a_fresh_cookie_is_not_reissued(logged_in): + # One Set-Cookie per client per day, not one per request. + assert "set-cookie" not in logged_in.get("/api/auth/me").headers + + +def test_using_the_app_pushes_the_expiry_back(logged_in, monkeypatch): + # Treat every cookie as due for renewal rather than waiting a day. + monkeypatch.setattr("app.auth.SESSION_RENEW_AFTER", -1) + before = logged_in.cookies[COOKIE_NAME] + + # Signed timestamps have one-second resolution, so a reissue inside the + # same second is byte-identical and would prove nothing about the window + # having moved. Wait out the tick. + time.sleep(1.1) + + resp = logged_in.get("/api/auth/me") + assert resp.status_code == 200 + assert "set-cookie" in resp.headers, "an in-use session was not renewed" + + # The replacement is a genuinely newer cookie, and it works. + after = logged_in.cookies[COOKIE_NAME] + assert after != before + assert _issued_at(after) > _issued_at(before), "the expiry did not move" + assert logged_in.get("/api/auth/me").status_code == 200 + + +def test_renewal_reaches_endpoints_that_return_a_response_directly(teacher_client, monkeypatch): + """qr.svg returns a FileResponse, and those take a different path out. + + FastAPI merges a dependency's response headers only when the endpoint + returns data to serialise, so setting the cookie in the dependency would + be dropped here without any error — the projected view would quietly stop + renewing while every JSON endpoint kept working. + """ + monkeypatch.setattr("app.auth.SESSION_RENEW_AFTER", -1) + quiz_id, _, _ = make_quiz_with_question(teacher_client) + code = teacher_client.post(f"/api/sessions?quiz_id={quiz_id}").json()["code"] + + resp = teacher_client.get(f"/api/sessions/{code}/qr.svg") + assert resp.status_code == 200 + assert resp.headers["content-type"].startswith("image/svg+xml") + assert "set-cookie" in resp.headers, "a Response-returning endpoint did not renew" + + +def test_an_unauthenticated_request_is_not_given_a_cookie(client, monkeypatch): + monkeypatch.setattr("app.auth.SESSION_RENEW_AFTER", -1) + assert "set-cookie" not in client.get("/api/auth/me").headers diff --git a/frontend/src/app/auth.interceptor.ts b/frontend/src/app/auth.interceptor.ts index 0792703..503872a 100644 --- a/frontend/src/app/auth.interceptor.ts +++ b/frontend/src/app/auth.interceptor.ts @@ -13,10 +13,10 @@ import { AuthService } from './auth.service'; * failed request — "Session not found.", a broken QR image — while the page * still looks logged in, which is impossible to act on. * - * The common case is the plainest one: the cookie is older than - * SESSION_MAX_AGE, so a teacher who logged in yesterday finds every request - * failing today. The server distinguishes that ("Session expired") from a - * cookie it cannot verify ("Invalid session"); both mean log in again here. + * The server renews a cookie that is in use, so this should now only be + * reached after a real week away — or when the cookie cannot be verified at + * all. It distinguishes the two ("Session expired" vs "Invalid session"); + * both mean log in again here. */ export const authErrorInterceptor: HttpInterceptorFn = (req, next) => { const router = inject(Router); From efcdfbb07264b1fa903359b2fc087518dd06cb5e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 17:45:32 +0000 Subject: [PATCH 4/4] Stop regenerating the session secret MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The secret every request verifies the session cookie against was a plain @property, so it was recomputed on each request, and its create path checked whether the file existed and then wrote it. Both are wrong: - Concurrent callers with no file yet each generated their own and overwrote the last. A test driving twelve at once produced six different secrets from a single cold start. - A plain write truncates, so a concurrent reader saw an empty file and generated yet another. - An unreadable file (one left by an image that ran as a different uid) fell through `except OSError: pass` to a per-process random value, silently. The symptom is a cookie accepted by one request and rejected by the next, reported only as 401 — POST /api/sessions succeeding and every follow-up GET for that session failing. The file is now created with O_EXCL, so exactly one caller wins and the rest adopt its value, and the result is cached per process. The last-resort random secret is logged as an error rather than used in silence. /api/health additionally reports the instance id and a truncated hash of the secret, so two processes that disagree can be told apart from outside the container; the same line is logged at startup. The hash is 32 bits of a 384-bit token — enough to compare, useless for forging. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018u3wNzXNdrPw2Z59WjGHYv --- CLAUDE.md | 10 ++++ backend/app/config.py | 73 +++++++++++++++++++++------ backend/app/main.py | 31 +++++++++--- backend/tests/test_session_secret.py | 75 ++++++++++++++++++++++++++++ 4 files changed, 168 insertions(+), 21 deletions(-) create mode 100644 backend/tests/test_session_secret.py diff --git a/CLAUDE.md b/CLAUDE.md index 3a3245a..4e7cfe9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -142,6 +142,16 @@ quizbinf/ 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 `/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 diff --git a/backend/app/config.py b/backend/app/config.py index bc4183c..17015e1 100644 --- a/backend/app/config.py +++ b/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) diff --git a/backend/app/main.py b/backend/app/main.py index 7ec0b2e..e2ace4b 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -1,5 +1,7 @@ +import hashlib import logging import re +import secrets from contextlib import asynccontextmanager from pathlib import Path @@ -41,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( @@ -111,20 +118,32 @@ async def renew_session_cookie(request: Request, call_next): 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], } diff --git a/backend/tests/test_session_secret.py b/backend/tests/test_session_secret.py new file mode 100644 index 0000000..a6c8f56 --- /dev/null +++ b/backend/tests/test_session_secret.py @@ -0,0 +1,75 @@ +"""Where the cookie-signing secret comes from. + +Every request verifies the session cookie against this value, so anything that +lets it differ between two adjacent requests logs everybody out at random: one +call succeeds, the next answers 401 with no state having changed. +""" + +import logging +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path + +from app.config import Settings + + +def _settings(tmp_path: Path) -> Settings: + return Settings(data_dir=str(tmp_path), session_secret=None) + + +def test_the_secret_is_generated_once_and_persisted(tmp_path): + s = _settings(tmp_path) + first = s.resolved_session_secret + + assert (tmp_path / "session_secret").read_text().strip() == first + # A second process reading the same volume must agree. + assert _settings(tmp_path).resolved_session_secret == first + + +def test_the_secret_does_not_change_between_requests(tmp_path): + """It is read on every request, so re-reading must not re-generate.""" + s = _settings(tmp_path) + assert len({s.resolved_session_secret for _ in range(50)}) == 1 + + +def test_a_concurrent_cold_start_agrees_on_one_secret(tmp_path): + """The case that actually bites: several requests arriving at once with + no secret file yet — the first burst after a deploy, or after the file has + been deleted. + + A check-then-write leaves every racer generating its own and overwriting + the last, so the cookie handed out by one request is rejected by the next. + """ + settings = [_settings(tmp_path) for _ in range(12)] + with ThreadPoolExecutor(max_workers=12) as pool: + secrets_seen = set(pool.map(lambda s: s.resolved_session_secret, settings)) + + assert len(secrets_seen) == 1, ( + f"{len(secrets_seen)} different secrets from one cold start; " + "cookies signed by one worker would be rejected by another" + ) + assert (tmp_path / "session_secret").read_text().strip() in secrets_seen + + +def test_an_unreadable_secret_file_is_reported_loudly(tmp_path, caplog): + """Falling back silently is the worst outcome: a per-process random secret + rejects every cookie the moment there is more than one process, and looks + exactly like a mysterious 401. + """ + secret_file = tmp_path / "session_secret" + secret_file.write_text("stored-secret") + secret_file.chmod(0o000) + + s = _settings(tmp_path) + with caplog.at_level(logging.ERROR): + value = s.resolved_session_secret + + if value == "stored-secret": # running as root, which ignores the mode + return + assert caplog.records, "an unusable session secret was not reported" + + +def test_an_explicit_secret_wins(tmp_path): + s = Settings(data_dir=str(tmp_path), session_secret="configured") + assert s.resolved_session_secret == "configured" + # And nothing is written to the volume when it was configured. + assert not (tmp_path / "session_secret").exists()