From 2e2a098bee62a2e0e1bb2fc61f45c4339240f9d4 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 03:13:50 +0200 Subject: [PATCH 1/6] fix(webhooks): wire receiver to vector sync queue and fix registered URI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /webhooks/nextcloud endpoint was a no-op stub that logged the payload and returned 200 OK; webhook deletions never reached Qdrant. Compounding that, _get_webhook_uri() registered the docker-compose internal hostname (http://mcp:8000) with Nextcloud whenever /.dockerenv existed β€” including ECS Fargate β€” so cloud deployments were registering a URL NC could not resolve. - New vector/webhook_parser.py extracts a DocumentTask from NodeCreatedEvent / NodeWrittenEvent / BeforeNodeDeletedEvent payloads scoped to */files/Notes/*.md (matching the registered preset filters). - New vector/webhook_receiver.py pushes that task onto the same send-stream the scanner uses (app.state.document_send_stream), with 503 when sync is not running so NC retries delivery. - _get_webhook_uri() now prefers NEXTCLOUD_MCP_SERVER_URL over the /.dockerenv branch, so the explicit public URL set on cloud tasks wins; docker-compose dev still falls back to the internal name when no public URL is configured. Calendar / Tables event parsing is intentionally out of scope here. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/app.py | 31 +-- nextcloud_mcp_server/auth/webhook_routes.py | 46 +++-- nextcloud_mcp_server/vector/webhook_parser.py | 86 ++++++++ .../vector/webhook_receiver.py | 82 ++++++++ tests/unit/test_webhook_endpoint.py | 144 ++++++++++++++ tests/unit/test_webhook_parser.py | 188 ++++++++++++++++++ tests/unit/test_webhook_uri.py | 95 +++++++++ 7 files changed, 624 insertions(+), 48 deletions(-) create mode 100644 nextcloud_mcp_server/vector/webhook_parser.py create mode 100644 nextcloud_mcp_server/vector/webhook_receiver.py create mode 100644 tests/unit/test_webhook_endpoint.py create mode 100644 tests/unit/test_webhook_parser.py create mode 100644 tests/unit/test_webhook_uri.py diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index e0b1c87a..f5e72660 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -130,6 +130,7 @@ from nextcloud_mcp_server.vector.oauth_sync import ( user_manager_task, ) from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client +from nextcloud_mcp_server.vector.webhook_receiver import handle_nextcloud_webhook logger = logging.getLogger(__name__) HTTPXClientInstrumentor().instrument() @@ -1950,30 +1951,6 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = status_code=status_code, ) - async def handle_nextcloud_webhook(request): - """Test webhook endpoint to capture and log Nextcloud webhook payloads. - - This is a temporary endpoint for testing webhook schemas and payloads. - It logs the full payload and returns 200 OK immediately. - """ - - try: - payload = await request.json() - logger.info("=" * 80) - logger.info("πŸ”” Webhook received from Nextcloud:") - logger.info(json.dumps(payload, indent=2, sort_keys=True)) - logger.info("=" * 80) - - return JSONResponse( - {"status": "received", "timestamp": payload.get("time")}, - status_code=200, - ) - except Exception as e: - logger.error(f"❌ Failed to parse webhook payload: {e}") - return JSONResponse( - {"error": "invalid_payload", "message": str(e)}, status_code=400 - ) - # Add Protected Resource Metadata (PRM) endpoint for OAuth mode routes = [] @@ -1982,11 +1959,13 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = routes.append(Route("/health/ready", health_ready, methods=["GET"])) logger.info("Health check endpoints enabled: /health/live, /health/ready") - # Add test webhook endpoint (for development/testing) + # Add Nextcloud webhook receiver (queues DocumentTasks for vector sync). + # Implementation lives in vector/webhook_receiver.py; the handler reads + # the send-stream from request.app.state.document_send_stream. routes.append( Route("/webhooks/nextcloud", handle_nextcloud_webhook, methods=["POST"]) ) - logger.info("Test webhook endpoint enabled: /webhooks/nextcloud") + logger.info("Webhook endpoint enabled: /webhooks/nextcloud") # Add management API endpoints for Nextcloud PHP app # Tier 1: Public endpoints (no auth required) diff --git a/nextcloud_mcp_server/auth/webhook_routes.py b/nextcloud_mcp_server/auth/webhook_routes.py index e7ee3c66..7606e2ea 100644 --- a/nextcloud_mcp_server/auth/webhook_routes.py +++ b/nextcloud_mcp_server/auth/webhook_routes.py @@ -77,33 +77,37 @@ async def _get_installed_apps(http_client: httpx.AsyncClient) -> list[str]: def _get_webhook_uri() -> str: """Get the webhook endpoint URI for this MCP server. - This function determines the correct webhook URL based on the environment: - 1. Uses WEBHOOK_INTERNAL_URL if explicitly set (highest priority) - 2. Detects Docker environment and uses internal service name - 3. Falls back to NEXTCLOUD_MCP_SERVER_URL + Priority (highest first): + 1. ``WEBHOOK_INTERNAL_URL`` β€” explicit override, e.g. for split + internal/external URLs. + 2. ``NEXTCLOUD_MCP_SERVER_URL`` β€” the configured public URL. This is + set on cloud deployments (ECS, k8s) and is the URL Nextcloud must + POST to. + 3. ``/.dockerenv`` (or podman / ``DOCKER_CONTAINER=true``) β†’ internal + docker-compose service name. Only relevant when no public URL is + configured β€” i.e. local dev where MCP and NC share a Docker + network. + 4. ``http://localhost:8000`` β€” last-resort fallback. - In Docker environments, Nextcloud needs to reach the MCP service using - the internal Docker network hostname (e.g., http://mcp:8000), not localhost. - - Returns: - Full webhook endpoint URL accessible from Nextcloud + Note: ECS Fargate containers also expose ``/.dockerenv``. Without this + priority order, cloud deployments would silently register an internal + docker-compose hostname (e.g. ``http://mcp:8000``) that NC cannot + resolve, dropping every webhook delivery. """ - # Explicit override (highest priority) webhook_url = os.getenv("WEBHOOK_INTERNAL_URL") if webhook_url: return f"{webhook_url}/webhooks/nextcloud" - # Detect Docker environment - # Check for common Docker indicators - is_docker = ( - os.path.exists("/.dockerenv") # Docker container marker file - or os.path.exists("/run/.containerenv") # Podman marker - or os.getenv("DOCKER_CONTAINER") == "true" # Explicit flag - ) + server_url = os.getenv("NEXTCLOUD_MCP_SERVER_URL") + if server_url: + return f"{server_url}/webhooks/nextcloud" + is_docker = ( + os.path.exists("/.dockerenv") + or os.path.exists("/run/.containerenv") + or os.getenv("DOCKER_CONTAINER") == "true" + ) if is_docker: - # In Docker, use internal service name from NEXTCLOUD_MCP_SERVICE_NAME - # or default to 'mcp' (docker-compose service name) service_name = os.getenv("NEXTCLOUD_MCP_SERVICE_NAME", "mcp") port = os.getenv("NEXTCLOUD_MCP_PORT", "8000") logger.debug( @@ -111,9 +115,7 @@ def _get_webhook_uri() -> str: ) return f"http://{service_name}:{port}/webhooks/nextcloud" - # Fallback to configured server URL (for non-Docker deployments) - server_url = os.getenv("NEXTCLOUD_MCP_SERVER_URL", "http://localhost:8000") - return f"{server_url}/webhooks/nextcloud" + return "http://localhost:8000/webhooks/nextcloud" async def _get_authenticated_client(request: Request) -> httpx.AsyncClient: diff --git a/nextcloud_mcp_server/vector/webhook_parser.py b/nextcloud_mcp_server/vector/webhook_parser.py new file mode 100644 index 00000000..82297729 --- /dev/null +++ b/nextcloud_mcp_server/vector/webhook_parser.py @@ -0,0 +1,86 @@ +"""Parse Nextcloud webhook payloads into DocumentTask objects. + +Maps Nextcloud webhook events to vector-sync DocumentTasks. The handler at +``/webhooks/nextcloud`` calls :func:`extract_document_task` and forwards any +non-None result to the same processor send-stream the scanner uses. + +Currently scoped to file (note) events. Calendar / Tables events fall through +to ``None`` for now; those parsers can be added in follow-up changes. + +See ADR-010 for the design and ``webhook-testing-findings.md`` for real +captured payloads. +""" + +import logging +import re + +from nextcloud_mcp_server.vector.scanner import DocumentTask + +logger = logging.getLogger(__name__) + +_FILE_EVENT_CREATED = "OCP\\Files\\Events\\Node\\NodeCreatedEvent" +_FILE_EVENT_WRITTEN = "OCP\\Files\\Events\\Node\\NodeWrittenEvent" +_FILE_EVENT_BEFORE_DELETED = "OCP\\Files\\Events\\Node\\BeforeNodeDeletedEvent" + +# Matches paths inside any user's Notes folder ending in .md, e.g. +# "/admin/files/Notes/Sub/Note.md" or "/alice/files/Notes/foo.md". +_NOTES_PATH_RE = re.compile(r"^/[^/]+/files/Notes/.+\.md$") + + +def extract_document_task(payload: dict) -> DocumentTask | None: + """Convert a Nextcloud webhook payload into a DocumentTask. + + Returns None for any event we don't (yet) handle, or any event whose + target isn't a markdown file under a user's Notes folder. Callers should + treat None as "ignored" β€” not an error. + """ + try: + event = payload["event"] + event_class = event["class"] + user_id = payload["user"]["uid"] + except (KeyError, TypeError): + logger.debug("Webhook payload missing user/event/class fields") + return None + + if event_class in ( + _FILE_EVENT_CREATED, + _FILE_EVENT_WRITTEN, + _FILE_EVENT_BEFORE_DELETED, + ): + return _parse_file_event(event_class, event, user_id, payload.get("time", 0)) + + logger.debug("Ignoring webhook for unsupported event: %s", event_class) + return None + + +def _parse_file_event( + event_class: str, event: dict, user_id: str, time: int +) -> DocumentTask | None: + node = event.get("node") or {} + path = node.get("path", "") + node_id = node.get("id") + + if not _NOTES_PATH_RE.match(path): + # Not a note file β€” could be a parent folder, an unrelated file, etc. + return None + + if node_id is None: + # BeforeNodeDeletedEvent should still carry node.id; if it doesn't + # we can't address the Qdrant points to delete. Skip rather than + # guess β€” the polling scanner will catch up via its grace period. + logger.warning( + "Webhook %s for note %s missing node.id; falling back to scanner", + event_class, + path, + ) + return None + + operation = "delete" if event_class == _FILE_EVENT_BEFORE_DELETED else "index" + + return DocumentTask( + user_id=user_id, + doc_id=str(node_id), + doc_type="note", + operation=operation, + modified_at=int(time), + ) diff --git a/nextcloud_mcp_server/vector/webhook_receiver.py b/nextcloud_mcp_server/vector/webhook_receiver.py new file mode 100644 index 00000000..0086eb5d --- /dev/null +++ b/nextcloud_mcp_server/vector/webhook_receiver.py @@ -0,0 +1,82 @@ +"""HTTP receiver for Nextcloud webhooks. + +Routes inbound webhooks to the same processor send-stream the scanner uses. +The receiver is registered as a Starlette route at ``/webhooks/nextcloud`` +in :mod:`nextcloud_mcp_server.app`. +""" + +import logging + +from starlette.requests import Request +from starlette.responses import JSONResponse + +from nextcloud_mcp_server.vector.webhook_parser import extract_document_task + +logger = logging.getLogger(__name__) + + +async def handle_nextcloud_webhook(request: Request) -> JSONResponse: + """Receive a Nextcloud webhook and queue a DocumentTask for vector sync. + + Returns quickly so NC's webhook worker is not blocked. The send-stream is + read from ``request.app.state.document_send_stream``; when vector sync + isn't running we return 503 so NC retries delivery. + """ + try: + payload = await request.json() + except Exception as e: + logger.warning(f"Webhook payload was not valid JSON: {e}") + return JSONResponse( + {"status": "error", "message": "invalid JSON"}, + status_code=400, + ) + + task = extract_document_task(payload) + if task is None: + event_class = (payload.get("event") or {}).get("class", "") + logger.debug("Webhook ignored (unsupported event): %s", event_class) + return JSONResponse( + {"status": "ignored", "reason": "unsupported event"}, + status_code=200, + ) + + send_stream = getattr(request.app.state, "document_send_stream", None) + if send_stream is None: + logger.warning( + "Webhook received but vector sync is not running; rejecting so NC retries" + ) + return JSONResponse( + {"status": "unavailable", "reason": "vector sync not running"}, + status_code=503, + ) + + try: + await send_stream.send(task) + except Exception as e: + logger.error( + "Failed to queue webhook task for %s_%s: %s", + task.doc_type, + task.doc_id, + e, + ) + return JSONResponse( + {"status": "error", "message": "queue unavailable"}, + status_code=500, + ) + + logger.info( + "Webhook queued %s_%s (%s) for user %s", + task.doc_type, + task.doc_id, + task.operation, + task.user_id, + ) + return JSONResponse( + { + "status": "queued", + "doc_type": task.doc_type, + "doc_id": task.doc_id, + "operation": task.operation, + }, + status_code=200, + ) diff --git a/tests/unit/test_webhook_endpoint.py b/tests/unit/test_webhook_endpoint.py new file mode 100644 index 00000000..6ae7c1bd --- /dev/null +++ b/tests/unit/test_webhook_endpoint.py @@ -0,0 +1,144 @@ +"""Unit tests for the ``/webhooks/nextcloud`` HTTP receiver. + +Builds a minimal Starlette app around ``handle_nextcloud_webhook`` so we can +drive it with ``TestClient`` without standing up the full FastMCP server. +""" + +import anyio +import pytest +from starlette.applications import Starlette +from starlette.routing import Route +from starlette.testclient import TestClient + +from nextcloud_mcp_server.vector.webhook_receiver import handle_nextcloud_webhook + +pytestmark = pytest.mark.unit + + +def _make_app(send_stream=None) -> Starlette: + app = Starlette( + routes=[ + Route("/webhooks/nextcloud", handle_nextcloud_webhook, methods=["POST"]) + ] + ) + app.state.document_send_stream = send_stream + return app + + +_NOTE_CREATED = { + "user": {"uid": "admin", "displayName": "admin"}, + "time": 1762850245, + "event": { + "class": "OCP\\Files\\Events\\Node\\NodeCreatedEvent", + "node": { + "id": 437, + "path": "/admin/files/Notes/Webhooks/Webhook Test Note.md", + }, + }, +} + + +_NOTE_DELETED = { + "user": {"uid": "alice"}, + "time": 1762851093, + "event": { + "class": "OCP\\Files\\Events\\Node\\BeforeNodeDeletedEvent", + "node": {"id": 99, "path": "/alice/files/Notes/foo.md"}, + }, +} + + +def test_index_event_queues_task_and_returns_200(): + send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=4) + app = _make_app(send_stream=send_stream) + + with TestClient(app) as client: + response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED) + + assert response.status_code == 200 + assert response.json()["status"] == "queued" + assert response.json()["operation"] == "index" + assert response.json()["doc_id"] == "437" + + task = receive_stream.receive_nowait() + assert task.user_id == "admin" + assert task.doc_id == "437" + assert task.operation == "index" + assert task.doc_type == "note" + + +def test_delete_event_queues_delete_task(): + send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=4) + app = _make_app(send_stream=send_stream) + + with TestClient(app) as client: + response = client.post("/webhooks/nextcloud", json=_NOTE_DELETED) + + assert response.status_code == 200 + assert response.json()["operation"] == "delete" + + task = receive_stream.receive_nowait() + assert task.operation == "delete" + assert task.doc_id == "99" + assert task.user_id == "alice" + + +def test_unsupported_event_is_ignored(): + send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=4) + app = _make_app(send_stream=send_stream) + + payload = { + "user": {"uid": "admin"}, + "time": 1, + "event": { + "class": "OCP\\Calendar\\Events\\CalendarObjectCreatedEvent", + "objectData": {"id": 7}, + }, + } + + with TestClient(app) as client: + response = client.post("/webhooks/nextcloud", json=payload) + + assert response.status_code == 200 + assert response.json()["status"] == "ignored" + + with pytest.raises(anyio.WouldBlock): + receive_stream.receive_nowait() + + +def test_invalid_json_returns_400(): + app = _make_app(send_stream=None) + + with TestClient(app) as client: + response = client.post( + "/webhooks/nextcloud", + content=b"not json", + headers={"content-type": "application/json"}, + ) + + assert response.status_code == 400 + assert response.json()["status"] == "error" + + +def test_returns_503_when_send_stream_not_wired(): + """Vector sync not running β†’ tell NC to retry instead of dropping the + event.""" + app = _make_app(send_stream=None) + + with TestClient(app) as client: + response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED) + + assert response.status_code == 503 + assert response.json()["status"] == "unavailable" + + +def test_returns_500_when_stream_is_closed(): + send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=1) + receive_stream.close() # close receiver β†’ send raises BrokenResourceError + app = _make_app(send_stream=send_stream) + + with TestClient(app) as client: + response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED) + + assert response.status_code == 500 + assert response.json()["status"] == "error" diff --git a/tests/unit/test_webhook_parser.py b/tests/unit/test_webhook_parser.py new file mode 100644 index 00000000..0f044915 --- /dev/null +++ b/tests/unit/test_webhook_parser.py @@ -0,0 +1,188 @@ +"""Unit tests for the Nextcloud webhook payload parser. + +Payload examples are taken from real Nextcloud captures recorded in +``webhook-testing-findings.md``. +""" + +import pytest + +from nextcloud_mcp_server.vector.webhook_parser import extract_document_task + + +@pytest.mark.unit +def test_node_created_event_returns_index_task(): + payload = { + "user": {"uid": "admin", "displayName": "admin"}, + "time": 1762850245, + "event": { + "class": "OCP\\Files\\Events\\Node\\NodeCreatedEvent", + "node": { + "id": 437, + "path": "/admin/files/Notes/Webhooks/Webhook Test Note.md", + }, + }, + } + + task = extract_document_task(payload) + + assert task is not None + assert task.user_id == "admin" + assert task.doc_id == "437" + assert task.doc_type == "note" + assert task.operation == "index" + assert task.modified_at == 1762850245 + + +@pytest.mark.unit +def test_node_written_event_returns_index_task(): + payload = { + "user": {"uid": "admin", "displayName": "admin"}, + "time": 1762850960, + "event": { + "class": "OCP\\Files\\Events\\Node\\NodeWrittenEvent", + "node": { + "id": 437, + "path": "/admin/files/Notes/Webhooks/Webhook Test Note.md", + }, + }, + } + + task = extract_document_task(payload) + + assert task is not None + assert task.operation == "index" + assert task.doc_id == "437" + + +@pytest.mark.unit +def test_before_node_deleted_event_returns_delete_task(): + payload = { + "user": {"uid": "alice", "displayName": "Alice"}, + "time": 1762851093, + "event": { + "class": "OCP\\Files\\Events\\Node\\BeforeNodeDeletedEvent", + "node": { + "id": 437, + "path": "/alice/files/Notes/Webhooks/Webhook Test Note.md", + }, + }, + } + + task = extract_document_task(payload) + + assert task is not None + assert task.user_id == "alice" + assert task.operation == "delete" + assert task.doc_id == "437" + assert task.doc_type == "note" + + +@pytest.mark.unit +def test_node_id_is_normalized_to_string(): + """NC sends node.id as int; we always emit str (per ADR-010 Β§A.3).""" + payload = { + "user": {"uid": "admin"}, + "time": 1, + "event": { + "class": "OCP\\Files\\Events\\Node\\NodeCreatedEvent", + "node": {"id": 437, "path": "/admin/files/Notes/foo.md"}, + }, + } + + task = extract_document_task(payload) + + assert task is not None + assert isinstance(task.doc_id, str) + assert task.doc_id == "437" + + +@pytest.mark.unit +def test_path_outside_notes_returns_none(): + payload = { + "user": {"uid": "admin"}, + "time": 1, + "event": { + "class": "OCP\\Files\\Events\\Node\\NodeCreatedEvent", + "node": {"id": 1, "path": "/admin/files/Documents/foo.md"}, + }, + } + + assert extract_document_task(payload) is None + + +@pytest.mark.unit +def test_non_markdown_inside_notes_returns_none(): + payload = { + "user": {"uid": "admin"}, + "time": 1, + "event": { + "class": "OCP\\Files\\Events\\Node\\NodeCreatedEvent", + "node": {"id": 1, "path": "/admin/files/Notes/image.png"}, + }, + } + + assert extract_document_task(payload) is None + + +@pytest.mark.unit +def test_parent_folder_event_returns_none(): + """Creating a note fires events for the parent folder too β€” ignore those.""" + payload = { + "user": {"uid": "admin"}, + "time": 1, + "event": { + "class": "OCP\\Files\\Events\\Node\\NodeCreatedEvent", + "node": {"id": 100, "path": "/admin/files/Notes/Webhooks"}, + }, + } + + assert extract_document_task(payload) is None + + +@pytest.mark.unit +def test_unknown_event_class_returns_none(): + payload = { + "user": {"uid": "admin"}, + "time": 1, + "event": { + "class": "OCP\\Calendar\\Events\\CalendarObjectCreatedEvent", + "objectData": {"id": 7, "uri": "x.ics"}, + }, + } + + assert extract_document_task(payload) is None + + +@pytest.mark.unit +def test_node_deleted_event_without_id_returns_none(): + """``NodeDeletedEvent`` (no node.id) is not the registered event, but if + one ever leaks through we ignore it rather than guess at the doc_id β€” + the polling scanner will catch up via its grace period.""" + payload = { + "user": {"uid": "admin"}, + "time": 1, + "event": { + "class": "OCP\\Files\\Events\\Node\\BeforeNodeDeletedEvent", + "node": {"path": "/admin/files/Notes/foo.md"}, + }, + } + + assert extract_document_task(payload) is None + + +@pytest.mark.unit +def test_missing_user_field_returns_none(): + payload = { + "time": 1, + "event": { + "class": "OCP\\Files\\Events\\Node\\NodeCreatedEvent", + "node": {"id": 1, "path": "/admin/files/Notes/foo.md"}, + }, + } + + assert extract_document_task(payload) is None + + +@pytest.mark.unit +def test_empty_payload_returns_none(): + assert extract_document_task({}) is None diff --git a/tests/unit/test_webhook_uri.py b/tests/unit/test_webhook_uri.py new file mode 100644 index 00000000..1d6bb18e --- /dev/null +++ b/tests/unit/test_webhook_uri.py @@ -0,0 +1,95 @@ +"""Unit tests for ``_get_webhook_uri`` priority order. + +Cloud deployments register the webhook URI returned by this function with +Nextcloud. ECS Fargate also exposes ``/.dockerenv``, so an explicit public +URL must win over the docker auto-detection branch. +""" + +import pytest + +from nextcloud_mcp_server.auth.webhook_routes import _get_webhook_uri + +ENV_VARS = ( + "WEBHOOK_INTERNAL_URL", + "NEXTCLOUD_MCP_SERVER_URL", + "NEXTCLOUD_MCP_SERVICE_NAME", + "NEXTCLOUD_MCP_PORT", + "DOCKER_CONTAINER", +) + + +@pytest.fixture(autouse=True) +def _clean_env(monkeypatch): + for name in ENV_VARS: + monkeypatch.delenv(name, raising=False) + + +def _no_docker_markers(monkeypatch): + monkeypatch.setattr( + "nextcloud_mcp_server.auth.webhook_routes.os.path.exists", + lambda _path: False, + ) + + +def _docker_markers(monkeypatch): + monkeypatch.setattr( + "nextcloud_mcp_server.auth.webhook_routes.os.path.exists", + lambda path: path == "/.dockerenv", + ) + + +@pytest.mark.unit +def test_webhook_internal_url_wins_over_everything(monkeypatch): + monkeypatch.setenv("WEBHOOK_INTERNAL_URL", "https://internal.example.com") + monkeypatch.setenv("NEXTCLOUD_MCP_SERVER_URL", "https://public.example.com") + _docker_markers(monkeypatch) + + assert _get_webhook_uri() == "https://internal.example.com/webhooks/nextcloud" + + +@pytest.mark.unit +def test_public_url_wins_over_docker_detection(monkeypatch): + """The bug-fix case: ECS containers have /.dockerenv but a public URL is + set. Docker auto-detection must NOT clobber the explicit public URL.""" + monkeypatch.setenv( + "NEXTCLOUD_MCP_SERVER_URL", "https://holy-bluegill.astrolabecloud.com" + ) + _docker_markers(monkeypatch) + + assert ( + _get_webhook_uri() + == "https://holy-bluegill.astrolabecloud.com/webhooks/nextcloud" + ) + + +@pytest.mark.unit +def test_docker_detection_used_when_no_public_url(monkeypatch): + """docker-compose dev: no public URL set, /.dockerenv exists β†’ use the + docker-compose service name.""" + _docker_markers(monkeypatch) + + assert _get_webhook_uri() == "http://mcp:8000/webhooks/nextcloud" + + +@pytest.mark.unit +def test_docker_detection_honors_service_name_and_port_overrides(monkeypatch): + monkeypatch.setenv("NEXTCLOUD_MCP_SERVICE_NAME", "mcp-login-flow") + monkeypatch.setenv("NEXTCLOUD_MCP_PORT", "8004") + _docker_markers(monkeypatch) + + assert _get_webhook_uri() == "http://mcp-login-flow:8004/webhooks/nextcloud" + + +@pytest.mark.unit +def test_docker_container_env_var_triggers_docker_branch(monkeypatch): + monkeypatch.setenv("DOCKER_CONTAINER", "true") + _no_docker_markers(monkeypatch) + + assert _get_webhook_uri() == "http://mcp:8000/webhooks/nextcloud" + + +@pytest.mark.unit +def test_localhost_fallback_when_nothing_set(monkeypatch): + _no_docker_markers(monkeypatch) + + assert _get_webhook_uri() == "http://localhost:8000/webhooks/nextcloud" From 224428fca524ebd6e24ddbb77dade983bd532dd7 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 03:49:50 +0200 Subject: [PATCH 2/6] fix(webhooks): authenticate deliveries via WEBHOOK_SECRET; review nits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds optional shared-secret authentication for /webhooks/nextcloud, addressing the security follow-up flagged in #747. Behavior: - WEBHOOK_SECRET set: registrations pass authMethod="header" with authData={"Authorization": "Bearer "} (encrypted at-rest in Nextcloud's DB and forwarded on every delivery). The receiver validates the same header with hmac.compare_digest before parsing any payload; missing/invalid β†’ 401. - WEBHOOK_SECRET unset: registrations stay on authMethod="none" and the receiver accepts unauthenticated POSTs (logging a one-time startup warning). Backward compatible β€” operators can roll out at their own pace. Implementation notes: - WebhooksClient.create_webhook gains an `auth_data` parameter mapped to NC's `authData` body field; this is distinct from the existing `headers` parameter (`headers` is plaintext static request headers, `authData` is encrypted at-rest in NC and only emitted when authMethod="header"). The previous `auth_method="bearer"` mention in the docstring was incorrect β€” NC supports only "none" and "header". - A small `webhook_auth_pair()` helper in auth/webhook_routes.py centralises the secretβ†’(auth_method, auth_data) resolution so the preset flow and the Astrolabe-facing /api/v1/webhooks endpoint stay in sync. Also addresses the smaller review points from #747: - f-string β†’ lazy %s formatting in webhook_receiver.py and webhook_routes.py. - Move `int(time)` inside webhook_parser's try/except so a malformed `time` field returns None instead of raising ValueError. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/webhooks.py | 11 +- nextcloud_mcp_server/auth/webhook_routes.py | 26 ++++- nextcloud_mcp_server/client/webhooks.py | 14 ++- nextcloud_mcp_server/config.py | 13 +++ nextcloud_mcp_server/vector/webhook_parser.py | 9 +- .../vector/webhook_receiver.py | 42 ++++++- tests/client/test_webhooks_client.py | 34 ++++++ tests/unit/test_webhook_endpoint.py | 103 ++++++++++++++++++ tests/unit/test_webhook_parser.py | 30 +++++ tests/unit/test_webhook_uri.py | 36 +++++- 10 files changed, 306 insertions(+), 12 deletions(-) diff --git a/nextcloud_mcp_server/api/webhooks.py b/nextcloud_mcp_server/api/webhooks.py index 34d68ad9..e042164b 100644 --- a/nextcloud_mcp_server/api/webhooks.py +++ b/nextcloud_mcp_server/api/webhooks.py @@ -18,6 +18,7 @@ from nextcloud_mcp_server.api.management import ( extract_bearer_token, validate_token_and_get_user, ) +from nextcloud_mcp_server.auth.webhook_routes import webhook_auth_pair from nextcloud_mcp_server.client.webhooks import WebhooksClient from ..http import nextcloud_httpx_client @@ -213,10 +214,16 @@ async def create_webhook(request: Request) -> JSONResponse: headers={"Authorization": f"Bearer {token}"}, timeout=30.0, ) as client: - # Use WebhooksClient to create webhook + # Use WebhooksClient to create webhook. Inject auth headers when + # WEBHOOK_SECRET is configured so deliveries are authenticated. webhooks_client = WebhooksClient(client, user_id) + auth_method, auth_data = webhook_auth_pair() webhook_data = await webhooks_client.create_webhook( - event=event, uri=uri, event_filter=event_filter + event=event, + uri=uri, + event_filter=event_filter, + auth_method=auth_method, + auth_data=auth_data, ) return JSONResponse({"webhook": webhook_data}) diff --git a/nextcloud_mcp_server/auth/webhook_routes.py b/nextcloud_mcp_server/auth/webhook_routes.py index 7606e2ea..f292e63c 100644 --- a/nextcloud_mcp_server/auth/webhook_routes.py +++ b/nextcloud_mcp_server/auth/webhook_routes.py @@ -14,6 +14,7 @@ from starlette.responses import HTMLResponse from nextcloud_mcp_server.auth.permissions import is_nextcloud_admin from nextcloud_mcp_server.client.webhooks import WebhooksClient +from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.server.webhook_presets import ( WEBHOOK_PRESETS, filter_presets_by_installed_apps, @@ -111,13 +112,33 @@ def _get_webhook_uri() -> str: service_name = os.getenv("NEXTCLOUD_MCP_SERVICE_NAME", "mcp") port = os.getenv("NEXTCLOUD_MCP_PORT", "8000") logger.debug( - f"Docker environment detected, using internal URL: http://{service_name}:{port}" + "Docker environment detected, using internal URL: http://%s:%s", + service_name, + port, ) return f"http://{service_name}:{port}/webhooks/nextcloud" return "http://localhost:8000/webhooks/nextcloud" +def webhook_auth_pair() -> tuple[str, dict[str, str] | None]: + """Resolve ``(auth_method, auth_data)`` for new webhook registrations. + + When ``WEBHOOK_SECRET`` is set, returns + ``("header", {"Authorization": f"Bearer {secret}"})`` so NC stores the + credential encrypted at-rest and forwards it on every delivery. When + unset, returns ``("none", None)`` β€” backward-compatible with deployments + that haven't rolled out webhook auth yet. + + Shared by both registration call sites: the ``/app/webhooks`` preset + flow and the Astrolabe-facing ``/api/v1/webhooks`` endpoint. + """ + secret = get_settings().webhook_secret + if not secret: + return ("none", None) + return ("header", {"Authorization": f"Bearer {secret}"}) + + async def _get_authenticated_client(request: Request) -> httpx.AsyncClient: """Get an authenticated HTTP client for Nextcloud API calls. @@ -400,11 +421,14 @@ async def enable_webhook_preset(request: Request) -> HTMLResponse: webhook_uri = _get_webhook_uri() registered_ids = [] + auth_method, auth_data = webhook_auth_pair() for event_config in preset["events"]: webhook_data = await webhooks_client.create_webhook( event=event_config["event"], uri=webhook_uri, event_filter=event_config["filter"] if event_config["filter"] else None, + auth_method=auth_method, + auth_data=auth_data, ) webhook_id = webhook_data["id"] registered_ids.append(webhook_id) diff --git a/nextcloud_mcp_server/client/webhooks.py b/nextcloud_mcp_server/client/webhooks.py index e1b206be..d956a330 100644 --- a/nextcloud_mcp_server/client/webhooks.py +++ b/nextcloud_mcp_server/client/webhooks.py @@ -41,6 +41,7 @@ class WebhooksClient(BaseNextcloudClient): http_method: str = "POST", auth_method: str = "none", headers: Optional[Dict[str, str]] = None, + auth_data: Optional[Dict[str, str]] = None, event_filter: Optional[Dict[str, Any]] = None, ) -> Dict[str, Any]: """Register a new webhook for the specified event. @@ -49,8 +50,14 @@ class WebhooksClient(BaseNextcloudClient): event: Fully qualified event class name (e.g., "OCP\\Files\\Events\\Node\\NodeCreatedEvent") uri: Webhook endpoint URL to receive event notifications http_method: HTTP method for webhook delivery (default: "POST") - auth_method: Authentication method ("none", "bearer", etc.) - headers: Custom headers to include in webhook requests (e.g., Authorization header) + auth_method: Authentication method. Nextcloud's webhook_listeners + app accepts only ``"none"`` or ``"header"``. + headers: Optional static request headers attached to every + delivery (stored in clear text on the NC side). + auth_data: When ``auth_method="header"``, a dict of headers + holding the auth credentials. Stored encrypted at-rest in + Nextcloud's database and merged into the delivery request + at send time. Required when ``auth_method="header"``. event_filter: JSON object specifying event filters (e.g., {"user.uid": "bob"}) Returns: @@ -66,6 +73,9 @@ class WebhooksClient(BaseNextcloudClient): if headers: data["headers"] = headers + if auth_data: + data["authData"] = auth_data + if event_filter: data["eventFilter"] = event_filter diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index b84c989c..4ed370e6 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -53,6 +53,10 @@ _DEFAULTS: dict[str, Any] = { # None = ephemeral per-process tempfile (see get_token_db_path()). # Set TOKEN_STORAGE_DB to persist tokens across restarts. "token_storage_db": None, + # Webhook delivery authentication (ADR-010): when set, registrations + # tell NC to add `Authorization: Bearer ` to webhook deliveries + # and the receiver rejects unauthenticated requests. + "webhook_secret": None, # Vector sync "vector_sync_scan_interval": 300, "vector_sync_processor_workers": 3, @@ -430,6 +434,13 @@ class Settings: token_encryption_key: str | None = None token_storage_db: str | None = None + # Webhook delivery authentication (ADR-010). + # When set, the registrar passes Authorization: Bearer as the + # webhook authData and the receiver validates the same header on each + # delivery. When unset, registration uses authMethod="none" and the + # receiver accepts unauthenticated POSTs (backward-compatible). + webhook_secret: str | None = None + # Vector sync settings (ADR-007) vector_sync_enabled: bool = False vector_sync_scan_interval: int = 300 # seconds (5 minutes) @@ -767,6 +778,8 @@ def get_settings() -> Settings: # Token and webhook storage settings "token_encryption_key": "TOKEN_ENCRYPTION_KEY", "token_storage_db": "TOKEN_STORAGE_DB", + # Webhook auth (ADR-010) + "webhook_secret": "WEBHOOK_SECRET", # Vector sync settings (ADR-007) "vector_sync_scan_interval": "VECTOR_SYNC_SCAN_INTERVAL", "vector_sync_processor_workers": "VECTOR_SYNC_PROCESSOR_WORKERS", diff --git a/nextcloud_mcp_server/vector/webhook_parser.py b/nextcloud_mcp_server/vector/webhook_parser.py index 82297729..dfcb3710 100644 --- a/nextcloud_mcp_server/vector/webhook_parser.py +++ b/nextcloud_mcp_server/vector/webhook_parser.py @@ -38,8 +38,9 @@ def extract_document_task(payload: dict) -> DocumentTask | None: event = payload["event"] event_class = event["class"] user_id = payload["user"]["uid"] - except (KeyError, TypeError): - logger.debug("Webhook payload missing user/event/class fields") + time = int(payload.get("time", 0) or 0) + except (KeyError, TypeError, ValueError): + logger.debug("Webhook payload has missing or malformed envelope fields") return None if event_class in ( @@ -47,7 +48,7 @@ def extract_document_task(payload: dict) -> DocumentTask | None: _FILE_EVENT_WRITTEN, _FILE_EVENT_BEFORE_DELETED, ): - return _parse_file_event(event_class, event, user_id, payload.get("time", 0)) + return _parse_file_event(event_class, event, user_id, time) logger.debug("Ignoring webhook for unsupported event: %s", event_class) return None @@ -82,5 +83,5 @@ def _parse_file_event( doc_id=str(node_id), doc_type="note", operation=operation, - modified_at=int(time), + modified_at=time, ) diff --git a/nextcloud_mcp_server/vector/webhook_receiver.py b/nextcloud_mcp_server/vector/webhook_receiver.py index 0086eb5d..44c52945 100644 --- a/nextcloud_mcp_server/vector/webhook_receiver.py +++ b/nextcloud_mcp_server/vector/webhook_receiver.py @@ -5,15 +5,37 @@ The receiver is registered as a Starlette route at ``/webhooks/nextcloud`` in :mod:`nextcloud_mcp_server.app`. """ +import hmac import logging from starlette.requests import Request from starlette.responses import JSONResponse +from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.vector.webhook_parser import extract_document_task logger = logging.getLogger(__name__) +_warned_about_missing_secret = False + + +def _warn_missing_secret_once() -> None: + """Log a one-time WARNING when WEBHOOK_SECRET is unset. + + The receiver still accepts unauthenticated POSTs in this case so existing + deployments keep working, but the operator should know they're running + without webhook auth. + """ + global _warned_about_missing_secret + if _warned_about_missing_secret: + return + _warned_about_missing_secret = True + logger.warning( + "WEBHOOK_SECRET is not set; /webhooks/nextcloud accepts " + "unauthenticated requests. Set WEBHOOK_SECRET and re-register " + "webhooks to enable Authorization: Bearer validation." + ) + async def handle_nextcloud_webhook(request: Request) -> JSONResponse: """Receive a Nextcloud webhook and queue a DocumentTask for vector sync. @@ -21,11 +43,29 @@ async def handle_nextcloud_webhook(request: Request) -> JSONResponse: Returns quickly so NC's webhook worker is not blocked. The send-stream is read from ``request.app.state.document_send_stream``; when vector sync isn't running we return 503 so NC retries delivery. + + When ``WEBHOOK_SECRET`` is set, the request must carry + ``Authorization: Bearer `` (registered via ``authData`` so NC + forwards it on every delivery); requests without a valid header are + rejected with 401 before any further work. """ + secret = get_settings().webhook_secret + if secret: + provided = request.headers.get("authorization", "") + expected = f"Bearer {secret}" + if not provided or not hmac.compare_digest(provided, expected): + logger.warning("Webhook rejected: missing or invalid Authorization header") + return JSONResponse( + {"status": "unauthorized"}, + status_code=401, + ) + else: + _warn_missing_secret_once() + try: payload = await request.json() except Exception as e: - logger.warning(f"Webhook payload was not valid JSON: {e}") + logger.warning("Webhook payload was not valid JSON: %s", e) return JSONResponse( {"status": "error", "message": "invalid JSON"}, status_code=400, diff --git a/tests/client/test_webhooks_client.py b/tests/client/test_webhooks_client.py index 6c5022f9..bb0eb995 100644 --- a/tests/client/test_webhooks_client.py +++ b/tests/client/test_webhooks_client.py @@ -169,6 +169,40 @@ async def test_create_webhook_with_auth_headers(webhooks_client, mocker): assert call_args[1]["json"]["headers"] == {"Authorization": "Bearer secret-token"} +@pytest.mark.unit +async def test_create_webhook_with_auth_data(webhooks_client, mocker): + """``auth_data`` lands in the OCS body as ``authData`` so NC encrypts + the credentials at-rest and merges them in at delivery time.""" + mock_response = mocker.Mock() + mock_response.json.return_value = { + "ocs": { + "data": { + "id": 126, + "uri": "http://example.com/webhook", + "event": "OCP\\Files\\Events\\Node\\NodeCreatedEvent", + "authMethod": "header", + } + } + } + + mock_make_request = mocker.patch.object( + WebhooksClient, "_make_request", return_value=mock_response + ) + + await webhooks_client.create_webhook( + event="OCP\\Files\\Events\\Node\\NodeCreatedEvent", + uri="http://example.com/webhook", + auth_method="header", + auth_data={"Authorization": "Bearer supersecret"}, + ) + + call_args = mock_make_request.call_args + assert call_args[1]["json"]["authMethod"] == "header" + assert call_args[1]["json"]["authData"] == {"Authorization": "Bearer supersecret"} + # The static `headers` field must NOT be set when only auth_data is passed. + assert "headers" not in call_args[1]["json"] + + @pytest.mark.unit async def test_delete_webhook(webhooks_client, mocker): """Test deleting a webhook registration.""" diff --git a/tests/unit/test_webhook_endpoint.py b/tests/unit/test_webhook_endpoint.py index 6ae7c1bd..077aad19 100644 --- a/tests/unit/test_webhook_endpoint.py +++ b/tests/unit/test_webhook_endpoint.py @@ -10,11 +10,32 @@ from starlette.applications import Starlette from starlette.routing import Route from starlette.testclient import TestClient +from nextcloud_mcp_server.config import Settings +from nextcloud_mcp_server.vector import webhook_receiver from nextcloud_mcp_server.vector.webhook_receiver import handle_nextcloud_webhook pytestmark = pytest.mark.unit +@pytest.fixture(autouse=True) +def _reset_warned_flag(): + """The receiver warns once per process when WEBHOOK_SECRET is missing. + Reset between tests so each gets a clean slate.""" + webhook_receiver._warned_about_missing_secret = False + yield + webhook_receiver._warned_about_missing_secret = False + + +def _patch_secret(monkeypatch, secret: str | None) -> None: + """Make ``get_settings()`` (as called inside the receiver) return a + Settings instance with the given ``webhook_secret``.""" + monkeypatch.setattr( + webhook_receiver, + "get_settings", + lambda: Settings(webhook_secret=secret), + ) + + def _make_app(send_stream=None) -> Starlette: app = Starlette( routes=[ @@ -142,3 +163,85 @@ def test_returns_500_when_stream_is_closed(): assert response.status_code == 500 assert response.json()["status"] == "error" + + +# --- WEBHOOK_SECRET authentication --------------------------------------- + + +def test_secret_set_valid_bearer_header_queues_task(monkeypatch): + _patch_secret(monkeypatch, "supersecret") + send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=4) + app = _make_app(send_stream=send_stream) + + with TestClient(app) as client: + response = client.post( + "/webhooks/nextcloud", + json=_NOTE_CREATED, + headers={"Authorization": "Bearer supersecret"}, + ) + + assert response.status_code == 200 + assert response.json()["status"] == "queued" + assert receive_stream.receive_nowait().doc_id == "437" + + +def test_secret_set_missing_authorization_returns_401(monkeypatch): + _patch_secret(monkeypatch, "supersecret") + send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=4) + app = _make_app(send_stream=send_stream) + + with TestClient(app) as client: + response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED) + + assert response.status_code == 401 + assert response.json()["status"] == "unauthorized" + with pytest.raises(anyio.WouldBlock): + receive_stream.receive_nowait() + + +def test_secret_set_wrong_secret_returns_401(monkeypatch): + _patch_secret(monkeypatch, "supersecret") + send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=4) + app = _make_app(send_stream=send_stream) + + with TestClient(app) as client: + response = client.post( + "/webhooks/nextcloud", + json=_NOTE_CREATED, + headers={"Authorization": "Bearer wrong"}, + ) + + assert response.status_code == 401 + with pytest.raises(anyio.WouldBlock): + receive_stream.receive_nowait() + + +def test_secret_set_wrong_scheme_returns_401(monkeypatch): + """A token without the Bearer prefix is rejected.""" + _patch_secret(monkeypatch, "supersecret") + send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=4) + app = _make_app(send_stream=send_stream) + + with TestClient(app) as client: + response = client.post( + "/webhooks/nextcloud", + json=_NOTE_CREATED, + headers={"Authorization": "supersecret"}, + ) + + assert response.status_code == 401 + + +def test_secret_unset_accepts_unauthenticated(monkeypatch): + """Backward compat: deployments that haven't yet set WEBHOOK_SECRET keep + working β€” the receiver accepts unauthenticated POSTs and logs a one-time + warning.""" + _patch_secret(monkeypatch, None) + send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=4) + app = _make_app(send_stream=send_stream) + + with TestClient(app) as client: + response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED) + + assert response.status_code == 200 + assert receive_stream.receive_nowait().doc_id == "437" diff --git a/tests/unit/test_webhook_parser.py b/tests/unit/test_webhook_parser.py index 0f044915..ddcc86a6 100644 --- a/tests/unit/test_webhook_parser.py +++ b/tests/unit/test_webhook_parser.py @@ -186,3 +186,33 @@ def test_missing_user_field_returns_none(): @pytest.mark.unit def test_empty_payload_returns_none(): assert extract_document_task({}) is None + + +@pytest.mark.unit +def test_non_numeric_time_field_returns_none(): + """A malformed ``time`` field must not raise ValueError out of the parser.""" + payload = { + "user": {"uid": "admin"}, + "time": "not-a-number", + "event": { + "class": "OCP\\Files\\Events\\Node\\NodeCreatedEvent", + "node": {"id": 1, "path": "/admin/files/Notes/foo.md"}, + }, + } + + assert extract_document_task(payload) is None + + +@pytest.mark.unit +def test_missing_time_field_defaults_to_zero(): + payload = { + "user": {"uid": "admin"}, + "event": { + "class": "OCP\\Files\\Events\\Node\\NodeCreatedEvent", + "node": {"id": 1, "path": "/admin/files/Notes/foo.md"}, + }, + } + + task = extract_document_task(payload) + assert task is not None + assert task.modified_at == 0 diff --git a/tests/unit/test_webhook_uri.py b/tests/unit/test_webhook_uri.py index 1d6bb18e..d5834351 100644 --- a/tests/unit/test_webhook_uri.py +++ b/tests/unit/test_webhook_uri.py @@ -1,4 +1,5 @@ -"""Unit tests for ``_get_webhook_uri`` priority order. +"""Unit tests for ``_get_webhook_uri`` priority order and the +``webhook_auth_kwargs`` registration helper. Cloud deployments register the webhook URI returned by this function with Nextcloud. ECS Fargate also exposes ``/.dockerenv``, so an explicit public @@ -7,7 +8,12 @@ URL must win over the docker auto-detection branch. import pytest -from nextcloud_mcp_server.auth.webhook_routes import _get_webhook_uri +from nextcloud_mcp_server.auth import webhook_routes +from nextcloud_mcp_server.auth.webhook_routes import ( + _get_webhook_uri, + webhook_auth_pair, +) +from nextcloud_mcp_server.config import Settings ENV_VARS = ( "WEBHOOK_INTERNAL_URL", @@ -93,3 +99,29 @@ def test_localhost_fallback_when_nothing_set(monkeypatch): _no_docker_markers(monkeypatch) assert _get_webhook_uri() == "http://localhost:8000/webhooks/nextcloud" + + +# --- webhook_auth_pair() -------------------------------------------------- + + +def _patch_secret(monkeypatch, secret: str | None) -> None: + monkeypatch.setattr( + webhook_routes, + "get_settings", + lambda: Settings(webhook_secret=secret), + ) + + +@pytest.mark.unit +def test_auth_pair_returns_none_when_secret_unset(monkeypatch): + _patch_secret(monkeypatch, None) + assert webhook_auth_pair() == ("none", None) + + +@pytest.mark.unit +def test_auth_pair_emits_bearer_header_when_secret_set(monkeypatch): + _patch_secret(monkeypatch, "supersecret") + assert webhook_auth_pair() == ( + "header", + {"Authorization": "Bearer supersecret"}, + ) From f5f05b7c844fcbd57c070b8af120709d6ded2eec Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 04:02:26 +0200 Subject: [PATCH 3/6] refactor(webhooks): address PR review on auth-pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - webhook_receiver: always run hmac.compare_digest (drop the `not provided or` short-circuit) so the constant-time path is taken regardless of whether the Authorization header is present. - client/webhooks: modernise the new `auth_data` type hint to `dict[str, str] | None` per CLAUDE.md. - tests/client: rename `test_create_webhook_with_auth_headers` β†’ `test_create_webhook_with_static_headers` and use `auth_method="header"` (NC's webhook_listeners only supports "none" and "header"; the previous "bearer" value was invalid). - auth/webhook_routes: extract `_register_preset_webhooks` from `enable_webhook_preset` so the auth-threading behaviour is testable without standing up a Starlette app + auth middleware. - tests/unit: new test_webhook_routes_register covering the helper with secret set / unset, and verifying ids round-trip in order. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/auth/webhook_routes.py | 50 +++++++---- nextcloud_mcp_server/client/webhooks.py | 2 +- .../vector/webhook_receiver.py | 5 +- tests/client/test_webhooks_client.py | 19 +++-- tests/unit/test_webhook_routes_register.py | 84 +++++++++++++++++++ 5 files changed, 136 insertions(+), 24 deletions(-) create mode 100644 tests/unit/test_webhook_routes_register.py diff --git a/nextcloud_mcp_server/auth/webhook_routes.py b/nextcloud_mcp_server/auth/webhook_routes.py index f292e63c..b5cd98d0 100644 --- a/nextcloud_mcp_server/auth/webhook_routes.py +++ b/nextcloud_mcp_server/auth/webhook_routes.py @@ -17,6 +17,7 @@ from nextcloud_mcp_server.client.webhooks import WebhooksClient from nextcloud_mcp_server.config import get_settings from nextcloud_mcp_server.server.webhook_presets import ( WEBHOOK_PRESETS, + WebhookPreset, filter_presets_by_installed_apps, get_preset, ) @@ -139,6 +140,38 @@ def webhook_auth_pair() -> tuple[str, dict[str, str] | None]: return ("header", {"Authorization": f"Bearer {secret}"}) +async def _register_preset_webhooks( + webhooks_client: WebhooksClient, + preset: WebhookPreset, + webhook_uri: str, +) -> list[int]: + """Register every event in a preset against a single MCP webhook URI. + + Threads the resolved ``(auth_method, auth_data)`` from + :func:`webhook_auth_pair` onto each registration call so deliveries + carry the configured ``Authorization`` header (when ``WEBHOOK_SECRET`` + is set) or fall through to ``authMethod="none"`` (backward-compatible + when it's not). + + Extracted from :func:`enable_webhook_preset` so the auth-threading + behaviour is testable without standing up a Starlette app. + """ + auth_method, auth_data = webhook_auth_pair() + registered_ids: list[int] = [] + for event_config in preset["events"]: + webhook_data = await webhooks_client.create_webhook( + event=event_config["event"], + uri=webhook_uri, + event_filter=event_config["filter"] if event_config["filter"] else None, + auth_method=auth_method, + auth_data=auth_data, + ) + webhook_id = webhook_data["id"] + registered_ids.append(webhook_id) + logger.info("Registered webhook %s for %s", webhook_id, event_config["event"]) + return registered_ids + + async def _get_authenticated_client(request: Request) -> httpx.AsyncClient: """Get an authenticated HTTP client for Nextcloud API calls. @@ -419,20 +452,9 @@ async def enable_webhook_preset(request: Request) -> HTMLResponse: # Register webhooks webhooks_client = WebhooksClient(http_client, username) webhook_uri = _get_webhook_uri() - registered_ids = [] - - auth_method, auth_data = webhook_auth_pair() - for event_config in preset["events"]: - webhook_data = await webhooks_client.create_webhook( - event=event_config["event"], - uri=webhook_uri, - event_filter=event_config["filter"] if event_config["filter"] else None, - auth_method=auth_method, - auth_data=auth_data, - ) - webhook_id = webhook_data["id"] - registered_ids.append(webhook_id) - logger.info(f"Registered webhook {webhook_id} for {event_config['event']}") + registered_ids = await _register_preset_webhooks( + webhooks_client, preset, webhook_uri + ) # Persist webhook IDs to database storage = _get_storage(request) diff --git a/nextcloud_mcp_server/client/webhooks.py b/nextcloud_mcp_server/client/webhooks.py index d956a330..cf5bd1fd 100644 --- a/nextcloud_mcp_server/client/webhooks.py +++ b/nextcloud_mcp_server/client/webhooks.py @@ -41,7 +41,7 @@ class WebhooksClient(BaseNextcloudClient): http_method: str = "POST", auth_method: str = "none", headers: Optional[Dict[str, str]] = None, - auth_data: Optional[Dict[str, str]] = None, + auth_data: dict[str, str] | None = None, event_filter: Optional[Dict[str, Any]] = None, ) -> Dict[str, Any]: """Register a new webhook for the specified event. diff --git a/nextcloud_mcp_server/vector/webhook_receiver.py b/nextcloud_mcp_server/vector/webhook_receiver.py index 44c52945..214954b4 100644 --- a/nextcloud_mcp_server/vector/webhook_receiver.py +++ b/nextcloud_mcp_server/vector/webhook_receiver.py @@ -53,7 +53,10 @@ async def handle_nextcloud_webhook(request: Request) -> JSONResponse: if secret: provided = request.headers.get("authorization", "") expected = f"Bearer {secret}" - if not provided or not hmac.compare_digest(provided, expected): + # Always run compare_digest so the constant-time path is taken even + # when the header is missing β€” `compare_digest("", expected)` returns + # False without leaking length information. + if not hmac.compare_digest(provided, expected): logger.warning("Webhook rejected: missing or invalid Authorization header") return JSONResponse( {"status": "unauthorized"}, diff --git a/tests/client/test_webhooks_client.py b/tests/client/test_webhooks_client.py index bb0eb995..b09bf594 100644 --- a/tests/client/test_webhooks_client.py +++ b/tests/client/test_webhooks_client.py @@ -135,8 +135,11 @@ async def test_create_webhook_with_filter(webhooks_client, mocker): @pytest.mark.unit -async def test_create_webhook_with_auth_headers(webhooks_client, mocker): - """Test creating a webhook with authentication headers.""" +async def test_create_webhook_with_static_headers(webhooks_client, mocker): + """Static request headers (the ``headers`` field) ride on every delivery + independently of ``authData``. NC's webhook_listeners app accepts only + ``authMethod="none"`` or ``"header"`` β€” pre-existing tests previously + referenced ``"bearer"`` which is not a valid value.""" mock_response = mocker.Mock() mock_response.json.return_value = { "ocs": { @@ -144,7 +147,7 @@ async def test_create_webhook_with_auth_headers(webhooks_client, mocker): "id": 125, "uri": "http://example.com/webhook", "event": "OCP\\Files\\Events\\Node\\NodeCreatedEvent", - "authMethod": "bearer", + "authMethod": "header", } } } @@ -156,17 +159,17 @@ async def test_create_webhook_with_auth_headers(webhooks_client, mocker): webhook_data = await webhooks_client.create_webhook( event="OCP\\Files\\Events\\Node\\NodeCreatedEvent", uri="http://example.com/webhook", - auth_method="bearer", - headers={"Authorization": "Bearer secret-token"}, + auth_method="header", + headers={"X-Trace-Id": "trace-123"}, ) assert webhook_data["id"] == 125 - assert webhook_data["authMethod"] == "bearer" + assert webhook_data["authMethod"] == "header" mock_make_request.assert_called_once() call_args = mock_make_request.call_args - assert call_args[1]["json"]["authMethod"] == "bearer" - assert call_args[1]["json"]["headers"] == {"Authorization": "Bearer secret-token"} + assert call_args[1]["json"]["authMethod"] == "header" + assert call_args[1]["json"]["headers"] == {"X-Trace-Id": "trace-123"} @pytest.mark.unit diff --git a/tests/unit/test_webhook_routes_register.py b/tests/unit/test_webhook_routes_register.py new file mode 100644 index 00000000..ae7568f0 --- /dev/null +++ b/tests/unit/test_webhook_routes_register.py @@ -0,0 +1,84 @@ +"""Unit tests for ``_register_preset_webhooks``. + +The helper threads ``webhook_auth_pair()`` into each ``create_webhook`` +call, so it is the integration point between the secret-resolution logic +and the OCS client. These tests verify the wiring without standing up a +full Starlette app. +""" + +import pytest + +from nextcloud_mcp_server.auth import webhook_routes +from nextcloud_mcp_server.auth.webhook_routes import _register_preset_webhooks +from nextcloud_mcp_server.client.webhooks import WebhooksClient +from nextcloud_mcp_server.config import Settings +from nextcloud_mcp_server.server.webhook_presets import get_preset + +pytestmark = pytest.mark.unit + + +def _patch_secret(monkeypatch, secret: str | None) -> None: + monkeypatch.setattr( + webhook_routes, + "get_settings", + lambda: Settings(webhook_secret=secret), + ) + + +def _make_webhooks_client(mocker, ids: list[int]): + """Mock WebhooksClient.create_webhook to return one fake webhook per id.""" + client = mocker.AsyncMock(spec=WebhooksClient) + client.create_webhook.side_effect = [{"id": i} for i in ids] + return client + + +async def test_register_threads_bearer_auth_when_secret_set(monkeypatch, mocker): + _patch_secret(monkeypatch, "supersecret") + preset = get_preset("notes_sync") + assert preset is not None + client = _make_webhooks_client(mocker, ids=[101, 102, 103]) + + registered = await _register_preset_webhooks( + client, preset, "https://mcp.example.com/webhooks/nextcloud" + ) + + assert registered == [101, 102, 103] + assert client.create_webhook.await_count == len(preset["events"]) + + expected_auth = {"Authorization": "Bearer supersecret"} + for call, event_config in zip( + client.create_webhook.await_args_list, preset["events"] + ): + kwargs = call.kwargs + assert kwargs["event"] == event_config["event"] + assert kwargs["uri"] == "https://mcp.example.com/webhooks/nextcloud" + assert kwargs["auth_method"] == "header" + assert kwargs["auth_data"] == expected_auth + # notes_sync uses path filters; ensure they round-trip through the helper + assert kwargs["event_filter"] == event_config["filter"] + + +async def test_register_uses_none_auth_when_secret_unset(monkeypatch, mocker): + _patch_secret(monkeypatch, None) + preset = get_preset("notes_sync") + assert preset is not None + client = _make_webhooks_client(mocker, ids=[1, 2, 3]) + + await _register_preset_webhooks( + client, preset, "https://mcp.example.com/webhooks/nextcloud" + ) + + for call in client.create_webhook.await_args_list: + assert call.kwargs["auth_method"] == "none" + assert call.kwargs["auth_data"] is None + + +async def test_register_returns_ids_in_call_order(monkeypatch, mocker): + _patch_secret(monkeypatch, None) + preset = get_preset("notes_sync") + assert preset is not None + client = _make_webhooks_client(mocker, ids=[42, 43, 44]) + + ids = await _register_preset_webhooks(client, preset, "https://example.com/wh") + + assert ids == [42, 43, 44] From c1368b9a7f3d59076bf3a1a63e3f0332b03507cb Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 04:17:00 +0200 Subject: [PATCH 4/6] refactor(webhooks): bound queue waits, route URLs through dynaconf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses round-3 review feedback on PR #747: - webhook_receiver: wrap send_stream.send() in anyio.fail_after(1.0) and return 503 with reason="queue full" if the queue is saturated. Avoids pinning the handler until NC's outbound timeout fires; the 503 retry contract is the same as the existing "sync not running" branch. - webhook_receiver: revise the compare_digest comment to match what the function actually guarantees β€” it avoids the per-character short-circuit of `==` but is not fully constant-time across length differences. - _get_webhook_uri: read WEBHOOK_INTERNAL_URL and NEXTCLOUD_MCP_SERVER_URL via dynaconf so operators using settings.toml (rather than env vars) aren't silently routed into the docker/localhost fallback. Adds webhook_internal_url to Settings/_DEFAULTS/_field_map; nextcloud_mcp_server_url already existed. Docker-detection markers stay on os.getenv since they're container-runtime signals, not user-facing config. - webhook_routes: sweep remaining f-string logger calls to lazy %s formatting per CLAUDE.md. - client/webhooks: modernise full file's type hints to dict / list / | None per CLAUDE.md. Tests: - New test_returns_503_when_queue_is_full exercises the timeout branch with a saturated buffer and a shortened deadline. - test_webhook_uri tests now patch get_settings (matching the auth-pair tests in the same file) instead of monkeypatching env vars directly. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/auth/webhook_routes.py | 44 ++++++++------- nextcloud_mcp_server/client/webhooks.py | 18 +++---- nextcloud_mcp_server/config.py | 8 +++ .../vector/webhook_receiver.py | 24 +++++++-- tests/unit/test_webhook_endpoint.py | 27 ++++++++++ tests/unit/test_webhook_uri.py | 53 ++++++++++++------- 6 files changed, 122 insertions(+), 52 deletions(-) diff --git a/nextcloud_mcp_server/auth/webhook_routes.py b/nextcloud_mcp_server/auth/webhook_routes.py index b5cd98d0..5fcfb7cd 100644 --- a/nextcloud_mcp_server/auth/webhook_routes.py +++ b/nextcloud_mcp_server/auth/webhook_routes.py @@ -72,7 +72,7 @@ async def _get_installed_apps(http_client: httpx.AsyncClient) -> list[str]: app_keys = set(capabilities.keys()) - core_keys return sorted(app_keys) except Exception as e: - logger.warning(f"Failed to get installed apps from capabilities: {e}") + logger.warning("Failed to get installed apps from capabilities: %s", e) return [] @@ -81,10 +81,10 @@ def _get_webhook_uri() -> str: Priority (highest first): 1. ``WEBHOOK_INTERNAL_URL`` β€” explicit override, e.g. for split - internal/external URLs. - 2. ``NEXTCLOUD_MCP_SERVER_URL`` β€” the configured public URL. This is - set on cloud deployments (ECS, k8s) and is the URL Nextcloud must - POST to. + internal/external URLs (read via dynaconf, so env vars and + settings.toml both work). + 2. ``NEXTCLOUD_MCP_SERVER_URL`` β€” the configured public URL set on + cloud deployments (ECS, k8s); the URL NC must POST to. 3. ``/.dockerenv`` (or podman / ``DOCKER_CONTAINER=true``) β†’ internal docker-compose service name. Only relevant when no public URL is configured β€” i.e. local dev where MCP and NC share a Docker @@ -96,14 +96,16 @@ def _get_webhook_uri() -> str: docker-compose hostname (e.g. ``http://mcp:8000``) that NC cannot resolve, dropping every webhook delivery. """ - webhook_url = os.getenv("WEBHOOK_INTERNAL_URL") - if webhook_url: - return f"{webhook_url}/webhooks/nextcloud" + settings = get_settings() + if settings.webhook_internal_url: + return f"{settings.webhook_internal_url}/webhooks/nextcloud" - server_url = os.getenv("NEXTCLOUD_MCP_SERVER_URL") - if server_url: - return f"{server_url}/webhooks/nextcloud" + if settings.nextcloud_mcp_server_url: + return f"{settings.nextcloud_mcp_server_url}/webhooks/nextcloud" + # Docker-environment markers stay on os.getenv: they're container-runtime + # signals (filesystem markers, optional service-name override) rather + # than user-facing config that would belong in settings.toml. is_docker = ( os.path.exists("/.dockerenv") or os.path.exists("/run/.containerenv") @@ -284,7 +286,7 @@ async def _get_enabled_presets( return enabled_presets except Exception as e: - logger.error(f"Failed to list webhooks: {e}") + logger.error("Failed to list webhooks: %s", e) return {} @@ -329,7 +331,7 @@ async def webhook_management_pane(request: Request) -> HTMLResponse: # Get installed apps to filter presets installed_apps = await _get_installed_apps(http_client) - logger.debug(f"Installed apps: {installed_apps}") + logger.debug("Installed apps: %s", installed_apps) # Get currently enabled presets (from database or API) enabled_presets = await _get_enabled_presets(webhooks_client, storage) @@ -404,7 +406,7 @@ async def webhook_management_pane(request: Request) -> HTMLResponse: return HTMLResponse(content=html_content) except Exception as e: - logger.error(f"Error loading webhook management pane: {e}", exc_info=True) + logger.error("Error loading webhook management pane: %s", e, exc_info=True) return HTMLResponse( content=f"""
@@ -462,7 +464,9 @@ async def enable_webhook_preset(request: Request) -> HTMLResponse: for webhook_id in registered_ids: await storage.store_webhook(webhook_id, preset_id) logger.info( - f"Persisted {len(registered_ids)} webhook(s) for preset '{preset_id}' to database" + "Persisted %d webhook(s) for preset '%s' to database", + len(registered_ids), + preset_id, ) # Return updated card @@ -494,7 +498,7 @@ async def enable_webhook_preset(request: Request) -> HTMLResponse: ) except Exception as e: - logger.error(f"Failed to enable preset {preset_id}: {e}", exc_info=True) + logger.error("Failed to enable preset %s: %s", preset_id, e, exc_info=True) return HTMLResponse( content=f'
Failed to enable preset: {str(e)}
', status_code=500, @@ -548,13 +552,15 @@ async def disable_webhook_preset(request: Request) -> HTMLResponse: for webhook_id in webhook_ids: await webhooks_client.delete_webhook(webhook_id) - logger.info(f"Deleted webhook {webhook_id} from preset {preset_id}") + logger.info("Deleted webhook %s from preset %s", webhook_id, preset_id) # Remove from database if storage: deleted_count = await storage.clear_preset_webhooks(preset_id) logger.info( - f"Removed {deleted_count} webhook(s) for preset '{preset_id}' from database" + "Removed %d webhook(s) for preset '%s' from database", + deleted_count, + preset_id, ) # Return updated card @@ -584,7 +590,7 @@ async def disable_webhook_preset(request: Request) -> HTMLResponse: ) except Exception as e: - logger.error(f"Failed to disable preset {preset_id}: {e}", exc_info=True) + logger.error("Failed to disable preset %s: %s", preset_id, e, exc_info=True) return HTMLResponse( content=f'
Failed to disable preset: {str(e)}
', status_code=500, diff --git a/nextcloud_mcp_server/client/webhooks.py b/nextcloud_mcp_server/client/webhooks.py index cf5bd1fd..8934298c 100644 --- a/nextcloud_mcp_server/client/webhooks.py +++ b/nextcloud_mcp_server/client/webhooks.py @@ -1,6 +1,6 @@ """Client for Nextcloud Webhook Listeners API operations.""" -from typing import Any, Dict, List, Optional +from typing import Any from nextcloud_mcp_server.client.base import BaseNextcloudClient @@ -11,15 +11,15 @@ class WebhooksClient(BaseNextcloudClient): app_name = "webhooks" def _get_webhook_headers( - self, additional_headers: Optional[Dict[str, str]] = None - ) -> Dict[str, str]: + self, additional_headers: dict[str, str] | None = None + ) -> dict[str, str]: """Get standard headers required for Webhook Listeners API calls.""" headers = {"OCS-APIRequest": "true", "Accept": "application/json"} if additional_headers: headers.update(additional_headers) return headers - async def list_webhooks(self) -> List[Dict[str, Any]]: + async def list_webhooks(self) -> list[dict[str, Any]]: """List all registered webhooks for the current user. Returns: @@ -40,10 +40,10 @@ class WebhooksClient(BaseNextcloudClient): uri: str, http_method: str = "POST", auth_method: str = "none", - headers: Optional[Dict[str, str]] = None, + headers: dict[str, str] | None = None, auth_data: dict[str, str] | None = None, - event_filter: Optional[Dict[str, Any]] = None, - ) -> Dict[str, Any]: + event_filter: dict[str, Any] | None = None, + ) -> dict[str, Any]: """Register a new webhook for the specified event. Args: @@ -63,7 +63,7 @@ class WebhooksClient(BaseNextcloudClient): Returns: Webhook registration details including webhook ID """ - data: Dict[str, Any] = { + data: dict[str, Any] = { "httpMethod": http_method, "uri": uri, "event": event, @@ -101,7 +101,7 @@ class WebhooksClient(BaseNextcloudClient): headers=headers, ) - async def get_webhook(self, webhook_id: int) -> Dict[str, Any]: + async def get_webhook(self, webhook_id: int) -> dict[str, Any]: """Get details of a specific webhook registration. Args: diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 4ed370e6..13a829fe 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -57,6 +57,9 @@ _DEFAULTS: dict[str, Any] = { # tell NC to add `Authorization: Bearer ` to webhook deliveries # and the receiver rejects unauthenticated requests. "webhook_secret": None, + # Internal URL override for webhook registration; wins over + # NEXTCLOUD_MCP_SERVER_URL when set (e.g. split internal/external URLs). + "webhook_internal_url": None, # Vector sync "vector_sync_scan_interval": 300, "vector_sync_processor_workers": 3, @@ -440,6 +443,10 @@ class Settings: # delivery. When unset, registration uses authMethod="none" and the # receiver accepts unauthenticated POSTs (backward-compatible). webhook_secret: str | None = None + # Internal URL override for webhook registration. Highest-priority + # source for the URL we register with NC (above + # nextcloud_mcp_server_url and the docker-detection fallback). + webhook_internal_url: str | None = None # Vector sync settings (ADR-007) vector_sync_enabled: bool = False @@ -780,6 +787,7 @@ def get_settings() -> Settings: "token_storage_db": "TOKEN_STORAGE_DB", # Webhook auth (ADR-010) "webhook_secret": "WEBHOOK_SECRET", + "webhook_internal_url": "WEBHOOK_INTERNAL_URL", # Vector sync settings (ADR-007) "vector_sync_scan_interval": "VECTOR_SYNC_SCAN_INTERVAL", "vector_sync_processor_workers": "VECTOR_SYNC_PROCESSOR_WORKERS", diff --git a/nextcloud_mcp_server/vector/webhook_receiver.py b/nextcloud_mcp_server/vector/webhook_receiver.py index 214954b4..fc630cd5 100644 --- a/nextcloud_mcp_server/vector/webhook_receiver.py +++ b/nextcloud_mcp_server/vector/webhook_receiver.py @@ -8,6 +8,7 @@ in :mod:`nextcloud_mcp_server.app`. import hmac import logging +import anyio from starlette.requests import Request from starlette.responses import JSONResponse @@ -53,9 +54,10 @@ async def handle_nextcloud_webhook(request: Request) -> JSONResponse: if secret: provided = request.headers.get("authorization", "") expected = f"Bearer {secret}" - # Always run compare_digest so the constant-time path is taken even - # when the header is missing β€” `compare_digest("", expected)` returns - # False without leaking length information. + # Use compare_digest to avoid the character-by-character short-circuit + # of `==`. compare_digest still returns False for differing lengths + # but isn't fully constant-time across them; that's fine here β€” a + # secret length leak is not a sensitive signal. if not hmac.compare_digest(provided, expected): logger.warning("Webhook rejected: missing or invalid Authorization header") return JSONResponse( @@ -94,7 +96,21 @@ async def handle_nextcloud_webhook(request: Request) -> JSONResponse: ) try: - await send_stream.send(task) + with anyio.fail_after(1.0): + await send_stream.send(task) + except TimeoutError: + # Queue is saturated (default 10 000 tasks). Returning 503 lets NC + # retry rather than pinning this handler until its outbound timeout + # fires; the queue-pressure signal also surfaces in metrics. + logger.warning( + "Webhook task drop: queue full for %s_%s", + task.doc_type, + task.doc_id, + ) + return JSONResponse( + {"status": "unavailable", "reason": "queue full"}, + status_code=503, + ) except Exception as e: logger.error( "Failed to queue webhook task for %s_%s: %s", diff --git a/tests/unit/test_webhook_endpoint.py b/tests/unit/test_webhook_endpoint.py index 077aad19..4704e9e9 100644 --- a/tests/unit/test_webhook_endpoint.py +++ b/tests/unit/test_webhook_endpoint.py @@ -165,6 +165,33 @@ def test_returns_500_when_stream_is_closed(): assert response.json()["status"] == "error" +def test_returns_503_when_queue_is_full(monkeypatch): + """When the processor queue is saturated, the handler must time out + quickly with 503 instead of pinning until NC's outbound timeout fires.""" + # Speed up the test β€” a 1s deadline matches production but is overkill + # for a unit test that's specifically exercising the timeout branch. + # Capture the original BEFORE patching so the override doesn't recurse + # into itself (the receiver imports the same anyio module object). + real_fail_after = anyio.fail_after + monkeypatch.setattr( + "nextcloud_mcp_server.vector.webhook_receiver.anyio.fail_after", + lambda _seconds: real_fail_after(0.05), + ) + + # Buffer of 1, no consumer β†’ first send fills it, second blocks. + send_stream, _receive_stream = anyio.create_memory_object_stream(max_buffer_size=1) + send_stream.send_nowait("sentinel") # type: ignore[arg-type] + app = _make_app(send_stream=send_stream) + + with TestClient(app) as client: + response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED) + + assert response.status_code == 503 + body = response.json() + assert body["status"] == "unavailable" + assert body["reason"] == "queue full" + + # --- WEBHOOK_SECRET authentication --------------------------------------- diff --git a/tests/unit/test_webhook_uri.py b/tests/unit/test_webhook_uri.py index d5834351..84045dcc 100644 --- a/tests/unit/test_webhook_uri.py +++ b/tests/unit/test_webhook_uri.py @@ -1,9 +1,13 @@ """Unit tests for ``_get_webhook_uri`` priority order and the -``webhook_auth_kwargs`` registration helper. +``webhook_auth_pair`` registration helper. Cloud deployments register the webhook URI returned by this function with Nextcloud. ECS Fargate also exposes ``/.dockerenv``, so an explicit public -URL must win over the docker auto-detection branch. +URL must win over the docker auto-detection branch. The URL fields are +read via dynaconf (``Settings``), so tests patch ``get_settings`` directly. +The docker-detection markers (``/.dockerenv``, ``DOCKER_CONTAINER``, +``NEXTCLOUD_MCP_SERVICE_NAME``, ``NEXTCLOUD_MCP_PORT``) remain on +``os.getenv`` and are exercised via env-var monkeypatching. """ import pytest @@ -15,9 +19,7 @@ from nextcloud_mcp_server.auth.webhook_routes import ( ) from nextcloud_mcp_server.config import Settings -ENV_VARS = ( - "WEBHOOK_INTERNAL_URL", - "NEXTCLOUD_MCP_SERVER_URL", +DOCKER_ENV_VARS = ( "NEXTCLOUD_MCP_SERVICE_NAME", "NEXTCLOUD_MCP_PORT", "DOCKER_CONTAINER", @@ -26,10 +28,21 @@ ENV_VARS = ( @pytest.fixture(autouse=True) def _clean_env(monkeypatch): - for name in ENV_VARS: + for name in DOCKER_ENV_VARS: monkeypatch.delenv(name, raising=False) +def _patch_settings(monkeypatch, **overrides) -> None: + """Make ``get_settings()`` (as called inside webhook_routes) return a + Settings instance with the given URL/secret fields set; everything else + falls back to the dataclass defaults.""" + monkeypatch.setattr( + webhook_routes, + "get_settings", + lambda: Settings(**overrides), + ) + + def _no_docker_markers(monkeypatch): monkeypatch.setattr( "nextcloud_mcp_server.auth.webhook_routes.os.path.exists", @@ -46,8 +59,11 @@ def _docker_markers(monkeypatch): @pytest.mark.unit def test_webhook_internal_url_wins_over_everything(monkeypatch): - monkeypatch.setenv("WEBHOOK_INTERNAL_URL", "https://internal.example.com") - monkeypatch.setenv("NEXTCLOUD_MCP_SERVER_URL", "https://public.example.com") + _patch_settings( + monkeypatch, + webhook_internal_url="https://internal.example.com", + nextcloud_mcp_server_url="https://public.example.com", + ) _docker_markers(monkeypatch) assert _get_webhook_uri() == "https://internal.example.com/webhooks/nextcloud" @@ -57,8 +73,9 @@ def test_webhook_internal_url_wins_over_everything(monkeypatch): def test_public_url_wins_over_docker_detection(monkeypatch): """The bug-fix case: ECS containers have /.dockerenv but a public URL is set. Docker auto-detection must NOT clobber the explicit public URL.""" - monkeypatch.setenv( - "NEXTCLOUD_MCP_SERVER_URL", "https://holy-bluegill.astrolabecloud.com" + _patch_settings( + monkeypatch, + nextcloud_mcp_server_url="https://holy-bluegill.astrolabecloud.com", ) _docker_markers(monkeypatch) @@ -72,6 +89,7 @@ def test_public_url_wins_over_docker_detection(monkeypatch): def test_docker_detection_used_when_no_public_url(monkeypatch): """docker-compose dev: no public URL set, /.dockerenv exists β†’ use the docker-compose service name.""" + _patch_settings(monkeypatch) _docker_markers(monkeypatch) assert _get_webhook_uri() == "http://mcp:8000/webhooks/nextcloud" @@ -79,6 +97,7 @@ def test_docker_detection_used_when_no_public_url(monkeypatch): @pytest.mark.unit def test_docker_detection_honors_service_name_and_port_overrides(monkeypatch): + _patch_settings(monkeypatch) monkeypatch.setenv("NEXTCLOUD_MCP_SERVICE_NAME", "mcp-login-flow") monkeypatch.setenv("NEXTCLOUD_MCP_PORT", "8004") _docker_markers(monkeypatch) @@ -88,6 +107,7 @@ def test_docker_detection_honors_service_name_and_port_overrides(monkeypatch): @pytest.mark.unit def test_docker_container_env_var_triggers_docker_branch(monkeypatch): + _patch_settings(monkeypatch) monkeypatch.setenv("DOCKER_CONTAINER", "true") _no_docker_markers(monkeypatch) @@ -96,6 +116,7 @@ def test_docker_container_env_var_triggers_docker_branch(monkeypatch): @pytest.mark.unit def test_localhost_fallback_when_nothing_set(monkeypatch): + _patch_settings(monkeypatch) _no_docker_markers(monkeypatch) assert _get_webhook_uri() == "http://localhost:8000/webhooks/nextcloud" @@ -104,23 +125,15 @@ def test_localhost_fallback_when_nothing_set(monkeypatch): # --- webhook_auth_pair() -------------------------------------------------- -def _patch_secret(monkeypatch, secret: str | None) -> None: - monkeypatch.setattr( - webhook_routes, - "get_settings", - lambda: Settings(webhook_secret=secret), - ) - - @pytest.mark.unit def test_auth_pair_returns_none_when_secret_unset(monkeypatch): - _patch_secret(monkeypatch, None) + _patch_settings(monkeypatch, webhook_secret=None) assert webhook_auth_pair() == ("none", None) @pytest.mark.unit def test_auth_pair_emits_bearer_header_when_secret_set(monkeypatch): - _patch_secret(monkeypatch, "supersecret") + _patch_settings(monkeypatch, webhook_secret="supersecret") assert webhook_auth_pair() == ( "header", {"Authorization": "Bearer supersecret"}, From 4a3857aabb74d82490160aec77a9ebe49f4f4651 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 14:17:45 +0200 Subject: [PATCH 5/6] fix(webhooks): escape HTML in error responses, compare bearer as bytes Address the two Security findings from PR review: - webhook_receiver: encode Authorization header and expected bearer to utf-8 bytes before hmac.compare_digest. Conventional form; doesn't rely on Python's implicit ASCII encoding. - webhook_routes: html.escape user-influenced and exception-derived strings before interpolating into HTMLResponse content. Covers the preset_id path param echoed in the "Unknown preset" branch and the str(e) text rendered on handler exceptions. Adds regression tests verifying compare_digest is invoked on bytes and that " + + with TestClient(app) as client: + response = client.post(f"/app/webhooks/enable/{payload}") + + assert response.status_code == 404 + assert "<script>alert(1)</script>" in response.text + assert "" not in response.text + + +def test_disable_unknown_preset_id_is_html_escaped(monkeypatch): + _stub_admin_path(monkeypatch) + monkeypatch.setattr(webhook_routes, "get_preset", lambda _id: None) + + app = _make_app() + payload = "" + + with TestClient(app) as client: + response = client.delete(f"/app/webhooks/disable/{payload}") + + assert response.status_code == 404 + assert "<script>alert(2)</script>" in response.text + assert "" not in response.text + + +def test_enable_exception_message_is_html_escaped(monkeypatch): + """If the handler raises, the exception text must be escaped before + it lands in the 500 response body.""" + + async def _boom(_request): + raise RuntimeError("

") + + monkeypatch.setattr(webhook_routes, "_get_authenticated_client", _boom) + + app = _make_app() + + with TestClient(app) as client: + response = client.post("/app/webhooks/enable/notes_sync") + + assert response.status_code == 500 + assert "</p><script>x</script>" in response.text + assert "" not in response.text + + +def test_disable_exception_message_is_html_escaped(monkeypatch): + async def _boom(_request): + raise RuntimeError("

") + + monkeypatch.setattr(webhook_routes, "_get_authenticated_client", _boom) + + app = _make_app() + + with TestClient(app) as client: + response = client.delete("/app/webhooks/disable/notes_sync") + + assert response.status_code == 500 + assert "</p><script>y</script>" in response.text + assert "" not in response.text From affe29f72e9ecdce888b59a41ac2384109d08afa Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 14:37:34 +0200 Subject: [PATCH 6/6] fix(webhooks): escape webhook_uri, lazy logging, document 401 header omission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address round-5 reviewer feedback on PR #747: - Escape `webhook_uri` in the admin pane HTML template so an operator- controlled env value (`WEBHOOK_INTERNAL_URL`, `NEXTCLOUD_MCP_SERVER_URL`) can't inject markup. The sibling `preset_id` and exception messages were already escaped β€” this one was the odd one out. - Convert the eight remaining f-string `logger.warning`/`logger.error` calls in `api/webhooks.py` to lazy `%s` formatting, matching the style already adopted by `webhook_receiver.py` and `webhook_routes.py`. - Document why the 401 from `handle_nextcloud_webhook` deliberately omits `WWW-Authenticate`: NC's webhook delivery worker has no auth-flow state machine to negotiate against, the bearer is a static shared secret configured out-of-band via `WEBHOOK_SECRET`, and a challenge response wouldn't change client behaviour. The existing warning log already records the rejection. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/webhooks.py | 16 ++++++++-------- nextcloud_mcp_server/auth/webhook_routes.py | 2 +- nextcloud_mcp_server/vector/webhook_receiver.py | 7 +++++++ 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/nextcloud_mcp_server/api/webhooks.py b/nextcloud_mcp_server/api/webhooks.py index e042164b..3f9a3a28 100644 --- a/nextcloud_mcp_server/api/webhooks.py +++ b/nextcloud_mcp_server/api/webhooks.py @@ -37,7 +37,7 @@ async def get_installed_apps(request: Request) -> JSONResponse: # Validate OAuth token and extract user user_id, validated = await validate_token_and_get_user(request) except Exception as e: - logger.warning(f"Unauthorized access to /api/v1/apps: {e}") + logger.warning("Unauthorized access to /api/v1/apps: %s", e) return JSONResponse( { "error": "Unauthorized", @@ -86,7 +86,7 @@ async def get_installed_apps(request: Request) -> JSONResponse: return JSONResponse({"apps": apps}) except Exception as e: - logger.error(f"Error getting installed apps for user {user_id}: {e}") + logger.error("Error getting installed apps for user %s: %s", user_id, e) return JSONResponse( { "error": "Internal error", @@ -107,7 +107,7 @@ async def list_webhooks(request: Request) -> JSONResponse: # Validate OAuth token and extract user user_id, validated = await validate_token_and_get_user(request) except Exception as e: - logger.warning(f"Unauthorized access to /api/v1/webhooks: {e}") + logger.warning("Unauthorized access to /api/v1/webhooks: %s", e) return JSONResponse( { "error": "Unauthorized", @@ -142,7 +142,7 @@ async def list_webhooks(request: Request) -> JSONResponse: return JSONResponse({"webhooks": webhooks}) except Exception as e: - logger.error(f"Error listing webhooks for user {user_id}: {e}") + logger.error("Error listing webhooks for user %s: %s", user_id, e) return JSONResponse( { "error": "Internal error", @@ -170,7 +170,7 @@ async def create_webhook(request: Request) -> JSONResponse: # Validate OAuth token and extract user user_id, validated = await validate_token_and_get_user(request) except Exception as e: - logger.warning(f"Unauthorized access to /api/v1/webhooks: {e}") + logger.warning("Unauthorized access to /api/v1/webhooks: %s", e) return JSONResponse( { "error": "Unauthorized", @@ -229,7 +229,7 @@ async def create_webhook(request: Request) -> JSONResponse: return JSONResponse({"webhook": webhook_data}) except Exception as e: - logger.error(f"Error creating webhook for user {user_id}: {e}") + logger.error("Error creating webhook for user %s: %s", user_id, e) return JSONResponse( { "error": "Internal error", @@ -250,7 +250,7 @@ async def delete_webhook(request: Request) -> JSONResponse: # Validate OAuth token and extract user user_id, validated = await validate_token_and_get_user(request) except Exception as e: - logger.warning(f"Unauthorized access to /api/v1/webhooks: {e}") + logger.warning("Unauthorized access to /api/v1/webhooks: %s", e) return JSONResponse( { "error": "Unauthorized", @@ -301,7 +301,7 @@ async def delete_webhook(request: Request) -> JSONResponse: return JSONResponse({"success": True, "message": "Webhook deleted"}) except Exception as e: - logger.error(f"Error deleting webhook for user {user_id}: {e}") + logger.error("Error deleting webhook for user %s: %s", user_id, e) return JSONResponse( { "error": "Internal error", diff --git a/nextcloud_mcp_server/auth/webhook_routes.py b/nextcloud_mcp_server/auth/webhook_routes.py index 360a101d..6e04031a 100644 --- a/nextcloud_mcp_server/auth/webhook_routes.py +++ b/nextcloud_mcp_server/auth/webhook_routes.py @@ -394,7 +394,7 @@ async def webhook_management_pane(request: Request) -> HTMLResponse:

About Webhooks

Webhooks enable real-time synchronization by notifying this server when content changes in Nextcloud.

-

Endpoint: {webhook_uri}

+

Endpoint: {html.escape(webhook_uri)}

Available Presets

diff --git a/nextcloud_mcp_server/vector/webhook_receiver.py b/nextcloud_mcp_server/vector/webhook_receiver.py index 1c25c19b..d55d666c 100644 --- a/nextcloud_mcp_server/vector/webhook_receiver.py +++ b/nextcloud_mcp_server/vector/webhook_receiver.py @@ -60,6 +60,13 @@ async def handle_nextcloud_webhook(request: Request) -> JSONResponse: # for differing lengths but isn't fully constant-time across them; # that's fine here β€” a secret length leak is not a sensitive signal. if not hmac.compare_digest(provided, expected): + # Intentionally omit WWW-Authenticate. RFC 7235 Β§4.1 says a 401 + # SHOULD carry it, but Nextcloud's webhook delivery worker has no + # auth-flow state machine to negotiate against β€” the bearer is a + # static shared secret configured out-of-band via WEBHOOK_SECRET, + # and a challenge response wouldn't change client behaviour. + # Surfacing it would only mislead operators into expecting a + # renegotiation that doesn't exist. logger.warning("Webhook rejected: missing or invalid Authorization header") return JSONResponse( {"status": "unauthorized"},