fix(auth): rate-limit request-access + close two token races (C-3.5, C-3.11)
C-3.5: /auth/request-access had no rate limiter (unlike register/login/reset/demo) and answered "approved" for an allow-listed email — a public enumeration oracle for the allow-list, and an unthrottled invite/admin-mail firehose. It now rate-limits per client IP and returns a uniform "pending" for every outcome (allowed, pending, new, throttled); an already-allowed user can still just sign in with Google. C-3.11: _consume_token was a read-check-write, so two concurrent requests could both consume the same verify/reset token. It's now a single conditional UPDATE (...WHERE used_at IS NULL AND expires_at >= now RETURNING user_id) — only the request that flips used_at gets a user back.
This commit is contained in:
+30
-10
@@ -8,7 +8,7 @@ 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 import delete, func, select, update
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
from app import email as email_mod
|
||||
@@ -31,6 +31,7 @@ _DECOY_PASSWORD_HASH = hash_password(secrets.token_urlsafe(16))
|
||||
_register_limiter = RateLimiter(max_events=5, window_seconds=300)
|
||||
_login_limiter = RateLimiter(max_events=10, window_seconds=300)
|
||||
_reset_limiter = RateLimiter(max_events=5, window_seconds=300)
|
||||
_request_access_limiter = RateLimiter(max_events=5, window_seconds=300)
|
||||
# At most one "your account is suspended" email per address per hour, so a suspended user who
|
||||
# keeps retrying (or a script with their valid credentials) can't be used to mailbomb them.
|
||||
_suspend_email_limiter = RateLimiter(max_events=1, window_seconds=3600)
|
||||
@@ -512,16 +513,26 @@ async def logout(request: Request):
|
||||
@router.post("/request-access")
|
||||
async def request_access(
|
||||
payload: dict,
|
||||
request: Request,
|
||||
background: BackgroundTasks,
|
||||
db: Session = Depends(get_db),
|
||||
) -> dict:
|
||||
"""Public: ask for access. Idempotent — repeat requests for the same email don't
|
||||
duplicate or re-spam admins (upsert returns None for an already-pending row)."""
|
||||
duplicate or re-spam admins (upsert returns None for an already-pending row).
|
||||
|
||||
Answers a uniform ``"pending"`` for EVERY outcome — already-allowed, newly-pending,
|
||||
already-pending, and rate-limited alike. The old ``"approved"`` reply for an allow-listed
|
||||
address made this a public enumeration oracle for the allow-list (which the rest of the module
|
||||
is careful to avoid, cf. demo_login); an already-allowed user can still just sign in with Google.
|
||||
Rate-limited per client IP like register/login/reset, and a block returns "pending" too so the
|
||||
endpoint can't be probed for throttling either."""
|
||||
email = (payload.get("email") or "").strip().lower()
|
||||
if not valid_email(email):
|
||||
raise HTTPException(status_code=400, detail="Enter a valid email address.")
|
||||
if not _request_access_limiter.allow(_client_ip(request)):
|
||||
return {"status": "pending"}
|
||||
if is_allowed(db, email):
|
||||
return {"status": "approved"} # already allowed — just sign in
|
||||
return {"status": "pending"} # already allowed — don't reveal it
|
||||
inv = upsert_pending_invite(db, email)
|
||||
admins = admin_notify_emails(db)
|
||||
if inv is not None and admins:
|
||||
@@ -583,18 +594,27 @@ def _issue_token(db: Session, user: User, kind: str, ttl: timedelta) -> str:
|
||||
|
||||
|
||||
def _consume_token(db: Session, raw: str | None, kind: str) -> User | None:
|
||||
"""Validate + burn a token. Returns its user, or None if missing/expired/used/wrong kind."""
|
||||
"""Validate + burn a token atomically. Returns its user, or None if missing/expired/used/wrong
|
||||
kind. The burn is a single conditional UPDATE (…WHERE used_at IS NULL…RETURNING) rather than a
|
||||
read-check-write, so two concurrent requests can't both consume the same verify/reset token —
|
||||
only the one whose UPDATE flips used_at gets a row back."""
|
||||
if not raw:
|
||||
return None
|
||||
now = datetime.now(timezone.utc)
|
||||
row = db.execute(
|
||||
select(AuthToken).where(
|
||||
AuthToken.token_hash == hash_token(raw), AuthToken.kind == kind
|
||||
update(AuthToken)
|
||||
.where(
|
||||
AuthToken.token_hash == hash_token(raw),
|
||||
AuthToken.kind == kind,
|
||||
AuthToken.used_at.is_(None),
|
||||
AuthToken.expires_at >= now,
|
||||
)
|
||||
).scalar_one_or_none()
|
||||
if row is None or row.used_at is not None or row.expires_at < datetime.now(timezone.utc):
|
||||
return None
|
||||
row.used_at = datetime.now(timezone.utc)
|
||||
.values(used_at=now)
|
||||
.returning(AuthToken.user_id)
|
||||
).first()
|
||||
db.commit()
|
||||
if row is None:
|
||||
return None
|
||||
return db.get(User, row.user_id)
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user