Skip to content

Commit 50ae54d

Browse files
bokelleyclaude
andcommitted
feat(signing): apply 2nd-pass review — trust_env, validate-before-sign, tests
Second-pass expert review (security-reviewer, code-reviewer, python-expert, ad-tech-protocol-expert) on the scope-down commit 3fd2c49 surfaced 1 ship-blocker, 2 real bugs, 3 missing tests, and a nit. All addressed. Ship-blocker (security-reviewer): 1. WebhookSender's per-request httpx.AsyncClient missed trust_env=False. httpx defaults trust_env=True, which routes the signed webhook through any HTTPS_PROXY / HTTP_PROXY env var, bypassing the AsyncIpPinnedTransport entirely. Every other pinned-transport callsite in this codebase explicitly sets trust_env=False (default_jwks_fetcher, async_default_jwks_fetcher, revocation_fetcher); the webhook sender was the outlier. An attacker who controls process env (sidecar config, dotenv, malicious cluster egress policy) could otherwise pivot to receiving the signed webhook body. One-line fix at webhook_sender.py:577 with a regression test that asserts the kwarg is set on the per-request client. Bugs (python-expert): 2. Stale docstrings in build_ip_pinned_transport and build_async_ip_pinned_transport claimed allowed_ports defaults to DEFAULT_ALLOWED_PORTS ({443, 8443}) — but the scope-down flipped the default to None (no port filter). Adopters reading the docstring would hit confusing rejections. Updated both to describe the actual behavior. 3. _send_bytes signed the body before SSRF-validating the URL. Restructured so the pinned-transport build (which runs SSRF + port validation) happens first; signing only after validation succeeds. Hostile URLs no longer leave a signed payload in process memory for faulthandler / custom logging hooks to capture on exception. New regression tests (code-reviewer + security-reviewer): 4. test_owned_client_default_allows_non_standard_ports — sender-level positive analog of the validator-level test_ssrf_default_imposes_no_port_filter. Confirms the permissive port default reaches the actual delivery path; AdCP-spec-compliant buyers on :9443 (Tomcat) and similar non-standard ports succeed without explicit allowlist. 5. test_operator_supplied_client_bypasses_ssrf_guard — named regression guard for the documented contract. Without this, a future refactor that mistakenly applies pin-and-bind to both branches would break ASGI-based unit tests and any vetted-egress-proxy deployment that routes via private networks. 6. test_owned_client_ignores_https_proxy_env — regression guard for trust_env=False. Patches HTTPS_PROXY in env, asserts the per-request client constructs with trust_env=False so the proxy is ignored. Code-reviewer nit: 7. Deduplicated DEFAULT_ALLOWED_PORTS rationale block-comment between adcp.signing.jwks (constant definition) and tests/conformance/signing/test_jwks.py. Kept at the constant-definition site; test file points to it. Commit type changed from fix(signing) to feat(signing): The PR adds public surface (DEFAULT_ALLOWED_PORTS export, new kwargs on validate_jwks_uri / resolve_and_validate_host / build_*_pinned_transport / WebhookSender / from_jwk / from_pem) and changes WebhookSender._send_bytes behavior on the owned-client path (now SSRF-validates and pin-binds every delivery). Per semver, additive public-API surface = minor; the security-fix-via-strictening-default is also conventionally a minor bump. release-please should tag this as 4.1.0, not 4.0.1. If squash-merging, the maintainer should use a feat(signing): PR title so the squash subject carries the conventional-commit type that release-please reads. Tests: 2257 passing locally (3 new). Pre-commit clean (black, ruff, mypy, bandit). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 3fd2c49 commit 50ae54d

4 files changed

Lines changed: 205 additions & 25 deletions

File tree

