Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions app/api/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
17 changes: 17 additions & 0 deletions app/api/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion app/api/routes/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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="/",
)
Expand Down
7 changes: 6 additions & 1 deletion app/api/routes/web/_shared.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion app/api/routes/web/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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="/",
)
Expand Down
6 changes: 6 additions & 0 deletions app/core/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
14 changes: 14 additions & 0 deletions app/core/security/passwords.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
15 changes: 9 additions & 6 deletions app/core/services/user_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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

Expand Down
67 changes: 67 additions & 0 deletions tests/test_security_hardening.py
Original file line number Diff line number Diff line change
@@ -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": "[email protected]", "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": "[email protected]", "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("[email protected]", "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
Loading