diff --git a/app/api/main.py b/app/api/main.py index bf608ba..9050c6d 100644 --- a/app/api/main.py +++ b/app/api/main.py @@ -41,16 +41,17 @@ def create_app(*, lifespan: Lifespan = scheduler_lifespan) -> FastAPI: version="0.1.0", debug=settings.app_debug, lifespan=lifespan, - docs_url="/docs", - redoc_url="/redoc", - openapi_url="/openapi.json", + # Don't expose the interactive API explorer / schema in production. + docs_url="/docs" if settings.app_env != "production" else None, + redoc_url="/redoc" if settings.app_env != "production" else None, + openapi_url="/openapi.json" if settings.app_env != "production" else None, ) app.add_middleware( SessionMiddleware, secret_key=settings.app_secret_key, same_site="lax", - https_only=settings.app_env == "production", + https_only=settings.cookie_secure, ) add_security_headers(app) diff --git a/app/api/middleware.py b/app/api/middleware.py index 2900772..f592ad7 100644 --- a/app/api/middleware.py +++ b/app/api/middleware.py @@ -2,12 +2,27 @@ import secrets from collections.abc import Awaitable, Callable +from urllib.parse import urlparse from fastapi import FastAPI, Request, Response +from fastapi.responses import JSONResponse # Swagger/ReDoc need a CDN + inline scripts; don't impose the app CSP on them. _CSP_EXEMPT_PREFIXES = ("/docs", "/redoc", "/openapi.json") +_UNSAFE_METHODS = {"POST", "PUT", "PATCH", "DELETE"} + + +def _is_cross_origin(request: Request) -> bool: + """A browser-issued state-changing request whose Origin doesn't match the host. + Defends cookie-authenticated forms against CSRF (alongside SameSite=lax). Non- + browser clients (the agent, curl) omit Origin and are unaffected; the Bearer API + carries no ambient cookie, so it isn't CSRF-able anyway.""" + origin = request.headers.get("origin") + if not origin: + return False + return urlparse(origin).netloc != request.headers.get("host", "") + def _content_security_policy(nonce: str) -> str: # Strict by default. `wasm-unsafe-eval` lets the vendored hash-wasm Argon2 module @@ -35,6 +50,8 @@ def add_security_headers(app: FastAPI) -> None: async def _security_headers( request: Request, call_next: Callable[[Request], Awaitable[Response]] ) -> Response: + if request.method in _UNSAFE_METHODS and _is_cross_origin(request): + return JSONResponse({"detail": "cross-origin request blocked"}, status_code=403) nonce = secrets.token_urlsafe(16) request.state.csp_nonce = nonce response = await call_next(request) diff --git a/app/api/routes/auth.py b/app/api/routes/auth.py index f89a81c..46e71b1 100644 --- a/app/api/routes/auth.py +++ b/app/api/routes/auth.py @@ -122,7 +122,7 @@ async def oidc_callback(request: Request, session: DBSession) -> RedirectRespons value=jwt_token, httponly=True, samesite="lax", - secure=settings.app_env == "production", + secure=settings.cookie_secure, max_age=settings.jwt_access_ttl_minutes * 60, path="/", ) diff --git a/app/api/routes/web/_shared.py b/app/api/routes/web/_shared.py index 98a2f43..f5e43e3 100644 --- a/app/api/routes/web/_shared.py +++ b/app/api/routes/web/_shared.py @@ -53,7 +53,12 @@ async def resolve_current_user(request: Request, session: AsyncSession) -> User user_id = uuid.UUID(sub) except ValueError: return None - return await UserService(session).get_by_id(user_id) + user = await UserService(session).get_by_id(user_id) + # Mirror the API check: a deactivated account must lose access immediately, + # not linger until the cookie's JWT expires. + if user is None or not user.is_active: + return None + return user def login_redirect() -> RedirectResponse: diff --git a/app/api/routes/web/auth.py b/app/api/routes/web/auth.py index 45da94c..cfbe928 100644 --- a/app/api/routes/web/auth.py +++ b/app/api/routes/web/auth.py @@ -50,7 +50,7 @@ async def login_submit( value=token, httponly=True, samesite="lax", - secure=settings.app_env == "production", + secure=settings.cookie_secure, max_age=settings.jwt_access_ttl_minutes * 60, path="/", ) diff --git a/app/core/config.py b/app/core/config.py index 06fb88f..a6f5681 100644 --- a/app/core/config.py +++ b/app/core/config.py @@ -54,6 +54,12 @@ class Settings(BaseSettings): trends_retention_days: int = 365 trends_rollup_lookback_hours: int = 6 + @property + def cookie_secure(self) -> bool: + """Mark session cookies `Secure` everywhere except local development, so a + non-production HTTPS deployment doesn't ship cookies over plaintext.""" + return self.app_env != "development" + @lru_cache def get_settings() -> Settings: diff --git a/app/core/security/passwords.py b/app/core/security/passwords.py index 419d838..1a92c2f 100644 --- a/app/core/security/passwords.py +++ b/app/core/security/passwords.py @@ -25,3 +25,17 @@ def verify_password(plain: str, hashed: str) -> bool: return _hasher.verify(hashed, plain) except VerifyMismatchError: return False + + +# A throwaway hash so a login attempt costs the same whether or not the account +# exists — removes the user-enumeration timing oracle. +_DUMMY_HASH = _hasher.hash("ghostmon-timing-equalizer") + + +def dummy_verify(plain: str) -> None: + """Run a hash verification against a constant hash (always fails) to equalise + timing when there is no real password hash to check.""" + try: + _hasher.verify(_DUMMY_HASH, plain) + except VerifyMismatchError: + pass diff --git a/app/core/services/user_service.py b/app/core/services/user_service.py index df0216f..f3d16c6 100644 --- a/app/core/services/user_service.py +++ b/app/core/services/user_service.py @@ -5,7 +5,7 @@ from app.core.models.user import AuthProvider, User from app.core.schemas.user import UserCreate -from app.core.security.passwords import hash_password, verify_password +from app.core.security.passwords import dummy_verify, hash_password, verify_password class UserService: @@ -34,11 +34,14 @@ async def create_local(self, data: UserCreate) -> User: async def authenticate_local(self, email: str, password: str) -> User | None: user = await self.get_by_email(email) - if user is None or user.auth_provider != AuthProvider.LOCAL: - return None - if not user.password_hash or not verify_password(password, user.password_hash): - return None - if not user.is_active: + if user is not None and user.auth_provider == AuthProvider.LOCAL and user.password_hash: + valid = verify_password(password, user.password_hash) + else: + # No such local account: still spend the hashing time so the response + # latency doesn't reveal whether the email exists. + dummy_verify(password) + valid = False + if user is None or not valid or not user.is_active: return None return user diff --git a/tests/test_security_hardening.py b/tests/test_security_hardening.py new file mode 100644 index 0000000..5ee7cd4 --- /dev/null +++ b/tests/test_security_hardening.py @@ -0,0 +1,67 @@ +"""Hardening from the pentest: CSRF origin-check, cookie Secure, is_active on the +web cookie surface, and constant-time login paths.""" + +from __future__ import annotations + +from typing import Any + +import httpx + +from app.core.config import Settings +from app.core.services.user_service import UserService + + +async def test_csrf_blocks_cross_origin_unsafe_request(client: httpx.AsyncClient) -> None: + resp = await client.post( + "/api/auth/login", + data={"username": "a@b.io", "password": "whatever-1234"}, + headers={"origin": "https://evil.example"}, + ) + assert resp.status_code == 403 + assert "cross-origin" in resp.text.lower() + + +async def test_csrf_allows_request_without_origin(client: httpx.AsyncClient) -> None: + # Non-browser clients (agent, curl) omit Origin and must not be blocked. + resp = await client.post( + "/api/auth/login", + data={"username": "a@b.io", "password": "whatever-1234"}, + ) + assert resp.status_code != 403 # reaches auth (401 for bad creds), not CSRF-blocked + + +def test_cookie_secure_follows_environment() -> None: + key = "x" * 16 + assert Settings(app_secret_key=key, app_env="development").cookie_secure is False + assert Settings(app_secret_key=key, app_env="production").cookie_secure is True + assert Settings(app_secret_key=key, app_env="staging").cookie_secure is True + + +async def test_deactivated_user_loses_web_access( + web_client: httpx.AsyncClient, session: Any, user: Any +) -> None: + # web_client is authenticated as `user`; deactivating must revoke web access now. + home = await web_client.get("/hosts") + assert home.status_code == 200 + + user.is_active = False + session.add(user) + await session.commit() + + after = await web_client.get("/hosts") + assert after.status_code == 303 + assert "/login" in after.headers.get("location", "") + + +async def test_authenticate_local_rejects_unknown_wrong_and_inactive( + session: Any, user: Any +) -> None: + svc = UserService(session) + # Unknown email exercises the dummy-verify (timing-equalising) path without error. + assert await svc.authenticate_local("nobody@example.com", "some-password-12") is None + assert await svc.authenticate_local(user.email, "wrong-password-12") is None + assert await svc.authenticate_local(user.email, "alice-secret-password") is not None + + user.is_active = False + await session.commit() + assert await svc.authenticate_local(user.email, "alice-secret-password") is None