Skip to content

Commit bc8da3a

Browse files
bokelleyclaude
bokelley
and
claude
authored
fix(signing): validate-before-sign symmetry in deliver() + HMAC SSRF coverage + 4.1 migration notes
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) <noreply@anthropic.com>
1 parent 072998a commit bc8da3a

4 files changed

Lines changed: 156 additions & 13 deletions

File tree

‎.gitignore‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,3 +177,5 @@ IMPLEMENTATION_PLAN.md
177177
!.claude/
178178
!.claude/agents/
179179
.claude/settings*.json
180+
# Local Conductor / harness runtime state — written by /loop and similar
181+
.claude/scheduled_tasks.lock

‎MIGRATION_v4.0_to_v4.1.md‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,80 @@ actions = valid_actions_for_status("pending_start") # creatives approved,
8888
If you stored `"pending_activation"` as a status string anywhere, map it to
8989
`"pending_start"` on read.
9090

91+
## 4. Signing-prep hardening — three behavior changes
92+
93+
The 4.1 release includes the foundation-audit signing-prep work. Most of
94+
it is purely additive (new opt-in kwargs, new exports), but three places
95+
silently changed default behavior:
96+
97+
### 4a. `WebhookSender` and `webhooks.deliver()` now run SSRF guards
98+
99+
When the sender owns its httpx client (default path — no `client=` passed),
100+
every webhook delivery now resolves the URL, validates against an SSRF
101+
range list (loopback / RFC 1918 / link-local / CGNAT / IPv6 ULA / multicast
102+
/ cloud-metadata), and pins the connection to the resolved IP. Plus
103+
`follow_redirects=False` and `trust_env=False` close the rebinding-via-
104+
redirect and `HTTPS_PROXY` env-var bypass.
105+
106+
For production deployments posting to real buyer URLs, this is a
107+
no-op. For **dev/CI fixtures posting to internal endpoints** (loopback,
108+
private, link-local), webhooks will start raising `SSRFValidationError`.
109+
Two opt-outs:
110+
111+
```python
112+
# Owned-client path — pass allow_private=True to disable the IP-range check
113+
sender = WebhookSender.from_jwk(jwk, allow_private_destinations=True)
114+
await deliver(config, payload, allow_private=True)
115+
116+
# Operator-supplied client path — the framework trusts the operator's
117+
# transport completely, no SSRF guard runs (vetted egress proxy, ASGI
118+
# test transport, etc.)
119+
sender = WebhookSender.from_jwk(jwk, client=my_httpx_client)
120+
```
121+
122+
Operators who want a hardened destination-port allowlist as defense
123+
in depth opt INTO `DEFAULT_ALLOWED_PORTS = frozenset({443, 8443})`
124+
(now exported from `adcp.signing`):
125+
126+
```python
127+
from adcp.signing import DEFAULT_ALLOWED_PORTS
128+
129+
sender = WebhookSender.from_jwk(jwk, allowed_destination_ports=DEFAULT_ALLOWED_PORTS)
130+
```
131+
132+
The default is permissive (`None` = no port filter) because AdCP doesn't
133+
constrain `pushNotificationConfig.url` ports.
134+
135+
### 4b. `VerifierCapability.covers_content_digest` defaults to `"either"`
136+
137+
The default flipped from `"required"` to `"either"` to align with the
138+
AdCP 3.0 schema (`get-adcp-capabilities-response.json` declares
139+
`"either"` as the default explicitly). The schema rationale recommends
140+
`"required"` for spend-committing operations in production, and AdCP
141+
4.0 recommends `"required"` more broadly.
142+
143+
Adopters who relied on the implicit `"required"` default lose
144+
body-integrity authentication on signed requests not enumerated in
145+
`required_for`. **The webhook-signing profile (`adcp.signing.webhook_verifier`)
146+
is unaffected — it hard-codes `"required"`** so every signed webhook
147+
still carries a body-integrity-binding `Content-Digest`.
148+
149+
To preserve the prior strict behavior, opt INTO `"required"` explicitly
150+
or use `required_for` to promote spend-committing operations:
151+
152+
```python
153+
# Before (4.0): VerifierCapability() defaulted to covers_content_digest="required"
154+
cap = VerifierCapability()
155+
156+
# After (4.1): explicit opt-in
157+
cap = VerifierCapability(covers_content_digest="required")
158+
159+
# Or: scope the strictness to operations that move money
160+
cap = VerifierCapability(
161+
required_for=frozenset({"create_media_buy", "update_media_buy"}),
162+
)
163+
```
164+
91165
## What to test after upgrading
92166

93167
- 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
96170
`FormatId` references and update the expected values.
97171
- If you operate a media-buy state machine, search for `pending_activation`
98172
in your codebase.
173+
- If you have **dev/CI fixtures posting webhooks to private/internal IPs**,
174+
add `allow_private=True` (on `deliver()`) or
175+
`allow_private_destinations=True` (on `WebhookSender`).
176+
- If you constructed `VerifierCapability()` with no kwargs and relied on
177+
the implicit `"required"` body-digest enforcement, set
178+
`covers_content_digest="required"` explicitly or scope via `required_for`.

