Skip to content

feat: add webhook proof-of-control helper#843

Merged
bokelley merged 4 commits into
mainfrom
bokelley/webhook-proof-helper
May 24, 2026
Merged

feat: add webhook proof-of-control helper#843
bokelley merged 4 commits into
mainfrom
bokelley/webhook-proof-helper

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Summary

  • add a durable webhook proof-of-control challenge helper for active notification_configs[]
  • support RFC 9421 WebhookSender challenges plus legacy Bearer/HMAC authentication
  • validate destination URLs, require exact challenge echo via challenge or token, and surface typed errors for seller handlers
  • tighten webhook sender reserved headers so callers cannot override Host, Content-Length, auth, or signature-binding headers

Closes #839.

Validation

  • Expert review: code-reviewer and security-reviewer passes after fixes
  • PYTHONPATH=src python3 -m ruff check src/
  • PYTHONPATH=src python3 -m mypy src/adcp/
  • PYTHONPATH=src python3 -m mypy --strict tests/type_checks/
  • PYTHONPATH=src python3 -m pytest tests/ -q

Comment thread src/adcp/webhooks.py Fixed
Comment thread src/adcp/webhooks.py Fixed
@bokelley bokelley marked this pull request as ready for review May 24, 2026 01:31
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 24, 2026
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Clean fix for #839 with the right shape: a registration-time helper that layers on top of validate_webhook_destination_url's SSRF guard, refuses operator-supplied client= / sender transport_hooks, and surfaces typed WebhookChallengeErrors sellers can map to INVALID_REQUEST in sync_accounts errors.

Things I checked

  • SSRF guard is intact end-to-end. validate_webhook_destination_url at src/adcp/webhooks.py:1377 returns destination.effective_url; same string is passed to both build_async_ip_pinned_transport and http_client.post in _send_legacy_webhook_challenge (src/adcp/webhooks.py:1237, :1283). follow_redirects=False + trust_env=False both set.
  • Reserved-header tightening at src/adcp/webhook_auth.py:52 (content-length, host) plus pseudo-header rejection at webhook_auth.py:206-209. _RESERVED_HEADERS at webhooks.py:769-781 already covers authorization, signature, signature-input, content-digest, x-adcp-signature, x-adcp-timestamp — auth-binding surface is complete.
  • No credential leak in WebhookChallengeError.to_error() at webhooks.py:943-951 — emits only {code, message, field, suggestion}. Bearer/HMAC value is interpolated only into outgoing Authorization / X-AdCP-Signature headers (webhooks.py:1252, :1254), never into raised messages.
  • Sender hardening rationale (webhooks.py:1334-1349) is load-bearing: a sender-owned client= bypasses the pinned-transport path, and a sender transport_hook could swap to a different validated host after validate_webhook_destination_url has already pinned the original IP. For proof-of-control the pinned IP must match the URL the seller is about to persist.
  • Public API: pure additions to src/adcp/__init__.py (6 new exports) with matching snapshot bump in tests/fixtures/public_api_snapshot.json. feat: is the right conventional-commit prefix — no removal, no signature change.

Follow-ups (non-blocking — file as issues)

  1. TOCTOU: SSRFValidationError escapes the wrapper. _send_legacy_webhook_challenge at webhooks.py:1237 builds the pinned transport inside the second try block at webhooks.py:1400-1448. SSRFValidationError does not subclass ValueError / httpx.HTTPError / httpx.TimeoutException, so a DNS-rebind race between validate_webhook_destination_url and connect raises an unwrapped SSRFValidationError instead of a typed WebhookChallengeError(reason="ssrf_rejected"). Security still holds — the SSRF check fires — but the typed error contract leaks. Add an except SSRFValidationError arm.
  2. Spec the wire shape upstream. ad-tech-protocol-expert flagged: schemas/cache/3.1.0-beta.3/core/notification-config.json:17 calls for "an activation challenge or equivalent proof-of-control" but does not pin a payload shape, type discriminator value, or echo field. This SDK is effectively defining the convention. Before durable adoption, file an AdCP spec PR pinning (a) type: "webhook.challenge", (b) required fields, (c) challenge as canonical with token as deprecated alias. Otherwise the next SDK invents its own shape and sellers reject each other's challenges.
  3. Test coverage gaps in tests/test_webhook_challenge.py: httpx.TimeoutExceptionreason="timeout", httpx.HTTPError (non-timeout, non-status) → reason="request_failed", invalid JSON body → reason="invalid_json", non-mapping JSON (b'[1,2,3]') → reason="invalid_json", missing-echo (body with neither key) → reason="missing_echo", plus ambiguous_auth_mode, ambiguous_client, ambiguous_timeout, sender_required guards are unhit. Only challenge_mismatch and http_status exercised end-to-end.

Minor nits (non-blocking)

  1. hmac.compare_digest for echo check. webhooks.py:1019 uses plain ==. Not exploitable — secrets.token_urlsafe(32) gives 256 bits of single-use entropy — but every other secret-shaped comparison in the signing stack uses hmac.compare_digest. Cheap consistency win.
  2. Private-attribute brittleness. webhooks.py:1334-1335 reads _owns_client / _transport_hooks via getattr(cast(Any, sender), ..., default). A future WebhookSender subclass that drops those attributes silently passes the proof-of-control safety check (fails open). Promote to a public proof_of_control_safe property on WebhookSender so subclassing fails closed.
  3. transfer-encoding not in _BASE_RESERVED. webhook_auth.py:52. httpx blocks it at the transport, but the comment at _validate_header_value says the helper boundary is meant to be self-sufficient. Add for consistency.
  4. Reserved-header tightening is silently breaking-adjacent. Adding host, content-length to _BASE_RESERVED will start rejecting Host/Content-Length from any pre-existing caller passing them to send_raw / send_mcp / etc. Right call — overriding Host defeats SSRF — but worth a release-note line so adopters know to migrate.
  5. Deprecation-warning order. webhooks.py:1231 fires _warn_auth_deprecation_once() before validating extra_headers at :1267-1273. A caller passing an invalid header still emits the deprecation warning. Reorder.

Approving on the strength of the SSRF / pinned-transport / sender-hardening invariants holding end-to-end. The TOCTOU wrapper gap and the spec-upstream work are real but neither blocks ship.

Comment thread src/adcp/webhooks.py Fixed
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 24, 2026
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Fail-closed sender contract (no operator-supplied client, no transport hooks, RFC 9421 only) plus the post-hook signature-target consistency makes this safe to ship.

Things I checked

  • SSRF / DNS-rebind TOCTOU. _post_managed_webhook_challenge posts to destination.effective_url and builds a fresh build_async_ip_pinned_transport against it. The connect-time IP pin is the load-bearing guarantee, not the validator's first resolution. src/adcp/webhooks.py:1325-1337.
  • Signature target equals connect target. Both _send_sender_webhook_challenge:1306 and _send_legacy_webhook_challenge:1272 sign + POST destination.effective_url (post-hook). A rewrite cannot land the signed body elsewhere.
  • Host / pseudo-header override closed. _BASE_RESERVED in src/adcp/webhook_auth.py:52 adds content-length, host; merge_extra_headers:206-207 rejects HTTP/2 pseudo-headers (:authority). Asserted at tests/test_webhook_challenge.py:356-359.
  • trust_env=False, follow_redirects=False at webhooks.py:1334-1335 — closes env-proxy and 3xx-exfil paths.
  • No credentials in the challenge body. create_webhook_challenge_payload:961-981 emits only {type, challenge, account_id, subscriber_id}.
  • send_webhook_challenge (sender helper) is wire-clean. Body is pre-serialized without idempotency_key; _send_bytes does not set an Idempotency-Key header. The challenge value only surfaces on WebhookDeliveryResult.idempotency_key for caller traceability.
  • Public-API snapshot is additions-only. validate_webhook_destination_url's str → str | AnyUrl is a covariant input widening. feat: is the right semver signal.

Follow-ups (non-blocking — file as issues)

  • Pin the wire shape in the spec. core/notification-config.json says only "activation challenge or equivalent proof-of-control." type: "webhook.challenge" and the challenge/token echo-key tolerance are this SDK's calls. The dotted reverse-DNS discriminator value is also notable — every other AdCP envelope I checked uses snake_case notification_type / task_type. File an AdCP RFC pinning the discriminator + echo key, align with the JS SDK, and mark this helper provisional in the docstring until then.
  • Preflight ordering puts DNS before payload validation. challenge_webhook_destination:1410-1416 runs validate_webhook_destination_url before create_webhook_challenge_payload. So account_id="" / subscriber_id="" / challenge="" callers pay the resolution round-trip before hitting invalid_configuration. tests/test_webhook_challenge.py:1284 reads green only because _public_dns(monkeypatch) stubs DNS. Reorder: build the payload first, validate the URL second.
  • _validate_header_value:1980 doesn't catch Unicode whitespace. The boundary contract advertises broad control-char rejection but only forbids \r\n\x00. The legacy Bearer/HMAC credential path runs values through this gate at webhooks.py:1237. U+00A0 encodes cleanly to one latin-1 byte. Not header injection — but tighten to value.encode("latin-1") round-trip or value.isprintable() to match the docstring.
  • _BASE_RESERVED widening is a behavior change shipped under feat:. Adopters passing extra_headers={"Host": "..."} to any WebhookSender.send_* now raise ValueError. Hardening is right; call it out in the release notes when the version cuts.
  • Test coverage gaps. No coverage for missing_echo, invalid_json, timeout, ambiguous_auth_mode, or sender_required. Each is ~6 lines.

Minor nits (non-blocking)

  1. Sender-internals access via getattr / cast(Any, sender). webhooks.py:1297, 1373-1374, 1439 reach into sender._auth / _owns_client / _transport_hooks / _timeout. A rename inside webhook_sender.py:221-225 silently breaks the helper at runtime. Either expose read-only properties on WebhookSender, or move the helper into webhook_sender so the access is intra-module.
  2. error_url="" on early failures. webhooks.py:1365: str("") == "", so ambiguous_auth_mode / sender_required errors surface url="" rather than None. Cosmetic.

LGTM. Follow-ups noted below.

Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Fail-closed sender mode guards plus SSRF-pinned transport on every challenge path make this the right shape for proof-of-control.

Things I checked

  • Sender preconditions at src/adcp/webhooks.py:1366-1409 reject all five unsafe combinations: ambiguous auth mode, custom client=, sender transport_hooks, non-RFC9421 sender, and the both-None case. No flag bypasses the IP pin.
  • Both challenge paths route through the tightened reserved-header set. _BASE_RESERVED at webhook_auth.py:52 now includes host, content-length; pseudo-header reject (:authority) at webhook_auth.py:207. test_challenge_webhook_destination_rejects_sender_host_override covers it.
  • follow_redirects=False, trust_env=False at webhooks.py:1334-1335 blocks redirect-to-metadata pivots and proxy exfil of the legacy credential.
  • secrets.token_urlsafe(32) at :958 — 256 bits, adequate. wch_ prefix is fine.
  • WebhookChallengeError carries code/message/reason/field/url/status_code/suggestion; none echo the Bearer/HMAC credential. _validate_header_value at :1237 rejects CR/LF before the credential reaches a header.
  • Conventional-commit feat: is correct — additive exports only. validate_webhook_destination_url widening from str to str | AnyUrl at :1060 is parameter widening, backward-compatible.
  • tests/fixtures/public_api_snapshot.json updated for new exports; no drift.

Follow-ups (non-blocking — file as issues)

  • Unbounded response body read. webhooks.py:1451, 1464 calls response.content against the buyer-controlled URL with no size cap. The 10s timeout bounds latency but not bytes; a buyer pointing notification_configs[].url at a multi-GB endpoint can OOM the seller during challenge. Outbound has _MAX_BODY_BYTES = 10 * 1024 * 1024 at :790; inbound should mirror it. Stream the response with a running cap and raise WebhookChallengeError(reason=\"response_too_large\"). Security medium.
  • Echo field ambiguity. validate_webhook_challenge_response:1018 accepts either challenge or token. Two field names creates a permanent ambiguity matrix — every future receiver picks one, senders must support both forever. PubSubHubbub, Slack url_verification, Stripe all converge on a single field. Resolve to challenge, treat token as a deprecation-warning interop shim, before this hardens into the de facto Python wire shape.
  • Challenge body carries account_id and subscriber_id. create_webhook_challenge_payload:976-981. Proof-of-control orthodoxy sends the nonce only — including IDs tempts receivers to validate against local state (wrong: the only invariant a challenge proves is URL control) and leaks ID-shape to URLs that do not yet have a seller relationship.
  • Discriminator divergence. :977 emits \"type\": \"webhook.challenge\", but every other AdCP webhook in this repo discriminates on notification_type. Either align or RFC the new top-level discriminator upstream — silent divergence is the wrong outcome for the Python reference implementation.
  • Blanket INVALID_REQUEST for transport errors. WebhookChallengeError.code = \"INVALID_REQUEST\" at :925 maps every failure — timeout, request_failed, ssrf_rejected, http_status — to the buyer-input error bucket. Transport failures are not buyer-input issues. Derive code from self.reason: timeout/request_failedUPSTREAM_UNAVAILABLE, unsafe_sender_client/ambiguous_auth_mode → seller-config error.
  • Private-attribute access into WebhookSender. _send_sender_webhook_challenge:1297 (_auth), :1373 (_owns_client), :1374 (_transport_hooks), :1439 (_timeout). Four sibling-module private attrs with no contract — a refactor in webhook_sender.py AttributeErrors here. Expose typed properties or an internal _proof_of_control_handle().
  • except ValueError swallows WebhookChallengeError. webhooks.py:1479. Since WebhookChallengeError inherits ValueError, any future validation inside _send_*_webhook_challenge that raises WebhookChallengeError gets re-wrapped with reason=\"invalid_configuration\", masking the original reason. Narrow the catch or drop the ValueError base.

Minor nits (non-blocking)

  1. Constant-time echo compare. webhooks.py:1020 uses value == challenge. Single-use 256-bit token, seller-side compare — not exploitable in practice. hmac.compare_digest(str(value), challenge) is the defense-in-depth one-liner.
  2. send_webhook_challenge returns a label, not a replay key. webhook_sender.py:866-872 passes challenge_value as idempotency_key to _send_bytes, but the challenge body intentionally omits the idempotency_key field. The returned WebhookDeliveryResult.idempotency_key is therefore a log label — a caller who feeds it into resend() will not dedupe on the receiver. Worth one docstring sentence.
  3. error_url not scrubbed for embedded userinfo. webhooks.py:1365 computes error_url = str(url) before validate_webhook_destination_url strips user:pass@. The early sender-mode guards bake the unscrubbed URL into WebhookChallengeError.url. Buyer's own credential, so self-leak, but worth sanitizing at the boundary.

The three-commit arc (b1e5629f10e458105242f7cb) lands fine — the unreachable guard at the end was load-bearing for mypy in one revision and dead in the next.

LGTM. Follow-ups noted below.

@bokelley bokelley merged commit 232f7ef into main May 24, 2026
23 checks passed
@bokelley bokelley deleted the bokelley/webhook-proof-helper branch May 24, 2026 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add helper for durable webhook proof-of-control challenges

1 participant