From 2dd458b025676b2a2664b8e367219dc007336e32 Mon Sep 17 00:00:00 2001 From: refined-element Date: Fri, 17 Jul 2026 16:16:57 -0400 Subject: [PATCH 1/2] fix(discover): skip malformed capability events instead of aborting discovery A capability event with a malformed `price` tag made discover() throw and abort the whole batch, dropping every valid capability with it. One hostile relay publishing one bad event could DoS discovery for every agent. Parse each authenticated event independently: skip and warn (loudly, never silently) on a malformed event, and return the rest. Includes a mutation-verified regression test that places the poison event mid-batch so a fix surviving only a trailing bad event would still fail. Fail-open audit ledger item 41. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01CYXrdWWjKo5AyGqFAfrdgK --- src/le_agent_sdk/agent/manager.py | 23 +++++++++-- tests/test_security_regressions.py | 64 ++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/src/le_agent_sdk/agent/manager.py b/src/le_agent_sdk/agent/manager.py index 15ea7b7..f27e069 100644 --- a/src/le_agent_sdk/agent/manager.py +++ b/src/le_agent_sdk/agent/manager.py @@ -219,10 +219,25 @@ async def discover( ) raw_events = await self._query_relays([nostr_filter], timeout=timeout) - return [ - AgentCapability.from_nostr_event(e) - for e in self._filter_authentic(raw_events) - ] + + # Parse each authenticated event INDEPENDENTLY. A malformed tag on one + # event (e.g. an unparseable `price` amount) must not abort the whole + # batch: a single hostile relay publishing one bad capability event + # would otherwise DoS discovery for every agent. Fail closed, loudly — + # the offending event is skipped and the skip is logged as a WARNING, + # never silently swallowed. + capabilities: list[AgentCapability] = [] + for event in self._filter_authentic(raw_events): + try: + capabilities.append(AgentCapability.from_nostr_event(event)) + except Exception as exc: + logger.warning( + "Skipping malformed capability event %.16s...: %s. " + "The relay may be malicious or misbehaving.", + event.get("id", ""), + exc, + ) + return capabilities async def publish_capability(self, capability: AgentCapability) -> str: """Publish a capability advertisement to relays. diff --git a/tests/test_security_regressions.py b/tests/test_security_regressions.py index a78f60b..a959f05 100644 --- a/tests/test_security_regressions.py +++ b/tests/test_security_regressions.py @@ -11,6 +11,7 @@ 5. AgentManager trusting unverified relay events. """ +import logging from unittest.mock import AsyncMock, patch import pytest @@ -437,6 +438,69 @@ async def test_listen_requests_drops_forged_events(self): assert len(received) == 1 assert received[0].pubkey == "aa" * 32, "forged request was yielded to caller" + @pytest.mark.asyncio + async def test_discover_skips_one_malformed_price_and_keeps_the_batch(self, caplog): + """Finding 6 (ledger #41): one bad `price` tag must not DoS discovery. + + A single hostile relay publishing one capability event with an + unparseable amount (e.g. ``["price", "abc"]``) used to raise ValueError + out of discover()'s list comprehension, dropping EVERY capability in the + batch — including all the well-formed ones. Parsing must be per-event: + the malformed event is skipped (failed closed) and logged (loudly), and + the valid capabilities are still returned. + + The malformed event is placed in the MIDDLE of the batch so a naive + fix that only survives a trailing bad event would still fail this. + """ + valid_a = { + "id": "good-a", + "pubkey": "aa" * 32, + "created_at": 1700000000, + "kind": 38400, + "content": "Valid A", + "tags": [["d", "svc-a"], ["price", "100", "sats", "per-request"]], + "sig": "", + } + malformed = { + "id": "bad-mid", + "pubkey": "cc" * 32, + "created_at": 1700000001, + "kind": 38400, + "content": "Malformed price", + "tags": [["d", "svc-bad"], ["price", "abc"]], + "sig": "", + } + valid_b = { + "id": "good-b", + "pubkey": "bb" * 32, + "created_at": 1700000002, + "kind": 38400, + "content": "Valid B", + "tags": [["d", "svc-b"], ["price", "200"]], + "sig": "", + } + + mgr = AgentManager() + with patch.object(mgr, "_query_relays", new_callable=AsyncMock) as mock_query: + mock_query.return_value = [valid_a, malformed, valid_b] + # Authenticity is orthogonal to parsing: stub verify() True so every + # event reaches the parse path (the drop-on-forgery path is tested + # separately above). + with patch.object(NostrEvent, "verify", return_value=True): + with caplog.at_level(logging.WARNING): + caps = await mgr.discover() + + # The batch must NOT abort: both well-formed capabilities survive. + assert len(caps) == 2 + assert {c.service_id for c in caps} == {"svc-a", "svc-b"} + # The malformed event is skipped, not included. + assert "svc-bad" not in {c.service_id for c in caps} + # ...and its rejection is loud: a WARNING naming the offending event id. + assert any( + record.levelno == logging.WARNING and "bad-mid" in record.getMessage() + for record in caplog.records + ), "malformed event was skipped silently instead of logged" + @pytest.mark.asyncio async def test_listen_requests_surfaces_missing_dep_without_reconnect_storm(self): """A missing dep must not be mistaken for a relay fault and retried.""" From 1b6cc38d7b1f0380ce9344f94701ca9c44d164f8 Mon Sep 17 00:00:00 2001 From: refined-element Date: Fri, 17 Jul 2026 16:34:02 -0400 Subject: [PATCH 2/2] fix(discover): also skip non-dict and field-missing relay events (ledger 41) The price-tag fix closed the parse step, but two more single-hostile-event DoS vectors sat upstream of it in discover(), still able to abort the whole batch: A. A relay event dict missing a committed field (pubkey/created_at/kind/ tags/content) made NostrEvent.verify() -> compute_id() raise KeyError out of _is_event_authentic()/_filter_authentic(). _is_event_authentic now drops the offending event (warn, return False) while STILL propagating a RuntimeError crypto-backend fault (CryptoBackendUnavailableError is a RuntimeError subclass) so a backend outage fails closed and loudly rather than masquerading as "nothing found". B. A non-dict relay payload (str/list) made _query_relays do `.get` on a str, raising AttributeError. _query_relays now skips non-dict payloads with a warning before the `.get`. Both are mutation-verified with poison events placed mid-batch. The existing crypto-backend-unavailable / verification-error-propagation tests continue to pass: Runtime(backend) faults propagate; hostile-event parse faults (KeyError/ TypeError/ValueError) are dropped. Fail-open audit ledger item 41. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01CYXrdWWjKo5AyGqFAfrdgK --- src/le_agent_sdk/agent/manager.py | 36 ++++++++++++- tests/test_security_regressions.py | 86 ++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+), 1 deletion(-) diff --git a/src/le_agent_sdk/agent/manager.py b/src/le_agent_sdk/agent/manager.py index f27e069..bc04eb7 100644 --- a/src/le_agent_sdk/agent/manager.py +++ b/src/le_agent_sdk/agent/manager.py @@ -82,7 +82,31 @@ def _is_event_authentic(event: dict[str, Any]) -> bool: fault affecting every event, and silently returning zero results would misrepresent it as "nothing found". """ - if NostrEvent.verify(event): + try: + verified = NostrEvent.verify(event) + except RuntimeError: + # Environment/crypto-backend fault affecting every event — propagate + # (fail closed, loudly). CryptoBackendUnavailableError is a + # RuntimeError subclass, and a hostile event cannot induce a + # RuntimeError from verify() (malformed input raises KeyError/ + # TypeError, dropped below), so propagating RuntimeError surfaces a + # backend outage without letting one bad event abort the batch. + # Downgrading it to "unauthentic" would silently return zero results + # and misrepresent a backend outage as "nothing found". + raise + except Exception as exc: + # A hostile/malformed relay event (missing committed field, wrong + # types) can make verify() raise while computing the id. Drop only + # that event — one bad event must not abort the whole query. + logger.warning( + "Dropping malformed relay event %.16s...: %s. " + "The relay may be malicious or misbehaving.", + event.get("id", ""), + exc, + ) + return False + + if verified: return True logger.warning( @@ -166,6 +190,16 @@ async def _query_relays( for result in results: if isinstance(result, list): for event in result: + if not isinstance(event, dict): + # A hostile relay can send a non-dict payload (str/list). + # Drop it before `.get` raises AttributeError out of the + # whole query — one bad event must not DoS discovery. + logger.warning( + "Dropping non-dict relay payload of type %s. " + "The relay may be malicious or misbehaving.", + type(event).__name__, + ) + continue event_id = event.get("id", "") if event_id and event_id not in seen_ids: seen_ids.add(event_id) diff --git a/tests/test_security_regressions.py b/tests/test_security_regressions.py index a959f05..d1d1a51 100644 --- a/tests/test_security_regressions.py +++ b/tests/test_security_regressions.py @@ -501,6 +501,92 @@ async def test_discover_skips_one_malformed_price_and_keeps_the_batch(self, capl for record in caplog.records ), "malformed event was skipped silently instead of logged" + @pytest.mark.asyncio + async def test_discover_skips_field_missing_event_and_keeps_batch(self, caplog): + """Vector A (ledger #41): a dict missing committed fields must not DoS. + + _is_event_authentic -> NostrEvent.verify -> compute_id subscripts + pubkey/created_at/kind/tags/content. A relay event missing any of them + raised KeyError out of _filter_authentic, killing the whole discover() + batch — the same single-hostile-event DoS the price fix set out to + close, one step earlier in the pipeline. The malformed event must be + dropped (failed closed) and logged (loudly); the two genuinely-signed + capabilities must still come back. + + These are REAL signed events (coincurve is available in the test env), + and verify() is deliberately NOT stubbed so the malformed event actually + reaches the id-computation that raises. Poison event is mid-batch. + """ + signed_a = NostrEvent.create( + kind=38400, + content="Valid A", + tags=[["d", "svc-a"], ["price", "100"]], + private_key="11" * 32, + ) + signed_b = NostrEvent.create( + kind=38400, + content="Valid B", + tags=[["d", "svc-b"], ["price", "200"]], + private_key="22" * 32, + ) + # Missing pubkey/created_at/tags/content -> verify() raises KeyError. + field_missing = {"id": "bad-missing", "kind": 38400} + + mgr = AgentManager() + with patch.object(mgr, "_query_relays", new_callable=AsyncMock) as mock_query: + mock_query.return_value = [signed_a, field_missing, signed_b] + with caplog.at_level(logging.WARNING): + caps = await mgr.discover() + + assert len(caps) == 2 + assert {c.service_id for c in caps} == {"svc-a", "svc-b"} + assert "bad-missing" not in {c.event_id for c in caps} + assert any( + record.levelno == logging.WARNING and "bad-missing" in record.getMessage() + for record in caplog.records + ), "field-missing event was dropped silently instead of logged" + + @pytest.mark.asyncio + async def test_discover_skips_non_dict_relay_payload_and_keeps_batch(self, caplog): + """Vector B (ledger #41): a non-dict relay payload must not DoS. + + _query_relays did ``event.get("id", "")`` over whatever the relay + returned. A hostile relay sending a str/list instead of an event dict + raised AttributeError out of discover(). It must be dropped before the + ``.get``, keeping the valid capabilities. + + _query_relay (the PER-relay method) is patched so the REAL _query_relays + runs its new isinstance guard. Poison payload is mid-batch. + """ + signed_a = NostrEvent.create( + kind=38400, + content="Valid A", + tags=[["d", "svc-a"], ["price", "100"]], + private_key="11" * 32, + ) + signed_b = NostrEvent.create( + kind=38400, + content="Valid B", + tags=[["d", "svc-b"], ["price", "200"]], + private_key="22" * 32, + ) + + mgr = AgentManager() + + async def fake_query_relay(url, filters, timeout): + return [signed_a, "not-a-dict", signed_b] + + with patch.object(mgr, "_query_relay", side_effect=fake_query_relay): + with caplog.at_level(logging.WARNING): + caps = await mgr.discover() + + assert len(caps) == 2 + assert {c.service_id for c in caps} == {"svc-a", "svc-b"} + assert any( + record.levelno == logging.WARNING and "non-dict" in record.getMessage().lower() + for record in caplog.records + ), "non-dict relay payload was dropped silently instead of logged" + @pytest.mark.asyncio async def test_listen_requests_surfaces_missing_dep_without_reconnect_storm(self): """A missing dep must not be mistaken for a relay fault and retried."""