diff --git a/README.md b/README.md index ecdeb81..4d96ee2 100644 --- a/README.md +++ b/README.md @@ -63,7 +63,7 @@ A [free Hookdeck account](https://dashboard.hookdeck.com/signup) is enough for d Events flow: **provider -> Hookdeck -> (CLI or HTTP push) -> plugin listener -> Hermes agent run**, with three reliability layers on top: -1. **Signature verification.** Every delivery carries an `x-hookdeck-signature` header, verified with HMAC-SHA256 in constant time. Provider-side verification (Stripe, Shopify, GitHub, and ~140 others) happens at Hookdeck's edge before the event ever reaches you — but only once you paste that provider's signing secret onto the source in the Hookdeck dashboard. `hermes hookdeck setup` creates the source with the right type and cannot set the secret; until it is set, a typed source accepts unsigned and forged payloads. `hermes hookdeck doctor` reports what the source actually verified. +1. **Signature verification.** Every delivery carries an `x-hookdeck-signature` header, verified with HMAC-SHA256 in constant time. Provider-side verification (Stripe, Shopify, GitHub, and ~140 others) happens at Hookdeck's edge before the event ever reaches you — but only once you paste that provider's signing secret onto the source in the Hookdeck dashboard. A source whose `type` names the provider can only take that secret through the dashboard; a generic source carrying `auth_type` can be configured over the API. Either way `hermes hookdeck setup` does not set it today, and until it is set the source accepts unsigned and forged payloads. `hermes hookdeck doctor` reports what the source actually verified. 2. **Run ledger.** A local SQLite database records each delivery attempt and its agent-run outcome. If the process crashes mid-run, boot-time recovery finds the orphaned events and re-runs them. 3. **Backpressure.** `max_concurrent` caps simultaneous agent runs. Requests over the cap get a 503 with `Retry-After`, and Hookdeck redelivers on schedule instead of piling runs onto your box. diff --git a/hookdeck/cli.py b/hookdeck/cli.py index 4df6e5d..6dc1ad5 100644 --- a/hookdeck/cli.py +++ b/hookdeck/cli.py @@ -507,77 +507,99 @@ async def _go() -> str: async def _check_source_verification(api: HookdeckAPI, routes: dict) -> list[Check]: - """Whether a provider-typed source is actually verifying anything. - - Setting a source's type to STRIPE or GITHUB does *not* switch verification - on. The provider's own signing secret has to be set on the source, and - until it is the source accepts anything — measured: an unsigned request and - one carrying `sha256=deadbeef` were both accepted by a GITHUB source with - no secret, and both produced events. - - The source's own record does not say whether a secret is configured; a - source with one set is byte-identical to one without over the API. So the - only signal is observed traffic, where each request carries `verified`. - That means this can confirm a problem but never confirm its absence, and it - says which of the two it is doing rather than implying the stronger one. + """Whether a source that should be verifying actually is. + + Naming a provider does not switch verification on. The provider's own + signing secret has to be set, and until it is the source accepts anything — + measured: an unsigned request and one carrying `sha256=deadbeef` were both + accepted by a GITHUB source with no secret, and both produced events. + + Two things are asked, because they answer different questions: + + * **Is it configured?** A generic source carrying `auth_type: STRIPE` + reports that back, so this is a direct answer. A source whose `type` + names the provider hides its config entirely — one with a secret set is + byte-identical to one without — so for those there is nothing to read. + * **Is it working?** Only traffic can say. Each inbound request carries + `verified`, and a configured-but-wrong secret looks exactly like a + missing one from the outside. Stripe issues a different secret per + endpoint, so a mismatch is a realistic way to get here. + + Configured is therefore never treated as sufficient: it refines the message + and never suppresses the traffic check. + + The source is read from the API rather than from our config, because the + config says what we would provision, not what is there. A source created by + hand, or changed in the dashboard afterwards, is the case worth catching. """ - typed = { - name: route - for name, route in routes.items() - if str(route.get("source_type") or "WEBHOOK").upper() != "WEBHOOK" - } - if not typed: - return [] - checks: list[Check] = [] - for route_name, route in typed.items(): - source_type = str(route["source_type"]).upper() + for route_name, route in routes.items(): source_name = route.get("source") or route_name found = _models(await api.list_sources(name=source_name)) if not found: continue source = found[0] + name = source.get("name") or source_name + declared = ((source.get("config") or {}).get("auth_type") or "").upper() + source_type = str(source.get("type") or "WEBHOOK").upper() + + # A plain WEBHOOK source with no provider auth verifies with the + # project's own signing secret, which the adapter checks on every + # delivery. Nothing to say. + if source_type == "WEBHOOK" and not declared: + continue + + scheme = declared or source_type requests = _models( await api.list_requests(source_id=source.get("id"), limit=10) ) - if not requests: - checks.append( - Check( - True, - f"source '{source.get('name')}' is type {source_type} — no " - "traffic yet, so verification is unconfirmed", - note=( - f"A {source_type} source verifies nothing until the " - "provider's signing secret is set on it in the Hookdeck " - "dashboard, and the API does not report whether it is. " - "Until then the source accepts forged payloads." - ), - ) - ) - continue - unverified = [r for r in requests if not r.get("verified")] + if unverified: checks.append( Check( False, - f"source '{source.get('name')}' is type {source_type} but " + f"source '{name}' should verify as {scheme}, but " f"{len(unverified)} of its last {len(requests)} requests " "were not verified — its signing secret is missing or does " "not match the sender's.", note=( "Set the provider's signing secret on the source in the " - "Hookdeck dashboard. Without it the source accepts " - "anything, including forged payloads." + "Hookdeck dashboard, and check it is the one for the " + "endpoint actually sending. Until it matches, the " + "source accepts anything, including forged payloads." ), ) ) + elif requests: + checks.append( + Check( + True, + f"source '{name}' verified all of its last {len(requests)} " + f"requests as {scheme}", + ) + ) + elif declared: + checks.append( + Check( + True, + f"source '{name}' is configured to verify as {scheme} — no " + "traffic yet to confirm the secret is the right one", + ) + ) else: checks.append( Check( True, - f"source '{source.get('name')}' verified all of its last " - f"{len(requests)} requests as {source_type}", + f"source '{name}' is type {scheme} — no traffic yet, so " + "verification is unconfirmed", + note=( + f"A {scheme} source verifies nothing until the " + "provider's signing secret is set on it in the Hookdeck " + "dashboard, and for this source shape the API does not " + "report whether it is. Until then it accepts forged " + "payloads." + ), ) ) return checks diff --git a/hookdeck/tools.py b/hookdeck/tools.py index c8fc7a2..b2b8a4a 100644 --- a/hookdeck/tools.py +++ b/hookdeck/tools.py @@ -110,6 +110,26 @@ def _with_ledger(action) -> None: ledger.close() +def _payload_text(body: Any) -> str: + """The payload out of ``GET /events/{id}/raw_body``. + + The endpoint answers ``{"body": ""}`` — a wrapper, not the payload. + Returning it whole hands the model an escaped string inside an envelope + when what it asked for was what the provider sent. + + Missed for a long time because a model tidies it up when it summarises, so + a transcript looks right while the tool's own return value is wrong. + + Matched exactly — one key, holding a string — rather than on the presence + of ``body``. A payload is third-party JSON and may well have a ``body`` + field of its own; unwrapping that would quietly return a fragment of the + event as though it were the whole thing. + """ + if isinstance(body, dict) and set(body) == {"body"} and isinstance(body["body"], str): + return body["body"] + return body if isinstance(body, str) else json.dumps(body) + + def _models(result: Any) -> list[dict]: if not isinstance(result, dict): return [] @@ -203,8 +223,7 @@ def hookdeck_get_event_body(args: dict) -> str: async def _go() -> str: async with HookdeckAPI() as api: body = await api.get_event_raw_body(event_id) - text = json.dumps(body) if not isinstance(body, str) else body - return text[:8000] + return _payload_text(body)[:8000] return _run(_go()) diff --git a/tests/test_cli.py b/tests/test_cli.py index 24524b9..e175239 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -686,7 +686,9 @@ def _typed_route_doctor(doctor_env, fake_api, monkeypatch, *, requests): "rules": [{"type": "retry", "count": 10, "response_status_codes": retryable_status_codes()}]}] } - fake_api.responses["list_sources"] = {"models": [{"id": "src_1", "name": "payments"}]} + fake_api.responses["list_sources"] = { + "models": [{"id": "src_1", "name": "payments", "type": "STRIPE"}] + } fake_api.responses["list_requests"] = {"models": requests} _configure(doctor_env, secret="s", cli_config_path="", routes={"payments": {"source": "payments", "source_type": "STRIPE"}}) @@ -756,3 +758,98 @@ def test_a_plain_webhook_source_is_not_nagged_about_provider_secrets( out = capsys.readouterr().out assert "verification is unconfirmed" not in out assert not calls_named(fake_api, "list_requests") + + +def test_doctor_reads_auth_type_directly_when_the_source_states_it( + doctor_env, fake_api, monkeypatch, capsys +): + # A generic source carrying `auth_type: STRIPE` reports it back, so there + # is no need to infer verification from traffic. A typed source hides its + # config entirely, which is why the traffic fallback exists at all. + from hookdeck.provision import retryable_status_codes + + monkeypatch.setenv("HOOKDECK_API_KEY", "key") + doctor_env.setattr(cli.shutil, "which", lambda _b: "/usr/local/bin/hookdeck") + doctor_env.setattr(cli, "_cli_version", lambda _b: "2.4.0") + doctor_env.setattr(cli, "_other_hookdeck_binaries", lambda _r: []) + fake_api.responses["list_connections"] = { + "models": [{"name": "payments", "team_id": "tm_1", + "rules": [{"type": "retry", "count": 10, + "response_status_codes": retryable_status_codes()}]}] + } + fake_api.responses["list_sources"] = { + "models": [{"id": "src_1", "name": "payments", "type": "WEBHOOK", + "config": {"auth_type": "STRIPE", "auth": {}}}] + } + fake_api.responses["list_requests"] = {"models": []} + _configure(doctor_env, secret="s", cli_config_path="", + routes={"payments": {"source": "payments", "source_type": "STRIPE"}}) + + assert cli.hookdeck_command(_ns("doctor")) == 0 + out = capsys.readouterr().out + assert "configured to verify as STRIPE" in out + assert "no traffic yet to confirm the secret is the right one" in out + # Configured is not working: the traffic check still runs, because a wrong + # secret looks identical to a missing one from the outside. + assert calls_named(fake_api, "list_requests") + + +def test_a_declared_auth_type_does_not_excuse_failing_traffic( + doctor_env, fake_api, monkeypatch, capsys +): + # The regression this exists for: reporting "configured to verify as + # STRIPE" and stopping there. A wrong secret leaves auth_type set and every + # request failing — and Stripe issues a different secret per endpoint, so + # pasting the wrong one is a realistic way to arrive here. + from hookdeck.provision import retryable_status_codes + + monkeypatch.setenv("HOOKDECK_API_KEY", "key") + doctor_env.setattr(cli.shutil, "which", lambda _b: "/usr/local/bin/hookdeck") + doctor_env.setattr(cli, "_cli_version", lambda _b: "2.4.0") + doctor_env.setattr(cli, "_other_hookdeck_binaries", lambda _r: []) + fake_api.responses["list_connections"] = { + "models": [{"name": "payments", "team_id": "tm_1", + "rules": [{"type": "retry", "count": 10, + "response_status_codes": retryable_status_codes()}]}] + } + fake_api.responses["list_sources"] = { + "models": [{"id": "src_1", "name": "payments", "type": "WEBHOOK", + "config": {"auth_type": "STRIPE", "auth": {}}}] + } + fake_api.responses["list_requests"] = {"models": [{"verified": False}] * 3} + _configure(doctor_env, secret="s", cli_config_path="", + routes={"payments": {"source": "payments", "source_type": "STRIPE"}}) + + assert cli.hookdeck_command(_ns("doctor")) == 1 + out = capsys.readouterr().out + assert "3 of its last 3 requests were not verified" in out + assert "the one for the endpoint actually sending" in out + + +def test_the_source_is_read_from_the_api_not_from_our_config( + doctor_env, fake_api, monkeypatch, capsys +): + # Config says what we would provision; the source says what is there. A + # source changed in the dashboard afterwards is the case worth catching, so + # a route claiming WEBHOOK must not hide a provider-typed source. + from hookdeck.provision import retryable_status_codes + + monkeypatch.setenv("HOOKDECK_API_KEY", "key") + doctor_env.setattr(cli.shutil, "which", lambda _b: "/usr/local/bin/hookdeck") + doctor_env.setattr(cli, "_cli_version", lambda _b: "2.4.0") + doctor_env.setattr(cli, "_other_hookdeck_binaries", lambda _r: []) + fake_api.responses["list_connections"] = { + "models": [{"name": "payments", "team_id": "tm_1", + "rules": [{"type": "retry", "count": 10, + "response_status_codes": retryable_status_codes()}]}] + } + fake_api.responses["list_sources"] = { + "models": [{"id": "src_1", "name": "payments", "type": "GITHUB"}] + } + fake_api.responses["list_requests"] = {"models": [{"verified": False}]} + # Route says nothing about a source type at all. + _configure(doctor_env, secret="s", cli_config_path="", + routes={"payments": {"source": "payments"}}) + + assert cli.hookdeck_command(_ns("doctor")) == 1 + assert "should verify as GITHUB" in capsys.readouterr().out diff --git a/tests/test_tools.py b/tests/test_tools.py index 2943d8d..dbd9c5b 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -401,6 +401,44 @@ def test_an_event_body_is_truncated_before_it_reaches_the_context(api): assert len(call("hookdeck_get_event_body", {"event_id": "evt_1"})) == 8000 +def test_the_raw_body_envelope_is_unwrapped(api): + # The endpoint answers {"body": ""}. Returning that whole hands the + # model an escaped string inside an envelope instead of the payload — + # invisible in a transcript, because the model tidies it up when it + # summarises. + api.responses["get_event_raw_body"] = { + "body": '{"kind":"charge.succeeded","amount":2000}' + } + assert call("hookdeck_get_event_body", {"event_id": "evt_1"}) == ( + '{"kind":"charge.succeeded","amount":2000}' + ) + + +def test_an_unwrapped_string_body_still_works(api): + api.responses["get_event_raw_body"] = '{"kind":"already-plain"}' + assert call("hookdeck_get_event_body", {"event_id": "evt_1"}) == ( + '{"kind":"already-plain"}' + ) + + +def test_only_the_exact_envelope_shape_is_unwrapped(api): + # Measured: the endpoint always answers exactly {"body": ""}. So a + # dict shaped any other way is the payload, not the envelope — including + # third-party JSON that happens to carry its own `body` field. Unwrapping + # that would return a fragment of the event as though it were the whole + # thing, which is worse than the envelope it replaced. + api.responses["get_event_raw_body"] = {"body": "inner", "from": "acme"} + assert json.loads(call("hookdeck_get_event_body", {"event_id": "evt_1"})) == { + "body": "inner", + "from": "acme", + } + + api.responses["get_event_raw_body"] = {"body": {"kind": "structured"}} + assert json.loads(call("hookdeck_get_event_body", {"event_id": "evt_1"})) == { + "body": {"kind": "structured"} + } + + def test_a_structured_body_is_serialised(api): api.responses["get_event_raw_body"] = {"type": "charge.succeeded"} assert json.loads(call("hookdeck_get_event_body", {"event_id": "evt_1"})) == {