diff --git a/backend/api/dav.py b/backend/api/dav.py index d618c25f3..75151af64 100644 --- a/backend/api/dav.py +++ b/backend/api/dav.py @@ -1,7 +1,11 @@ import logging +import re from html import escape as escape_xml_text -from urllib.parse import unquote +from unicodedata import category as unicode_category +from urllib.parse import unquote_to_bytes +from defusedxml import ElementTree as DefusedElementTree +from defusedxml.common import DefusedXmlException from fastapi import APIRouter, Depends, HTTPException, Request, Response from sqlalchemy.ext.asyncio import AsyncSession @@ -14,14 +18,84 @@ router = APIRouter(prefix="/dav", tags=["dav"]) +_INVALID_RAW_PERCENT_ESCAPE = re.compile(br"%(?![0-9A-Fa-f]{2})") +_SECOND_PASS_PERCENT_ESCAPE = re.compile(br"%[0-9A-Fa-f]{2}") +_ENCODED_CONTROL_CHARACTER = re.compile(br"%(?:0[0-9A-Fa-f]|1[0-9A-Fa-f]|7[Ff])") +_DAV_RAW_PATH_MAX_OCTETS = 8192 +_DAV_DECODED_PATH_MAX_CHARACTERS = 8192 +_DAV_PROPFIND_BODY_MAX_OCTETS = 8192 +_DAV_PROJECT_COLLECTION_MEMBER_LIMIT = 256 +_XML_SPACE_CHARACTERS = frozenset(" \t\r\n") +_DAV_PROPFIND_DIRECTIVE_TAGS = frozenset( + { + "{DAV:}allprop", + "{DAV:}include", + "{DAV:}prop", + "{DAV:}propname", + } +) + + +def _validate_dav_raw_request_path(request: Request, decoded_path: str) -> None: + """Validate wire encoding before trusting the framework-decoded DAV path.""" + raw_path = request.scope.get("raw_path") + if raw_path is None: + if len(decoded_path) > _DAV_DECODED_PATH_MAX_CHARACTERS: + raise HTTPException( + status_code=414, + detail="DAV decoded path exceeds 8192 characters", + ) + if "%" in decoded_path: + raise HTTPException( + status_code=400, + detail="DAV raw path required for percent-bearing path", + ) + return + if not isinstance(raw_path, bytes): + raise HTTPException(status_code=400, detail="DAV raw path is unavailable") + if len(raw_path) > _DAV_RAW_PATH_MAX_OCTETS: + raise HTTPException( + status_code=414, + detail="DAV raw path exceeds 8192 octets", + ) + if any(byte < 0x20 or byte == 0x7F for byte in raw_path): + raise HTTPException(status_code=400, detail="DAV path contains control characters") + if _INVALID_RAW_PERCENT_ESCAPE.search(raw_path): + raise HTTPException( + status_code=400, detail="DAV path contains invalid percent encoding" + ) + if _ENCODED_CONTROL_CHARACTER.search(raw_path): + raise HTTPException(status_code=400, detail="DAV path contains control characters") + first_wire_decode = unquote_to_bytes(raw_path) + if _SECOND_PASS_PERCENT_ESCAPE.search(first_wire_decode): + raise HTTPException( + status_code=400, detail="DAV path contains nested percent encoding" + ) + + def _normalize_dav_authorization_path(path: str) -> str: + """Normalize the path value after ASGI routing has already decoded the target.""" + source_has_leading_slash = path.startswith("/") 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 "\ufffd" in normalized_path or any( + 0xD800 <= ord(character) <= 0xDFFF for character in normalized_path + ): + raise HTTPException(status_code=400, detail="DAV path contains invalid Unicode") + if any(unicode_category(character) == "Cc" for character in normalized_path): + raise HTTPException(status_code=400, detail="DAV path contains control characters") + if normalized_path.startswith("/") and not source_has_leading_slash: + raise HTTPException( + status_code=400, + detail="DAV path contains ambiguous empty segments", + ) + path_segments = normalized_path.split("/") + has_traversal_segment = any(segment in {".", ".."} for segment in path_segments) + if "//" in normalized_path and not has_traversal_segment: + raise HTTPException( + status_code=400, + detail="DAV path contains ambiguous empty segments", + ) + return normalized_path def _dav_path_owner_user_id(path: str) -> str | None: @@ -91,18 +165,176 @@ def _dav_xml_response(responses: list[str]) -> Response: ) +def _dav_finite_depth_error_response() -> Response: + return Response( + content=( + '\n' + '' + ), + media_type="application/xml", + status_code=403, + ) + + +def _dav_project_member_limit_error_response() -> Response: + return Response( + content=( + '\n' + '' + '' + ), + media_type="application/xml", + status_code=403, + ) + + def _dav_depth(request: Request) -> str: - depth = request.headers.get("Depth", "1").strip().lower() - if depth == "0": - return "0" - return "1" + depth_header = request.headers.get("Depth") + if depth_header is None: + return "infinity" + depth = depth_header.strip().lower() + if depth not in {"0", "1", "infinity"}: + raise HTTPException( + status_code=400, + detail="DAV Depth must be 0, 1, or infinity", + ) + return depth + + +def _has_non_xml_space_content(text: str | None) -> bool: + """Return whether element-only content contains characters outside XML S.""" + return bool(text) and any(character not in _XML_SPACE_CHARACTERS for character in text) + + +def _validate_dav_empty_directive(element, *, directive_name: str) -> None: + """Validate EMPTY directive text while ignoring RFC-permitted extension children.""" + extension_children = list(element) + if _has_non_xml_space_content(element.text) or any( + _has_non_xml_space_content(extension_child.tail) + for extension_child in extension_children + ): + raise HTTPException( + status_code=400, + detail=f"DAV {directive_name} directive must not contain text", + ) + + +def _validate_dav_property_name_container(element, *, directive_name: str) -> None: + """Reject text, mixed content, or property values in name-only selectors.""" + property_names = list(element) + if _has_non_xml_space_content(element.text) or any( + _has_non_xml_space_content(property_name.tail) + for property_name in property_names + ): + raise HTTPException( + status_code=400, + detail=f"DAV {directive_name} directive must not contain text", + ) + if any( + list(property_name) or _has_non_xml_space_content(property_name.text) + for property_name in property_names + ): + raise HTTPException( + status_code=400, + detail=f"DAV {directive_name} directive must contain property names only", + ) + + +async def _validate_dav_propfind_body(request: Request) -> None: + """Validate the bounded PROPFIND body semantics this discovery slice supports.""" + request_body = bytearray() + async for body_chunk in request.stream(): + if not body_chunk: + continue + if len(request_body) + len(body_chunk) > _DAV_PROPFIND_BODY_MAX_OCTETS: + raise HTTPException( + status_code=413, + detail="DAV PROPFIND body exceeds 8192 octets", + ) + request_body.extend(body_chunk) + + if not request_body: + return + + try: + propfind_element = DefusedElementTree.fromstring(bytes(request_body)) + except (DefusedXmlException, DefusedElementTree.ParseError) as exc: + raise HTTPException( + status_code=400, + detail="DAV PROPFIND body must be well-formed XML", + ) from exc + + if propfind_element.tag != "{DAV:}propfind": + raise HTTPException( + status_code=400, + detail="DAV PROPFIND body must contain DAV:propfind", + ) + + all_children = list(propfind_element) + if _has_non_xml_space_content(propfind_element.text) or any( + _has_non_xml_space_content(child.tail) for child in all_children + ): + raise HTTPException( + status_code=400, + detail="DAV PROPFIND body must use element-only directive content", + ) + + directives = [ + child for child in all_children if child.tag in _DAV_PROPFIND_DIRECTIVE_TAGS + ] + + if len(directives) == 1 and directives[0].tag == "{DAV:}allprop": + _validate_dav_empty_directive(directives[0], directive_name="allprop") + return + + if len(directives) == 2 and {element.tag for element in directives} == { + "{DAV:}allprop", + "{DAV:}include", + }: + allprop_element = next( + element for element in directives if element.tag == "{DAV:}allprop" + ) + include_element = next( + element for element in directives if element.tag == "{DAV:}include" + ) + _validate_dav_empty_directive(allprop_element, directive_name="allprop") + _validate_dav_property_name_container( + include_element, + directive_name="include", + ) + raise HTTPException( + status_code=501, + detail="DAV allprop include semantics are not implemented", + ) + + if len(directives) == 1 and directives[0].tag == "{DAV:}propname": + _validate_dav_empty_directive(directives[0], directive_name="propname") + raise HTTPException( + status_code=501, + detail="DAV propname PROPFIND semantics are not implemented", + ) + + if len(directives) == 1 and directives[0].tag == "{DAV:}prop": + _validate_dav_property_name_container( + directives[0], + directive_name="prop", + ) + raise HTTPException( + status_code=501, + detail="DAV selected-property PROPFIND semantics are not implemented", + ) + + raise HTTPException( + status_code=400, + detail="DAV PROPFIND body has invalid directive structure", + ) def _project_folder_response(path_owner_user_id: str, folder: dict) -> str: folder_uid = str(folder["folder_uid"]) project_name = str(folder["project_name"]) return _dav_response_xml( - href=f"/api/dav/{path_owner_user_id}/projects/{folder_uid}", + href=f"/api/dav/{path_owner_user_id}/projects/{folder_uid}/", display_name=project_name, is_collection=True, ) @@ -121,18 +353,37 @@ async def _handle_project_propfind( path_owner_user_id = segments[0] depth = _dav_depth(request) + if depth == "infinity": + return _dav_finite_depth_error_response() + folder_uid = segments[2] if len(segments) == 3 else None if len(segments) > 3: raise HTTPException(status_code=404, detail="DAV project folder not found") + addressed_collection_response = _dav_response_xml( + href=f"/api/dav/{path_owner_user_id}/projects/", + display_name="projects", + is_collection=True, + ) if folder_uid is None and depth == "0": + return _dav_xml_response([addressed_collection_response]) + + if folder_uid is None: + folders = await webdav_service.get_project_folders_from_db( + db, + auth_context.user_id, + auth_context.organization_id, + max_results=_DAV_PROJECT_COLLECTION_MEMBER_LIMIT + 1, + ) + if len(folders) > _DAV_PROJECT_COLLECTION_MEMBER_LIMIT: + return _dav_project_member_limit_error_response() return _dav_xml_response( [ - _dav_response_xml( - href=f"/api/dav/{path_owner_user_id}/projects/", - display_name="projects", - is_collection=True, - ) + addressed_collection_response, + *[ + _project_folder_response(path_owner_user_id, folder) + for folder in folders + ], ] ) @@ -142,13 +393,12 @@ async def _handle_project_propfind( auth_context.organization_id, folder_uid=folder_uid, ) - - if folder_uid is None: - return _dav_xml_response( - [_project_folder_response(path_owner_user_id, folder) for folder in folders] - ) - if folders: + if depth == "1": + raise HTTPException( + status_code=501, + detail="DAV project collection member enumeration is not implemented", + ) return _dav_xml_response( [_project_folder_response(path_owner_user_id, folder) for folder in folders] ) @@ -173,32 +423,30 @@ async def dav_handler( Provider-backed writeback stays fail-closed until source capability and ETag/If-Match enforcement are available through signed writeback intents. """ - _ensure_dav_owner_scope(path, auth_context) - safe_path = repr(path)[1:-1] + _validate_dav_raw_request_path(request, path) + if path.startswith("/"): + raise HTTPException( + status_code=400, + detail="DAV path contains ambiguous empty segments", + ) + canonical_path = _normalize_dav_authorization_path(path) + _ensure_dav_owner_scope(canonical_path, auth_context) + safe_path = repr(canonical_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) + return Response(status_code=200, headers={"Allow": "OPTIONS, PROPFIND"}) if request.method == "PROPFIND": + await _validate_dav_propfind_body(request) return await _handle_project_propfind( request=request, - path=path, + path=canonical_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", diff --git a/backend/services/webdav_service.py b/backend/services/webdav_service.py index 284bd1696..b2c1998ef 100644 --- a/backend/services/webdav_service.py +++ b/backend/services/webdav_service.py @@ -142,6 +142,7 @@ async def get_project_folders_from_db( user_id: str, organization_id: str | None, folder_uid: str | None = None, + max_results: int | None = None, ) -> List[Dict[str, Any]]: stmt = select(ProjectFolder).where( ProjectFolder.user_id == user_id, @@ -151,6 +152,8 @@ async def get_project_folders_from_db( ) if folder_uid is not None: stmt = stmt.where(ProjectFolder.folder_uid == folder_uid) + if max_results is not None: + stmt = stmt.limit(max_results) result = await session.execute(stmt) return [ { diff --git a/backend/tests/test_dav_api.py b/backend/tests/test_dav_api.py index 70d455ea9..2dffaa3d3 100644 --- a/backend/tests/test_dav_api.py +++ b/backend/tests/test_dav_api.py @@ -1,8 +1,11 @@ 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 @@ -15,10 +18,13 @@ @pytest.fixture def stub_dav_project_folders(monkeypatch): - async def fake_project_folders(db, user_id, organization_id, folder_uid=None): + async def fake_project_folders( + db, user_id, organization_id, folder_uid=None, max_results=None + ): assert user_id == "user123" assert organization_id == "org-acme" if folder_uid is None: + assert max_results == 257 return [ { "folder_uid": "demo", @@ -28,6 +34,7 @@ async def fake_project_folders(db, user_id, organization_id, folder_uid=None): "organization_id": organization_id, } ] + assert max_results is None return [ { "folder_uid": folder_uid, @@ -64,8 +71,13 @@ def test_dav_route_uses_signed_session_dependency(): def test_dav_options(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 "DAV" not in response.headers + assert {method.strip() for method in response.headers["Allow"].split(",")} == { + "OPTIONS", + "PROPFIND", + } def test_dav_rejects_different_user_path(dev_auth_dependency_overrides): @@ -97,7 +109,9 @@ def test_dav_rejects_ownerless_options_before_capability_discovery( def test_dav_propfind(dev_auth_dependency_overrides, stub_dav_project_folders): with TestClient(app) as client: response = client.request( - "PROPFIND", "/dav/user123/projects/", headers=AUTH_HEADERS + "PROPFIND", + "/dav/user123/projects/", + headers={**AUTH_HEADERS, "Depth": "1"}, ) assert response.status_code == 207 assert " None: + await dav_handler( + request=request, + path=malicious_path, + auth_context=auth_context, + ) + + with pytest.raises(HTTPException) as exc_info: + asyncio.run(run_handler()) + + 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) + + +@pytest.mark.parametrize( + "request_path", + [ + "/dav/user123/projects/%252e%252e", + "/dav/user123/projects/%25252e%25252e", + "/dav/user123/projects/alice%255c..%255cbob", + "/dav/user123/projects/%2525", + "/dav/user123/projects/%25%32%65", + "/dav/user123/projects/alice%25%35%63..%25%35%63bob", + ], +) +def test_dav_route_rejects_ambiguous_nested_encoding( + dev_auth_dependency_overrides, + request_path: str, +) -> None: + with TestClient(app) as client: + response = client.request("PROPFIND", request_path, headers=AUTH_HEADERS) + + assert response.status_code == 400 + assert response.json()["detail"] == "DAV path contains nested percent encoding" + + +@pytest.mark.parametrize( + "request_path", + [ + "/dav/user123/projects/report%25", + "/dav/user123/projects/%25report", + ], +) +def test_dav_route_preserves_encoded_percent_as_data( + dev_auth_dependency_overrides, + stub_dav_project_folders, + request_path: str, +) -> None: + with TestClient(app) as client: + response = client.request( + "PROPFIND", + request_path, + headers={**AUTH_HEADERS, "Depth": "0"}, + ) + + assert response.status_code == 207 + assert "%" in response.text + + +@pytest.mark.parametrize( + "request_path", + [ + "/dav/user123/projects/%GG", + "/dav/user123/projects/%2", + ], +) +def test_dav_route_rejects_malformed_raw_percent_escape( + dev_auth_dependency_overrides, + request_path: str, +) -> None: + with TestClient(app) as client: + response = client.request("PROPFIND", request_path, headers=AUTH_HEADERS) + + assert response.status_code == 400 + assert response.json()["detail"] == "DAV path contains invalid percent encoding" + + +@pytest.mark.parametrize( + "request_path", + [ + "/dav/user123/projects/%00", + "/dav/user123/projects/%0A", + "/dav/user123/projects/%7F", + "/dav/user123/projects/%C2%80", + "/dav/user123/projects/%C2%9F", + ], +) +def test_dav_route_rejects_percent_encoded_control_character( + dev_auth_dependency_overrides, + request_path: str, +) -> None: + with TestClient(app) as client: + response = client.request("PROPFIND", request_path, headers=AUTH_HEADERS) + + assert response.status_code == 400 + assert response.json()["detail"] == "DAV path contains control characters" - # 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(). + +def test_dav_route_rejects_invalid_utf8_replacement( + dev_auth_dependency_overrides, +) -> None: + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123/projects/%FF", + headers=AUTH_HEADERS, + ) + + assert response.status_code == 400 + assert response.json()["detail"] == "DAV path contains invalid Unicode" + + +@pytest.mark.parametrize( + "request_path", + [ + "/dav/user123/projects/%2e%2e", + "/dav/user123/projects/%5c..%5c", + ], +) +def test_dav_route_rejects_single_decode_traversal( + dev_auth_dependency_overrides, + request_path: str, +) -> None: + with TestClient(app) as client: + response = client.request("PROPFIND", request_path, headers=AUTH_HEADERS) + + assert response.status_code == 403 + assert response.json()["detail"] == "DAV path must include an owner user" + + +def test_dav_missing_raw_path_fails_closed_for_residual_percent( + dev_auth_dependency_overrides, +) -> None: import asyncio + from fastapi import Request + from api.auth import AuthContext + from api.dav import dav_handler + scope = { "type": "http", "method": "OPTIONS", "headers": [], + "path": "/dav/user123/projects/report%", } + request = Request(scope) + auth_context = AuthContext( + user_id="user123", + organization_id="org1", + role="user", + group_ids=[], + workspace_id="ws1", + ) + + with pytest.raises(HTTPException) as exc_info: + asyncio.run( + dav_handler( + request=request, + path="user123/projects/report%", + auth_context=auth_context, + ) + ) + + assert exc_info.value.status_code == 400 + assert exc_info.value.detail == "DAV raw path required for percent-bearing path" + + +def test_dav_missing_raw_path_allows_percent_free_decoded_path( + dev_auth_dependency_overrides, +) -> None: + import asyncio - 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 fastapi import Request + + from api.auth import AuthContext + from api.dav import dav_handler + + request = Request( + { + "type": "http", + "method": "OPTIONS", + "headers": [], + "path": "/dav/user123/projects/", + } + ) + auth_context = AuthContext( + user_id="user123", + organization_id="org1", + role="user", + group_ids=[], + workspace_id="ws1", + ) + + response = asyncio.run( + dav_handler( + request=request, + path="user123/projects/", + auth_context=auth_context, + ) + ) - from api.dav import dav_handler - await dav_handler(request=req, path=malicious_path, auth_context=auth_ctx) + assert response.status_code == 200 + assert "DAV" not in response.headers + assert response.headers.get("Allow") == "OPTIONS, PROPFIND" - 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. +def test_normalize_dav_authorization_path_treats_route_path_as_already_decoded() -> None: + assert _normalize_dav_authorization_path("/alice/docs") == "/alice/docs" + assert _normalize_dav_authorization_path("/%2e%2e/bob") == "/%2e%2e/bob" + assert _normalize_dav_authorization_path("/alice%/docs") == "/alice%/docs" - # 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 - assert found_in_logs, "DAV Request log was not found" +def test_normalize_dav_authorization_path_has_no_recursive_input_amplification() -> None: + path = "/alice/" + ("segment-" * 8192) + assert _normalize_dav_authorization_path(path) == path diff --git a/backend/tests/test_dav_canonical_path_succession.py b/backend/tests/test_dav_canonical_path_succession.py new file mode 100644 index 000000000..25deb412b --- /dev/null +++ b/backend/tests/test_dav_canonical_path_succession.py @@ -0,0 +1,140 @@ +"""Regression coverage for DAV canonical-path succession and request bounds.""" + +import pytest +from fastapi import HTTPException, Request +from fastapi.testclient import TestClient + +from api.dav import _validate_dav_raw_request_path +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: pytest.MonkeyPatch) -> None: + """Return one project folder through the production DAV service seam.""" + + async def fake_project_folders( + db: object, + user_id: str, + organization_id: str | None, + folder_uid: str | None = None, + ) -> list[dict[str, str | 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_propagates_framework_decoded_backslashes_to_project_routing( + dev_auth_dependency_overrides, + stub_dav_project_folder, +) -> None: + """One decoded path representation must drive authorization and routing.""" + + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123%5Cprojects%5Cdemo", + headers={**AUTH_HEADERS, "Depth": "0"}, + ) + + assert response.status_code == 207 + assert "demo" in response.text + + +@pytest.mark.parametrize( + "request_path", + [ + "/dav//user123/projects", + "/dav/user123//projects", + "/dav/user123/projects//demo", + "/dav/user123/projects//", + ], +) +def test_dav_rejects_ambiguous_empty_path_segments( + dev_auth_dependency_overrides, + request_path: str, +) -> None: + """Distinct URI empty segments must not collapse onto canonical DAV resources.""" + + with TestClient(app) as client: + response = client.request("OPTIONS", request_path, headers=AUTH_HEADERS) + + assert response.status_code == 400 + assert response.json()["detail"] == "DAV path contains ambiguous empty segments" + + +def test_dav_raw_path_enforces_explicit_8192_octet_resource_boundary() -> None: + """The wire-path validator must bound work while supporting RFC 9110's minimum.""" + + accepted_raw_path = b"/dav/" + (b"x" * (8192 - len(b"/dav/"))) + accepted_request = Request( + { + "type": "http", + "method": "OPTIONS", + "headers": [], + "path": "/dav/" + ("x" * (8192 - len("/dav/"))), + "raw_path": accepted_raw_path, + } + ) + _validate_dav_raw_request_path( + accepted_request, + "x" * (8192 - len(b"/dav/")), + ) + + rejected_request = Request( + { + "type": "http", + "method": "OPTIONS", + "headers": [], + "path": "/dav/" + ("x" * (8193 - len("/dav/"))), + "raw_path": accepted_raw_path + b"x", + } + ) + with pytest.raises(HTTPException) as exc_info: + _validate_dav_raw_request_path( + rejected_request, + "x" * (8193 - len(b"/dav/")), + ) + + assert exc_info.value.status_code == 414 + assert exc_info.value.detail == "DAV raw path exceeds 8192 octets" + + +def test_dav_missing_raw_path_bounds_decoded_fallback() -> None: + """An ASGI server without raw_path still receives a bounded fail-closed path.""" + + request = Request( + { + "type": "http", + "method": "OPTIONS", + "headers": [], + "path": "/dav/" + ("x" * 8193), + } + ) + + with pytest.raises(HTTPException) as exc_info: + _validate_dav_raw_request_path(request, "x" * 8193) + + assert exc_info.value.status_code == 414 + assert exc_info.value.detail == "DAV decoded path exceeds 8192 characters" diff --git a/backend/tests/test_dav_collection_href_contract.py b/backend/tests/test_dav_collection_href_contract.py new file mode 100644 index 000000000..85f59d3f1 --- /dev/null +++ b/backend/tests/test_dav_collection_href_contract.py @@ -0,0 +1,64 @@ +"""Executable contract for canonical WebDAV collection URLs.""" + +import defusedxml.ElementTree as ET +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", + "Depth": "1", +} + + +def test_propfind_emits_trailing_slashes_for_collection_hrefs( + dev_auth_dependency_overrides, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Every generated URL that identifies a collection should use canonical slash form.""" + + async def project_folders( + db: object, + user_id: str, + organization_id: str | None, + folder_uid: str | None = None, + max_results: int | None = None, + ) -> list[dict[str, str | None]]: + assert user_id == "user123" + assert organization_id == "org-acme" + assert folder_uid is None + assert max_results == 257 + 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", + project_folders, + ) + + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123/projects/", + headers=AUTH_HEADERS, + ) + + assert response.status_code == 207 + root = ET.fromstring(response.text) + hrefs = [item.findtext("{DAV:}href") for item in root.findall("{DAV:}response")] + assert hrefs == [ + "/api/dav/user123/projects/", + "/api/dav/user123/projects/demo/", + ] diff --git a/backend/tests/test_dav_depth_contract.py b/backend/tests/test_dav_depth_contract.py new file mode 100644 index 000000000..aa517c9bd --- /dev/null +++ b/backend/tests/test_dav_depth_contract.py @@ -0,0 +1,376 @@ +"""Executable contract for bounded WebDAV PROPFIND Depth handling.""" + +import defusedxml.ElementTree as ET +import pytest +from fastapi.testclient import TestClient + +from main import app +from services.webdav_service import WebDavService, webdav_service + +AUTH_HEADERS = { + "X-User-Id": "user123", + "X-User-Role": "organization_admin", + "X-Organization-Id": "org-acme", +} + + +@pytest.fixture +def stub_dav_project_folders(monkeypatch: pytest.MonkeyPatch) -> None: + """Provide one collection so Depth handling reaches the PROPFIND boundary.""" + + async def fake_project_folders( + db: object, + user_id: str, + organization_id: str | None, + folder_uid: str | None = None, + max_results: int | None = None, + ) -> list[dict[str, str | None]]: + assert user_id == "user123" + assert organization_id == "org-acme" + assert folder_uid is None + assert max_results == 257 + 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 _assert_finite_depth_error(response) -> None: + assert response.status_code == 403 + assert response.headers["content-type"].startswith("application/xml") + root = ET.fromstring(response.text) + assert root.tag == "{DAV:}error" + assert root.find("{DAV:}propfind-finite-depth") is not None + + +def test_propfind_depth_zero_returns_only_addressed_collection( + dev_auth_dependency_overrides, + stub_dav_project_folders, +) -> None: + """Depth zero must not enumerate the addressed collection's members.""" + + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123/projects/", + headers={**AUTH_HEADERS, "Depth": "0"}, + ) + + assert response.status_code == 207 + root = ET.fromstring(response.text) + responses = root.findall("{DAV:}response") + assert len(responses) == 1 + assert root.findtext(".//{DAV:}displayname") == "projects" + assert "demo" not in response.text + + +def test_propfind_depth_one_returns_addressed_collection_and_direct_member( + dev_auth_dependency_overrides, + stub_dav_project_folders, +) -> None: + """Depth one must include both the addressed collection and direct members.""" + + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123/projects/", + headers={**AUTH_HEADERS, "Depth": "1"}, + ) + + assert response.status_code == 207 + root = ET.fromstring(response.text) + responses = root.findall("{DAV:}response") + assert len(responses) == 2 + assert [ + item.findtext(".//{DAV:}displayname") for item in responses + ] == ["projects", "demo"] + + +def test_propfind_project_collection_depth_zero_returns_addressed_collection( + dev_auth_dependency_overrides, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Depth zero remains a supported property-only read for a known project.""" + + async def selected_project_folder( + db: object, + user_id: str, + organization_id: str | None, + folder_uid: str | None = None, + max_results: int | None = None, + ) -> list[dict[str, str | None]]: + assert user_id == "user123" + assert organization_id == "org-acme" + assert folder_uid == "demo" + assert max_results is None + return [ + { + "folder_uid": "demo", + "project_name": "Demo project", + "webdav_path": "/projects/demo", + "owner_user_id": user_id, + "organization_id": organization_id, + } + ] + + monkeypatch.setattr( + webdav_service, + "get_project_folders_from_db", + selected_project_folder, + ) + + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123/projects/demo/", + headers={**AUTH_HEADERS, "Depth": "0"}, + ) + + assert response.status_code == 207 + root = ET.fromstring(response.text) + responses = root.findall("{DAV:}response") + assert len(responses) == 1 + assert responses[0].findtext(".//{DAV:}displayname") == "Demo project" + + +def test_propfind_project_collection_depth_one_rejects_unimplemented_member_enumeration( + dev_auth_dependency_overrides, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A project collection must not masquerade as empty when members are unknown.""" + + async def selected_project_folder( + db: object, + user_id: str, + organization_id: str | None, + folder_uid: str | None = None, + max_results: int | None = None, + ) -> list[dict[str, str | None]]: + assert user_id == "user123" + assert organization_id == "org-acme" + assert folder_uid == "demo" + assert max_results is None + 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", + selected_project_folder, + ) + + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123/projects/demo/", + headers={**AUTH_HEADERS, "Depth": "1"}, + ) + + assert response.status_code == 501 + assert response.json()["detail"] == ( + "DAV project collection member enumeration is not implemented" + ) + + +def test_propfind_depth_one_accepts_exact_product_ceiling( + dev_auth_dependency_overrides, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Exactly 256 members remains a complete successful depth-one response.""" + + async def maximum_project_folders( + db: object, + user_id: str, + organization_id: str | None, + folder_uid: str | None = None, + max_results: int | None = None, + ) -> list[dict[str, str | None]]: + assert user_id == "user123" + assert organization_id == "org-acme" + assert folder_uid is None + assert max_results == 257 + return [ + { + "folder_uid": f"folder-{index}", + "project_name": f"Folder {index}", + "webdav_path": f"/projects/folder-{index}", + "owner_user_id": user_id, + "organization_id": organization_id, + } + for index in range(256) + ] + + monkeypatch.setattr( + webdav_service, + "get_project_folders_from_db", + maximum_project_folders, + ) + + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123/projects/", + headers={**AUTH_HEADERS, "Depth": "1"}, + ) + + assert response.status_code == 207 + root = ET.fromstring(response.text) + responses = root.findall("{DAV:}response") + assert len(responses) == 257 + assert responses[0].findtext(".//{DAV:}displayname") == "projects" + assert responses[-1].findtext(".//{DAV:}displayname") == "Folder 255" + + +def test_propfind_depth_one_rejects_member_set_above_product_ceiling( + dev_auth_dependency_overrides, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Depth one must refuse an oversized collection instead of materializing it.""" + + async def too_many_project_folders( + db: object, + user_id: str, + organization_id: str | None, + folder_uid: str | None = None, + max_results: int | None = None, + ) -> list[dict[str, str | None]]: + assert user_id == "user123" + assert organization_id == "org-acme" + assert folder_uid is None + assert max_results == 257 + return [ + { + "folder_uid": f"folder-{index}", + "project_name": f"Folder {index}", + "webdav_path": f"/projects/folder-{index}", + "owner_user_id": user_id, + "organization_id": organization_id, + } + for index in range(257) + ] + + monkeypatch.setattr( + webdav_service, + "get_project_folders_from_db", + too_many_project_folders, + ) + + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123/projects/", + headers={**AUTH_HEADERS, "Depth": "1"}, + ) + + assert response.status_code == 403 + assert response.headers["content-type"].startswith("application/xml") + root = ET.fromstring(response.text) + assert root.tag == "{DAV:}error" + assert root.find("{urn:naruon:dav}project-member-limit") is not None + + +@pytest.mark.asyncio +async def test_project_folder_reader_applies_requested_sql_limit() -> None: + """The database read itself must stop at the caller's bounded probe size.""" + + statements: list[object] = [] + + class EmptyScalars: + def all(self) -> list[object]: + return [] + + class EmptyResult: + def scalars(self) -> EmptyScalars: + return EmptyScalars() + + class RecordingSession: + async def execute(self, statement: object) -> EmptyResult: + statements.append(statement) + return EmptyResult() + + service = WebDavService() + await service.get_project_folders_from_db( + RecordingSession(), + "user123", + "org-acme", + max_results=257, + ) + await service.get_project_folders_from_db( + RecordingSession(), + "user123", + "org-acme", + ) + + limited_sql = str(statements[0].compile(compile_kwargs={"literal_binds": True})) + unbounded_sql = str(statements[1].compile(compile_kwargs={"literal_binds": True})) + assert "LIMIT 257" in limited_sql + assert "LIMIT" not in unbounded_sql + + +def test_propfind_without_depth_fails_closed_as_infinite_depth( + dev_auth_dependency_overrides, + stub_dav_project_folders, +) -> None: + """Missing Depth follows RFC 4918's infinity default, which Naruon declines.""" + + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123/projects/", + headers=AUTH_HEADERS, + ) + + _assert_finite_depth_error(response) + + +def test_propfind_infinite_depth_returns_webdav_precondition( + dev_auth_dependency_overrides, + stub_dav_project_folders, +) -> None: + """Unbounded traversal is rejected with the RFC 4918 finite-depth precondition.""" + + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123/projects/", + headers={**AUTH_HEADERS, "Depth": "infinity"}, + ) + + _assert_finite_depth_error(response) + + +@pytest.mark.parametrize("depth", ["", "2", "children", "0, 1"]) +def test_propfind_rejects_invalid_depth_values( + dev_auth_dependency_overrides, + stub_dav_project_folders, + depth: str, +) -> None: + """A Depth value outside RFC 4918's grammar must not be coerced to depth one.""" + + with TestClient(app) as client: + response = client.request( + "PROPFIND", + "/dav/user123/projects/", + headers={**AUTH_HEADERS, "Depth": depth}, + ) + + assert response.status_code == 400 + assert response.json()["detail"] == "DAV Depth must be 0, 1, or infinity" diff --git a/backend/tests/test_dav_propfind_body_contract.py b/backend/tests/test_dav_propfind_body_contract.py new file mode 100644 index 000000000..c46efdb95 --- /dev/null +++ b/backend/tests/test_dav_propfind_body_contract.py @@ -0,0 +1,304 @@ +"""Executable contract for bounded PROPFIND request-body semantics.""" + +import defusedxml.ElementTree as ET +from fastapi.testclient import TestClient + +from main import app + +AUTH_HEADERS = { + "X-User-Id": "user123", + "X-User-Role": "organization_admin", + "X-Organization-Id": "org-acme", + "Depth": "0", + "Content-Type": "application/xml; charset=utf-8", +} + + +def _propfind_request(body: bytes): + with TestClient(app) as client: + return client.request( + "PROPFIND", + "/dav/user123/projects/", + headers=AUTH_HEADERS, + content=body, + ) + + +def test_empty_propfind_body_keeps_supported_discovery_profile( + dev_auth_dependency_overrides, +) -> None: + """An empty body remains equivalent to the supported allprop profile.""" + + response = _propfind_request(b"") + + assert response.status_code == 207 + root = ET.fromstring(response.text) + assert root.findtext(".//{DAV:}displayname") == "projects" + assert root.find(".//{DAV:}resourcetype/{DAV:}collection") is not None + + +def test_explicit_allprop_keeps_supported_discovery_profile( + dev_auth_dependency_overrides, +) -> None: + """Explicit allprop uses the same bounded property profile as an empty body.""" + + response = _propfind_request( + b'' + ) + + assert response.status_code == 207 + root = ET.fromstring(response.text) + assert root.findtext(".//{DAV:}displayname") == "projects" + assert root.find(".//{DAV:}resourcetype/{DAV:}collection") is not None + + +def test_propname_is_not_silently_coerced_to_allprop( + dev_auth_dependency_overrides, +) -> None: + """Unsupported propname semantics must fail instead of returning property values.""" + + response = _propfind_request( + b'' + ) + + assert response.status_code == 501 + + +def test_named_prop_is_not_silently_coerced_to_allprop( + dev_auth_dependency_overrides, +) -> None: + """Unsupported named-property selection must not return the allprop profile.""" + + response = _propfind_request( + b'' + ) + + assert response.status_code == 501 + + +def test_named_prop_value_is_invalid_before_mode_refusal( + dev_auth_dependency_overrides, +) -> None: + """A prop selector may name properties but must not carry property values.""" + + response = _propfind_request( + b'unexpected' + b'' + ) + + assert response.status_code == 400 + + +def test_allprop_include_is_recognized_but_not_silently_ignored( + dev_auth_dependency_overrides, +) -> None: + """A valid allprop/include request must fail until include semantics exist.""" + + response = _propfind_request( + b'' + b'' + ) + + assert response.status_code == 501 + + +def test_include_property_value_is_invalid_before_mode_refusal( + dev_auth_dependency_overrides, +) -> None: + """An include selector names properties; nested property values are invalid.""" + + response = _propfind_request( + b'' + b'unexpected' + ) + + assert response.status_code == 400 + + +def test_include_mixed_text_is_invalid_before_mode_refusal( + dev_auth_dependency_overrides, +) -> None: + """DAV:include cannot contain text or mixed content.""" + + response = _propfind_request( + b'unexpected' + b'' + ) + + assert response.status_code == 400 + + +def test_non_empty_allprop_with_include_is_invalid_before_mode_refusal( + dev_auth_dependency_overrides, +) -> None: + """DAV:allprop stays element-only when paired with a valid include directive.""" + + response = _propfind_request( + b'unexpected' + b'' + ) + + assert response.status_code == 400 + + +def test_propfind_root_text_is_rejected( + dev_auth_dependency_overrides, +) -> None: + """The element-only propfind grammar must reject non-whitespace root text.""" + + response = _propfind_request( + b'unexpected' + ) + + assert response.status_code == 400 + + +def test_unicode_spaces_are_not_silently_treated_as_xml_whitespace( + dev_auth_dependency_overrides, +) -> None: + """Only XML S characters may separate element-only PROPFIND grammar.""" + + bodies = [ + '\u00a0', + '\u00a0', + '\u00a0' + '', + ] + + for body in bodies: + response = _propfind_request(body.encode("utf-8")) + assert response.status_code == 400 + + +def test_propfind_child_tail_text_is_rejected( + dev_auth_dependency_overrides, +) -> None: + """The element-only propfind grammar must reject non-whitespace child tails.""" + + response = _propfind_request( + b'unexpected' + ) + + assert response.status_code == 400 + + +def test_non_empty_propname_is_rejected_as_invalid_grammar( + dev_auth_dependency_overrides, +) -> None: + """DAV:propname is element-only and must fail before unsupported-mode classification.""" + + response = _propfind_request( + b'unexpected' + b'' + ) + + assert response.status_code == 400 + + +def test_malformed_propfind_xml_is_rejected( + dev_auth_dependency_overrides, +) -> None: + """Malformed XML must not be ignored and converted into a successful discovery.""" + + response = _propfind_request( + b'' + ) + + assert response.status_code == 400 + + +def test_non_propfind_root_is_rejected( + dev_auth_dependency_overrides, +) -> None: + """A well-formed XML body with the wrong root is not a PROPFIND request body.""" + + response = _propfind_request(b'') + + assert response.status_code == 400 + + +def test_allprop_character_content_is_rejected( + dev_auth_dependency_overrides, +) -> None: + """DAV:allprop may carry extensions, but direct character content is invalid.""" + + response = _propfind_request( + b'unexpected' + ) + + assert response.status_code == 400 + + +def test_conflicting_propfind_directives_are_rejected( + dev_auth_dependency_overrides, +) -> None: + """A body cannot ask for both allprop and propname semantics.""" + + response = _propfind_request( + b'' + ) + + assert response.status_code == 400 + + +def test_propfind_external_entity_is_rejected( + dev_auth_dependency_overrides, +) -> None: + """PROPFIND XML parsing must not resolve attacker-controlled external entities.""" + + response = _propfind_request( + b']>' + b'&xxe;' + b'' + ) + + assert response.status_code == 400 + + +def test_propfind_body_work_is_bounded( + dev_auth_dependency_overrides, +) -> None: + """A request body above the Naruon discovery ceiling must fail before XML parsing.""" + + response = _propfind_request(b"x" * 8193) + + assert response.status_code == 413 + + +def test_unrecognized_propfind_extension_element_is_ignored( + dev_auth_dependency_overrides, +) -> None: + """Unexpected command extensions must be processed as if they were absent.""" + + response = _propfind_request( + b'' + b'' + ) + + assert response.status_code == 207 + + +def test_allprop_extension_child_is_ignored( + dev_auth_dependency_overrides, +) -> None: + """RFC extension children do not make DAV:allprop invalid for processing.""" + + response = _propfind_request( + b'' + b'' + ) + + assert response.status_code == 207 + + +def test_allprop_include_order_is_not_semantic( + dev_auth_dependency_overrides, +) -> None: + """Element order does not change recognition of allprop/include semantics.""" + + response = _propfind_request( + b'' + b'' + ) + + assert response.status_code == 501 diff --git a/backend/tests/test_dav_unsupported_write_body.py b/backend/tests/test_dav_unsupported_write_body.py new file mode 100644 index 000000000..439958f2b --- /dev/null +++ b/backend/tests/test_dav_unsupported_write_body.py @@ -0,0 +1,51 @@ +"""Regression coverage for fail-closed DAV writes without body consumption.""" + +import asyncio + +from fastapi import Request + +from api.auth import AuthContext +from api.dav import dav_handler + + +def test_unsupported_dav_put_rejects_without_consuming_request_body() -> None: + """A known-unsupported write must not buffer an attacker-controlled body.""" + receive_calls = 0 + + async def receive() -> dict[str, object]: + nonlocal receive_calls + receive_calls += 1 + return { + "type": "http.request", + "body": b"BEGIN:VCALENDAR\r\nEND:VCALENDAR", + "more_body": False, + } + + request = Request( + { + "type": "http", + "method": "PUT", + "headers": [], + "path": "/dav/user123/projects/file.ics", + "raw_path": b"/dav/user123/projects/file.ics", + }, + receive, + ) + auth_context = AuthContext( + user_id="user123", + organization_id="org-acme", + role="user", + group_ids=[], + workspace_id="ws1", + ) + + response = asyncio.run( + dav_handler( + request=request, + path="user123/projects/file.ics", + auth_context=auth_context, + ) + ) + + assert response.status_code == 501 + assert receive_calls == 0 diff --git a/docs/doctoring/dav-authorization-path-decoding.md b/docs/doctoring/dav-authorization-path-decoding.md new file mode 100644 index 000000000..d125cd797 --- /dev/null +++ b/docs/doctoring/dav-authorization-path-decoding.md @@ -0,0 +1,84 @@ +# DAV authorization-path decoding boundary + +## Decision + +Naruon authorizes DAV paths against the Unicode `path` value supplied by ASGI routing and does not percent-decode that value again. The original request-target bytes, when supplied as ASGI `raw_path`, are used to validate wire-level percent syntax and to determine whether exactly one wire decode would leave another `%HH` escape. Backslashes are normalized to `/` once, and that same canonical path is then used for owner checks, logging, and DAV project routing. + +This separates two representations that must not be conflated: + +- `raw_path`: original request-target path bytes. It is the only representation that can distinguish a literal percent encoded as `%25` from malformed raw `%` syntax or an ambiguous nested encoding such as `%252e` or `%25%32%65`. +- `path`: framework-decoded Unicode path. Authorization, owner extraction and traversal checks consume this representation without another percent-decoding pass. + +The ASGI HTTP specification defines `path` as having percent-encoded and UTF-8 byte sequences decoded into characters. It defines `raw_path` as the original path bytes and notes that `raw_path` is optional. Therefore a second application `unquote()` over a route parameter is not a neutral normalization step: it can create a second interpretation of data that the framework has already decoded. + +RFC 3986 §2.4 warns that implementations must not decode the same string more than once. CWE-174 describes the corresponding weakness as double decoding that can introduce dangerous input after an earlier validation step. Naruon also applies an application-layer request-path ceiling of 8192 raw octets when `raw_path` is available; when an ASGI server omits `raw_path`, the decoded fallback is capped at 8192 characters. The 8192-octet wire ceiling is a Naruon resource policy, not an HTTP maximum, and remains above RFC 9110 §4.1's recommendation that implementations support URI references of at least 8000 octets. + +## Failure lineage + +Issue #1344 identified the prior recursive-unquote loop as the wrong authorization boundary. PR #1645 initially replaced the loop with one explicit `unquote()`, but its helper-only tests did not account for framework decoding. That intermediate state also rejected legitimate encoded-percent data because `%25` became `%` and was then classified as a malformed escape. + +The current repair is intentionally narrower: + +1. `c91988398414db3cd0226749aaa336c268543dbc` adds route-level RED cases for framework-decoded nested traversal, literal encoded percent data, malformed raw escapes, encoded control characters, invalid UTF-8 replacement, single-decode traversal, and a large non-recursive path. +2. `eafcc8d4720e58bb04d3029774e5c31f38a6e1e0` removes application percent-decoding from authorization normalization and validates raw request-target syntax before owner checks. +3. ASGI specifies that `raw_path` may be absent. `b6931bf55a4a3320e103b417ee384c82f12444ac` therefore adds a second RED case: if raw provenance is unavailable and the decoded path still contains `%`, Naruon must not guess whether that percent came from valid encoded data, malformed wire syntax or a nested encoding. +4. `42b5af38f21231aa4cad5f2e0ce0768f81fc1ee4` makes that fallback fail closed while permitting percent-free decoded paths on ASGI servers that omit `raw_path`. +5. `9a294e3bf3f7330f1ab51a7c6878864481f40d9f` adds RED cases for split nested encodings such as `%25%32%65`. A raw-prefix regex that only recognized `%25` followed by literal hex characters could miss these encodings even though one wire decode produces `%2e` or `%5c`. +6. `1df3aa46bce2e7fa28b6301e9e9cc846fd5cef7f` replaces that raw-prefix heuristic with one `unquote_to_bytes(raw_path)` used only for ambiguity classification, then rejects the request if the resulting bytes still contain a valid `%HH` escape. Authorization itself continues to consume the framework-decoded `path` without another decode. +7. CodeRabbit review on predecessor head `55068845c22c195c68e15429072cdb689245021a` identified a still-valid decoded-control gap: UTF-8 percent encodings such as `%C2%80` become C1 control characters in ASGI `path`, while the raw-byte C0/DEL check cannot classify them. +8. `f5fd8a467e6d17b71288ed09ed38a4dc0f1a2157` adds route-level RED cases for `%C2%80` and `%C2%9F`; `9d0d3af2fc9c193fde1c0b3cc635fc6daa9fed3d` rejects every decoded Unicode General Category `Cc` character before authorization or logging. +9. `dcdd606b7d6f426fc1e4e7c3f004c9d829932351` tightens the direct-handler regression so ESC, LF and CR are required to fail before any `DAV Request` log record is emitted, replacing the weaker predecessor assertion that only required escaped logging. +10. Current-head Codex review on `9edf30727433b3a4fafbc4523424175ad4f426dc` found an acceptance-coverage gap rather than a production defect: the fail-closed no-`raw_path` branch had a negative residual-percent test, but the documented positive invariant for a percent-free decoded path was not executable. `f29706fb88528a1cf1f2e23a2f46c4c4a7816e79` adds a direct-handler `OPTIONS` regression with no `raw_path` and a percent-free owner path, requiring HTTP 200 and capability discovery. No production logic changes in that commit. +11. Fresh capability review then found that the same `OPTIONS` response overstated the implemented surface: it advertised WebDAV classes `1, 2, 3`, `calendar-access`, `addressbook`, and methods including LOCK, COPY, MOVE and PROPPATCH while the current slice operationally implements only authenticated `OPTIONS` discovery and `PROPFIND`; provider-backed writeback remains fail-closed. RED `2b55a02c6a4f9e03d9f8cb1a09a7419d832dfd24` makes the mismatch executable. `6a565ed2c4fb3e18a6d9366fac5c2b7d708c1709` removes the unsupported `DAV` compliance advertisement and limits `Allow` to `OPTIONS, PROPFIND`. `0990d0ff215d6c203f221b5e64bd5954c2eaa909` aligns the existing owned OPTIONS regressions, and `25531c0d1dad1ad57cf1211df050c6067af3d2f0` folds the temporary RED-only file into `test_dav_api.py` so the effective owned file set remains narrow. +12. A fresh predecessor audit found that older Draft #1345 still carried two valid DAV invariants that #1645 had not actually succeeded: its canonicalized backslash path was propagated into project routing, and it imposed an explicit bounded path budget. On pre-repair #1645, owner authorization normalized `user123\\projects\\demo`, but `PROPFIND` passed the original backslash form to `_dav_path_segments()`, producing a 404 instead of the same project collection reached by slash syntax. RED `68ce68a952a9cbc027eeda8ff1117322e5209f4b`, cleaned to isolated functional RED `b2eb1fe85b714a49481bd26c06fe62573069e4b5`, restores route-level backslash and bounded-resource acceptance. Causal fix `cff6e61d4c96ce077b21517e1c126baaaa607203` propagates one canonical path through authorization/logging/routing and applies the 8192-octet wire ceiling plus bounded no-`raw_path` fallback. + +The #1345 audit also separated valid predecessor behavior from superseded choices. Its workspace-document organization isolation is already represented more completely in the inherited #1417 ancestry by `_document_organization_filter`, current data-surface/action regressions, and real PostgreSQL legacy-NULL organization tests. Its `DAV: 1` advertisement is intentionally not inherited because the later standards review established that the partial gateway does not yet satisfy WebDAV class-1 MUST requirements. Its choice to leave unsupported methods unregistered is likewise not a required successor invariant: #1645 may register selected method names solely to return explicit 501 responses, provided `Allow` remains limited to methods the target resource actually supports. #1345 must therefore remain a frozen predecessor until this succession is verified on exact-head evidence; it is not a second active DAV source writer. + +The old `0bdaf4fedc001fe43326fa67390f05f83a718238` checks were admitted while #1645 targeted `develop`; they are historical after the PR was retargeted to canonical parent #1417 and do not certify the current `(PR, base ref/SHA, head SHA)` identity. The CodeRabbit `CHANGES_REQUESTED` review on `55068845...` is also predecessor evidence after the C1 RED/fix/test sequence; its finding is retained as repair lineage, not as an unresolved current-head defect. The Codex finding on `9edf307...` is retained as evidence-quality lineage; because later commits change the exact head, predecessor review conclusions are not treated as current-head approval. + +## Invariants + +- Authorization never recursively percent-decodes a framework route value. +- Malformed raw percent triplets fail with HTTP 400 when `raw_path` is available. +- A raw `%25` that decodes to a literal percent is allowed when the first wire decode does not leave a valid `%HH` escape. +- Any raw representation whose first wire decode leaves a valid percent triplet, including contiguous `%252e` and split `%25%32%65`, is rejected as ambiguous before authorization. +- Percent-encoded C0/DEL control characters fail before owner or DAV operation handling. +- Framework-decoded Unicode control characters in General Category `Cc`, including C1 controls produced from UTF-8 percent encodings, fail before authorization or request logging. +- Invalid UTF-8 replacement/surrogate values in the framework-decoded path fail before authorization. +- `.` and `..` segments, including those produced by the framework's single decode and Windows-separator normalization, remain unauthorized. +- Backslash normalization produces one canonical path used consistently for owner checks, request logging, and DAV project routing; authorization must not validate one representation and execute another. +- Raw DAV request paths above 8192 octets fail with HTTP 414 before percent/control scans. When `raw_path` is unavailable, decoded paths above 8192 characters fail with HTTP 414 before fallback interpretation. +- If `raw_path` is unavailable, a residual `%` in the decoded path fails closed because its wire provenance cannot be established. Percent-free decoded paths remain supported and must not be rejected solely because an ASGI server omits `raw_path`. +- The normalization and raw-validation path is linear in input length; there is no recursive or fixed-round decode loop. +- `Allow` advertises only methods operationally supported by the current target resource. A handler that intentionally returns 501 is not advertised as supported. +- The partial DAV gateway does not emit a `DAV` compliance header until the resource actually satisfies the corresponding WebDAV/extension requirements. In particular, current discovery support is not represented as class 1/2/3, CalDAV `calendar-access`, or CardDAV `addressbook` compliance. + +## Capability-discovery decision + +HTTP Semantics defines `Allow` as the methods advertised as supported by the target resource. It is not a roadmap or a list of methods that the router can syntactically receive. RFC 4918 likewise defines the `DAV` response header as a compliance advertisement: class 1 requires all WebDAV MUST requirements; class 2 adds locking requirements; class 3 is also a conformance claim. RFC 4918 requires capabilities such as PROPPATCH, COPY and MOVE for DAV-compliant resources. Naruon's current partial discovery gateway does not implement those contracts and explicitly returns 501 for provider-backed write operations. Advertising them would cause standards-aware clients to select behaviors the resource cannot honor. + +The extension tokens are equally normative, not descriptive labels. RFC 4791 §5.1 states that advertising `calendar-access` in `DAV` indicates support for all MUST-level CalDAV requirements. RFC 6352 §6.1 states that `addressbook` indicates support for all MUST-level requirements and REQUIRED CardDAV features. Naruon does not yet implement those complete contracts, so neither token may be emitted merely because calendar/address data is a future or partial product concern. + +The current decision is therefore fail-closed capability discovery: return HTTP 200 to authenticated `OPTIONS`, advertise `Allow: OPTIONS, PROPFIND`, and omit `DAV`. When full WebDAV/CalDAV/CardDAV behavior is implemented, each compliance token and method must be introduced together with its normative contract, authorization, conditional/write semantics, interoperability fixtures and executable acceptance. Some additional method names remain registered so those requests can receive an explicit 501 response; methods not registered follow the framework's method handling. Neither route registration nor an intentional 501 qualifies a method for `Allow`. + +## Reproducible acceptance + +The primary owned executable acceptance remains `backend/tests/test_dav_api.py`. `backend/tests/test_dav_canonical_path_succession.py` carries the predecessor-succession regressions for route-level canonical backslash propagation and the explicit request-path ceiling. Required evidence includes raw, singly encoded, contiguous nested and split nested traversal cases; encoded-percent data; malformed triplets; slash/backslash variants; encoded C0/DEL and decoded C1 controls; invalid Unicode; direct-handler pre-log rejection of control characters; both negative residual-percent and positive percent-free behavior when `raw_path` is absent; route-level TestClient behavior; accurate OPTIONS capability advertisement; explicit 8192-octet/character boundary behavior; and large-input behavior that remains linear rather than recursively amplified. The current exact branch must run these tests plus Ruff and the repository security/CI gates after every source, test or document change. A predecessor-head pass is not current-head evidence. + +Issue #1344 stays open until current-base exact-head hosted checks, current-head independent review and all valid findings are complete. This document does not claim protected integration, release, deployment, broader DAV writeback support, or a security certification. + +## References + +ASGI Team. (n.d.). *HTTP & WebSocket ASGI message format*. ASGI 3.0 documentation. Retrieved September 10, 2026, from https://asgi.readthedocs.io/en/latest/specs/www.html + +Berners-Lee, T., Fielding, R., & Masinter, L. (2005). *Uniform Resource Identifier (URI): Generic Syntax* (RFC 3986). Internet Engineering Task Force. https://doi.org/10.17487/RFC3986 + +Daboo, C. (2011). *CardDAV: vCard extensions to Web Distributed Authoring and Versioning (WebDAV)* (RFC 6352). Internet Engineering Task Force. https://doi.org/10.17487/RFC6352 + +Daboo, C., Desruisseaux, B., & Dusseault, L. (2007). *Calendaring extensions to WebDAV (CalDAV)* (RFC 4791). Internet Engineering Task Force. https://doi.org/10.17487/RFC4791 + +Dusseault, L. (2007). *HTTP extensions for Web Distributed Authoring and Versioning (WebDAV)* (RFC 4918). Internet Engineering Task Force. https://doi.org/10.17487/RFC4918 + +Fielding, R., Nottingham, M., & Reschke, J. (2022). *HTTP semantics* (RFC 9110). Internet Engineering Task Force. https://doi.org/10.17487/RFC9110 + +The MITRE Corporation. (n.d.). *CWE-174: Double Decoding of the Same Data* (CWE List Version 4.20). Common Weakness Enumeration. Retrieved September 10, 2026, from https://cwe.mitre.org/data/definitions/174.html diff --git a/docs/doctoring/dav-collection-url-canonicalization.md b/docs/doctoring/dav-collection-url-canonicalization.md new file mode 100644 index 000000000..2e16ca234 --- /dev/null +++ b/docs/doctoring/dav-collection-url-canonicalization.md @@ -0,0 +1,30 @@ +# DAV collection URL canonicalization + +## Problem + +Naruon's PROPFIND response marked project folders as `DAV:collection` resources but emitted their `DAV:href` values without a trailing slash. RFC 4918 states that wherever a server produces a URL referring to a collection, it SHOULD include the trailing slash. The mismatch could make WebDAV clients treat the same collection URL in multiple forms and complicate member-URL resolution. + +## Decision + +All Naruon-generated `DAV:href` values that identify collections use the trailing-slash form. This is response canonicalization only: authorization, owner scoping, database lookup, collection membership, and supported PROPFIND semantics are unchanged. + +The root project collection already emitted `/api/dav/{owner}/projects/`. Project-folder collections now emit `/api/dav/{owner}/projects/{folder_uid}/`. + +## Evidence + +- Source-order regression: `ba992e3e49aa2ad9d21fe7af7f727d69faff7b03` requires both addressed and member collection hrefs to use trailing slashes. The predecessor implementation fails the member assertion because it emitted `/projects/demo`. +- Minimal production repair: `a755e4573e833e816b15820535296503d4b04f03` changes only the generated project-folder collection href to `/projects/{folder_uid}/`. +- Acceptance requires exact-head repository CI and an independent post-last-push review; predecessor receipts do not transfer to this source-changing descendant. + +## Traceability + +Code: + +- `backend/api/dav.py::_project_folder_response` +- `backend/tests/test_dav_collection_href_contract.py::test_propfind_emits_trailing_slashes_for_collection_hrefs` + +Standard: + +Dusseault, L. M. (Ed.). (2007). *HTTP extensions for Web Distributed Authoring and Versioning (WebDAV)* (RFC 4918). Internet Engineering Task Force. https://doi.org/10.17487/RFC4918 + +Relevant requirement: RFC 4918 §5.1 states that when a server produces a URL referring to a collection, the trailing-slash form should be used. diff --git a/docs/doctoring/dav-empty-segment-canonicalization.md b/docs/doctoring/dav-empty-segment-canonicalization.md new file mode 100644 index 000000000..d5443118f --- /dev/null +++ b/docs/doctoring/dav-empty-segment-canonicalization.md @@ -0,0 +1,51 @@ +# DAV empty-segment canonicalization + +Observed: 2026-09-11 + +## Problem + +Naruon's DAV owner/routing path is normalized after ASGI decoding. `_dav_path_segments()` historically discarded empty segments, and `_dav_path_owner_user_id()` strips the route-root/trailing slash characters after authorization checks. Distinct request-target paths such as `user123//projects`, `user123/projects//demo`, and `/dav//user123/projects` must not silently collapse onto the canonical single-slash resource. + +RFC 3986 does not define adjacent slash characters as redundant. Section 3.3 defines a path as a sequence of slash-separated segments, permits zero-length segments through `segment = *pchar`, and treats non-dot path segments as opaque to generic URI syntax. Silently dropping an interior empty segment therefore changes URI path structure rather than applying a generic URI normalization rule. + +This does **not** claim that RFC 3986 forbids empty segments. Naruon deliberately applies a stricter authenticated DAV route contract: interior empty segments, repeated trailing separators, and an extra separator immediately after the `/dav/` route prefix are rejected rather than collapsed onto another owner resource. A single leading slash remains valid when `_normalize_dav_authorization_path()` is called with an already decoded absolute path, and a single trailing slash remains valid for collection URLs. + +## RED → repair + +- RED `999d7411b8b5188277ffd899c2257dde36886f61` requires ambiguous interior/repeated empty segments to return HTTP 400 rather than reaching `OPTIONS` successfully. +- RED extension `ce7ddd2c3d6e49909351595d4747cca2594527dc` covers an extra route separator such as `/dav//user123/projects`. +- Initial fix `d2eb3dc44f05e7ebb6915c5477b9c62ba6cfd7ac` rejected every normalized path beginning with `/`. That was too broad: `_normalize_dav_authorization_path()` is also a reusable post-ASGI helper whose established contract accepts one route-root slash. +- Hosted exact-head Application CI `34545435466` on `1a815094991b5c04df955066290a8d33c155c54e` supplied the reality RED: 3 failures / 1955 passes / 2 live-smoke skips. Two failures proved that the helper incorrectly rejected `/alice/docs` and the long `/alice/...` non-recursive path. The third proved that framework-decoded `\..\` traversal was being reclassified from the authorization boundary's 403 to an empty-segment 400. +- Causal repair `2b91bcea871f9151abfaac22702f1c3b1bfc5857` preserves one literal leading slash for the reusable helper, rejects a leading separator created only by backslash normalization, keeps interior/repeated empty-segment rejection, and lets dot-segment traversal continue to the existing owner-authorization fail-closed path. `dav_handler()` separately rejects a captured path beginning with `/`, which is how the registered `/dav/{path:path}` route distinguishes `/dav//...` from its canonical form. + +The repair intentionally retains the existing single-backslash compatibility normalization: one framework-decoded backslash separator between ordinary segments becomes one slash and uses the same canonical authorization path. A backslash-created leading separator remains ambiguous and is rejected. Traversal segments are not relabelled as malformed empty-segment syntax; they continue to the pre-existing 403 authorization rejection. + +## Decision + +Alternatives considered: + +- **Continue dropping empty segments:** rejected because distinct URI paths remain indistinguishable at the application boundary. +- **Reject every normalized leading slash in the helper:** rejected after exact-head CI demonstrated that it violates the helper's established already-decoded absolute-path contract. +- **Redirect ambiguous requests to a canonical path:** rejected for this authenticated DAV gateway because redirect semantics introduce another method/body/authentication boundary without product value. +- **Separate route-capture ambiguity from reusable-path normalization:** selected. The route handler rejects the extra separator represented by a captured leading slash, while the normalization helper preserves one genuine route-root slash and rejects only ambiguous separators that would otherwise change resource identity. + +Risk: clients that relied on duplicate-slash tolerance now receive HTTP 400. That behavior was never an advertised Naruon contract and represented ambiguous resource identity. Canonical single-slash collection URLs and the helper's single absolute-path leading slash remain supported. + +## Acceptance + +The route-level regression exercises `OPTIONS` because it proves the request-target invariant before method-specific DAV handling or database access. Existing unit coverage simultaneously locks the reusable helper's one-leading-slash behavior and the single-decode traversal contract. The same canonical path is then used for owner authorization, logging, and PROPFIND routing. + +The exact-head workflow after `2b91bcea871f9151abfaac22702f1c3b1bfc5857` is separate merge evidence. Predecessor Docker/Bandit successes and the predecessor Application CI failure are not transferred as GREEN receipts. + +## Traceability + +- Production: `backend/api/dav.py::_normalize_dav_authorization_path`, `backend/api/dav.py::dav_handler` +- Regressions: `backend/tests/test_dav_canonical_path_succession.py::test_dav_rejects_ambiguous_empty_path_segments`, `backend/tests/test_dav_api.py::test_normalize_dav_authorization_path_treats_route_path_as_already_decoded`, `backend/tests/test_dav_api.py::test_dav_route_rejects_single_decode_traversal` +- Hosted RED: Application CI `34545435466` on `1a815094991b5c04df955066290a8d33c155c54e` +- Causal repair: `2b91bcea871f9151abfaac22702f1c3b1bfc5857` +- PR: `ContextualWisdomLab/naruon#1645` +- Parent authority: `ContextualWisdomLab/naruon#1417` + +## Reference + +Berners-Lee, T., Fielding, R., & Masinter, L. (2005). *Uniform Resource Identifier (URI): Generic Syntax* (RFC 3986, STD 66). RFC Editor. https://doi.org/10.17487/RFC3986 diff --git a/docs/doctoring/dav-project-collection-depth.md b/docs/doctoring/dav-project-collection-depth.md new file mode 100644 index 000000000..233246ac0 --- /dev/null +++ b/docs/doctoring/dav-project-collection-depth.md @@ -0,0 +1,48 @@ +# DAV project-collection Depth truthfulness + +Observed: 2026-09-11 + +## Problem + +Naruon's `/dav/{owner}/projects/` collection is a server-side registry surface. A direct project URL such as `/dav/{owner}/projects/{folder_uid}/` is emitted as a `DAV:collection`, but Naruon does not yet enumerate the customer-owned provider folder's internal members through this gateway. + +Before this repair, a `PROPFIND` with `Depth: 1` against a direct project collection returned HTTP 207 with only the addressed project collection. That response was indistinguishable from a complete depth-one result for an actually empty collection. It therefore asserted a negative fact about provider membership that Naruon had not queried and could not establish. + +RFC 4918 defines `Depth: 1` as applying a method to the addressed resource and its internal members, and requires a collection PROPFIND multistatus to include a response for each member URL to the requested depth. Returning only the addressed collection is therefore not a truthful implementation of depth-one traversal when internal membership is unknown. + +## Decision + +Keep the two supported discovery layers distinct. + +- `PROPFIND /dav/{owner}/projects/` with `Depth: 1` continues to enumerate the Naruon-owned project registry's direct `ProjectFolder` members, subject to the existing 256-member product ceiling and 257-row overflow probe. +- `PROPFIND /dav/{owner}/projects/{folder_uid}/` with `Depth: 0` continues to return the selected project collection's own properties after server-authoritative owner/organization lookup. +- `PROPFIND /dav/{owner}/projects/{folder_uid}/` with `Depth: 1` now returns HTTP 501 after confirming that the project collection exists. Naruon cannot truthfully enumerate that customer-owned provider collection until a released source/provider contract supplies direct-member discovery. +- Missing direct project collections continue to return 404 rather than being converted into a generic capability error. + +HTTP 501 is used as a product capability boundary, not as a claim that RFC 4918 mandates this status for partial WebDAV servers. Naruon intentionally does not advertise full WebDAV compliance while required class-1 behavior is incomplete. RFC 9110 defines 501 as indicating that the server does not support the functionality required to fulfill a request. + +## Rejected alternatives + +Returning the addressed collection alone with 207 was rejected because it can make an unknown provider collection appear empty. Returning an arbitrary truncated member list was rejected because it would manufacture incomplete membership without signaling loss. Fetching provider contents directly from this route was rejected because provider execution, credentials, source capability, and conflict semantics belong to the signed source/runner boundary rather than this registry reader. + +## Executable lineage + +- Predecessor exact head: `6b2f2befa3533c507c885818bf52d66adc51a470`. +- Reality RED: `3c1ba9207b46243712f229ac1f75b76a6eebf867` adds a route-level contract requiring direct project `Depth: 1` to fail closed instead of reporting a false empty collection. +- Minimal causal fix: `dbcad849194153e1dc19bb570424cca5670339ac` preserves the existing DB existence lookup and returns 501 only after the selected project collection is found. +- Regression alignment: `cf08479d7645c027680600a9758849845d384a79` and `8c794f15a19dcba394d6d691bd0c76271c7fa90f` move existing XML-escaping and encoded-percent property-read probes to `Depth: 0`; those tests never established provider child enumeration and must not encode that unsupported assumption. +- Harness repair and positive acceptance: `f574524f089a3e37b0df35526855c46a0f8bed57` restores the pre-existing finite-depth assertion helper accidentally omitted while introducing the RED test and adds an explicit successful direct-project `Depth: 0` regression. The intermediate omission affected test code only and is not used as acceptance evidence. +- Hosted regression finding: Application CI `34543857634` on `3a89ec5d90dca0c073039ea476ff2f0409409483` completed frontend and backend lint successfully, but the PostgreSQL-backed backend suite ended `1 failed, 1953 passed, 2 skipped`. The sole failure was `test_propfind_propagates_framework_decoded_backslashes_to_project_routing`, whose canonical-path purpose still encoded the now-unsupported direct-project `Depth: 1` behavior and expected 207 rather than the truthful 501 boundary. +- Causal test repair: `dd6ac12f4fb077153ea944d4a741e816064d8a51` changes only that canonical-path regression to `Depth: 0`. This keeps the test focused on framework-decoded backslash normalization and project routing while no longer asserting provider child enumeration that Naruon does not implement. + +The production change is confined to `backend/api/dav.py`. The new depth contract is in `backend/tests/test_dav_depth_contract.py`; existing path-safety probes remain in `backend/tests/test_dav_api.py` and `backend/tests/test_dav_canonical_path_succession.py`. + +## Remaining boundary + +This repair does not implement provider-backed WebDAV member enumeration and does not make Naruon a file-store source of truth. A future implementation must consume a released source/provider contract, preserve signed user/organization/workspace authority, bound result cardinality and payload work, and provide exact-head tests showing that every returned member is authorized and server-authoritative before `Depth: 1` can return a successful direct-project multistatus. + +## References + +Dusseault, L. (Ed.). (2007). *HTTP extensions for Web Distributed Authoring and Versioning (WebDAV)* (RFC 4918). RFC Editor. https://doi.org/10.17487/RFC4918 + +Fielding, R., Nottingham, M., & Reschke, J. (2022). *HTTP semantics* (RFC 9110). RFC Editor. https://doi.org/10.17487/RFC9110 diff --git a/docs/doctoring/dav-propfind-depth.md b/docs/doctoring/dav-propfind-depth.md new file mode 100644 index 000000000..0758678df --- /dev/null +++ b/docs/doctoring/dav-propfind-depth.md @@ -0,0 +1,72 @@ +# DAV PROPFIND finite-depth and request-body boundary + +## Decision + +Naruon's current DAV surface is a bounded collection-discovery gateway, not a full WebDAV-compliant resource. `PROPFIND` therefore accepts only explicit finite `Depth: 0` and `Depth: 1` traversal. It does not silently coerce missing, `infinity`, or malformed `Depth` values to depth one. + +RFC 4918 §9.1 requires a PROPFIND client to submit `Depth: 0`, `Depth: 1`, or `Depth: infinity`, requires WebDAV-compliant resources to support depths zero and one, and permits servers to disable infinite-depth PROPFIND because of performance and security concerns. The same section recommends treating a missing Depth header as `Depth: infinity`. RFC 4918 §9.1.1 permits a server to reject an infinite-depth PROPFIND on a collection with HTTP 403 and recommends the `DAV:propfind-finite-depth` precondition. RFC 4918 §10.2 defines the Depth header grammar as exactly `0`, `1`, or `infinity`; depth one applies the method to the addressed resource and its internal members. RFC 4918 §8.3.1 illustrates the same response shape: a depth-one request to a collection returns one `href` for the collection itself and one for each direct internal member represented in the response. + +Naruon applies that protocol meaning without making a WebDAV compliance claim: + +- `Depth: 0` returns only the addressed collection representation. +- `Depth: 1` returns the addressed collection representation first and then its direct project-folder members when the collection contains at most 256 members. +- Root `Depth: 1` probes at most 257 rows in PostgreSQL. The extra row is only an overflow sentinel; Naruon never emits a silently truncated 256-member success response. +- A root collection with more than 256 direct project-folder members returns HTTP 403 with the Naruon extension precondition `urn:naruon:dav:project-member-limit`. +- `Depth: infinity` returns HTTP 403 with an XML `DAV:propfind-finite-depth` precondition because unbounded recursive traversal is not implemented. +- A missing Depth header follows RFC 4918's recommended infinity interpretation and receives the same finite-depth rejection. Naruon does not reinterpret absence as depth one. +- Values outside the RFC 4918 grammar return HTTP 400 instead of being silently normalized to a supported value. + +The 256-member ceiling is a Naruon product resource policy, not an RFC 4918 limit. RFC 4918 §16 permits WebDAV error bodies to be extended with custom child elements in a namespace other than the reserved `DAV:` namespace and notes that 403 is appropriate when a request should not simply be repeated because the same server-side condition will make it fail again. The numeric status therefore remains meaningful to generic clients while the namespaced precondition lets Naruon-aware clients distinguish the collection-size policy from the RFC-defined infinite-depth refusal. + +### Request-body semantics + +RFC 4918 §9.1 makes the request body semantically significant: `prop` requests selected property values, `propname` requests property names, and `allprop` requests the normal property-value set; an empty body is treated as `allprop`. Section 14.18 defines `prop` as an element-only property container and §14.20 additionally requires a `prop` used inside `propfind` not to contain property values. Section 14.8 gives `include` the same name-only, no-text/mixed-content role for additional properties. Section 14.20 defines `propfind` with the grammar `propname | (allprop, include?) | prop`, and §14.21 defines `propname` as `EMPTY`. A server must not silently choose a different directive or classify invalid property-name syntax as merely unsupported semantics. + +The previous Naruon route ignored every PROPFIND body. A valid `propname` or selected `prop` request therefore returned the same hard-coded property values as an empty request, and malformed XML could still produce a 207 response. That is a request-semantics mismatch even though Naruon does not advertise a `DAV` compliance class. + +The current bounded discovery profile now has an explicit body contract: + +- an empty body and an explicit empty `DAV:allprop` use the existing supported discovery-property profile; +- valid name-only `DAV:prop` and empty `DAV:propname` directives return HTTP 501 until their distinct response semantics are implemented; +- valid empty `allprop` plus a name-only `include` returns HTTP 501 rather than silently dropping the requested additional properties; +- malformed XML, a non-`DAV:propfind` root, non-whitespace text or child tails in element-only `DAV:propfind`, a non-empty `DAV:propname`, a non-empty `allprop` whether standalone or paired with `include`, text/mixed content or property values inside `prop`/`include`, or conflicting/invalid directive structure returns HTTP 400; +- XML is parsed with the repository's existing `defusedxml` dependency, so entity expansion and external-entity constructs fail closed; +- application-level PROPFIND body accumulation is capped at 8192 octets and returns HTTP 413 on overflow. + +The 8192-octet body ceiling is a Naruon product resource policy, not an RFC 4918 value. Streaming the ASGI request body avoids application-level unbounded aggregation; it does not claim that an HTTP server or intermediary never buffers transport chunks before the application receives them. The gateway remains intentionally partial: truthful refusal is preferred to fabricating `prop`, `propname`, or `include` behavior. + +This decision is deliberately consistent with the existing capability-discovery boundary in `dav-authorization-path-decoding.md`: `OPTIONS` advertises `Allow: OPTIONS, PROPFIND` but omits the `DAV` compliance header. Supporting a bounded subset of PROPFIND semantics is not represented as class-1 WebDAV compliance. + +## Failure lineage + +The predecessor helper `_dav_depth()` used `request.headers.get("Depth", "1")`, returned `"0"` only for zero, and mapped every other value to `"1"`. That produced three distinct semantic errors: a missing Depth header became depth one rather than the RFC 4918 infinity default, an explicit `Depth: infinity` was silently reduced to one level, and malformed values such as `Depth: 2` or `Depth: children` were accepted as depth one. + +Reality RED `e0e14695e8365458bb7815c2035c2d3c8c641295` adds route-level acceptance for the bounded policy: missing/infinite depth must return the finite-depth WebDAV precondition and malformed values must return 400. On the predecessor implementation those requests instead succeeded as depth-one PROPFIND responses. + +Causal production fix `377693c9aa0b5a6dc7bb2cbd7f9664773af41ed6` makes the parser preserve the three RFC-defined depth values, treats an absent header as infinity, rejects values outside the grammar, and returns the `DAV:propfind-finite-depth` XML error for unsupported infinite traversal. Compatibility-test children `0dc0db0713222cc4078251476d8085a2f2204005` and `a32d65d246c13c969ce98fc4c892e7c4436e341d` make pre-existing successful PROPFIND regressions state their intended finite `Depth: 1` explicitly instead of depending on the former non-standard default. Test-only child `71b9d043d68e5448a541f1f2b4959fcfab66d847` additionally locks the positive `Depth: 0` invariant: the root collection returns exactly its own representation and does not enumerate the `demo` child. + +A subsequent standards audit found a second interoperability defect inside the now-explicit depth-one path. The root project collection returned only its direct folder members and omitted the addressed collection itself. RFC 4918 depth-one semantics include both the resource and its internal members; omitting the target gives clients an incomplete multistatus view even though the request succeeds. Reality RED `268a41a342e7f730c8a6a4e905d414d797aaeebd` requires the project collection and its direct `demo` member to appear together. Minimal production fix `e9666093217674687ef7f0c62248a5bb637097f1` reuses one addressed-collection representation for both depth zero and depth one, prepending it to the direct-member responses without changing authorization or infinite-depth policy. + +Resource review then found that "bounded" was still false for member cardinality: root depth-one discovery called `get_project_folders_from_db()` without a SQL limit and materialized every matching project folder before serializing every response. Reality RED `facfdcc1dad83bb5f1b9855c823e39fde3c74b2e` requires the DAV root query to request a 257-row overflow probe and requires a 257-member result to fail closed instead of returning a 207 response. Service child `b784ce93591c496e6b5a0d8b36341c3a4a8b4091` adds an optional SQL `LIMIT` capability without changing existing unbounded callers. DAV fix `093e748cbc7ce34360ef3cdef017957909542d23` uses `LIMIT 257`, returns normal depth-one output only for at most 256 members, and returns the namespaced 403 precondition above on overflow. Compatibility child `e28199395aac868b6d287d24aabea7c3e533811a` updates existing DAV stubs to assert the bounded query contract, and `18b1bd70281c12050083b87d3da3e6e0a47a149a` verifies that the service-level `max_results` parameter is actually compiled into SQL rather than being an API-only hint. + +Request-body audit then found a fourth semantics/resource defect: the route never inspected the PROPFIND body, so different RFC-defined requests collapsed to the same successful response and attacker-controlled XML could be arbitrarily large without an application-level discovery-body ceiling. Source-order RED `bad0f67c54a267c3b72ec7ff12f494d4707655f5` requires selected-property and `propname` requests not to masquerade as allprop, malformed/conflicting bodies to fail, and an 8193-octet body to be rejected. Causal fix `56a77d109ee429e7258a77f803a794c1925f9098` adds bounded streaming, `defusedxml` parsing, RFC-grammar validation, and truthful 501 refusal for valid but unsupported property-selection modes. Acceptance child `27b909f09cb4b53000475f44d546769fb23d0e20` adds explicit `allprop/include`, wrong-root, non-empty-allprop, and external-entity cases while preserving empty/explicit-allprop success. + +Hosted security execution on doctoring head `847e51b9e80007f8bd940e9ff65124a487c02a0b` then produced a real Bandit RED: run `34528199522` failed at `Run Bandit Scan`. The parser itself already used `defusedxml`, but production code imported `xml.etree.ElementTree.ParseError` solely for exception typing, crossing the scanner's insecure-XML import boundary. Minimal child `79722fedadcacb3c8daa554d0a0fa6a546fce973` removes that stdlib XML import and catches `DefusedElementTree.ParseError` from the safe parser module. Fresh Bandit run `34528443794` is terminal success on that exact repair; no scanner configuration or workflow gate was weakened. + +Independent review of `79722fed...` found one more grammar bug: directive classification inspected child tags but not non-whitespace `propfind` text, child tails, or `propname` content. As a result `unexpected` could still succeed as allprop and non-empty `propname` could be classified as a valid-but-unsupported mode. Source-order RED `55f8f14c51c0df01552e18a185d27c2de6846870` adds route-level cases for root text, child-tail text, and non-empty `propname`. Minimal fix `1c133f6784b965e1cbe5b145841fa1374d1c2262` enforces the element-only `propfind` grammar and the EMPTY `propname` contract before semantic-mode classification. + +The next exact-range review exposed the same invariant missing from the valid-but-unsupported `allprop + include` branch: standalone `allprop` was checked as EMPTY, but a non-empty `allprop` followed by `include` returned 501 before syntax validation. Source-order RED `cb19cb3af4d890717e15c9c00021a55fa1eef78a` requires that invalid combination to return 400. Minimal fix `a4429f95ca2127a1fbed41e7eafefeadfc06b054` validates the `allprop` element before returning the truthful 501 for otherwise syntactically valid `allprop + include` requests. + +A final syntax-before-semantics audit found the same fail-open classification inside the name-only containers themselves. The unsupported `prop` branch returned 501 even when a named property carried a value, while `allprop + include` returned 501 even if `include` contained mixed text or a property value. RFC 4918 §§14.8, 14.18, and 14.20 define these as property-name containers and prohibit text/mixed content; `prop` inside `propfind` also MUST NOT contain property values. Source-order RED `d6385bd8fddf2e50133527d96ab02ce1fe3553ca` adds route-level invalid-value/mixed-content cases. Minimal fix `fe275cb27b79fb37137a900eecf0c918ca7b7bcf` introduces one name-container grammar validator and runs it before the truthful 501 unsupported-mode refusal for `prop` and `include`. XML extension attributes remain untouched; the repair only rejects content that would turn a property name into text, mixed content, or a value. + +## Invariants and acceptance + +Protocol depth and request-body directive are both part of request semantics and must not be rewritten merely to obtain a successful response. The server may bound work by refusing infinity, an oversized direct-member set, an oversized body, or a valid directive whose semantics are not yet implemented, but it must distinguish refusal from successful traversal/property retrieval. Syntax validation precedes unsupported-mode classification: invalid request grammar is not converted into HTTP 501 merely because the nearest valid semantic mode is not implemented. For a collection, depth zero is self-only and depth one is self plus direct internal members; returning only children or silently truncating members is not an equivalent representation. Authentication and canonical-path authorization still execute before PROPFIND body/depth handling, so invalid ownership or path representations are not disclosed through parser or depth responses. + +Executable acceptance is owned by `backend/tests/test_dav_depth_contract.py` and `backend/tests/test_dav_propfind_body_contract.py`, with compatibility coverage in `backend/tests/test_dav_api.py` and `backend/tests/test_dav_canonical_path_succession.py`. The depth contract covers explicit zero and one, depth-one target-plus-member shape, the 256-member success ceiling and 257-row overflow probe, the RFC-recommended missing-header infinity interpretation, explicit infinity refusal, malformed values, and the WebDAV error preconditions. The body contract covers empty/allprop equivalence within Naruon's supported property profile, truthful refusal of syntactically valid name-only `prop`, empty `propname`, and empty `allprop/include`, malformed/conflicting XML, element-only root/tail enforcement, non-empty `propname`, non-empty `allprop` in both standalone and include forms, invalid property values or mixed text inside `prop`/`include`, external entities, and the 8192-octet application-level ceiling. The exact branch head must run these tests together with the repository's full backend, security, and image-validation gates after every source, test, or documentation change. Earlier GREEN runs are predecessor evidence after any later source, test, or doctoring commit. + +No claim is made that Naruon implements full `allprop`, selected `prop`, `propname`, `include`, infinite-depth traversal, PROPPATCH, COPY, MOVE, locking, CalDAV, CardDAV, or WebDAV class-1 compliance. Those capabilities require their own complete contracts and executable interoperability evidence before advertisement. + +## References + +Dusseault, L. (2007). *HTTP extensions for Web Distributed Authoring and Versioning (WebDAV)* (RFC 4918). Internet Engineering Task Force. https://doi.org/10.17487/RFC4918 \ No newline at end of file diff --git a/docs/doctoring/dav-propfind-xml-extensibility.md b/docs/doctoring/dav-propfind-xml-extensibility.md new file mode 100644 index 000000000..31d524b69 --- /dev/null +++ b/docs/doctoring/dav-propfind-xml-extensibility.md @@ -0,0 +1,37 @@ +# DAV PROPFIND XML extensibility boundary + +## Decision + +Naruon's bounded PROPFIND parser follows RFC 4918 XML extensibility rules while keeping its intentionally narrow implemented DAV semantics. Unexpected XML elements and extension attributes are processed as if absent. Recognized `DAV:propfind` directives retain their syntax-before-semantics validation, and unsupported `propname`, selected `prop`, and `allprop` + `include` semantics continue to fail closed with HTTP 501 rather than being coerced to the supported bounded `allprop` profile. + +RFC 4918 Section 17 requires WebDAV processors to process unexpected elements and attributes, including elements defined for another context, as if they were not present. It also states that extension attributes may be added and that element ordering is irrelevant unless otherwise stated. Consequently, an extension element beside `DAV:allprop`, or inside the `EMPTY` `DAV:allprop`/`DAV:propname` structural element, must not by itself turn a request into HTTP 400. Direct character content remains invalid because element-containing productions cannot be extended with text. + +The `DAV:prop` and `DAV:include` containers are different: their child elements are property names by definition, not ignorable structural extensions. Naruon therefore continues to reject property values, nested child content, and mixed character data in those name-only selectors before returning 501 for the unsupported valid mode. + +`DAV:allprop` + `DAV:include` recognition is order-insensitive. Naruon still returns 501 because include semantics are not implemented; changing XML element order cannot convert the same defined request into malformed input. + +## Failure and repair lineage + +Source-order RED `c0b154d70a62f3f46e98b866dbc62a0d15d7f4ea` adds route-level cases proving that an unexpected command extension beside `DAV:allprop`, an extension child of `DAV:allprop`, and reordered `DAV:include` + `DAV:allprop` must not be rejected solely because of extension placement or ordering. + +The first production edit `8d6815db3c04c4b5aecd24bbbae6ad49f86ce1fd` contained unrelated handler-shape drift while applying the parser change. It is not acceptance evidence. Forward repair `db35735e62551264bb1ba2709fb95eeaa252651a` restores the predecessor handler exactly and confines the effective production delta to XML-extension processing: recognized directive filtering, order-insensitive `allprop`/`include`, and an EMPTY-directive validator that ignores extension children while retaining XML-S-only parent text checks. No force push or destructive rebase was used. + +Test correction `12cdf2914d40956b6306d4559442f8d69b7c69f6` removes an older assertion that treated a child element inside `DAV:allprop` as invalid merely because the DTD calls `allprop` EMPTY. RFC 4918 Section 17 explicitly permits extension elements even for EMPTY element types. The replacement assertion preserves the actual grammar invariant: direct non-whitespace character content is invalid. + +## Acceptance + +The current branch must satisfy the complete DAV test suite, including: + +- unexpected `propfind` extension element + `allprop` returns the supported 207 profile; +- extension children inside `allprop` are ignored for processing; +- `include` + `allprop` is recognized independent of order and returns the truthful unsupported-mode 501; +- direct non-XML-S character content remains 400; +- existing malformed XML, XXE, body-size, Depth, authorization, collection-cardinality, and unsupported-write regressions remain GREEN. + +Repository-hosted exact-head checks and a qualifying independent post-last-push review remain required. Predecessor approvals or workflow runs do not transfer to a source-changing descendant. + +## References + +Dusseault, L. (2007). *HTTP extensions for Web Distributed Authoring and Versioning (WebDAV)* (RFC 4918). Internet Engineering Task Force. https://doi.org/10.17487/RFC4918 + +Bray, T., Paoli, J., Sperberg-McQueen, C. M., Maler, E., & Yergeau, F. (Eds.). (2008). *Extensible Markup Language (XML) 1.0 (Fifth Edition).* World Wide Web Consortium. https://www.w3.org/TR/xml/ diff --git a/docs/doctoring/dav-propfind-xml-whitespace.md b/docs/doctoring/dav-propfind-xml-whitespace.md new file mode 100644 index 000000000..169863735 --- /dev/null +++ b/docs/doctoring/dav-propfind-xml-whitespace.md @@ -0,0 +1,27 @@ +# DAV PROPFIND XML whitespace boundary + +## Decision + +Naruon's bounded PROPFIND parser tolerates formatting whitespace between element-only grammar nodes, but the tolerated set is the XML 1.0 `S` production only: space (`#x20`), tab (`#x9`), carriage return (`#xD`), and line feed (`#xA`). It does not use Python's broader Unicode `str.strip()` classification as a substitute for XML grammar. + +This matters because RFC 4918 defines `DAV:propfind` and its `prop`/`include` property-name containers as XML element-only/name-only syntax. Python considers characters such as U+00A0 NO-BREAK SPACE to be whitespace for `str.strip()`, while XML 1.0 does not include U+00A0 in production `S`. Treating U+00A0 as ignorable formatting would silently convert character data into grammar whitespace and could classify an invalid request as a supported `allprop` or a valid-but-unsupported `prop`/`include` mode. + +The parser therefore uses an explicit XML-space predicate. Normal XML indentation remains accepted; non-XML Unicode spacing characters remain character content and are rejected before semantic-mode classification. + +## Failure lineage + +Source-order RED `e5a19a246af2fe2bf324ab0baf57eb7fd26bcfb6` adds route-level requests containing U+00A0 in three positions that the predecessor's `.strip()` checks treated as empty: `propfind` root text, `allprop` content, and `include` mixed content. Each request must return HTTP 400. + +Minimal production fix `39a3e05bc5ffd62dc6b1096733fb2600b6468e82` replaces broad Unicode-strip truth tests with `_has_non_xml_space_content()`, whose accepted formatting set is exactly XML `S`. The same predicate is used for root text, directive tails, `allprop`, `propname`, and the `prop`/`include` property-name-container validator so syntax-before-semantics does not diverge by branch. + +This is a grammar correction, not a Unicode-path policy. DAV path canonicalization and C0/C1 rejection remain owned by the existing authorization-path contract. No request-body size, Depth, collection-cardinality, authorization, provider-writeback, or dependency behavior changes here. + +## Acceptance + +`backend/tests/test_dav_propfind_body_contract.py::test_unicode_spaces_are_not_silently_treated_as_xml_whitespace` is the executable regression. Exact-head repository CI and independent review are still required after the source/test/doc sequence; predecessor GREEN or approval does not transfer. + +## References + +Bray, T., Paoli, J., Sperberg-McQueen, C. M., Maler, E., & Yergeau, F. (Eds.). (2008). *Extensible Markup Language (XML) 1.0 (Fifth Edition).* World Wide Web Consortium. https://www.w3.org/TR/xml/ + +Dusseault, L. (2007). *HTTP extensions for Web Distributed Authoring and Versioning (WebDAV)* (RFC 4918). Internet Engineering Task Force. https://doi.org/10.17487/RFC4918 \ No newline at end of file diff --git a/docs/doctoring/dav-unsupported-write-body.md b/docs/doctoring/dav-unsupported-write-body.md new file mode 100644 index 000000000..b1ef500b0 --- /dev/null +++ b/docs/doctoring/dav-unsupported-write-body.md @@ -0,0 +1,33 @@ +# DAV unsupported-write request-body boundary + +## Decision + +Naruon currently does not implement provider-backed DAV write semantics. A `PUT` request that has passed authentication and DAV path validation therefore fails closed with HTTP 501 without consuming or buffering the request body. + +The prior implementation called `await request.body()` before returning the already-determined 501 result. That body had no accepted domain meaning: it was not parsed, validated, persisted, forwarded, or used to decide the response. Buffering it nevertheless made memory and receive-path work scale with attacker-controlled request content before an unconditional rejection. + +RFC 9110 defines `PUT` in terms of replacing the target resource with request content, while also stating that methods not implemented by the origin server should receive 501. Because this DAV slice does not implement the write operation, reading application content cannot complete any supported PUT semantics. The resource-safe boundary is therefore to reject after authentication/path validation and before application-level body consumption. Transport framing remains the HTTP server's responsibility; this decision does not claim that the network stack receives zero octets. + +## RED to fix + +- `fe678f6b4b52eecdfee97f1c3852f601cb53eec5` adds `backend/tests/test_dav_unsupported_write_body.py`. A direct ASGI request supplies a receive callback that records every body read. On the predecessor implementation, `dav_handler()` calls `request.body()`, so the regression is RED because `receive_calls` becomes nonzero. +- `90ba3d984863db8464b7b15fd5a216b0558b493f` is the minimal causal production fix. It removes the unused PUT body buffering and its byte-count log while preserving authentication, path validation, the existing warning, response text, and HTTP 501 contract. + +## Invariants + +- Unsupported provider-backed DAV `PUT` remains authenticated and path-validated before rejection. +- The application does not call `Request.body()`, iterate the request stream, parse content, or allocate a body-sized buffer for a PUT that is unconditionally rejected as not implemented. +- The response remains HTTP 501 until a real write aggregate, conditional semantics, authorization contract, persistence/provider adapter, and executable acceptance are implemented. +- A future write implementation must introduce an explicit content-size policy and streaming strategy rather than silently restoring unbounded buffering. + +## Reproducible acceptance + +`backend/tests/test_dav_unsupported_write_body.py` is the focused regression. It must return 501 while the custom ASGI receive callback remains uncalled. The existing DAV suite remains responsible for authentication, owner scoping, canonical path handling, capability discovery, traversal/encoding rejection, and provider-backed write fail closure. + +This evidence is application-level. It does not prove that an HTTP server or intermediary never reads request bytes, and it is not a generic denial-of-service certification. + +## References + +Fielding, R., Nottingham, M., & Reschke, J. (2022). *HTTP semantics* (RFC 9110). Internet Engineering Task Force. https://doi.org/10.17487/RFC9110 + +Thomson, M., & Nottingham, M. (2022). *HTTP/1.1* (RFC 9112). Internet Engineering Task Force. https://doi.org/10.17487/RFC9112