fix(auth): security hardening — token encryption, timing, adoption + isolation

From the auth /security-review (contained fixes; architectural SA3 proxy-trust +
SA4 session-revocation deferred to the user):
- SB1: encrypt the OAuth access_token at rest (was plaintext while refresh_token
  was encrypted) — a ~1h Google bearer credential. New security.decrypt_optional()
  falls back to a refresh for legacy plaintext tokens; column is unbounded String.
  E2E-verified: token refreshed → stored as Fernet ciphertext → 319-subscription
  YouTube sync succeeded.
- SA5: password_login is no longer a timing/enumeration oracle — it always runs
  argon2 (against a decoy hash for unknown/passwordless emails), so response time
  can't reveal whether an account exists.
- SB2: Google login only ADOPTS+activates a pre-existing password account when
  Google actually attests email_verified (defense-in-depth against takeover); and
  the email sync won't overwrite with a value another account owns (avoids a 500).
- demo_login clears the wallet first (demo can't switch to / act as a real account
  via the multi-account header); switch_account rejects a suspended target (which
  would otherwise clear the whole session on the next request).

Re-review clean; ruff clean; localdev boots; YouTube auth path E2E-verified.
This commit is contained in:
npeter83 2026-07-11 21:26:39 +02:00
parent f5fac09833
commit e92751dbce
4 changed files with 51 additions and 8 deletions

View file

