fix(security): harden external sinks and callbacks

[bokelley]

Summary

• validate and canonicalize A2A callback destinations with a fail-closed policy
• make the SQLAlchemy push-notification store tenant-scoped and compatible with a2a-sdk 1.0
• add a reusable production callback-security helper
• contain audit-sink failures and harden external property-list handling

Why

External callback and sink boundaries could accept unsafe destinations, cross tenant boundaries, or let downstream failures affect request processing.

Validation

• 122 focused A2A, callback-security, audit-sink, and property-list tests passed
• independent security review against current origin/main

Compatibility

The default callback policy is fail closed. Deployments that need dynamic destinations or DNS pinning must supply a custom sender and enforce resolution at connection time.

[aao-ipr-bot]

⚠️ Argus review could not complete

The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.

View workflow run

_{This is an automated message from the Argus AI review workflow.}

[KonstantinMirin]

Review — PR #1002

Overview — The a2a-sdk 1.0 migration is right: set_info / get_info / delete_info now take the ServerCallContext the 1.0 ABC actually passes, and test_sqlite_push_config_store_rejects_untrusted_destinations pins the one ordering that matters — the address check runs before the host allowlist, so an operator who allowlists 127.0.0.1 still gets rejected.

Findings 1-4 are one root, already open as #1004: outbound destination policy is re-derived at each feature instead of owned at one boundary. I have added this PR's evidence there rather than restating the argument here — the sites below stand on their own, but the fix for all four is the same seam.

Should fix

1. A fourth hand-rolled destination validator, weaker than the one the SDK already exports for this field

src/adcp/server/a2a_push_security.py:26-52

adcp.webhooks.validate_webhook_destination_url (src/adcp/webhooks.py:1049) is the registration-time validator for exactly this field. Its docstring names push_notification_config.url, it routes the address decision through resolve_and_validate_host, and it raises WebhookDestinationValidationError carrying code = "INVALID_REQUEST". The new module reimplements scheme / userinfo / port / host handling from scratch and substitutes not address.is_global for the shared classifier.

src/adcp/signing/jwks.py:83-85 states in this repo, verbatim:

not ip.is_global is NOT a usable substitute: it reports True (i.e. globally reachable) for the 6to4-relay, AS112, AMT and ORCHIDv2 ranges on every supported version, so it would close none of these holes.

Those are the ranges commits 8e51a3ea, b6b3f47f and 61e6d926 added to _EXTRA_BLOCKED_NETWORKS days ago. Run against the new validator with the host allowlisted — the exact case test_sqlite_push_config_store_rejects_untrusted_destinations exists to cover, since the allowlist is not supposed to override the address check:

destination	a2a_push_security	webhooks.validate_webhook_destination_url
192.88.99.1 (RFC 7526 6to4 relay)	ACCEPTED	rejected
192.31.196.1 (RFC 7535 AS112-v4)	ACCEPTED	rejected
192.52.193.1 (RFC 7450 AMT)	ACCEPTED	rejected
192.175.48.1 (RFC 7534 AS112)	ACCEPTED	rejected
2001:20::1 (RFC 7343 ORCHIDv2)	ACCEPTED	rejected
64:ff9b::7f00:1 (NAT64 wrapping 127.0.0.1)	ACCEPTED	rejected
https://callback.example/hook#frag	ACCEPTED	rejected (fragment)
https://callback.example/hook\r\nX-Inject: 1	ACCEPTED, persisted verbatim	rejected (control characters)

Three more consequences of writing a fresh policy instead of calling the existing one:

• Port 443 only (:41-42), with no kwarg to widen it. The pinned schema schemas/cache/3.1/core/push-notification-config.json @ AdCP 3.1.8 says: "publishers SHOULD NOT enforce a destination-port allowlist by default, since buyers legitimately host receivers on non-standard TLS ports (:9443, :4443, path-routed multi-tenant gateways)." A buyer on :9443 cannot register at all. The SDK's own DEFAULT_ALLOWED_PORTS = frozenset({443, 8443}) (src/adcp/signing/jwks.py:112) is opt-in for this reason, and WebhookDestinationPolicy.production() defaults allowed_destination_ports=None.
• Bare ValueError at all five raise sites. a2a-sdk 1.0.1 does not catch it: it unwinds to jsonrpc_dispatcher.py:340-344, which does logger.exception('Unhandled exception') and returns InternalError(message=str(e)). The buyer gets -32603 for a request they can trivially correct, with no error_code and no recovery, and every rejected registration writes an ERROR-level traceback that an unauthenticated caller on message/send can trigger at will. src/adcp/server/translate.py:480 already maps recovery="correctable" to InvalidParamsError (-32602).
• canonicalize_host(parts.hostname) at :44 is unguarded, so attacker-controlled hostnames escape as raw idna errors — https://xn--<0x80>.example/hook raises idna.core.InvalidCodepoint: Codepoint U+0080 at position 1 of '\x80' not allowed, and str(e) goes onto the wire as the InternalError message. resolve_and_validate_host catches (idna.IDNAError, UnicodeError, UnicodeEncodeError) at src/adcp/signing/jwks.py:279-282.

Root cause: the abstraction is not missing, it is unused. Delete the policy body of validate_push_notification_url and delegate to validate_webhook_destination_url(url, policy=WebhookDestinationPolicy.production(...), field="push_notification_config.url"), layering only the a2a-specific allowed_destination_hosts membership test on top of the returned WebhookDestinationValidation.hostname. That inherits _EXTRA_BLOCKED_NETWORKS, DNS resolution with the pinned IP, control-character and fragment rejection, the spec-grounded port default, and the typed INVALID_REQUEST error — and the next reserved-range fix lands in one place instead of two.

