Merge chore/code-hygiene: Phase 2 #8 Auth security fixes (contained batch)
Last-admin lockout guard; access_token encryption at rest (SB1, E2E-verified); password-login timing/enumeration; Google-adoption email_verified check; demo + switch-account isolation. Architectural SA3 (proxy trust) + SA4 (session revocation) deferred to the user. Verified: ruff, re-review clean, localdev boots, YouTube sync works with encrypted token.
This commit is contained in:
commit
41a915f23c
5 changed files with 58 additions and 10 deletions
|
|
@ -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)
|
||||||
|
|
|
||||||
|
|
@ -166,7 +166,12 @@ def set_user_role(
|
||||||
raise HTTPException(status_code=400, detail="The demo account's role can't be changed.")
|
raise HTTPException(status_code=400, detail="The demo account's role can't be changed.")
|
||||||
if target.id == admin.id:
|
if target.id == admin.id:
|
||||||
raise HTTPException(status_code=400, detail="You can't change your own role.")
|
raise HTTPException(status_code=400, detail="You can't change your own role.")
|
||||||
if target.role == "admin" and role == "user" and count_admins(db) <= 1:
|
if (
|
||||||
|
target.role == "admin"
|
||||||
|
and role == "user"
|
||||||
|
and not target.is_suspended
|
||||||
|
and count_admins(db, active_only=True) <= 1
|
||||||
|
):
|
||||||
raise HTTPException(status_code=400, detail="Can't remove the last admin.")
|
raise HTTPException(status_code=400, detail="Can't remove the last admin.")
|
||||||
changed = target.role != role
|
changed = target.role != role
|
||||||
target.role = role
|
target.role = role
|
||||||
|
|
@ -233,7 +238,7 @@ def admin_delete_user(
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=400, detail="Delete your own account from Settings → Account."
|
status_code=400, detail="Delete your own account from Settings → Account."
|
||||||
)
|
)
|
||||||
if target.role == "admin" and count_admins(db) <= 1:
|
if target.role == "admin" and not target.is_suspended and count_admins(db, active_only=True) <= 1:
|
||||||
raise HTTPException(status_code=400, detail="Can't delete the last admin.")
|
raise HTTPException(status_code=400, detail="Can't delete the last admin.")
|
||||||
purge_user(db, target, background)
|
purge_user(db, target, background)
|
||||||
return {"deleted": user_id}
|
return {"deleted": user_id}
|
||||||
|
|
|
||||||
|
|
@ -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}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -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
|
||||||
|
|
|
||||||
|
|
@ -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:
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue