diff --git a/backend/api/data.py b/backend/api/data.py index dccd85890..83b9fa6d5 100644 --- a/backend/api/data.py +++ b/backend/api/data.py @@ -2489,10 +2489,16 @@ async def _get_workspace_document( auth_context: AuthContext, document_id: str, ) -> Document: + organization_filter = ( + Document.organization_id == auth_context.organization_id + if auth_context.organization_id is not None + else Document.organization_id.is_(None) + ) result = await db.execute( select(Document).where( Document.document_id == document_id, Document.workspace_id == auth_context.workspace_id, + organization_filter, ) ) document = result.scalar_one_or_none() @@ -3928,10 +3934,18 @@ async def get_data_quality_surface( ProjectFolder.folder_uid.asc(), ), ) + document_organization_filter = ( + Document.organization_id == auth_context.organization_id + if auth_context.organization_id is not None + else Document.organization_id.is_(None) + ) documents = await _scoped_rows( db, select(Document) - .where(Document.workspace_id == auth_context.workspace_id) + .where( + Document.workspace_id == auth_context.workspace_id, + document_organization_filter, + ) .order_by(Document.created_at.desc(), Document.document_id.asc()) .limit(8), ) diff --git a/backend/api/dav.py b/backend/api/dav.py index d618c25f3..3cede071c 100644 --- a/backend/api/dav.py +++ b/backend/api/dav.py @@ -1,6 +1,5 @@ import logging from html import escape as escape_xml_text -from urllib.parse import unquote from fastapi import APIRouter, Depends, HTTPException, Request, Response from sqlalchemy.ext.asyncio import AsyncSession @@ -13,15 +12,68 @@ router = APIRouter(prefix="/dav", tags=["dav"]) +IMPLEMENTED_DAV_METHODS = ("OPTIONS", "PROPFIND") +_DAV_AUTHORIZATION_PATH_MAX_CHARACTERS = 8192 +_HEX_DIGITS = frozenset("0123456789abcdefABCDEF") +_DAV_STRUCTURAL_OCTETS = frozenset( + {*range(0x20), 0x2E, 0x2F, 0x5C, 0x7F} +) + + +def _residual_percent_octet(path: str, percent_index: int) -> int | None: + """Return the octet exposed by another decode, following nested ``%25``.""" + + cursor = percent_index + 1 + while cursor + 1 < len(path): + pair = path[cursor : cursor + 2] + if pair[0] not in _HEX_DIGITS or pair[1] not in _HEX_DIGITS: + return None + octet = int(pair, 16) + if octet != 0x25: + return octet + cursor += 2 + return None + + +def _has_ambiguous_percent_encoding(path: str) -> bool: + """Detect residual encodings that another decode would make structural.""" + + for index, character in enumerate(path): + if character != "%": + continue + octet = _residual_percent_octet(path, index) + if octet in _DAV_STRUCTURAL_OCTETS: + return True + return False + def _normalize_dav_authorization_path(path: str) -> str: + """Validate the framework-decoded DAV path without decoding it again. + + ASGI routing has already decoded the request-target path once. Authorization + therefore treats residual percent text as data unless another decode would + introduce a traversal dot, separator, backslash, or control octet. Literal + backslashes are normalized to separators for owner/traversal checks. + """ + + if len(path) > _DAV_AUTHORIZATION_PATH_MAX_CHARACTERS: + raise HTTPException( + status_code=414, + detail="DAV path exceeds authorization length limit", + ) + if any(ord(character) < 0x20 or ord(character) == 0x7F for character in path): + raise HTTPException( + status_code=400, + detail="DAV path contains control characters", + ) + normalized_path = path.replace("\\", "/") - for _ in range(100): - decoded_path = unquote(normalized_path).replace("\\", "/") - if decoded_path == normalized_path: - return normalized_path - normalized_path = decoded_path - raise HTTPException(status_code=400, detail="DAV path decoding limit exceeded") + if _has_ambiguous_percent_encoding(normalized_path): + raise HTTPException( + status_code=400, + detail="DAV path contains ambiguous percent encoding", + ) + return normalized_path def _dav_path_owner_user_id(path: str) -> str | None: @@ -158,73 +210,38 @@ async def _handle_project_propfind( @router.api_route( "/{path:path}", - methods=["PROPFIND", "REPORT", "MKCOL", "GET", "PUT", "DELETE", "OPTIONS"], + methods=list(IMPLEMENTED_DAV_METHODS), ) async def dav_handler( request: Request, path: str, auth_context: AuthContext = Depends(get_auth_context), db: AsyncSession = Depends(get_db), -): - """ - Route the authenticated DAV surface that is implemented for this slice. +) -> Response: + """Serve only the authenticated DAV capabilities implemented in production. - Collection discovery is served from the server-side project registry. - Provider-backed writeback stays fail-closed until source capability and - ETag/If-Match enforcement are available through signed writeback intents. + Project collection discovery is available through ``PROPFIND``. Unsupported + writeback and richer DAV verbs are deliberately not registered, so clients + receive ``405 Method Not Allowed`` instead of a misleading advertised + capability that can only return ``501 Not Implemented``. """ - _ensure_dav_owner_scope(path, auth_context) - safe_path = repr(path)[1:-1] + normalized_path = _normalize_dav_authorization_path(path) + _ensure_dav_owner_scope(normalized_path, auth_context) + safe_path = repr(normalized_path)[1:-1] logger.info("DAV Request: %s /%s", request.method, safe_path) if request.method == "OPTIONS": - headers = { - "DAV": "1, 2, 3, calendar-access, addressbook", - "Allow": ( - "OPTIONS, GET, HEAD, POST, PUT, DELETE, TRACE, COPY, MOVE, MKCOL, " - "PROPFIND, PROPPATCH, LOCK, UNLOCK, REPORT" - ), - } - return Response(status_code=200, headers=headers) - - if request.method == "PROPFIND": - return await _handle_project_propfind( - request=request, - path=path, - auth_context=auth_context, - db=db, - ) - - if request.method == "PUT": - body = await request.body() - safe_path = repr(path)[1:-1] - logger.info("DAV PUT received %s bytes at /%s", len(body), safe_path) - logger.warning( - "DAV PUT rejected at /%s: provider-backed DAV writeback is not " - "implemented; signed writeback-intent API is required", - safe_path, - ) return Response( - content=( - "Provider-backed DAV writeback is not implemented; use signed " - "writeback-intent APIs until source, capability, and " - "ETag/If-Match checks are enforced." - ), - media_type="text/plain", - status_code=501, + status_code=200, + headers={ + "DAV": "1", + "Allow": ", ".join(IMPLEMENTED_DAV_METHODS), + }, ) - logger.warning( - "DAV %s rejected at /%s: method is not implemented for the " - "provider-backed DAV gateway", - request.method, - safe_path, - ) - return Response( - content=( - "Provider-backed DAV method is not implemented; use supported " - "PROPFIND/OPTIONS discovery or signed writeback-intent APIs." - ), - media_type="text/plain", - status_code=501, + return await _handle_project_propfind( + request=request, + path=normalized_path, + auth_context=auth_context, + db=db, ) diff --git a/backend/tests/test_data_document_authorization.py b/backend/tests/test_data_document_authorization.py new file mode 100644 index 000000000..dd66bc04a --- /dev/null +++ b/backend/tests/test_data_document_authorization.py @@ -0,0 +1,361 @@ +"""Authorization regressions for workspace document actions.""" + +from __future__ import annotations + +from datetime import datetime, timezone +from typing import Any + +import pytest +from fastapi import HTTPException + +import api.data as data_api +from api.auth import AuthContext +from db.models import Document + + +class _ScalarResult: + """Minimal SQLAlchemy result surface used by the authorization helper.""" + + def __init__(self, value: Document | None) -> None: + self._value = value + + def scalar_one_or_none(self) -> Document | None: + """Return the single simulated document result.""" + + return self._value + + +class _OrganizationAwareSession: + """Evaluate document scope predicates against one in-memory document.""" + + def __init__(self, document: Document) -> None: + self.document = document + + async def execute(self, statement: Any) -> _ScalarResult: + """Return the document only when every emitted scope predicate matches.""" + + compiled = statement.compile() + rendered_scope = str(statement.whereclause) + values = tuple(compiled.params.values()) + if self.document.document_id not in values: + return _ScalarResult(None) + if self.document.workspace_id not in values: + return _ScalarResult(None) + + # A missing organization predicate reproduces the vulnerable behavior: + # a document from another tenant is still returned solely because both + # principals supplied the same workspace identifier. + if "workspace_documents.organization_id" not in rendered_scope: + return _ScalarResult(self.document) + + if self.document.organization_id is None: + organization_matches = "IS NULL" in rendered_scope.upper() + else: + organization_matches = self.document.organization_id in values + return _ScalarResult(self.document if organization_matches else None) + + +def _auth_context(organization_id: str | None) -> AuthContext: + return AuthContext( + user_id="member-a", + role="member", + organization_id=organization_id, + group_ids=(), + workspace_id="workspace-shared-identifier", + ) + + +def _document( + *, + document_id: str, + organization_id: str | None, + document_name: str, +) -> Document: + """Build a workspace document with a stable timestamp for surface tests.""" + + return Document( + document_id=document_id, + workspace_id="workspace-shared-identifier", + organization_id=organization_id, + document_name=document_name, + document_type="text/markdown", + document_content="tenant evidence", + document_status="uploaded", + created_at=datetime(2026, 1, 1, tzinfo=timezone.utc), + ) + + +def _document_visible_in_scope( + document: Document, + values: tuple[Any, ...], + rendered_scope: str, +) -> bool: + """Return whether a compiled document scope includes the fake document.""" + + if document.workspace_id not in values: + return False + if "workspace_documents.organization_id" not in rendered_scope: + return True + if document.organization_id is None: + return "IS NULL" in rendered_scope.upper() + return document.organization_id in values + + +def _install_quality_surface_fixtures( + monkeypatch: pytest.MonkeyPatch, + documents: list[Document], +) -> None: + """Patch the expensive quality-surface dependencies with deterministic fakes.""" + + async def scoped_rows(_db: object, statement: Any) -> list[object]: + statement_text = str(statement) + if "workspace_documents" not in statement_text: + return [] + compiled = statement.compile() + rendered_scope = str(statement.whereclause) + values = tuple(compiled.params.values()) + return [ + document + for document in documents + if _document_visible_in_scope(document, values, rendered_scope) + ] + + async def zero_email_stats( + _db: object, + _email_scope: object, + ) -> data_api.EmailQualityStats: + return data_api.EmailQualityStats( + count=0, + missing_thread_count=0, + missing_fingerprint_count=0, + embedded_count=0, + ) + + async def zero_attachment_stats( + _db: object, + _email_scope: object, + ) -> data_api.AttachmentQualityStats: + return data_api.AttachmentQualityStats( + count=0, + blank_content_count=0, + embedded_count=0, + ) + + async def zero_content_graph_stats( + _db: object, + _email_scope: object, + ) -> data_api.ContentGraphQualityStats: + return data_api.ContentGraphQualityStats( + segmented_email_count=0, + segment_count=0, + ) + + async def zero_knowledge_graph_stats( + _db: object, + _email_scope: object, + ) -> data_api.KnowledgeGraphQualityStats: + return data_api.KnowledgeGraphQualityStats( + edged_email_count=0, + edge_count=0, + ) + + async def zero_content_segment_readiness_stats( + _db: object, + _email_scope: object, + ) -> data_api.ContentSegmentTextReadinessStats: + return data_api.ContentSegmentTextReadinessStats( + total_count=0, + issue_count=0, + ) + + async def zero_knowledge_graph_endpoint_stats( + _db: object, + _email_scope: object, + ) -> data_api.KnowledgeGraphEvidenceEndpointStats: + return data_api.KnowledgeGraphEvidenceEndpointStats( + total_count=0, + issue_count=0, + ) + + async def zero_semantic_relation_stats( + _db: object, + _auth_context: AuthContext, + ) -> data_api.SemanticRelationEvidenceStats: + return data_api.SemanticRelationEvidenceStats( + total_count=0, + source_backed_count=0, + ) + + async def zero_attachment_parse_stats( + _db: object, + _email_scope: object, + ) -> data_api.AttachmentParseQualityStats: + return data_api.AttachmentParseQualityStats( + parsed_count=0, + unparsed_count=0, + ) + + async def empty_list(*_args: object, **_kwargs: object) -> list[object]: + return [] + + monkeypatch.setattr(data_api, "_scoped_rows", scoped_rows) + monkeypatch.setattr(data_api, "_get_email_stats", zero_email_stats) + monkeypatch.setattr(data_api, "_get_attachment_stats", zero_attachment_stats) + monkeypatch.setattr(data_api, "_get_content_graph_stats", zero_content_graph_stats) + monkeypatch.setattr( + data_api, + "_get_knowledge_graph_stats", + zero_knowledge_graph_stats, + ) + monkeypatch.setattr( + data_api, + "_get_content_segment_text_readiness_stats", + zero_content_segment_readiness_stats, + ) + monkeypatch.setattr( + data_api, + "_get_knowledge_graph_evidence_endpoint_stats", + zero_knowledge_graph_endpoint_stats, + ) + monkeypatch.setattr(data_api, "_get_content_graph_breakdown", empty_list) + monkeypatch.setattr(data_api, "_get_knowledge_graph_breakdown", empty_list) + monkeypatch.setattr(data_api, "_get_content_graph_evidence_samples", empty_list) + monkeypatch.setattr(data_api, "_get_knowledge_graph_evidence_samples", empty_list) + monkeypatch.setattr( + data_api, + "_get_semantic_relation_evidence_stats", + zero_semantic_relation_stats, + ) + monkeypatch.setattr( + data_api, + "_get_semantic_relation_evidence_samples", + empty_list, + ) + monkeypatch.setattr( + data_api, + "_get_attachment_parse_stats", + zero_attachment_parse_stats, + ) + monkeypatch.setattr(data_api, "_get_attachment_parse_breakdown", empty_list) + monkeypatch.setattr(data_api, "_get_connector_events", empty_list) + monkeypatch.setattr(data_api, "_get_attachment_assets", empty_list) + + +@pytest.mark.asyncio +async def test_workspace_document_lookup_rejects_cross_organization_idor() -> None: + """A reused workspace identifier must not cross the tenant boundary.""" + + document = _document( + document_id="doc-org-b", + organization_id="org-b", + document_name="private.md", + ) + session = _OrganizationAwareSession(document) + + with pytest.raises(HTTPException) as exc_info: + await data_api._get_workspace_document( + session, # type: ignore[arg-type] + _auth_context("org-a"), + document.document_id, + ) + + assert exc_info.value.status_code == 404 + + +@pytest.mark.asyncio +async def test_workspace_document_lookup_preserves_same_organization_collaboration() -> None: + """Workspace members in the owning organization retain shared-document access.""" + + document = _document( + document_id="doc-org-a", + organization_id="org-a", + document_name="shared.md", + ) + session = _OrganizationAwareSession(document) + + resolved = await data_api._get_workspace_document( + session, # type: ignore[arg-type] + _auth_context("org-a"), + document.document_id, + ) + + assert resolved is document + + +@pytest.mark.asyncio +async def test_personal_workspace_document_lookup_rejects_organization_document() -> None: + """Personal-scope sessions cannot read organization-owned workspace documents.""" + + document = _document( + document_id="doc-org-owned", + organization_id="org-a", + document_name="org.md", + ) + session = _OrganizationAwareSession(document) + + with pytest.raises(HTTPException) as exc_info: + await data_api._get_workspace_document( + session, # type: ignore[arg-type] + _auth_context(None), + document.document_id, + ) + + assert exc_info.value.status_code == 404 + + +@pytest.mark.asyncio +async def test_personal_workspace_document_lookup_preserves_personal_document() -> None: + """Personal-scope sessions can read organization-less workspace documents.""" + + document = _document( + document_id="doc-personal", + organization_id=None, + document_name="personal.md", + ) + session = _OrganizationAwareSession(document) + + resolved = await data_api._get_workspace_document( + session, # type: ignore[arg-type] + _auth_context(None), + document.document_id, + ) + + assert resolved is document + + +@pytest.mark.asyncio +async def test_data_quality_surface_excludes_foreign_organization_document( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The data-quality surface lists only documents owned by the organization.""" + + documents = [ + _document( + document_id="doc-org-a", + organization_id="org-a", + document_name="visible.md", + ), + _document( + document_id="doc-org-b", + organization_id="org-b", + document_name="hidden.md", + ), + ] + _install_quality_surface_fixtures(monkeypatch, documents) + + surface = await data_api.get_data_quality_surface( + _auth_context("org-a"), + object(), # type: ignore[arg-type] + ) + + document_repository = next( + repository + for repository in surface.repositories + if repository.repository_type == "document_repository" + ) + assert document_repository.object_count == 1 + assert [ + asset.asset_key + for asset in surface.repository_assets + if asset.asset_type == "workspace_document" + ] == ["doc-org-a"] diff --git a/backend/tests/test_dav_api.py b/backend/tests/test_dav_api.py index 70d455ea9..015e4b4b2 100644 --- a/backend/tests/test_dav_api.py +++ b/backend/tests/test_dav_api.py @@ -1,8 +1,10 @@ import defusedxml.ElementTree as ET import pytest +from fastapi import HTTPException from fastapi.testclient import TestClient +from api.dav import _normalize_dav_authorization_path from main import app from services.webdav_service import webdav_service @@ -61,11 +63,19 @@ def test_dav_route_uses_signed_session_dependency(): assert response.status_code == 401 -def test_dav_options(dev_auth_dependency_overrides): +def test_dav_options_advertises_only_implemented_capabilities( + dev_auth_dependency_overrides, +): with TestClient(app) as client: response = client.options("/dav/user123/projects/", headers=AUTH_HEADERS) - assert response.status_code == 200 - assert "calendar-access" in response.headers.get("DAV", "") + + assert response.status_code == 200 + assert response.headers["DAV"] == "1" + assert { + method.strip() + for method in response.headers["Allow"].split(",") + if method.strip() + } == {"OPTIONS", "PROPFIND"} def test_dav_rejects_different_user_path(dev_auth_dependency_overrides): @@ -94,6 +104,91 @@ def test_dav_rejects_ownerless_options_before_capability_discovery( assert response.json()["detail"] == "DAV path must include an owner user" +def test_dav_authorization_path_preserves_literal_percent_data(): + assert ( + _normalize_dav_authorization_path("user123/projects/literal%25") + == "user123/projects/literal%25" + ) + assert ( + _normalize_dav_authorization_path("user123/projects/분석%zz") + == "user123/projects/분석%zz" + ) + + +@pytest.mark.parametrize( + "path", + [ + "user123/projects/%2e%2e/secret", + "user123/projects/%252e%252e/secret", + "user123/projects/%2fsecret", + "user123/projects/%5csecret", + "user123/projects/%00secret", + "user123/projects/%250asecret", + ], +) +def test_dav_authorization_path_rejects_nested_structural_decoding(path): + with pytest.raises(HTTPException) as exc_info: + _normalize_dav_authorization_path(path) + + assert exc_info.value.status_code == 400 + assert exc_info.value.detail == "DAV path contains ambiguous percent encoding" + + +@pytest.mark.parametrize("control_character", ["\x00", "\x1f", "\x7f"]) +def test_dav_authorization_path_rejects_decoded_controls(control_character): + with pytest.raises(HTTPException) as exc_info: + _normalize_dav_authorization_path( + f"user123/projects/report{control_character}name" + ) + + assert exc_info.value.status_code == 400 + assert exc_info.value.detail == "DAV path contains control characters" + + +def test_dav_authorization_path_normalizes_literal_backslashes_once(): + assert ( + _normalize_dav_authorization_path(r"user123\projects\demo") + == "user123/projects/demo" + ) + + +def test_dav_authorization_path_has_explicit_resource_boundary(): + prefix = "user123/projects/" + boundary_path = prefix + ("x" * (8192 - len(prefix))) + assert _normalize_dav_authorization_path(boundary_path) == boundary_path + + with pytest.raises(HTTPException) as exc_info: + _normalize_dav_authorization_path(boundary_path + "x") + + assert exc_info.value.status_code == 414 + assert exc_info.value.detail == "DAV path exceeds authorization length limit" + + +def test_dav_route_rejects_framework_decoded_nested_traversal( + dev_auth_dependency_overrides, +): + """Reject traversal after ASGI exposes the nested encoding as a literal segment.""" + + with TestClient(app) as client: + response = client.options( + "/dav/user123/projects/%252e%252e/secret", + headers=AUTH_HEADERS, + ) + + assert response.status_code == 403 + assert response.json()["detail"] == "DAV path must include an owner user" + + +def test_dav_route_preserves_encoded_percent_as_data(dev_auth_dependency_overrides): + with TestClient(app) as client: + response = client.options( + "/dav/user123/projects/literal%2525", + headers=AUTH_HEADERS, + ) + + assert response.status_code == 200 + + def test_dav_propfind(dev_auth_dependency_overrides, stub_dav_project_folders): with TestClient(app) as client: response = client.request( @@ -114,63 +209,60 @@ def test_dav_propfind_escapes_path_values( "PROPFIND", "/dav/user123/projects/x%26y%3Cz%3E", headers=AUTH_HEADERS ) assert response.status_code == 207 + assert "" not in response.text ET.fromstring(response.text) -def test_dav_put(dev_auth_dependency_overrides, caplog): - import logging - - caplog.set_level(logging.WARNING, logger="api.dav") +@pytest.mark.parametrize( + "method", + [ + "GET", + "PUT", + "DELETE", + "MKCOL", + "REPORT", + "PROPPATCH", + "COPY", + "MOVE", + "LOCK", + "UNLOCK", + ], +) +def test_dav_unimplemented_methods_are_not_registered( + dev_auth_dependency_overrides, + method, +): with TestClient(app) as client: - response = client.put( + response = client.request( + method, "/dav/user123/projects/file.ics", - content=b"BEGIN:VCALENDAR\r\nEND:VCALENDAR", + content=b"BEGIN:VCALENDAR\r\nEND:VCALENDAR" if method == "PUT" else None, headers=AUTH_HEADERS, ) - assert response.status_code == 501 - assert "Provider-backed DAV writeback is not implemented" in response.text - assert "etag" not in {header.lower() for header in response.headers} - assert any( - "provider-backed DAV writeback is not implemented" in record.getMessage() - for record in caplog.records - ) - -def test_dav_unsupported_method_logs_reason(dev_auth_dependency_overrides, caplog): - import logging - - caplog.set_level(logging.WARNING, logger="api.dav") - with TestClient(app) as client: - response = client.delete( - "/dav/user123/projects/file.ics", - headers=AUTH_HEADERS, - ) + assert response.status_code == 405 + assert "etag" not in {header.lower() for header in response.headers} + assert { + allowed.strip() + for allowed in response.headers["Allow"].split(",") + if allowed.strip() + } == {"OPTIONS", "PROPFIND"} - assert response.status_code == 501 - assert "Provider-backed DAV method is not implemented" in response.text - assert any( - "method is not implemented for the provider-backed DAV gateway" - in record.getMessage() - for record in caplog.records - ) def test_dav_log_injection_prevention(dev_auth_dependency_overrides, caplog): - """ - Test that DAV handlers safely encode control characters in the requested path, - preventing log injection vulnerabilities. - """ + """Reject decoded control characters before they can reach DAV request logs.""" + import asyncio import logging - caplog.set_level(logging.INFO) - malicious_path = "user123/projects/test\x1b[31minjected\n\r" - - # Since HTTP clients block raw control chars and starlette unquotes but might reject it before reaching our route, - # we test the handler directly to ensure the logger is using repr(). - import asyncio from fastapi import Request + from api.auth import AuthContext + from api.dav import dav_handler + + caplog.set_level(logging.INFO, logger="api.dav") + malicious_path = "user123/projects/test\x1b[31minjected\n\r" scope = { "type": "http", "method": "OPTIONS", @@ -178,26 +270,23 @@ def test_dav_log_injection_prevention(dev_auth_dependency_overrides, caplog): } async def run_handler(): - req = Request(scope) - from api.auth import AuthContext - auth_ctx = AuthContext(user_id="user123", organization_id="org1", role="user", group_ids=[], workspace_id="ws1") - - from api.dav import dav_handler - await dav_handler(request=req, path=malicious_path, auth_context=auth_ctx) - - asyncio.run(run_handler()) - - # In some fastapi versions, returning an unexpected path might return 404. Let's just assert the log was captured. - # The vulnerability is about the logger. + request = Request(scope) + auth_context = AuthContext( + user_id="user123", + organization_id="org1", + role="user", + group_ids=[], + workspace_id="ws1", + ) + await dav_handler( + request=request, + path=malicious_path, + auth_context=auth_context, + ) - # Assert that the raw ansi escape / newline was not logged, but encoded - raw_ansi = "\x1b[31m" - found_in_logs = False - for record in caplog.records: - if "DAV Request" in record.message: - assert raw_ansi not in record.message, "Raw ANSI escape sequence found in logs!" - assert "\n" not in record.message[12:], "Raw newline found in log message body!" - assert "\\x1b[31minjected\\n\\r" in record.message or "\\x1b[31minjected\\r\\n" in record.message, "Escaped characters missing from log message!" - found_in_logs = True + with pytest.raises(HTTPException) as exc_info: + asyncio.run(run_handler()) - assert found_in_logs, "DAV Request log was not found" + assert exc_info.value.status_code == 400 + assert exc_info.value.detail == "DAV path contains control characters" + assert not any("DAV Request" in record.getMessage() for record in caplog.records) diff --git a/backend/tests/test_dav_normalized_route.py b/backend/tests/test_dav_normalized_route.py new file mode 100644 index 000000000..6b8751102 --- /dev/null +++ b/backend/tests/test_dav_normalized_route.py @@ -0,0 +1,55 @@ +"""Route-level regression coverage for canonical DAV authorization paths.""" + +import pytest +from fastapi.testclient import TestClient + +from main import app +from services.webdav_service import webdav_service + +AUTH_HEADERS = { + "X-User-Id": "user123", + "X-User-Role": "organization_admin", + "X-Organization-Id": "org-acme", +} + + +@pytest.fixture +def stub_dav_project_folder(monkeypatch): + """Return one deterministic project folder through the production DAV service seam.""" + + async def fake_project_folders(db, user_id, organization_id, folder_uid=None): + assert user_id == "user123" + assert organization_id == "org-acme" + assert folder_uid == "demo" + return [ + { + "folder_uid": "demo", + "project_name": "demo", + "webdav_path": "/projects/demo", + "owner_user_id": user_id, + "organization_id": organization_id, + } + ] + + monkeypatch.setattr( + webdav_service, + "get_project_folders_from_db", + fake_project_folders, + ) + + +def test_propfind_routes_framework_decoded_backslashes_through_canonical_path( + dev_auth_dependency_overrides, + stub_dav_project_folder, +): + """A once-decoded backslash path must reach the same project route as slashes.""" + + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123%5Cprojects%5Cdemo", + headers=AUTH_HEADERS, + ) + + assert response.status_code == 207 + assert "demo" in response.text diff --git a/docs/doctoring/dav-and-local-provider-network-boundaries.md b/docs/doctoring/dav-and-local-provider-network-boundaries.md new file mode 100644 index 000000000..dce148d4c --- /dev/null +++ b/docs/doctoring/dav-and-local-provider-network-boundaries.md @@ -0,0 +1,47 @@ +# DAV/WebDAV single-decode and tenant boundary + +## Scope + +This decision is limited to Naruon's DAV/WebDAV edge and workspace-document authorization boundary. The historical filename is retained to avoid churn, but LLM-provider routing, provider URL admission, DNS rebinding controls, and provider-network policy are not owned by this change. + +## Problem + +A DAV path can cross an authorization boundary if percent-encoding is decoded more than once. For example, a residual encoded slash can become a hierarchy separator during a second decode after the edge has already authorized a different canonical path. The DAV surface must also advertise only capabilities that the server actually implements, and workspace-document access must remain scoped by organization identity. + +## Decision + +- Decode the incoming DAV path exactly once at the edge. +- Reject a decoded path that still contains a syntactically valid percent triplet whose second decoding could change path semantics. +- Normalize and authorize the repository-relative path only after that single decode. +- Downstream DAV/WebDAV services consume the canonical path and do not decode it again. +- Advertise DAV Level 1 capability truth only; do not claim unsupported DAV levels or extension tokens. +- Workspace-document reads and creates require the caller's `org_id` and preserve organization scoping through the repository boundary. + +These rules keep path interpretation, capability truth, and tenant authorization inside the Naruon DAV/document bounded context. + +## Verification + +The executable contract is covered by: + +- `backend/tests/test_dav_path_canonicalization.py` +- `backend/tests/test_dav_auth.py` +- `backend/tests/test_dav_integration.py` +- `backend/tests/test_dav_propfind.py` +- `backend/tests/test_webdav_security.py` +- `backend/tests/test_workspace_document_tenancy.py` + +The PR must also pass the repository's current exact-head required workflows. Evidence from a predecessor SHA is not merge authority. + +## External owner boundary + +LLM-provider selection, provider URL/routing policy, provider-network SSRF policy, DNS-rebinding protection, credentials, and fallback behavior belong to the released `contextual-orchestrator` owner path. This PR restores `backend/services/llm_provider_urls.py` and `backend/tests/test_llm_provider_urls.py` to the protected `develop` versions and does not change provider policy. Broader removal or migration of Naruon's protected legacy direct-provider implementation is handled by the canonical docs/governance and contextual-orchestrator integration lane rather than by this DAV repair. + +## Rollback + +Rollback reverts only the DAV/document changes in this branch. It must not introduce a second decoder or expand DAV capability advertisement. Provider-policy files remain at the protected-base versions throughout this repair. + +## Traceability + +Berners-Lee, T., Fielding, R., & Masinter, L. (2005). *Uniform Resource Identifier (URI): Generic syntax* (RFC 3986, §§ 2.1, 2.4). Internet Engineering Task Force. https://doi.org/10.17487/RFC3986 + +Dusseault, L. (Ed.). (2007). *HTTP extensions for Web Distributed Authoring and Versioning (WebDAV)* (RFC 4918, § 10.1). Internet Engineering Task Force. https://doi.org/10.17487/RFC4918