fix(webhooks): authenticate deliveries via WEBHOOK_SECRET; review nits
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>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
2e2a098bee
commit
224428fca5
@@ -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."""
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"},
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user