fix: shut every password door under SSO-only, and keep the token out of the URL
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=<a live bearer token, good for a day> 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN
This commit is contained in:
parent
6137bf65b8
commit
c9e0655d6e
11 changed files with 273 additions and 36 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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}")
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -2285,6 +2285,14 @@
|
|||
"422"
|
||||
]
|
||||
},
|
||||
"POST /api/v1/auth/sso/exchange": {
|
||||
"body": true,
|
||||
"params": [],
|
||||
"responses": [
|
||||
"200",
|
||||
"422"
|
||||
]
|
||||
},
|
||||
"POST /api/v1/categories/": {
|
||||
"body": true,
|
||||
"params": [],
|
||||
|
|
|
|||
121
backend/tests/test_sso_hardening.py
Normal file
121
backend/tests/test_sso_hardening.py
Normal file
|
|
@ -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()
|
||||
|
|
@ -393,8 +393,10 @@ export default function ArticleReader({
|
|||
the prose, and the site menu is in the header where it belongs. */}
|
||||
{drawerOpen && (
|
||||
<div className="article-drawer-head">
|
||||
<button type="button" className="article-drawer-close" aria-label="Close"
|
||||
onClick={() => setDrawerOpen(false)}>✕</button>
|
||||
{/* 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. */}
|
||||
<div className="article-drawer-tabs" role="tablist">
|
||||
<button type="button" role="tab" aria-selected={drawerTab === 'menu'}
|
||||
onClick={() => setDrawerTab('menu')}>Main menu</button>
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -1602,12 +1602,9 @@ const timerStarted = timeLeft !== null
|
|||
<div className="quiz-drawer-head">
|
||||
{/* 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. */}
|
||||
<button type="button" className="quiz-drawer-close" aria-label="Close"
|
||||
onClick={() => setNavOpen(false)}>✕</button>
|
||||
{/* 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. */}
|
||||
<div className="quiz-drawer-tabs" role="tablist">
|
||||
<button type="button" role="tab" aria-selected={drawerTab === 'menu'}
|
||||
onClick={() => setDrawerTab('menu')}>Main menu</button>
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -121,12 +121,9 @@ export default function ResultsPage() {
|
|||
<div className="quiz-drawer" onClick={e => e.target === e.currentTarget && setNavOpen(false)}>
|
||||
<div className="quiz-drawer-panel" role="dialog" aria-modal="true" aria-label="Answer review">
|
||||
<div className="quiz-drawer-head">
|
||||
{/* 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. */}
|
||||
<button type="button" className="quiz-drawer-close" aria-label="Close"
|
||||
onClick={() => setNavOpen(false)}>✕</button>
|
||||
{/* 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. */}
|
||||
<div className="quiz-drawer-tabs" role="tablist">
|
||||
<button type="button" role="tab" aria-selected={drawerTab === 'menu'}
|
||||
onClick={() => setDrawerTab('menu')}>Main menu</button>
|
||||
|
|
|
|||
|
|
@ -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=<a live bearer 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 (
|
||||
|
|
|
|||
Loading…
Reference in a new issue