Fix CSRF validation for duplicate cookie values on PATCH
This commit is contained in:
@@ -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()
|
||||
|
||||
|
||||
@@ -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."""
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user