From 07ab624d2e1f4504a59c9d86787335664e67fafc Mon Sep 17 00:00:00 2001 From: GitHub CI Date: Thu, 16 Apr 2026 22:30:20 -0700 Subject: [PATCH] fix(a2a): accept top-level `token` on PushNotificationConfig (reopens #61) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real root cause of push-notification failures against Workstacean. Visibility fix in #62 surfaced the actual error — Workstacean's callback endpoint was returning HTTP 401 "Invalid notification token", not the 404/silence we initially feared. The A2A spec's PushNotificationConfig allows two interchangeable shapes for the webhook bearer token: 1. Top-level `token` — the simple form @a2a-js/sdk serialises by default. Workstacean's SkillDispatcherPlugin uses this path: `setTaskPushNotificationConfig({ pushNotificationConfig: {url, token} })` (a2a-executor.ts:329-331). 2. Structured `authentication.credentials` — RFC-8821 AuthenticationInfo with `schemes` + `credentials`. Quinn only read shape #2. Workstacean's top-level token fell on the floor → stored `cfg.token = None` → `_deliver_webhook` skipped the `Authorization: Bearer ` header → Workstacean's callback handler found no bearer, computed `providedToken = ""`, mismatched the expected token, and rejected with 401 for every delivery. Live evidence from v0.1.8 logs: push config registered (jsonrpc) task=1167ad7f... → .../callback/1167ad7f... webhook delivered task=1167ad7f... state=completed → .../callback/1167ad7f... (401) Fix: factored token extraction into `_extract_push_token(cfg)` that checks top-level first, falls back to `authentication.credentials`, and returns None cleanly when neither is present. All three PushNotificationConfig parse sites (JSON-RPC set handler, REST alias handler, shared `_parse_push_config` at submit time) now route through it — no more divergent token parsing across the three entry points. 4 regression tests cover: top-level token, structured authentication, both-present-precedence, none-present-is-None. Co-Authored-By: Claude Opus 4.6 (1M context) --- a2a_handler.py | 43 +++++++++++++++++++++++++++++++++------ tests/test_a2a_handler.py | 41 +++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 6 deletions(-) diff --git a/a2a_handler.py b/a2a_handler.py index fd5da5f..aa8c45a 100644 --- a/a2a_handler.py +++ b/a2a_handler.py @@ -617,14 +617,47 @@ def _parse_push_config(configuration: dict) -> PushNotificationConfig | None: if not _is_safe_webhook_url(url): logger.warning("[a2a] rejected unsafe webhook url: %s", url) return None - auth = cfg.get("authentication") or {} return PushNotificationConfig( url=url, - token=auth.get("credentials"), + token=_extract_push_token(cfg), id=cfg.get("id", str(uuid4())), ) +def _extract_push_token(cfg: dict) -> str | None: + """Pull the bearer token from a PushNotificationConfig. + + The A2A spec's PushNotificationConfig allows two shapes for the + authentication secret, and clients choose between them at will: + + 1. **Top-level ``token``** — the simple form every SDK produces by + default. ``@a2a-js/sdk`` serialises ``{url, token}`` directly + and Workstacean's ``SkillDispatcherPlugin`` takes this path + (``a2a-executor.ts:329`` → ``setTaskPushNotificationConfig({ + pushNotificationConfig: { url, token } })``). + 2. **Structured ``authentication.credentials``** — the RFC-8821 + ``AuthenticationInfo`` form with ``schemes`` + ``credentials``. + + Before this helper Quinn only read (2), so Workstacean's top-level + token fell on the floor → Quinn sent callbacks with no + ``Authorization`` header → Workstacean returned 401 "Invalid + notification token" → TaskTracker fell back to polling every + dispatch (quinn#61 follow-up). + + Preference: top-level ``token`` first (what most callers send), fall + back to ``authentication.credentials``. Either one null-safely + resolves to ``None`` which disables the header entirely — the same + behaviour as the SSRF guard letting a public-IP callback through + unauthenticated. + """ + top_level = cfg.get("token") + if isinstance(top_level, str) and top_level: + return top_level + auth = cfg.get("authentication") or {} + creds = auth.get("credentials") + return creds if isinstance(creds, str) and creds else None + + # ── Webhook delivery ────────────────────────────────────────────────────────── @@ -1199,10 +1232,9 @@ def _rpc_result(result): "webhook url rejected: must be http/https, public IP, " "not loopback/private/link-local/multicast/reserved", ) - auth = cfg_in.get("authentication") or {} cfg = PushNotificationConfig( url=url, - token=auth.get("credentials"), + token=_extract_push_token(cfg_in), id=cfg_in.get("id", str(uuid4())), ) async with _store._lock: @@ -1407,10 +1439,9 @@ async def _create_push_config(task_id: str, request: Request, body: dict): "not loopback/private/link-local/multicast/reserved", ) - auth = body.get("authentication") or {} cfg = PushNotificationConfig( url=url, - token=auth.get("credentials"), + token=_extract_push_token(body), id=body.get("id", str(uuid4())), ) diff --git a/tests/test_a2a_handler.py b/tests/test_a2a_handler.py index 8c34d1b..7ec250d 100644 --- a/tests/test_a2a_handler.py +++ b/tests/test_a2a_handler.py @@ -285,6 +285,47 @@ def test_parse_push_config_rejects_unsafe_url(): assert cfg is None +def test_extract_push_token_top_level(): + """A2A spec PushNotificationConfig accepts two token shapes: + top-level `token` (what @a2a-js/sdk serialises by default and + Workstacean's SkillDispatcherPlugin uses) OR structured + `authentication.credentials`. Before quinn only read the latter — + Workstacean's top-level token landed in `cfg.token` as None, + Quinn's `_deliver_webhook` skipped the Authorization header, and + Workstacean's callback endpoint returned 401 on every delivery. + quinn#61 follow-up.""" + from a2a_handler import _extract_push_token + assert _extract_push_token({"token": "ws-token-xyz"}) == "ws-token-xyz" + + +def test_extract_push_token_structured_authentication(): + """The RFC-8821 AuthenticationInfo shape still works.""" + from a2a_handler import _extract_push_token + assert _extract_push_token({ + "authentication": {"schemes": ["Bearer"], "credentials": "rfc-token"}, + }) == "rfc-token" + + +def test_extract_push_token_prefers_top_level_when_both_present(): + """If a caller sends both (unlikely but spec-permitted), top-level + wins — it's the common case and structured is the fallback.""" + from a2a_handler import _extract_push_token + assert _extract_push_token({ + "token": "preferred", + "authentication": {"credentials": "fallback"}, + }) == "preferred" + + +def test_extract_push_token_none_when_absent(): + """Neither shape present → None. Delivery still fires, but without + an Authorization header (for public or unauthenticated callbacks).""" + from a2a_handler import _extract_push_token + assert _extract_push_token({}) is None + assert _extract_push_token({"token": ""}) is None + assert _extract_push_token({"authentication": {}}) is None + assert _extract_push_token({"token": None}) is None + + # ── SSRF allowlist (trusted docker-network hosts) ────────────────────────────