From 75b92162c497d32d6d32ce4cec4ea392e06660d5 Mon Sep 17 00:00:00 2001 From: garethx Date: Thu, 13 Aug 2026 11:22:44 +0100 Subject: [PATCH 1/2] Unwrap the raw-body envelope, and read auth_type where the API states it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both from the OpenClaw session's live-testing notes, verified here against the same project before acting on either. **`hookdeck_get_event_body` returned an envelope, not a payload.** `GET /events/{id}/raw_body` answers `{"body": ""}`, and we handed that back whole — so the model asked what a provider sent and received an escaped string inside a wrapper. Confirmed live against a real event. Worth recording how it hid. This tool was exercised live earlier in the week and looked right, because the agent's summary showed a clean payload: the model unwraps it while writing prose. What was checked was the transcript, not the tool's return value. A test asserting the tool's own output would have caught it on day one, and now does. **doctor can sometimes just ask.** The earlier check inferred verification from observed traffic, on the grounds that a source's config never reveals whether a secret is set. That holds for a source whose `type` names the provider, and I had generalised it. It is not true of a generic source carrying `auth_type: STRIPE`, which reports that `auth_type` back — measured: an unsigned request to one came back VERIFICATION_FAILED, and the source's config showed `auth_type: STRIPE` with the secret itself hidden. So doctor now reads `auth_type` when it is there and falls back to traffic when it is not, and the README no longer says the secret can only come from the dashboard — that is true of one shape, not both. Co-Authored-By: Claude Opus 5 --- README.md | 2 +- hookdeck/cli.py | 30 +++++++++++++++++++++++++----- hookdeck/tools.py | 19 +++++++++++++++++-- tests/test_cli.py | 31 +++++++++++++++++++++++++++++++ tests/test_tools.py | 27 +++++++++++++++++++++++++++ 5 files changed, 101 insertions(+), 8 deletions(-) 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..c588c1a 100644 --- a/hookdeck/cli.py +++ b/hookdeck/cli.py @@ -515,11 +515,18 @@ async def _check_source_verification(api: HookdeckAPI, routes: dict) -> list[Che 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. + There are two source shapes and the API is only forthcoming about one: + + * a **typed** source (`type: STRIPE`) hides its config entirely — one with + a secret set is byte-identical to one without, confirmed against a source + whose secret was definitely configured. Nothing to read. + * a **generic** source carrying `auth_type: STRIPE` reports that + `auth_type` back, though not the secret. That is a direct answer. + + So this reads `auth_type` when it is there, and falls back to observed + traffic — the `verified` flag on inbound requests — when it is not. The + fallback can confirm a problem but never its absence, and says which of the + two it is doing rather than implying the stronger one. """ typed = { name: route @@ -537,6 +544,19 @@ async def _check_source_verification(api: HookdeckAPI, routes: dict) -> list[Che if not found: continue source = found[0] + + # A generic source states its auth_type, so no inference is needed. + configured = ((source.get("config") or {}).get("auth_type") or "").upper() + if configured: + checks.append( + Check( + True, + f"source '{source.get('name')}' is configured to verify as " + f"{configured}", + ) + ) + continue + requests = _models( await api.list_requests(source_id=source.get("id"), limit=10) ) diff --git a/hookdeck/tools.py b/hookdeck/tools.py index c8fc7a2..f2648a4 100644 --- a/hookdeck/tools.py +++ b/hookdeck/tools.py @@ -110,6 +110,22 @@ 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. + """ + if isinstance(body, dict) and "body" in body: + inner = body["body"] + return inner if isinstance(inner, str) else json.dumps(inner) + return body if isinstance(body, str) else json.dumps(body) + + def _models(result: Any) -> list[dict]: if not isinstance(result, dict): return [] @@ -203,8 +219,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..d9d685d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -756,3 +756,34 @@ 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", + "config": {"auth_type": "STRIPE", "auth": {}}}] + } + _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 + # No need to guess from traffic when the source answers directly. + assert not calls_named(fake_api, "list_requests") diff --git a/tests/test_tools.py b/tests/test_tools.py index 2943d8d..3455d8e 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -401,6 +401,33 @@ 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_a_wrapper_holding_a_structured_body_is_serialised(api): + api.responses["get_event_raw_body"] = {"body": {"kind": "structured"}} + assert json.loads(call("hookdeck_get_event_body", {"event_id": "evt_1"})) == { + "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"})) == { From 8685532c703b3710ccde94c0443e63a5c04a4c59 Mon Sep 17 00:00:00 2001 From: garethx Date: Thu, 13 Aug 2026 11:30:06 +0100 Subject: [PATCH 2/2] Do not let a declared auth_type excuse failing traffic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of my own change, which had introduced a worse bug than the one it fixed. Reading `auth_type` told doctor the source was *configured*, and I then returned early — so a source with `auth_type` set and the wrong secret reported green while refusing every request. The traffic check I was replacing would have caught exactly that, and Stripe issues a different signing secret per endpoint, so a mismatch is the realistic way to get there rather than an exotic one. Configured now refines the message and never suppresses the check. Two further corrections while re-reading it: * The check keyed off `source_type` in our config, which says what we would provision rather than what exists. It now reads the source from the API, so a source created by hand or changed in the dashboard is seen for what it is — and the generic `auth_type` shape, which lives on a WEBHOOK-typed source, is no longer excluded by the very filter meant to find it. * `_payload_text` unwrapped any dict containing `body`. Measured across several events, the envelope is always exactly `{"body": ""}`, so it now matches that shape and nothing else. A payload is third-party JSON and may carry its own `body` field; unwrapping that would hand back a fragment of the event as though it were the whole thing. Four mutations checked. The first attempt at one of them was a no-op — it assigned a variable the next line overwrote — and reported as survived; re-run properly, the short-circuit is caught by two tests. Co-Authored-By: Claude Opus 5 --- hookdeck/cli.py | 126 ++++++++++++++++++++++---------------------- hookdeck/tools.py | 10 ++-- tests/test_cli.py | 74 ++++++++++++++++++++++++-- tests/test_tools.py | 15 +++++- 4 files changed, 154 insertions(+), 71 deletions(-) diff --git a/hookdeck/cli.py b/hookdeck/cli.py index c588c1a..6dc1ad5 100644 --- a/hookdeck/cli.py +++ b/hookdeck/cli.py @@ -507,97 +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. - - There are two source shapes and the API is only forthcoming about one: - - * a **typed** source (`type: STRIPE`) hides its config entirely — one with - a secret set is byte-identical to one without, confirmed against a source - whose secret was definitely configured. Nothing to read. - * a **generic** source carrying `auth_type: STRIPE` reports that - `auth_type` back, though not the secret. That is a direct answer. - - So this reads `auth_type` when it is there, and falls back to observed - traffic — the `verified` flag on inbound requests — when it is not. The - fallback can confirm a problem but never its absence, and 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] - - # A generic source states its auth_type, so no inference is needed. - configured = ((source.get("config") or {}).get("auth_type") or "").upper() - if configured: - checks.append( - Check( - True, - f"source '{source.get('name')}' is configured to verify as " - f"{configured}", - ) - ) + 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 f2648a4..b2b8a4a 100644 --- a/hookdeck/tools.py +++ b/hookdeck/tools.py @@ -119,10 +119,14 @@ def _payload_text(body: Any) -> str: 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 "body" in body: - inner = body["body"] - return inner if isinstance(inner, str) else json.dumps(inner) + 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) diff --git a/tests/test_cli.py b/tests/test_cli.py index d9d685d..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"}}) @@ -776,14 +778,78 @@ def test_doctor_reads_auth_type_directly_when_the_source_states_it( "response_status_codes": retryable_status_codes()}]}] } fake_api.responses["list_sources"] = { - "models": [{"id": "src_1", "name": "payments", + "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 - # No need to guess from traffic when the source answers directly. - assert not calls_named(fake_api, "list_requests") + 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 3455d8e..dbd9c5b 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -421,10 +421,21 @@ def test_an_unwrapped_string_body_still_works(api): ) -def test_a_wrapper_holding_a_structured_body_is_serialised(api): +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"})) == { - "kind": "structured" + "body": {"kind": "structured"} }