From 2ee4d03e3f5799ef7b85ca0ada8376e0de2992aa Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 6 May 2026 19:53:07 +0200 Subject: [PATCH] fix(webdav): address PR #764 review round 3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- nextcloud_mcp_server/client/webdav.py | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/nextcloud_mcp_server/client/webdav.py b/nextcloud_mcp_server/client/webdav.py index 6911c1bc..2e1a11ed 100644 --- a/nextcloud_mcp_server/client/webdav.py +++ b/nextcloud_mcp_server/client/webdav.py @@ -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