Skip to content

feat(signing): close 4 SSRF gaps and add opt-in port hardening (foundation audit)#297

Merged
bokelley merged 4 commits intomainfrom
bokelley/signing-prep-fixes
Apr 29, 2026
Merged

feat(signing): close 4 SSRF gaps and add opt-in port hardening (foundation audit)#297
bokelley merged 4 commits intomainfrom
bokelley/signing-prep-fixes

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

@bokelley bokelley commented Apr 29, 2026

Summary

Pre-foundation cleanup surfacing from the v6.0 DecisioningPlatform foundation audit (see .context/foundations-audit/FINDINGS.md on PR #290). Each fix closes a real bug or spec gap in the existing adcp.signing surface independently of the framework work that builds on top — these can ship today regardless of whether the DecisioningPlatform RFC lands.

What landed (4 SSRF gaps + opt-in port hardening + ergonomics)

  1. WebhookSender._send_bytes per-request IP-pinned transport — previous impl reused one httpx.AsyncClient and bypassed the IP-pinned transport entirely; buyer-supplied URLs pointing at 127.0.0.1 or AWS metadata would deliver. Now: per-request pinned transport on the owned-client path with trust_env=False (closes HTTPS_PROXY env-var bypass) and follow_redirects=False. Operator-supplied clients trusted to own their own SSRF.
  2. Validate-before-sign ordering — pinned-transport build (which validates) now runs before sign_webhook. A hostile URL never produces an Ed25519/ES256 signature in process memory.
  3. Optional port allowlist as opt-in operator hardeningvalidate_jwks_uri / resolve_and_validate_host / build_*_pinned_transport accept allowed_ports: frozenset[int] | None = None; default None imposes no port filter (AdCP doesn't constrain webhook ports). Operators opt INTO DEFAULT_ALLOWED_PORTS={443, 8443} (now exported from adcp.signing) for hardening.
  4. operation_id docstring fix — previous docstring said "deprecated from payload" contradicting the schema which mandates the echo. Field was already supported; only doc was wrong.
  5. from_jwk / from_pem SSRF kwarg forwarding — both factories now accept and forward allow_private_destinations and allowed_destination_ports, fixing an API trap where the documented happy path couldn't configure SSRF policy.
  6. assert self._client is not None → explicit RuntimeError on operator-supplied client path. python -O strips asserts; the state was reachable via aclose() then re-send.
  7. Stale docstrings in build_ip_pinned_transport / build_async_ip_pinned_transport — claimed allowed_ports defaults to DEFAULT_ALLOWED_PORTS but actual default is None.

Test plan

  • pytest tests/conformance/signing/ — 393 tests passing locally
  • pytest tests/ — 2258 tests passing locally (4 new regression tests)
  • mypy src/adcp/ — clean
  • ruff check src/ tests/conformance/signing/ — clean
  • black --check src/ tests/conformance/signing/ — clean
  • CI on 3.10/3.11/3.12/3.13
  • No backward-compat breaks for existing v4 ADCPHandler adopters — port allowlist defaults to permissive (None); SSRF range check + IP pin are bug fixes for a previously-unguarded delivery path

Semver

Title feat(signing): so release-please tags this as 4.1.0 (minor):

  • New public exports (DEFAULT_ALLOWED_PORTS)
  • New kwargs on public surfaces (validate_jwks_uri, resolve_and_validate_host, build_ip_pinned_transport, build_async_ip_pinned_transport, WebhookSender.__init__, from_jwk, from_pem)
  • WebhookSender._send_bytes semantics changed for owned-client path (now SSRF-validates and pin-binds every delivery)

If squash-merging, the feat(signing): PR title carries the conventional-commit type that release-please reads.

Deferred (tracked separately)

Foundation PR (#290):

  • WebhookTransport Protocol with enforces_ssrf_at_connect=True attestation (security M1)
  • JWKS multi-tenant scoping with the right axis — (jwks_uri, kid) not tenant_id (security M4 + protocol-expert)
  • LRU cache of pinned transports for keep-alive perf (security L2)

Separate issues:

🤖 Generated with Claude Code

bokelley and others added 2 commits April 29, 2026 11:38
…audit

Pre-foundation cleanup surfacing from the v6.0 DecisioningPlatform
foundation audit. Each fix closes a real bug or spec gap in the existing
adcp.signing surface independently of the framework work that builds on
top.

1. Port allowlist for SSRF-validated outbound HTTP
   (adcp.signing.jwks.{validate_jwks_uri, resolve_and_validate_host},
   ip_pinned_transport.{build_ip_pinned_transport, build_async_ip_pinned_transport})
   - Default permits {443, 8443}; rejects :25, :6379, :11211, etc. on
     resolved public IPs. Buyers can no longer smuggle traffic to
     internal SMTP / Redis / Memcached via webhook URLs on non-standard
     ports even when the IP itself is routable.
   - Configurable via allowed_ports kwarg; empty frozenset is the
     test-only escape hatch.
   - Test: tests/conformance/signing/test_jwks.py
     (test_ssrf_rejects_disallowed_ports + parametrized matrix)

2. WebhookSender owned-client path uses pin-and-bind transport
   (adcp.webhook_sender.WebhookSender._send_bytes)
   - Previous implementation reused a single httpx.AsyncClient across
     all destinations and bypassed the IP-pinned transport entirely.
     A buyer-supplied webhook URL pointing at 127.0.0.1 or AWS metadata
     would deliver successfully.
   - Now: when the sender owns its httpx client (default), every
     delivery builds a per-request AsyncIpPinnedTransport. Per-request
     re-resolution is intentional — keeping a pinned transport alive
     across deliveries to the same hostname would defeat the rebinding
     defense.
   - When the operator supplies their own client (vetted egress proxy,
     ASGI test transport), the framework trusts them completely; the
     operator owns SSRF guarantees on their transport.
   - Tests: test_owned_client_rejects_loopback_destination,
     test_owned_client_rejects_disallowed_port

3. Tenant-scoped JWKS resolver Protocol
   (adcp.signing.jwks.{JwksResolver, AsyncJwksResolver},
   adcp.signing.verifier.VerifyOptions, webhook_verifier.WebhookVerifyOptions)
   - Adds optional ``tenant_id`` kwarg to the resolver Protocol
     so a resolver instance shared across tenants can refuse keys
     outside the active tenant's published JWKS. Cross-tenant key
     confusion (a buyer signing for tenant B who knows tenant A's
     key_id) is closed at the resolver layer, not the verifier.
   - Single-tenant in-tree impls (Static, Caching, AsyncCaching)
     accept the kwarg as a pass-through — tenant scoping is a wrapper
     concern, and adopters compose tenant-scoped resolvers around
     existing single-tenant resolvers.
   - VerifyOptions.tenant_id and WebhookVerifyOptions.tenant_id thread
     the value through; verifier.py:227 passes it on resolver call.
   - Test: tests/conformance/signing/test_jwks.py
     (test_tenant_scoping_wrapper_pattern — reference pattern for
     adopters; test_static_resolver_accepts_tenant_id_kwarg —
     backward-compat invariant)

4. content-digest required-by-default for inbound request signing
   (adcp.signing.verifier.VerifierCapability)
   - Default already correct (covers_content_digest="required" at
     verifier.py:95). Adds a regression test pinning the default so a
     future "make it lenient" refactor surfaces in CI.
   - Body integrity must be authenticated end-to-end; "either" or
     "forbidden" lets a MITM inside TLS termination swap bodies on
     signed requests whose digest isn't covered.
   - Test: tests/conformance/signing/test_verifier_defaults.py
     (test_default_covers_content_digest_is_required)

5. WebhookPayload.operation_id docstring fix
   (adcp.webhooks.create_mcp_webhook_payload)
   - Docstring previously said "deprecated from payload, should be in
     URL routing, but included for backward compatibility."
     Contradicted the schema at mcp-webhook-payload.json which says
     publishers MUST echo this back so buyers correlate notifications
     without parsing URL paths.
   - Field already supported in the payload constructor; only the
     docstring needed correction. Adds a test confirming the
     end-to-end echo from send_mcp(operation_id=...) into the
     delivered payload.
   - Test: test_send_mcp_threads_operation_id_into_payload

All 5 fixes ship together as a security-prep PR before the v6.0
DecisioningPlatform foundation work lands. Each is independent of the
others; reviewers can evaluate by gap.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Expert review of the prep PR (security-reviewer, code-reviewer,
python-expert, ad-tech-protocol-expert) flagged 3 changes that should
move to the foundation PR or be reversed:

1. JWKS tenant_id kwarg removed
   - Protocol expert: tenant_id is the wrong axis for JWKS isolation;
     AdCP anchors keys at agents[].jwks_uri, not at a seller-internal
     tenant_id. CachingJwksResolver(jwks_uri=...) already isolates by
     URL.
   - Security: the kwarg was opt-in with no enforcement signal —
     adopters who built a tenant-scoping resolver and forgot to thread
     tenant_id on every call would silently fall back to single-tenant.
   - Python: the JWKS document verifier (jws.py) didn't thread the
     kwarg, so the multi-tenant guarantee only held on the RFC 9421
     path — undercutting the stated security improvement.
   - Code review: Protocol shape change with no compat shim was a
     breaking change for external resolver implementations.
   - Resolution: drop the kwarg from this prep PR. Reintroduce in the
     foundation PR with the spec-correct axis (likely (jwks_uri, key_id)
     or (agent_url, key_id)) and after the spec project clarifies
     multi-tenant key isolation guidance.

2. Port allowlist default flipped to permissive
   - Protocol expert: AdCP doesn't constrain pushNotificationConfig.url
     ports (push-notification-config.json:7-11). Defaulting to {443,
     8443} silently rejects legitimate buyers on :9443 (Tomcat),
     :4443 (Spring Boot), or path-routed multi-tenant gateways.
   - Security M2: implicit-HTTP rejection (port 80) wasn't documented
     as scheme enforcement; adopters hitting it would widen the
     allowlist and re-enable plaintext.
   - Resolution: default allowed_ports=None (no port filter); operators
     opt INTO {443, 8443} hardening by passing
     allowed_ports=DEFAULT_ALLOWED_PORTS. The constant is exported
     from adcp.signing for adopters who want the recommended posture.
     The IP-range check + IP pinning still apply regardless of port —
     the smuggle vector to internal services on the same routable IP
     is closed by IP-range rejection, not by port enforcement.

3. covers_content_digest='required' regression test dropped
   - Protocol expert: AdCP 3.0 spec explicitly sets
     "default": "either" (get-adcp-capabilities-response.json:912-921)
     with the rationale "'required' is recommended for spend-committing
     operations in production; 4.0 recommends 'required' for those
     operations."
   - Existing code shipped "required" as the default before this PR —
     a pre-existing divergence from the spec that's not my PR's bug to
     pin. Drop the regression test that locked in the wrong default.
     File a separate issue to address the spec divergence.

Also fixed (real bugs from the same review):

4. WebhookSender.from_jwk / from_pem now forward
   allow_private_destinations + allowed_destination_ports to the
   constructor. Documented happy-path adopters can now configure SSRF
   policy without dropping to __init__.

5. Replaced `assert self._client is not None` (mypy-narrowing) on the
   operator-supplied client path with an explicit RuntimeError. The
   state is reachable (aclose() then re-send) and python -O strips
   asserts, leaving the call to silently NoneType.post().

Net result for this PR (now 3 fixes instead of 5):

- IP-pinned webhook delivery (the actual security hole)
- Optional port allowlist as opt-in operator hardening
- operation_id docstring fix (schema-mandated echo)

Plus the from_jwk/from_pem ergonomics + assert-to-raise fix.

Tests: 2254 passing locally. New tests:
- test_jwks.py::test_ssrf_default_imposes_no_port_filter (parametrized)
  — confirms :9443/:4443/:8080/:80 all pass without explicit allowlist
- test_jwks.py::test_ssrf_default_allowlist_passes_canonical_https_ports
- test_jwks.py::test_ssrf_empty_allowlist_rejects_every_port (sentinel)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley
Copy link
Copy Markdown
Contributor Author

Expert review applied — PR scoped down

Pushed 3fd2c493 addressing the 4-reviewer feedback:

Reverted (moved to foundation PR):

  • JWKS tenant_id kwarg — protocol expert flagged tenant_id as wrong axis (AdCP anchors keys at agents[].jwks_uri); security flagged opt-in-with-no-enforcement; python-expert flagged JWS verifier gap; code-reviewer flagged breaking Protocol change with no compat shim. Reintroduce in foundation PR with spec-correct axis after spec project clarifies multi-tenant key isolation.
  • covers_content_digest='required' regression test — protocol expert: AdCP 3.0 schema explicitly says "default": "either". Existing code shipped "required" (pre-existing spec divergence); my PR shouldn't be the one to cement it. Filed for separate triage.

Flipped to opt-in (was: forced default):

  • Port allowlist — protocol expert: AdCP doesn't constrain pushNotificationConfig.url ports; defaulting to {443, 8443} silently rejects legitimate buyers on :9443 (Tomcat), :4443 (Spring Boot), path-routed gateways. New default: allowed_ports=None (no filter). Operators opt INTO hardening via allowed_ports=DEFAULT_ALLOWED_PORTS. IP-range + IP-pinning still applied regardless — the smuggle vector to internal services on the same routable IP is closed by IP-range rejection, not port enforcement.

Fixed (real bugs caught by review):

  • WebhookSender.from_jwk / from_pem now forward allow_private_destinations and allowed_destination_ports (python-expert chore(main): release 0.1.1 #2, code-reviewer blocker)
  • assert self._client is not None on operator-supplied client path replaced with explicit RuntimeErrorpython -O strips asserts, state is reachable via aclose() + re-send (python-expert chore(main): release 0.1.2 #3)

Net PR scope:

  1. IP-pinned webhook delivery (the actual security hole — webhook_sender._send_bytes previously bypassed pin-and-bind entirely)
  2. Optional port allowlist as opt-in operator hardening + DEFAULT_ALLOWED_PORTS exported
  3. operation_id docstring fix (schema-mandated echo)
  4. from_jwk/from_pem ergonomics
  5. assertraise on operator-client path

2254 tests passing. Pre-commit clean. Diff stat: ~270 lines (down from 398).

Items deferred to foundation PR or separate work:

  • JWKS tenant scoping (axis TBD pending spec clarification)
  • covers_content_digest default revert (separate issue)
  • WebhookTransport Protocol with enforces_ssrf_at_connect attestation (security M1 — foundation PR)
  • LRU cache of pinned transports keyed by (hostname, expiry) (security L2 perf opt — foundation or follow-up)
  • __aenter__ eagerly creating unused httpx client in pin-and-bind mode (code-reviewer nit — follow-up)

@bokelley
Copy link
Copy Markdown
Contributor Author

Thanks for the detailed breakdown, @bokelley. The scope-down looks well-reasoned — deferring JWKS tenant scoping and covers_content_digest to dedicated work is the right call given the expert flags, and the opt-in port allowlist with DEFAULT_ALLOWED_PORTS exported is a cleaner operator story. The IP-pinned path fix (item 2) is the highest-value piece anyway. Ready for human review as-is.


Generated by Claude Code

…n, 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>
@bokelley bokelley changed the title fix(signing): close 5 SSRF and tenant-isolation gaps from foundation audit feat(signing): close 4 SSRF gaps and add opt-in port hardening (foundation audit) Apr 29, 2026
@bokelley
Copy link
Copy Markdown
Contributor Author

Second-pass review applied — 50ae54d9

All 4 reviewers approved the scope-down with these targeted findings:

Ship-blocker (security): missing trust_env=False on per-request httpx.AsyncClient. httpx defaults to trust_env=True, routing the request through HTTPS_PROXY/HTTP_PROXY env vars and bypassing the AsyncIpPinnedTransport entirely. Fixed at webhook_sender.py:577 with a regression test that asserts the kwarg is set.

Real bugs (python-expert):

  • Stale docstrings in build_ip_pinned_transport / build_async_ip_pinned_transport claimed allowed_ports defaults to DEFAULT_ALLOWED_PORTS — actual default is None. Updated.
  • _send_bytes signed body before SSRF-validating URL. Restructured: pinned-transport build (which runs SSRF + port validation) now happens first; signing only after. Hostile URLs no longer leave a signed payload in process memory.

3 missing regression tests (code-reviewer + security-reviewer):

  • test_owned_client_default_allows_non_standard_ports — sender-level positive analog of validator-level test
  • test_operator_supplied_client_bypasses_ssrf_guard — named regression for documented contract
  • test_owned_client_ignores_https_proxy_env — guards trust_env=False against future regressions

Code-reviewer nit: deduplicated DEFAULT_ALLOWED_PORTS rationale block-comment between adcp.signing.jwks and test_jwks.py. Kept at constant-definition site.

Semver retag: PR title and final commit retagged from fix(signing) to feat(signing) per code-reviewer's analysis. Public surface widened (new export, new kwargs on 6+ public surfaces), WebhookSender._send_bytes semantics changed for owned-client path. Both qualify as minor (4.1.0), not patch. release-please reads the squash-merge subject (= PR title) for the conventional-commit type.

Final stats:

  • 5 commits → 11 files changed, ~+513/-188
  • Tests: 2257 passing locally (3 new)
  • Pre-commit clean: black, ruff, mypy, bandit

Net PR scope:

  1. IP-pinned webhook delivery + trust_env=False (the actual security hole)
  2. Optional port allowlist as opt-in operator hardening + DEFAULT_ALLOWED_PORTS exported
  3. operation_id docstring fix
  4. from_jwk/from_pem SSRF kwarg forwarding
  5. assertraise on operator-client path
  6. Validate-before-sign ordering
  7. Stale docstring fix

Items deferred to foundation PR or separate work:

  • JWKS multi-tenant scoping (axis decision pending spec clarification)
  • covers_content_digest default revert to spec-mandated "either" (pre-existing divergence)
  • WebhookTransport Protocol with enforces_ssrf_at_connect attestation (foundation PR)
  • LRU cache of pinned transports for keep-alive perf (foundation or follow-up)
  • __aenter__ eagerly creating unused httpx client in pin-and-bind mode (cosmetic nit)
  • webhooks.deliver() legacy path lacks SSRF guard (deprecated, separate PR)

Spec-project items to file upstream (separate work, not in this PR):

  • Specify URL→operation_id extraction convention in mcp-webhook-payload.json
  • Declare pushNotificationConfig.url ports unconstrained
  • Multi-tenant JWKS isolation guidance ((jwks_uri, kid) axis)
  • AdCP recommends {443, 8443} for hardened deployments

@bokelley
Copy link
Copy Markdown
Contributor Author

Acknowledged — second-pass summary received. The deferred items and upstream spec gaps are noted; no action needed from me here. Ready for human review when you are.


Generated by Claude Code

@bokelley
Copy link
Copy Markdown
Contributor Author

Acknowledged — thanks for confirming. The PR is in good shape for human review.


Generated by Claude Code

…tion

Final-pass review on 50ae54d surfaced three test-quality nits, all addressed.

1. test_owned_client_default_allows_non_standard_ports docstring overclaim:
   the test patches build_async_ip_pinned_transport with a fake, so the
   IP-range check inside the real builder doesn't fire. Soften the claim
   and point at test_owned_client_rejects_loopback_destination for the
   IP-range coverage.

2. test_owned_client_ignores_https_proxy_env now also asserts
   "transport" in captured_kwargs. Without this, a future refactor that
   moves trust_env=False to the eager __aenter__ construction (away
   from the per-request construction where the proxy-bypass guard
   actually lives) would pass the test while leaving the per-request
   client vulnerable.

3. test_owned_client_rejects_hostile_url_before_signing: new test for
   the validate-before-sign claim made in the _send_bytes docstring.
   Patches sign_webhook to a MagicMock, points the URL at 127.0.0.1,
   asserts SSRFValidationError raises AND mock_sign.called is False.
   No Ed25519/ES256 signature ever materializes for a URL that fails
   the SSRF guard.

Tests: 2258 passing locally (up from 2257). Pre-commit clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley
Copy link
Copy Markdown
Contributor Author

Fold-candidate from triage: issue #300 tracks a one-line cleanup that was deferred from this PR's python-expert review — skip await self._get_client() in __aenter__ when self._owns_client=True, since the post-#297 owned-client path in _send_bytes builds a fresh pinned-transport client per request and ignores self._client. The fix is in src/adcp/webhook_sender.py (same file as this PR) and is non-breaking. Consider folding it in before merge to ship the cleanup as a coherent unit; aclose() already guards on self._client is not None so no other code needs to change.

Triaged by Claude Code. Session: https://claude.ai/code/${CLAUDE_CODE_REMOTE_SESSION_ID}


Generated by Claude Code

@bokelley
Copy link
Copy Markdown
Contributor Author

Issue #299 tracks the same SSRF fix for deliver() (the deprecated AdCP 3.x path, src/adcp/webhooks.py:861–868). Since this is the same hardening effort and the PR is still iterating, worth considering folding it in before merge.

The change is small — on the owned-client path, swap the bare httpx.AsyncClient(timeout=…) for one backed by build_async_ip_pinned_transport(url, allow_private=allow_private_destinations), add allow_private_destinations: bool = False and allowed_destination_ports: frozenset[int] | None = None kwargs to deliver()'s public surface, and update the security note in its docstring. build_async_ip_pinned_transport already exists on main, so no new infrastructure needed.

If you prefer to keep PR #297's scope tight, #299 will resurface as a follow-up after merge. Either way is fine — just flagging while you're actively in the area.


Posted by Claude Code triage (issue #299). Session: https://claude.ai/code/session_01SaZb7fWVnax15UzFjAWCUs


Generated by Claude Code

@bokelley bokelley merged commit 84b837e into main Apr 29, 2026
11 checks passed
bokelley added a commit that referenced this pull request Apr 29, 2026
Closes #298.

The AdCP 3.0 schema (`schemas/cache/protocol/get-adcp-capabilities-response.json:912-921`)
declares `covers_content_digest` default as "either", with the rationale:
"'required' is recommended for spend-committing operations in
production; 4.0 recommends 'required' for those operations."

`VerifierCapability.covers_content_digest` defaulted to "required" — a
pre-existing divergence from the spec. Surfaced by ad-tech-protocol-expert
review on PR #297 (foundation audit).

Operators who want body-integrity authentication end-to-end on every
request opt INTO `covers_content_digest="required"` explicitly, OR use
`required_for=frozenset({"create_media_buy", ...})` to promote
spend-committing operations selectively. The webhook-signing profile
(`adcp.signing.webhook_verifier`) hard-codes "required" regardless —
webhook bodies always carry signed digests.

Test renamed: `test_verifier_capability_defaults_to_either_digest`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request Apr 29, 2026
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>
bokelley added a commit that referenced this pull request Apr 29, 2026
* fix(signing): default covers_content_digest to spec-mandated 'either'

Closes #298.

The AdCP 3.0 schema (`schemas/cache/protocol/get-adcp-capabilities-response.json:912-921`)
declares `covers_content_digest` default as "either", with the rationale:
"'required' is recommended for spend-committing operations in
production; 4.0 recommends 'required' for those operations."

`VerifierCapability.covers_content_digest` defaulted to "required" — a
pre-existing divergence from the spec. Surfaced by ad-tech-protocol-expert
review on PR #297 (foundation audit).

Operators who want body-integrity authentication end-to-end on every
request opt INTO `covers_content_digest="required"` explicitly, OR use
`required_for=frozenset({"create_media_buy", ...})` to promote
spend-committing operations selectively. The webhook-signing profile
(`adcp.signing.webhook_verifier`) hard-codes "required" regardless —
webhook bodies always carry signed digests.

Test renamed: `test_verifier_capability_defaults_to_either_digest`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* 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>

* test(signing): apply review nits — module-top imports, drop test prefix, docstring

Code-reviewer + python-expert review of PR #303 surfaced four small
inline polish items, all addressed.

1. Module-top imports for `from adcp.signing import SSRFValidationError`
   and `from unittest.mock import patch` in test_webhooks_deliver.py —
   the rest of the file imports at module top; local imports were
   inconsistent with the file's existing style.

2. Renamed three new tests to drop the `test_deliver_*` prefix.
   The file is already scoped to `deliver()` tests (per its docstring);
   the prefix was redundant. Tests are now:
   - test_owned_client_rejects_loopback_destination
   - test_allow_private_dev_escape_hatch
   - test_operator_supplied_client_skips_ssrf_guard

3. Added a comment in test_allow_private_dev_escape_hatch flagging
   that the patch target depends on deliver()'s lazy in-function
   import. If the import moves to module-level on adcp.webhooks, the
   patch target must follow. Future-you note.

4. VerifierCapability docstring: "AdCP 4.0 expected to recommend"
   → "AdCP 4.0 recommends" (grounded in the spec rationale at
   get-adcp-capabilities-response.json).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request Apr 30, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant