fix(security): require WEBHOOK_SECRET for the Nextcloud webhook receiver
GHSA-8vh3-g2qg-2h2c (CVSS 9.1, CWE-306): POST /webhooks/nextcloud had no
authentication when WEBHOOK_SECRET was unset (the default). The receiver
trusted the attacker-supplied user.uid and fed it to Qdrant, letting an
unauthenticated network caller delete or re-index any user's vector
embeddings.
Webhooks now require WEBHOOK_SECRET end-to-end:
- app.py: the /webhooks/nextcloud route is only mounted when WEBHOOK_SECRET
is set; otherwise it 404s and a startup warning notes vector sync falls
back to the polling scanner.
- webhook_receiver.py: removed the warn-and-accept fallback. No secret -> 503,
missing/invalid bearer -> 401; the payload is never processed unauthenticated.
- webhook_routes.py / api/webhooks.py: webhook_auth_pair() raises
WebhookSecretNotConfigured instead of returning authMethod="none"; both
registration entry points return a clear 503 so no dead unauthenticated
webhooks are created.
Also expose webhooks availability to the Astrolabe UI via GET /api/v1/status
("webhooks_enabled": bool), set WEBHOOK_SECRET on the docker-compose
semantic-search dev services, and update env.sample + ADR-010 / ADR-018 /
webhook-management-guide docs.
Vector sync still works without a secret via the polling scanner.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
a818b49b3f
commit
4fc2b10945
@@ -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",
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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=(
|
||||
'<div class="warning">Webhooks are disabled: WEBHOOK_SECRET is '
|
||||
"not set. Configure it and restart the server to enable webhook "
|
||||
"presets.</div>"
|
||||
),
|
||||
status_code=503,
|
||||
)
|
||||
except Exception as e:
|
||||
logger.error("Failed to enable preset %s: %s", preset_id, e, exc_info=True)
|
||||
return HTMLResponse(
|
||||
|
||||
@@ -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 <secret> 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 <secret> 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
|
||||
|
||||
@@ -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 <secret>`` (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 <secret>`` (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()
|
||||
|
||||
Reference in New Issue
Block a user