src/adcp/signing/ip_pinned_transport.py

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -295,11 +295,14 @@ def build_ip_pinned_transport(
295295
"""Resolve ``uri`` once and return a transport pinned to the validated IP.
296296
297297
Raises :class:`SSRFValidationError` if the URI's scheme isn't
298-
``http``/``https``, the port is not in the allowlist, the host
299-
doesn't resolve, or every resolved IP is in a blocked range.
298+
``http``/``https``, ``allowed_ports`` is set and the URI's port is
299+
outside it, the host doesn't resolve, or every resolved IP is in a
300+
blocked range.
300301
301-
``allowed_ports`` defaults to
302-
:data:`adcp.signing.jwks.DEFAULT_ALLOWED_PORTS` (`{443, 8443}`).
302+
``allowed_ports`` defaults to ``None`` (no port filter — AdCP
303+
doesn't constrain webhook ports). Hardened deployments pass
304+
:data:`adcp.signing.jwks.DEFAULT_ALLOWED_PORTS` (`{443, 8443}`)
305+
or a custom set.
303306
304307
Typical use inside a fetcher::
305308
@@ -328,8 +331,9 @@ def build_async_ip_pinned_transport(
328331
function itself is not awaitable. The returned transport plugs
329332
into :class:`httpx.AsyncClient`.
330333
331-
``allowed_ports`` defaults to
332-
:data:`adcp.signing.jwks.DEFAULT_ALLOWED_PORTS` (`{443, 8443}`).
334+
``allowed_ports`` defaults to ``None`` (no port filter); see
335+
:func:`build_ip_pinned_transport` for the hardening kwarg
336+
semantics.
333337
"""
334338
hostname, resolved_ip, _port = resolve_and_validate_host(
335339
uri,

src/adcp/webhook_sender.py

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,10 @@
4444
load_private_key_pem,
4545
private_key_from_jwk,
4646
)
47-
from adcp.signing.ip_pinned_transport import build_async_ip_pinned_transport
47+
from adcp.signing.ip_pinned_transport import (
48+
AsyncIpPinnedTransport,
49+
build_async_ip_pinned_transport,
50+
)
4851
from adcp.signing.webhook_signer import sign_webhook
4952
from adcp.types import GeneratedTaskStatus
5053
from adcp.types.generated_poc.core.async_response_data import AdcpAsyncResponseData
@@ -505,7 +508,26 @@ async def _send_bytes(
505508
with mTLS to a known buyer set, or an ASGI transport for testing),
506509
the sender trusts the operator's transport completely. Pin-and-bind
507510
is skipped; the operator's transport owns SSRF.
511+
512+
On the owned-client path, SSRF validation runs **before** signing
513+
so a hostile URL is rejected without first generating an
514+
Ed25519/ES256 signature over the body. That signature would
515+
otherwise sit in process memory until the SSRF rejection —
516+
anything that snapshots locals on exception (faulthandler,
517+
custom logging) could capture it. Validate first, sign second.
508518
"""
519+
# Build the pinned transport up-front for the owned-client path.
520+
# This runs SSRF + port validation against the URL before any
521+
# signing happens; a hostile URL raises SSRFValidationError here
522+
# and the body never gets signed.
523+
transport: AsyncIpPinnedTransport | None = None
524+
if self._owns_client:
525+
transport = build_async_ip_pinned_transport(
526+
url,
527+
allow_private=self._allow_private_destinations,
528+
allowed_ports=self._allowed_destination_ports,
529+
)
530+
509531
base_headers = {"Content-Type": "application/json"}
510532
signed = sign_webhook(
511533
method="POST",
@@ -529,27 +551,26 @@ async def _send_bytes(
529551
for k, v in extra_headers.items():
530552
headers[k] = v
531553

532-
if self._owns_client:
533-
# Per-request pinned transport. Building one client per delivery
534-
# is the security-correct choice: keeping a pinned transport
535-
# alive across deliveries to the same hostname would defeat the
536-
# rebinding defense (the IP would be frozen at first delivery).
537-
transport = build_async_ip_pinned_transport(
538-
url,
539-
allow_private=self._allow_private_destinations,
540-
allowed_ports=self._allowed_destination_ports,
541-
)
554+
if transport is not None:
555+
# Owned-client path. ``trust_env=False`` prevents httpx from
556+
# routing the request through ``HTTPS_PROXY`` / ``HTTP_PROXY``
557+
# env vars — every other pinned-transport callsite in the
558+
# codebase sets this for the same reason (default_jwks_fetcher,
559+
# async_default_jwks_fetcher, revocation_fetcher). Without it,
560+
# an attacker who controls process env can route the signed
561+
# webhook through their endpoint, defeating the IP pin entirely.
542562
async with httpx.AsyncClient(
543563
transport=transport,
544564
timeout=self._timeout,
545565
follow_redirects=False,
566+
trust_env=False,
546567
) as client:
547568
response = await client.post(url, content=body, headers=headers)
548569
else:
549570
# Operator-supplied client — they own the SSRF guarantees on
550571
# their transport (proxy allowlist, mTLS, etc.). Reachable as
551-
# None after aclose(); a runtime check beats an assert that
552-
# python -O strips to silently NoneType.post().
572+
# None after aclose(); explicit raise survives ``python -O``
573+
# which would strip an assert.
553574
if self._client is None:
554575
raise RuntimeError(
555576
"WebhookSender's operator-supplied client was already "

tests/conformance/signing/test_jwks.py

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,7 @@ def test_ssrf_caps_resolved_address_scan() -> None:
101101

102102

103103
# ---- Port allowlist (opt-in operator hardening) ----
104-
#
105-
# AdCP doesn't constrain webhook ports in the spec, so the default validator
106-
# imposes no port filter. Adopters who want a hardening posture pass
107-
# ``allowed_ports=DEFAULT_ALLOWED_PORTS`` (or a custom set); rejection of
108-
# non-standard ports closes a smuggle vector for buyers bouncing traffic to
109-
# internal services on the same routable IP.
104+
# Rationale lives in adcp.signing.jwks.DEFAULT_ALLOWED_PORTS docstring.
110105

111106

112107
@pytest.mark.parametrize(

tests/conformance/signing/test_webhook_sender_e2e.py

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -487,3 +487,163 @@ async def echo(request: Request, op_id: str) -> JSONResponse:
487487

488488
assert result.ok
489489
assert captured_payloads[0]["operation_id"] == "op_abc123"
490+
491+
492+
@pytest.mark.asyncio
493+
async def test_owned_client_default_allows_non_standard_ports() -> None:
494+
"""The default ``WebhookSender`` (no operator client, no
495+
``allowed_destination_ports``) accepts AdCP-spec-compliant buyers on
496+
non-standard ports — :9443 (Tomcat default), :4443 (Spring Boot
497+
default), path-routed multi-tenant gateways. The IP-range check still
498+
applies; we just don't impose a port filter unless the operator
499+
explicitly opts in.
500+
501+
Sender-level positive analog of test_ssrf_default_imposes_no_port_filter
502+
in test_jwks.py — confirms the permissive default reaches the actual
503+
delivery path, not just the underlying validator."""
504+
from unittest.mock import patch
505+
506+
captured: list[tuple[str, int]] = []
507+
app = FastAPI()
508+
509+
@app.post("/webhooks/adcp")
510+
async def echo(_request: Request) -> JSONResponse:
511+
return JSONResponse({"ok": True}, status_code=200)
512+
513+
asgi_transport = httpx.ASGITransport(app=app)
514+
515+
# Stub the pinned-transport build so we don't open a real socket to
516+
# a public IP; capture that the build was attempted for the
517+
# non-standard port and route the actual POST through ASGI.
518+
def fake_build(uri: str, **_kwargs: Any) -> Any:
519+
from urllib.parse import urlparse
520+
521+
parsed = urlparse(uri)
522+
captured.append((parsed.hostname or "", parsed.port or 443))
523+
return asgi_transport
524+
525+
sender = WebhookSender.from_jwk(
526+
{**WEBHOOK_JWK, "d": WEBHOOK_JWK["_private_d_for_test_only"]},
527+
)
528+
with patch(
529+
"adcp.webhook_sender.build_async_ip_pinned_transport",
530+
side_effect=fake_build,
531+
):
532+
async with sender:
533+
result = await sender.send_mcp(
534+
url="http://test:9443/webhooks/adcp",
535+
task_id="task_nonstd",
536+
task_type="create_media_buy",
537+
status="completed",
538+
)
539+
540+
assert result.ok
541+
assert captured == [("test", 9443)]
542+
543+
544+
@pytest.mark.asyncio
545+
async def test_operator_supplied_client_bypasses_ssrf_guard() -> None:
546+
"""When the operator passes their own httpx client (vetted egress
547+
proxy, ASGI test transport, etc.), the framework trusts them
548+
completely — pin-and-bind is skipped and the SSRF range check does
549+
NOT fire. The operator owns SSRF on their transport.
550+
551+
Named regression test for the documented contract; without this, a
552+
future refactor that mistakenly applies pin-and-bind to both
553+
branches breaks ASGI-based unit tests and any vetted-proxy
554+
deployments that route via private networks."""
555+
app = FastAPI()
556+
557+
@app.post("/webhooks/adcp")
558+
async def echo(_request: Request) -> JSONResponse:
559+
return JSONResponse({"ok": True}, status_code=200)
560+
561+
transport = httpx.ASGITransport(app=app)
562+
# base_url is loopback-equivalent. With the SSRF guard active this
563+
# would raise SSRFValidationError; under the operator-trust contract
564+
# it must succeed.
565+
async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
566+
sender = WebhookSender.from_jwk(
567+
{**WEBHOOK_JWK, "d": WEBHOOK_JWK["_private_d_for_test_only"]},
568+
client=client,
569+
)
570+
result = await sender.send_mcp(
571+
url="http://test/webhooks/adcp",
572+
task_id="task_op_trust",
573+
task_type="create_media_buy",
574+
status="completed",
575+
)
576+
577+
assert result.ok
578+
579+
580+
@pytest.mark.asyncio
581+
async def test_owned_client_ignores_https_proxy_env() -> None:
582+
"""``HTTPS_PROXY`` / ``HTTP_PROXY`` env vars MUST NOT defeat the IP
583+
pin on the owned-client path. httpx's default ``trust_env=True``
584+
routes requests through proxy env vars, which would bypass the
585+
AsyncIpPinnedTransport's network_backend entirely — an attacker who
586+
controls process env (sidecar config, dotenv, malicious cluster
587+
egress policy) could otherwise pivot to receiving the signed
588+
webhook body.
589+
590+
The sender constructs its per-request ``httpx.AsyncClient`` with
591+
``trust_env=False`` to close this. Regression guard: if a future
592+
refactor drops the kwarg, this test catches it by setting a proxy
593+
env var that points at an unreachable address; the pinned transport
594+
must still route via the resolved IP and reach the test ASGI app
595+
(which we can't directly observe under a real socket, so we assert
596+
the proxy var is ignored by checking the constructor config)."""
597+
import os
598+
from unittest.mock import MagicMock, patch
599+
600+
sender = WebhookSender.from_jwk(
601+
{**WEBHOOK_JWK, "d": WEBHOOK_JWK["_private_d_for_test_only"]},
602+
)
603+
604+
captured_kwargs: dict[str, Any] = {}
605+
606+
class _FakeAsyncClient:
607+
def __init__(self, **kwargs: Any) -> None:
608+
# Capture kwargs only from the per-request construction in
609+
# _send_bytes; __aenter__'s eager _get_client() also flows
610+
# through here but its kwargs don't affect the per-request
611+
# delivery path. The last writer wins; the per-request call
612+
# is the one we care about.
613+
captured_kwargs.update(kwargs)
614+
self._response = MagicMock(status_code=200, headers={}, content=b"{}")
615+
616+
async def __aenter__(self) -> _FakeAsyncClient:
617+
return self
618+
619+
async def __aexit__(self, *_args: Any) -> None:
620+
return None
621+
622+
async def aclose(self) -> None:
623+
return None
624+
625+
async def post(self, *_args: Any, **_kwargs: Any) -> Any:
626+
return self._response
627+
628+
# Set HTTPS_PROXY pointing at an unreachable address. With
629+
# trust_env=True (the httpx default), this would override the
630+
# transport. The sender MUST pass trust_env=False to ignore it.
631+
with patch.dict(os.environ, {"HTTPS_PROXY": "http://attacker.invalid:9999"}):
632+
with patch(
633+
"adcp.webhook_sender.build_async_ip_pinned_transport",
634+
return_value=MagicMock(), # transport itself isn't used here
635+
):
636+
with patch("adcp.webhook_sender.httpx.AsyncClient", _FakeAsyncClient):
637+
async with sender:
638+
await sender.send_mcp(
639+
url="https://buyer.example.com/webhooks/adcp",
640+
task_id="task_proxy",
641+
task_type="create_media_buy",
642+
status="completed",
643+
)
644+
645+
assert captured_kwargs.get("trust_env") is False, (
646+
"WebhookSender's per-request httpx.AsyncClient must construct with "
647+
"trust_env=False — otherwise HTTPS_PROXY env vars defeat the IP pin"
648+
)
649+
assert captured_kwargs.get("follow_redirects") is False

0 commit comments

Comments
 (0)