core-lead/molecule-core
1
0
Fork 0
forked from molecule-ai/molecule-core
Code Pull Requests Activity

fix(channel): validate peer_id at envelope build — close path-traversal foothold

Browse Source 
Two trust-boundary leaks surfaced in code review of the channel-envelope
enrichment work:

1. _agent_card_url_for(peer_id) interpolated raw input into
   ${PLATFORM_URL}/registry/discover/<peer_id> with no UUID guard. An
   upstream row with peer_id=`../../foo` produced an agent-visible URL
   pointing at a sibling registry path. Same trust-boundary rationale
   discover_peer's docstring already calls out: "never interpolate
   path-traversal characters into the URL". Now gated by _validate_peer_id;
   returns "" on validation failure.

2. _build_channel_notification echoed raw peer_id back into
   meta["peer_id"], which on the push path renders inside the agent's
   <channel peer_id="..." kind="..."> XML-attribute context. Attacker
   bytes (control chars, embedded quotes) would land in agent-rendered
   text wired into the next conversation turn. Now canonicalised through
   _validate_peer_id before any meta write; on validation failure we
   set "" rather than reflecting the raw bytes.

Defense-in-depth — both layers gate independently. Mutation-verified by
stashing both prod-side files and confirming both regression tests fail.

Tests:
- test_envelope_enrichment_invalid_peer_id_skips_lookup: updated to
  pin the safe behavior (peer_id="" + agent_card_url absent), not the
  prior leak shape.
- test_envelope_enrichment_strips_path_traversal_peer_id: NEW. Hard
  regression for peer_id="../../foo" — pins both the URL-builder and
  the meta echo against this specific exploit shape.
- Two existing tests updated to use UUID-shape placeholders instead
  of "ws-peer-uuid" / "peer-ws-uuid" since those non-UUIDs now correctly
  get stripped by the validator.

Resolves the Required-grade finding from the multi-axis review on PR #2471.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang 2026-05-01 18:43:49 -07:00
parent 885eff2350
commit 0b979aed78
3 changed files with 77 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
workspace/a2a_client.py
View File

@ -120,13 +120,22 @@ def enrich_peer_metadata(peer_id: str, *, now: float | None = None) -> dict | No
def _agent_card_url_for(peer_id: str) -> str:
"""Construct the platform-side agent-card URL for ``peer_id``.
Returns the empty string when ``peer_id`` is not a UUID — same
trust-boundary rationale as ``discover_peer``: never interpolate
path-traversal characters into a URL. An invalid id reflected back
to the receiving agent as ``…/registry/discover/../../foo`` is a
foothold we close at construction time.
Uses the registry's discovery path so the agent receiving a push
can hit a single endpoint to enumerate the sender's capabilities
+ role + URL. Same shape every workspace exposes regardless of
runtime — claude-code, hermes, langchain wrappers all register
through ``/registry/register`` and surface through ``/registry/discover``.
"""
return f"{PLATFORM_URL}/registry/discover/{peer_id}"
safe_id = _validate_peer_id(peer_id)
if safe_id is None:
return ""
return f"{PLATFORM_URL}/registry/discover/{safe_id}"
# Sentinel prefix for errors originating from send_a2a_message / child agents.
# Used by delegate_task to distinguish real errors from normal response text.

32
workspace/a2a_mcp_server.py
View File

