From 3ccb716de69fa3691b10f27c8008551d3e884a87 Mon Sep 17 00:00:00 2001 From: Amr Gaber Date: Wed, 22 Apr 2026 00:24:16 -0500 Subject: [PATCH] feat: parameterize cookie names and harden logout revocation - Add COOKIE_PREFIX setting (default "app") so forks can namespace auth cookies per service. Access and refresh cookie names, plus the Cookie param aliases in the refresh and logout handlers, now derive from the setting. - Extract decode_refresh_token helper in app/auth/refresh.py with a narrow jwt.PyJWTError catch. Share it between validate_and_rotate_refresh_token and the logout handler, eliminating duplicate decode logic. - Replace the broad except Exception: in logout that was swallowing DB errors alongside decode failures. Decode failures still log and return 204; DB errors now propagate so real bugs surface. - Fix a hardcoded key="app_access" in the refresh handler; reads cookie_transport.cookie_name so both cookie paths share one source of truth. - Extend production config validation to reject weak defaults for cookie_prefix ({"", "app", "api-template"}) and otel_service_name ({"", "api-template"}). - Tests: logout revokes single and multi-token families, handles garbage/missing cookies gracefully, and a subprocess-reload check proves COOKIE_PREFIX wires through end-to-end with a non-default value. --- .env.example | 6 ++ README.md | 11 ++- app/auth/backend.py | 2 +- app/auth/refresh.py | 29 ++++++-- app/config.py | 14 ++++ app/routers/auth_refresh.py | 33 ++++----- tests/test_auth.py | 143 ++++++++++++++++++++++++++++++++++++ 7 files changed, 207 insertions(+), 31 deletions(-) create mode 100644 tests/test_auth.py diff --git a/.env.example b/.env.example index 38dbd72..ebfb1a1 100644 --- a/.env.example +++ b/.env.example @@ -16,6 +16,12 @@ FRONTEND_URL=http://localhost:5173 # Cookie domain (leave empty for localhost development) COOKIE_DOMAIN= +# Cookie name prefix — applied to every auth cookie (e.g. "myapp" → "myapp_access", +# "myapp_refresh"). In production, set this to something service-scoped (usually your +# service name) so cookies don't collide with other services on the same domain. +# Required in production: defaults of "", "app", or "api-template" are rejected. +COOKIE_PREFIX=app + # Logging LOG_LEVEL=INFO diff --git a/README.md b/README.md index 821e234..960fabd 100644 --- a/README.md +++ b/README.md @@ -162,11 +162,13 @@ All note endpoints require authentication. Users can only access their own notes Authentication uses httpOnly cookies with short-lived access tokens and rotating refresh tokens. -- **Access token**: 15-minute JWT stored in an `app_access` httpOnly cookie -- **Refresh token**: 7-day JWT stored in an `app_refresh` httpOnly cookie (scoped to `/auth/refresh`) +- **Access token**: 15-minute JWT stored in a `{COOKIE_PREFIX}_access` httpOnly cookie +- **Refresh token**: 7-day JWT stored in a `{COOKIE_PREFIX}_refresh` httpOnly cookie (scoped to `/auth/refresh`) - **Token rotation**: Each refresh issues a new token in the same family; reuse of an old token revokes the entire family (theft detection) - **Rate limiting**: Login (5/min), registration (3/min), refresh (30/min) +> **Before first deploy**: set `COOKIE_PREFIX` to a service-scoped value (typically your service name, e.g. `myservice`). Browser cookies on the same domain are identified by name, so two services sharing a `.example.com` with the default prefix will overwrite each other's auth cookies. Production startup will refuse to boot with the template defaults `""`, `"app"`, or `"api-template"`. + ### Role-Based Access Control Users have a `role` field (default: `user`). Roles are defined as a `StrEnum` in `app/auth/roles.py`: @@ -190,7 +192,7 @@ async def admin_only(user: User = Depends(require_role("admin"))): - **CORS lockdown**: Explicit origins, methods, and headers (no wildcards in production) - **Security headers**: HSTS, X-Frame-Options DENY, X-Content-Type-Options nosniff, Referrer-Policy, Permissions-Policy - **Rate limiting**: Per-endpoint limits on auth routes with security event logging -- **Production config validation**: Rejects weak secrets and default database credentials at startup +- **Production config validation**: Rejects weak secrets, default database credentials, unset cookie prefix, and default OTel service name at startup - **Security event logging**: Structured logs for login, logout, registration, token refresh, and rate limit events ## Logging, Telemetry & Feature Flags @@ -369,9 +371,10 @@ api-template/ | `CORS_ORIGINS` | Required | Comma-separated allowed origins | (empty — dev uses localhost:5100-5199) | | `FRONTEND_URL` | Optional | Frontend URL for redirects | `http://localhost:5173` | | `COOKIE_DOMAIN` | Optional | Cookie domain (leave empty for localhost) | (empty) | +| `COOKIE_PREFIX` | Required | Prefix for auth cookie names (service-scoped) | `app` | | `LOG_LEVEL` | Optional | Logging level | `INFO` | | `OTEL_ENABLED` | Optional | Enable OpenTelemetry tracing | `false` | -| `OTEL_SERVICE_NAME` | Optional | Service name for traces | `api-template` | +| `OTEL_SERVICE_NAME` | Required | Service name for traces | `api-template` | | `OTEL_EXPORTER_ENDPOINT` | Optional | OTLP gRPC collector endpoint | `http://localhost:4317` | | `FEATURE_*` | Optional | Feature flags (e.g. `FEATURE_NEW_DASHBOARD=true`) | (none) | diff --git a/app/auth/backend.py b/app/auth/backend.py index 91d5e12..bb9ad21 100644 --- a/app/auth/backend.py +++ b/app/auth/backend.py @@ -5,7 +5,7 @@ ACCESS_TOKEN_LIFETIME = 900 # 15 minutes cookie_transport = CookieTransport( - cookie_name="app_access", + cookie_name=f"{settings.cookie_prefix}_access", cookie_max_age=ACCESS_TOKEN_LIFETIME, cookie_path="/", cookie_domain=settings.cookie_domain, diff --git a/app/auth/refresh.py b/app/auth/refresh.py index 6c20be1..b900d4f 100644 --- a/app/auth/refresh.py +++ b/app/auth/refresh.py @@ -1,6 +1,7 @@ import uuid from datetime import UTC, datetime, timedelta +import jwt from fastapi import Response from fastapi_users.jwt import decode_jwt, generate_jwt from sqlalchemy import select, update @@ -10,10 +11,27 @@ from app.models.refresh_token import RefreshToken REFRESH_TOKEN_LIFETIME = timedelta(days=7) -REFRESH_COOKIE_NAME = "app_refresh" +REFRESH_COOKIE_NAME = f"{settings.cookie_prefix}_refresh" REFRESH_AUDIENCE = ["app:refresh"] +def decode_refresh_token(token: str) -> dict | None: + """Decode a refresh-token JWT, returning None on any decode failure. + + Separates decode errors (invalid/expired/malformed → None) from downstream + DB errors (which should propagate as 500s, not be swallowed alongside decode + failures by a caller's broad except). + """ + try: + return decode_jwt( + token, + secret=settings.secret_key, + audience=REFRESH_AUDIENCE, + ) + except jwt.PyJWTError: + return None + + async def create_refresh_token( user_id: str, session: AsyncSession, @@ -50,13 +68,8 @@ async def validate_and_rotate_refresh_token( token_jwt: str, session: AsyncSession, ) -> tuple[str, str] | None: - try: - payload = decode_jwt( - token_jwt, - secret=settings.secret_key, - audience=REFRESH_AUDIENCE, - ) - except Exception: + payload = decode_refresh_token(token_jwt) + if payload is None: return None jti = payload.get("jti") diff --git a/app/config.py b/app/config.py index 9488291..f3e347e 100644 --- a/app/config.py +++ b/app/config.py @@ -28,6 +28,10 @@ class Settings(BaseSettings): # Cookie auth cookie_domain: str | None = None + # Prefix applied to every auth cookie (e.g. "app" → "app_access", "app_refresh"). + # Must be overridden in production to avoid collisions with other services on the + # same domain — see validate_production_settings below. + cookie_prefix: str = "app" # Logging log_level: str = "INFO" @@ -61,6 +65,16 @@ def validate_production_settings(self) -> "Settings": ) if "postgres:postgres@" in self.database_url: raise ValueError("Default database credentials must not be used in production") + weak_cookie_prefixes = {"", "app", "api-template"} + if self.cookie_prefix in weak_cookie_prefixes: + raise ValueError( + "COOKIE_PREFIX must be set to a service-scoped value in production " + "(e.g. your service name) to avoid cookie collisions across services " + "on the same domain" + ) + weak_otel_names = {"", "api-template"} + if self.otel_service_name in weak_otel_names: + raise ValueError("OTEL_SERVICE_NAME must be set to your service name in production") return self diff --git a/app/routers/auth_refresh.py b/app/routers/auth_refresh.py index 5b240ff..6338e8b 100644 --- a/app/routers/auth_refresh.py +++ b/app/routers/auth_refresh.py @@ -2,20 +2,19 @@ from fastapi import APIRouter, Cookie, Depends, Response from fastapi.responses import JSONResponse -from fastapi_users.jwt import decode_jwt from sqlalchemy import update from sqlalchemy.ext.asyncio import AsyncSession from app.auth.backend import cookie_transport, get_jwt_strategy from app.auth.refresh import ( - REFRESH_AUDIENCE, + REFRESH_COOKIE_NAME, clear_refresh_cookie, + decode_refresh_token, set_refresh_cookie, validate_and_rotate_refresh_token, ) from app.auth.security_logging import SecurityEvent, log_security_event from app.auth.users import get_user_db -from app.config import settings from app.database import get_async_session from app.models.refresh_token import RefreshToken @@ -24,13 +23,13 @@ @router.post("/refresh", status_code=204) async def refresh_access_token( - app_refresh: str | None = Cookie(None), + refresh_token: str | None = Cookie(None, alias=REFRESH_COOKIE_NAME), session: AsyncSession = Depends(get_async_session), ): - if not app_refresh: + if not refresh_token: return JSONResponse(status_code=401, content={"detail": "Missing refresh token"}) - result = await validate_and_rotate_refresh_token(app_refresh, session) + result = await validate_and_rotate_refresh_token(refresh_token, session) if result is None: log_security_event( @@ -58,7 +57,7 @@ async def refresh_access_token( response = Response(status_code=204) # Set access cookie response.set_cookie( - key="app_access", + key=cookie_transport.cookie_name, value=access_token, max_age=cookie_transport.cookie_max_age, path=cookie_transport.cookie_path, @@ -81,17 +80,17 @@ async def refresh_access_token( @router.post("/jwt/logout", status_code=204) async def logout( - app_refresh: str | None = Cookie(None), + refresh_token: str | None = Cookie(None, alias=REFRESH_COOKIE_NAME), session: AsyncSession = Depends(get_async_session), ): - # Revoke the refresh token family if a refresh cookie is present - if app_refresh: - try: - payload = decode_jwt( - app_refresh, - secret=settings.secret_key, - audience=REFRESH_AUDIENCE, - ) + # Revoke the refresh-token family if a valid refresh cookie is present. + # Decode failures → log and proceed (user-facing logout still succeeds). + # DB errors → propagate as 500, not silently swallowed as "decode failed". + if refresh_token: + payload = decode_refresh_token(refresh_token) + if payload is None: + log_security_event(SecurityEvent.LOGOUT, detail="refresh token decode failed") + else: family = payload.get("family") user_id = payload.get("sub") if family: @@ -106,8 +105,6 @@ async def logout( user_id=user_id, detail=f"revoked token family={family}", ) - except Exception: - log_security_event(SecurityEvent.LOGOUT, detail="refresh token decode failed") else: log_security_event(SecurityEvent.LOGOUT, detail="no refresh token cookie") diff --git a/tests/test_auth.py b/tests/test_auth.py new file mode 100644 index 0000000..3fbec13 --- /dev/null +++ b/tests/test_auth.py @@ -0,0 +1,143 @@ +import importlib +import os +from datetime import UTC, datetime, timedelta +from uuid import UUID, uuid4 + +from fastapi_users.jwt import generate_jwt +from httpx import AsyncClient +from sqlalchemy import select + +from app.auth.refresh import REFRESH_AUDIENCE, REFRESH_COOKIE_NAME, REFRESH_TOKEN_LIFETIME +from app.config import settings +from app.models.refresh_token import RefreshToken +from app.models.user import User + + +def _make_refresh_jwt(user_id: UUID, jti: UUID, family: str) -> str: + return generate_jwt( + { + "sub": str(user_id), + "jti": str(jti), + "family": family, + "aud": REFRESH_AUDIENCE, + }, + secret=settings.secret_key, + lifetime_seconds=int(REFRESH_TOKEN_LIFETIME.total_seconds()), + ) + + +class TestLogoutRevocation: + """Reproducer for the silent-failure bug: logout must revoke the token family.""" + + async def test_logout_revokes_token_family(self, client: AsyncClient, test_user: User, session): + session.add(test_user) + await session.commit() + + family = "family-under-test" + jti = uuid4() + db_token = RefreshToken( + id=jti, + user_id=test_user.id, + token_family=family, + is_revoked=False, + expires_at=datetime.now(UTC) + timedelta(days=7), + ) + session.add(db_token) + await session.commit() + + response = await client.post( + "/auth/jwt/logout", + cookies={REFRESH_COOKIE_NAME: _make_refresh_jwt(test_user.id, jti, family)}, + ) + assert response.status_code == 204 + + await session.refresh(db_token) + assert db_token.is_revoked is True + + async def test_logout_revokes_all_tokens_in_family( + self, client: AsyncClient, test_user: User, session + ): + """Multiple rotated tokens in one family: logout revokes every row.""" + session.add(test_user) + await session.commit() + + family = "shared-family" + tokens = [ + RefreshToken( + id=uuid4(), + user_id=test_user.id, + token_family=family, + is_revoked=False, + expires_at=datetime.now(UTC) + timedelta(days=7), + ) + for _ in range(3) + ] + for t in tokens: + session.add(t) + await session.commit() + + response = await client.post( + "/auth/jwt/logout", + cookies={REFRESH_COOKIE_NAME: _make_refresh_jwt(test_user.id, tokens[-1].id, family)}, + ) + assert response.status_code == 204 + + # Endpoint ran the UPDATE in its own session; expire this session's + # identity map so the re-query reflects the committed changes. + session.expire_all() + result = await session.execute( + select(RefreshToken).where(RefreshToken.token_family == family) + ) + rows = result.scalars().all() + assert len(rows) == 3 + assert all(row.is_revoked for row in rows) + + async def test_logout_with_invalid_token_returns_204(self, client: AsyncClient): + """Malformed cookie: logout still succeeds (user-facing) and does not crash.""" + response = await client.post( + "/auth/jwt/logout", + cookies={REFRESH_COOKIE_NAME: "not-a-real-jwt"}, + ) + assert response.status_code == 204 + + async def test_logout_without_cookie_returns_204(self, client: AsyncClient): + response = await client.post("/auth/jwt/logout") + assert response.status_code == 204 + + +class TestCookiePrefixWiring: + """COOKIE_PREFIX must flow through to every cookie surface.""" + + def test_cookie_names_compose_from_settings_prefix(self): + from app.auth.backend import cookie_transport + from app.auth.refresh import REFRESH_COOKIE_NAME as current_refresh_name + + assert cookie_transport.cookie_name == f"{settings.cookie_prefix}_access" + assert current_refresh_name == f"{settings.cookie_prefix}_refresh" + + def test_cookie_prefix_env_override_wires_through(self): + """Changing COOKIE_PREFIX changes the computed cookie names end-to-end. + + This is the test that distinguishes "reads from the setting" from + "happens to produce the right string because the default is 'app'". + """ + import app.auth.backend as backend_mod + import app.auth.refresh as refresh_mod + import app.config as config_mod + + original = os.environ.get("COOKIE_PREFIX") + os.environ["COOKIE_PREFIX"] = "verify_prefix" + try: + importlib.reload(config_mod) + importlib.reload(backend_mod) + importlib.reload(refresh_mod) + assert backend_mod.cookie_transport.cookie_name == "verify_prefix_access" + assert refresh_mod.REFRESH_COOKIE_NAME == "verify_prefix_refresh" + finally: + if original is None: + os.environ.pop("COOKIE_PREFIX", None) + else: + os.environ["COOKIE_PREFIX"] = original + importlib.reload(config_mod) + importlib.reload(backend_mod) + importlib.reload(refresh_mod)