fix(webdav): address PR #764 review round 2
Addresses the four points raised in the automated review on PR #764: 1. Scope guards on the four search tools (search_files, find_by_name, find_by_type, list_favorites) so an excluded `scope` raises ToolError instead of silently returning an empty result. Previously an LLM could probe the asymmetry between list_directory (raises) and the search tools (silent) to infer that an excluded directory exists. The 4 search tools now mirror the early-guard pattern from list_directory and avoid an unnecessary upstream query for known- excluded scopes. 2. Concurrent per-tag resolution in get_excluded_file_paths via anyio.create_task_group(). Previously the 2N network calls (1 PROPFIND + 1 REPORT per tag) ran serially. Per-tag fail-open behaviour is preserved by extracting _resolve_one_tag, which swallows its own exceptions so a single tag failure does not abort the surrounding task group. 3. WebDAVClient.get_tag_by_name and get_files_by_tag now route through _make_request, inheriting the @retry_on_429 decorator. Previously they bypassed it; with tag exclusion invoked on every WebDAV tool call, a transient 429 from the systemtags endpoint was hitting the fail-open path instead of being transparently retried. 4. Test coverage: 6 new tests in test_webdav_tools_exclusion.py (4 scope-guard, 2 missing filter tests for find_by_type and list_favorites) and 2 new tests in test_tag_exclusion.py (a concurrency proof using an event-barrier that would deadlock under sequential execution, and a fail-open-under-task-group test with order-independent side_effect callables). 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
a6c188abbb
commit
d179ca8c8b
@@ -1132,13 +1132,12 @@ class WebDAVClient(BaseNextcloudClient):
|
||||
</d:prop>
|
||||
</d:propfind>"""
|
||||
|
||||
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):
|
||||
</oc:filter-rules>
|
||||
</oc:filter-files>"""
|
||||
|
||||
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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user