Coverage to add with it: tests/test_a2a_push_security.py:56 parametrizes 127.0.0.1 / 10.0.0.1 / ::1 / fe80::1, the four addresses where is_global happens to agree with the repo's classifier, so it reads as proof of a property the code does not have. Add the six rows above; they fail today and pass after the delegation. And add one test that posts a CreateTaskPushNotificationConfig through the app from create_a2a_server(...) and asserts error.code plus error.data.error_code / error.data.recovery — nothing in either test file references -32602, -32603, InvalidParamsError or InternalError today, so the wire envelope is ungraded.

2. The anonymous-scope guard stayed on the branch a2a-sdk never takes

examples/a2a_db_tasks.py:422-441, examples/a2a_sqlalchemy_tasks.py:319-333

Both _scope() implementations now short-circuit on if context is not None. a2a-sdk 1.0.1's DefaultRequestHandler passes a ServerCallContext on every store call (default_request_handler.py:309, 532, 565, 644, 676), and ServerCallContext.user is non-optional with default_factory=UnauthenticatedUser. So the scope_provider / ContextVar branch is dead on the live path, and the _warned_anonymous / RuntimeWarning guard lives exclusively on the dead branch:

store = SqlitePushNotificationConfigStore(db_path=..., scope_provider=lambda: "tenant-a",
                                          allowed_destination_hosts=frozenset({"callback.example"}))
await store.set_info("t1", cfg, ServerCallContext(user=UnauthenticatedUser()))
# warnings: []
# SELECT scope FROM a2a_push_configs -> [('__anonymous__',)]

Two unauthenticated callers now share one scope and read each other's callback URLs and plaintext authentication.credentials, with no diagnostic. The docstring this diff rewrote still promises "Fails loudly on anonymous fallback." (examples/a2a_db_tasks.py:364). Before the diff the unset ContextVar produced the warning that told the operator to wire auth.

Root cause: scope now has two sources of truth, selected by is None, and the loud-on-anonymous invariant guards only one of them. Make the context derivation return str | None and let _scope() own the warn-once plus fall-through, so the contract holds on whichever source produced the identity.

The two tests that look like coverage both call without a context and pass either way: test_sqlite_push_config_store_isolates_scopes_by_contextvar and test_sqlite_push_config_store_warns_once_on_anonymous_scope. And tests/test_a2a_push_security.py:139 sets _push_config_scope to "tenant-a" before passing an unauthenticated context — deleting that set/reset pair changes nothing, because the ContextVar is never read. The missing input is set_info(..., ServerCallContext(user=UnauthenticatedUser())) on a store with a working scope_provider, asserting the stored scope.

3. The guard is opt-in per store class, and write-side only

src/adcp/server/a2a_server.py:1182-1186 and :1226-1231; examples/a2a_db_tasks.py:487-499, examples/a2a_sqlalchemy_tasks.py:359-372

create_a2a_server(..., push_config_store=...) is the single point every tasks/pushNotificationConfig/set funnels through, and it forwards the adopter's store to a2a-sdk with no destination policy. The new guard is called from inside two example classes, so it protects those two and nothing else:

store = InMemoryPushNotificationConfigStore()          # a2a-sdk's own
create_a2a_server(H(), name="x", push_config_store=store)
await store.set_info("t1", TaskPushNotificationConfig(
    id="c1", task_id="t1", url="http://169.254.169.254/latest/meta-data/"), ServerCallContext())
# get_info -> ['http://169.254.169.254/latest/meta-data/']

a2a_push_security is also imported by zero files under src/ and is not re-exported from adcp/server/__init__.py, unlike every other public module there — so the PR body's "reusable production callback-security helper" is reachable only by deep-importing an unexported module.

The second half: set_info validates, get_info does not. Rows outlive the process while the allowlist is rebuilt per boot from A2A_PUSH_ALLOWED_HOSTS, so removing a host has no effect on configs already registered — a2a-sdk's BasePushNotificationSender.send_notification reads through config_store.get_info(...) and POSTs the full task JSON to whatever it finds. Verified: a store rebuilt with an empty allowlist still returns ['https://cb.example/hook'].

The module docstring's reason for stopping at storage does not hold. It says "The installed a2a-sdk exposes a PushNotificationSender interface rather than an HTTP transport hook", but BasePushNotificationSender.__init__(self, httpx_client: httpx.AsyncClient, config_store, context) takes an injected client, which is the seam build_async_ip_pinned_transport was written for. src/adcp/audit_sink.py:288-290 and src/adcp/webhook_sender.py:984-988 both already use it.

Root cause: the policy is a property of the server, not of each store class. Give create_a2a_server / serve a push_destination_policy= (or allowed_push_hosts=) parameter and wrap whatever store it is handed in a validating decorator that re-checks on read; ship the send-side half wired to an httpx.AsyncClient(transport=build_async_ip_pinned_transport(...)) instead of documenting it as adopter homework. Correct the docstring either way, and export the module's public names.

4. Two reference examples, three divergent contracts for the same decision

examples/a2a_db_tasks.py:128-137 vs examples/a2a_sqlalchemy_tasks.py:185-197; examples/a2a_sqlalchemy_tasks.py:346; examples/a2a_db_tasks.py:548-550 vs examples/a2a_sqlalchemy_tasks.py:446-448

