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) <noreply@anthropic.com>
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 <script> payloads (in preset_id and exception messages) are
emitted as escaped entities, not active markup.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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) <noreply@anthropic.com>
- 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) <noreply@anthropic.com>
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 <secret>"} (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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
