diff --git a/CLAUDE.md b/CLAUDE.md index 3d3ab25..413da35 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,7 +55,7 @@ Full guide: [docs/testing.md](docs/testing.md). - Test files mirror the source tree: `src/sync_engine/store/base.py` → `tests/store/test_*.py`. - One test per behavior, not per function. - Naming: `test___`. -- Webhook tests mock signature verification (never depend on secrets). +- Webhook tests compute real HMAC signatures with a fixture secret — do not mock `verify_hmac_sha256` unless testing the handler in isolation from the verifier. Never hard-code production secrets. - Reconciliation tests mock the REST API (no network calls in CI). - Use `InMemoryStore` as the store fixture — never a real backend. @@ -91,12 +91,13 @@ If a new implementation *contradicts* an existing ADR, the new ADR must referenc |---|---|---| | [ADR-001](docs/adr/ADR-001-hybrid-rest-webhook.md) | Hybrid REST/webhook architecture | Accepted | | [ADR-002](docs/adr/ADR-002-pluggable-store-interface.md) | Pluggable store interface via abstract base class | Accepted | +| [ADR-003](docs/adr/ADR-003-webhook-hmac-sha256-signature-verification.md) | HMAC-SHA256 signature verification for webhook reception | Accepted | --- ## Git -- Current development branch: `claude/sync-engine-bootstrap-wkdEx` +- Current development branch: `claude/webhook-verifier-handler-eked86` - Commit messages in English, imperative mood, no trailing period. - Format: `: ` — types: `feat`, `fix`, `docs`, `refactor`, `test`, `chore` - Do not commit secrets, `.env` files, or tokens. diff --git a/docs/adr/ADR-003-webhook-hmac-sha256-signature-verification.md b/docs/adr/ADR-003-webhook-hmac-sha256-signature-verification.md new file mode 100644 index 0000000..a8ccfce --- /dev/null +++ b/docs/adr/ADR-003-webhook-hmac-sha256-signature-verification.md @@ -0,0 +1,78 @@ +# ADR-003 — HMAC-SHA256 signature verification for webhook reception + +**Status**: Accepted +**Date**: 2026-06-09 +**Deciders**: fullstack-pilot team + +--- + +## Context + +ADR-001 establishes that the webhook channel is the primary change-propagation path and that the engine must verify each webhook's signature before inserting it into the queue (invariant #4 in CLAUDE.md). The verification mechanism was not specified by ADR-001. + +Three schemes were considered: + +1. **Symmetric HMAC-SHA256** — sender and receiver share a secret; the sender attaches `HMAC(secret, body)` as a header. +2. **Asymmetric signature (RSA/ECDSA)** — sender signs the payload with a private key; receiver verifies with the public key. +3. **Bearer token** — sender includes a static shared token in a header; receiver checks equality. + +--- + +## Decision + +We use **HMAC-SHA256** with a shared secret. + +The signature is transmitted in a request header (e.g. `X-Hub-Signature-256`) as either a bare hex digest or a `sha256=` prefixed string; both forms are accepted. + +Comparison is performed with `hmac.compare_digest` (constant-time) to prevent timing-oracle attacks. + +Verification is isolated in `src/sync_engine/webhook/verifier.py` as a single pure function: + +```python +def verify_hmac_sha256(payload_bytes: bytes, signature_header: str, secret: str) -> None: ... +``` + +`WebhookHandler` calls this function as the first step in `handle()`. Any failure raises `WebhookSignatureError` before any queue write occurs (invariant #4). + +--- + +## Why HMAC-SHA256 over asymmetric signatures + +- HMAC-SHA256 is the de-facto standard in the webhook ecosystem (GitHub, Stripe, Shopify all use it). Third-party sources that emit webhooks to this engine will already be configured for it. +- Asymmetric signatures add key distribution and rotation complexity for no benefit at this scale: the receiver is a single service, not a distributed public verifier. +- The security model is equivalent when the shared secret is stored as an environment variable and never committed. + +## Why HMAC-SHA256 over bearer token + +- A bearer token is a static equality check; its security degrades if the comparison is not constant-time (timing oracle) or if the token leaks in logs. +- HMAC binds the signature to the payload body, so a replayed token with a different payload is rejected. A bearer token provides no such protection. + +--- + +## Constant-time comparison requirement + +Naive string comparison (`==`) leaks timing information proportional to the length of the shared prefix, enabling offline secret reconstruction. `hmac.compare_digest` compares in fixed time regardless of where the strings diverge. This is mandatory for all signature comparisons in the engine — any future verification code must follow the same constraint. + +--- + +## Consequences + +**Positive:** +- Standard scheme: every major webhook source can be configured to sign with HMAC-SHA256 out of the box. +- Timing-safe by construction: `hmac.compare_digest` is the only permitted comparison. +- Isolated: `verifier.py` has no side effects and is independently testable without mocking. +- Tests compute real HMAC signatures with a fixture secret — no mocking needed, the full verification path is exercised. + +**Negative / accepted costs:** +- Secret rotation requires coordinating the sender and receiver simultaneously; there is no multi-key grace window in the current implementation. +- The `sha256=` prefix stripping is a convention, not a standard — future sources may use a different prefix, requiring an extension to `verifier.py`. + +--- + +## Rejected alternatives + +| Alternative | Reason for rejection | +|---|---| +| Asymmetric signature (RSA/ECDSA) | Disproportionate key management overhead; incompatible with most third-party webhook senders | +| Bearer token | Does not bind signature to payload body; provides no replay protection | +| No verification | Violates invariant #4 in CLAUDE.md — rejected unconditionally | diff --git a/docs/testing.md b/docs/testing.md index bf18c3b..e18b29b 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -21,7 +21,8 @@ tests/ ├── store/ │ └── test_memory_store.py ├── webhook/ -│ └── test_handler.py # (upcoming) +│ ├── test_verifier.py +│ └── test_handler.py ├── reconciliation/ │ └── test_loop.py # (upcoming) └── processor/ @@ -71,16 +72,26 @@ def store() -> InMemoryStore: ### Webhook signature verification -Never depend on real secrets in tests. Patch the verification call: +`verify_hmac_sha256` is a pure function with no side effects — test it directly +by computing the expected HMAC with a fixture secret. No mocking needed: ```python -from unittest.mock import patch +import hashlib, hmac +from sync_engine.webhook.verifier import verify_hmac_sha256 -def test_valid_webhook_is_queued(store): - with patch("sync_engine.webhook.handler.verify_signature", return_value=True): - ... +SECRET = "test-secret" +PAYLOAD = b'{"event": "push"}' + +def _sig(payload: bytes = PAYLOAD, secret: str = SECRET) -> str: + return hmac.new(secret.encode(), payload, hashlib.sha256).hexdigest() + +def test_valid_signature_raises_nothing() -> None: + verify_hmac_sha256(PAYLOAD, _sig(), SECRET) # must not raise ``` +Mock `verify_hmac_sha256` only when testing higher-level code (e.g. an HTTP +layer) that would otherwise require wiring a valid secret end-to-end. + ### REST API (reconciliation) No network calls in CI. Patch `httpx.Client` or inject a fake transport: diff --git a/pyproject.toml b/pyproject.toml index 2e6679d..0db8be3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -14,8 +14,8 @@ dependencies = [ [project.optional-dependencies] dev = [ "pytest", - "ruff", - "black", + "ruff==0.16.0", + "black==26.5.1", ] [tool.hatch.build.targets.wheel] diff --git a/src/sync_engine/webhook/__init__.py b/src/sync_engine/webhook/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/src/sync_engine/webhook/handler.py b/src/sync_engine/webhook/handler.py new file mode 100644 index 0000000..d261647 --- /dev/null +++ b/src/sync_engine/webhook/handler.py @@ -0,0 +1,43 @@ +import logging + +from sync_engine.exceptions import DuplicateEventError +from sync_engine.store.base import SyncStore +from sync_engine.webhook.verifier import verify_hmac_sha256 + +logger = logging.getLogger(__name__) + + +class WebhookHandler: + def __init__(self, store: SyncStore, secret: str) -> None: + self._store = store + self._secret = secret + + def handle( + self, + event_id: str, + payload: dict, + raw_bytes: bytes, + signature_header: str, + ) -> None: + """Verify, deduplicate, and enqueue an incoming webhook event. + + Raises: + WebhookSignatureError: signature missing or invalid (logged at WARNING). + DuplicateEventError: event_id already processed (logged at DEBUG). + """ + # verify_hmac_sha256 already logs at WARNING and raises WebhookSignatureError + verify_hmac_sha256(raw_bytes, signature_header, self._secret) + + if self._store.is_event_processed(event_id): + logger.debug("Duplicate event received, skipping: %s", event_id) + raise DuplicateEventError(f"Event already processed: {event_id}") + + already_queued = any( + queued_id == event_id + for queued_id, _ in self._store.dequeue_unacknowledged() + ) + if already_queued: + logger.debug("Duplicate event already queued, skipping: %s", event_id) + raise DuplicateEventError(f"Event already queued: {event_id}") + + self._store.enqueue_webhook(event_id, payload) diff --git a/src/sync_engine/webhook/verifier.py b/src/sync_engine/webhook/verifier.py new file mode 100644 index 0000000..2979328 --- /dev/null +++ b/src/sync_engine/webhook/verifier.py @@ -0,0 +1,29 @@ +import hashlib +import hmac +import logging + +from sync_engine.exceptions import WebhookSignatureError + +logger = logging.getLogger(__name__) + + +def verify_hmac_sha256( + payload_bytes: bytes, signature_header: str, secret: str +) -> None: + """Verify the HMAC-SHA256 signature of a webhook payload. + + Raises WebhookSignatureError if the header is missing or the signature does + not match. Uses hmac.compare_digest to prevent timing attacks. + """ + if not signature_header: + logger.warning("Webhook received with missing signature header") + raise WebhookSignatureError("Missing signature header") + + expected = hmac.new(secret.encode(), payload_bytes, hashlib.sha256).hexdigest() + + # Accept bare hex digest or "sha256=" prefix + received = signature_header.removeprefix("sha256=") + + if not hmac.compare_digest(expected, received): + logger.warning("Webhook signature mismatch") + raise WebhookSignatureError("Signature mismatch") diff --git a/tests/store/test_memory_store.py b/tests/store/test_memory_store.py index 4cf385a..eb50c20 100644 --- a/tests/store/test_memory_store.py +++ b/tests/store/test_memory_store.py @@ -1,4 +1,4 @@ -from datetime import datetime, timezone +from datetime import UTC, datetime import pytest @@ -15,14 +15,14 @@ def test_get_last_sync_at_before_any_write_returns_none(store): def test_set_last_sync_at_get_returns_same_value(store): - dt = datetime(2026, 1, 1, 12, 0, 0, tzinfo=timezone.utc) + dt = datetime(2026, 1, 1, 12, 0, 0, tzinfo=UTC) store.set_last_sync_at(dt) assert store.get_last_sync_at() == dt def test_set_last_sync_at_overwrites_previous_value(store): - first = datetime(2026, 1, 1, tzinfo=timezone.utc) - second = datetime(2026, 6, 1, tzinfo=timezone.utc) + first = datetime(2026, 1, 1, tzinfo=UTC) + second = datetime(2026, 6, 1, tzinfo=UTC) store.set_last_sync_at(first) store.set_last_sync_at(second) assert store.get_last_sync_at() == second diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 6746315..9175383 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -24,5 +24,7 @@ def test_reconciliation_error_caught_as_sync_error(): def test_sync_error_caught_as_base_exception(): - with pytest.raises(Exception): + # Deliberately blind: verifies SyncError stays catchable by a bare + # `except Exception` handler elsewhere in the codebase. + with pytest.raises(Exception): # noqa: B017 raise SyncError("base error") diff --git a/tests/webhook/__init__.py b/tests/webhook/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/webhook/test_handler.py b/tests/webhook/test_handler.py new file mode 100644 index 0000000..20e146a --- /dev/null +++ b/tests/webhook/test_handler.py @@ -0,0 +1,66 @@ +import hashlib +import hmac + +import pytest + +from sync_engine.exceptions import DuplicateEventError, WebhookSignatureError +from sync_engine.store.memory import InMemoryStore +from sync_engine.webhook.handler import WebhookHandler + +SECRET = "handler-secret" +EVENT_ID = "evt-001" +PAYLOAD = {"action": "opened"} +RAW = b'{"action": "opened"}' + + +def _sig(payload: bytes = RAW, secret: str = SECRET) -> str: + return hmac.new(secret.encode(), payload, hashlib.sha256).hexdigest() + + +@pytest.fixture() +def store() -> InMemoryStore: + return InMemoryStore() + + +@pytest.fixture() +def handler(store: InMemoryStore) -> WebhookHandler: + return WebhookHandler(store, SECRET) + + +def test_valid_event_is_enqueued(store: InMemoryStore, handler: WebhookHandler) -> None: + handler.handle(EVENT_ID, PAYLOAD, RAW, _sig()) + + unacked = store.dequeue_unacknowledged() + assert len(unacked) == 1 + assert unacked[0] == (EVENT_ID, PAYLOAD) + + +def test_duplicate_event_raises_and_not_reenqueued( + store: InMemoryStore, handler: WebhookHandler +) -> None: + store.mark_event_processed(EVENT_ID) + + with pytest.raises(DuplicateEventError): + handler.handle(EVENT_ID, PAYLOAD, RAW, _sig()) + + assert store.dequeue_unacknowledged() == [] + + +def test_duplicate_event_already_queued_raises_and_does_not_overwrite( + store: InMemoryStore, handler: WebhookHandler +) -> None: + handler.handle(EVENT_ID, PAYLOAD, RAW, _sig()) + + with pytest.raises(DuplicateEventError): + handler.handle(EVENT_ID, PAYLOAD, RAW, _sig()) + + assert store.dequeue_unacknowledged() == [(EVENT_ID, PAYLOAD)] + + +def test_bad_signature_raises_and_not_enqueued( + store: InMemoryStore, handler: WebhookHandler +) -> None: + with pytest.raises(WebhookSignatureError): + handler.handle(EVENT_ID, PAYLOAD, RAW, "bad-sig") + + assert store.dequeue_unacknowledged() == [] diff --git a/tests/webhook/test_verifier.py b/tests/webhook/test_verifier.py new file mode 100644 index 0000000..747919f --- /dev/null +++ b/tests/webhook/test_verifier.py @@ -0,0 +1,36 @@ +import hashlib +import hmac + +import pytest + +from sync_engine.exceptions import WebhookSignatureError +from sync_engine.webhook.verifier import verify_hmac_sha256 + +SECRET = "test-secret" +PAYLOAD = b'{"event": "push"}' + + +def _make_sig(payload: bytes, secret: str) -> str: + return hmac.new(secret.encode(), payload, hashlib.sha256).hexdigest() + + +def test_valid_signature_raises_nothing() -> None: + sig = _make_sig(PAYLOAD, SECRET) + verify_hmac_sha256(PAYLOAD, sig, SECRET) + + +def test_valid_signature_with_prefix_raises_nothing() -> None: + sig = "sha256=" + _make_sig(PAYLOAD, SECRET) + verify_hmac_sha256(PAYLOAD, sig, SECRET) + + +def test_tampered_payload_raises_signature_error() -> None: + sig = _make_sig(PAYLOAD, SECRET) + tampered = b'{"event": "delete"}' + with pytest.raises(WebhookSignatureError): + verify_hmac_sha256(tampered, sig, SECRET) + + +def test_missing_header_raises_signature_error() -> None: + with pytest.raises(WebhookSignatureError): + verify_hmac_sha256(PAYLOAD, "", SECRET)