From c9e0655d6ef3e554070216f74c2d31c2afc70aad Mon Sep 17 00:00:00 2001 From: Daniel Date: Sun, 13 Sep 2026 05:55:37 +0200 Subject: [PATCH] fix: shut every password door under SSO-only, and keep the token out of the URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from a security pass on the SSO path, both real. sso_only gated login and the login codes and nothing else. Register, forgot-password, reset-password, resend-verification and setting a password through PUT /auth/me all went through — so a site running single sign-on could still mint a password account nobody vetted, and if the flag were ever turned off, there it would be. One helper, five doors, 403 with a reason at each. And the access token travelled in a query string. The SSO redirect is a page load, so the browser asked nginx for /sso-callback?token= and nginx logs the request line — every sign-in wrote one into the frontend container's access log, the browser's history, and the Referer of whatever loaded next. It carries a one-time code now: a random 32 bytes parked in Redis for sixty seconds, traded at POST /auth/sso/exchange for the token, and deleted as it is read, so a code replayed from any of those places buys nothing. Also the OIDC state cookie, which is what stops an authorization response being replayed at you: same_site lax (strict drops it on the provider's top-level GET and fails every sign-in) and secure whenever APP_URL is https. And one cross, not two. The header's menu button is already a cross while a drawer is open, so the drawer's own close button was a second control an inch below it for the same job — gone from the article, the player and the review. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- backend/app/main.py | 17 ++- backend/app/routers/auth.py | 90 +++++++++++++++- backend/app/schemas/auth.py | 6 ++ backend/tests/api-contract.json | 8 ++ backend/tests/test_sso_hardening.py | 121 ++++++++++++++++++++++ frontend/src/components/ArticleReader.jsx | 6 +- frontend/src/pages/ArticlesPage.css | 5 - frontend/src/pages/QuizPage.jsx | 9 +- frontend/src/pages/QuizPlayer.css | 5 - frontend/src/pages/ResultsPage.jsx | 9 +- frontend/src/pages/SsoCallbackPage.jsx | 33 ++++-- 11 files changed, 273 insertions(+), 36 deletions(-) create mode 100644 backend/tests/test_sso_hardening.py diff --git a/backend/app/main.py b/backend/app/main.py index f80275e..4435697 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -576,9 +576,22 @@ app.add_middleware( expose_headers=["X-New-Token"], # Allow frontend to read this header ) -# Session middleware for OIDC state (authlib needs it) +# Session middleware for OIDC state (authlib needs it). +# +# The OIDC state and nonce live in this cookie for the length of one redirect, +# and they are what stops somebody replaying an authorization response at you. +# `lax` because the provider sends the browser back with a top-level GET, which +# lax allows and `strict` would drop — taking the state with it and failing +# every sign-in. Secure whenever the site is served over https, which is what +# APP_URL says; a plain-http development host keeps a cookie it can actually +# set. from starlette.middleware.sessions import SessionMiddleware -app.add_middleware(SessionMiddleware, secret_key=settings.SECRET_KEY) +app.add_middleware( + SessionMiddleware, + secret_key=settings.SECRET_KEY, + same_site="lax", + https_only=str(getattr(settings, "APP_URL", "")).startswith("https://"), +) # Add token refresh middleware AFTER CORS (middleware applies in reverse) from app.utils.auth import TokenRefreshMiddleware diff --git a/backend/app/routers/auth.py b/backend/app/routers/auth.py index 0b1ce3b..b856287 100644 --- a/backend/app/routers/auth.py +++ b/backend/app/routers/auth.py @@ -1,3 +1,4 @@ +import logging import secrets from datetime import datetime, timedelta @@ -11,7 +12,7 @@ from app.models.email_verification import EmailVerification from app.models.password_reset import PasswordReset from app.schemas.auth import ( UserCreate, UserResponse, Token, LoginRequest, LogoutRequest, RefreshRequest, - UserUpdateMe, ForgotPasswordRequest, ResetPasswordRequest, + UserUpdateMe, ForgotPasswordRequest, ResetPasswordRequest, SsoExchangeRequest, ) from app.services import email_service from app.utils.auth import ( @@ -19,6 +20,7 @@ from app.utils.auth import ( get_current_user, ) +logger = logging.getLogger(__name__) router = APIRouter() @@ -126,6 +128,7 @@ def signup_policy(db: Session = Depends(get_db)): @router.post("/register") async def register(user_data: UserCreate, background_tasks: BackgroundTasks, db: Session = Depends(get_db)): + _refuse_when_sso_only("Accounts are created through SSO on this site.") # Failing open: an hCaptcha outage should cost the site a little spam, not # every account that would have been created while it lasted. await captcha.require_human(user_data.captcha_token, fail_open=True) @@ -322,6 +325,7 @@ def verify_email(token: str, db: Session = Depends(get_db)): @router.post("/resend-verification") async def resend_verification(data: ForgotPasswordRequest, background_tasks: BackgroundTasks, db: Session = Depends(get_db)): + _refuse_when_sso_only("Accounts are created through SSO on this site.") email_normalized = data.email.lower().strip() user = db.query(User).filter(User.email == email_normalized).first() if not user: @@ -348,6 +352,7 @@ async def resend_verification(data: ForgotPasswordRequest, background_tasks: Bac @router.post("/forgot-password") async def forgot_password(data: ForgotPasswordRequest, background_tasks: BackgroundTasks, db: Session = Depends(get_db)): + _refuse_when_sso_only("There is no password to reset. Please use SSO.") email_normalized = data.email.lower().strip() _check_reset_rate_limit(db, email_normalized) @@ -371,6 +376,7 @@ async def forgot_password(data: ForgotPasswordRequest, background_tasks: Backgro @router.post("/reset-password") def reset_password(data: ResetPasswordRequest, db: Session = Depends(get_db)): + _refuse_when_sso_only("There is no password to reset. Please use SSO.") if len(data.new_password) < 8: raise HTTPException(status_code=400, detail="Password must be at least 8 characters") @@ -458,6 +464,10 @@ def get_me(current_user: User = Depends(get_current_user)): @router.put("/me") def update_me(data: UserUpdateMe, db: Session = Depends(get_db), current_user: User = Depends(get_current_user)): if data.new_password: + # Setting one is the same door. A password nobody can log in with is + # not harmless: it is a credential waiting for the flag to be turned + # off again. + _refuse_when_sso_only("Passwords are not used on this site. Please use SSO.") # A first password is not a change of one. Somebody who signed in with # a code or through single sign-on has none to confirm, and asking for # it locked them out of ever setting one — the only way through was @@ -498,6 +508,69 @@ def _get_sso_settings(): return {"sso_only": False} +#: How long a one-time code is worth anything. The browser is mid-redirect and +#: spends it immediately; a minute is generous for that and short enough that a +#: code seen in a log is already dead. +SSO_CODE_TTL = 60 + + +def _sso_store(): + import redis as redis_lib + from app.config import settings as cfg + return redis_lib.from_url(cfg.REDIS_URL, decode_responses=True, socket_connect_timeout=1) + + +def _stash_sso_token(token: str) -> str | None: + """Park a freshly minted token behind a random code. None if Redis is away.""" + code = secrets.token_urlsafe(32) + try: + _sso_store().setex(f"sso:exchange:{code}", SSO_CODE_TTL, token) + return code + except Exception: + return None + + +@router.post("/sso/exchange", response_model=Token) +def sso_exchange(data: SsoExchangeRequest): + """Trade a one-time code for the token it stands for. + + Once. The key is deleted as it is read, so a code replayed from a log, a + history entry or a Referer header buys nothing. + """ + from app.config import settings as cfg + + key = f"sso:exchange:{data.code}" + try: + store = _sso_store() + # GETDEL where the server has it, which is atomic and settles the race + # between two tabs; the pipeline is the same thing for an older Redis. + try: + token = store.getdel(key) + except Exception: + pipe = store.pipeline() + pipe.get(key) + pipe.delete(key) + token = pipe.execute()[0] + except Exception: + raise HTTPException(status_code=503, detail="Could not complete sign-in. Please try again.") + if not token: + raise HTTPException(status_code=400, detail="That sign-in link has expired. Please sign in again.") + return Token(access_token=token, expires_in=cfg.ACCESS_TOKEN_EXPIRE_MINUTES * 60) + + +def _refuse_when_sso_only(what: str = "Password login is disabled. Please use SSO.") -> None: + """No password door while the site is single sign-on. + + Login and the login codes checked this; register, forgot-password and + reset-password did not. So with SSO-only on, somebody could still be + issued a password account they could not use — and if the flag were ever + turned off, that is a password account nobody vetted, sitting in the + users table waiting. + """ + if _get_sso_settings()["sso_only"]: + raise HTTPException(status_code=403, detail=what) + + @router.get("/sso/config") def sso_config(): """Public endpoint — tells frontend whether SSO is available and login mode.""" @@ -616,5 +689,16 @@ async def sso_callback(request: Request, db: Session = Depends(get_db)): db.commit() db.refresh(user) - access_token = create_access_token(data={"sub": user.email}) - return RedirectResponse(url=f"{cfg.APP_URL}/sso-callback?token={access_token}") + # A code in the address bar, never the token itself. + # + # This redirect is a page load: the browser asks nginx for it, and nginx + # logs `"$request"` — so every sign-in wrote a live bearer token, good for + # a day, into the frontend container's access log, and into the browser's + # history, and into the Referer of whatever the page loaded next. The code + # that goes there instead is worth one exchange, within a minute, and is + # gone the moment it is spent. + code = _stash_sso_token(create_access_token(data={"sub": user.email})) + if code is None: + logger.error("SSO succeeded but the exchange store is unreachable") + return RedirectResponse(url=f"{cfg.APP_URL}/login?error=sso_failed") + return RedirectResponse(url=f"{cfg.APP_URL}/sso-callback?code={code}") diff --git a/backend/app/schemas/auth.py b/backend/app/schemas/auth.py index ea48d92..ce5cef9 100644 --- a/backend/app/schemas/auth.py +++ b/backend/app/schemas/auth.py @@ -95,6 +95,12 @@ class ResetPasswordRequest(BaseModel): new_password: str +class SsoExchangeRequest(BaseModel): + """The one-time code the SSO redirect leaves in the address bar.""" + + code: str = Field(min_length=8, max_length=200) + + class LoginCodeRequest(BaseModel): email: EmailStr diff --git a/backend/tests/api-contract.json b/backend/tests/api-contract.json index c5680d5..b8e28ab 100644 --- a/backend/tests/api-contract.json +++ b/backend/tests/api-contract.json @@ -2285,6 +2285,14 @@ "422" ] }, + "POST /api/v1/auth/sso/exchange": { + "body": true, + "params": [], + "responses": [ + "200", + "422" + ] + }, "POST /api/v1/categories/": { "body": true, "params": [], diff --git a/backend/tests/test_sso_hardening.py b/backend/tests/test_sso_hardening.py new file mode 100644 index 0000000..c8395f5 --- /dev/null +++ b/backend/tests/test_sso_hardening.py @@ -0,0 +1,121 @@ +"""The password doors while a site is single sign-on, and the token in the URL. + +Run: DATABASE_URL=sqlite:///:memory: PYTHONPATH=backend python -m unittest discover -s backend/tests +""" +import os +os.environ.setdefault("DATABASE_URL", "sqlite:///:memory:") + +import unittest +from unittest.mock import patch + +from fastapi import FastAPI +from fastapi.testclient import TestClient +from sqlalchemy import create_engine +from sqlalchemy.orm import Session +from sqlalchemy.pool import StaticPool + +from app.database import Base, get_db +from app.models.user import User +from app.routers import auth +from app.utils.auth import get_current_user + + +class SsoOnlyClosesThePasswordDoors(unittest.TestCase): + """Login checked the flag. Everything else that makes a password did not. + + With SSO-only on, an account could still be registered, and a reset link + still issued and spent — so the site could accumulate password accounts + nobody vetted, waiting for the flag to be turned off again. + """ + + def setUp(self): + self.engine = create_engine("sqlite://", connect_args={"check_same_thread": False}, + poolclass=StaticPool) + Base.metadata.create_all(self.engine) + self.db = Session(self.engine) + # No password of their own, so the "set a first password" path in + # PUT /auth/me is the one under test rather than bcrypt's opinion of a + # fake hash. + self.user = User(id=1, name="Reader", email="reader@example.com", + hashed_password=None) + self.db.add(self.user) + self.db.commit() + app = FastAPI() + app.include_router(auth.router, prefix="/auth") + app.dependency_overrides[get_db] = lambda: self.db + app.dependency_overrides[get_current_user] = lambda: self.user + self.client = TestClient(app) + + def tearDown(self): + self.client.close() + self.db.close() + self.engine.dispose() + + def calls(self): + return [ + ("/auth/register", {"email": "new@example.com", "password": "abcdefgh", + "name": "New"}), + ("/auth/forgot-password", {"email": "reader@example.com"}), + ("/auth/reset-password", {"token": "x" * 20, "new_password": "abcdefgh"}), + ("/auth/resend-verification", {"email": "reader@example.com"}), + ("/auth/me", {"new_password": "abcdefgh"}), + ] + + def test_every_password_door_is_shut(self): + with patch.object(auth, "_get_sso_settings", return_value={"sso_only": True}): + for path, body in self.calls(): + method = self.client.put if path == "/auth/me" else self.client.post + response = method(path, json=body) + self.assertEqual(response.status_code, 403, f"{path}: {response.text}") + self.assertIn("SSO", response.json()["detail"], path) + + def test_and_open_again_when_the_site_is_not_sso_only(self): + # Not asserting success — these have their own rules — only that the + # flag is no longer the thing refusing them. + with patch.object(auth, "_get_sso_settings", return_value={"sso_only": False}): + for path, body in self.calls(): + method = self.client.put if path == "/auth/me" else self.client.post + self.assertNotEqual(method(path, json=body).status_code, 403, path) + + +class OneTimeCodeTests(unittest.TestCase): + """The redirect carries a code, and the code is worth exactly one exchange. + + It used to carry the access token itself, in a query string that nginx + writes to its access log on every sign-in — a live bearer token, good for + a day, in a file, in the browser's history, and in the next Referer. + """ + + def setUp(self): + app = FastAPI() + app.include_router(auth.router, prefix="/auth") + self.client = TestClient(app) + self.store = {} + + def tearDown(self): + self.client.close() + + class FakeRedis: + def __init__(self, store): self.store = store + def setex(self, key, ttl, value): self.store[key] = value + def getdel(self, key): return self.store.pop(key, None) + + def test_a_code_is_spent_once(self): + with patch.object(auth, "_sso_store", return_value=self.FakeRedis(self.store)): + code = auth._stash_sso_token("a.jwt.value") + self.assertTrue(code and "a.jwt.value" not in code) + first = self.client.post("/auth/sso/exchange", json={"code": code}) + self.assertEqual(first.status_code, 200, first.text) + self.assertEqual(first.json()["access_token"], "a.jwt.value") + # Replayed from a log, a history entry or a Referer: worth nothing. + again = self.client.post("/auth/sso/exchange", json={"code": code}) + self.assertEqual(again.status_code, 400) + + def test_an_unknown_code_is_refused(self): + with patch.object(auth, "_sso_store", return_value=self.FakeRedis(self.store)): + response = self.client.post("/auth/sso/exchange", json={"code": "y" * 40}) + self.assertEqual(response.status_code, 400) + + +if __name__ == "__main__": + unittest.main() diff --git a/frontend/src/components/ArticleReader.jsx b/frontend/src/components/ArticleReader.jsx index a1c012e..e48af67 100644 --- a/frontend/src/components/ArticleReader.jsx +++ b/frontend/src/components/ArticleReader.jsx @@ -393,8 +393,10 @@ export default function ArticleReader({ the prose, and the site menu is in the header where it belongs. */} {drawerOpen && (
- + {/* No close button of its own. The header's menu button is a + cross while this is open, an inch above — two crosses for one + job, and the one people reach for is the one that is in the + same place on every screen. */}
diff --git a/frontend/src/pages/ArticlesPage.css b/frontend/src/pages/ArticlesPage.css index de4ba40..537dd5f 100644 --- a/frontend/src/pages/ArticlesPage.css +++ b/frontend/src/pages/ArticlesPage.css @@ -160,11 +160,6 @@ display: flex; align-items: center; gap: 10px; padding: 10px 12px; border-bottom: 1px solid var(--border); flex: none; } - .article-drawer-close { - flex: none; width: 34px; height: 34px; - background: none; border: 0; border-radius: 8px; - font-size: 1rem; cursor: pointer; color: var(--text-muted); - } .article-drawer-tabs { display: flex; flex: 1; border: 1px solid var(--border); border-radius: 8px; overflow: hidden; diff --git a/frontend/src/pages/QuizPage.jsx b/frontend/src/pages/QuizPage.jsx index 6265a08..c8d97ae 100644 --- a/frontend/src/pages/QuizPage.jsx +++ b/frontend/src/pages/QuizPage.jsx @@ -1602,12 +1602,9 @@ const timerStarted = timeLeft !== null
{/* The same burger that opened it closes it: on a phone this drawer is what that button does while a session is open. */} - {/* A cross, because the menu is open. The button that opened - it kept its ☰ while the drawer covered the screen, which - reads as a second menu to open rather than the way out of - the one you are looking at. */} - + {/* No close button here either: the header's menu button is a + cross while this is open, and it is the one that is in the + same place on every screen. */}
diff --git a/frontend/src/pages/QuizPlayer.css b/frontend/src/pages/QuizPlayer.css index 358bfd8..a67899e 100644 --- a/frontend/src/pages/QuizPlayer.css +++ b/frontend/src/pages/QuizPlayer.css @@ -498,11 +498,6 @@ body:has(.quiz-player.is-boxed) .site-footer { display: none; } display: flex; align-items: center; gap: 10px; padding: 10px 12px; border-bottom: 1px solid var(--border); flex: none; } -.quiz-drawer-close { - flex: none; width: 34px; height: 34px; - background: none; border: 0; border-radius: 8px; - font-size: 1rem; cursor: pointer; color: var(--text-muted); -} .quiz-drawer-tabs { display: flex; flex: 1; border: 1px solid var(--border); border-radius: 8px; overflow: hidden; } .quiz-drawer-tabs button { flex: 1; padding: 9px 6px; min-height: 40px; diff --git a/frontend/src/pages/ResultsPage.jsx b/frontend/src/pages/ResultsPage.jsx index 5283abe..9f15934 100644 --- a/frontend/src/pages/ResultsPage.jsx +++ b/frontend/src/pages/ResultsPage.jsx @@ -121,12 +121,9 @@ export default function ResultsPage() {
e.target === e.currentTarget && setNavOpen(false)}>
- {/* A cross, because the menu is open. The button that opened - it kept its ☰ while the drawer covered the screen, which - reads as a second menu to open rather than the way out of - the one you are looking at. */} - + {/* No close button here either: the header's menu button is a + cross while this is open, and it is the one that is in the + same place on every screen. */}
diff --git a/frontend/src/pages/SsoCallbackPage.jsx b/frontend/src/pages/SsoCallbackPage.jsx index 273d08b..29b52a4 100644 --- a/frontend/src/pages/SsoCallbackPage.jsx +++ b/frontend/src/pages/SsoCallbackPage.jsx @@ -1,19 +1,38 @@ -import { useEffect } from 'react' +import { useEffect, useRef } from 'react' import { useSearchParams, useNavigate } from 'react-router-dom' +import api from '../api/client' import { useAuth } from '../context/AuthContext' +/** + * Where the provider sends you back to, holding a code rather than a token. + * + * The token used to travel in this page's own query string, which meant the + * browser asked nginx for `/sso-callback?token=` — and + * nginx logs the request line. Every sign-in wrote a token good for a day into + * the frontend container's access log, the browser's history, and the Referer + * of whatever loaded next. + * + * So the redirect carries a one-time code instead, worth a single exchange + * inside a minute. It is spent here, over POST, where nothing writes it down. + */ export default function SsoCallbackPage() { const [searchParams] = useSearchParams() const navigate = useNavigate() const { loginWithToken } = useAuth() + // React runs an effect twice in development's strict mode, and the code is + // deliberately good for one exchange — the second attempt would fail and + // bounce a perfectly good sign-in to the error page. + const spent = useRef(false) useEffect(() => { - const token = searchParams.get('token') - if (token) { - loginWithToken(token).then(() => navigate('/')).catch(() => navigate('/login?error=sso_failed')) - } else { - navigate('/login?error=sso_failed') - } + if (spent.current) return + spent.current = true + const code = searchParams.get('code') + if (!code) { navigate('/login?error=sso_failed'); return } + api.post('/auth/sso/exchange', { code }) + .then(res => loginWithToken(res.data.access_token)) + .then(() => navigate('/')) + .catch(() => navigate('/login?error=sso_failed')) }, []) return (