fix(webdav): harden offset/key truthiness and escape SEARCH mime type
Optional review hardening on #849 (non-blocking nits from the approve): - `_build_search_xml`: emit `<d:firstresult>` on `offset is not None` rather than truthiness, so a future explicit offset=0 isn't silently dropped. - `_key`: key on `file_id is not None` so a (hypothetical) file_id of 0 isn't treated as absent and mis-keyed onto path. - `_type_search_args`: XML-escape the MIME type before interpolating it into the SEARCH literal (defense-in-depth for any future user-supplied value), with a unit test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
eaa898e6eb
commit
9452057570
@@ -6,6 +6,7 @@ import xml.etree.ElementTree as ET
|
|||||||
from email.utils import parsedate_to_datetime
|
from email.utils import parsedate_to_datetime
|
||||||
from typing import Any, Dict, List, Optional, Tuple
|
from typing import Any, Dict, List, Optional, Tuple
|
||||||
from urllib.parse import unquote
|
from urllib.parse import unquote
|
||||||
|
from xml.sax.saxutils import escape as xml_escape
|
||||||
|
|
||||||
from httpx import HTTPStatusError
|
from httpx import HTTPStatusError
|
||||||
|
|
||||||
@@ -759,7 +760,12 @@ class WebDAVClient(BaseNextcloudClient):
|
|||||||
# producer omits fileid. ``id(item)`` is a last resort so an item
|
# producer omits fileid. ``id(item)`` is a last resort so an item
|
||||||
# missing both never collapses into another under a shared ``None``
|
# missing both never collapses into another under a shared ``None``
|
||||||
# key (which would silently drop rows from the result set).
|
# key (which would silently drop rows from the result set).
|
||||||
return item.get("file_id") or item.get("path") or id(item)
|
# ``is not None`` rather than truthiness so a (hypothetical)
|
||||||
|
# file_id of 0 isn't treated as absent.
|
||||||
|
file_id = item.get("file_id")
|
||||||
|
if file_id is not None:
|
||||||
|
return file_id
|
||||||
|
return item.get("path") or id(item)
|
||||||
|
|
||||||
results: List[Dict[str, Any]] = []
|
results: List[Dict[str, Any]] = []
|
||||||
seen: set[Any] = set()
|
seen: set[Any] = set()
|
||||||
@@ -894,7 +900,9 @@ class WebDAVClient(BaseNextcloudClient):
|
|||||||
limit_parts = []
|
limit_parts = []
|
||||||
if limit:
|
if limit:
|
||||||
limit_parts.append(f"<d:nresults>{limit}</d:nresults>")
|
limit_parts.append(f"<d:nresults>{limit}</d:nresults>")
|
||||||
if offset:
|
# ``is not None`` (not truthiness) so a future explicit offset=0 is
|
||||||
|
# emitted rather than silently dropped.
|
||||||
|
if offset is not None:
|
||||||
limit_parts.append(f"<d:firstresult>{offset}</d:firstresult>")
|
limit_parts.append(f"<d:firstresult>{offset}</d:firstresult>")
|
||||||
limit_xml = f"<d:limit>{''.join(limit_parts)}</d:limit>" if limit_parts else ""
|
limit_xml = f"<d:limit>{''.join(limit_parts)}</d:limit>" if limit_parts else ""
|
||||||
|
|
||||||
@@ -1160,12 +1168,15 @@ class WebDAVClient(BaseNextcloudClient):
|
|||||||
@staticmethod
|
@staticmethod
|
||||||
def _type_search_args(mime_type: str) -> Tuple[str, List[str]]:
|
def _type_search_args(mime_type: str) -> Tuple[str, List[str]]:
|
||||||
"""Build the where-clause + property list for a MIME-type SEARCH."""
|
"""Build the where-clause + property list for a MIME-type SEARCH."""
|
||||||
|
# Escape so a caller-supplied MIME type can't break the SEARCH XML or
|
||||||
|
# inject elements. All current callers pass literal strings, but this
|
||||||
|
# keeps the boundary safe for any future user-supplied value.
|
||||||
where_conditions = f"""
|
where_conditions = f"""
|
||||||
<d:like>
|
<d:like>
|
||||||
<d:prop>
|
<d:prop>
|
||||||
<d:getcontenttype/>
|
<d:getcontenttype/>
|
||||||
</d:prop>
|
</d:prop>
|
||||||
<d:literal>{mime_type}</d:literal>
|
<d:literal>{xml_escape(mime_type)}</d:literal>
|
||||||
</d:like>
|
</d:like>
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
|||||||
@@ -164,6 +164,17 @@ async def test_find_all_by_type_delegates_to_search_files_all(mocker):
|
|||||||
assert "application/pdf" in kwargs["where_conditions"]
|
assert "application/pdf" in kwargs["where_conditions"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_type_search_args_escapes_mime_type(mocker):
|
||||||
|
"""A MIME value with XML metacharacters must not break / inject into the SEARCH."""
|
||||||
|
client = _make_client(mocker)
|
||||||
|
where, properties = client._type_search_args("application/pdf<&>")
|
||||||
|
# The injected metacharacters are escaped inside the <d:literal>, so they
|
||||||
|
# can't break the SEARCH XML or introduce new elements.
|
||||||
|
assert "pdf<&>" in where
|
||||||
|
assert "pdf<&>" not in where
|
||||||
|
assert "fileid" in properties
|
||||||
|
|
||||||
|
|
||||||
def test_build_search_xml_emits_offset_only_when_set(mocker):
|
def test_build_search_xml_emits_offset_only_when_set(mocker):
|
||||||
client = _make_client(mocker)
|
client = _make_client(mocker)
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user