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.

[KonstantinMirin]

not address.is_global reopens the ranges this branch just closed. src/adcp/signing/jwks.py:83-85 says so 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 8e51a3ea / b6b3f47f / 61e6d926 added to _EXTRA_BLOCKED_NETWORKS. With the host allowlisted, this validator accepts 192.88.99.1, 192.31.196.1, 192.52.193.1, 192.175.48.1, 2001:20::1 and 64:ff9b::7f00:1; adcp.webhooks.validate_webhook_destination_url rejects all six.

It also never resolves DNS, so the range check only ever fires for IP literals — an allowlisted hostname resolving into any reserved range passes unconditionally.

Delegate to validate_webhook_destination_url(url, policy=WebhookDestinationPolicy.production(...), field="push_notification_config.url") and layer the allowed_destination_hosts membership test on the returned WebhookDestinationValidation.hostname. That inherits _EXTRA_BLOCKED_NETWORKS, DNS resolution with a pinned IP, and control-character / fragment rejection (both https://callback.example/hook#frag and https://callback.example/hook\r\nX-Inject: 1 are accepted and persisted verbatim today).

[KonstantinMirin]

Hardcoded destination-port allowlist with no kwarg to widen it. schemas/cache/3.1/core/push-notification-config.json @ AdCP 3.1.8, properties.url.description: "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 a callback at all.

The SDK already models this: DEFAULT_ALLOWED_PORTS = frozenset({443, 8443}) at src/adcp/signing/jwks.py:112 is opt-in, and WebhookDestinationPolicy.production() defaults allowed_destination_ports=None. Take the same shape — permissive by default, operator passes a set to harden.

[KonstantinMirin]

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

canonicalize_host(parts.hostname) on line 44 is unguarded too, so attacker-controlled hostnames escape as raw idna errors whose str() lands on the wire: https://xn--<U+0080>.example/hook raises idna.core.InvalidCodepoint: Codepoint U+0080 at position 1 of '\x80' not allowed. resolve_and_validate_host catches (idna.IDNAError, UnicodeError, UnicodeEncodeError) at src/adcp/signing/jwks.py:279-282.

WebhookDestinationValidationError already subclasses ValueError, carries code = "INVALID_REQUEST" and reason / field, and src/adcp/server/translate.py:480 maps recovery="correctable" to InvalidParamsError (-32602).

[KonstantinMirin]

This branch always wins on the live path, which makes the _warned_anonymous guard below unreachable. 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.

With scope_provider=lambda: "tenant-a" and set_info(..., ServerCallContext(user=UnauthenticatedUser())): warnings [], stored scope __anonymous__. Two unauthenticated callers share one bucket and read each other's callback URLs and plaintext authentication.credentials, with no diagnostic — while the docstring at :364 still promises "Fails loudly on anonymous fallback."

Make _scope_from_server_context return str | None and let _scope() own the warn-once plus fall-through, so the contract holds whichever source produced the identity.

[KonstantinMirin]

Same shape as the SQLite store: a2a-sdk always passes a context, so _push_config_scope is now dead and the RuntimeWarning below never fires. _scope(ctx) returns "anonymous" with the ContextVar set to "tenant-a". Fix alongside examples/a2a_db_tasks.py:423 — one contract, applied to both.

[KonstantinMirin]

The gate lands on one of the sinks that receive this field. make_audit_middleware builds every failure event with error_message=str(exc)[:200] at :405; LoggingAuditSink.record at :193 emits event.model_dump_json() — the whole event, error_message included — and every adopter-implemented AuditSink gets the same ungated field.

If the stated reason holds ("exception messages can contain request values, upstream response fragments, or credentials"), the redaction belongs at event construction, not per sink: a redact_error_messages flag on make_audit_middleware, or an AuditEvent that carries the class name by default and the message under an explicit opt-in. Then no sink can leak it by omission.

Separately, grep -rn include_error_message returns four hits, all in this file — no test constructs SlackAlertSink(..., include_error_message=True). Invert or shadow the flag and the suite stays green while the operator who opted in gets no error text.

[KonstantinMirin]

Nothing enforces "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

The change also inverts the convention documented in this package — src/adcp/decisioning/dispatch.py:598: "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. In exchange the operator loses the ability to tell a fetch timeout from a 404 from a TLS failure.

Pick one boundary: keep the log line as it was, or drop the from exc chain / 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.

[KonstantinMirin]

These four are exactly the addresses where is_global agrees with the repo's classifier, so this reads as proof of a property the code does not have. Add 192.88.99.1, 192.31.196.1, 192.52.193.1, 192.175.48.1, 2001:20::1 and 64:ff9b::7f00:1 — all six are in _EXTRA_BLOCKED_NETWORKS or resolved by resolve_and_validate_host, all six are accepted today, and all six pass once validate_push_notification_url delegates to the shared classifier.

[KonstantinMirin]

This setup is inert. The context passed on line 148 makes _scope take the context is not None branch, so _push_config_scope is never read — store._scope(ctx) returns "anonymous" with the var set to "tenant-a". Deleting the set/reset pair changes nothing about the outcome. Either drop it, or assert the precedence it appears to assume, which is the assertion that would have caught the scope regression.

[KonstantinMirin]

This asserts the "a2a v1 contract" by calling the example's own methods in the arg order the author believes the SDK uses; it never references a2a.server.tasks.push_notification_config_store.PushNotificationConfigStore. If the order were wrong the test would pass identically.

More importantly, every store test in this PR calls set_info directly, so nothing drives the guard through the path production uses. tests/test_a2a_server.py:1190 test_custom_push_config_store_receives_sets_from_handler already shows the pattern — build the app via create_a2a_server(push_config_store=...) and drive handler.on_create_task_push_notification_config(params, _empty_call_context()). The same call with url="https://attacker.example/hook" would grade both the refusal and the buyer-visible envelope, and _empty_call_context() is exactly ServerCallContext(user=UnauthenticatedUser()), so it would have surfaced the scope regression too.