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>
Six findings raised in the PR review:
🔴 Blocking
- Fail-open on tag-resolution errors. get_excluded_file_paths now
wraps each tag's get_tag_by_name and get_files_by_tag call in
try/except; failures log a warning and the tag is skipped, rather
than propagating to the caller and disabling all WebDAV tools when
the systemtags endpoint is degraded. Documented in the docstring as
the intended fail-open behaviour (threat model is preventing
accidental exfiltration, not surviving server compromise).
🟡 Important
- nc_webdav_list_directory now raises ToolError when the listed path
itself is tagged, instead of silently returning an empty listing
after a wasted PROPFIND. Behaviour now mirrors the mutating tools.
- Destination error messages in move/copy/create_directory said "is
inside" but is_path_excluded matches exact paths too. Reworded to
"is or is inside".
🟢 Nits
- get_excluded_file_paths log message clarified: N counts
directly-tagged paths, not total descendants.
- Test isolation: tests/unit/conftest.py already has an autouse
_reload_dynaconf_after_test fixture that handles teardown. Removed
the redundant module-local fixture I had drafted; documented the
reliance in the module docstring instead.
- Added tests/unit/test_webdav_tools_exclusion.py: 12 server-layer
tests that register the WebDAV tools on a fresh FastMCP and invoke
each tool's underlying function with a mocked excluded set, asserting
ToolError is raised / results filtered as expected. Catches future
guard-integration regressions (e.g. wrong argument order).
Also added two unit tests for the new fail-open behaviour.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hide sensitive files/folders from the WebDAV MCP tool surface by
tagging them with a configured Nextcloud system tag. Defence-in-depth
control for users who connect LLMs to accounts holding contracts,
medical records, credentials, etc.
A new EXCLUDED_TAGS env var (comma-separated tag names, empty by
default) gates an exclusion layer that runs at the start of every
WebDAV tool call: tag names are resolved to tag IDs, those IDs are
expanded to the set of tagged paths, then listings/searches are
filtered and read/write/delete/move/copy operations on excluded paths
raise ToolError. Tagged folders exclude their descendants via prefix
match. Empty EXCLUDED_TAGS disables the feature entirely.
The threat model is preventing accidental data exfiltration via the
LLM tool surface — not hiding files from a determined operator. The
docs explicitly recommend creating exclusion tags with
user_assignable=false so the credentials the MCP server uses cannot
remove the tag.
Implementation:
- config.py: add `excluded_tags` to _DEFAULTS, Settings, and the
_field_map alongside other comma-separated env vars.
- client/webdav.py: get_files_by_tag now requests <d:resourcetype/>
and surfaces is_directory so tagged directories can recursively
exclude descendants.
- server/tag_exclusion.py (new): get_excluded_tag_names,
get_excluded_file_paths, is_path_excluded.
- server/webdav.py: exclusion guards in all 11 WebDAV tools;
read/write/create/delete/move/copy raise ToolError, list/search
tools silently filter excluded entries. Existing f-string log
calls converted to lazy %-style.
- tests: 17 new unit tests covering path-matching edge cases
(shared-prefix non-match, descendants of excluded dirs), tag-name
parsing, and get_excluded_file_paths with mocked WebDAV; 1 new
client test asserting <d:resourcetype/> -> is_directory parsing.
- docs/configuration.md: new "Tag-Based File Exclusion" section with
per-tool effect table, security guidance, and per-call cost note.
- README.md: feature mention under Key Features.
Closes#710.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Many identity providers (AWS Cognito, Okta, Azure AD) reject or mishandle
colons in OAuth scope names. This migrates all custom scopes from
`resource:action` to `resource.action` format (e.g., `notes:read` →
`notes.read`), which is universally accepted and aligns with industry
conventions (Microsoft, Google).
Includes Alembic migration 004 for stored scope strings and ADR-024
documenting the rationale and RFC references.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address all reviewer comments from PR #387:
1. ✅ Add unit tests for annotations (tests/server/test_annotations.py)
- 10 comprehensive test functions validating all annotation patterns
- Tests for titles, read-only, destructive, idempotent operations
- Validates specific ADR-017 decisions (webdav write, semantic search)
- Cross-category consistency checks
2. ✅ Fix nc_webdav_write_file idempotency classification
- Changed from idempotentHint=False to idempotentHint=True
- Rationale: Uses HTTP PUT without version control
- Writing same content to same path = same end state (idempotent)
3. ✅ Fix semantic search openWorldHint inconsistency
- Changed from openWorldHint=False to openWorldHint=True
- Rationale: Consistent with other Nextcloud tools
- Nextcloud is external to MCP server (indexed data is implementation detail)
4. ✅ Update ADR-017 with resolved decisions
- Converted Open Questions to Resolved Questions
- Added detailed rationale for webdav write and semantic search
- Updated status from Proposed to Implemented
- Added decision timeline with dates
5. ✅ Add MCP Tool Annotations guidelines to CLAUDE.md
- Comprehensive section with code examples for all patterns
- Key principles documented (idempotency, destructive, open world)
- References ADR-017 for detailed rationale
All OAuth tools verified to have proper annotations (oauth_tools.py lines 686-751).
The Smithery scanner was reporting "0 tools" despite the server returning
valid tool definitions. Root cause: the server was returning SSE-formatted
responses (event: message\ndata: {...}) which the scanner couldn't parse.
Changes:
- Add json_response=True to FastMCP for Smithery stateless mode
- Clean up verbose docstring examples in semantic.py and webdav.py
The MCP spec allows both SSE and plain JSON responses for HTTP transport.
Setting json_response=True returns Content-Type: application/json with
plain JSON-RPC instead of text/event-stream with SSE format.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Resolves the token exchange implementation gap where get_session_client()
was implemented but never used by tools. Unifies token acquisition into a
single async get_client() method that handles both pass-through and token
exchange modes transparently.
Core Changes:
- Make get_client() async and merge token exchange logic into it
- Remove scopes parameter from token exchange (Nextcloud doesn't support OAuth scopes)
- Update all 8 tool modules to use await get_client(ctx)
- Fix provisioning decorator to skip checks in BasicAuth mode
Token Acquisition Modes:
1. BasicAuth: Returns shared client (no token operations)
2. OAuth pass-through (default): Verifies and passes Flow 1 token to Nextcloud
3. OAuth token exchange (opt-in): Exchanges Flow 1 token for ephemeral token via RFC 8693
Key Architectural Clarifications:
- Progressive Consent (Flow 1/2) = Authorization architecture
- Token Exchange = Token acquisition pattern during tool execution
- Refresh tokens from Flow 2 are NEVER used for tool calls (only background jobs)
- Nextcloud scopes are "soft-scopes" enforced by MCP server, not IdP
Documentation Updates:
- ADR-004: Added comprehensive token acquisition patterns section
- CRITICAL-TOKEN-EXCHANGE-PATTERN.md: Updated to reflect implementation status
- CLAUDE.md: Updated architectural patterns with async get_client()
Testing:
- All 36 unit tests passing
- All 4 smoke tests passing (BasicAuth mode)
- Linting issues fixed (ruff)
Configuration:
ENABLE_TOKEN_EXCHANGE=false (default) - pass-through mode
ENABLE_TOKEN_EXCHANGE=true (opt-in) - token exchange mode
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
