fix(webdav): address PR #764 review round 4
- Guard against malformed PROPFIND responses where tag["id"] is None before calling get_files_by_tag (prevents <oc:systemtag>None</oc:systemtag> dispatch). - Add OCS-APIRequest: true header to get_tag_by_name and get_files_by_tag to match every other PROPFIND/REPORT in the file — fixes a latent reverse-proxy compatibility hazard. - Add test_copy_resource_blocks_excluded_source to mirror the existing move-source coverage; closes the asymmetric test gap. - Add test_skips_tag_with_missing_id covering the new fail-open branch in _resolve_one_tag. - Reword _resolve_one_tag docstring: "distinct slot" was misleading (tasks append rather than pre-allocate). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
2ee4d03e3f
commit
56f01b3499
@@ -1135,7 +1135,11 @@ class WebDAVClient(BaseNextcloudClient):
|
|||||||
response = await self._make_request(
|
response = await self._make_request(
|
||||||
"PROPFIND",
|
"PROPFIND",
|
||||||
"/remote.php/dav/systemtags/",
|
"/remote.php/dav/systemtags/",
|
||||||
headers={"Depth": "1", "Content-Type": "text/xml"},
|
headers={
|
||||||
|
"Depth": "1",
|
||||||
|
"Content-Type": "text/xml",
|
||||||
|
"OCS-APIRequest": "true",
|
||||||
|
},
|
||||||
content=propfind_body,
|
content=propfind_body,
|
||||||
)
|
)
|
||||||
# Redundant after _make_request (which raises on non-2xx) but
|
# Redundant after _make_request (which raises on non-2xx) but
|
||||||
@@ -1223,7 +1227,7 @@ class WebDAVClient(BaseNextcloudClient):
|
|||||||
response = await self._make_request(
|
response = await self._make_request(
|
||||||
"REPORT",
|
"REPORT",
|
||||||
f"{self._get_webdav_base_path()}/",
|
f"{self._get_webdav_base_path()}/",
|
||||||
headers={"Content-Type": "text/xml"},
|
headers={"Content-Type": "text/xml", "OCS-APIRequest": "true"},
|
||||||
content=report_body,
|
content=report_body,
|
||||||
)
|
)
|
||||||
# Redundant after _make_request (which raises on non-2xx) but
|
# Redundant after _make_request (which raises on non-2xx) but
|
||||||
|
|||||||
@@ -46,11 +46,11 @@ async def _resolve_one_tag(
|
|||||||
) -> None:
|
) -> None:
|
||||||
"""Resolve a single tag's paths and append them as a set to *results*.
|
"""Resolve a single tag's paths and append them as a set to *results*.
|
||||||
|
|
||||||
Each task writes to a distinct slot in the shared list — append from
|
Each task appends its own set to the shared list; ``list.append`` is
|
||||||
cooperative tasks is safe under anyio (single-threaded between
|
atomic between cooperative yields under anyio (single-threaded
|
||||||
awaits) without an explicit lock. Swallows its own exceptions so a
|
between awaits) so no explicit lock is needed. Swallows its own
|
||||||
failure for one tag does not abort the surrounding task group
|
exceptions so a failure for one tag does not abort the surrounding
|
||||||
(preserves fail-open per-tag semantics).
|
task group (preserves fail-open per-tag semantics).
|
||||||
"""
|
"""
|
||||||
try:
|
try:
|
||||||
tag = await webdav.get_tag_by_name(tag_name)
|
tag = await webdav.get_tag_by_name(tag_name)
|
||||||
@@ -67,6 +67,15 @@ async def _resolve_one_tag(
|
|||||||
logger.debug("Excluded tag %r does not exist — skipping", tag_name)
|
logger.debug("Excluded tag %r does not exist — skipping", tag_name)
|
||||||
return
|
return
|
||||||
|
|
||||||
|
if tag.get("id") is None:
|
||||||
|
# Malformed PROPFIND response: <oc:systemtag> entry without
|
||||||
|
# <oc:id/>. Skip rather than dispatch <oc:systemtag>None</oc:systemtag>.
|
||||||
|
logger.debug(
|
||||||
|
"Excluded tag %r has no id in PROPFIND response — skipping",
|
||||||
|
tag_name,
|
||||||
|
)
|
||||||
|
return
|
||||||
|
|
||||||
try:
|
try:
|
||||||
files = await webdav.get_files_by_tag(tag["id"])
|
files = await webdav.get_files_by_tag(tag["id"])
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
|
|||||||
@@ -127,6 +127,32 @@ class TestGetExcludedFilePaths:
|
|||||||
webdav.get_tag_by_name.assert_awaited_once_with("does-not-exist")
|
webdav.get_tag_by_name.assert_awaited_once_with("does-not-exist")
|
||||||
webdav.get_files_by_tag.assert_not_called()
|
webdav.get_files_by_tag.assert_not_called()
|
||||||
|
|
||||||
|
@pytest.mark.unit
|
||||||
|
async def test_skips_tag_with_missing_id(self, mocker):
|
||||||
|
"""If get_tag_by_name returns a dict with id=None (malformed
|
||||||
|
PROPFIND response — <oc:systemtag> entry without <oc:id/>), skip
|
||||||
|
the tag rather than dispatching <oc:systemtag>None</oc:systemtag>
|
||||||
|
to get_files_by_tag (PR #764 review round 4)."""
|
||||||
|
mocker.patch(
|
||||||
|
"nextcloud_mcp_server.server.tag_exclusion.get_excluded_tag_names",
|
||||||
|
return_value=["malformed"],
|
||||||
|
)
|
||||||
|
webdav = AsyncMock()
|
||||||
|
webdav.get_tag_by_name = AsyncMock(
|
||||||
|
return_value={
|
||||||
|
"id": None,
|
||||||
|
"name": "malformed",
|
||||||
|
"userVisible": True,
|
||||||
|
"userAssignable": True,
|
||||||
|
}
|
||||||
|
)
|
||||||
|
|
||||||
|
result = await get_excluded_file_paths(webdav)
|
||||||
|
|
||||||
|
assert result == set()
|
||||||
|
webdav.get_tag_by_name.assert_awaited_once_with("malformed")
|
||||||
|
webdav.get_files_by_tag.assert_not_called()
|
||||||
|
|
||||||
@pytest.mark.unit
|
@pytest.mark.unit
|
||||||
async def test_fail_open_when_tag_lookup_raises(self, mocker, caplog):
|
async def test_fail_open_when_tag_lookup_raises(self, mocker, caplog):
|
||||||
"""If get_tag_by_name raises (e.g. 5xx from systemtags endpoint),
|
"""If get_tag_by_name raises (e.g. 5xx from systemtags endpoint),
|
||||||
|
|||||||
@@ -194,6 +194,23 @@ async def test_move_resource_blocks_excluded_destination_exact_match(
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
async def test_copy_resource_blocks_excluded_source(
|
||||||
|
webdav_tools, fake_client, patch_get_client, patch_excluded
|
||||||
|
):
|
||||||
|
patch_get_client(fake_client)
|
||||||
|
patch_excluded({"Secret.txt"})
|
||||||
|
|
||||||
|
fn = webdav_tools["nc_webdav_copy_resource"].fn
|
||||||
|
with pytest.raises(ToolError, match="source"):
|
||||||
|
await fn(
|
||||||
|
source_path="/Secret.txt",
|
||||||
|
destination_path="/Public/copy.txt",
|
||||||
|
ctx=_mock_ctx(fake_client),
|
||||||
|
)
|
||||||
|
|
||||||
|
fake_client.webdav.copy_resource.assert_not_called()
|
||||||
|
|
||||||
|
|
||||||
async def test_copy_resource_blocks_excluded_destination_descendant(
|
async def test_copy_resource_blocks_excluded_destination_descendant(
|
||||||
webdav_tools, fake_client, patch_get_client, patch_excluded
|
webdav_tools, fake_client, patch_get_client, patch_excluded
|
||||||
):
|
):
|
||||||
|
|||||||
Reference in New Issue
Block a user