🐐 Several security fixes
This commit is contained in:
+31
-10
@@ -2,16 +2,17 @@ import logging
|
||||
import re
|
||||
import secrets
|
||||
import uuid
|
||||
from datetime import datetime, timedelta
|
||||
from datetime import datetime, timedelta, timezone
|
||||
|
||||
from fastapi import APIRouter, Depends, HTTPException, Request
|
||||
from fastapi.security import OAuth2PasswordRequestForm
|
||||
from pydantic import BaseModel
|
||||
from sqlalchemy import func
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
from core.auth import (
|
||||
create_access_token, create_refresh_token,
|
||||
decode_refresh_token, hash_password, verify_password,
|
||||
DUMMY_PASSWORD_HASH, create_access_token, create_refresh_token,
|
||||
decode_refresh_token_payload, hash_password, token_issued_before, verify_password,
|
||||
)
|
||||
from core.database import get_db
|
||||
from core.dependencies import get_current_user, limiter
|
||||
@@ -58,7 +59,9 @@ def validate_register(username: str, email: str, password: str) -> str | None:
|
||||
return "Username must be at least 2 characters"
|
||||
if len(username) > 16:
|
||||
return "Username must be 16 characters or fewer"
|
||||
if not re.match(r"^[^\s@]+@[^\s@]+\.[^\s@]+$", email):
|
||||
# Permissive by design — unicode names are fine. Safety comes from escaping at the
|
||||
# point of use: html.escape in email_utils, and exact comparison instead of ILIKE.
|
||||
if len(email) > 254 or not re.match(r"^[^\s@]+@[^\s@]+\.[A-Za-z]{2,}$", email):
|
||||
return "Please enter a valid email"
|
||||
domain = email.split("@")[-1].lower()
|
||||
if domain in _disposable_blocklist:
|
||||
@@ -76,7 +79,9 @@ def register(request: Request, req: RegisterRequest, db: Session = Depends(get_d
|
||||
err = validate_register(req.username, req.email, req.password)
|
||||
if err:
|
||||
raise HTTPException(status_code=400, detail=err)
|
||||
if db.query(UserModel).filter(UserModel.username.ilike(req.username)).first():
|
||||
# Equality, not ilike: as a LIKE *pattern* an unescaped '%' matches every row and
|
||||
# 'a_c' matches 'abc', so registration could be made to reject valid names.
|
||||
if db.query(UserModel).filter(func.lower(UserModel.username) == req.username.lower()).first():
|
||||
raise HTTPException(status_code=400, detail="Username already taken")
|
||||
if db.query(UserModel).filter(UserModel.email == req.email).first():
|
||||
raise HTTPException(status_code=400, detail="Email already registered")
|
||||
@@ -106,8 +111,12 @@ def register(request: Request, req: RegisterRequest, db: Session = Depends(get_d
|
||||
@router.post("/login")
|
||||
@limiter.limit("10/minute")
|
||||
def login(request: Request, form: OAuth2PasswordRequestForm = Depends(), db: Session = Depends(get_db)):
|
||||
user = db.query(UserModel).filter(UserModel.username.ilike(form.username)).first()
|
||||
if not user or not verify_password(form.password, user.password_hash):
|
||||
user = db.query(UserModel).filter(func.lower(UserModel.username) == form.username.lower()).first()
|
||||
# Verify unconditionally: short-circuiting on a missing user would return before
|
||||
# bcrypt runs, and that timing gap enumerates usernames just as well as an explicit
|
||||
# "no such user" would — which the other handlers here deliberately avoid.
|
||||
password_ok = verify_password(form.password, user.password_hash if user else DUMMY_PASSWORD_HASH)
|
||||
if not user or not password_ok:
|
||||
raise HTTPException(status_code=400, detail="Invalid username or password")
|
||||
user.last_active_at = datetime.now()
|
||||
db.commit()
|
||||
@@ -130,6 +139,7 @@ def reset_password(request: Request, req: ResetPasswordRequest, user: UserModel
|
||||
if req.current_password == req.new_password:
|
||||
raise HTTPException(status_code=400, detail="New password must be different from current password")
|
||||
user.password_hash = hash_password(req.new_password)
|
||||
user.token_valid_after = datetime.now(timezone.utc)
|
||||
db.commit()
|
||||
return {"message": "Password updated"}
|
||||
|
||||
@@ -166,6 +176,9 @@ def reset_password_with_token(request: Request, req: ResetPasswordWithTokenReque
|
||||
if len(req.new_password) > 256:
|
||||
raise HTTPException(status_code=400, detail="Password must be 256 characters or fewer")
|
||||
user.password_hash = hash_password(req.new_password)
|
||||
# This is the flow a locked-out or compromised user reaches for, so it must evict
|
||||
# whoever else is holding a token for this account.
|
||||
user.token_valid_after = datetime.now(timezone.utc)
|
||||
user.reset_token = None
|
||||
user.reset_token_expires_at = None
|
||||
db.commit()
|
||||
@@ -209,12 +222,20 @@ def resend_verification(request: Request, req: ResendVerificationRequest, db: Se
|
||||
@router.post("/auth/refresh")
|
||||
@limiter.limit("20/minute")
|
||||
def refresh(request: Request, req: RefreshRequest, db: Session = Depends(get_db)):
|
||||
user_id = decode_refresh_token(req.refresh_token)
|
||||
if not user_id:
|
||||
payload = decode_refresh_token_payload(req.refresh_token)
|
||||
if not payload or not payload.get("sub"):
|
||||
raise HTTPException(status_code=401, detail="Invalid or expired refresh token")
|
||||
user = db.query(UserModel).filter(UserModel.id == uuid.UUID(user_id)).first()
|
||||
try:
|
||||
user_uuid = uuid.UUID(payload["sub"])
|
||||
except ValueError:
|
||||
raise HTTPException(status_code=401, detail="Invalid or expired refresh token")
|
||||
user = db.query(UserModel).filter(UserModel.id == user_uuid).first()
|
||||
if not user:
|
||||
raise HTTPException(status_code=401, detail="User not found")
|
||||
# Refresh tokens live 30 days, so this is what actually stops a stolen one from
|
||||
# being replayed into a fresh token after the owner resets their password.
|
||||
if token_issued_before(payload, user.token_valid_after):
|
||||
raise HTTPException(status_code=401, detail="Invalid or expired refresh token")
|
||||
user.last_active_at = datetime.now()
|
||||
db.commit()
|
||||
return {
|
||||
|
||||
Reference in New Issue
Block a user