This PR extracted _scope_from_server_context in the SQLite example, which requires is_authenticated before trusting user_name, and routed the SQLAlchemy example's push configs through _scope_from_context, which has no such check. Same input, opposite verdict — for a User reporting is_authenticated=False, user_name="tenant-a", the shape any "parse the JWT sub before verifying the signature" middleware produces:

sqlite : __anonymous__
sqlalch: tenant-a

An unverified caller lands in tenant-a's push-config partition. The same hunk also dropped the context.user is None guard at :196 — safe against pydantic-constructed contexts, but a hardening removal inside a hardening PR.

The config_id fallback diverges the same way. SQLite synthesises f"auto-{uuid.uuid4()}"; SQLAlchemy now falls back to the URL (:346). Three identical id-less registrations on one task:

sqlalchemy rows: 1
sqlite     rows: 3

SQLite has test_sqlite_push_config_store_synthesises_config_id_when_omitted; the SQLAlchemy branch has no test that reaches it, because the only id-less construction in the suite raises on the URL policy first. The same diff deleted the SQLite comment block that named this collapse as a footgun, so the divergence now has neither a warning nor a test.

Third copy: the A2A_PUSH_ALLOWED_HOSTS env split is duplicated verbatim in both main() functions.

Root cause: the helper was extracted for DRY inside one file instead of being placed where both callers reach it. a2a_push_security.py is already imported by both — export scope_from_server_context(context) with the is_authenticated requirement and an allowed_push_hosts_from_env() helper from there, use them in both examples for TaskStore and PushNotificationConfigStore alike, and pick one config_id fallback.

5. The docs that adopters copy still describe the pre-PR contract, and the documented wiring is now deny-all

src/adcp/server/a2a_server.py:1022-1028; docs/handler-authoring.md:1074-1081, :1094, :1108-1110

Three statements the diff contradicts:

• The shipped push_config_store docstring says "unlike TaskStore, a2a-sdk's PushNotificationConfigStore ABC does not pass a ServerCallContext to set_info / get_info / delete_info". inspect.signature on the installed ABC gives (self, task_id, notification_config, context) — context is required, and the whole premise of this PR is that 1.0 passes it. docs/handler-authoring.md:1108-1110 repeats the claim.
• docs/handler-authoring.md:1094 says "The reference impl does NOT validate URLs". It now does.
• The copy-paste snippet at docs/handler-authoring.md:1074-1081 builds SqlitePushNotificationConfigStore("/var/lib/myagent/push_configs.db") with no allowed_destination_hosts, which under the new deny-all default rejects every push registration at runtime.

Docs are a call site of this change like any other. Update all three and give the snippet an explicit allowed_destination_hosts=... so the documented wiring accepts a registration.

6. error_message redaction reaches one of the sinks that receive it

src/adcp/audit_sink.py:322 (gated), :193 (not gated), :405 (source)

make_audit_middleware builds every failure event with error_message=str(exc)[:200] at :405. SlackAlertSink._format now gates it behind include_error_message, on the stated grounds that "exception messages can contain request values, upstream response fragments, or credentials" (:229-232). LoggingAuditSink.record at :193 emits event.model_dump_json(), the whole event with error_message included, and every adopter-implemented AuditSink receives the same ungated field. That reasoning also sits awkwardly next to item 7 in this same PR, which treats a server log as a place credentials must not reach.

Root cause: the redaction decision belongs at event construction, not per sink. Put a redact_error_messages flag on make_audit_middleware, or have AuditEvent carry the class name by default and the message only under an explicit opt-in. Then no sink, including adopter-written ones, can leak the field by omission.

grep -rn include_error_message returns four hits, all in src/adcp/audit_sink.py — no test constructs SlackAlertSink(..., include_error_message=True). Invert, ignore or shadow the flag and the suite stays green while the operator who opted in silently gets no error text. Add the True case asserting msg= appears in the posted text.

7. The property-list log change states a guarantee the code does not provide

src/adcp/decisioning/property_list.py:106-124

The new comment says the exception "remains available to trusted in-process callers without entering normal server logs". Line 124 is still raise AdcpError(...) from exc, so the cause stays attached and any logger.exception / exc_info=True on the AdcpError prints it:

cause: RuntimeError('Authorization=Bearer secret_bearer_token')
SECRET IN FORMATTED TRACEBACK: True

Nothing enforces the claim. The change also inverts the convention documented in the same package. src/adcp/decisioning/dispatch.py:598 says "The full traceback (with message) lives in the server log via logger.exception; only the wire response is sanitized to a class-name breadcrumb", which is what the pre-diff comment here said too. In exchange the operator loses the ability to tell a fetch timeout from a 404 from a TLS failure.

Pick one boundary and hold it. Either keep the log line as it was, because logs are trusted per the documented model, or, if logs are now untrusted, redact where the exception is captured rather than where one message is formatted: drop the from exc chain, or scrub at the sink. test_fetch_failure_log_omits_exception_text asserts on the single logger.warning record, so it proves the format string changed, not that the secret stays out of logs.

Notes

