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]