diff --git a/nextcloud_mcp_server/client/webdav.py b/nextcloud_mcp_server/client/webdav.py index f72baf44..6911c1bc 100644 --- a/nextcloud_mcp_server/client/webdav.py +++ b/nextcloud_mcp_server/client/webdav.py @@ -1132,13 +1132,12 @@ class WebDAVClient(BaseNextcloudClient): """ - response = await self._client.request( + response = await self._make_request( "PROPFIND", "/remote.php/dav/systemtags/", headers={"Depth": "1"}, content=propfind_body, ) - response.raise_for_status() # Parse XML response root = ET.fromstring(response.content) @@ -1216,12 +1215,11 @@ class WebDAVClient(BaseNextcloudClient): """ - response = await self._client.request( + response = await self._make_request( "REPORT", f"{self._get_webdav_base_path()}/", content=report_body, ) - response.raise_for_status() # Parse XML response root = ET.fromstring(response.content) diff --git a/nextcloud_mcp_server/server/tag_exclusion.py b/nextcloud_mcp_server/server/tag_exclusion.py index ad5323e5..5abb55bc 100644 --- a/nextcloud_mcp_server/server/tag_exclusion.py +++ b/nextcloud_mcp_server/server/tag_exclusion.py @@ -23,6 +23,8 @@ is created with ``user_assignable=false``. import logging +import anyio + from nextcloud_mcp_server.client.webdav import WebDAVClient from nextcloud_mcp_server.config import get_settings @@ -37,12 +39,65 @@ def get_excluded_tag_names() -> list[str]: return [t.strip() for t in raw.split(",") if t.strip()] +async def _resolve_one_tag( + tag_name: str, + webdav: WebDAVClient, + excluded: set[str], + lock: anyio.Lock, +) -> None: + """Resolve a single tag's paths and merge into *excluded* under *lock*. + + Swallows its own exceptions so a failure for one tag does not abort + the surrounding task group (preserves fail-open per-tag semantics). + """ + try: + tag = await webdav.get_tag_by_name(tag_name) + except Exception as e: + logger.warning( + "Tag exclusion lookup failed for tag %r (%s); " + "skipping — files tagged with this tag will be visible", + tag_name, + e, + ) + return + + if tag is None: + logger.debug("Excluded tag %r does not exist — skipping", tag_name) + return + + try: + files = await webdav.get_files_by_tag(tag["id"]) + except Exception as e: + logger.warning( + "Tag exclusion file enumeration failed for tag %r (%s); " + "skipping — files tagged with this tag will be visible", + tag_name, + e, + ) + return + + async with lock: + for f in files: + path = _normalise_path(f["path"]) + excluded.add(path) + if f.get("is_directory"): + logger.debug( + "Excluding directory %r (tag %r) — descendants will be hidden", + path, + tag_name, + ) + + async def get_excluded_file_paths(webdav: WebDAVClient) -> set[str]: """Resolve excluded tags to the set of paths they cover. Tagged directories are added as their own normalised path; descendants are blocked via prefix match in :func:`is_path_excluded`. + Per-tag resolution is fanned out via ``anyio.create_task_group`` so + that the 2N network calls (1 PROPFIND + 1 REPORT per tag) run + concurrently rather than serially. + **Failure mode is fail-open per tag**: if the systemtags endpoint is unreachable or returns an error for a given tag, that tag is skipped with a warning rather than propagating the error. Reasoning: the @@ -56,42 +111,10 @@ async def get_excluded_file_paths(webdav: WebDAVClient) -> set[str]: return set() excluded: set[str] = set() - for tag_name in tag_names: - try: - tag = await webdav.get_tag_by_name(tag_name) - except Exception as e: - logger.warning( - "Tag exclusion lookup failed for tag %r (%s); " - "skipping — files tagged with this tag will be visible", - tag_name, - e, - ) - continue - - if tag is None: - logger.debug("Excluded tag %r does not exist — skipping", tag_name) - continue - - try: - files = await webdav.get_files_by_tag(tag["id"]) - except Exception as e: - logger.warning( - "Tag exclusion file enumeration failed for tag %r (%s); " - "skipping — files tagged with this tag will be visible", - tag_name, - e, - ) - continue - - for f in files: - path = _normalise_path(f["path"]) - excluded.add(path) - if f.get("is_directory"): - logger.debug( - "Excluding directory %r (tag %r) — descendants will be hidden", - path, - tag_name, - ) + lock = anyio.Lock() + async with anyio.create_task_group() as tg: + for tag_name in tag_names: + tg.start_soon(_resolve_one_tag, tag_name, webdav, excluded, lock) if excluded: # `len(excluded)` counts directly-tagged entries — descendants of diff --git a/nextcloud_mcp_server/server/webdav.py b/nextcloud_mcp_server/server/webdav.py index 7220613b..39ce775b 100644 --- a/nextcloud_mcp_server/server/webdav.py +++ b/nextcloud_mcp_server/server/webdav.py @@ -392,6 +392,13 @@ def configure_webdav_tools(mcp: FastMCP): """ client = await get_client(ctx) + # Resolve once and use for both the scope guard and the result filter. + excluded = await get_excluded_file_paths(client.webdav) + if scope and is_path_excluded(scope, excluded): + raise ToolError( + f"Access denied: scope {scope!r} is tagged with an excluded tag" + ) + # Build where conditions based on filters conditions = [] @@ -462,8 +469,7 @@ def configure_webdav_tools(mcp: FastMCP): limit=limit, ) - # Filter out tagged-excluded paths. - excluded = await get_excluded_file_paths(client.webdav) + # Filter out tagged-excluded paths from the result set. if excluded: results = [ r for r in results if not is_path_excluded(r.get("path", ""), excluded) @@ -511,10 +517,14 @@ def configure_webdav_tools(mcp: FastMCP): SearchFilesResponse with list of matching files """ client = await get_client(ctx) + excluded = await get_excluded_file_paths(client.webdav) + if scope and is_path_excluded(scope, excluded): + raise ToolError( + f"Access denied: scope {scope!r} is tagged with an excluded tag" + ) results = await client.webdav.find_by_name( pattern=pattern, scope=scope, limit=limit ) - excluded = await get_excluded_file_paths(client.webdav) if excluded: results = [ r for r in results if not is_path_excluded(r.get("path", ""), excluded) @@ -550,10 +560,14 @@ def configure_webdav_tools(mcp: FastMCP): SearchFilesResponse with list of matching files """ client = await get_client(ctx) + excluded = await get_excluded_file_paths(client.webdav) + if scope and is_path_excluded(scope, excluded): + raise ToolError( + f"Access denied: scope {scope!r} is tagged with an excluded tag" + ) results = await client.webdav.find_by_type( mime_type=mime_type, scope=scope, limit=limit ) - excluded = await get_excluded_file_paths(client.webdav) if excluded: results = [ r for r in results if not is_path_excluded(r.get("path", ""), excluded) @@ -588,8 +602,12 @@ def configure_webdav_tools(mcp: FastMCP): SearchFilesResponse with list of favorite files """ client = await get_client(ctx) - results = await client.webdav.list_favorites(scope=scope, limit=limit) excluded = await get_excluded_file_paths(client.webdav) + if scope and is_path_excluded(scope, excluded): + raise ToolError( + f"Access denied: scope {scope!r} is tagged with an excluded tag" + ) + results = await client.webdav.list_favorites(scope=scope, limit=limit) if excluded: results = [ r for r in results if not is_path_excluded(r.get("path", ""), excluded) diff --git a/tests/unit/test_tag_exclusion.py b/tests/unit/test_tag_exclusion.py index fc591480..9e071ded 100644 --- a/tests/unit/test_tag_exclusion.py +++ b/tests/unit/test_tag_exclusion.py @@ -10,6 +10,7 @@ import logging import os from unittest.mock import AsyncMock, patch +import anyio import pytest from nextcloud_mcp_server.config import _reload_config @@ -218,3 +219,82 @@ class TestGetExcludedFilePaths: assert result == {"Secret.txt", "Private", "Other/notes.md"} assert webdav.get_files_by_tag.await_count == 2 + + @pytest.mark.unit + async def test_resolves_tags_concurrently(self, mocker): + """Per-tag resolution must run under a task group so the 2N + network calls overlap (PR review #764). The barrier event below + only completes if all three tags' ``get_tag_by_name`` invocations + are in flight simultaneously — sequential resolution would block + the first task forever and trip ``fail_after``. + """ + tags = ["a", "b", "c"] + mocker.patch( + "nextcloud_mcp_server.server.tag_exclusion.get_excluded_tag_names", + return_value=tags, + ) + + all_started = anyio.Event() + started_count = 0 + + async def fake_get_tag(tag_name: str): + nonlocal started_count + started_count += 1 + if started_count == len(tags): + all_started.set() + with anyio.fail_after(2.0): + await all_started.wait() + return {"id": ord(tag_name), "name": tag_name} + + async def fake_get_files(tag_id: int): + return [{"path": f"/tag-{tag_id}.txt", "is_directory": False}] + + webdav = AsyncMock() + webdav.get_tag_by_name = AsyncMock(side_effect=fake_get_tag) + webdav.get_files_by_tag = AsyncMock(side_effect=fake_get_files) + + with anyio.fail_after(5.0): + result = await get_excluded_file_paths(webdav) + + assert result == {f"tag-{ord(t)}.txt" for t in tags} + + @pytest.mark.unit + async def test_fail_open_under_task_group(self, mocker, caplog): + """One tag failing must not abort sibling tasks in the task group + (PR review #764). Uses callable side_effects keyed by tag name so + the assertion is independent of the order in which the task group + schedules the per-tag coroutines. + """ + mocker.patch( + "nextcloud_mcp_server.server.tag_exclusion.get_excluded_tag_names", + return_value=["good-1", "broken", "good-2"], + ) + + async def fake_get_tag(tag_name: str): + if tag_name == "broken": + raise RuntimeError("upstream 503") + return {"id": hash(tag_name) & 0xFFFF, "name": tag_name} + + async def fake_get_files(tag_id: int): + # Map tag_id back to a deterministic path. + return [{"path": f"/tagged-by-{tag_id}.txt", "is_directory": False}] + + webdav = AsyncMock() + webdav.get_tag_by_name = AsyncMock(side_effect=fake_get_tag) + webdav.get_files_by_tag = AsyncMock(side_effect=fake_get_files) + + caplog.set_level( + logging.WARNING, logger="nextcloud_mcp_server.server.tag_exclusion" + ) + result = await get_excluded_file_paths(webdav) + + # Both healthy tags must contribute one path each; the broken + # tag is silently skipped. + good_1_id = hash("good-1") & 0xFFFF + good_2_id = hash("good-2") & 0xFFFF + assert result == { + f"tagged-by-{good_1_id}.txt", + f"tagged-by-{good_2_id}.txt", + } + assert "Tag exclusion lookup failed" in caplog.text + assert "broken" in caplog.text diff --git a/tests/unit/test_webdav_tools_exclusion.py b/tests/unit/test_webdav_tools_exclusion.py index 3a229672..c1a116c9 100644 --- a/tests/unit/test_webdav_tools_exclusion.py +++ b/tests/unit/test_webdav_tools_exclusion.py @@ -288,3 +288,96 @@ async def test_find_by_name_filters_excluded( result = await fn(pattern="%.txt", ctx=_mock_ctx(fake_client)) assert [r.path for r in result.results] == ["/visible.txt"] + + +async def test_find_by_type_filters_excluded( + webdav_tools, fake_client, patch_get_client, patch_excluded +): + patch_get_client(fake_client) + patch_excluded({"Secret.txt"}) + fake_client.webdav.find_by_type = AsyncMock( + return_value=[ + {"path": "/Secret.txt", "name": "Secret.txt", "is_directory": False}, + {"path": "/visible.txt", "name": "visible.txt", "is_directory": False}, + ] + ) + + fn = webdav_tools["nc_webdav_find_by_type"].fn + result = await fn(mime_type="text/plain", ctx=_mock_ctx(fake_client)) + + assert [r.path for r in result.results] == ["/visible.txt"] + + +async def test_list_favorites_filters_excluded( + webdav_tools, fake_client, patch_get_client, patch_excluded +): + patch_get_client(fake_client) + patch_excluded({"Secret.txt"}) + fake_client.webdav.list_favorites = AsyncMock( + return_value=[ + {"path": "/Secret.txt", "name": "Secret.txt", "is_directory": False}, + {"path": "/visible.txt", "name": "visible.txt", "is_directory": False}, + ] + ) + + fn = webdav_tools["nc_webdav_list_favorites"].fn + result = await fn(ctx=_mock_ctx(fake_client)) + + assert [r.path for r in result.results] == ["/visible.txt"] + + +# ── Search-tool scope guards (review #764) ────────────────────────────── + + +async def test_search_files_raises_when_scope_excluded( + webdav_tools, fake_client, patch_get_client, patch_excluded +): + """Mirror the ``list_directory`` early guard so the four search tools + cannot silently return an empty result for an excluded ``scope``.""" + patch_get_client(fake_client) + patch_excluded({"Private"}) + + fn = webdav_tools["nc_webdav_search_files"].fn + with pytest.raises(ToolError, match="excluded tag"): + await fn(ctx=_mock_ctx(fake_client), scope="/Private", name_pattern="%.txt") + + fake_client.webdav.search_files.assert_not_called() + + +async def test_find_by_name_raises_when_scope_excluded( + webdav_tools, fake_client, patch_get_client, patch_excluded +): + patch_get_client(fake_client) + patch_excluded({"Private"}) + + fn = webdav_tools["nc_webdav_find_by_name"].fn + with pytest.raises(ToolError, match="excluded tag"): + await fn(pattern="%.txt", scope="/Private", ctx=_mock_ctx(fake_client)) + + fake_client.webdav.find_by_name.assert_not_called() + + +async def test_find_by_type_raises_when_scope_excluded( + webdav_tools, fake_client, patch_get_client, patch_excluded +): + patch_get_client(fake_client) + patch_excluded({"Private"}) + + fn = webdav_tools["nc_webdav_find_by_type"].fn + with pytest.raises(ToolError, match="excluded tag"): + await fn(mime_type="text/plain", scope="/Private", ctx=_mock_ctx(fake_client)) + + fake_client.webdav.find_by_type.assert_not_called() + + +async def test_list_favorites_raises_when_scope_excluded( + webdav_tools, fake_client, patch_get_client, patch_excluded +): + patch_get_client(fake_client) + patch_excluded({"Private"}) + + fn = webdav_tools["nc_webdav_list_favorites"].fn + with pytest.raises(ToolError, match="excluded tag"): + await fn(ctx=_mock_ctx(fake_client), scope="/Private") + + fake_client.webdav.list_favorites.assert_not_called()