Skip to content

Commit 189ad6f

Browse files
bokelleyclaude
andcommitted
feat(signing): wire SSRF guard into legacy webhooks.deliver()
Closes #299. `adcp.webhooks.deliver()` (the legacy AdCP 3.x HMAC-SHA256 / Bearer auth path) constructed an unpinned `httpx.AsyncClient(timeout=...)` and POSTed to a buyer-controlled URL when no operator client was supplied. No SSRF guard. A buyer-supplied URL pointing at 127.0.0.1 or AWS metadata would deliver successfully — same security hole that PR #297 closed for `WebhookSender`. Surfaced by security-reviewer on PR #297 as L4 — explicitly deferred from the prep PR scope. Now addressed. The fix mirrors the WebhookSender pattern at webhook_sender.py:_send_bytes: - When operator supplies a client, trust them completely (vetted egress proxy with mTLS, ASGI test transport, etc.). - When sender owns the client, build a per-request AsyncIpPinnedTransport via build_async_ip_pinned_transport(url, ...). trust_env=False prevents HTTPS_PROXY env-var bypass. follow_redirects=False prevents rebinding-via-redirect. New kwargs forwarded to the pinned-transport build: - allow_private: bool = False — dev/CI escape hatch for internal endpoints - allowed_ports: frozenset[int] | None = None — opt-in port-allowlist hardening Three regression tests: - test_deliver_owned_client_rejects_loopback_destination - test_deliver_allow_private_dev_escape_hatch - test_deliver_operator_supplied_client_skips_ssrf_guard This is a behavior change for adopters on the legacy `deliver()` path posting to private/internal endpoints (dev/test fixtures); they need to add `allow_private=True` to preserve workflow. Production deployments posting to real buyer URLs are unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 2fdc3d6 commit 189ad6f

2 files changed

Lines changed: 146 additions & 16 deletions

File tree

src/adcp/webhooks.py

Lines changed: 51 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -705,6 +705,8 @@ async def deliver(
705705
extra_headers: Mapping[str, str] | None = None,
706706
timeout_seconds: float | None = None,
707707
token_field: str | None = None,
708+
allow_private: bool = False,
709+
allowed_ports: frozenset[int] | None = None,
708710
) -> httpx.Response:
709711
"""Dispatch one legacy-auth webhook in a single call.
710712
@@ -728,11 +730,23 @@ async def deliver(
728730
:func:`create_mcp_webhook_payload` / :func:`create_a2a_webhook_payload`),
729731
an a2a ``Task`` / ``TaskStatusUpdateEvent``, or a plain dict.
730732
Models are dumped with ``mode="json", exclude_none=True``.
731-
client: Optional shared ``httpx.AsyncClient``. Recommended in
732-
production for connection pooling and egress-policy enforcement
733-
(a custom ``httpx.BaseTransport`` is the right place to block
734-
SSRF to private IPs — the helper validates scheme but cannot
735-
see post-DNS resolution without racing TOCTOU).
733+
client: Optional shared ``httpx.AsyncClient``. When supplied, the
734+
caller owns SSRF guarantees — the helper trusts the operator's
735+
transport completely (typically a vetted egress proxy with
736+
mTLS, or an ASGI transport for testing). When omitted, the
737+
helper builds a per-request :class:`adcp.signing.IpPinnedTransport`
738+
so the URL is resolved, SSRF-validated, and pinned to the
739+
resolved IP — same defense applied to :class:`WebhookSender`.
740+
allow_private: Forwarded to the per-request pinned transport
741+
(owned-client path only). ``False`` (default) rejects URLs
742+
whose resolved IP is in a private / loopback / link-local
743+
range. Set ``True`` for dev/CI fixtures that post to internal
744+
endpoints; production should leave it ``False``.
745+
allowed_ports: Forwarded to the per-request pinned transport
746+
(owned-client path only). ``None`` (default) imposes no port
747+
filter — AdCP doesn't constrain webhook ports. Hardened
748+
deployments pass :data:`adcp.signing.DEFAULT_ALLOWED_PORTS`
749+
(`{443, 8443}`) or a custom set.
736750
extra_headers: Merged last. May not override any of
737751
``Content-Type``, ``Content-Digest``, ``Content-Length``,
738752
``Host``, ``Authorization``, ``Signature``, ``Signature-Input``,
@@ -768,10 +782,13 @@ async def deliver(
768782
DeprecationWarning (fires once): ``authentication`` is a 3.x fallback.
769783
770784
Security notes:
771-
* ``config.url`` is buyer-controlled. The helper enforces HTTPS and
772-
rejects control characters but does NOT block private / link-local
773-
destinations — wire an egress policy via ``client.transport`` to
774-
stop SSRF into your VPC or cloud metadata service.
785+
* ``config.url`` is buyer-controlled. The helper enforces HTTPS,
786+
rejects control characters, AND (on the owned-client path)
787+
builds a per-request IP-pinned transport that runs the full
788+
SSRF range check (loopback / RFC 1918 / link-local / CGNAT /
789+
IPv6 ULA / multicast / cloud metadata) and pins the connection
790+
to the validated IP. Operator-supplied clients skip the SSRF
791+
guard — they own egress policy on their transport.
775792
* ``config.token`` sits in the request body, so any receiver that
776793
logs bodies retains it indefinitely. Treat the token as a
777794
medium-sensitivity correlator, not a long-lived secret.
@@ -862,14 +879,32 @@ async def deliver(
862879
_validate_header_value(f"extra_headers[{key!r}]", value)
863880
headers[key] = value
864881

865-
owns_client = client is None
866882
effective_timeout = timeout_seconds if timeout_seconds is not None else _DEFAULT_TIMEOUT_SECONDS
867-
http_client = client or httpx.AsyncClient(timeout=effective_timeout)
868-
try:
869-
return await http_client.post(url, content=body_bytes, headers=headers)
870-
finally:
871-
if owns_client:
872-
await http_client.aclose()
883+
if client is None:
884+
# Owned-client path: build a per-request IP-pinned transport so
885+
# the URL is SSRF-validated and pinned to the resolved IP, with
886+
# follow_redirects=False (rebinding-via-redirect defense) and
887+
# trust_env=False (HTTPS_PROXY env vars cannot route the request
888+
# away from the pinned target). Mirrors the WebhookSender pattern
889+
# — see adcp.webhook_sender._send_bytes for the same shape.
890+
from adcp.signing.ip_pinned_transport import build_async_ip_pinned_transport
891+
892+
transport = build_async_ip_pinned_transport(
893+
url,
894+
allow_private=allow_private,
895+
allowed_ports=allowed_ports,
896+
)
897+
async with httpx.AsyncClient(
898+
transport=transport,
899+
timeout=effective_timeout,
900+
follow_redirects=False,
901+
trust_env=False,
902+
) as http_client:
903+
return await http_client.post(url, content=body_bytes, headers=headers)
904+
# Operator-supplied client: trust them completely; they own SSRF
905+
# guarantees on their transport (vetted egress proxy, ASGI test
906+
# transport, etc.).
907+
return await client.post(url, content=body_bytes, headers=headers)
873908

874909

875910
def _extract_config_fields(

tests/test_webhooks_deliver.py

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -546,3 +546,98 @@ def test_normalize_passthrough_for_unknown_enum_prefixes() -> None:
546546
assert out["status"]["state"] == "completed"
547547
assert out["status"]["message"]["role"] == "user"
548548
assert out["role"] == "user"
549+
550+
551+
# -- SSRF guard regression tests (parity with WebhookSender) ---------------
552+
553+
554+
@pytest.mark.asyncio
555+
async def test_deliver_owned_client_rejects_loopback_destination() -> None:
556+
"""The legacy ``deliver()`` helper now wires the same per-request
557+
IP-pinned transport that ``WebhookSender._send_bytes`` uses (see #299).
558+
A buyer-supplied URL pointing at 127.0.0.1 must reject before the
559+
POST hits a real socket — the previous unguarded ``httpx.AsyncClient``
560+
would have delivered to the loopback address.
561+
562+
Operator-supplied clients (``client=...``) skip the SSRF guard by
563+
design; they own egress policy on their transport."""
564+
from adcp.signing import SSRFValidationError
565+
566+
config = PushNotificationConfig(
567+
url="https://127.0.0.1/webhooks/adcp",
568+
authentication=PNAuthentication(schemes=["Bearer"], credentials=_BEARER_TOKEN),
569+
)
570+
payload = create_mcp_webhook_payload(
571+
task_id="task_ssrf",
572+
task_type="create_media_buy",
573+
status="completed",
574+
)
575+
576+
with pytest.raises(SSRFValidationError):
577+
await deliver(config, payload)
578+
579+
580+
@pytest.mark.asyncio
581+
async def test_deliver_allow_private_dev_escape_hatch() -> None:
582+
"""Adopters with dev/CI fixtures posting to internal endpoints
583+
pass ``allow_private=True`` to disable the IP-range check. Pin-and-
584+
bind still applies (URL gets resolved + connection pinned), and the
585+
rest of the contract stands — bytes signed == bytes posted."""
586+
from unittest.mock import patch
587+
588+
config = PushNotificationConfig(
589+
url="https://10.0.0.1/webhooks/adcp",
590+
authentication=PNAuthentication(schemes=["Bearer"], credentials=_BEARER_TOKEN),
591+
)
592+
payload = create_mcp_webhook_payload(
593+
task_id="task_priv",
594+
task_type="create_media_buy",
595+
status="completed",
596+
)
597+
598+
# Stub the pinned-transport build with a transport that actually
599+
# accepts a body without opening a socket. We're testing the kwarg
600+
# plumbing, not the network round-trip.
601+
captured: dict[str, Any] = {}
602+
603+
def fake_build(uri: str, **kwargs: Any) -> Any:
604+
captured.update(kwargs)
605+
captured["uri"] = uri
606+
return httpx.MockTransport(lambda _req: httpx.Response(200))
607+
608+
with patch(
609+
"adcp.signing.ip_pinned_transport.build_async_ip_pinned_transport",
610+
side_effect=fake_build,
611+
):
612+
response = await deliver(config, payload, allow_private=True)
613+
614+
assert response.status_code == 200
615+
assert captured["allow_private"] is True
616+
assert captured["uri"] == "https://10.0.0.1/webhooks/adcp"
617+
618+
619+
@pytest.mark.asyncio
620+
async def test_deliver_operator_supplied_client_skips_ssrf_guard() -> None:
621+
"""When the operator provides their own ``httpx.AsyncClient``,
622+
``deliver()`` does NOT build a pinned transport — the operator owns
623+
SSRF on their transport (vetted egress proxy, ASGI test transport).
624+
A request that would fail the SSRF range check under the owned-client
625+
path (resolves to a private/loopback IP) succeeds via the operator's
626+
MockTransport without any DNS lookup."""
627+
# https:// is required by the scheme check; MockTransport captures
628+
# the request without a real socket so SSRF range never fires.
629+
config = PushNotificationConfig(
630+
url="https://operator-trusted.test/webhooks/adcp",
631+
authentication=PNAuthentication(schemes=["Bearer"], credentials=_BEARER_TOKEN),
632+
)
633+
payload = create_mcp_webhook_payload(
634+
task_id="task_op",
635+
task_type="create_media_buy",
636+
status="completed",
637+
)
638+
639+
transport = httpx.MockTransport(lambda _req: httpx.Response(200))
640+
async with httpx.AsyncClient(transport=transport) as client:
641+
response = await deliver(config, payload, client=client)
642+
643+
assert response.status_code == 200

0 commit comments

Comments
 (0)