From 77f8f9f800779d886cbe1c5529277f3dc8aa2a94 Mon Sep 17 00:00:00 2001 From: Matt Mabrey Date: Thu, 3 Sep 2026 11:13:28 -0700 Subject: [PATCH] Fix T10.2: install SessionMiddleware, required by Authlib MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit okta_login() and okta_callback() (T10.2) both crash with a 500 (AssertionError: SessionMiddleware must be installed to access request.session) against a real Okta client, because Authlib's authorize_redirect() and authorize_access_token() both store/read OIDC state and nonce in request.session. Never caught by T10.2's or T10.3's own verification because every prior test mocked authorize_redirect / authorize_access_token directly, bypassing Authlib's real implementation entirely. Found while starting T10.5 and reproducing the real flow. Adds starlette.middleware.sessions.SessionMiddleware, on its own cookie (wp_oauth_state, distinct from the app's real session cookie wp_session) with a short 10-minute lifetime and same_site=lax so it survives the top-level redirect back from Okta. This cookie carries nothing but ephemeral per-attempt OAuth state — no identity, no long-term secret — so it reuses auth.SECRET_KEY rather than adding a new required config knob. Reused in T10.5 to carry the post-login redirect target across the same round trip. Adds itsdangerous to requirements.txt — SessionMiddleware's hard dependency, not previously needed anywhere in this app. Verified: reproduced the crash against server.app with a fake (network- bypassed) Authlib client and no SessionMiddleware, confirmed the AssertionError, then confirmed the same request succeeds (302 to the authorize URL, wp_oauth_state cookie set) once the middleware is added. wave-10.md T10.2 (bug fix) --- server/app.py | 17 +++++++++++++++++ server/requirements.txt | 2 ++ 2 files changed, 19 insertions(+) diff --git a/server/app.py b/server/app.py index c1c4e6b..f45b7a1 100644 --- a/server/app.py +++ b/server/app.py @@ -24,6 +24,7 @@ from fastapi.staticfiles import StaticFiles from pydantic import BaseModel, ConfigDict, Field from sqlalchemy import select, delete, func from sqlalchemy.orm import Session +from starlette.middleware.sessions import SessionMiddleware from .db import Base, engine, get_db from . import models, auth, notify, assets_db, okta_auth @@ -58,6 +59,22 @@ if _origins: allow_methods=["*"], allow_headers=["*"], expose_headers=["X-Total-Count"], ) +# Authlib's Okta client needs request.session to carry the OIDC state/nonce (and, +# below, our own post-login redirect target) across the round trip to Okta and +# back — it raises an AssertionError without this. Bug found in T10.2 (those +# routes never crashed in testing because every prior check mocked +# authorize_redirect/authorize_access_token directly, bypassing Authlib's real +# implementation); fixed here rather than reworking already-shipped T10.2 code. +# +# This is NOT the app's session cookie — wp_session (auth.py) still carries the +# actual signed-in identity, unchanged. This cookie holds nothing but ephemeral, +# per-attempt OAuth state, so it gets a short lifetime and a plain secret reuse +# (auth.SECRET_KEY) rather than its own required config knob. +app.add_middleware( + SessionMiddleware, secret_key=auth.SECRET_KEY, session_cookie="wp_oauth_state", + same_site="lax", https_only=False, max_age=600, +) + # ── Authentication gate ──────────────────────────────────────────────────────── # Every /api/ data route requires a valid session cookie. Login, health, and the diff --git a/server/requirements.txt b/server/requirements.txt index adb8568..8bcd040 100644 --- a/server/requirements.txt +++ b/server/requirements.txt @@ -21,3 +21,5 @@ PyJWT==2.13.0 # signed session tokens starlette==1.3.1 # pinned transitive (cookie / CORS handling — security-relevant) Authlib==1.7.2 # Okta OIDC authorization-code flow (T10.1, wave 10 / D15) httpx==0.28.1 # Authlib's OIDC client needs an HTTP client; explicit, not transitive +itsdangerous==2.2.0 # signs the OAuth-state cookie SessionMiddleware sets — required by + # Authlib's authorize_redirect/authorize_access_token, not optional