diff --git a/hookdeck/cli.py b/hookdeck/cli.py index 6dc1ad5..0dab4b0 100644 --- a/hookdeck/cli.py +++ b/hookdeck/cli.py @@ -553,14 +553,28 @@ async def _check_source_verification(api: HookdeckAPI, routes: dict) -> list[Che requests = _models( await api.list_requests(source_id=source.get("id"), limit=10) ) - unverified = [r for r in requests if not r.get("verified")] - - if unverified: + # Only requests that actually carry the field can be judged. Treating + # an absent `verified` as false would turn a listing that simply does + # not report it into "your source is accepting forgeries" — a false + # alarm that sends someone to re-paste a secret that was never wrong. + judged = [r for r in requests if "verified" in r] + unverified = [r for r in judged if not r["verified"]] + + if requests and not judged: + checks.append( + Check( + True, + f"source '{name}' should verify as {scheme}, but this " + "Hookdeck response does not report whether its requests " + "were verified — unconfirmed", + ) + ) + elif unverified: checks.append( Check( False, f"source '{name}' should verify as {scheme}, but " - f"{len(unverified)} of its last {len(requests)} requests " + f"{len(unverified)} of its last {len(judged)} requests " "were not verified — its signing secret is missing or does " "not match the sender's.", note=( @@ -571,11 +585,11 @@ async def _check_source_verification(api: HookdeckAPI, routes: dict) -> list[Che ), ) ) - elif requests: + elif judged: checks.append( Check( True, - f"source '{name}' verified all of its last {len(requests)} " + f"source '{name}' verified all of its last {len(judged)} " f"requests as {scheme}", ) ) diff --git a/tests/test_cli.py b/tests/test_cli.py index e175239..099a76f 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -853,3 +853,63 @@ def test_the_source_is_read_from_the_api_not_from_our_config( assert cli.hookdeck_command(_ns("doctor")) == 1 assert "should verify as GITHUB" in capsys.readouterr().out + + +def test_requests_without_a_verified_field_are_unknown_not_failures( + doctor_env, fake_api, monkeypatch, capsys +): + # A sibling integration measured `/requests` listings that omit `verified`. + # It is present on this project, but if it ever is not, reading a missing + # key as false turns "the API did not say" into "your source is accepting + # forgeries" — and sends someone to re-paste a secret that was fine. + 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": "STRIPE"}] + } + # Requests exist, but none reports the field. + fake_api.responses["list_requests"] = {"models": [{"id": "req_1"}, {"id": "req_2"}]} + _configure(doctor_env, secret="s", cli_config_path="", + routes={"payments": {"source": "payments", "source_type": "STRIPE"}}) + + assert cli.hookdeck_command(_ns("doctor")) == 0 # unknowable, not broken + out = capsys.readouterr().out + assert "does not report whether its requests were verified" in out + assert "were not verified" not in out + + +def test_a_mix_is_judged_only_on_what_reports_itself( + doctor_env, fake_api, monkeypatch, capsys +): + 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": "STRIPE"}] + } + fake_api.responses["list_requests"] = { + "models": [{"verified": False}, {"id": "no_field"}, {"verified": True}] + } + _configure(doctor_env, secret="s", cli_config_path="", + routes={"payments": {"source": "payments", "source_type": "STRIPE"}}) + + assert cli.hookdeck_command(_ns("doctor")) == 1 + # Two judged, not three: the one that said nothing is not counted either way. + assert "1 of its last 2 requests were not verified" in capsys.readouterr().out