From dab91c379e0cdc04d68903d552009581930072ff Mon Sep 17 00:00:00 2001 From: Naseem Alnaji Date: Sat, 8 Aug 2026 15:03:57 +0000 Subject: [PATCH 1/2] fix(redaction): close forgery gap in event-level hook, dedupe id generation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the redact_event hook found the RESTORED_FIELDS set was too narrow — a hook could forge or erase actor identity, tags, and properties despite those being protected from the string-level hook. Widen it to match, tighten the hook's typing to RedactEventFunction/Event|None so a non-conforming return is a type error rather than a silent contract, document the mutate-and-return / full-dump-not-diff contract everywhere a hook author would read it, drop the undocumented "or times out" claim, and dedupe the id-generation snippet in event_queue.py into one helper. Bump to 2.0.2 (patch). --- README.md | 2 ++ pyproject.toml | 2 +- src/agentcat/modules/event_queue.py | 14 +++++++---- src/agentcat/modules/redaction.py | 36 +++++++++++++++++++++++------ src/agentcat/types.py | 27 ++++++++++++++++------ tests/test_redaction.py | 36 +++++++++++++++++++++++++++++ 6 files changed, 97 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 961c34b..cde0c07 100644 --- a/README.md +++ b/README.md @@ -145,6 +145,8 @@ agentcat.track(server, "proj_0000000", AgentCatOptions(redact_sensitive_informat For redaction decisions that need more context than a single string — such as which tool was called or what type of event is being published — use the event-level `redact_event` hook. It receives the full event and returns a modified event, or `None` to drop the event entirely. It may be sync or async, runs before `redact_sensitive_information` (so it sees raw values), and can be combined with it. +Mutate the event you're given and return it, as in the example below — don't construct or return a different `Event`. The return value replaces the event's fields wholesale, not as a diff against the original, so returning a partial or freshly-built event will silently drop every field you didn't set. + ```python from agentcat import AgentCatOptions diff --git a/pyproject.toml b/pyproject.toml index 0b15a20..4f5f40d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "agentcat" -version = "2.0.1" +version = "2.0.2" description = "Analytics tool for MCP (Model Context Protocol) servers, Claude Connectors, and ChatGPT Plugins - tracks tool usage patterns and provides insights" authors = [ { name = "AgentCat, Inc.", email = "support@agentcat.com" }, diff --git a/src/agentcat/modules/event_queue.py b/src/agentcat/modules/event_queue.py index 0fdb5e2..20cbc16 100644 --- a/src/agentcat/modules/event_queue.py +++ b/src/agentcat/modules/event_queue.py @@ -47,6 +47,12 @@ class _Stop: _STOP = _Stop() +def _ensure_event_id(event: UnredactedEvent) -> None: + """Assign a ksuid if the event doesn't already have one.""" + if not event.id: + event.id = generate_prefixed_ksuid(EVENT_ID_PREFIX) + + class EventQueue: """Manages event queue and sending to AgentCat API.""" @@ -159,8 +165,7 @@ def _process_event(self, event: UnredactedEvent) -> None: # Event-level redaction hook runs first, on raw values, and may # drop the event entirely. try: - if not event.id: - event.id = generate_prefixed_ksuid(EVENT_ID_PREFIX) + _ensure_event_id(event) redacted_event = apply_event_redaction(event, event.event_redaction_fn) if redacted_event is None: write_to_log(f"Event {event.id} dropped by redact_event hook") @@ -179,8 +184,7 @@ def _process_event(self, event: UnredactedEvent) -> None: if event and event.redaction_fn: # Redact sensitive information if a redaction function is provided try: - if not event.id: - event.id = generate_prefixed_ksuid(EVENT_ID_PREFIX) + _ensure_event_id(event) redacted_event = redact_event(event, event.redaction_fn) # The redacted event is already the full event object, not a dict event = redacted_event @@ -207,7 +211,7 @@ def _process_event(self, event: UnredactedEvent) -> None: ) if event: - event.id = event.id or generate_prefixed_ksuid("evt") + _ensure_event_id(event) # Send to AgentCat API only if project_id exists if event.project_id: diff --git a/src/agentcat/modules/redaction.py b/src/agentcat/modules/redaction.py index 9537042..7549a40 100644 --- a/src/agentcat/modules/redaction.py +++ b/src/agentcat/modules/redaction.py @@ -5,7 +5,7 @@ from agentcat.modules.hooks import drive_hook_result if TYPE_CHECKING: - from agentcat.types import Event, UnredactedEvent + from agentcat.types import Event, RedactEventFunction, UnredactedEvent # Set of field names that should be protected from redaction. @@ -158,8 +158,8 @@ def redact_event(event: "UnredactedEvent", redact_fn: Callable[[str], str]) -> " def _sync_event_redactor( - redact_event_fn: Callable[["Event"], Any], -) -> Callable[["Event"], Any]: + redact_event_fn: "RedactEventFunction", +) -> Callable[["Event"], "Event | None"]: """A synchronous view of the customer's event-level redaction hook. Same rationale as `_sync_redactor`: the publish worker is a thread with no @@ -167,7 +167,7 @@ def _sync_event_redactor( completion here rather than assigned straight into the pipeline. """ - def run(event: "Event") -> Any: + def run(event: "Event") -> "Event | None": return drive_hook_result(redact_event_fn(event), "redact_event") return run @@ -175,20 +175,28 @@ def run(event: "Event") -> Any: # Fields restored from the original event after the event-level redaction # hook runs, regardless of what the hook returns. These are system-managed — -# not consumer-settable — mirroring RESTORED_FIELDS in the TypeScript SDK and -# the equivalent snapshot/restore around ApplyEventRedaction in the Go SDK. +# not consumer-settable. Broader than the equivalent RESTORED_FIELDS in the +# TypeScript SDK and the snapshot/restore around ApplyEventRedaction in the Go +# SDK, which cover only id/session_id/project_id/event_type/timestamp and so +# still let a hook forge or erase actor identity, tags, and properties. RESTORED_FIELDS: Set[str] = { "id", "session_id", "project_id", "event_type", "timestamp", + "actor_id", + "identify_actor_given_id", + "identify_actor_name", + "identify_data", + "tags", + "properties", } def apply_event_redaction( event: "UnredactedEvent", - redact_event_fn: Callable[["Event"], Any], + redact_event_fn: "RedactEventFunction", ) -> "UnredactedEvent | None": """ Applies the customer's event-level redaction hook to an event. @@ -199,6 +207,20 @@ def apply_event_redaction( force-restored from the original event afterward, so a hook cannot forge or erase what AgentCat itself assigned. + `RedactEventFunction` (see types.py) declares the hook returns a pydantic + `Event | None` — mutate and return the `Event` you were handed, not a + different object. That contract is enforced by type hints only, not at + runtime: `redact_event_fn` is called and its result is trusted to be + `Event`-shaped, so a hook that ignores its type hints and returns + something else (e.g. a plain dict) raises `AttributeError` here rather + than degrading gracefully. Type-check hooks against `RedactEventFunction` + (mypy/pyright) to catch this before it ships. + + The return value is also applied as a wholesale replacement of the + event's fields (`result.model_dump()`, not a diff against the original), + so a freshly-built or partial `Event` — even though it satisfies the type + hint — will silently null out every field the hook didn't set. + The hook is handed a plain `Event` built from the dump, excluding `redaction_fn`/`event_redaction_fn` — they are machinery, not event data, and the customer's hook must not be handed its own function objects. diff --git a/src/agentcat/types.py b/src/agentcat/types.py index c52b2b0..7b7582d 100644 --- a/src/agentcat/types.py +++ b/src/agentcat/types.py @@ -39,9 +39,17 @@ # Type alias for redaction function RedactionFunction = Callable[[str], str | Awaitable[str]] # Type alias for the event-level redaction hook. Receives the full event -# (raw, unredacted values — it runs before RedactionFunction) and returns a -# modified event, or None to drop the event entirely. Accepts sync or async -# callables (mirrors RedactionFunction). +# (raw, unredacted values — it runs before RedactionFunction) and MUST return +# a pydantic Event or None to drop the event entirely — never a dict or other +# object, even though one might satisfy a looser hand-written signature. This +# is enforced by type checking only: type-check hooks against +# RedactEventFunction (mypy/pyright), since a hook that returns something +# else at runtime fails inside apply_event_redaction rather than degrading +# gracefully. Accepts sync or async callables (mirrors RedactionFunction). +# Mutate and return the event you're handed — the return value replaces the +# event's fields wholesale, not a diff, so a freshly-built or partial event +# silently loses every field it didn't set. See the redact_event option on +# AgentCatOptions for the full contract. RedactEventFunction = Callable[ ["Event"], Optional["Event"] | Awaitable[Optional["Event"]] ] @@ -213,10 +221,15 @@ class AgentCatOptions: # Event-level redaction hook, invoked with the full event (raw, unredacted # values) before redact_sensitive_information runs. May return a modified # event, or None to drop the event entirely. May be sync or async. The - # system-managed fields id, session_id, project_id, event_type, and - # timestamp cannot be changed by this hook — they are restored from the - # original event afterward. If the hook raises (or times out), the event - # is dropped rather than published unredacted. + # system-managed fields id, session_id, project_id, event_type, + # timestamp, actor_id, identify_actor_given_id, identify_actor_name, + # identify_data, tags, and properties cannot be changed by this hook — + # they are restored from the original event afterward. If the hook + # raises, the event is dropped rather than published unredacted. + # Mutate and return the event you're given rather than constructing a new + # one: the return value replaces the event's fields wholesale (it is not + # diffed against the original), so a freshly-built or partial event + # silently drops everything you didn't set. redact_event: RedactEventFunction | None = None exporters: dict[str, ExporterConfig] | None = None # Debug logging to ~/agentcat.log. Tri-state: None (the default) defers to diff --git a/tests/test_redaction.py b/tests/test_redaction.py index 0f20934..3daf762 100644 --- a/tests/test_redaction.py +++ b/tests/test_redaction.py @@ -555,8 +555,44 @@ def forge(event): "project_id", "event_type", "timestamp", + "actor_id", + "identify_actor_given_id", + "identify_actor_name", + "identify_data", + "tags", + "properties", } + def test_restored_fields_survive_actor_and_tag_forgery(self): + """A hook cannot reassign the Actor an event is attributed to, or + forge the tags/properties a customer's own callbacks attached.""" + + def forge(event): + event.actor_id = "actor_forged" + event.identify_actor_given_id = "forged-actor" + event.identify_actor_name = "Forged Actor" + event.identify_data = {"email": "attacker@evil.com"} + event.tags = {"env": "forged"} + event.properties = {"forged": True} + return event + + original = self._event( + actor_id="actor_real", + identify_actor_given_id="real-actor-42", + identify_actor_name="Real Actor", + identify_data={"email": "real@example.com"}, + tags={"env": "prod"}, + properties={"real": True}, + ) + result = apply_event_redaction(original, forge) + + assert result.actor_id == original.actor_id + assert result.identify_actor_given_id == original.identify_actor_given_id + assert result.identify_actor_name == original.identify_actor_name + assert result.identify_data == original.identify_data + assert result.tags == original.tags + assert result.properties == original.properties + def test_a_raising_hook_propagates_so_the_queue_drops_the_event(self): def boom(_event): raise RuntimeError("event redaction exploded") From 2924f71bd85925f1c471efbd202458b61567ee7d Mon Sep 17 00:00:00 2001 From: Naseem Alnaji Date: Sat, 8 Aug 2026 15:55:36 +0000 Subject: [PATCH 2/2] fix(redaction): protect client/server identity, stop dict aliasing RESTORED_FIELDS omitted client_name/client_version/server_name/server_version, so a redact_event hook could forge or erase MCP client/server identity even though those fields are protected from the string-level hook. The restored tags/properties/identify_data were also aliased rather than copied, letting in-place mutation of one event's dict corrupt another that shared the reference. Also backfills test coverage: two tests that passed regardless of the behavior they claimed to check, plus new coverage for SystemExit handling in the event-level hook, an async hook resolving to None, the original-event- unmutated invariant, a hook returning a non-Event value, server/client field protection, and the partial-Event field-nulling behavior. --- src/agentcat/modules/redaction.py | 16 ++- tests/test_event_queue.py | 32 +++++- tests/test_redaction.py | 156 ++++++++++++++++++++++++++++-- 3 files changed, 194 insertions(+), 10 deletions(-) diff --git a/src/agentcat/modules/redaction.py b/src/agentcat/modules/redaction.py index 7549a40..243ad39 100644 --- a/src/agentcat/modules/redaction.py +++ b/src/agentcat/modules/redaction.py @@ -1,5 +1,6 @@ """PII redaction for AgentCat logs.""" +import copy from typing import Any, TYPE_CHECKING, Callable, Set from agentcat.modules.hooks import drive_hook_result @@ -178,7 +179,8 @@ def run(event: "Event") -> "Event | None": # not consumer-settable. Broader than the equivalent RESTORED_FIELDS in the # TypeScript SDK and the snapshot/restore around ApplyEventRedaction in the Go # SDK, which cover only id/session_id/project_id/event_type/timestamp and so -# still let a hook forge or erase actor identity, tags, and properties. +# still let a hook forge or erase actor identity, client/server identity, +# tags, and properties. RESTORED_FIELDS: Set[str] = { "id", "session_id", @@ -191,6 +193,10 @@ def run(event: "Event") -> "Event | None": "identify_data", "tags", "properties", + "client_name", + "client_version", + "server_name", + "server_version", } @@ -246,7 +252,13 @@ def apply_event_redaction( if result is None: return None - restored = {field: getattr(event, field) for field in RESTORED_FIELDS} + # Deep-copied, not aliased: tags/properties/identify_data are mutable, and + # a customer callback commonly hands back the same cached dict across + # events — aliasing here would let mutating one event's dict silently + # corrupt another event that shares the reference. + restored = { + field: copy.deepcopy(getattr(event, field)) for field in RESTORED_FIELDS + } updated = result.model_dump(warnings=False) updated.update(restored) redacted: UnredactedEvent = event.model_copy(update=updated) diff --git a/tests/test_event_queue.py b/tests/test_event_queue.py index ef5a9b6..67ec7e9 100644 --- a/tests/test_event_queue.py +++ b/tests/test_event_queue.py @@ -1,6 +1,7 @@ """Test event queue functionality.""" import queue +import sys import threading import time from datetime import datetime, timezone @@ -287,7 +288,13 @@ def test_process_event_with_event_redaction(self, mock_apply): session_id="session-123", timestamp=datetime.now(timezone.utc), resource_name="dropped-and-replaced", - event_redaction_fn=None, # This should be cleared after redaction + # A real (unmocked) apply_event_redaction preserves the original + # hook on its returned copy — model_copy(update=...) never + # touches event_redaction_fn, since it's excluded from the dump + # and isn't in RESTORED_FIELDS. Mirroring that here means the + # `is None` assertion below actually exercises _process_event's + # own clearing line rather than just echoing the fixture. + event_redaction_fn=mock_event_redaction_fn, ) mock_apply.return_value = redacted_event @@ -357,6 +364,29 @@ def test_process_event_event_redaction_failure(self, mock_log, mock_apply): assert "test-id" in log_message mock_send.assert_not_called() + def test_process_event_event_redaction_hook_sys_exit_does_not_kill_worker(self): + """A redact_event hook calling sys.exit() must not propagate out of + _process_event — SystemExit doesn't subclass Exception, so it needs + its own except clause, and _worker's own `except Exception` wrapper + would not have caught it either.""" + eq = EventQueue() + + def exits(event): + sys.exit(1) + + event = UnredactedEvent( + id="test-id", + event_type="mcp:tools/call", + project_id="project-123", + session_id="session-123", + timestamp=datetime.now(timezone.utc), + event_redaction_fn=exits, + ) + + with patch.object(eq, "_send_event") as mock_send: + eq._process_event(event) # must not raise SystemExit + mock_send.assert_not_called() + def test_process_event_event_hook_runs_before_string_hook(self): """Ordering: the event hook must see raw values, before the string hook ever runs — nothing mocked, both hooks wired for real.""" diff --git a/tests/test_redaction.py b/tests/test_redaction.py index 3daf762..036bd78 100644 --- a/tests/test_redaction.py +++ b/tests/test_redaction.py @@ -2,6 +2,7 @@ import pytest from typing import Any, Dict +from unittest.mock import patch from agentcat.modules.redaction import ( redact_strings_in_object, redact_event, @@ -400,6 +401,9 @@ def _event(**overrides): "parameters": {"arguments": {"text": "SECRET body"}}, "response": {"content": [{"type": "text", "text": "SECRET answer"}]}, "client_name": "SECRET client", + "client_version": "SECRET client version", + "server_name": "SECRET server", + "server_version": "SECRET server version", "identify_actor_given_id": "SECRET actor", "identify_data": {"email": "SECRET@example.com"}, "tags": {"env": "SECRET tag"}, @@ -420,9 +424,13 @@ def redact_fn(s: str) -> str: assert result.response == { "content": [{"type": "text", "text": "[REDACTED] answer"}] } - # client_name is now a protected field (see PROTECTED_FIELDS) — it - # must survive untouched, same as the other protected fields below. + # client/server identity fields are protected (see PROTECTED_FIELDS) + # — they must survive untouched, same as the other protected fields + # below. assert result.client_name == "SECRET client" + assert result.client_version == "SECRET client version" + assert result.server_name == "SECRET server" + assert result.server_version == "SECRET server version" # ...and the original is untouched, so a failure downstream cannot # publish a half-redacted object. assert event.parameters == {"arguments": {"text": "SECRET body"}} @@ -438,6 +446,9 @@ def redact_fn(s: str) -> str: assert result.event_type == "mcp:tools/call" assert result.resource_name == "add_todo" assert result.client_name == "SECRET client" + assert result.client_version == "SECRET client version" + assert result.server_name == "SECRET server" + assert result.server_version == "SECRET server version" assert result.identify_actor_given_id == "SECRET actor" assert result.identify_data == {"email": "SECRET@example.com"} assert result.tags == {"env": "SECRET tag"} @@ -519,6 +530,24 @@ def hook(event): assert result is not None assert result.response is None + def test_apply_event_redaction_leaves_the_original_event_unmutated(self): + """The hook must be handed a copy, never the caller's own object — + the publish worker (and any other in-flight reference) still holds + the original, so a hook that mutates the event it receives (as in + test_hook_can_modify_the_event above) must not leave that mutation + visible there.""" + + def hook(event): + event.response = None + return event + + original = self._event() + apply_event_redaction(original, hook) + + assert original.response == { + "content": [{"type": "text", "text": "raw response"}] + } + def test_hook_returning_none_drops_the_event(self): def drop_get_credentials(event): if event.resource_name == "get_credentials": @@ -561,6 +590,10 @@ def forge(event): "identify_data", "tags", "properties", + "client_name", + "client_version", + "server_name", + "server_version", } def test_restored_fields_survive_actor_and_tag_forgery(self): @@ -593,6 +626,53 @@ def forge(event): assert result.tags == original.tags assert result.properties == original.properties + def test_restored_fields_survive_client_and_server_forgery(self): + """A hook cannot reassign which MCP client/server an event came from.""" + + def forge(event): + event.client_name = "forged-client" + event.client_version = "0.0.0-forged" + event.server_name = "forged-server" + event.server_version = "0.0.0-forged" + return event + + original = self._event( + client_name="real-client", + client_version="1.2.3", + server_name="real-server", + server_version="4.5.6", + ) + result = apply_event_redaction(original, forge) + + assert result.client_name == original.client_name + assert result.client_version == original.client_version + assert result.server_name == original.server_name + assert result.server_version == original.server_version + + def test_restored_dict_fields_are_copied_not_aliased(self): + """The restored tags/properties/identify_data must be independent + copies — mutating the result must not corrupt the original event a + publish worker or another in-flight event may still hold.""" + + original = self._event( + tags={"env": "prod"}, + properties={"plan": "premium"}, + identify_data={"email": "real@example.com"}, + ) + result = apply_event_redaction(original, lambda event: event) + + assert result.tags is not original.tags + assert result.properties is not original.properties + assert result.identify_data is not original.identify_data + + result.tags["env"] = "MUTATED" + result.properties["plan"] = "MUTATED" + result.identify_data["email"] = "MUTATED" + + assert original.tags == {"env": "prod"} + assert original.properties == {"plan": "premium"} + assert original.identify_data == {"email": "real@example.com"} + def test_a_raising_hook_propagates_so_the_queue_drops_the_event(self): def boom(_event): raise RuntimeError("event redaction exploded") @@ -600,6 +680,43 @@ def boom(_event): with pytest.raises(RuntimeError, match="event redaction exploded"): apply_event_redaction(self._event(), boom) + def test_hook_returning_a_non_event_value_raises(self): + """Documented fail-loud contract (see apply_event_redaction's and + RedactEventFunction's docstrings): the pydantic-Event return type is + enforced by type hints only, not at runtime, so a hook that ignores + its type hint and returns a plain dict must raise rather than + silently degrade to some fallback behavior.""" + + def hook(event): + return {**event.model_dump(), "response": None} + + with pytest.raises(AttributeError): + apply_event_redaction(self._event(), hook) + + def test_hook_returning_a_fresh_partial_event_nulls_unset_fields(self): + """Documented footgun (see apply_event_redaction's docstring): the + hook's return value replaces the event's fields wholesale — a full + model_dump(), not a diff against the original — so a hook that + returns a freshly-built Event with only some fields set, instead of + mutating and returning the one it was handed, silently nulls out + everything it didn't set.""" + from agentcat.types import Event as EventModel + + def hook(event): + return EventModel( + event_type=event.event_type, resource_name=event.resource_name + ) + + original = self._event() + result = apply_event_redaction(original, hook) + + assert result.resource_name == original.resource_name + assert result.user_intent is None + assert result.parameters is None + assert result.response is None + # ...while the original the hook was handed a dump of is untouched. + assert original.user_intent == "raw intent" + def test_an_async_hook_is_driven_to_completion(self): async def hook(event): event.user_intent = "async-modified" @@ -608,18 +725,43 @@ async def hook(event): result = apply_event_redaction(self._event(), hook) assert result.user_intent == "async-modified" + def test_an_async_hook_resolving_to_none_drops_the_event(self): + """The None-drop check must run against the AWAITED result, not the + coroutine object drive_hook_result is handed — a coroutine is always + truthy, so a check performed before awaiting would never drop.""" + + async def drop_it(event): + return None + + result = apply_event_redaction(self._event(), drop_it) + assert result is None + def test_hook_never_sees_the_function_fields(self): + """`Event` never declares redaction_fn/event_redaction_fn at all, so + `hasattr` on the hook's input is True regardless of whether + apply_event_redaction actually excludes them from the dump — it + would read False even if the exclusion were deleted. Patch in + `UnredactedEvent` (which DOES declare those fields) as the hook's + input type instead, so a real callable surviving the dump shows up + as a non-None attribute rather than a silently-absent one.""" + import agentcat.types as types_module + seen = {} def hook(event): - seen["has_redaction_fn"] = hasattr(event, "redaction_fn") - seen["has_event_redaction_fn"] = hasattr(event, "event_redaction_fn") + seen["redaction_fn"] = getattr(event, "redaction_fn", "MISSING") + seen["event_redaction_fn"] = getattr( + event, "event_redaction_fn", "MISSING" + ) return event event = self._event(redaction_fn=lambda s: s, event_redaction_fn=hook) - apply_event_redaction(event, hook) - assert seen["has_redaction_fn"] is False - assert seen["has_event_redaction_fn"] is False + + with patch.object(types_module, "Event", types_module.UnredactedEvent): + apply_event_redaction(event, hook) + + assert seen["redaction_fn"] is None + assert seen["event_redaction_fn"] is None def test_result_preserves_redaction_fn_for_the_string_hook_to_run_next(self): def string_redact_fn(s):