Merge feature/auth-loose-ends: close SA5 timing oracles + dedup messages_ws
register + password-reset-request move their lookup/create/token work off the response path (background task, own session) so response timing no longer reveals whether an email is registered. messages_ws shares resolved_user_id (now HTTPConnection-typed) instead of re-implementing the per-tab wallet-gated resolution. Behavior-preserving; reviewed (no security regression) + verified.
This commit is contained in:
commit
3f58ad0d84
2 changed files with 56 additions and 48 deletions
|
|
@ -7,13 +7,14 @@ import httpx
|
||||||
from authlib.integrations.starlette_client import OAuth, OAuthError
|
from authlib.integrations.starlette_client import OAuth, OAuthError
|
||||||
from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request
|
from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request
|
||||||
from fastapi.responses import JSONResponse, RedirectResponse
|
from fastapi.responses import JSONResponse, RedirectResponse
|
||||||
|
from starlette.requests import HTTPConnection
|
||||||
from sqlalchemy import delete, func, select
|
from sqlalchemy import delete, func, select
|
||||||
from sqlalchemy.orm import Session
|
from sqlalchemy.orm import Session
|
||||||
|
|
||||||
from app import email as email_mod
|
from app import email as email_mod
|
||||||
from app import sysconfig
|
from app import sysconfig
|
||||||
from app.config import settings
|
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.models import AuthToken, DemoWhitelist, Invite, OAuthToken, User
|
||||||
from app.ratelimit import RateLimiter
|
from app.ratelimit import RateLimiter
|
||||||
from app.security import decrypt, encrypt, hash_password, hash_token, verify_password
|
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)):
|
if not _register_limiter.allow(_client_ip(request)):
|
||||||
return {"status": "ok"} # silently throttle; uniform response
|
return {"status": "ok"} # silently throttle; uniform response
|
||||||
|
|
||||||
existing = db.execute(select(User).where(User.email == email)).scalar_one_or_none()
|
# Do the existence check + account creation OFF the response path (a background task with its own
|
||||||
if existing is None:
|
# session). The create path hashes the password + writes several rows + schedules emails; doing it
|
||||||
# Without working SMTP we can't deliver a verification link, so email ownership can't
|
# inline would make a NEW email respond measurably slower than an already-registered one (which
|
||||||
# gate sign-in. Admin approval stays the real gate (is_active=False); mark the account
|
# skips all that) — a timing oracle for enumeration. Off-path, any valid email responds the same.
|
||||||
# verified so the flow still completes on a no-SMTP self-host. See email.email_enabled().
|
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()
|
email_ok = email_mod.email_enabled()
|
||||||
user = User(
|
user = User(
|
||||||
email=email,
|
email=email,
|
||||||
|
|
@ -610,20 +624,13 @@ def register(
|
||||||
)
|
)
|
||||||
db.add(user)
|
db.add(user)
|
||||||
db.flush()
|
db.flush()
|
||||||
upsert_pending_invite(db, email) # admin-approval gate
|
upsert_pending_invite(db, email) # admin-approval gate (commits)
|
||||||
if email_ok:
|
if email_ok:
|
||||||
raw = _issue_token(db, user, "verify", VERIFY_TTL)
|
raw = _issue_token(db, user, "verify", VERIFY_TTL)
|
||||||
# Fragment (#), not a query token: keeps the token out of proxy/access logs + Referer.
|
# Fragment (#), not a query token: keeps the token out of proxy/access logs + Referer (SB3).
|
||||||
# The SPA reads the fragment and POSTs it to /auth/verify (SB3).
|
email_mod.send_verify_email(email, f"{_app_base()}/#verify={raw}")
|
||||||
background.add_task(
|
|
||||||
email_mod.send_verify_email, email, f"{_app_base()}/#verify={raw}"
|
|
||||||
)
|
|
||||||
if settings.admin_email_set:
|
if settings.admin_email_set:
|
||||||
background.add_task(
|
email_mod.send_admin_new_request(sorted(settings.admin_email_set), email)
|
||||||
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"}
|
|
||||||
|
|
||||||
|
|
||||||
@router.post("/verify")
|
@router.post("/verify")
|
||||||
|
|
@ -692,21 +699,30 @@ def password_reset_request(
|
||||||
payload: dict,
|
payload: dict,
|
||||||
request: Request,
|
request: Request,
|
||||||
background: BackgroundTasks,
|
background: BackgroundTasks,
|
||||||
db: Session = Depends(get_db),
|
|
||||||
) -> dict:
|
) -> dict:
|
||||||
"""Request a password-reset link. Uniform response regardless of whether the email has a
|
"""Request a password-reset link. Uniform response + timing regardless of whether the email has a
|
||||||
password account, so it can't probe for registered emails."""
|
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)):
|
if not _reset_limiter.allow(_client_ip(request)):
|
||||||
return {"status": "ok"}
|
return {"status": "ok"}
|
||||||
email = (payload.get("email") or "").strip().lower()
|
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):
|
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()
|
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:
|
if user is not None and user.password_hash and not user.is_demo:
|
||||||
raw = _issue_token(db, user, "reset", RESET_TTL)
|
raw = _issue_token(db, user, "reset", RESET_TTL)
|
||||||
# Token in the URL FRAGMENT (#), not the query (?): a fragment is never sent to the
|
email_mod.send_password_reset(email, f"{_app_base()}/#reset={raw}")
|
||||||
# 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"}
|
|
||||||
|
|
||||||
|
|
||||||
@router.post("/password-reset/confirm")
|
@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"
|
ACTIVE_ACCOUNT_HEADER = "x-siftlode-account"
|
||||||
|
|
||||||
|
|
||||||
def resolved_user_id(request: Request) -> tuple[int | None, bool]:
|
def resolved_user_id(conn: HTTPConnection) -> tuple[int | None, bool]:
|
||||||
"""Which account this request acts as, and whether that's the session's default account.
|
"""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)
|
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
|
plus a default `user_id`. A caller may override the default with the X-Siftlode-Account header,
|
||||||
header, but ONLY for an account already in the wallet — so a tab can't impersonate an account
|
but ONLY for an account already in the wallet — so a tab can't impersonate an account that never
|
||||||
that never authenticated here. Everything else (WebSocket, plain navigations) keeps using the
|
authenticated here. Everything else (WebSocket, plain navigations) keeps using the cookie default.
|
||||||
cookie default.
|
|
||||||
"""
|
"""
|
||||||
default_id = request.session.get("user_id")
|
default_id = conn.session.get("user_id")
|
||||||
wallet = request.session.get("account_ids") or []
|
wallet = conn.session.get("account_ids") or []
|
||||||
# Header for normal XHR; ?account= for contexts that can't set headers (WebSocket, and a
|
# 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
|
# plain <a> file download). Both are wallet-gated below, so neither can impersonate an
|
||||||
# account that never signed into this browser.
|
# 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:
|
if hdr:
|
||||||
try:
|
try:
|
||||||
hid = int(hdr)
|
hid = int(hdr)
|
||||||
|
|
|
||||||
|
|
@ -20,7 +20,7 @@ from pydantic import BaseModel
|
||||||
from sqlalchemy import and_, func, or_, select
|
from sqlalchemy import and_, func, or_, select
|
||||||
from sqlalchemy.orm import Session
|
from sqlalchemy.orm import Session
|
||||||
|
|
||||||
from app.auth import require_human
|
from app.auth import require_human, resolved_user_id
|
||||||
from app.db import SessionLocal, get_db
|
from app.db import SessionLocal, get_db
|
||||||
from app.models import Message, MessageKey, User
|
from app.models import Message, MessageKey, User
|
||||||
from app.ratelimit import RateLimiter
|
from app.ratelimit import RateLimiter
|
||||||
|
|
@ -363,19 +363,10 @@ async def messages_ws(ws: WebSocket) -> None:
|
||||||
"""Live push channel: authenticated by the session cookie, registers the connection so
|
"""Live push channel: authenticated by the session cookie, registers the connection so
|
||||||
sent messages reach this user's open tabs instantly. We don't consume client→server frames
|
sent messages reach this user's open tabs instantly. We don't consume client→server frames
|
||||||
(sending goes through POST /api/messages); the receive loop only detects disconnect."""
|
(sending goes through POST /api/messages); the receive loop only detects disconnect."""
|
||||||
# A browser WebSocket can't send custom headers, so the per-tab account (see
|
# Which signed-in account this socket acts as — the SHARED per-tab resolution (wallet-gated
|
||||||
# X-Siftlode-Account on HTTP) rides in the ?account= query param instead — honoured only for
|
# ?account= override, session default otherwise). A browser WebSocket can't send the
|
||||||
# an account already in this browser's wallet. Falls back to the session default.
|
# X-Siftlode-Account header, so resolved_user_id falls through to the ?account= query param.
|
||||||
sess = ws.session if "session" in ws.scope else {}
|
uid, _ = resolved_user_id(ws)
|
||||||
uid = sess.get("user_id")
|
|
||||||
q = ws.query_params.get("account")
|
|
||||||
if q:
|
|
||||||
try:
|
|
||||||
qid = int(q)
|
|
||||||
except ValueError:
|
|
||||||
qid = None
|
|
||||||
if qid is not None and qid in (sess.get("account_ids") or []):
|
|
||||||
uid = qid
|
|
||||||
if not uid:
|
if not uid:
|
||||||
await ws.close(code=1008)
|
await ws.close(code=1008)
|
||||||
return
|
return
|
||||||
|
|
@ -385,7 +376,7 @@ async def messages_ws(ws: WebSocket) -> None:
|
||||||
# SA4: honour server-side session revocation on the live channel too — reject a cookie whose
|
# SA4: honour server-side session revocation on the live channel too — reject a cookie whose
|
||||||
# recorded epoch is behind the account's current one (mirrors current_user). Without this a
|
# recorded epoch is behind the account's current one (mirrors current_user). Without this a
|
||||||
# copied cookie could keep receiving pushes after a password reset / "log out everywhere".
|
# copied cookie could keep receiving pushes after a password reset / "log out everywhere".
|
||||||
epochs = sess.get("epochs") or {}
|
epochs = ws.session.get("epochs") or {}
|
||||||
epoch_ok = user is not None and int(epochs.get(str(uid), 0)) == (user.session_epoch or 0)
|
epoch_ok = user is not None and int(epochs.get(str(uid), 0)) == (user.session_epoch or 0)
|
||||||
ok = epoch_ok and is_messageable_user(user)
|
ok = epoch_ok and is_messageable_user(user)
|
||||||
finally:
|
finally:
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue