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 && (