‎src/adcp/webhooks.py‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -819,6 +819,23 @@ async def deliver(
819819

820820
_warn_auth_deprecation_once()
821821

822+
# Build the pinned transport up-front (owned-client path). SSRF
823+
# validation runs synchronously inside ``build_async_ip_pinned_transport``
824+
# — a hostile URL raises ``SSRFValidationError`` before we serialize
825+
# the body or compute the HMAC, so a buyer-supplied 127.0.0.1 URL
826+
# does not produce an HMAC-over-buyer-body sitting in process memory
827+
# for fault-handlers / custom logging to capture on exception.
828+
# Mirrors the WebhookSender._send_bytes ordering.
829+
transport: Any = None
830+
if client is None:
831+
from adcp.signing.ip_pinned_transport import build_async_ip_pinned_transport
832+
833+
transport = build_async_ip_pinned_transport(
834+
url,
835+
allow_private=allow_private,
836+
allowed_ports=allowed_ports,
837+
)
838+
822839
body_dict = _payload_to_dict(payload)
823840
if token is not None and token_field is not None:
824841
_validate_header_value("config.token", token)
@@ -881,19 +898,11 @@ async def deliver(
881898

882899
effective_timeout = timeout_seconds if timeout_seconds is not None else _DEFAULT_TIMEOUT_SECONDS
883900
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-
)
901+
# Owned-client path. ``transport`` was built up-front so SSRF
902+
# rejected before signing; here we just construct the per-request
903+
# client. ``follow_redirects=False`` closes rebinding-via-redirect;
904+
# ``trust_env=False`` blocks ``HTTPS_PROXY`` env-var bypass.
905+
# Same shape as ``WebhookSender._send_bytes``.
897906
async with httpx.AsyncClient(
898907
transport=transport,
899908
timeout=effective_timeout,

‎tests/test_webhooks_deliver.py‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -644,3 +644,55 @@ async def test_operator_supplied_client_skips_ssrf_guard() -> None:
644644
response = await deliver(config, payload, client=client)
645645

646646
assert response.status_code == 200
647+
648+
649+
@pytest.mark.asyncio
650+
async def test_owned_client_rejects_hostile_url_before_hmac_signing() -> None:
651+
"""Validate-before-sign defense in depth, parity with WebhookSender:
652+
``deliver()`` builds the pinned transport (which validates the URL)
653+
before computing the HMAC over the body. A buyer-supplied URL
654+
pointing at 127.0.0.1 raises SSRFValidationError BEFORE
655+
``get_adcp_signed_headers_for_webhook`` runs — the HMAC over the
656+
buyer's body never sits in process memory waiting for the rejection
657+
that would otherwise come at delivery time.
658+
659+
Regression guard for the validate-before-sign reorder (PR #303
660+
follow-up). Mirrors test_owned_client_rejects_hostile_url_before_signing
661+
in test_webhook_sender_e2e.py."""
662+
config = PushNotificationConfig(
663+
url="https://127.0.0.1/webhooks/adcp",
664+
authentication=PNAuthentication(schemes=["HMAC-SHA256"], credentials=_SECRET),
665+
)
666+
payload = create_mcp_webhook_payload(
667+
task_id="task_no_hmac",
668+
task_type="create_media_buy",
669+
status="completed",
670+
)
671+
672+
with patch("adcp.webhooks.get_adcp_signed_headers_for_webhook") as mock_hmac:
673+
with pytest.raises(SSRFValidationError):
674+
await deliver(config, payload)
675+
assert mock_hmac.called is False, (
676+
"get_adcp_signed_headers_for_webhook was called even though SSRF "
677+
"validation rejected the URL — the HMAC over the buyer body would "
678+
"sit in process memory until the rejection. Validate-before-sign "
679+
"ordering is broken; check deliver()."
680+
)
681+
682+
683+
@pytest.mark.asyncio
684+
async def test_owned_client_rejects_loopback_destination_hmac_path() -> None:
685+
"""HMAC-SHA256 auth path goes through the same SSRF guard as Bearer.
686+
Coverage parity for both legacy auth schemes."""
687+
config = PushNotificationConfig(
688+
url="https://127.0.0.1/webhooks/adcp",
689+
authentication=PNAuthentication(schemes=["HMAC-SHA256"], credentials=_SECRET),
690+
)
691+
payload = create_mcp_webhook_payload(
692+
task_id="task_hmac_ssrf",
693+
task_type="create_media_buy",
694+
status="completed",
695+
)
696+
697+
with pytest.raises(SSRFValidationError):
698+
await deliver(config, payload)

0 commit comments

Comments
 (0)