Conversation
… SSRF coverage 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
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
Three small follow-ups from PR #303's second-pass expert review, plus the migration-guide notes for the signing-prep behavior changes landing in v4.1.
Changes
Validate-before-sign symmetry in
webhooks.deliver()— mirrors theWebhookSender._send_bytespattern from PR feat(signing): close 4 SSRF gaps and add opt-in port hardening (foundation audit) #297. The pinned-transport build (which runs SSRF + port validation) now runs BEFORE body assembly + HMAC computation. A hostile URL raisesSSRFValidationErrorbeforeget_adcp_signed_headers_for_webhookis called, so the HMAC-over-buyer-body never materializes in process memory.HMAC-SHA256 SSRF coverage — both legacy auth paths (Bearer and HMAC-SHA256) route through the same SSRF guard. The existing
test_owned_client_rejects_loopback_destinationonly exercised Bearer; addedtest_owned_client_rejects_loopback_destination_hmac_pathfor parity..gitignore— exclude.claude/scheduled_tasks.lock(Conductor harness runtime state).Migration guide —
MIGRATION_v4.0_to_v4.1.mdgains section chore(main): release 1.0.0 #4 covering the two signing-prep behavior changes adopters need to know about:WebhookSender+deliver()) — dev/CI fixtures posting to private IPs needallow_private=TrueVerifierCapability.covers_content_digestdefault flip from\"required\"to\"either\"per AdCP 3.0 spec — adopters who relied on the implicit strict default need to set it explicitlyTest plan
pytest tests/test_webhooks_deliver.py— 31 passing (2 new)pytest tests/conformance/signing/— 422 passingpytest tests/— 2284 passing locallymypy src/adcp/— cleanruff check+black --checkon changed files — cleanSemver
Title
fix(signing):because:No new public surface, no behavior change beyond what already shipped in PR #297 / #303 (which had
feat(signing):for that). release-please should treat this as patch-level on top of 4.1.0.🤖 Generated with Claude Code