@ -22,6 +22,10 @@ from app.utils import valid_email
PASSWORD_MIN_LEN = 10 PASSWORD_MIN_LEN = 10
VERIFY_TTL = timedelta(hours=24) VERIFY_TTL = timedelta(hours=24)
RESET_TTL = timedelta(hours=1) RESET_TTL = timedelta(hours=1)
# A throwaway argon2 hash to verify against when the email is unknown / has no password, so a
# login attempt pays the same KDF cost either way and its timing can't reveal whether the account
# exists (anti-enumeration). Computed once at startup; it never matches a real password.
_DECOY_PASSWORD_HASH = hash_password(secrets.token_urlsafe(16))
_register_limiter = RateLimiter(max_events=5, window_seconds=300) _register_limiter = RateLimiter(max_events=5, window_seconds=300)
_login_limiter = RateLimiter(max_events=10, window_seconds=300) _login_limiter = RateLimiter(max_events=10, window_seconds=300)
_reset_limiter = RateLimiter(max_events=5, window_seconds=300) _reset_limiter = RateLimiter(max_events=5, window_seconds=300)
@ -276,6 +280,12 @@ async def callback(
# hijack it. (Near-impossible with real Google accounts: email↔sub is stable.) # hijack it. (Near-impossible with real Google accounts: email↔sub is stable.)
log.warning("Google login email collision (different sub): %s", email) log.warning("Google login email collision (different sub): %s", email)
return RedirectResponse(url="/?access=denied") return RedirectResponse(url="/?access=denied")
if not userinfo.get("email_verified", False):
# Adopting (and activating) a pre-existing password account requires Google to
# actually attest the address — otherwise an unverified Google email that merely
# collides with a pending registration could seize that account. Defense-in-depth.
log.warning("Google login: unverified email, refusing to adopt account: %s", email)
return RedirectResponse(url="/?access=denied")
user = existing user = existing
user.google_sub = sub user.google_sub = sub
# Google verified the email and is_allowed granted access, so finish activating a # Google verified the email and is_allowed granted access, so finish activating a
@ -291,6 +301,12 @@ async def callback(
log.info("Suspended account blocked at Google login: %s", email) log.info("Suspended account blocked at Google login: %s", email)
_notify_suspended(background, user.email) _notify_suspended(background, user.email)
return RedirectResponse(url="/?login=suspended") return RedirectResponse(url="/?login=suspended")
# Keep the email in sync with Google, but never overwrite it with one another account already
# owns — that would hit the unique constraint and 500, wedging this user's sign-in (rare: a
# Google-side email change to a value that collides with a different local account).
if user.email != email:
clash = db.query(User).filter(User.email == email, User.id != user.id).one_or_none()
if clash is None:
user.email = email user.email = email
user.display_name = userinfo.get("name") user.display_name = userinfo.get("name")
user.avatar_url = userinfo.get("picture") user.avatar_url = userinfo.get("picture")
@ -337,7 +353,9 @@ def _store_token(db: Session, user: User, token: dict) -> None:
tok = user.token or OAuthToken(user=user) tok = user.token or OAuthToken(user=user)
if token.get("refresh_token"): if token.get("refresh_token"):
tok.refresh_token_enc = encrypt(token["refresh_token"]) tok.refresh_token_enc = encrypt(token["refresh_token"])
tok.access_token = token.get("access_token") # Encrypt the access token at rest too (it's a live ~1h bearer credential), matching the
# refresh token — a DB/backup/log leak otherwise hands out working Google API tokens.
tok.access_token = encrypt(token.get("access_token"))
expires_at = token.get("expires_at") expires_at = token.get("expires_at")
tok.expiry = datetime.fromtimestamp(expires_at, tz=timezone.utc) if expires_at else None tok.expiry = datetime.fromtimestamp(expires_at, tz=timezone.utc) if expires_at else None
tok.scopes = token.get("scope") or BASE_SCOPES tok.scopes = token.get("scope") or BASE_SCOPES
@ -471,6 +489,9 @@ def demo_login(payload: dict, request: Request, db: Session = Depends(get_db)) -
email = (payload.get("email") or "").strip().lower() email = (payload.get("email") or "").strip().lower()
if valid_email(email) and is_demo_allowed(db, email): if valid_email(email) and is_demo_allowed(db, email):
demo = get_or_create_demo_user(db) demo = get_or_create_demo_user(db)
# Isolate the demo: drop any real-account wallet on this tab first, so the demo session
# can't switch to / act as a previously signed-in account via the multi-account header.
request.session.clear()
request.session["user_id"] = demo.id request.session["user_id"] = demo.id
log.info("Demo login (key=%s, demo_id=%s)", email, demo.id) log.info("Demo login (key=%s, demo_id=%s)", email, demo.id)
return {"authenticated": True} return {"authenticated": True}
@ -603,7 +624,11 @@ def password_login(
email = (payload.get("email") or "").strip().lower() email = (payload.get("email") or "").strip().lower()
password = payload.get("password") or "" password = payload.get("password") or ""
user = db.execute(select(User).where(User.email == email)).scalar_one_or_none() user = db.execute(select(User).where(User.email == email)).scalar_one_or_none()
if user is None or user.is_demo or not verify_password(password, user.password_hash): # Always run the KDF (against a decoy hash for an unknown / passwordless / demo account) so the
# response time is the same whether or not the email exists — no timing-based enumeration.
candidate_hash = user.password_hash if (user and not user.is_demo) else None
password_ok = verify_password(password, candidate_hash or _DECOY_PASSWORD_HASH)
if user is None or user.is_demo or not password_ok:
raise HTTPException(status_code=401, detail="Invalid email or password.") raise HTTPException(status_code=401, detail="Invalid email or password.")
if user.is_suspended: if user.is_suspended:
_notify_suspended(background, user.email) _notify_suspended(background, user.email)

View file

@ -63,6 +63,10 @@ def switch_account(
raise HTTPException(status_code=404, detail="That account no longer exists.") raise HTTPException(status_code=404, detail="That account no longer exists.")
if not is_allowed(db, u.email): if not is_allowed(db, u.email):
raise HTTPException(status_code=403, detail="That account no longer has access.") raise HTTPException(status_code=403, detail="That account no longer has access.")
if u.is_suspended:
# Don't make a suspended account the active one — the next request's current_user would
# see the suspension and clear the WHOLE wallet session, logging out every account here.
raise HTTPException(status_code=403, detail="That account is suspended.")
request.session["user_id"] = target request.session["user_id"] = target
return {"ok": True, "user_id": target} return {"ok": True, "user_id": target}

View file

@ -49,3 +49,15 @@ def decrypt(value: str | None) -> str | None:
if value is None: if value is None:
return None return None
return _require_fernet().decrypt(value.encode()).decode() return _require_fernet().decrypt(value.encode()).decode()
def decrypt_optional(value: str | None) -> str | None:
"""Like decrypt() but returns None instead of raising when the value is absent or not a valid
ciphertext (e.g. a legacy plaintext token stored before encryption) so callers fall back to
a refresh rather than erroring."""
if not value:
return None
try:
return _require_fernet().decrypt(value.encode()).decode()
except Exception:
return None

View file

@ -13,7 +13,7 @@ import httpx
from app import quota, sysconfig from app import quota, sysconfig
from app.models import User from app.models import User
from app.security import decrypt from app.security import decrypt, decrypt_optional, encrypt
log = logging.getLogger("siftlode.youtube") log = logging.getLogger("siftlode.youtube")
@ -76,8 +76,9 @@ class YouTubeClient:
if tok is None: if tok is None:
raise YouTubeError("User has no stored OAuth token") raise YouTubeError("User has no stored OAuth token")
now = datetime.now(timezone.utc) now = datetime.now(timezone.utc)
if tok.access_token and tok.expiry and tok.expiry > now + timedelta(seconds=60): cached = decrypt_optional(tok.access_token)
return tok.access_token if cached and tok.expiry and tok.expiry > now + timedelta(seconds=60):
return cached
refresh = decrypt(tok.refresh_token_enc) refresh = decrypt(tok.refresh_token_enc)
if not refresh: if not refresh:
raise YouTubeError("No refresh token; user must re-authenticate") raise YouTubeError("No refresh token; user must re-authenticate")
@ -94,12 +95,13 @@ class YouTubeClient:
if resp.status_code != 200: if resp.status_code != 200:
raise YouTubeError(f"Token refresh failed: {resp.status_code} {resp.text[:200]}") raise YouTubeError(f"Token refresh failed: {resp.status_code} {resp.text[:200]}")
data = resp.json() data = resp.json()
tok.access_token = data["access_token"] access = data["access_token"]
tok.access_token = encrypt(access)
tok.expiry = now + timedelta(seconds=int(data.get("expires_in", 3600))) tok.expiry = now + timedelta(seconds=int(data.get("expires_in", 3600)))
self.db.add(tok) self.db.add(tok)
self.db.commit() self.db.commit()
log.info("Refreshed access token for user %s", self.user.id) log.info("Refreshed access token for user %s", self.user.id)
return tok.access_token return access
# --- core request --- # --- core request ---
def _get(self, path: str, params: dict, cost: int = 1, allow_key: bool = True) -> dict: def _get(self, path: str, params: dict, cost: int = 1, allow_key: bool = True) -> dict: