diff --git a/docker-compose.yml b/docker-compose.yml index 5290cfdb..293c6428 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -101,6 +101,9 @@ services: - ENABLE_SEMANTIC_SEARCH=true - VECTOR_SYNC_SCAN_INTERVAL=5 - VECTOR_SYNC_PROCESSOR_WORKERS=2 + # Required to enable the /webhooks/nextcloud receiver (GHSA-8vh3-g2qg-2h2c). + # Without it the route is not mounted and vector sync relies on polling. + - WEBHOOK_SECRET=${WEBHOOK_SECRET:-dev-webhook-secret-change-me} #- LOG_FORMAT=json @@ -160,6 +163,9 @@ services: - TOKEN_STORAGE_DB=/app/data/tokens.db - ENABLE_SEMANTIC_SEARCH=true + # Required to enable the /webhooks/nextcloud receiver (GHSA-8vh3-g2qg-2h2c). + # Without it the route is not mounted and vector sync relies on polling. + - WEBHOOK_SECRET=${WEBHOOK_SECRET:-dev-webhook-secret-change-me} # Tuned cadence for the multi-user background-sync integration suite, # which provisions a user, creates a note, then waits ~90s for it to be # indexed. Two independent knobs matter here: @@ -307,6 +313,9 @@ services: - ENABLE_SEMANTIC_SEARCH=true - VECTOR_SYNC_SCAN_INTERVAL=60 - VECTOR_SYNC_PROCESSOR_WORKERS=1 + # Required to enable the /webhooks/nextcloud receiver (GHSA-8vh3-g2qg-2h2c). + # Without it the route is not mounted and vector sync relies on polling. + - WEBHOOK_SECRET=${WEBHOOK_SECRET:-dev-webhook-secret-change-me} # Management API allowlist (ADR-018) — matches mcp-multi-user-basic so # the same configure_astrolabe_for_mcp_server fixture (which creates the diff --git a/docs/ADR-010-webhook-based-vector-sync.md b/docs/ADR-010-webhook-based-vector-sync.md index 921c3248..bc377609 100644 --- a/docs/ADR-010-webhook-based-vector-sync.md +++ b/docs/ADR-010-webhook-based-vector-sync.md @@ -249,16 +249,24 @@ This design keeps concerns separated: webhooks and scanner are independent produ ### Configuration -A new optional environment variable controls webhook authentication: +A **required** environment variable controls webhook authentication: ```bash -# Optional: Shared secret for webhook authentication -# If set, webhooks must include "Authorization: Bearer " header -# If unset, no authentication is required (useful for local development) +# REQUIRED for webhooks: shared secret for webhook authentication. +# Webhooks must include "Authorization: Bearer " header. WEBHOOK_SECRET= ``` -The webhook endpoint is automatically available at `/webhooks/nextcloud` when the MCP server starts. No feature flags or additional configuration needed—if Nextcloud sends webhooks to this endpoint, they will be processed. +> **Security (GHSA-8vh3-g2qg-2h2c).** The receiver trusts the `user.uid` in the +> payload and feeds it to Qdrant, so an unauthenticated POST could delete or +> re-index any user's embeddings. `WEBHOOK_SECRET` is therefore mandatory: +> when it is **unset**, the `/webhooks/nextcloud` route is **not mounted** (it +> returns 404) and the receiver refuses any request that reaches it (503). +> Webhook registration likewise refuses to create unauthenticated deliveries. +> Vector sync still works in this state via the polling scanner — webhooks are +> simply disabled until a secret is configured. + +The webhook endpoint is available at `/webhooks/nextcloud` only when `WEBHOOK_SECRET` is set. When configured, Nextcloud must forward the `Authorization: Bearer ` header (registration injects it automatically) on every delivery; requests without a valid header are rejected with 401. **Reducing Polling Frequency**: Administrators who configure webhooks may want to reduce polling frequency to minimize API load while maintaining safety reconciliation scans: @@ -296,11 +304,13 @@ Administrators who want to enable webhooks: - Endpoint: `https://:/webhooks/nextcloud` - Events: File created/updated/deleted, Calendar object events, Table row events - Filters: Exclude non-content files (images, videos), system directories - - Optional: Configure `Authorization: Bearer ` header + - Required: Configure `Authorization: Bearer ` header (the + MCP server's registration endpoints inject this automatically once + `WEBHOOK_SECRET` is set) 3. **Optionally reduce scanner frequency**: Set `VECTOR_SYNC_SCAN_INTERVAL=86400` (24 hours) 4. **Set up webhook workers** (optional): Configure dedicated background job workers for low-latency delivery -Existing deployments continue using polling without any changes. Webhooks are purely additive. +Deployments without `WEBHOOK_SECRET` continue using polling without any changes — the webhook route is simply not mounted. Webhooks are additive but require a configured secret (GHSA-8vh3-g2qg-2h2c). ## Consequences @@ -346,7 +356,7 @@ Logs include: ### Security Considerations -**Optional Authentication**: When `WEBHOOK_SECRET` is configured, webhook requests must include `Authorization: Bearer ` header. The server validates this before processing to prevent unauthorized document queueing. For local development, authentication can be disabled by leaving `WEBHOOK_SECRET` unset. +**Required Authentication (GHSA-8vh3-g2qg-2h2c)**: The receiver trusts the `user.uid` in the payload and feeds it to Qdrant, so unauthenticated access would let any network caller delete or re-index other users' embeddings. `WEBHOOK_SECRET` is therefore mandatory for webhooks: when it is unset the `/webhooks/nextcloud` route is not mounted (404) and the handler refuses any request that reaches it (503). When set, every request must include `Authorization: Bearer `; the server validates it before any processing and rejects mismatches with 401. There is no unauthenticated mode — local development that needs webhooks must also set a secret. **Payload Validation**: Webhook payloads are parsed and validated against expected schemas. Malformed payloads are rejected with 400 Bad Request responses. @@ -375,7 +385,7 @@ async def test_webhook_endpoint_parses_note_created_event(): 1. **Mock webhook delivery**: POST webhook payloads directly to the `/webhooks/nextcloud` endpoint 2. **Verify processing**: Check that documents are queued and eventually appear in Qdrant -3. **Test authentication**: Verify requests without valid auth header are rejected (when `WEBHOOK_SECRET` is set) +3. **Test authentication**: Verify requests without a valid auth header are rejected (401), and that with no `WEBHOOK_SECRET` the route is absent / the handler returns 503 ```python async def test_webhook_integration_mocked_delivery(): diff --git a/docs/ADR-018-nextcloud-php-app-for-settings-ui.md b/docs/ADR-018-nextcloud-php-app-for-settings-ui.md index decb7a8c..b27ff325 100644 --- a/docs/ADR-018-nextcloud-php-app-for-settings-ui.md +++ b/docs/ADR-018-nextcloud-php-app-for-settings-ui.md @@ -552,6 +552,10 @@ async def get_server_status(request: Request) -> JSONResponse: "version": __version__, "auth_mode": "oauth" if settings.enable_oauth else "basic", "vector_sync_enabled": settings.vector_sync_enabled, + # Whether the /webhooks/nextcloud receiver is active (gated on + # WEBHOOK_SECRET — GHSA-8vh3-g2qg-2h2c). Lets the UI show webhook sync + # as available/unavailable. + "webhooks_enabled": bool(settings.webhook_secret), "uptime_seconds": get_uptime(), "management_api_version": "v1", }) diff --git a/docs/webhook-management-guide.md b/docs/webhook-management-guide.md index cdc2cb9e..4490b8bf 100644 --- a/docs/webhook-management-guide.md +++ b/docs/webhook-management-guide.md @@ -246,14 +246,18 @@ php occ webhook_listeners:add --event "OCA\Tables\Event\RowDeletedEvent" --uri " ## Security Considerations -### Webhook Authentication -Configure `WEBHOOK_SECRET` to require authentication for incoming webhooks: +### Webhook Authentication (required — GHSA-8vh3-g2qg-2h2c) +`WEBHOOK_SECRET` is **required** to use webhooks. The receiver trusts the +`user.uid` in the payload and feeds it to Qdrant, so an unauthenticated POST +could delete or re-index any user's embeddings. When `WEBHOOK_SECRET` is unset, +the `/webhooks/nextcloud` route is **not mounted** (404) and registration +refuses to create webhooks; vector sync still runs via the polling scanner. ```bash -# MCP Server +# MCP Server (generate with: python -c "import secrets; print(secrets.token_urlsafe(32))") WEBHOOK_SECRET= -# Nextcloud webhook registration +# Nextcloud webhook registration — the Authorization header is mandatory php occ webhook_listeners:add \ --event "..." \ --uri "$MCP_URL/webhooks/nextcloud" \ diff --git a/env.sample b/env.sample index 906ab3f2..75d2cf5e 100644 --- a/env.sample +++ b/env.sample @@ -189,6 +189,15 @@ NEXTCLOUD_PASSWORD= # # Max queued documents (default: 10000) #VECTOR_SYNC_QUEUE_MAX_SIZE=10000 +# +# Webhook receiver authentication (REQUIRED for webhook-driven sync). +# Security (GHSA-8vh3-g2qg-2h2c): the /webhooks/nextcloud receiver trusts the +# user id in the payload and feeds it to Qdrant, so it must be authenticated. +# When WEBHOOK_SECRET is unset the route is NOT mounted and vector sync falls +# back to the polling scanner. When set, the receiver requires +# `Authorization: Bearer ` and webhook registration injects it. +# Generate one with: python -c "import secrets; print(secrets.token_urlsafe(32))" +#WEBHOOK_SECRET= # ===== DOCUMENT PROCESSING ===== # Extract text from PDFs, images, DOCX, etc. for semantic search diff --git a/nextcloud_mcp_server/api/management.py b/nextcloud_mcp_server/api/management.py index ee12ac2a..585cae03 100644 --- a/nextcloud_mcp_server/api/management.py +++ b/nextcloud_mcp_server/api/management.py @@ -243,6 +243,11 @@ async def get_server_status(request: Request) -> JSONResponse: "version": __version__, "auth_mode": auth_mode, "vector_sync_enabled": settings.vector_sync_enabled, + # Whether the /webhooks/nextcloud receiver is active. Gated on + # WEBHOOK_SECRET (GHSA-8vh3-g2qg-2h2c): without a secret the route is + # not mounted, so the Astrolabe UI can show webhooks as unavailable and + # vector sync falls back to the polling scanner. + "webhooks_enabled": bool(settings.webhook_secret), "uptime_seconds": uptime_seconds, "management_api_version": "1.0", } diff --git a/nextcloud_mcp_server/api/webhooks.py b/nextcloud_mcp_server/api/webhooks.py index dbcdaa1f..0e40b346 100644 --- a/nextcloud_mcp_server/api/webhooks.py +++ b/nextcloud_mcp_server/api/webhooks.py @@ -28,7 +28,10 @@ from nextcloud_mcp_server.api.management import ( validate_token_and_get_user, ) from nextcloud_mcp_server.auth.scope_authorization import ProvisioningRequiredError -from nextcloud_mcp_server.auth.webhook_routes import webhook_auth_pair +from nextcloud_mcp_server.auth.webhook_routes import ( + WebhookSecretNotConfigured, + webhook_auth_pair, +) from nextcloud_mcp_server.client.webhooks import WebhooksClient from ..http import nextcloud_httpx_client @@ -217,7 +220,19 @@ async def create_webhook(request: Request) -> JSONResponse: # Inject delivery auth headers when WEBHOOK_SECRET is configured so # that webhook deliveries from Nextcloud back to us are authenticated. webhooks_client = WebhooksClient(client, username) - auth_method, auth_data = webhook_auth_pair() + try: + auth_method, auth_data = webhook_auth_pair() + except WebhookSecretNotConfigured as e: + logger.warning( + "Webhook registration refused for user %s: %s", user_id, e + ) + return JSONResponse( + { + "error": "Webhooks disabled", + "message": str(e), + }, + status_code=503, + ) webhook_data = await webhooks_client.create_webhook( event=event, uri=uri, diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 510d80cf..6168fbc2 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -2321,10 +2321,23 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = # 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("Webhook endpoint enabled: /webhooks/nextcloud") + # + # Security (GHSA-8vh3-g2qg-2h2c): the receiver trusts the attacker-supplied + # user.uid in the payload and feeds it to Qdrant, so an unauthenticated + # POST could delete/re-index any user's embeddings. The route is therefore + # only mounted when WEBHOOK_SECRET is configured; without it the webhook + # feature is off and vector sync still reconciles via the polling scanner. + if settings.webhook_secret: + routes.append( + Route("/webhooks/nextcloud", handle_nextcloud_webhook, methods=["POST"]) + ) + logger.info("Webhook endpoint enabled: /webhooks/nextcloud") + else: + logger.warning( + "Webhook endpoint disabled: WEBHOOK_SECRET is not set. " + "/webhooks/nextcloud will return 404; vector sync relies on the " + "polling scanner. Set WEBHOOK_SECRET to enable webhook-driven sync." + ) # 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 6e04031a..a4d8a876 100644 --- a/nextcloud_mcp_server/auth/webhook_routes.py +++ b/nextcloud_mcp_server/auth/webhook_routes.py @@ -125,21 +125,40 @@ def _get_webhook_uri() -> str: return "http://localhost:8000/webhooks/nextcloud" -def webhook_auth_pair() -> tuple[str, dict[str, str] | None]: +class WebhookSecretNotConfigured(RuntimeError): + """Raised when a webhook registration is attempted without WEBHOOK_SECRET. + + Webhooks require ``WEBHOOK_SECRET`` (GHSA-8vh3-g2qg-2h2c): the receiver + route is not mounted without it, so registering a webhook would only create + an unauthenticated delivery target pointing at a non-existent endpoint. + """ + + +def webhook_auth_pair() -> tuple[str, dict[str, str]]: """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. + Returns ``("header", {"Authorization": f"Bearer {secret}"})`` so NC stores + the credential encrypted at-rest and forwards it on every delivery. + + ``WEBHOOK_SECRET`` is required (GHSA-8vh3-g2qg-2h2c): the receiver refuses + unauthenticated deliveries and ``app.py`` does not mount the route without + a secret, so registering an ``authMethod="none"`` webhook would only create + a dead, unauthenticated delivery target. Callers must surface + :class:`WebhookSecretNotConfigured` as a clear operator-facing error. Shared by both registration call sites: the ``/app/webhooks`` preset flow and the Astrolabe-facing ``/api/v1/webhooks`` endpoint. + + Raises: + WebhookSecretNotConfigured: when ``WEBHOOK_SECRET`` is unset. """ secret = get_settings().webhook_secret if not secret: - return ("none", None) + raise WebhookSecretNotConfigured( + "WEBHOOK_SECRET must be set to register webhooks. Without it the " + "/webhooks/nextcloud receiver is disabled and any registration " + "would point at a non-existent, unauthenticated endpoint." + ) return ("header", {"Authorization": f"Bearer {secret}"}) @@ -151,13 +170,16 @@ async def _register_preset_webhooks( """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). + :func:`webhook_auth_pair` onto each registration call so deliveries carry + the configured ``Authorization`` header. ``WEBHOOK_SECRET`` is required + (GHSA-8vh3-g2qg-2h2c): with no secret this raises + :class:`WebhookSecretNotConfigured` before any webhook is created. Extracted from :func:`enable_webhook_preset` so the auth-threading behaviour is testable without standing up a Starlette app. + + Raises: + WebhookSecretNotConfigured: when ``WEBHOOK_SECRET`` is unset. """ auth_method, auth_data = webhook_auth_pair() registered_ids: list[int] = [] @@ -498,6 +520,16 @@ async def enable_webhook_preset(request: Request) -> HTMLResponse: """ ) + except WebhookSecretNotConfigured as e: + logger.warning("Refusing to enable preset %s: %s", preset_id, e) + return HTMLResponse( + content=( + '
Webhooks are disabled: WEBHOOK_SECRET is ' + "not set. Configure it and restart the server to enable webhook " + "presets.
" + ), + status_code=503, + ) except Exception as e: logger.error("Failed to enable preset %s: %s", preset_id, e, exc_info=True) return HTMLResponse( diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index d97d508e..73cccf6e 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -722,11 +722,15 @@ 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 delivery authentication (ADR-010). REQUIRED for webhooks + # (GHSA-8vh3-g2qg-2h2c). When set, the registrar passes + # Authorization: Bearer as the webhook authData and the receiver + # validates the same header on each delivery. When unset, the + # /webhooks/nextcloud route is not mounted, the receiver refuses any request + # that reaches it (503), and registration refuses to create webhooks — the + # receiver trusts user.uid from the payload, so unauthenticated access would + # let any caller delete/re-index other users' embeddings. Vector sync still + # works via the polling scanner when this is unset. webhook_secret: str | None = None # Internal URL override for webhook registration. Highest-priority # source for the URL we register with NC (above diff --git a/nextcloud_mcp_server/vector/webhook_receiver.py b/nextcloud_mcp_server/vector/webhook_receiver.py index d2a95df6..d2e05042 100644 --- a/nextcloud_mcp_server/vector/webhook_receiver.py +++ b/nextcloud_mcp_server/vector/webhook_receiver.py @@ -23,18 +23,19 @@ _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. + The receiver refuses unauthenticated POSTs in this case (returns 503), and + ``app.py`` does not even mount the route without a secret — this branch is + defense-in-depth for any caller that wires the handler directly. The + operator should set WEBHOOK_SECRET to enable webhook-driven vector sync. """ 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." + "WEBHOOK_SECRET is not set; /webhooks/nextcloud rejects all requests " + "(503). Set WEBHOOK_SECRET and re-register webhooks to enable " + "Authorization: Bearer validation and webhook-driven vector sync." ) @@ -47,35 +48,50 @@ async def handle_nextcloud_webhook(request: Request) -> JSONResponse: ``INGEST_QUEUE=postgres``); 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. + ``WEBHOOK_SECRET`` is **required** (GHSA-8vh3-g2qg-2h2c). The endpoint is + only mounted by ``app.py`` when the secret is configured, and every request + must carry ``Authorization: Bearer `` (registered via ``authData`` + so NC forwards it on every delivery). The ``user.uid`` in the payload is + attacker-controllable and is fed to Qdrant, so an unauthenticated request + could delete or re-index any user's embeddings — never process one. Missing + or invalid headers are rejected with 401 before any further work; if the + secret is somehow unset we refuse with 503 rather than fall back to + unauthenticated processing. """ secret = get_settings().webhook_secret - if secret: - provided = request.headers.get("authorization", "").encode("utf-8") - expected = f"Bearer {secret}".encode("utf-8") - # Use compare_digest to avoid the character-by-character short-circuit - # of `==`. Comparing as bytes is the conventional form and avoids any - # surprise with non-ASCII input. 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): - # 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"}, - status_code=401, - ) - else: + if not secret: + # Defense-in-depth: the route should not be mounted without a secret, + # but if this handler is reached anyway, refuse rather than process an + # unauthenticated, attacker-controlled payload. _warn_missing_secret_once() + return JSONResponse( + { + "status": "unavailable", + "reason": "webhook authentication not configured", + }, + status_code=503, + ) + + provided = request.headers.get("authorization", "").encode("utf-8") + expected = f"Bearer {secret}".encode("utf-8") + # Use compare_digest to avoid the character-by-character short-circuit + # of `==`. Comparing as bytes is the conventional form and avoids any + # surprise with non-ASCII input. 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): + # 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"}, + status_code=401, + ) try: payload = await request.json() diff --git a/tests/unit/test_management_status_endpoint.py b/tests/unit/test_management_status_endpoint.py index 313a12a7..fcf967d8 100644 --- a/tests/unit/test_management_status_endpoint.py +++ b/tests/unit/test_management_status_endpoint.py @@ -36,6 +36,7 @@ def create_mock_settings( oidc_discovery_url: str | None = None, oidc_issuer: str | None = None, vector_sync_enabled: bool = False, + webhook_secret: str | None = None, nextcloud_url: str = "http://localhost", mcp_client_id: str | None = None, mcp_client_secret: str | None = None, @@ -47,6 +48,9 @@ def create_mock_settings( settings.oidc_discovery_url = oidc_discovery_url settings.oidc_issuer = oidc_issuer settings.vector_sync_enabled = vector_sync_enabled + # Explicit so bool(settings.webhook_secret) is deterministic (a bare + # MagicMock attribute is truthy, which would always report webhooks on). + settings.webhook_secret = webhook_secret settings.nextcloud_url = nextcloud_url settings.mcp_client_id = mcp_client_id settings.mcp_client_secret = mcp_client_secret @@ -342,3 +346,48 @@ class TestStatusEndpointBasicResponse: data = response.json() assert data["vector_sync_enabled"] is True + + def test_status_reports_webhooks_enabled_when_secret_set(self): + """webhooks_enabled is True when WEBHOOK_SECRET is configured.""" + mock_settings = create_mock_settings(webhook_secret="supersecret") + + with ( + patch( + "nextcloud_mcp_server.api.management.get_settings", + return_value=mock_settings, + ), + patch( + "nextcloud_mcp_server.api.management.detect_auth_mode", + return_value=AuthMode.SINGLE_USER_BASIC, + ), + ): + app = create_test_app() + client = TestClient(app) + response = client.get("/api/v1/status") + + assert response.status_code == 200 + assert response.json()["webhooks_enabled"] is True + + def test_status_reports_webhooks_disabled_when_secret_unset(self): + """webhooks_enabled is False when WEBHOOK_SECRET is unset (default). + + Security (GHSA-8vh3-g2qg-2h2c): the receiver route is not mounted + without a secret, so the Astrolabe UI can surface webhooks as off.""" + mock_settings = create_mock_settings(webhook_secret=None) + + with ( + patch( + "nextcloud_mcp_server.api.management.get_settings", + return_value=mock_settings, + ), + patch( + "nextcloud_mcp_server.api.management.detect_auth_mode", + return_value=AuthMode.SINGLE_USER_BASIC, + ), + ): + app = create_test_app() + client = TestClient(app) + response = client.get("/api/v1/status") + + assert response.status_code == 200 + assert response.json()["webhooks_enabled"] is False diff --git a/tests/unit/test_webhook_endpoint.py b/tests/unit/test_webhook_endpoint.py index 42b396e3..fe967941 100644 --- a/tests/unit/test_webhook_endpoint.py +++ b/tests/unit/test_webhook_endpoint.py @@ -16,6 +16,13 @@ from nextcloud_mcp_server.vector.webhook_receiver import handle_nextcloud_webhoo pytestmark = pytest.mark.unit +# WEBHOOK_SECRET is required (GHSA-8vh3-g2qg-2h2c): the receiver rejects any +# request without a matching bearer. The functional tests below exercise +# parsing/queueing, so they run with a secret configured (via the autouse +# fixture) and send the matching header (via ``_client``). +_TEST_SECRET = "testsecret" +_AUTH = {"Authorization": f"Bearer {_TEST_SECRET}"} + @pytest.fixture(autouse=True) def _reset_warned_flag(): @@ -26,6 +33,18 @@ def _reset_warned_flag(): webhook_receiver._warned_about_missing_secret = False +@pytest.fixture(autouse=True) +def _default_secret(monkeypatch): + """Default every test to a configured WEBHOOK_SECRET. Auth-specific tests + override this by calling ``_patch_secret`` in the test body (last patch + wins).""" + monkeypatch.setattr( + webhook_receiver, + "get_settings", + lambda: Settings(webhook_secret=_TEST_SECRET), + ) + + 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``.""" @@ -36,6 +55,13 @@ def _patch_secret(monkeypatch, secret: str | None) -> None: ) +def _client(app) -> TestClient: + """TestClient that sends the matching bearer by default. Per-request + ``headers=`` still override it (httpx request headers win over client + headers), so auth tests can send a wrong/absent token.""" + return TestClient(app, headers=_AUTH) + + def _make_app(send_stream=None) -> Starlette: app = Starlette( routes=[ @@ -138,7 +164,7 @@ 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: + with _client(app) as client: response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED) assert response.status_code == 200 @@ -157,7 +183,7 @@ 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: + with _client(app) as client: response = client.post("/webhooks/nextcloud", json=_NOTE_DELETED) assert response.status_code == 200 @@ -182,7 +208,7 @@ def test_unsupported_event_is_ignored(): }, } - with TestClient(app) as client: + with _client(app) as client: response = client.post("/webhooks/nextcloud", json=payload) assert response.status_code == 200 @@ -196,7 +222,7 @@ def test_deck_card_created_queues_index_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: + with _client(app) as client: response = client.post("/webhooks/nextcloud", json=_DECK_CARD_CREATED) assert response.status_code == 200 @@ -216,7 +242,7 @@ def test_deck_card_deleted_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: + with _client(app) as client: response = client.post("/webhooks/nextcloud", json=_DECK_CARD_DELETED) assert response.status_code == 200 @@ -235,7 +261,7 @@ def test_deck_board_updated_is_ignored(): 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: + with _client(app) as client: response = client.post("/webhooks/nextcloud", json=_DECK_BOARD_UPDATED) assert response.status_code == 200 @@ -251,7 +277,7 @@ def test_deck_card_missing_id_is_ignored(): 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: + with _client(app) as client: response = client.post( "/webhooks/nextcloud", json=_DECK_CARD_CREATED_MISSING_ID ) @@ -269,7 +295,7 @@ def test_note_missing_node_id_is_ignored(): 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: + with _client(app) as client: response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED_MISSING_ID) assert response.status_code == 200 @@ -282,7 +308,7 @@ def test_note_missing_node_id_is_ignored(): def test_invalid_json_returns_400(): app = _make_app(send_stream=None) - with TestClient(app) as client: + with _client(app) as client: response = client.post( "/webhooks/nextcloud", content=b"not json", @@ -298,7 +324,7 @@ def test_returns_503_when_send_stream_not_wired(): event.""" app = _make_app(send_stream=None) - with TestClient(app) as client: + with _client(app) as client: response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED) assert response.status_code == 503 @@ -310,7 +336,7 @@ def test_returns_500_when_stream_is_closed(): receive_stream.close() # close receiver → send raises BrokenResourceError app = _make_app(send_stream=send_stream) - with TestClient(app) as client: + with _client(app) as client: response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED) assert response.status_code == 500 @@ -335,7 +361,7 @@ def test_returns_503_when_queue_is_full(monkeypatch): send_stream.send_nowait("sentinel") # type: ignore[arg-type] app = _make_app(send_stream=send_stream) - with TestClient(app) as client: + with _client(app) as client: response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED) assert response.status_code == 503 @@ -352,7 +378,7 @@ def test_secret_set_valid_bearer_header_queues_task(monkeypatch): 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: + with _client(app) as client: response = client.post( "/webhooks/nextcloud", json=_NOTE_CREATED, @@ -369,6 +395,7 @@ def test_secret_set_missing_authorization_returns_401(monkeypatch): send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=4) app = _make_app(send_stream=send_stream) + # Bare client (no default auth header) so the request truly omits it. with TestClient(app) as client: response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED) @@ -383,7 +410,7 @@ def test_secret_set_wrong_secret_returns_401(monkeypatch): 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: + with _client(app) as client: response = client.post( "/webhooks/nextcloud", json=_NOTE_CREATED, @@ -401,7 +428,7 @@ def test_secret_set_wrong_scheme_returns_401(monkeypatch): 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: + with _client(app) as client: response = client.post( "/webhooks/nextcloud", json=_NOTE_CREATED, @@ -411,19 +438,22 @@ def test_secret_set_wrong_scheme_returns_401(monkeypatch): 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.""" +def test_secret_unset_rejects_with_503(monkeypatch): + """Security (GHSA-8vh3-g2qg-2h2c): when WEBHOOK_SECRET is unset the receiver + refuses to process the (attacker-controllable) payload. ``app.py`` does not + even mount the route in this case; this exercises the handler's + defense-in-depth branch and proves no task is queued.""" _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: + with _client(app) as client: response = client.post("/webhooks/nextcloud", json=_NOTE_CREATED) - assert response.status_code == 200 - assert receive_stream.receive_nowait().doc_id == "437" + assert response.status_code == 503 + assert response.json()["status"] == "unavailable" + with pytest.raises(anyio.WouldBlock): + receive_stream.receive_nowait() def test_compare_digest_is_called_with_bytes(monkeypatch, mocker): @@ -436,7 +466,7 @@ def test_compare_digest_is_called_with_bytes(monkeypatch, mocker): send_stream, _receive = anyio.create_memory_object_stream(max_buffer_size=4) app = _make_app(send_stream=send_stream) - with TestClient(app) as client: + with _client(app) as client: response = client.post( "/webhooks/nextcloud", json=_NOTE_CREATED, diff --git a/tests/unit/test_webhook_routes_register.py b/tests/unit/test_webhook_routes_register.py index ae7568f0..9470fbb1 100644 --- a/tests/unit/test_webhook_routes_register.py +++ b/tests/unit/test_webhook_routes_register.py @@ -9,7 +9,10 @@ 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.auth.webhook_routes import ( + WebhookSecretNotConfigured, + _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 @@ -58,23 +61,25 @@ async def test_register_threads_bearer_auth_when_secret_set(monkeypatch, mocker) assert kwargs["event_filter"] == event_config["filter"] -async def test_register_uses_none_auth_when_secret_unset(monkeypatch, mocker): +async def test_register_refuses_when_secret_unset(monkeypatch, mocker): + """Security (GHSA-8vh3-g2qg-2h2c): webhooks require WEBHOOK_SECRET. + Without it, registration raises instead of creating a dead, unauthenticated + (``authMethod="none"``) delivery target pointing at a disabled receiver.""" _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" - ) + with pytest.raises(WebhookSecretNotConfigured): + 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 + client.create_webhook.assert_not_called() async def test_register_returns_ids_in_call_order(monkeypatch, mocker): - _patch_secret(monkeypatch, None) + _patch_secret(monkeypatch, "supersecret") preset = get_preset("notes_sync") assert preset is not None client = _make_webhooks_client(mocker, ids=[42, 43, 44]) diff --git a/tests/unit/test_webhook_uri.py b/tests/unit/test_webhook_uri.py index 84045dcc..92951029 100644 --- a/tests/unit/test_webhook_uri.py +++ b/tests/unit/test_webhook_uri.py @@ -14,6 +14,7 @@ import pytest from nextcloud_mcp_server.auth import webhook_routes from nextcloud_mcp_server.auth.webhook_routes import ( + WebhookSecretNotConfigured, _get_webhook_uri, webhook_auth_pair, ) @@ -126,9 +127,12 @@ def test_localhost_fallback_when_nothing_set(monkeypatch): @pytest.mark.unit -def test_auth_pair_returns_none_when_secret_unset(monkeypatch): +def test_auth_pair_raises_when_secret_unset(monkeypatch): + """Security (GHSA-8vh3-g2qg-2h2c): no secret => no webhook registration. + The helper raises instead of returning an ``authMethod="none"`` pair.""" _patch_settings(monkeypatch, webhook_secret=None) - assert webhook_auth_pair() == ("none", None) + with pytest.raises(WebhookSecretNotConfigured): + webhook_auth_pair() @pytest.mark.unit diff --git a/tests/unit/test_webhooks_api_auth.py b/tests/unit/test_webhooks_api_auth.py index 9f1e70f4..6c653a4f 100644 --- a/tests/unit/test_webhooks_api_auth.py +++ b/tests/unit/test_webhooks_api_auth.py @@ -24,6 +24,7 @@ from nextcloud_mcp_server.api.webhooks import ( list_webhooks, ) from nextcloud_mcp_server.auth.scope_authorization import ProvisioningRequiredError +from nextcloud_mcp_server.auth.webhook_routes import WebhookSecretNotConfigured pytestmark = pytest.mark.unit @@ -145,7 +146,7 @@ async def test_create_webhook_uses_basic_auth(mocker): ) mocker.patch( "nextcloud_mcp_server.api.webhooks.webhook_auth_pair", - return_value=("none", None), + return_value=("header", {"Authorization": "Bearer supersecret"}), ) client = TestClient(_build_test_app()) @@ -163,6 +164,31 @@ async def test_create_webhook_uses_basic_auth(mocker): _assert_basic_auth_not_bearer(factory) +async def test_create_webhook_returns_503_when_secret_unset(mocker): + """Security (GHSA-8vh3-g2qg-2h2c): registration is refused without a + WEBHOOK_SECRET so no unauthenticated delivery target is created.""" + _patch_token_validation(mocker) + _patch_basic_auth(mocker, username="bob", app_password="bob-pwd") + _patch_outbound_client_factory(mocker) + mocker.patch( + "nextcloud_mcp_server.api.webhooks.webhook_auth_pair", + side_effect=WebhookSecretNotConfigured("WEBHOOK_SECRET must be set"), + ) + + client = TestClient(_build_test_app()) + resp = client.post( + "/api/v1/webhooks", + headers={"Authorization": "Bearer mcp-token"}, + json={ + "event": "OCP\\Events\\NodeCreated", + "uri": "http://mcp:8000/webhooks/nextcloud", + }, + ) + + assert resp.status_code == 503 + assert resp.json()["error"] == "Webhooks disabled" + + async def test_create_webhook_validates_required_fields(mocker): _patch_token_validation(mocker) _patch_basic_auth(mocker)