From cef477b877a9de59ff1e158e254f32f8ce06afd4 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 16 Jun 2026 01:21:01 +0200 Subject: [PATCH] =?UTF-8?q?fix(vector-sync):=20address=20round-3=20review?= =?UTF-8?q?=20=E2=80=94=20gate=20purge=20route,=20bound=20set,=20nits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - app.py: register /api/v1/vector-sync/purge only when vector_sync_enabled, so it returns 404 (not a 500 from get_qdrant_client) when sync is off - scanner: bound _consent_backstop_done so a long-running multi-tenant process with user churn can't grow it without limit (clears on overflow) - purge route: distinct 400 for a missing doc_types key; enforce the admin check even for an empty no-op request (destructive route) - tests: missing-key 400, admin-gated empty no-op, non-admin empty 403 The _consent_narrowed_doc_types precondition is enforced by its non-Optional frozenset[str] signature (ty rejects a None caller). The httpx.BasicAuth SonarCloud hotspot matches the existing webhook routes (false positive, credential from the app-password store) — left consistent for UI triage. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/api/vector_sync.py | 14 +++++++--- nextcloud_mcp_server/app.py | 17 +++++++----- nextcloud_mcp_server/vector/scanner.py | 13 +++++++++ tests/unit/test_vector_sync_purge_route.py | 31 +++++++++++++++++++++- 4 files changed, 64 insertions(+), 11 deletions(-) diff --git a/nextcloud_mcp_server/api/vector_sync.py b/nextcloud_mcp_server/api/vector_sync.py index f70cc00b..af013152 100644 --- a/nextcloud_mcp_server/api/vector_sync.py +++ b/nextcloud_mcp_server/api/vector_sync.py @@ -77,6 +77,11 @@ async def purge_doc_types_route(request: Request) -> JSONResponse: ) raw = body.get("doc_types") + if raw is None: + return JSONResponse( + {"error": "Bad request", "message": "doc_types is required"}, + status_code=400, + ) if not isinstance(raw, list) or not all(isinstance(d, str) for d in raw): return JSONResponse( { @@ -86,8 +91,6 @@ async def purge_doc_types_route(request: Request) -> JSONResponse: status_code=400, ) doc_types = [d for d in raw if d] - if not doc_types: - return JSONResponse({"purged": {}}) # Bound the batch: there are only a handful of real indexed types, so a huge # list is abuse — cap it rather than fan out unbounded count+delete calls. if len(doc_types) > _MAX_PURGE_DOC_TYPES: @@ -107,7 +110,9 @@ async def purge_doc_types_route(request: Request) -> JSONResponse: if not nextcloud_host: raise ValueError("Nextcloud host not configured") - # Verify admin via the caller's own app password before any deletion. + # Verify admin via the caller's own app password before any deletion — + # enforced even for an empty (no-op) request, since this is a + # destructive admin route. async with nextcloud_httpx_client( base_url=nextcloud_host, auth=httpx.BasicAuth(username, app_password), @@ -125,6 +130,9 @@ async def purge_doc_types_route(request: Request) -> JSONResponse: status_code=403, ) + if not doc_types: + return JSONResponse({"purged": {}}) + purged = await purge_doc_types(doc_types) logger.info("Vector-sync purge by admin %s: %s", user_id, purged) return JSONResponse({"purged": purged}) diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 34e05754..3d419a28 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -2424,14 +2424,17 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = Route("/api/v1/webhooks/{webhook_id}", delete_webhook, methods=["DELETE"]) ) # Vector-sync admin: purge indexed vectors by doc type (admin consent — - # called by Astrolabe when a source is disabled for semantic search) - routes.append( - Route( - "/api/v1/vector-sync/purge", - purge_doc_types_route, - methods=["POST"], + # called by Astrolabe when a source is disabled for semantic search). + # Gated on vector_sync_enabled: without it there is no Qdrant client, so + # the purge would 500 rather than no-op. + if settings.vector_sync_enabled: + routes.append( + Route( + "/api/v1/vector-sync/purge", + purge_doc_types_route, + methods=["POST"], + ) ) - ) # Access and scope management endpoints (ADR-022) routes.append( Route( diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index f55c0d1b..1587b078 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -293,6 +293,13 @@ _TEXT_BACKSTOP_DOC_TYPES: tuple[str, ...] = tuple(sorted(INDEXED_DOC_TYPES - {"f # the type is allowed again, so a later re-disable re-triggers the backstop. _consent_backstop_done: set[tuple[str, str]] = set() +# Safety bound on the tracking set so a long-running multi-tenant process with +# heavy user churn (deprovisioned users leave stale entries) can't grow it +# without limit. At <= len(INDEXED_DOC_TYPES) entries per user this is generous; +# on overflow we clear the whole set, which at worst re-fires the (idempotent) +# backstop once for currently-disabled types. +_CONSENT_BACKSTOP_MAX = 50_000 + async def _enqueue_deletes_for_disabled_types( user_id: str, @@ -366,6 +373,12 @@ async def _enqueue_deletes_for_disabled_types( queued += 1 # Mark this (user, doc_type) backstopped for the current disable episode # so subsequent scans don't re-enqueue the same idempotent deletes. + if len(_consent_backstop_done) >= _CONSENT_BACKSTOP_MAX: + logger.info( + "consent backstop tracking set hit %d entries; clearing", + _CONSENT_BACKSTOP_MAX, + ) + _consent_backstop_done.clear() _consent_backstop_done.add((user_id, doc_type)) return queued diff --git a/tests/unit/test_vector_sync_purge_route.py b/tests/unit/test_vector_sync_purge_route.py index 5b4e92aa..8053847f 100644 --- a/tests/unit/test_vector_sync_purge_route.py +++ b/tests/unit/test_vector_sync_purge_route.py @@ -112,10 +112,25 @@ def test_forbidden_when_not_admin(mocker): purge.assert_not_called() -def test_empty_doc_types_is_noop(mocker): +def test_missing_doc_types_key_returns_400(mocker): _patch_token(mocker) purge = _patch_purge(mocker) + client = TestClient(_build_app()) + resp = client.post("/api/v1/vector-sync/purge", json={}) + + assert resp.status_code == 400 + purge.assert_not_called() + + +def test_empty_doc_types_is_admin_gated_noop(mocker): + # An empty (no-op) request still requires admin — this is a destructive route. + _patch_token(mocker, "admin") + _patch_basic_auth(mocker, "admin") + _patch_outbound_client(mocker) + _patch_groups(mocker, ["admin"]) + purge = _patch_purge(mocker) + client = TestClient(_build_app()) resp = client.post("/api/v1/vector-sync/purge", json={"doc_types": []}) @@ -124,6 +139,20 @@ def test_empty_doc_types_is_noop(mocker): purge.assert_not_called() +def test_empty_doc_types_forbidden_for_non_admin(mocker): + _patch_token(mocker, "bob") + _patch_basic_auth(mocker, "bob") + _patch_outbound_client(mocker) + _patch_groups(mocker, ["users"]) + purge = _patch_purge(mocker) + + client = TestClient(_build_app()) + resp = client.post("/api/v1/vector-sync/purge", json={"doc_types": []}) + + assert resp.status_code == 403 + purge.assert_not_called() + + def test_admin_purge_happy_path(mocker): _patch_token(mocker, "admin") _patch_basic_auth(mocker, "admin")