@ -50,6 +50,7 @@ from a2a_client import ( # noqa: F401, E402
_A2A_ERROR_PREFIX,
_agent_card_url_for,
_peer_names,
_validate_peer_id,
discover_peer,
enrich_peer_metadata,
get_peers,
@ -402,16 +403,27 @@ def _build_channel_notification(msg: dict) -> dict:
peer_id = msg.get("peer_id") or ""
if peer_id:
record = enrich_peer_metadata(peer_id)
if record is not None:
if name := record.get("name"):
meta["peer_name"] = name
if role := record.get("role"):
meta["peer_role"] = role
# agent_card_url is constructable from peer_id alone; surface it
# even when enrichment fails so the receiving agent has a single
# endpoint to hit for capabilities lookup.
meta["agent_card_url"] = _agent_card_url_for(peer_id)
# Canonicalise via the same UUID guard discover_peer uses, so an
# upstream row with a malformed peer_id (path-traversal chars,
# control bytes, embedded XML quotes) can't reflect raw input
# into either the JSON-RPC envelope or the registry URL. Trust
# boundary lives here because peer_id is sourced from the inbox
# row, which is platform-trusted but not always agent-trusted.
safe_peer_id = _validate_peer_id(peer_id)
if safe_peer_id is None:
meta["peer_id"] = ""
else:
meta["peer_id"] = safe_peer_id
record = enrich_peer_metadata(safe_peer_id)
if record is not None:
if name := record.get("name"):
meta["peer_name"] = name
if role := record.get("role"):
meta["peer_role"] = role
# agent_card_url is constructable from peer_id alone; surface it
# even when enrichment fails so the receiving agent has a single
# endpoint to hit for capabilities lookup.
meta["agent_card_url"] = _agent_card_url_for(safe_peer_id)
return {
"jsonrpc": "2.0",

57
workspace/tests/test_a2a_mcp_server.py
View File

@ -197,7 +197,7 @@ def test_build_channel_notification_meta_carries_routing_fields():
payload = _build_channel_notification({
"activity_id": "act-7",
"text": "ping",
"peer_id": "ws-peer-uuid",
"peer_id": "11111111-2222-3333-4444-555555555555",
"kind": "peer_agent",
"method": "message/send",
"created_at": "2026-05-01T01:23:45Z",
@ -206,7 +206,7 @@ def test_build_channel_notification_meta_carries_routing_fields():
assert meta["source"] == "molecule"
assert meta["kind"] == "peer_agent"
assert meta["peer_id"] == "ws-peer-uuid"
assert meta["peer_id"] == "11111111-2222-3333-4444-555555555555"
assert meta["method"] == "message/send"
assert meta["activity_id"] == "act-7"
assert meta["ts"] == "2026-05-01T01:23:45Z"
@ -499,9 +499,12 @@ def test_envelope_enrichment_re_fetches_after_ttl(_reset_peer_metadata_cache):
def test_envelope_enrichment_invalid_peer_id_skips_lookup(_reset_peer_metadata_cache):
"""Defensive: a malformed peer_id (not a UUID) must not crash the
push path or cause a registry GET to be fired against an unsanitised
URL. enrich_peer_metadata returns None on validation failure; the
enrichment fields are simply absent."""
push path, must not fire a registry GET against an unsanitised URL,
and must not reflect the raw input back into either the envelope
`peer_id` field or the `agent_card_url`. UUID validation is a hard
trust boundary — the envelope's job is to surface metadata about
*trusted* peers, never to launder attacker-controlled bytes through
the JSON-RPC notification into the agent's rendered context."""
from a2a_mcp_server import _build_channel_notification
p, client = _patch_httpx_client(_make_httpx_response(200, {}))
@ -517,13 +520,43 @@ def test_envelope_enrichment_invalid_peer_id_skips_lookup(_reset_peer_metadata_c
"guards the URL-construction surface"
)
meta = payload["params"]["meta"]
assert meta["peer_id"] == "not-a-uuid"
# peer_id echo is canonicalised to empty-string on validation failure,
# so attacker bytes never reach the agent's <channel peer_id="..."> attr.
assert meta["peer_id"] == ""
assert "peer_name" not in meta
assert "peer_role" not in meta
# agent_card_url is constructed unconditionally from peer_id; even on
# an invalid id it's harmless (the receiving agent's GET will 404
# and it can fall back to inbox_pop without enrichment).
assert meta["agent_card_url"] == f"{__import__('a2a_client').PLATFORM_URL}/registry/discover/not-a-uuid"
# agent_card_url is omitted entirely rather than constructed against
# the unsanitised id — receiving agent gracefully degrades to
# inbox_pop without any URL to hit.
assert "agent_card_url" not in meta
def test_envelope_enrichment_strips_path_traversal_peer_id(_reset_peer_metadata_cache):
"""Hard regression for the trust-boundary issue surfaced in code review:
a peer_id containing path-traversal characters MUST NOT be interpolated
into the registry URL or echoed into the envelope. ``_agent_card_url_for``
builds against ``${PLATFORM_URL}/registry/discover/<peer_id>`` — without
the UUID guard, an upstream row with peer_id=``../../foo`` produces an
agent-visible URL pointing at a sibling path, and the receiving agent
would fetch from the wrong endpoint or the operator's reverse proxy
would normalise it into something unintended."""
from a2a_mcp_server import _build_channel_notification
p, client = _patch_httpx_client(_make_httpx_response(200, {}))
with p:
payload = _build_channel_notification({
"peer_id": "../../foo",
"kind": "peer_agent",
"text": "redirect-attempt",
})
assert client.get.call_count == 0
meta = payload["params"]["meta"]
assert meta["peer_id"] == ""
assert "agent_card_url" not in meta, (
"path-traversal peer_id leaked into agent_card_url — "
"_agent_card_url_for must call _validate_peer_id"
)
# ============== initialize handshake — capability declaration ==============
@ -877,7 +910,7 @@ async def test_inbox_bridge_emits_channel_notification_to_writer():
msg = {
"activity_id": "act-bridge-test",
"text": "hello from peer",
"peer_id": "peer-ws-uuid",
"peer_id": "11111111-2222-3333-4444-555555555555",
"kind": "peer_agent",
"method": "message/send",
"created_at": "2026-05-01T22:00:00Z",
@ -912,7 +945,7 @@ async def test_inbox_bridge_emits_channel_notification_to_writer():
meta = payload["params"]["meta"]
assert meta["source"] == "molecule"
assert meta["kind"] == "peer_agent"
assert meta["peer_id"] == "peer-ws-uuid"
assert meta["peer_id"] == "11111111-2222-3333-4444-555555555555"
assert meta["activity_id"] == "act-bridge-test"
assert meta["ts"] == "2026-05-01T22:00:00Z"
finally: