feat: add webhook URL policy and wholesale feed sender#833
Merged
Conversation
c381d06 to
24e3ea3
Compare
Contributor
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
Contributor
There was a problem hiding this comment.
LGTM. Clean fix. Right shape — registration-time policy is the same SSRF classifier as WebhookSender, just hoisted to a typed pre-store guard so sellers can fail closed without importing JWKS internals.
Things I checked
- SSRF defense ordering at
src/adcp/webhooks.py:965-1057: control-char input →apply_hooks→ post-hook control-char re-check → userinfo → fragment → scheme → require_https →resolve_and_validate_host. Right order. Fragment is rejected before DNS — proven by thefail_getaddrinfopoison-pill attests/test_webhook_destination_policy.py:84-93. - Post-hook control-char re-check at
webhooks.py:986-994is load-bearing —apply_hooksdoesn't strip CRLF that a custom transport hook could inject. - DNS rebinding closed at send time:
WebhookSender._send_bytesrebuilds a freshAsyncIpPinnedTransportviabuild_async_ip_pinned_transport(effective_url, ...)on every delivery, so the helper'sresolved_ipis informational, not authoritative.signing/jwks.py:266-267blocks cloud-metadata IPs before theallow_privategate —test_local_development_still_rejects_cloud_metadataconfirms. WebhookDestinationValidationError.to_error()atwebhooks.py:893-901returns only{code, message, field?, suggestion?}.effective_urldoes not leak through.- Wire envelope correct:
WholesaleFeedWebhookenvelope atwebhook_sender.py:778-791mapsnotification_id ← event.event_idperwholesale_feed_webhook.pydescription ("MUST equal event.event_id").cache_scope == event.payload.applies_to.scopecoupling atwebhook_sender.py:768-774mirrors the schema constraint. _entity_type_for_wholesale_notificationatwebhook_sender.py:131-141covers all nine upstream notification types (product.*→product,signal.*→signal,wholesale_feed.bulk_change→feed). Complete partition againstEntityType.- Exports are additive only —
__init__.pyandtests/fixtures/public_api_snapshot.jsondiffs are all+.feat:is the right semver. - No
# type: ignore, no test skips, no disabled CI checks in the diff.
Follow-ups (non-blocking — file as issues)
extra_headersis unvalidated on theWebhookSenderpath. Pre-existing —_send_bytesonly runsmerge_extra_headers's reserved-key check, not the_validate_header_value+_MAX_EXTRA_HEADERScap that the legacydeliver()inwebhooks.py:1268-1281enforces. The newsend_wholesale_feed*methods inherit this gap and widen the surface — wholesale-catalog notifications are exactly where sellers will want to echo buyer-controlled correlation IDs intoX-Buyer-Trace-Id. Fix at the helper boundary (_send_bytes), not per method. Fromsecurity-reviewer.subscription_event_typesis opt-in on the low-level method.send_wholesale_feed_to_subscriptionalways passes it through, but the lower-level entry point treats the subscription filter as optional ("optional but recommended" atwebhook_sender.py:726-729). The schema description onnotification_config.event_typessays sellers MUST NOT fire other types against the endpoint. Consider tightening to required, or at minimum log when absent. Fromad-tech-protocol-expert.WholesaleFeedEventhas no open-union forward-compat. UnlikeFormat.assets(patched via_forward_compat.py), theWholesaleFeedEventdiscriminator is strict. If AdCP 3.2 adds a tenth event type, this client hard-fails on receive. Worth confirming the upstream contract — the spec text nearwholesale_feed_event.py:656already says consumers MUST tolerate unknownrecommendationenum values, hinting at the same posture forevent_type. Fromad-tech-protocol-expert.
Minor nits (non-blocking)
_validate_policy_hookslets hookValueErrorpropagate raw.src/adcp/webhooks.py:925-929. Callers that catch onlyWebhookDestinationValidationErrorto map toINVALID_REQUESTwill miss this. Wrap inWebhookDestinationValidationError(reason="transport_hook_misconfigured", ...)for consistency. Arguably this is dev misconfiguration at construction time, not buyer input — defensible to leave.- Tests skip a few negative branches.
tests/test_wholesale_feed_webhook_sender.pyhas no negative case forcache_scopemismatch (webhook_sender.py:770) orentity_typemismatch (webhook_sender.py:762).tests/test_webhook_destination_policy.pyhas no negative case for embedded userinfo (webhooks.py:997-1006) or post-hook control-char re-check (webhooks.py:986-994). Happy paths plus the user's stated checklist are covered; SSRF-adjacent branches deserve a negative each. subscriber_idvalidation is looser than the schema.webhook_sender.py:732accepts any non-empty string; schema requires^[A-Za-z0-9_.:-]{1,64}$. Pydantic catches it onmodel_validateat line 778, but the failure surface is aValidationErrorrather than the sender's ownValueError— inconsistent with the explicit pre-checks foraccount_id/wholesale_feed_version. Either delete the three pre-checks (let Pydantic enforce) or add the regex here for symmetry. Same foridempotency_key(^[A-Za-z0-9_.:-]{16,255}$).
The lower-level entry point makes the "do not silently widen account notification filters" contract opt-in via subscription_event_types=None, and the schema says MUST — notable choice for the public surface.
LGTM. Follow-ups noted below.
This was referenced May 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
push_notification_config.urlandaccounts[].notification_configs[].urlNotificationConfig,WholesaleFeedEvent, andWholesaleFeedWebhookfrom stableadcpandadcp.typespathsWebhookSender.send_wholesale_feed(...)andsend_wholesale_feed_to_subscription(...)for signed wholesale product/signal feed notifications with event/account/subscriber validationWhy
Downstream sellers need to reject unsafe durable webhook subscriptions before storing them, without importing JWKS-named internals or reimplementing SSRF policy locally. Sellers also need stable AdCP 3.1 catalog notification types and a typed sender path instead of hand-building wholesale feed envelopes around
send_raw(...).Validation
PYTHONPATH=src pytest tests/test_webhook_destination_policy.py tests/test_wholesale_feed_webhook_sender.py tests/test_public_api.py tests/test_catalog_types.py tests/test_webhooks_deliver.py tests/conformance/signing/test_webhook_sender_e2e.py tests/conformance/signing/test_webhook_sender_alt_auth.py -qPYTHONPATH=src python3 -m ruff check src/adcp/webhooks.py src/adcp/webhook_sender.py src/adcp/types/__init__.py src/adcp/__init__.py tests/test_webhook_destination_policy.py tests/test_wholesale_feed_webhook_sender.py tests/test_public_api.pyPYTHONPATH=src python3 -m mypy src/adcp/webhooks.py src/adcp/webhook_sender.pygit diff --check