From 224428fca524ebd6e24ddbb77dade983bd532dd7 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 30 Apr 2026 03:49:50 +0200 Subject: [PATCH] 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"}, + )