• src/adcp/canonical_formats/references.py:509-514 carries the same not ip.is_global weakness and predates this PR. Cited only as evidence that the shared predicate is what is missing; fixing it is out of scope unless the delegation in item 1 naturally covers it.
• The PR body's "contain audit-sink failures" is not backed by anything in this diff. The containment (_emit's per-sink asyncio.wait_for plus swallow, src/adcp/audit_sink.py:427-458) is pre-existing and unmodified. No defect, just a description overclaim.
• src/adcp/audit_sink.py:177 names a class SlackAuditSink; the class is SlackAlertSink. Pre-existing line, untouched by the diff.
• The reformat-only hunks in src/adcp/decisioning/property_list.py:177-180, 260-263 and tests/test_decisioning_property_list.py are ruff line-reflow on lines this PR did not otherwise touch, not a deliberate style change.
• Commit convention question, not an assertion: fix(security) matches the maintainer's practice of treating hardening as fix, and nothing here moves signed bytes. But the 443-only default in item 1 is a hard behavior change for any adopter of these examples who registered a :9443 callback — either a BREAKING CHANGE: trailer, or resolve item 1 so it is not breaking at all.

[aao-ipr-bot]

Clean fail-closed hardening across four external boundaries. Right shape: the callback path now denies by default and only opens for operator-owned hosts, the decisioning path stops echoing credential-bearing exception text on the wire, and tenant scope keys on the verified principal instead of an ambient ContextVar.

Things I checked

• Allowlist canonicalization parity is byte-exact. normalize_allowed_push_hosts and validation.hostname both funnel through canonicalize_host (src/adcp/signing/_idna_canonicalize.py:44; the latter via resolve_and_validate_host → jwks.py:280) — case-fold, single trailing-dot strip, IDNA-2008 A-label, IP-literal short-circuit all identical. test_a2a_push_security.py proves the BÜCHER.Example. → xn--bcher-kva.example round-trip. security-reviewer: no bypass via userinfo, homoglyph, or trailing-dot.
• Empty allowlist fails closed. validation.hostname not in frozenset() is always true → raise. A2A_PUSH_ALLOWED_HOSTS defaults empty, so an unconfigured deployment denies every push destination. a2a_push_security.py:38-60.
• Tenant isolation holds on the explicit-context path. scope_from_server_context (a2a_push_security.py:24-30) returns None for UnauthenticatedUser, and because context is not None the store never falls through to the ambient provider/ContextVar — an unauthenticated request cannot inherit tenant-a. Both ..._explicit_unauthenticated_context_cannot_inherit_ambient_scope tests exercise exactly this.
• Credential-leak closure in the decisioning path. resolve_property_list rebuilds safe_origin as scheme://hostname[:port] (userinfo/fragment already rejected upstream), logs only type(exc).__name__, and from None drops the token-bearing __cause__ from wire errors and tracebacks. test_fetch_failure_log_omits_exception_text and test_error_details_do_not_include_auth_token confirm.
• Audit default is fail-closed. include_error_message defaults False in both SlackAlertSink and make_audit_middleware; error_type is always retained so triage signal survives. audit_sink.py:322,406.
• delete_info positional order is covered. test_sqlalchemy_push_store_matches_a2a_v1_set_get_delete_contract calls delete_info("task-1", tenant_a, "cfg-1") — (task_id, context, config_id) — and asserts cfg-1 deleted while cfg-2 survives, pinning the a2a-sdk 1.0 contract order against the cross-config-wipe scenario.
• Test scaffolding. The autouse getaddrinfo mock (test_a2a_push_security.py:30-38) is what lets .example hosts satisfy the production SSRF policy — the suite validates allowlist/scope logic, not live DNS. Worth knowing when reading the green checkmarks.
• Semver: SlackAlertSink / make_audit_middleware / the two example stores gained keyword-only params with defaults — additive, non-breaking. resolve_property_list signature unchanged. fix(security): is the right home for the behavior tightening below.

Follow-ups (non-blocking — file as issues)

• Reconcile the stale docstring. src/adcp/server/a2a_server.py:1022-1028 still asserts the PushNotificationConfigStore ABC "does not pass a ServerCallContext" — the exact opposite of what this PR now relies on. That block wasn't in the diff, so the repo ships both claims at once. Notable that the security fix and the docstring it obsoletes now disagree in the same tree.
• Add real-dispatch coverage for the authenticated-context path. Every new isolation test calls the store directly with a hand-built authenticated context; the one real-DefaultRequestHandler test (test_a2a_server.py:1152) passes an empty context. Nothing proves production dispatch actually threads an authenticated ServerCallContext into set_info/get_info/delete_info — the whole premise of the fix. Worst case is inert (falls back to prior ContextVar behavior, no regression), but it's untested.
• list_id regex admits pure dot-segments. ^[A-Za-z0-9._~-]+$ (property_list.py:113) matches . and .., and the documented fetcher's quote(list_id, safe='') does not encode dots — so .. survives into /property-lists/.. on the already-validated agent host. Not SSRF (host is validated) and not a leak, but reject all-dot values.
• Behavior change worth a changelog line. The resolve_property_list error details key renamed agent_url → agent_origin, and audit error_message now defaults to omitted. Both are intentional and security-motivated; adopters parsing those fields should know.

Minor nits (non-blocking)

1. a2a_push_security.py not re-exported from src/adcp/server/__init__.py. Examples reach it only by full module path, unlike sibling a2a_server symbols. Public-surface inconsistency, not a defect.
2. make_audit_middleware docstring gap. Gained include_error_message but no :param: entry, whereas SlackAlertSink's docstring was updated. audit_sink.py:333.
3. Double include_error_message gate. Middleware nulls error_message when False, and SlackAlertSink._format also gates — text emits only when both are True. Fail-safe, but two independent flags for one concept reads as confusing.

code-reviewer: no confirmed blocker (two Majors, both hinging on the a2a-sdk ABC signature it couldn't introspect — resolved above as Follow-ups). security-reviewer: no High, ship-worthy.

LGTM. Follow-ups noted below.

[aao-ipr-bot]

Approving on the strength of a fail-closed callback allowlist and the a2a-sdk 1.0 context migration being verified against upstream, not asserted. The architectural principle is right: an explicit ServerCallContext is authoritative for tenant scope, and an unauthenticated request must never inherit an ambient tenant from a ContextVar — that's the SSRF/isolation seam this PR closes.

Things I checked

• Allowlist canonicalization matches on both sides. validate_push_notification_url compares validation.hostname (from resolve_and_validate_host → canonicalize_host) against normalize_allowed_push_hosts (same canonicalize_host). IDNA A-label, ASCII-lowercase, single trailing-dot strip — identical transform, so xn--bcher-kva.EXAMPLE. matches an allowlist of BÜCHER.Example. and Cyrillic homoglyphs land on a different A-label and get denied. security-reviewer: sound. Fail-closed confirmed — empty allowlist → frozenset() → every host not in → reject (test_..._defaults_to_deny_all_destinations).
• SSRF classifier runs before the allowlist check, so RFC-1918 / link-local / metadata IPs are rejected regardless of allowlist contents (a2a_push_security.py:33).
• a2a-sdk 1.0.1 wire contract, verified against upstream v1.0.1 source by ad-tech-protocol-expert: PushNotificationConfigStore ABC threads context and orders delete_info(self, task_id, context, config_id=None) — the examples match names, ordering, and the context-before-config_id placement exactly. This migration actually repairs a latent bug: the old SQLAlchemy example read nested notification_config.push_notification_config.id/.url, which raises AttributeError against 1.0.x's flattened TaskPushNotificationConfig. security-reviewer's one gating question ("is context really threaded?") is answered by the upstream read — the isolation control is live, not inert.
• property_list.py info-leak hygiene. safe_origin is scheme://host[:port] from urlsplit(validation.original_url); userinfo is already rejected by validate_webhook_destination_url, list_id is regex-gated to [A-Za-z0-9._~-]+, from None drops the exception chain, and the log carries only type(exc).__name__. test_fetch_failure_log_omits_exception_text proves a bearer token in exception text never reaches the log or wire error.
• audit_sink.py double-gating (include_error_message=False nulls error_message in the middleware AND independently gates rendering in SlackAlertSink._format) is defense-in-depth, not redundancy. No path emits raw exception text by default (test_slack_alert_sink_omits_exception_message_by_default).
• No import-layering breach; decisioning imports from adcp.webhooks, not types/generated_poc. New adcp.server.a2a_push_security is additive — no public adcp.* export removed or resignatured, so fix(security): without ! is the correct semver signal.

Follow-ups (non-blocking — file as issues)

• Stale docstring in a2a_server.py:1022-1028. It still states the PushNotificationConfigStore ABC "does not pass a ServerCallContext" and prescribes the ContextVar-only pattern. Post-1.0 migration that's false — context is threaded. Not in this diff, but it now contradicts the examples this PR rewrote. Reconcile it.
• property_list.py hands the fetcher validation.original_url, not validation.resolved_ip. The framework resolves-and-validates the host, then discards the pinned IP and lets the adopter's fetcher re-resolve — a DNS-rebind TOCTOU that every adopter must re-close by hand. The docstring now says implementers MUST IP-pin; consider passing resolved_ip (or a pre-pinned transport) into the fetcher signature so the safe path is the default rather than an instruction.
• list_id regex admits . and ... Same-host path traversal one level up is possible via the documented {agent_url}/property-lists/{list_id} pattern; the host is SSRF-validated so blast radius is same-origin only. Tighten to reject ./.. for a true single-segment guarantee.

Minor nits (non-blocking)

1. SQLAlchemy example lacks the config_id comment its SQLite sibling carries. set_info uses notification_config.id or f\"auto-{uuid.uuid4()}\" (a2a_sqlalchemy_tasks.py), and the synthesized id is never written back into the persisted payload, so two id-less registrations accumulate distinct un-deletable rows. Behavior matches the SQLite reference and the old url-as-id scheme, so it's not a regression — but copy the explanatory comment from a2a_db_tasks.py:451-459 over so the accumulation isn't a surprise.
2. Deny-all still pays a DNS lookup. validate_destination resolves the host before the hostname not in allowed_hosts check, so an empty allowlist resolves DNS only to reject. Could short-circuit when canonical_allowed_hosts is empty (a2a_push_security.py:34). Cheap, cosmetic.

Safe to merge.

[aao-ipr-bot]

Clean security-hardening pass — the SSRF classifier is fail-closed both ways and the a2a-sdk 1.0 context threading is correct. Right architectural call to reuse the shared validate_webhook_destination_url classifier for the push-config store instead of hand-rolling a third allowlist.

Things I checked

• Fail-closed both ways (src/adcp/server/a2a_push_security.py:validate_push_notification_url). production() runs first and hardcodes allow_private_destinations=False, so allowlisting 127.0.0.1 still gets rejected by the classifier before the hostname allowlist check — the allowlist is a second gate, not a bypass. test_push_url_rejects_private_ipv4_and_ipv6_literals covers it. Empty frozenset() denies every destination.
• Canonicalization is symmetric. normalize_allowed_push_hosts and validation.hostname both pass through canonicalize_host (jwks.py:280 → webhooks.py:1184), so no IDNA/case/trailing-dot-equivalent host can be smuggled past the allowlist. test_push_url_canonicalizes_idna_case_and_trailing_dot confirms.
• a2a-sdk 1.0.1 ABC order. delete_info(self, task_id, context, config_id=None) matches the pinned ABC exactly — context before config_id. Both SQLite and SQLAlchemy examples are mutually consistent. ad-tech-protocol-expert: widening context to | None is an LSP-safe override that enables the context-less background-sender fallback.
• No credential leak on the property_list fetch path (src/adcp/decisioning/property_list.py). auth_token never logged; safe_origin is scheme+host+port only; from None suppresses the credential-bearing exception chain; wire details carry only list_id + agent_origin. test_fetch_failure_log_omits_exception_text proves the exception text (Authorization=Bearer …) stays out of logs.
• Slack egress gated (src/adcp/audit_sink.py). include_error_message is double-gated (middleware + _format), default False — raw exception text no longer reaches Slack unless an adopter opts in at both layers.
• No wire-shape regression. INVALID_REQUEST / SERVICE_UNAVAILABLE are existing enum values; nothing under generated_poc/ or any discriminated union changed. New module adcp.server.a2a_push_security is a pure addition — no removed export, no required→optional flip. fix(security): is the right prefix; behavior tightens defaults, no public signature breaks.
• Wire-level test exists — test_push_config_destination_policy_reaches_jsonrpc_error_envelope (tests/test_a2a_push_security.py) drives a real JSON-RPC CreateTaskPushNotificationConfig and asserts -32602 plus an empty store, so the primary user-facing path ships validated, not just the unit helper.

Follow-ups (non-blocking — file as issues)

• Malformed port escapes as an unmapped 500, not -32602. resolve_and_validate_host accesses parts.port unguarded (jwks.py:283), which raises a bare ValueError on https://host:not-a-port/; the try at webhooks.py:1165 only catches SSRFValidationError, and validate_a2a_push_notification_url only catches WebhookDestinationValidationError — so that input crosses the a2a boundary as an InternalError instead of the intended invalid-params envelope. It fails closed (nothing persisted), so this is error-contract, not security. The PR's own test_push_url_rejects_unsafe_authority_forms asserts the bare ValueError at the helper level, confirming the gap. Fix: catch ValueError in validate_a2a_push_notification_url, or wrap the port cast in WebhookDestinationValidationError. (code-reviewer flagged.)
• list_id regex admits . / ... re.fullmatch(r"[A-Za-z0-9._~-]+", list_id) permits pure-dot segments, and quote(list_id, safe='') does not encode dots, so list_id=".." survives into /property-lists/.. — collapses one path segment on the already-SSRF-validated host. No slash is reachable, so no full traversal, but the stated "single URL-safe path segment" intent isn't met. Reject ./... (Both security-reviewer and ad-tech-protocol-expert flagged.)
• Dropped typed field on the agent_url rejection. The policy-failure AdcpError in resolve_property_list passes details={"reason": ...} but omits field="property_list.agent_url" even though the validator was called with it — buyers use field to highlight the bad input.
• DNS-rebinding residual on the fetch path. resolve_property_list validates agent_url then hands validation.original_url (not resolved_ip) to the fetcher, which re-resolves independently — a host that validated public can rebind by fetch time. Mitigated: the docstring now mandates IP pinning and the reference fetcher uses build_async_ip_pinned_transport. Consider threading resolved_ip through the PropertyListFetcher Protocol so a naive adopter fetcher can't drop the pin.
• Call the audit default-change out in the PR body. LoggingAuditSink now loses error text by default (the middleware gates error_message), so adopters relying on it for local error observability lose it until they set include_error_message=True. Deliberate hardening, worth one line so it isn't a surprise.

Minor nits (non-blocking)

1. Double canonicalization. validate_push_notification_url re-runs normalize_allowed_push_hosts on hosts the store already normalized in __init__ (a2a_push_security.py). Idempotent, so harmless — defensible as belt-and-suspenders for the public entry point.

Approving on the strength of the fail-closed SSRF+allowlist composition plus the a2a-sdk 1.0.1 ABC alignment. Follow-ups noted above; the malformed-port one is the only one worth doing soon.

[aao-ipr-bot]

Solid hardening — fail-closed by default is the right shape, and the SSRF/allowlist core is correct. Holding at comment on one load-bearing question I could not resolve from the diff: whether the a2a-sdk 1.0.1 actually passes ServerCallContext to the push-config store on the wire. The whole "explicit context is authoritative" tenant-isolation story rests on that, and nothing in the PR proves it end-to-end.

Things I checked

• Empty-vs-None allowlist is fail-closed. a2a_push_security.py:88 — None → allowlist skipped but WebhookDestinationPolicy.production() still runs the reserved-range/metadata SSRF classifier; empty frozenset() → non-None empty set → validation.hostname not in frozenset() always true → deny-all. Confirmed by test_explicit_empty_allowlist_denies_every_destination.
• Allowlist canonicalization is symmetric. Both normalize_allowed_push_hosts and the validated hostname route through the same canonicalize_host (IDNA/case/trailing-dot), so an IDN/uppercase/host. variant can't slip past. test_push_url_canonicalizes_idna_case_and_trailing_dot covers it. SSRF check runs before the allowlist check, so an operator can't allowlist an internal host.
• property_list.py exception redaction is complete. Fetch-failure logs type(exc).__name__ + list_id + safe_origin only; wire error carries agent_origin, never the full URL or auth_token; both raises use from None. test_fetch_failure_log_omits_exception_text verifies the token never reaches logs.
• audit_sink.py suppression is double-gated. Middleware (error_message=... if include_error_message else None) and SlackAlertSink._format (if self._include_error_message and ...) both default False — defense in depth.
• Imports resolve. WebhookDestinationPolicy / WebhookDestinationValidationError / validate_webhook_destination_url (webhooks.py:786/853/1049), canonicalize_host (_idna_canonicalize.py:44), and the validation struct exposes .hostname / .original_url / .effective_url / .policy (webhooks.py:842). No import-time break; new a2a_push_security.__all__ is intact.

Expert verdicts: security-reviewer — sound-with-caveats (one Medium, below). code-reviewer — no blockers, CI green across 3.10–3.13. ad-tech-protocol-expert — sound-with-caveats, but flagged the delete_info order as a likely-unsound high-severity and could not confirm the ABC.

Open questions (what flips this to Approve)

1. Does a2a-sdk 1.0.1 actually pass ServerCallContext to the store methods on the wire? The PR premise says yes; a2a_server.py:1023-1025 (unmodified) says the ABC does not. One is stale. No test drives tenant isolation through the SDK dispatcher — the isolation tests all call the store directly with hand-built contexts, and the one wire test drives set_info's URL rejection, which fires whether or not context is passed. If the installed SDK passes context=None in production, every call falls through to the ContextVar / __anonymous__ path and the "explicit context is authoritative" isolation never engages. Flip to approve: a dispatcher-level test where tenant A cannot read/delete tenant B's config through the ASGI app with an authenticated principal — not just via direct store calls.
2. delete_info(self, task_id, context=None, config_id=None) parameter order. ad-tech-protocol-expert reads the a2a-sdk 1.0 ABC as delete_info(task_id, config_id=None, context=None) and the dispatcher as calling it positionally. If so, the SQLAlchemy example (new) and SQLite example (pre-existing) bind config_id into the context slot → scope collapses to __anonymous__ and deletes silently no-op. A reference impl that can't delete a compromised callback destination is a real footgun. No test exercises delete_info through the dispatcher. Flip to approve: confirm the 1.0.1 ABC order (paste the push_notification_config_store.py signature) and add a delete-through-dispatch test.

The PR's premise and a2a_server.py:1023 can't both be right; whichever way that resolves, one of them needs an edit.

Follow-ups (non-blocking — file as issues)

• security-reviewer Medium — A2A push SSRF error echoes the resolved internal IP to the wire. a2a_push_security.py:125 raises InvalidParamsError(message=str(exc), ...); str(exc) for an SSRF rejection carries resolved IP 10.x.y.z is in a reserved range into the JSON-RPC -32602 envelope. Any buyer permitted to call CreateTaskPushNotificationConfig in public_https / allowlist mode gets an internal DNS→IP reconnaissance oracle. property_list.py already does this right (maps to exc.reason, no IP) — mirror it: generic message + data={"reason": exc.reason, "field": exc.field}. Push is disabled by default, so this only bites operators who opt in — fix before turning push on for an open ecosystem.
• property_list.py hardcodes WebhookDestinationPolicy.production() with no adopter override. schemas/cache/3.1/core/property-list-ref.json types agent_url as bare format: uri — HTTPS-only + public-IP-only rejects schema-valid http:// and private/VPC/on-prem/dev agent URLs. Security-motivated, but consider making the policy injectable.
• audit_sink default behavior change is wider than Slack. make_audit_middleware(include_error_message=False) suppresses AuditEvent.error_message for all sinks. An adopter on a local LoggingAuditSink silently loses exception text on upgrade. Non-breaking signature, but call it out in the release notes.
• Stale doc. a2a_server.py:1023-1025 still describes the old no-context ABC — update it to match the new premise regardless of how Q1 resolves.

Minor nits (non-blocking)

1. list_id regex allows . and ... property_list.py — re.fullmatch(r"[A-Za-z0-9._~-]+", list_id) accepts . / .., and quote(list_id, safe='') does not encode dots, so .. survives into {agent_url}/property-lists/.. → parent-path reference on the (validated, same-buyer) agent origin. Bounded impact, but the regex exists precisely to guarantee a benign segment — reject . / .. explicitly. Separately, the regex is narrower than the schema (type: string, no pattern) and will reject spec-valid ids containing /, :, +; since you already recommend quote(...), encoding beats rejecting.
2. safe_origin doesn't re-bracket IPv6. property_list.py — urlsplit().hostname strips brackets, so an IPv6 agent_url renders as https://2001:db8::1:443 in the log/error string. Cosmetic (no network use).
3. config_id synthesis isn't echoed back. The examples synthesize id or f"auto-{uuid.uuid4()}" but write it only to the PK column, not the returned PushNotificationConfig payload — per A2A the create result should echo the assigned id so the client can later delete it. Example-only, but worth a comment.

Happy to flip to Approve once Q1 and Q2 are answered — a single wire-level tenant-isolation-plus-delete test would close both.

[bokelley]

Addressed the latest Argus review in f680c08:

• Confirmed the installed a2a-sdk 1.0.1 signatures are set_info(task_id, notification_config, context), get_info(task_id, context), and delete_info(task_id, context, config_id=None). The dispatcher invokes all three in that order.
• Added an authenticated A2A 1.0 ASGI/JSON-RPC regression covering create, cross-principal list/delete isolation, same-principal get, and delete-through-dispatch.
• Updated the stale create_a2a_server documentation to describe context-first scoping and ContextVar only as a context-free fallback.
• Redacted rejected URLs, resolved private IPs, and policy internals from the JSON-RPC error; the wire now exposes only a generic message plus stable code/reason/field.

Verification: 35 focused tests pass, and Black, Ruff, mypy, adopter fixtures, Bandit, and all commit hygiene hooks pass. No unresolved inline review threads remain. Please re-review the updated head.

[aao-ipr-bot]

Solid fail-closed hardening of the A2A callback and sink boundaries. The load-bearing move is making the destination policy explicit — disabled / public_https / allowlist — and refusing to reflect the rejected URL, resolved IP, or classifier internals onto the JSON-RPC surface. Fail-closed beats fail-open, and this fails closed in the right places.

Things I checked

• SSRF wire non-reflection. validate_a2a_push_notification_url (src/adcp/server/a2a_push_security.py:117) puts only {code, reason, field} into InvalidParamsError.data — the offending URL, the resolved private IP (webhooks.py:1173 embeds it in the classifier message), and effective_url/policy are all dropped. test_push_config_destination_policy_reaches_jsonrpc_error_envelope asserts "127.0.0.1" not in str(error). Right shape.
• Allowlist canonicalization is symmetric. validation.hostname comes back from resolve_and_validate_host → canonicalize_host, the same function normalize_allowed_push_hosts runs on the allowlist, so the not in canonical_allowed_hosts check can't spuriously fail on case/IDNA/trailing-dot. Empty frozenset() normalizes to deny-all; None is public mode. Verified against the IDNA/trailing-dot test.
• Explicit unauthenticated context can't inherit an ambient tenant. _scope(context) resolves scope_from_server_context(context) if context is not None else provider() — a supplied-but-unauthenticated context returns None → anonymous, never the ContextVar. test_sqlite_explicit_unauthenticated_context_cannot_inherit_ambient_scope covers it. This was the isolation break I was watching for; it holds.
• delete_info(task_id, context, config_id) SDK arg order. ad-tech-protocol-expert flagged this as block-until-confirmed — if the SDK bound config_id into the context slot, scope would resolve anonymous and a principal's own delete would miss its row. test_push_config_wire_dispatch_isolates_principals_and_deletes drives DeleteTaskPushNotificationConfig through the real dispatcher (ASGITransport, raise_app_exceptions=True) and asserts tenant-B's delete leaves tenant-A's row intact while tenant-A's removes it. That's the end-to-end proof, and it's green across 3.10–3.13. Concern resolved.
• property_list.py error path leaks nothing. list_id regex gate → destination validation raising AdcpError with from None (only exc.reason) → safe_origin rebuilt as scheme://host[:port] from validation.original_url, stripping userinfo/path/query so a token in the URL can't round-trip. auth_token never reaches details. Clean.
• CI: 4/4 Python versions, Postgres conformance, storyboard runners, conventional-commit format, schema freshness — all SUCCESS.

Follow-ups (non-blocking — file as issues)

• Slack webhook URL can still land in a traceback. security-reviewer (Medium): SlackAlertSink.record's raise_for_status() produces an HTTPStatusError whose message embeds the full hooks.slack.com/services/... URL — a bearer secret — and _emit's exc_info=True renders it into logs on any 400/404/429. Pre-existing, not introduced here, but it contradicts the module's own "URL never appears in tracebacks" claim. Catch httpx.HTTPStatusError in record and re-raise scrubbed (status only).
• Property-list validate is advisory, not load-bearing on the connection. resolve_property_list validates then hands the fetcher validation.original_url, discarding validation.resolved_ip. A buyer serving agent_url with TTL=0 can pass validation and rebind at connect. Mitigated for compliant adopters via the documented build_async_ip_pinned_transport, but the fetch Protocol can't receive the pinned IP, so a naive fetcher reopens the window. Consider threading the validated IP (or a prebuilt pinned transport) into PropertyListFetcher.fetch.
• agent_url → agent_origin is wire-visible. ad-tech-protocol-expert: sanitize_error_details passes non-AUTHORIZATION_REQUIRED details through verbatim, so this key and the message string are adopter-observable. AdCP details is free-form, so no semver break — but log-scrapers and detail consumers break silently. Give it a changelog line. While there: the new INVALID_REQUEST for a bad list_id sets details but not the typed field (property_list.list_id), and recovery="correctable" is the legacy alias for retry_with_changes.

Minor nits (non-blocking)

1. Audit-sink include_error_message is a two-gate AND. code-reviewer: the middleware capture gate (make_audit_middleware) and the SlackAlertSink._format egress gate both default False, so a SlackAlertSink(include_error_message=True) wired under the default middleware emits nothing, with no diagnostic — a notable sharp edge. Same default flip also strips error_message from durable sinks like LoggingAuditSink. Intended and security-motivated, but worth a release note and a :param include_error_message: docstring entry (currently undocumented on make_audit_middleware).
2. list_id regex admits . and ... re.fullmatch(r"[A-Za-z0-9._~-]+", list_id) accepts dot-segments, which survive quote(..., safe=''). Harmless (buyer's own SSRF-validated host, buyer's token), but reject {".", ".."} for defense-in-depth. src/adcp/decisioning/property_list.py.
3. normalize_allowed_push_hosts error type on the disabled path. It raises WebhookDestinationValidationError rather than the friendly ValueError the surrounding resolve_push_destination_settings branches otherwise emit for a malformed host. Cosmetic. a2a_push_security.py:26.

Approving on the strength of the wire-level delete/isolation test plus green CI across 3.10–3.13. Follow-ups noted above.