admin/mcp-nextcloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(webdav): address PR #764 review round 3

Browse Source 
Three important issues raised by the latest review on the
get_tag_by_name and get_files_by_tag methods:

1. Add explicit response.raise_for_status() after _make_request in both
   methods. _make_request already raises HTTPStatusError on non-2xx so
   the calls are redundant in practice, but keeping them visible at the
   call site makes the contract self-documenting and prevents a future
   refactor from silently feeding an error body into ET.fromstring.

2. Replace href_path.replace(webdav_prefix, "/") with a startswith +
   slice. str.replace strips every occurrence of the prefix; while no
   real Nextcloud path embeds the prefix mid-string, the fix removes
   the theoretical exposure and matches the pattern used elsewhere in
   the file.

3. Add Content-Type: text/xml to the systemtags PROPFIND headers.
   Other PROPFIND-with-body calls in this file (list_directory line
   240, list_attachments line 1041) include it; the systemtags PROPFIND
   was the only outlier. Same header added to the systemtag REPORT for
   symmetry.

No test changes — the existing get_files_by_tag mock test continues to
pass (the mock response yields valid XML so raise_for_status is a
no-op, and the user-relative path comparison is unaffected by the
prefix-strip swap on a non-adversarial path).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-05-06 19:53:07 +02:00
co-authored by Claude Opus 4.7
parent 35abfb2e3a
commit 2ee4d03e3f
1 changed files with 19 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
nextcloud_mcp_server/client/webdav.py
+19 -4
View File
@@ -1135,9 +1135,14 @@ class WebDAVClient(BaseNextcloudClient):
response = await self._make_request(
"PROPFIND",
"/remote.php/dav/systemtags/",
headers={"Depth": "1"},
headers={"Depth": "1", "Content-Type": "text/xml"},
content=propfind_body,
)
# Redundant after _make_request (which raises on non-2xx) but
# makes the contract explicit at the call site so a future
# refactor of _make_request cannot silently feed an error body
# into ET.fromstring below.
response.raise_for_status()
# Parse XML response
root = ET.fromstring(response.content)
@@ -1218,8 +1223,13 @@ class WebDAVClient(BaseNextcloudClient):
response = await self._make_request(
"REPORT",
f"{self._get_webdav_base_path()}/",
headers={"Content-Type": "text/xml"},
content=report_body,
)
# Redundant after _make_request (which raises on non-2xx) but
# makes the contract explicit at the call site — see the same
# rationale in get_tag_by_name.
response.raise_for_status()
# Parse XML response
root = ET.fromstring(response.content)
@@ -1261,11 +1271,16 @@ class WebDAVClient(BaseNextcloudClient):
and resourcetype_elem.find("d:collection", ns) is not None
)
# Decode href path and extract the file path
# Decode href path and extract the user-relative file path.
# str.replace() would strip every occurrence of the prefix,
# so an adversarially-named directory could collide; strip
# only the leading occurrence via startswith + slice.
href_path = unquote(href_elem.text)
# Remove WebDAV prefix to get user-relative path
webdav_prefix = f"/remote.php/dav/files/{self.username}/"
file_path = href_path.replace(webdav_prefix, "/")
if href_path.startswith(webdav_prefix):
file_path = "/" + href_path[len(webdav_prefix) :]
else:
file_path = href_path
# Parse last modified timestamp
last_modified_timestamp = None