From 1a04b23e891da1f7050dfb0caa52d813f9dc107f Mon Sep 17 00:00:00 2001 From: Beda Schmid Date: Mon, 2 Mar 2026 18:09:27 -0300 Subject: [PATCH] Fix CSRF validation for duplicate cookie values on PATCH --- backend/app/api/auth.py | 29 ++++++++++++- backend/tests/test_security_controls.py | 58 +++++++++++++++++++++++++ doc/operations-and-configuration.md | 1 + 3 files changed, 86 insertions(+), 2 deletions(-) diff --git a/backend/app/api/auth.py b/backend/app/api/auth.py index d4addbd..102ac52 100644 --- a/backend/app/api/auth.py +++ b/backend/app/api/auth.py @@ -54,6 +54,28 @@ def _requires_csrf_validation(method: str) -> bool: return method.upper() in CSRF_PROTECTED_METHODS +def _extract_cookie_values(request: Request, cookie_name: str) -> tuple[str, ...]: + """Extracts all values for one cookie name from raw Cookie header order.""" + + request_headers = getattr(request, "headers", None) + raw_cookie_header = request_headers.get("cookie", "") if request_headers is not None else "" + if not raw_cookie_header: + return () + + extracted_values: list[str] = [] + for cookie_pair in raw_cookie_header.split(";"): + normalized_pair = cookie_pair.strip() + if not normalized_pair or "=" not in normalized_pair: + continue + key, value = normalized_pair.split("=", 1) + if key.strip() != cookie_name: + continue + normalized_value = value.strip() + if normalized_value: + extracted_values.append(normalized_value) + return tuple(extracted_values) + + def _raise_unauthorized() -> None: """Raises a 401 challenge response for missing or invalid auth sessions.""" @@ -94,11 +116,14 @@ def get_request_auth_context( if _requires_csrf_validation(request.method) and using_cookie_session: normalized_csrf_header = (csrf_header or "").strip() + csrf_candidates = [candidate for candidate in _extract_cookie_values(request, CSRF_COOKIE_NAME) if candidate] normalized_csrf_cookie = (csrf_cookie or "").strip() + if normalized_csrf_cookie and normalized_csrf_cookie not in csrf_candidates: + csrf_candidates.append(normalized_csrf_cookie) if ( - not normalized_csrf_cookie + not csrf_candidates or not normalized_csrf_header - or not hmac.compare_digest(normalized_csrf_cookie, normalized_csrf_header) + or not any(hmac.compare_digest(candidate, normalized_csrf_header) for candidate in csrf_candidates) ): _raise_csrf_rejected() diff --git a/backend/tests/test_security_controls.py b/backend/tests/test_security_controls.py index 183f9fe..86c2161 100644 --- a/backend/tests/test_security_controls.py +++ b/backend/tests/test_security_controls.py @@ -364,6 +364,7 @@ if "app.services.routing_pipeline" not in sys.modules: from fastapi import HTTPException from app.api.auth import AuthContext, require_admin +from app.api import auth as auth_dependency_module from app.api import routes_auth as auth_routes_module from app.api import routes_documents as documents_routes_module from app.core import config as config_module @@ -420,6 +421,63 @@ class AuthDependencyTests(unittest.TestCase): resolved = require_admin(context=auth_context) self.assertEqual(resolved.role, UserRole.ADMIN) + def test_csrf_validation_accepts_matching_token_among_duplicate_cookie_values(self) -> None: + """PATCH CSRF validation accepts header token matching any duplicate csrf cookie value.""" + + request = SimpleNamespace( + method="PATCH", + headers={"cookie": "dcm_session=session-token; dcm_csrf=stale-token; dcm_csrf=fresh-token"}, + ) + resolved_session = SimpleNamespace( + id=uuid.uuid4(), + expires_at=datetime.now(UTC), + user=SimpleNamespace( + id=uuid.uuid4(), + username="admin", + role=UserRole.ADMIN, + ), + ) + with patch.object(auth_dependency_module, "resolve_auth_session", return_value=resolved_session): + context = auth_dependency_module.get_request_auth_context( + request=request, + credentials=None, + csrf_header="fresh-token", + csrf_cookie="stale-token", + session_cookie="session-token", + session=SimpleNamespace(), + ) + self.assertEqual(context.username, "admin") + self.assertEqual(context.role, UserRole.ADMIN) + + def test_csrf_validation_rejects_when_header_does_not_match_any_cookie_value(self) -> None: + """PATCH CSRF validation rejects requests when header token matches no csrf cookie values.""" + + request = SimpleNamespace( + method="PATCH", + headers={"cookie": "dcm_session=session-token; dcm_csrf=stale-token; dcm_csrf=fresh-token"}, + ) + resolved_session = SimpleNamespace( + id=uuid.uuid4(), + expires_at=datetime.now(UTC), + user=SimpleNamespace( + id=uuid.uuid4(), + username="admin", + role=UserRole.ADMIN, + ), + ) + with patch.object(auth_dependency_module, "resolve_auth_session", return_value=resolved_session): + with self.assertRaises(HTTPException) as raised: + auth_dependency_module.get_request_auth_context( + request=request, + credentials=None, + csrf_header="unknown-token", + csrf_cookie="stale-token", + session_cookie="session-token", + session=SimpleNamespace(), + ) + self.assertEqual(raised.exception.status_code, 403) + self.assertEqual(raised.exception.detail, "Invalid CSRF token") + class DocumentCatalogVisibilityTests(unittest.TestCase): """Verifies predefined tag and path discovery visibility by caller role.""" diff --git a/doc/operations-and-configuration.md b/doc/operations-and-configuration.md index e439a6a..8577758 100644 --- a/doc/operations-and-configuration.md +++ b/doc/operations-and-configuration.md @@ -137,6 +137,7 @@ Recommended LIVE pattern: - `VITE_ALLOWED_HOSTS` only affects development mode where Vite is running. - API auth cookies support optional domain and SameSite configuration through `AUTH_COOKIE_DOMAIN` and `AUTH_COOKIE_SAMESITE`. - HTTPS cookie security detection falls back to `PUBLIC_BASE_URL` scheme when proxy headers are missing. +- CSRF validation accepts header matches against any `dcm_csrf` cookie value in the request, covering stale plus fresh duplicate-cookie transitions. - Session authentication is cookie-based; browser reloads and new tabs can reuse an active session until it expires or is revoked. - Protected media and file download flows still use authenticated fetch plus blob/object URL handling.