From ca21b4a7a13a8fcf2820b58ae7700284216ae140 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Wed, 29 Apr 2026 20:09:24 -0400 Subject: [PATCH] fix(signing): validate-before-sign symmetry in legacy deliver(), HMAC SSRF coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small follow-ups from the PR #303 second-pass review (security-reviewer + code-reviewer flagged each as low-priority but worth doing for symmetry): 1. validate-before-sign in webhooks.deliver() — mirror WebhookSender ordering. The pinned-transport build (which runs SSRF + port validation) now runs BEFORE body assembly + HMAC computation. A buyer-supplied 127.0.0.1 URL raises SSRFValidationError before get_adcp_signed_headers_for_webhook is called, so the HMAC-over-buyer-body never sits in process memory waiting for the rejection (anything that snapshots locals on exception cannot capture an HMAC that wasn't computed). Matches the WebhookSender._send_bytes pattern shipped in PR #297. Regression test test_owned_client_rejects_hostile_url_before_hmac_signing patches get_adcp_signed_headers_for_webhook with a MagicMock and asserts it's never called. 2. HMAC-SHA256 SSRF coverage — the existing test_owned_client_rejects_loopback_destination only exercised the Bearer auth path. Both auth paths route through the same SSRF guard but the tests should cover both for parity. Added test_owned_client_rejects_loopback_destination_hmac_path. 3. .gitignore — exclude .claude/scheduled_tasks.lock (Conductor harness runtime state). Plus migration-guide section #4 covering the signing-prep behavior changes landing in 4.1: SSRF guards on WebhookSender + deliver(), and the covers_content_digest default flip from "required" to "either" (per AdCP 3.0 spec). Lists the opt-out kwargs adopters who relied on the prior defaults need to add. Tests: 2284 passing locally (2 new). Pre-commit clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- .gitignore | 2 + MIGRATION_v4.0_to_v4.1.md | 80 ++++++++++++++++++++++++++++++++++ src/adcp/webhooks.py | 35 +++++++++------ tests/test_webhooks_deliver.py | 52 ++++++++++++++++++++++ 4 files changed, 156 insertions(+), 13 deletions(-) diff --git a/.gitignore b/.gitignore index fe2cbe00..bbb7ab22 100644 --- a/.gitignore +++ b/.gitignore @@ -177,3 +177,5 @@ IMPLEMENTATION_PLAN.md !.claude/ !.claude/agents/ .claude/settings*.json +# Local Conductor / harness runtime state — written by /loop and similar +.claude/scheduled_tasks.lock diff --git a/MIGRATION_v4.0_to_v4.1.md b/MIGRATION_v4.0_to_v4.1.md index bfa04cc1..195dfcbb 100644 --- a/MIGRATION_v4.0_to_v4.1.md +++ b/MIGRATION_v4.0_to_v4.1.md @@ -88,6 +88,80 @@ actions = valid_actions_for_status("pending_start") # creatives approved, If you stored `"pending_activation"` as a status string anywhere, map it to `"pending_start"` on read. +## 4. Signing-prep hardening — three behavior changes + +The 4.1 release includes the foundation-audit signing-prep work. Most of +it is purely additive (new opt-in kwargs, new exports), but three places +silently changed default behavior: + +### 4a. `WebhookSender` and `webhooks.deliver()` now run SSRF guards + +When the sender owns its httpx client (default path — no `client=` passed), +every webhook delivery now resolves the URL, validates against an SSRF +range list (loopback / RFC 1918 / link-local / CGNAT / IPv6 ULA / multicast +/ cloud-metadata), and pins the connection to the resolved IP. Plus +`follow_redirects=False` and `trust_env=False` close the rebinding-via- +redirect and `HTTPS_PROXY` env-var bypass. + +For production deployments posting to real buyer URLs, this is a +no-op. For **dev/CI fixtures posting to internal endpoints** (loopback, +private, link-local), webhooks will start raising `SSRFValidationError`. +Two opt-outs: + +```python +# Owned-client path — pass allow_private=True to disable the IP-range check +sender = WebhookSender.from_jwk(jwk, allow_private_destinations=True) +await deliver(config, payload, allow_private=True) + +# Operator-supplied client path — the framework trusts the operator's +# transport completely, no SSRF guard runs (vetted egress proxy, ASGI +# test transport, etc.) +sender = WebhookSender.from_jwk(jwk, client=my_httpx_client) +``` + +Operators who want a hardened destination-port allowlist as defense +in depth opt INTO `DEFAULT_ALLOWED_PORTS = frozenset({443, 8443})` +(now exported from `adcp.signing`): + +```python +from adcp.signing import DEFAULT_ALLOWED_PORTS + +sender = WebhookSender.from_jwk(jwk, allowed_destination_ports=DEFAULT_ALLOWED_PORTS) +``` + +The default is permissive (`None` = no port filter) because AdCP doesn't +constrain `pushNotificationConfig.url` ports. + +### 4b. `VerifierCapability.covers_content_digest` defaults to `"either"` + +The default flipped from `"required"` to `"either"` to align with the +AdCP 3.0 schema (`get-adcp-capabilities-response.json` declares +`"either"` as the default explicitly). The schema rationale recommends +`"required"` for spend-committing operations in production, and AdCP +4.0 recommends `"required"` more broadly. + +Adopters who relied on the implicit `"required"` default lose +body-integrity authentication on signed requests not enumerated in +`required_for`. **The webhook-signing profile (`adcp.signing.webhook_verifier`) +is unaffected — it hard-codes `"required"`** so every signed webhook +still carries a body-integrity-binding `Content-Digest`. + +To preserve the prior strict behavior, opt INTO `"required"` explicitly +or use `required_for` to promote spend-committing operations: + +```python +# Before (4.0): VerifierCapability() defaulted to covers_content_digest="required" +cap = VerifierCapability() + +# After (4.1): explicit opt-in +cap = VerifierCapability(covers_content_digest="required") + +# Or: scope the strictness to operations that move money +cap = VerifierCapability( + required_for=frozenset({"create_media_buy", "update_media_buy"}), +) +``` + ## What to test after upgrading - Run your full test suite — the `pending_task_id` rename is a noisy compile @@ -96,3 +170,9 @@ If you stored `"pending_activation"` as a status string anywhere, map it to `FormatId` references and update the expected values. - If you operate a media-buy state machine, search for `pending_activation` in your codebase. +- If you have **dev/CI fixtures posting webhooks to private/internal IPs**, + add `allow_private=True` (on `deliver()`) or + `allow_private_destinations=True` (on `WebhookSender`). +- If you constructed `VerifierCapability()` with no kwargs and relied on + the implicit `"required"` body-digest enforcement, set + `covers_content_digest="required"` explicitly or scope via `required_for`. diff --git a/src/adcp/webhooks.py b/src/adcp/webhooks.py index b457332c..cf799340 100644 --- a/src/adcp/webhooks.py +++ b/src/adcp/webhooks.py @@ -819,6 +819,23 @@ async def deliver( _warn_auth_deprecation_once() + # Build the pinned transport up-front (owned-client path). SSRF + # validation runs synchronously inside ``build_async_ip_pinned_transport`` + # — a hostile URL raises ``SSRFValidationError`` before we serialize + # the body or compute the HMAC, so a buyer-supplied 127.0.0.1 URL + # does not produce an HMAC-over-buyer-body sitting in process memory + # for fault-handlers / custom logging to capture on exception. + # Mirrors the WebhookSender._send_bytes ordering. + transport: Any = None + if client is None: + from adcp.signing.ip_pinned_transport import build_async_ip_pinned_transport + + transport = build_async_ip_pinned_transport( + url, + allow_private=allow_private, + allowed_ports=allowed_ports, + ) + body_dict = _payload_to_dict(payload) if token is not None and token_field is not None: _validate_header_value("config.token", token) @@ -881,19 +898,11 @@ async def deliver( effective_timeout = timeout_seconds if timeout_seconds is not None else _DEFAULT_TIMEOUT_SECONDS if client is None: - # Owned-client path: build a per-request IP-pinned transport so - # the URL is SSRF-validated and pinned to the resolved IP, with - # follow_redirects=False (rebinding-via-redirect defense) and - # trust_env=False (HTTPS_PROXY env vars cannot route the request - # away from the pinned target). Mirrors the WebhookSender pattern - # — see adcp.webhook_sender._send_bytes for the same shape. - from adcp.signing.ip_pinned_transport import build_async_ip_pinned_transport - - transport = build_async_ip_pinned_transport( - url, - allow_private=allow_private, - allowed_ports=allowed_ports, - ) + # Owned-client path. ``transport`` was built up-front so SSRF + # rejected before signing; here we just construct the per-request + # client. ``follow_redirects=False`` closes rebinding-via-redirect; + # ``trust_env=False`` blocks ``HTTPS_PROXY`` env-var bypass. + # Same shape as ``WebhookSender._send_bytes``. async with httpx.AsyncClient( transport=transport, timeout=effective_timeout, diff --git a/tests/test_webhooks_deliver.py b/tests/test_webhooks_deliver.py index 621a751a..256eda70 100644 --- a/tests/test_webhooks_deliver.py +++ b/tests/test_webhooks_deliver.py @@ -644,3 +644,55 @@ async def test_operator_supplied_client_skips_ssrf_guard() -> None: response = await deliver(config, payload, client=client) assert response.status_code == 200 + + +@pytest.mark.asyncio +async def test_owned_client_rejects_hostile_url_before_hmac_signing() -> None: + """Validate-before-sign defense in depth, parity with WebhookSender: + ``deliver()`` builds the pinned transport (which validates the URL) + before computing the HMAC over the body. A buyer-supplied URL + pointing at 127.0.0.1 raises SSRFValidationError BEFORE + ``get_adcp_signed_headers_for_webhook`` runs — the HMAC over the + buyer's body never sits in process memory waiting for the rejection + that would otherwise come at delivery time. + + Regression guard for the validate-before-sign reorder (PR #303 + follow-up). Mirrors test_owned_client_rejects_hostile_url_before_signing + in test_webhook_sender_e2e.py.""" + config = PushNotificationConfig( + url="https://127.0.0.1/webhooks/adcp", + authentication=PNAuthentication(schemes=["HMAC-SHA256"], credentials=_SECRET), + ) + payload = create_mcp_webhook_payload( + task_id="task_no_hmac", + task_type="create_media_buy", + status="completed", + ) + + with patch("adcp.webhooks.get_adcp_signed_headers_for_webhook") as mock_hmac: + with pytest.raises(SSRFValidationError): + await deliver(config, payload) + assert mock_hmac.called is False, ( + "get_adcp_signed_headers_for_webhook was called even though SSRF " + "validation rejected the URL — the HMAC over the buyer body would " + "sit in process memory until the rejection. Validate-before-sign " + "ordering is broken; check deliver()." + ) + + +@pytest.mark.asyncio +async def test_owned_client_rejects_loopback_destination_hmac_path() -> None: + """HMAC-SHA256 auth path goes through the same SSRF guard as Bearer. + Coverage parity for both legacy auth schemes.""" + config = PushNotificationConfig( + url="https://127.0.0.1/webhooks/adcp", + authentication=PNAuthentication(schemes=["HMAC-SHA256"], credentials=_SECRET), + ) + payload = create_mcp_webhook_payload( + task_id="task_hmac_ssrf", + task_type="create_media_buy", + status="completed", + ) + + with pytest.raises(SSRFValidationError): + await deliver(config, payload)