Fix auth session persistence with HttpOnly cookies and CSRF
This commit is contained in:
@@ -5,7 +5,8 @@ from datetime import datetime
|
||||
from typing import Annotated
|
||||
from uuid import UUID
|
||||
|
||||
from fastapi import Depends, HTTPException, status
|
||||
import hmac
|
||||
from fastapi import Depends, HTTPException, Request, status
|
||||
from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
@@ -14,7 +15,26 @@ from app.models.auth import UserRole
|
||||
from app.services.authentication import resolve_auth_session
|
||||
|
||||
|
||||
try:
|
||||
from fastapi import Cookie, Header
|
||||
except (ImportError, AttributeError):
|
||||
|
||||
def Cookie(_default=None, **_kwargs): # type: ignore[no-untyped-def]
|
||||
"""Compatibility fallback for environments that stub fastapi without request params."""
|
||||
|
||||
return None
|
||||
|
||||
def Header(_default=None, **_kwargs): # type: ignore[no-untyped-def]
|
||||
"""Compatibility fallback for environments that stub fastapi without request params."""
|
||||
|
||||
return None
|
||||
|
||||
|
||||
bearer_auth = HTTPBearer(auto_error=False)
|
||||
SESSION_COOKIE_NAME = "dcm_session"
|
||||
CSRF_COOKIE_NAME = "dcm_csrf"
|
||||
CSRF_HEADER_NAME = "x-csrf-token"
|
||||
CSRF_PROTECTED_METHODS = frozenset({"POST", "PATCH", "PUT", "DELETE"})
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
@@ -28,8 +48,14 @@ class AuthContext:
|
||||
expires_at: datetime
|
||||
|
||||
|
||||
def _requires_csrf_validation(method: str) -> bool:
|
||||
"""Returns whether an HTTP method should be protected by cookie CSRF validation."""
|
||||
|
||||
return method.upper() in CSRF_PROTECTED_METHODS
|
||||
|
||||
|
||||
def _raise_unauthorized() -> None:
|
||||
"""Raises a 401 challenge response for missing or invalid bearer sessions."""
|
||||
"""Raises a 401 challenge response for missing or invalid auth sessions."""
|
||||
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_401_UNAUTHORIZED,
|
||||
@@ -38,19 +64,44 @@ def _raise_unauthorized() -> None:
|
||||
)
|
||||
|
||||
|
||||
def _raise_csrf_rejected() -> None:
|
||||
"""Raises a forbidden response for CSRF validation failure."""
|
||||
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_403_FORBIDDEN,
|
||||
detail="Invalid CSRF token",
|
||||
)
|
||||
|
||||
|
||||
def get_request_auth_context(
|
||||
request: Request,
|
||||
credentials: Annotated[HTTPAuthorizationCredentials | None, Depends(bearer_auth)],
|
||||
csrf_header: Annotated[str | None, Header(default=None, alias=CSRF_HEADER_NAME)],
|
||||
csrf_cookie: Annotated[str | None, Cookie(default=None, alias=CSRF_COOKIE_NAME)],
|
||||
session_cookie: Annotated[str | None, Cookie(default=None, alias=SESSION_COOKIE_NAME)],
|
||||
session: Annotated[Session, Depends(get_session)],
|
||||
) -> AuthContext:
|
||||
"""Authenticates bearer session token and returns role-bound request identity context."""
|
||||
"""Authenticates auth session token and validates CSRF for cookie sessions."""
|
||||
|
||||
if credentials is None:
|
||||
_raise_unauthorized()
|
||||
token = credentials.credentials.strip() if credentials is not None and credentials.credentials else ""
|
||||
using_cookie_session = False
|
||||
|
||||
token = credentials.credentials.strip()
|
||||
if not token:
|
||||
token = (session_cookie or "").strip()
|
||||
using_cookie_session = True
|
||||
if not token:
|
||||
_raise_unauthorized()
|
||||
|
||||
if _requires_csrf_validation(request.method) and using_cookie_session:
|
||||
normalized_csrf_header = (csrf_header or "").strip()
|
||||
normalized_csrf_cookie = (csrf_cookie or "").strip()
|
||||
if (
|
||||
not normalized_csrf_cookie
|
||||
or not normalized_csrf_header
|
||||
or not hmac.compare_digest(normalized_csrf_cookie, normalized_csrf_header)
|
||||
):
|
||||
_raise_csrf_rejected()
|
||||
|
||||
resolved_session = resolve_auth_session(session, token=token)
|
||||
if resolved_session is None or resolved_session.user is None:
|
||||
_raise_unauthorized()
|
||||
|
||||
@@ -1,11 +1,18 @@
|
||||
"""Authentication endpoints for credential login, session introspection, and logout."""
|
||||
|
||||
import logging
|
||||
import secrets
|
||||
from datetime import UTC, datetime
|
||||
|
||||
from fastapi import APIRouter, Depends, HTTPException, Request, status
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
from app.api.auth import AuthContext, require_user_or_admin
|
||||
from app.api.auth import (
|
||||
AuthContext,
|
||||
SESSION_COOKIE_NAME,
|
||||
CSRF_COOKIE_NAME,
|
||||
require_user_or_admin,
|
||||
)
|
||||
from app.db.base import get_session
|
||||
from app.schemas.auth import (
|
||||
AuthLoginRequest,
|
||||
@@ -21,6 +28,11 @@ from app.services.auth_login_throttle import (
|
||||
)
|
||||
from app.services.authentication import authenticate_user, issue_user_session, revoke_auth_session
|
||||
|
||||
try:
|
||||
from fastapi import Response
|
||||
except (ImportError, AttributeError):
|
||||
from fastapi.responses import Response
|
||||
|
||||
|
||||
router = APIRouter(prefix="/auth", tags=["auth"])
|
||||
logger = logging.getLogger(__name__)
|
||||
@@ -48,13 +60,82 @@ def _retry_after_headers(retry_after_seconds: int) -> dict[str, str]:
|
||||
return {"Retry-After": str(max(1, int(retry_after_seconds)))}
|
||||
|
||||
|
||||
def _is_https_request(request: Request) -> bool:
|
||||
"""Returns whether the incoming request should be treated as HTTPS for cookie flags."""
|
||||
|
||||
forwarded_protocol = request.headers.get("x-forwarded-proto", "").strip().lower().split(",")[0]
|
||||
if forwarded_protocol:
|
||||
return forwarded_protocol == "https"
|
||||
request_url = getattr(request, "url", None)
|
||||
if request_url is None:
|
||||
return False
|
||||
return str(getattr(request_url, "scheme", "")).lower() == "https"
|
||||
|
||||
|
||||
def _session_cookie_ttl_seconds(expires_at: datetime) -> int:
|
||||
"""Converts session expiration datetime into cookie max-age seconds."""
|
||||
|
||||
now = datetime.now(UTC)
|
||||
ttl = int((expires_at - now).total_seconds())
|
||||
return max(1, ttl)
|
||||
|
||||
|
||||
def _set_session_cookie(response: Response, session_token: str, *, expires_at: datetime, secure: bool) -> None:
|
||||
"""Stores the issued session token in a browser HttpOnly auth cookie."""
|
||||
|
||||
if response is None or not hasattr(response, "set_cookie"):
|
||||
return
|
||||
expires_seconds = _session_cookie_ttl_seconds(expires_at)
|
||||
response.set_cookie(
|
||||
SESSION_COOKIE_NAME,
|
||||
value=session_token,
|
||||
max_age=expires_seconds,
|
||||
httponly=True,
|
||||
secure=secure,
|
||||
samesite="strict",
|
||||
path="/",
|
||||
)
|
||||
|
||||
|
||||
def _set_csrf_cookie(
|
||||
response: Response,
|
||||
csrf_token: str,
|
||||
*,
|
||||
expires_at: datetime,
|
||||
secure: bool,
|
||||
) -> None:
|
||||
"""Stores an anti-CSRF token in a browser cookie for JavaScript-safe extraction."""
|
||||
|
||||
if response is None or not hasattr(response, "set_cookie"):
|
||||
return
|
||||
response.set_cookie(
|
||||
CSRF_COOKIE_NAME,
|
||||
value=csrf_token,
|
||||
max_age=_session_cookie_ttl_seconds(expires_at),
|
||||
httponly=False,
|
||||
secure=secure,
|
||||
samesite="strict",
|
||||
path="/",
|
||||
)
|
||||
|
||||
|
||||
def _clear_session_cookies(response: Response) -> None:
|
||||
"""Clears auth cookies returned by login responses."""
|
||||
|
||||
if response is None or not hasattr(response, "delete_cookie"):
|
||||
return
|
||||
response.delete_cookie(SESSION_COOKIE_NAME, path="/")
|
||||
response.delete_cookie(CSRF_COOKIE_NAME, path="/")
|
||||
|
||||
|
||||
@router.post("/login", response_model=AuthLoginResponse)
|
||||
def login(
|
||||
payload: AuthLoginRequest,
|
||||
request: Request,
|
||||
response: Response | None = None,
|
||||
session: Session = Depends(get_session),
|
||||
) -> AuthLoginResponse:
|
||||
"""Authenticates credentials with throttle protection and returns an issued bearer session token."""
|
||||
"""Authenticates credentials with throttle protection and returns issued session metadata."""
|
||||
|
||||
ip_address = _request_ip_address(request)
|
||||
try:
|
||||
@@ -120,10 +201,27 @@ def login(
|
||||
ip_address=ip_address,
|
||||
)
|
||||
session.commit()
|
||||
return AuthLoginResponse(
|
||||
access_token=issued_session.token,
|
||||
|
||||
csrf_token = secrets.token_urlsafe(32)
|
||||
secure_cookie = _is_https_request(request)
|
||||
_set_session_cookie(
|
||||
response,
|
||||
issued_session.token,
|
||||
expires_at=issued_session.expires_at,
|
||||
secure=secure_cookie,
|
||||
)
|
||||
_set_csrf_cookie(
|
||||
response,
|
||||
csrf_token,
|
||||
expires_at=issued_session.expires_at,
|
||||
secure=secure_cookie,
|
||||
)
|
||||
|
||||
return AuthLoginResponse(
|
||||
user=AuthUserResponse.model_validate(user),
|
||||
expires_at=issued_session.expires_at,
|
||||
access_token=issued_session.token,
|
||||
csrf_token=csrf_token,
|
||||
)
|
||||
|
||||
|
||||
@@ -143,10 +241,11 @@ def me(context: AuthContext = Depends(require_user_or_admin)) -> AuthSessionResp
|
||||
|
||||
@router.post("/logout", response_model=AuthLogoutResponse)
|
||||
def logout(
|
||||
response: Response | None = None,
|
||||
context: AuthContext = Depends(require_user_or_admin),
|
||||
session: Session = Depends(get_session),
|
||||
) -> AuthLogoutResponse:
|
||||
"""Revokes current bearer session token and confirms logout state."""
|
||||
"""Revokes current session token and clears client auth cookies."""
|
||||
|
||||
revoked = revoke_auth_session(
|
||||
session,
|
||||
@@ -154,4 +253,6 @@ def logout(
|
||||
)
|
||||
if revoked:
|
||||
session.commit()
|
||||
|
||||
_clear_session_cookies(response)
|
||||
return AuthLogoutResponse(revoked=revoked)
|
||||
|
||||
Reference in New Issue
Block a user