peter/siftlode
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(auth): close SA5 timing oracles on register/reset + dedup messages_ws

Browse source 
Loose ends to finish the auth security round:
- register + password-reset-request had an enumeration TIMING oracle: an already-
  registered email skipped the create path (hash + row writes + email scheduling) and
  responded measurably faster than a new one. Move the whole lookup+create (register)
  and lookup+token+email (reset) into a background task with its own DB session, so the
  endpoint returns in the same time for any valid email regardless of whether it exists.
  Verified: existing vs new now ~equal (was 34ms vs 82ms on register); accounts/tokens
  still created off-path.
- messages_ws re-implemented resolved_user_id's per-tab wallet-gated account resolution.
  Generalize resolved_user_id to take any HTTPConnection (Request OR WebSocket) and call
  it from the WS — one shared, wallet-gated resolution. Behavior-identical (unit-checked).
This commit is contained in:
npeter83 2026-07-12 04:13:11 +02:00
parent 56dfa9b427
commit f7fa516332
2 changed files with 56 additions and 48 deletions
Download patch file Download diff file Expand all files Collapse all files

83
backend/app/auth.py
View file

@ -7,13 +7,14 @@ import httpx
from authlib.integrations.starlette_client import OAuth, OAuthError
from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request
from fastapi.responses import JSONResponse, RedirectResponse
from starlette.requests import HTTPConnection
from sqlalchemy import delete, func, select
from sqlalchemy.orm import Session
from app import email as email_mod
from app import sysconfig
from app.config import settings
from app.db import get_db
from app.db import SessionLocal, get_db
from app.models import AuthToken, DemoWhitelist, Invite, OAuthToken, User
from app.ratelimit import RateLimiter
from app.security import decrypt, encrypt, hash_password, hash_token, verify_password
@ -596,11 +597,24 @@ def register(
if not _register_limiter.allow(_client_ip(request)):
return {"status": "ok"} # silently throttle; uniform response
existing = db.execute(select(User).where(User.email == email)).scalar_one_or_none()
if existing is None:
# Without working SMTP we can't deliver a verification link, so email ownership can't
# gate sign-in. Admin approval stays the real gate (is_active=False); mark the account
# verified so the flow still completes on a no-SMTP self-host. See email.email_enabled().
# Do the existence check + account creation OFF the response path (a background task with its own
# session). The create path hashes the password + writes several rows + schedules emails; doing it
# inline would make a NEW email respond measurably slower than an already-registered one (which
# skips all that) — a timing oracle for enumeration. Off-path, any valid email responds the same.
background.add_task(_register_account, email, password)
return {"status": "ok"}
def _register_account(email: str, password: str) -> None:
"""Background worker for /register: create the pending account (+ verification email + admin
notice) for a genuinely new email; no-op for an already-registered one. Runs after the response
with its own DB session, so the request's timing never reveals whether the email already exists."""
with SessionLocal() as db:
if db.execute(select(User).where(User.email == email)).scalar_one_or_none() is not None:
return # already registered — nothing to do (the uniform "ok" was already returned)
# Without working SMTP we can't deliver a verification link, so email ownership can't gate
# sign-in. Admin approval stays the real gate (is_active=False); mark the account verified so
# the flow still completes on a no-SMTP self-host. See email.email_enabled().
email_ok = email_mod.email_enabled()
user = User(
email=email,
@ -610,20 +624,13 @@ def register(
)
db.add(user)
db.flush()
upsert_pending_invite(db, email) # admin-approval gate
upsert_pending_invite(db, email) # admin-approval gate (commits)
if email_ok:
raw = _issue_token(db, user, "verify", VERIFY_TTL)
# Fragment (#), not a query token: keeps the token out of proxy/access logs + Referer.
# The SPA reads the fragment and POSTs it to /auth/verify (SB3).
background.add_task(
email_mod.send_verify_email, email, f"{_app_base()}/#verify={raw}"
)
# Fragment (#), not a query token: keeps the token out of proxy/access logs + Referer (SB3).
email_mod.send_verify_email(email, f"{_app_base()}/#verify={raw}")
if settings.admin_email_set:
background.add_task(
email_mod.send_admin_new_request, sorted(settings.admin_email_set), email
)
# Existing email → do nothing visible (no enumeration). Uniform success either way.
return {"status": "ok"}
email_mod.send_admin_new_request(sorted(settings.admin_email_set), email)
@router.post("/verify")
@ -692,21 +699,30 @@ def password_reset_request(
payload: dict,
request: Request,
background: BackgroundTasks,
db: Session = Depends(get_db),
) -> dict:
"""Request a password-reset link. Uniform response regardless of whether the email has a
password account, so it can't probe for registered emails."""
"""Request a password-reset link. Uniform response + timing regardless of whether the email has a
password account (the lookup runs off the response path), so it can't probe for registered emails."""
if not _reset_limiter.allow(_client_ip(request)):
return {"status": "ok"}
email = (payload.get("email") or "").strip().lower()
# Do the account lookup + token issue + email OFF the response path (a background task with its
# own session), so the endpoint takes the same time for any valid email whether or not it has a
# password account — no timing-based enumeration. Any valid email schedules the same task.
if valid_email(email):
background.add_task(_send_reset_if_eligible, email)
return {"status": "ok"}
def _send_reset_if_eligible(email: str) -> None:
"""Background worker for password_reset_request: issue a reset token + email the link, but ONLY
for a real (non-demo) password account. Runs after the response with its own DB session, so the
request's timing never reveals whether the account exists. Token rides the URL FRAGMENT (#) so it
can't leak into proxy/access logs or a Referer (SB3)."""
with SessionLocal() as db:
user = db.execute(select(User).where(User.email == email)).scalar_one_or_none()
if user is not None and user.password_hash and not user.is_demo:
raw = _issue_token(db, user, "reset", RESET_TTL)
# Token in the URL FRAGMENT (#), not the query (?): a fragment is never sent to the
# server, so it can't leak into proxy/access logs or a Referer header (SB3).
background.add_task(email_mod.send_password_reset, email, f"{_app_base()}/#reset={raw}")
return {"status": "ok"}
email_mod.send_password_reset(email, f"{_app_base()}/#reset={raw}")
@router.post("/password-reset/confirm")
@ -741,21 +757,22 @@ def password_reset_confirm(payload: dict, db: Session = Depends(get_db)) -> dict
ACTIVE_ACCOUNT_HEADER = "x-siftlode-account"
def resolved_user_id(request: Request) -> tuple[int | None, bool]:
"""Which account this request acts as, and whether that's the session's default account.
def resolved_user_id(conn: HTTPConnection) -> tuple[int | None, bool]:
"""Which account this connection acts as, and whether that's the session's default account.
Takes any HTTPConnection — an HTTP Request OR a WebSocket — so the WS push channel shares this
exact per-tab resolution instead of re-implementing it.
The signed cookie holds the *wallet* (`account_ids`, every account signed into this browser)
plus a default `user_id`. A request may override the default with the X-Siftlode-Account
header, but ONLY for an account already in the wallet — so a tab can't impersonate an account
that never authenticated here. Everything else (WebSocket, plain navigations) keeps using the
cookie default.
plus a default `user_id`. A caller may override the default with the X-Siftlode-Account header,
but ONLY for an account already in the wallet — so a tab can't impersonate an account that never
authenticated here. Everything else (WebSocket, plain navigations) keeps using the cookie default.
"""
default_id = request.session.get("user_id")
wallet = request.session.get("account_ids") or []
default_id = conn.session.get("user_id")
wallet = conn.session.get("account_ids") or []
# Header for normal XHR; ?account= for contexts that can't set headers (WebSocket, and a
# plain <a> file download). Both are wallet-gated below, so neither can impersonate an
# account that never signed into this browser.
hdr = request.headers.get(ACTIVE_ACCOUNT_HEADER) or request.query_params.get("account")
hdr = conn.headers.get(ACTIVE_ACCOUNT_HEADER) or conn.query_params.get("account")
if hdr:
try:
hid = int(hdr)