feat(signing): drain three foundation-audit deferreds (#298, #299, #300)#303
Merged
feat(signing): drain three foundation-audit deferreds (#298, #299, #300)#303
Conversation
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>
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>
…ix, 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>
bokelley
force-pushed
the
bokelley/signing-prep-followups
branch
from
7162676 to
e0f6177
Compare
Contributor
Author
Expert review pass complete — pushed
|
Contributor
Author
|
Acknowledged — clean rebase, 3 reviewers signed off, 2266 tests green, pre-commit clean. Nothing actionable from my side; standing by for CI matrix. Generated by Claude Code |
Contributor
Author
|
Noted — standing by with you. Will re-engage if CI flags anything. Generated by Claude Code |
Contributor
Author
|
Acknowledged — monitoring alongside you. Will surface anything actionable if CI raises a flag. Generated by Claude Code |
6 tasks
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Drains the three deferred items from PR #297's expert review. Each landed as a separate commit so reviewers can evaluate by issue.
0f9c3607—fix(signing): default covers_content_digest to spec-mandated 'either'999ed639—chore(signing): skip eager _get_client in WebhookSender.__aenter__71626766—feat(signing): wire SSRF guard into legacy webhooks.deliver()Behavior changes
VerifierCapability.covers_content_digestdefault flips from"required"to"either"(matches AdCP 3.0 schema). Operators who want body-integrity-on-every-request opt INTO"required"explicitly, OR userequired_for=frozenset({"create_media_buy", ...})for spend-committing operations.webhooks.deliver()now SSRF-validates buyer URLs and pins to the resolved IP on the owned-client path (mirrorsWebhookSender._send_bytes). Adopters usingdeliver()against private/internal endpoints in dev/CI need to addallow_private=True. Production posting to real buyer URLs is unaffected.WebhookSender.__aenter__no longer constructs an unusedhttpx.AsyncClienton the owned-client path. Pure cleanup, no observable behavior change.Test plan
pytest tests/conformance/signing/— 422 passingpytest tests/test_webhooks_deliver.py— 29 passing (3 new SSRF regression tests)pytest tests/— 2266 passing locallymypy src/adcp/— cleanruff checkon changed files — cleanblack --checkon changed files — cleanSemver
Title
feat(signing):so release-please tags this as the next minor bump after 4.1.0:allow_private,allowed_portskwargs ondeliver()) and changes owned-client behavior — additive minorIf squash-merging, the
feat(signing):PR title carries the conventional-commit type that release-please reads.Closes
🤖 Generated with Claude Code