From b245a3c21fb1ef7e2d96c656d31d0a9047e9fa3d Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Mon, 7 Sep 2026 16:33:19 -0300 Subject: [PATCH 01/33] feat(mcp): capture resource discovery and reads --- .sampo/changesets/calm-resource-atlas.md | 5 ++ posthog/mcp/README.md | 3 +- posthog/mcp/__init__.py | 14 ++-- posthog/mcp/_instrument_fastmcp.py | 4 ++ posthog/mcp/_instrument_lowlevel.py | 86 ++++++++++++++++++++++++ posthog/mcp/_instrument_v2.py | 78 +++++++++++++++++++++ posthog/mcp/_instrumentation.py | 49 ++++++++++++-- posthog/test/mcp/test_lowlevel.py | 54 +++++++++++++++ posthog/test/mcp/test_v2_lowlevel.py | 78 ++++++++++++++++++++- 9 files changed, 358 insertions(+), 13 deletions(-) create mode 100644 .sampo/changesets/calm-resource-atlas.md diff --git a/.sampo/changesets/calm-resource-atlas.md b/.sampo/changesets/calm-resource-atlas.md new file mode 100644 index 000000000..b7b038885 --- /dev/null +++ b/.sampo/changesets/calm-resource-atlas.md @@ -0,0 +1,5 @@ +--- +pypi/posthog: minor +--- + +Capture MCP resource discovery and reads from instrumented servers. diff --git a/posthog/mcp/README.md b/posthog/mcp/README.md index a8dcd1020..f6504c46b 100644 --- a/posthog/mcp/README.md +++ b/posthog/mcp/README.md @@ -1,7 +1,8 @@ # PostHog MCP analytics Product analytics for Model Context Protocol servers. Wrap a Python MCP server so -every tool call, agent intent, and failure is captured to PostHog as a `$mcp_*` event. +tool calls, agent intent, resource discovery and reads, and failures are captured +to PostHog as `$mcp_*` events. ```python from posthog import Posthog diff --git a/posthog/mcp/__init__.py b/posthog/mcp/__init__.py index 3d9a58223..ea4e4dbd5 100644 --- a/posthog/mcp/__init__.py +++ b/posthog/mcp/__init__.py @@ -4,10 +4,10 @@ """PostHog MCP analytics SDK — product analytics for Model Context Protocol servers. -Wrap a Python MCP server so every tool call, agent intent, and failure is -captured to PostHog as a ``$mcp_*`` event. Works with the MCP Python SDK 1.x -*and* 2.x (the 2026-07-28 spec revision) — the high-level server class moved -between majors, but ``instrument()`` is the same:: +Wrap a Python MCP server so tool calls, agent intent, resource discovery and +reads, and failures are captured to PostHog as ``$mcp_*`` events. Works with +the MCP Python SDK 1.x *and* 2.x (the 2026-07-28 spec revision) — the high-level +server class moved between majors, but ``instrument()`` is the same:: from posthog import Posthog from posthog.mcp import instrument @@ -215,9 +215,9 @@ def instrument( posthog_client: Optional[Client] = None, options: Optional[MCPAnalyticsOptions] = None, ) -> McpAnalytics: - """Instrument an MCP server so PostHog auto-captures tool calls, tool listings, - initialize, identity, and exceptions. Returns a handle whose ``capture()`` - records custom events. + """Instrument an MCP server so PostHog auto-captures tool calls, tool and + resource listings, resource reads, initialize, identity, and exceptions. + Returns a handle whose ``capture()`` records custom events. Idempotent per server instance — a second call reuses the existing tracking state instead of double-wrapping. Degrades to a no-op handle on any failure so diff --git a/posthog/mcp/_instrument_fastmcp.py b/posthog/mcp/_instrument_fastmcp.py index e4d8a84d7..aadaa46ea 100644 --- a/posthog/mcp/_instrument_fastmcp.py +++ b/posthog/mcp/_instrument_fastmcp.py @@ -27,6 +27,7 @@ import mcp.types as mcp_types from ._conversation_id import build_prompt_back +from ._instrument_lowlevel import _wrap_resource_requests from ._instrumentation import ( _to_jsonable, append_get_more_tools, @@ -53,6 +54,9 @@ def instrument_fastmcp(server: Any, data: MCPAnalyticsData) -> None: data.server_version = getattr(getattr(server, "_mcp_server", None), "version", None) _wrap_tool_manager_call(server, data) _wrap_list_tools_handler(server, data) + low_level = getattr(server, "_mcp_server", None) + if low_level is not None: + _wrap_resource_requests(low_level, data) # --- tool call seam ---------------------------------------------------------- diff --git a/posthog/mcp/_instrument_lowlevel.py b/posthog/mcp/_instrument_lowlevel.py index cc1432554..a349d6c21 100644 --- a/posthog/mcp/_instrument_lowlevel.py +++ b/posthog/mcp/_instrument_lowlevel.py @@ -22,12 +22,15 @@ from ._context_parameters import schema_has_param from ._conversation_id import build_prompt_back +from ._event_types import MCPAnalyticsEventType from ._instrumentation import ( _to_jsonable, append_get_more_tools, collect_listed_tools, extract_tools, mutate_tool_schema, + prepare_request, + record_resource_request, request_to_dict, resolve_session_and_client, start_tool_call_lifecycle, @@ -49,6 +52,7 @@ def instrument_low_level(server: Any, data: MCPAnalyticsData) -> None: data.server_version = getattr(server, "version", None) _wrap_call_tool(server, data, strip_injected=False) _wrap_list_tools(server, data, context_required=False) + _wrap_resource_requests(server, data) def instrument_fastmcp_v2(server: Any, data: MCPAnalyticsData) -> None: @@ -72,6 +76,88 @@ def instrument_fastmcp_v2(server: Any, data: MCPAnalyticsData) -> None: # sees: under `FastMCP(strict_input_validation=True)` every call fails with # "'context' is a required property". _wrap_list_tools(low_level, data, context_required=False) + _wrap_resource_requests(low_level, data) + + +def _wrap_resource_requests(server: Any, data: MCPAnalyticsData) -> None: + for request_type, event_type in ( + (mcp_types.ListResourcesRequest, MCPAnalyticsEventType.MCP_RESOURCES_LIST), + (mcp_types.ReadResourceRequest, MCPAnalyticsEventType.MCP_RESOURCES_READ), + ): + _wrap_resource_request(server, data, request_type, event_type) + + +def _wrap_resource_request( + server: Any, + data: MCPAnalyticsData, + request_type: Any, + event_type: str, +) -> None: + handlers = server.request_handlers + original = handlers.get(request_type) + if original is None or getattr(original, _WRAPPED_FLAG, False): + return + + async def handler(req: Any) -> Any: + client_name, client_version = _client_info(server) + protocol_version = _protocol_version(server) + mcp_session_id = _mcp_session_id(server) + token, client_name, client_version, protocol_version = ( + resolve_session_and_client( + mcp_session_id, client_name, client_version, protocol_version + ) + ) + request = request_to_dict(req) + extra = {"session_id": mcp_session_id, "ctx": _request_context(server)} + try: + session_id = await prepare_request( + data, + mcp_session_id=mcp_session_id, + client_name=client_name, + client_version=client_version, + protocol_version=protocol_version, + request=request, + extra=extra, + token=token, + ) + except Exception as error: # noqa: BLE001 - analytics must not break resources + log(f"Warning: could not prepare resource analytics: {error}") + return await original(req) + + start = time.monotonic() + try: + result = await original(req) + except Exception as error: + await record_resource_request( + data, + session_id, + event_type=event_type, + request=request, + error=error, + duration_ms=(time.monotonic() - start) * 1000, + client_name=client_name, + client_version=client_version, + protocol_version=protocol_version, + extra=extra, + ) + raise + + await record_resource_request( + data, + session_id, + event_type=event_type, + request=request, + response=result, + duration_ms=(time.monotonic() - start) * 1000, + client_name=client_name, + client_version=client_version, + protocol_version=protocol_version, + extra=extra, + ) + return result + + setattr(handler, _WRAPPED_FLAG, True) + handlers[request_type] = handler def _wrap_call_tool( diff --git a/posthog/mcp/_instrument_v2.py b/posthog/mcp/_instrument_v2.py index 437015bee..84d95b8a8 100644 --- a/posthog/mcp/_instrument_v2.py +++ b/posthog/mcp/_instrument_v2.py @@ -37,11 +37,14 @@ from ._context_parameters import schema_has_param from ._conversation_id import build_prompt_back +from ._event_types import MCPAnalyticsEventType from ._instrumentation import ( _to_jsonable, collect_listed_tools, mutate_tool_schema, params_to_request_dict, + prepare_request, + record_resource_request, resolve_session_and_client, start_tool_call_lifecycle, start_tools_list_lifecycle, @@ -63,6 +66,10 @@ # injected `context` parameter per entry point (see _wrap_v2_list_tools). _CALL_METHOD = "tools/call" _LIST_METHOD = "tools/list" +_RESOURCE_METHODS = { + "resources/list": MCPAnalyticsEventType.MCP_RESOURCES_LIST, + "resources/read": MCPAnalyticsEventType.MCP_RESOURCES_READ, +} def instrument_mcpserver_v2(server: Any, data: MCPAnalyticsData) -> None: @@ -81,6 +88,8 @@ def instrument_mcpserver_v2(server: Any, data: MCPAnalyticsData) -> None: ) _wrap_tool_manager_call_v2(server, data) _wrap_v2_list_tools(low_level, data, context_required=True, high_level=server) + for method, event_type in _RESOURCE_METHODS.items(): + _wrap_v2_resource_request(low_level, data, method, event_type) _patch_add_request_handler(low_level, data, wrap_call=False, high_level=server) @@ -93,6 +102,8 @@ def instrument_lowlevel_v2(server: Any, data: MCPAnalyticsData) -> None: data.server_version = getattr(server, "version", None) _wrap_v2_call_tool(server, data) _wrap_v2_list_tools(server, data, context_required=False) + for method, event_type in _RESOURCE_METHODS.items(): + _wrap_v2_resource_request(server, data, method, event_type) _patch_add_request_handler(server, data, wrap_call=True) @@ -127,6 +138,8 @@ def add_request_handler(method: str, params_type: Any, handler: Any) -> None: context_required=high_level is not None, high_level=high_level, ) + elif method in _RESOURCE_METHODS: + _wrap_v2_resource_request(server, data, method, _RESOURCE_METHODS[method]) setattr(add_request_handler, _WRAPPED_FLAG, True) server.add_request_handler = add_request_handler @@ -440,6 +453,71 @@ async def handler(ctx: Any, params: Any) -> Any: _replace_handler(server, _CALL_METHOD, handler, entry.params_type) +def _wrap_v2_resource_request( + server: Any, data: MCPAnalyticsData, method: str, event_type: str +) -> None: + entry = server.get_request_handler(method) + if entry is None or getattr(entry.handler, _WRAPPED_FLAG, False): + return + original = entry.handler + + async def handler(ctx: Any, params: Any) -> Any: + token, client_name, client_version, protocol_version, mcp_session_id = ( + _resolve_ctx(ctx) + ) + request = params_to_request_dict(method, params, by_alias=True) + extra: Dict[str, Any] = {"session_id": mcp_session_id, "ctx": ctx} + try: + session_id = await prepare_request( + data, + mcp_session_id=mcp_session_id, + client_name=client_name, + client_version=client_version, + protocol_version=protocol_version, + request=request, + extra=extra, + token=token, + ) + except Exception as error: # noqa: BLE001 - analytics must not break resources + log(f"Warning: could not prepare resource analytics: {error}") + return await original(ctx, params) + + start = time.monotonic() + try: + result = await original(ctx, params) + except Exception as error: + await record_resource_request( + data, + session_id, + event_type=event_type, + request=request, + error=error, + duration_ms=(time.monotonic() - start) * 1000, + client_name=client_name, + client_version=client_version, + protocol_version=protocol_version, + extra=extra, + ) + raise + + await record_resource_request( + data, + session_id, + event_type=event_type, + request=request, + response=result, + duration_ms=(time.monotonic() - start) * 1000, + client_name=client_name, + client_version=client_version, + protocol_version=protocol_version, + extra=extra, + ) + return result + + setattr(handler, _WRAPPED_FLAG, True) + _replace_handler(server, method, handler, entry.params_type) + + # --- tools/list ------------------------------------------------------------------- diff --git a/posthog/mcp/_instrumentation.py b/posthog/mcp/_instrumentation.py index b9c052b0f..afb62ccc7 100644 --- a/posthog/mcp/_instrumentation.py +++ b/posthog/mcp/_instrumentation.py @@ -2,10 +2,9 @@ # Copyright (c) 2025 MCPcat # Licensed under the MIT License: https://github.com/MCPCat/mcpcat-typescript-sdk/blob/main/LICENSE -"""Shared tool-call / tools-list / initialize lifecycle used by both the FastMCP -and low-level server adapters. The adapters resolve transport-specific details -(client info, session id, raw result shape) and delegate the analytics flow here -so both stay in sync.""" +"""Shared MCP request lifecycles used by both the FastMCP and low-level server +adapters. The adapters resolve transport-specific details (client info, session +id, raw result shape) and delegate analytics policy here so both stay in sync.""" from __future__ import annotations @@ -834,3 +833,45 @@ async def record_tools_list( fire_and_forget(capture_event(data, event), data) except Exception as err: # noqa: BLE001 - isolate analytics from the tool path log(f"record_tools_list failed (event dropped): {err}") + + +async def record_resource_request( + data: MCPAnalyticsData, + session_id: str, + *, + event_type: str, + request: Dict[str, Any], + response: Any = None, + error: Any = None, + duration_ms: Optional[float] = None, + client_name: Optional[str] = None, + client_version: Optional[str] = None, + protocol_version: Optional[str] = None, + extra: Optional[Dict[str, Any]] = None, +) -> None: + """Record resources/list or resources/read without affecting dispatch.""" + try: + params = request.get("params") + uri = params.get("uri") if isinstance(params, dict) else None + event: Dict[str, Any] = { + "event_type": event_type, + "session_id": session_id, + "resource_name": uri + if event_type == MCPAnalyticsEventType.MCP_RESOURCES_READ + else None, + "parameters": build_captured_mcp_parameters(request), + "response": _wrap_response(response) if response is not None else None, + "duration": duration_ms, + "client_name": client_name, + "client_version": client_version, + "protocol_version": protocol_version, + "is_error": error is not None, + "timestamp": datetime.now(timezone.utc), + } + if error is not None: + event["error"] = capture_exception(error) + await _apply_event_properties(data, event, request, extra) + stamp_transport_identity(event, extra) + fire_and_forget(capture_event(data, event), data) + except Exception as err: # noqa: BLE001 - isolate analytics from the request path + log(f"record_resource_request failed (event dropped): {err}") diff --git a/posthog/test/mcp/test_lowlevel.py b/posthog/test/mcp/test_lowlevel.py index eb50ff1e1..f6c557183 100644 --- a/posthog/test/mcp/test_lowlevel.py +++ b/posthog/test/mcp/test_lowlevel.py @@ -34,6 +34,35 @@ async def call_tool(name, arguments): return [mcp_types.TextContent(type="text", text=str(arguments.get("msg")))] raise ValueError("boom") + async def list_resources(_request): + return mcp_types.ServerResult( + mcp_types.ListResourcesResult( + resources=[ + mcp_types.Resource( + name="Guide", + uri="file:///guide.md", + mimeType="text/markdown", + ) + ] + ) + ) + + async def read_resource(request): + return mcp_types.ServerResult( + mcp_types.ReadResourceResult( + contents=[ + mcp_types.TextResourceContents( + uri=request.params.uri, + mimeType="text/markdown", + text="# Guide", + ) + ] + ) + ) + + server.request_handlers[mcp_types.ListResourcesRequest] = list_resources + server.request_handlers[mcp_types.ReadResourceRequest] = read_resource + return server @@ -62,6 +91,31 @@ async def test_list_tools_injects_optional_context_and_captures(): assert listed and listed[0]["properties"]["$mcp_listed_tool_names"] == ["echo"] +async def test_resource_discovery_and_read_are_captured(): + server = make_server() + client = FakeClient() + instrument(server, client) + + list_handler = server.request_handlers[mcp_types.ListResourcesRequest] + await list_handler(mcp_types.ListResourcesRequest()) + read_handler = server.request_handlers[mcp_types.ReadResourceRequest] + result = await read_handler( + mcp_types.ReadResourceRequest( + params=mcp_types.ReadResourceRequestParams(uri="file:///guide.md") + ) + ) + await _flush() + + assert result.root.contents[0].text == "# Guide" + listed = _events(client, "$mcp_resources_list") + assert len(listed) == 1 + read = _events(client, "$mcp_resource_read") + assert len(read) == 1 + assert read[0]["properties"]["$mcp_resource_name"] == "file:///guide.md" + assert read[0]["properties"]["$mcp_is_error"] is False + assert read[0]["properties"]["$mcp_response"]["contents"][0]["text"] == "# Guide" + + async def test_tool_call_success_captures_intent(): server = make_server() client = FakeClient() diff --git a/posthog/test/mcp/test_v2_lowlevel.py b/posthog/test/mcp/test_v2_lowlevel.py index eb543f86f..23bb0b336 100644 --- a/posthog/test/mcp/test_v2_lowlevel.py +++ b/posthog/test/mcp/test_v2_lowlevel.py @@ -58,13 +58,41 @@ async def on_list_tools(ctx, params): ] ) - return Server( + server = Server( "test-low-v2", version="1.2.3", on_call_tool=on_call_tool, on_list_tools=on_list_tools, ) + async def on_list_resources(ctx, params): + return mcp_types.ListResourcesResult( + resources=[ + mcp_types.Resource( + name="Guide", uri="file:///guide.md", mime_type="text/markdown" + ) + ] + ) + + async def on_read_resource(ctx, params): + return mcp_types.ReadResourceResult( + contents=[ + mcp_types.TextResourceContents( + uri=params.uri, + mime_type="text/markdown", + text="# Guide", + ) + ] + ) + + server.add_request_handler( + "resources/list", mcp_types.PaginatedRequestParams, on_list_resources + ) + server.add_request_handler( + "resources/read", mcp_types.ReadResourceRequestParams, on_read_resource + ) + return server + async def _call_tool(server, name, arguments, ctx=None): entry = server.get_request_handler("tools/call") @@ -77,6 +105,11 @@ async def _list_tools(server, ctx=None): return await entry.handler(ctx or fake_ctx(method="tools/list"), None) +async def _resource_request(server, method, params=None, ctx=None): + entry = server.get_request_handler(method) + return await entry.handler(ctx or fake_ctx(method=method), params) + + # --- tools/list -------------------------------------------------------------- @@ -100,6 +133,29 @@ async def test_list_tools_injects_optional_context_and_captures(): assert listed[0]["properties"]["$mcp_server_name"] == "test-low-v2" +async def test_resource_discovery_and_read_are_captured(): + server = make_server() + client = FakeClient() + instrument(server, client) + + await _resource_request(server, "resources/list") + result = await _resource_request( + server, + "resources/read", + mcp_types.ReadResourceRequestParams(uri="file:///guide.md"), + ) + await _flush() + + assert result.contents[0].text == "# Guide" + listed = _events(client, "$mcp_resources_list") + assert len(listed) == 1 + read = _events(client, "$mcp_resource_read") + assert len(read) == 1 + assert read[0]["properties"]["$mcp_resource_name"] == "file:///guide.md" + assert read[0]["properties"]["$mcp_protocol_version"] == "2026-07-28" + assert read[0]["properties"]["$mcp_response"]["contents"][0]["text"] == "# Guide" + + # --- tools/call -------------------------------------------------------------- @@ -200,13 +256,33 @@ async def late_call_tool(ctx, params): "tools/call", mcp_types.CallToolRequestParams, late_call_tool ) + async def late_read_resource(ctx, params): + return mcp_types.ReadResourceResult( + contents=[ + mcp_types.TextResourceContents(uri=params.uri, text="late resource") + ] + ) + + server.add_request_handler( + "resources/read", mcp_types.ReadResourceRequestParams, late_read_resource + ) + result = await _call_tool(server, "anything", {"context": "late registration"}) + resource = await _resource_request( + server, + "resources/read", + mcp_types.ReadResourceRequestParams(uri="file:///late.txt"), + ) await _flush() assert result.content[0].text == "late ok" + assert resource.contents[0].text == "late resource" calls = _events(client, "$mcp_tool_call") assert len(calls) == 1 assert calls[0]["properties"]["$mcp_tool_name"] == "anything" + reads = _events(client, "$mcp_resource_read") + assert len(reads) == 1 + assert reads[0]["properties"]["$mcp_resource_name"] == "file:///late.txt" async def test_initialize_and_session_reuse_across_calls(): From a341587a545e1a47d35962e203b0e52e8910c65f Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Mon, 7 Sep 2026 16:54:39 -0300 Subject: [PATCH 02/33] fix(mcp): keep resource bodies out of analytics --- posthog/mcp/_instrument_lowlevel.py | 1 - posthog/mcp/_instrument_v2.py | 1 - posthog/mcp/_instrumentation.py | 2 -- posthog/test/mcp/test_lowlevel.py | 2 +- posthog/test/mcp/test_v2_lowlevel.py | 2 +- 5 files changed, 2 insertions(+), 6 deletions(-) diff --git a/posthog/mcp/_instrument_lowlevel.py b/posthog/mcp/_instrument_lowlevel.py index a349d6c21..e4eb36238 100644 --- a/posthog/mcp/_instrument_lowlevel.py +++ b/posthog/mcp/_instrument_lowlevel.py @@ -147,7 +147,6 @@ async def handler(req: Any) -> Any: session_id, event_type=event_type, request=request, - response=result, duration_ms=(time.monotonic() - start) * 1000, client_name=client_name, client_version=client_version, diff --git a/posthog/mcp/_instrument_v2.py b/posthog/mcp/_instrument_v2.py index 84d95b8a8..411d2329a 100644 --- a/posthog/mcp/_instrument_v2.py +++ b/posthog/mcp/_instrument_v2.py @@ -505,7 +505,6 @@ async def handler(ctx: Any, params: Any) -> Any: session_id, event_type=event_type, request=request, - response=result, duration_ms=(time.monotonic() - start) * 1000, client_name=client_name, client_version=client_version, diff --git a/posthog/mcp/_instrumentation.py b/posthog/mcp/_instrumentation.py index afb62ccc7..b8ef4d2a0 100644 --- a/posthog/mcp/_instrumentation.py +++ b/posthog/mcp/_instrumentation.py @@ -841,7 +841,6 @@ async def record_resource_request( *, event_type: str, request: Dict[str, Any], - response: Any = None, error: Any = None, duration_ms: Optional[float] = None, client_name: Optional[str] = None, @@ -860,7 +859,6 @@ async def record_resource_request( if event_type == MCPAnalyticsEventType.MCP_RESOURCES_READ else None, "parameters": build_captured_mcp_parameters(request), - "response": _wrap_response(response) if response is not None else None, "duration": duration_ms, "client_name": client_name, "client_version": client_version, diff --git a/posthog/test/mcp/test_lowlevel.py b/posthog/test/mcp/test_lowlevel.py index f6c557183..d282ff7bb 100644 --- a/posthog/test/mcp/test_lowlevel.py +++ b/posthog/test/mcp/test_lowlevel.py @@ -113,7 +113,7 @@ async def test_resource_discovery_and_read_are_captured(): assert len(read) == 1 assert read[0]["properties"]["$mcp_resource_name"] == "file:///guide.md" assert read[0]["properties"]["$mcp_is_error"] is False - assert read[0]["properties"]["$mcp_response"]["contents"][0]["text"] == "# Guide" + assert "$mcp_response" not in read[0]["properties"] async def test_tool_call_success_captures_intent(): diff --git a/posthog/test/mcp/test_v2_lowlevel.py b/posthog/test/mcp/test_v2_lowlevel.py index 23bb0b336..1fa15cca3 100644 --- a/posthog/test/mcp/test_v2_lowlevel.py +++ b/posthog/test/mcp/test_v2_lowlevel.py @@ -153,7 +153,7 @@ async def test_resource_discovery_and_read_are_captured(): assert len(read) == 1 assert read[0]["properties"]["$mcp_resource_name"] == "file:///guide.md" assert read[0]["properties"]["$mcp_protocol_version"] == "2026-07-28" - assert read[0]["properties"]["$mcp_response"]["contents"][0]["text"] == "# Guide" + assert "$mcp_response" not in read[0]["properties"] # --- tools/call -------------------------------------------------------------- From 62caaf165743df8de0dc4a9c5a3f27fddd802e48 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Tue, 8 Sep 2026 19:03:23 -0300 Subject: [PATCH 03/33] fix(mcp): redact credentials in captured resource addresses Apply existing credential redaction to resource-read names before the primary event and exception sibling are built. Preserve the original URI and resource result or exception received by the caller. Extend the existing resource tests with successful and failing reads containing an invented token; the two new cases fail before this fix under each MCP major. Document the capture boundary and before_send. Validation: MCP v1 245 passed; MCP v2 225 passed and 13 expected skips. Ruff lint and formatting pass. Mypy baseline passes (227 source files). --- posthog/mcp/README.md | 5 ++ posthog/mcp/_sanitization.py | 6 +++ posthog/test/mcp/test_lowlevel.py | 72 +++++++++++++++++++++------- posthog/test/mcp/test_v2_lowlevel.py | 64 +++++++++++++++++++------ 4 files changed, 114 insertions(+), 33 deletions(-) diff --git a/posthog/mcp/README.md b/posthog/mcp/README.md index f6504c46b..252d665a9 100644 --- a/posthog/mcp/README.md +++ b/posthog/mcp/README.md @@ -4,6 +4,11 @@ Product analytics for Model Context Protocol servers. Wrap a Python MCP server s tool calls, agent intent, resource discovery and reads, and failures are captured to PostHog as `$mcp_*` events. +Resource bodies are not captured. Resource addresses use the same credential +redaction as request parameters, including on failed reads. Requests and responses +keep their original addresses. Use `before_send` to remove any additional +application-specific sensitive data. + ```python from posthog import Posthog from posthog.mcp import instrument diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index fd547f418..06ad641cb 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -13,6 +13,8 @@ import re from typing import Any, Dict +from ._event_types import MCPAnalyticsEventType + # SDK-injected arguments stripped from captured $mcp_parameters (they surface as # dedicated properties: $mcp_intent and $mcp_conversation_id). _INJECTED_ARGUMENT_NAMES = ("context", "conversation_id") @@ -105,6 +107,10 @@ def sanitize_event(event: Dict[str, Any]) -> Dict[str, Any]: if result.get("parameters") is not None: result["parameters"] = sanitize_captured_value(result["parameters"]) + if result.get("event_type") == MCPAnalyticsEventType.MCP_RESOURCES_READ: + if result.get("resource_name") is not None: + result["resource_name"] = sanitize_captured_value(result["resource_name"]) + # The intent comes straight from an agent-narrated `context` string, so it # can contain a secret the LLM read aloud. Redact it like any other value. if result.get("user_intent") is not None: diff --git a/posthog/test/mcp/test_lowlevel.py b/posthog/test/mcp/test_lowlevel.py index d282ff7bb..821adb62f 100644 --- a/posthog/test/mcp/test_lowlevel.py +++ b/posthog/test/mcp/test_lowlevel.py @@ -1,5 +1,9 @@ """End-to-end tests for the low-level mcp.server.Server adapter (Milestone 3).""" +import json + +import pytest + import mcp.types as mcp_types from mcp.server.lowlevel import Server @@ -11,7 +15,7 @@ ) -def make_server(): +def make_server(*, resource_error: bool = False) -> Server: server = Server("test-lowlevel") @server.list_tools() @@ -48,6 +52,8 @@ async def list_resources(_request): ) async def read_resource(request): + if resource_error: + raise ValueError(f"Cannot read {request.params.uri}") return mcp_types.ServerResult( mcp_types.ReadResourceResult( contents=[ @@ -91,29 +97,59 @@ async def test_list_tools_injects_optional_context_and_captures(): assert listed and listed[0]["properties"]["$mcp_listed_tool_names"] == ["echo"] -async def test_resource_discovery_and_read_are_captured(): - server = make_server() +@pytest.mark.parametrize( + "uri, captured_uri, resource_error", + [ + ("file:///guide.md", "file:///guide.md", False), + ( + "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", + "https://example.com/guide?token=[redacted]", + False, + ), + ( + "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", + "https://example.com/guide?token=[redacted]", + True, + ), + ], +) +async def test_resource_discovery_and_read_are_captured( + uri: str, captured_uri: str, resource_error: bool +) -> None: + server = make_server(resource_error=resource_error) client = FakeClient() instrument(server, client) - list_handler = server.request_handlers[mcp_types.ListResourcesRequest] - await list_handler(mcp_types.ListResourcesRequest()) - read_handler = server.request_handlers[mcp_types.ReadResourceRequest] - result = await read_handler( - mcp_types.ReadResourceRequest( - params=mcp_types.ReadResourceRequestParams(uri="file:///guide.md") - ) + await server.request_handlers[mcp_types.ListResourcesRequest]( + mcp_types.ListResourcesRequest() + ) + request = mcp_types.ReadResourceRequest( + params=mcp_types.ReadResourceRequestParams(uri=uri) ) + read = server.request_handlers[mcp_types.ReadResourceRequest](request) + if resource_error: + with pytest.raises(ValueError) as caught: + await read + assert str(caught.value) == f"Cannot read {uri}" + else: + result = await read + assert result.root.contents[0].text == "# Guide" + assert str(result.root.contents[0].uri) == uri await _flush() - assert result.root.contents[0].text == "# Guide" - listed = _events(client, "$mcp_resources_list") - assert len(listed) == 1 - read = _events(client, "$mcp_resource_read") - assert len(read) == 1 - assert read[0]["properties"]["$mcp_resource_name"] == "file:///guide.md" - assert read[0]["properties"]["$mcp_is_error"] is False - assert "$mcp_response" not in read[0]["properties"] + assert len(_events(client, "$mcp_resources_list")) == 1 + reads = _events(client, "$mcp_resource_read") + assert len(reads) == 1 + props = reads[0]["properties"] + assert props["$mcp_resource_name"] == captured_uri + assert props["$mcp_parameters"]["request"]["params"]["uri"] == captured_uri + assert props["$mcp_is_error"] is resource_error + assert "$mcp_response" not in props + exceptions = _events(client, "$exception") + assert len(exceptions) == int(resource_error) + if resource_error: + assert exceptions[0]["properties"]["$mcp_resource_name"] == captured_uri + assert "phx_EXAMPLEONLYFAKEVALUE00000000000" not in json.dumps(client.events) async def test_tool_call_success_captures_intent(): diff --git a/posthog/test/mcp/test_v2_lowlevel.py b/posthog/test/mcp/test_v2_lowlevel.py index 1fa15cca3..f239a44a5 100644 --- a/posthog/test/mcp/test_v2_lowlevel.py +++ b/posthog/test/mcp/test_v2_lowlevel.py @@ -7,6 +7,8 @@ converting to ``is_error`` results. """ +import json + import pytest import mcp.types as mcp_types @@ -22,7 +24,7 @@ from posthog.test.mcp._helpers_v2 import fake_ctx -def make_server(): +def make_server(*, resource_error: bool = False) -> Server: async def on_call_tool(ctx, params): if params.name == "boom": raise ValueError("explode") @@ -75,6 +77,8 @@ async def on_list_resources(ctx, params): ) async def on_read_resource(ctx, params): + if resource_error: + raise ValueError(f"Cannot read {params.uri}") return mcp_types.ReadResourceResult( contents=[ mcp_types.TextResourceContents( @@ -133,27 +137,57 @@ async def test_list_tools_injects_optional_context_and_captures(): assert listed[0]["properties"]["$mcp_server_name"] == "test-low-v2" -async def test_resource_discovery_and_read_are_captured(): - server = make_server() +@pytest.mark.parametrize( + "uri, captured_uri, resource_error", + [ + ("file:///guide.md", "file:///guide.md", False), + ( + "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", + "https://example.com/guide?token=[redacted]", + False, + ), + ( + "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", + "https://example.com/guide?token=[redacted]", + True, + ), + ], +) +async def test_resource_discovery_and_read_are_captured( + uri: str, captured_uri: str, resource_error: bool +) -> None: + server = make_server(resource_error=resource_error) client = FakeClient() instrument(server, client) await _resource_request(server, "resources/list") - result = await _resource_request( - server, - "resources/read", - mcp_types.ReadResourceRequestParams(uri="file:///guide.md"), + read = _resource_request( + server, "resources/read", mcp_types.ReadResourceRequestParams(uri=uri) ) + if resource_error: + with pytest.raises(ValueError) as caught: + await read + assert str(caught.value) == f"Cannot read {uri}" + else: + result = await read + assert result.contents[0].text == "# Guide" + assert str(result.contents[0].uri) == uri await _flush() - assert result.contents[0].text == "# Guide" - listed = _events(client, "$mcp_resources_list") - assert len(listed) == 1 - read = _events(client, "$mcp_resource_read") - assert len(read) == 1 - assert read[0]["properties"]["$mcp_resource_name"] == "file:///guide.md" - assert read[0]["properties"]["$mcp_protocol_version"] == "2026-07-28" - assert "$mcp_response" not in read[0]["properties"] + assert len(_events(client, "$mcp_resources_list")) == 1 + reads = _events(client, "$mcp_resource_read") + assert len(reads) == 1 + props = reads[0]["properties"] + assert props["$mcp_resource_name"] == captured_uri + assert props["$mcp_parameters"]["request"]["params"]["uri"] == captured_uri + assert props["$mcp_is_error"] is resource_error + assert "$mcp_response" not in props + exceptions = _events(client, "$exception") + assert len(exceptions) == int(resource_error) + if resource_error: + assert exceptions[0]["properties"]["$mcp_resource_name"] == captured_uri + assert "phx_EXAMPLEONLYFAKEVALUE00000000000" not in json.dumps(client.events) + assert props["$mcp_protocol_version"] == "2026-07-28" # --- tools/call -------------------------------------------------------------- From 6dea4debebd9c0f7802e079eb610984a590d1b06 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Tue, 8 Sep 2026 19:43:40 -0300 Subject: [PATCH 04/33] fix(mcp): redact credentials embedded in captured URLs Parse captured URLs to remove userinfo and credential query values, including common signed URL fields. Apply the same sanitization to URLs inside exception messages without changing handler requests or responses. Document the limits of key-based URL redaction. Verification: reproduced the credential leak before the fix. MCP v1 suite: 260 passed; v2 suite: 240 passed, 13 skipped. Ruff check and format passed; mypy baseline passed for 227 files. Regression coverage includes encoded keys, duplicate query parameters, malformed URLs, and success/error events. --- posthog/mcp/README.md | 10 +++-- posthog/mcp/_sanitization.py | 33 +++++++++++++++++ posthog/test/mcp/test_lowlevel.py | 55 ++++++++++++++++++++++++++-- posthog/test/mcp/test_pipeline.py | 37 +++++++++++++++++++ posthog/test/mcp/test_v2_lowlevel.py | 55 ++++++++++++++++++++++++++-- 5 files changed, 180 insertions(+), 10 deletions(-) diff --git a/posthog/mcp/README.md b/posthog/mcp/README.md index 252d665a9..7366ceba2 100644 --- a/posthog/mcp/README.md +++ b/posthog/mcp/README.md @@ -4,10 +4,12 @@ Product analytics for Model Context Protocol servers. Wrap a Python MCP server s tool calls, agent intent, resource discovery and reads, and failures are captured to PostHog as `$mcp_*` events. -Resource bodies are not captured. Resource addresses use the same credential -redaction as request parameters, including on failed reads. Requests and responses -keep their original addresses. Use `before_send` to remove any additional -application-specific sensitive data. +Resource bodies are not captured. Captured URLs redact usernames, passwords, and +known credential query parameters, including signed URL credentials. This also +applies when a failed read repeats the URL in its error message. Other query +parameters and fragments can still contain application-specific sensitive data. +Requests and responses keep their original addresses. Use `before_send` to remove +any additional application-specific sensitive data. ```python from posthog import Posthog diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 06ad641cb..21f02e84f 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -12,6 +12,7 @@ import re from typing import Any, Dict +from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit from ._event_types import MCPAnalyticsEventType @@ -29,6 +30,37 @@ re.IGNORECASE, ) +_URL_PATTERN = re.compile(r"\b[a-z][a-z0-9+.-]{0,63}://[^\s<>\"']+", re.IGNORECASE) +_SENSITIVE_QUERY_KEY_PATTERN = re.compile( + r"^(auth|key|credential|signature|sig|AWSAccessKeyId|GoogleAccessId|" + r"Policy|Key-Pair-Id|X-Amz-(Credential|Signature|Security-Token)|" + r"X-Goog-(Credential|Signature))$", + re.IGNORECASE, +) + + +def _sanitize_url(match: re.Match[str]) -> str: + value = match.group(0) + try: + url = urlsplit(value) + query = parse_qsl(url.query, keep_blank_values=True) + sanitized_query = [ + (key, _REDACTED_VALUE) + if _should_redact_key(key) or _SENSITIVE_QUERY_KEY_PATTERN.fullmatch(key) + else (key, item) + for key, item in query + ] + netloc = url.netloc + if "@" in netloc: + netloc = "%5Bredacted%5D@" + netloc.rsplit("@", 1)[1] + if netloc == url.netloc and sanitized_query == query: + return value + return urlunsplit( + (url.scheme, netloc, url.path, urlencode(sanitized_query), url.fragment) + ) + except ValueError: + return _REDACTED_VALUE + def _is_record(value: Any) -> bool: return isinstance(value, dict) @@ -41,6 +73,7 @@ def _should_redact_key(key: str) -> bool: def _sanitize_string(value: str) -> str: if len(value) >= _SIZE_GATE and _BASE64_PATTERN.match(value): return "[binary data redacted - not supported by PostHog MCP analytics]" + value = _URL_PATTERN.sub(_sanitize_url, value) return _redact_secret_tokens(_POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value)) diff --git a/posthog/test/mcp/test_lowlevel.py b/posthog/test/mcp/test_lowlevel.py index 821adb62f..abf0754d4 100644 --- a/posthog/test/mcp/test_lowlevel.py +++ b/posthog/test/mcp/test_lowlevel.py @@ -101,14 +101,54 @@ async def test_list_tools_injects_optional_context_and_captures(): "uri, captured_uri, resource_error", [ ("file:///guide.md", "file:///guide.md", False), + ( + "https://fakeuser:fakepass@example.com/guide", + "https://%5Bredacted%5D@example.com/guide", + False, + ), + ( + "https://fakeuser:fakepass@example.com/guide", + "https://%5Bredacted%5D@example.com/guide", + True, + ), + ( + "https://example.com/guide?token=fakesecret&chapter=intro", + "https://example.com/guide?token=%5Bredacted%5D&chapter=intro", + False, + ), + ( + "https://example.com/guide?token=fakesecret&chapter=intro", + "https://example.com/guide?token=%5Bredacted%5D&chapter=intro", + True, + ), + ( + "https://example.com/guide?access_token=fakeaccess&X-Amz-Credential=fakecredential&X-Amz-Signature=fakesignature", + "https://example.com/guide?access_token=%5Bredacted%5D&X-Amz-Credential=%5Bredacted%5D&X-Amz-Signature=%5Bredacted%5D", + False, + ), + ( + "https://example.com/guide?access_token=fakeaccess&X-Amz-Credential=fakecredential&X-Amz-Signature=fakesignature", + "https://example.com/guide?access_token=%5Bredacted%5D&X-Amz-Credential=%5Bredacted%5D&X-Amz-Signature=%5Bredacted%5D", + True, + ), + ( + "ui://guide/page?%74oken=fakesecret&TOKEN=fakeaccess&chapter=intro#section", + "ui://guide/page?token=%5Bredacted%5D&TOKEN=%5Bredacted%5D&chapter=intro#section", + False, + ), + ( + "ui://guide/page?%74oken=fakesecret&TOKEN=fakeaccess&chapter=intro#section", + "ui://guide/page?token=%5Bredacted%5D&TOKEN=%5Bredacted%5D&chapter=intro#section", + True, + ), ( "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", - "https://example.com/guide?token=[redacted]", + "https://example.com/guide?token=%5Bredacted%5D", False, ), ( "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", - "https://example.com/guide?token=[redacted]", + "https://example.com/guide?token=%5Bredacted%5D", True, ), ], @@ -149,7 +189,16 @@ async def test_resource_discovery_and_read_are_captured( assert len(exceptions) == int(resource_error) if resource_error: assert exceptions[0]["properties"]["$mcp_resource_name"] == captured_uri - assert "phx_EXAMPLEONLYFAKEVALUE00000000000" not in json.dumps(client.events) + for secret in ( + "phx_EXAMPLEONLYFAKEVALUE00000000000", + "fakeuser", + "fakepass", + "fakesecret", + "fakeaccess", + "fakecredential", + "fakesignature", + ): + assert secret not in json.dumps(client.events) async def test_tool_call_success_captures_intent(): diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index f57c1572b..fbdc7f87c 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -2,6 +2,8 @@ from datetime import datetime, timezone +import pytest + from posthog.mcp.constants import ( POSTHOG_MCP_ANALYTICS_SOURCE, PostHogMCPAnalyticsEvent, @@ -75,6 +77,41 @@ def test_sanitize_redacts_large_base64(): assert sanitize_captured_value(blob).startswith("[binary data redacted") +@pytest.mark.parametrize( + "value, expected", + [ + ( + "https://example.com/guide?token=fakesecret&token=fakeaccess&empty=", + "https://example.com/guide?token=%5Bredacted%5D&token=%5Bredacted%5D&empty=", + ), + ( + "https://example.com/guide?X-Goog-Credential=fakecredential&X-Goog-Signature=fakesignature", + "https://example.com/guide?X-Goog-Credential=%5Bredacted%5D&X-Goog-Signature=%5Bredacted%5D", + ), + ( + "https://example.com/guide?sig=fakesignature&Signature=fakesignature&X-Amz-Security-Token=fakesecret", + "https://example.com/guide?sig=%5Bredacted%5D&Signature=%5Bredacted%5D&X-Amz-Security-Token=%5Bredacted%5D", + ), + ( + "https://fakeuser@example.com/guide", + "https://%5Bredacted%5D@example.com/guide", + ), + ( + "https://example.com/guide?%61=hello%20world&empty=#part", + "https://example.com/guide?%61=hello%20world&empty=#part", + ), + ( + "Cannot read https://fakeuser:fakepass@example.com/guide or https://example.com/guide?token=fakesecret", + "Cannot read https://%5Bredacted%5D@example.com/guide or https://example.com/guide?token=%5Bredacted%5D", + ), + ("https://fakeuser:fakepass@[invalid/guide?token=fakesecret", "[redacted]"), + ], +) +def test_sanitize_url_credentials(value: str, expected: str) -> None: + assert sanitize_captured_value(value) == expected + assert sanitize_captured_value(expected) == expected + + def test_sanitize_event_replaces_image_and_audio_blocks(): event = { "response": { diff --git a/posthog/test/mcp/test_v2_lowlevel.py b/posthog/test/mcp/test_v2_lowlevel.py index f239a44a5..ce3d50332 100644 --- a/posthog/test/mcp/test_v2_lowlevel.py +++ b/posthog/test/mcp/test_v2_lowlevel.py @@ -141,14 +141,54 @@ async def test_list_tools_injects_optional_context_and_captures(): "uri, captured_uri, resource_error", [ ("file:///guide.md", "file:///guide.md", False), + ( + "https://fakeuser:fakepass@example.com/guide", + "https://%5Bredacted%5D@example.com/guide", + False, + ), + ( + "https://fakeuser:fakepass@example.com/guide", + "https://%5Bredacted%5D@example.com/guide", + True, + ), + ( + "https://example.com/guide?token=fakesecret&chapter=intro", + "https://example.com/guide?token=%5Bredacted%5D&chapter=intro", + False, + ), + ( + "https://example.com/guide?token=fakesecret&chapter=intro", + "https://example.com/guide?token=%5Bredacted%5D&chapter=intro", + True, + ), + ( + "https://example.com/guide?access_token=fakeaccess&X-Amz-Credential=fakecredential&X-Amz-Signature=fakesignature", + "https://example.com/guide?access_token=%5Bredacted%5D&X-Amz-Credential=%5Bredacted%5D&X-Amz-Signature=%5Bredacted%5D", + False, + ), + ( + "https://example.com/guide?access_token=fakeaccess&X-Amz-Credential=fakecredential&X-Amz-Signature=fakesignature", + "https://example.com/guide?access_token=%5Bredacted%5D&X-Amz-Credential=%5Bredacted%5D&X-Amz-Signature=%5Bredacted%5D", + True, + ), + ( + "ui://guide/page?%74oken=fakesecret&TOKEN=fakeaccess&chapter=intro#section", + "ui://guide/page?token=%5Bredacted%5D&TOKEN=%5Bredacted%5D&chapter=intro#section", + False, + ), + ( + "ui://guide/page?%74oken=fakesecret&TOKEN=fakeaccess&chapter=intro#section", + "ui://guide/page?token=%5Bredacted%5D&TOKEN=%5Bredacted%5D&chapter=intro#section", + True, + ), ( "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", - "https://example.com/guide?token=[redacted]", + "https://example.com/guide?token=%5Bredacted%5D", False, ), ( "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", - "https://example.com/guide?token=[redacted]", + "https://example.com/guide?token=%5Bredacted%5D", True, ), ], @@ -186,7 +226,16 @@ async def test_resource_discovery_and_read_are_captured( assert len(exceptions) == int(resource_error) if resource_error: assert exceptions[0]["properties"]["$mcp_resource_name"] == captured_uri - assert "phx_EXAMPLEONLYFAKEVALUE00000000000" not in json.dumps(client.events) + for secret in ( + "phx_EXAMPLEONLYFAKEVALUE00000000000", + "fakeuser", + "fakepass", + "fakesecret", + "fakeaccess", + "fakecredential", + "fakesignature", + ): + assert secret not in json.dumps(client.events) assert props["$mcp_protocol_version"] == "2026-07-28" From 0eb054cfd0d5551ece1b1957d1d6deaf815cda70 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Wed, 9 Sep 2026 15:06:07 -0300 Subject: [PATCH 05/33] fix(mcp): bound captured URL parsing Reject captured URLs over 8,192 characters before copying or parsing them and cap parse_qsl at 128 fields. Preserve caller requests and responses. Document the limits and verify the boundary behavior in plain URLs and exception messages. Add real high-level resource-adapter coverage for early/late registration, idempotency, success/failure events, duration, and response-body exclusion. Validation: MCP v1 338 passed; MCP v2 312 passed, 17 expected skips. Ruff lint/format and mypy baseline passed. The new adapter test scores 10.0 in CodeScene; broader existing sanitizer complexity is left unchanged. --- posthog/mcp/README.md | 4 +- posthog/mcp/_sanitization.py | 8 ++- posthog/test/mcp/test_pipeline.py | 30 ++++++++++ posthog/test/mcp/test_resources.py | 88 ++++++++++++++++++++++++++++++ 4 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 posthog/test/mcp/test_resources.py diff --git a/posthog/mcp/README.md b/posthog/mcp/README.md index b90b25274..5493e7490 100644 --- a/posthog/mcp/README.md +++ b/posthog/mcp/README.md @@ -5,7 +5,9 @@ tool calls, agent intent, resource discovery and reads, and failures are capture to PostHog as `$mcp_*` events. Resource bodies are not captured. Captured URLs redact usernames, passwords, and -known credential query parameters, including signed URL credentials. This also +known credential query parameters, including signed URL credentials. URLs longer +than 8,192 characters or with more than 128 query fields are redacted entirely +to bound parsing work. This also applies when a failed read repeats the URL in its error message. Other query parameters and fragments can still contain application-specific sensitive data. Requests and responses keep their original addresses. Use `before_send` to remove diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index c4273758b..6cd5000d6 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -31,6 +31,8 @@ ) _URL_PATTERN = re.compile(r"\b[a-z][a-z0-9+.-]{0,63}://[^\s<>\"']+", re.IGNORECASE) +_MAX_URL_LENGTH = 8192 +_MAX_URL_QUERY_FIELDS = 128 _SENSITIVE_QUERY_KEY_PATTERN = re.compile( r"^(auth|key|credential|signature|sig|AWSAccessKeyId|GoogleAccessId|" r"Policy|Key-Pair-Id|X-Amz-(Credential|Signature|Security-Token)|" @@ -40,10 +42,14 @@ def _sanitize_url(match: re.Match[str]) -> str: + if match.end() - match.start() > _MAX_URL_LENGTH: + return _REDACTED_VALUE value = match.group(0) try: url = urlsplit(value) - query = parse_qsl(url.query, keep_blank_values=True) + query = parse_qsl( + url.query, keep_blank_values=True, max_num_fields=_MAX_URL_QUERY_FIELDS + ) sanitized_query = [ (key, _REDACTED_VALUE) if _should_redact_key(key) or _SENSITIVE_QUERY_KEY_PATTERN.fullmatch(key) diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 54875af1d..755f2c3fd 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -113,6 +113,36 @@ def test_sanitize_url_credentials(value: str, expected: str) -> None: assert sanitize_captured_value(expected) == expected +@pytest.mark.parametrize( + "uri, oversized", + [ + pytest.param("https://example.com/" + "a/" * 4086, False, id="length-limit"), + pytest.param( + "https://example.com/" + "a/" * 4086 + "a", True, id="length-over-limit" + ), + pytest.param( + "https://example.com/?" + "&".join(["page=1"] * 128), + False, + id="field-limit", + ), + pytest.param( + "https://example.com/?" + "&".join(["page=1"] * 129), + True, + id="fields-over-limit", + ), + pytest.param( + "https://example.com/?" + "&" * 128 + "token=fakesecret", + True, + id="empty-fields", + ), + ], +) +def test_sanitize_url_bounds(uri: str, oversized: bool) -> None: + expected = "[redacted]" if oversized else uri + assert sanitize_captured_value(uri) == expected + assert sanitize_captured_value(f"Cannot read {uri}") == f"Cannot read {expected}" + + def test_sanitize_event_replaces_image_and_audio_blocks(): event = { "response": { diff --git a/posthog/test/mcp/test_resources.py b/posthog/test/mcp/test_resources.py new file mode 100644 index 000000000..28caf7f39 --- /dev/null +++ b/posthog/test/mcp/test_resources.py @@ -0,0 +1,88 @@ +"""Resource tracking through the real high-level server adapters.""" + +import json + +import mcp.types as mcp_types +import pytest + +from posthog.mcp import instrument +from posthog.test.mcp._helpers import ( + MCP_MAJOR, + FakeClient, + events_named, + flush_background, +) + + +@pytest.fixture(params=["official", "fastmcp"]) +def server(request): + if request.param == "fastmcp": + if MCP_MAJOR >= 2: + pytest.skip("jlowin FastMCP requires MCP SDK v1") + from fastmcp import FastMCP as Server + elif MCP_MAJOR < 2: + from mcp.server.fastmcp import FastMCP as Server + else: + from mcp.server.mcpserver import MCPServer as Server + + return Server("resource-test") + + +@pytest.mark.parametrize("register_late", [False, True]) +@pytest.mark.parametrize("resource_error", [False, True]) +async def test_highlevel_resource_tracking( + server, register_late: bool, resource_error: bool +) -> None: + client = FakeClient() + body = " ".join(["Private", "document", "contents"]) + if register_late: + instrument(server, client) + + @server.resource("file:///guide.md") + async def guide() -> str: + if resource_error: + raise ValueError("resource unavailable") + return body + + instrument(server, client) + + async def dispatch(method, params=None): + if MCP_MAJOR < 2: + request = ( + mcp_types.ListResourcesRequest() + if method == "resources/list" + else mcp_types.ReadResourceRequest(params=params) + ) + handler = server._mcp_server.request_handlers[type(request)] + return (await handler(request)).root + + from posthog.test.mcp._helpers_v2 import fake_ctx + + entry = server._lowlevel_server.get_request_handler(method) + return await entry.handler(fake_ctx(method=method), params) + + listing = await dispatch("resources/list") + assert str(listing.resources[0].uri) == "file:///guide.md" + read = dispatch( + "resources/read", mcp_types.ReadResourceRequestParams(uri="file:///guide.md") + ) + if resource_error: + message = "resource unavailable" if MCP_MAJOR < 2 else "Error reading resource" + with pytest.raises(Exception, match=message): + await read + else: + result = await read + assert result.contents[0].text == body + assert str(result.contents[0].uri) == "file:///guide.md" + await flush_background() + + assert len(events_named(client, "$mcp_resources_list")) == 1 + reads = events_named(client, "$mcp_resource_read") + assert len(reads) == 1 + props = reads[0]["properties"] + assert props["$mcp_resource_name"] == "file:///guide.md" + assert props["$mcp_is_error"] is resource_error + assert props["$mcp_duration_ms"] >= 0 + assert "$mcp_response" not in props + assert len(events_named(client, "$exception")) == int(resource_error) + assert body not in json.dumps(client.events) From df1c8142428ad8d77a2e87b2dcb2ddfc61d7b999 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 10:39:10 -0300 Subject: [PATCH 06/33] fix(mcp): widen URL redaction, capture resource listings and template listings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed - URL sanitizer: drop the leading `\b` (it left `resource_https://user:pw@host` entirely unredacted), split trailing prose punctuation off before parsing and re-append it, match sensitive query keys per `-`/`_`/`.` segment plus a short exact list (`code` exact-only so it can't eat `country_code`), normalize `;` to `&` before splitting fields, redact `=`-shaped fragments, and sanitize one level of URL nested inside a retained query value. Only the part that actually changed is re-serialized, so an untouched `#section-2` stays byte-for-byte. - `resources/templates/list` is instrumented on every adapter and emitted as `$mcp_resources_list`; the captured `request.method` separates it from `resources/list`. - Listing events carry the listing as `$mcp_response` (names/uris/mime types are metadata, not a resource body — reads still capture no response). An empty listing is not flagged as an error: a template-only server legitimately lists no static resources. - `$identify` falls back to `params.uri` when a request has no `name`, and `resource_name` is sanitized on every event rather than only on reads — so a credential-bearing read uri is redacted there too. Why Reviewer follow-ups on posthog-python#928; the same semantics ship in posthog-js#4830 so both SDKs redact identically. How tested - `.venv/bin/pytest posthog/test/mcp -q` -> 358 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 330 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Notes for the reviewer - The spec's vector table is now the parametrized `test_sanitize_url_credentials` rows, including the `sort_key` over-redaction and the `country_code` keep. - The URL-key decision lives in `_should_redact_query_key`, kept separate to keep `_sanitize_url` shallow (CodeScene flagged complexity here before). Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- .sampo/changesets/calm-resource-atlas.md | 2 +- posthog/mcp/README.md | 21 ++-- posthog/mcp/_instrument_lowlevel.py | 8 ++ posthog/mcp/_instrument_v2.py | 5 + posthog/mcp/_instrumentation.py | 14 ++- posthog/mcp/_internal.py | 9 +- posthog/mcp/_sanitization.py | 140 ++++++++++++++++++----- posthog/test/mcp/_helpers.py | 7 ++ posthog/test/mcp/test_lowlevel.py | 115 ++++++++++++++++++- posthog/test/mcp/test_pipeline.py | 46 ++++++++ posthog/test/mcp/test_resources.py | 76 +++++++++--- posthog/test/mcp/test_v2_lowlevel.py | 77 ++++++++++++- 12 files changed, 457 insertions(+), 63 deletions(-) diff --git a/.sampo/changesets/calm-resource-atlas.md b/.sampo/changesets/calm-resource-atlas.md index b7b038885..241c60e7c 100644 --- a/.sampo/changesets/calm-resource-atlas.md +++ b/.sampo/changesets/calm-resource-atlas.md @@ -2,4 +2,4 @@ pypi/posthog: minor --- -Capture MCP resource discovery and reads from instrumented servers. +Capture MCP resource discovery and reads from instrumented servers. URL credential redaction (userinfo, credential-named query and fragment parameters) now applies to every captured string, including existing `$mcp_tool_call` parameters, responses and error messages, so URLs in existing tool-call data will show `%5Bredacted%5D` values after upgrading. diff --git a/posthog/mcp/README.md b/posthog/mcp/README.md index 5493e7490..a5c38d645 100644 --- a/posthog/mcp/README.md +++ b/posthog/mcp/README.md @@ -4,14 +4,19 @@ Product analytics for Model Context Protocol servers. Wrap a Python MCP server s tool calls, agent intent, resource discovery and reads, and failures are captured to PostHog as `$mcp_*` events. -Resource bodies are not captured. Captured URLs redact usernames, passwords, and -known credential query parameters, including signed URL credentials. URLs longer -than 8,192 characters or with more than 128 query fields are redacted entirely -to bound parsing work. This also -applies when a failed read repeats the URL in its error message. Other query -parameters and fragments can still contain application-specific sensitive data. -Requests and responses keep their original addresses. Use `before_send` to remove -any additional application-specific sensitive data. +Resource bodies are not captured. Resource and resource-template listings are: +a listing is metadata (names, uris, mime types), so `$mcp_resources_list` carries +it as `$mcp_response`. + +Captured URLs redact usernames, passwords, and credential-named query and +fragment parameters, including signed URL credentials. This applies to every +captured string, tool call parameters, responses and error messages included, so +it also covers a failed read that repeats the URL in its error message. URLs +longer than 8,192 characters or with more than 128 query fields are redacted +entirely to bound parsing work. Other query and fragment parameters can still +contain application-specific sensitive data. Requests and responses keep their +original addresses. Use `before_send` to remove any additional +application-specific sensitive data. ```python from posthog import Posthog diff --git a/posthog/mcp/_instrument_lowlevel.py b/posthog/mcp/_instrument_lowlevel.py index 7633b92fc..d66337e63 100644 --- a/posthog/mcp/_instrument_lowlevel.py +++ b/posthog/mcp/_instrument_lowlevel.py @@ -32,6 +32,7 @@ prepare_request, record_resource_request, request_to_dict, + resource_listing_response, resolve_session_and_client, start_tool_call_lifecycle, start_tools_list_lifecycle, @@ -83,6 +84,12 @@ def instrument_fastmcp_v2(server: Any, data: MCPAnalyticsData) -> None: def _wrap_resource_requests(server: Any, data: MCPAnalyticsData) -> None: for request_type, event_type in ( (mcp_types.ListResourcesRequest, MCPAnalyticsEventType.MCP_RESOURCES_LIST), + # Templates are listings too: the captured request method separates + # `resources/templates/list` from `resources/list` on the same event. + ( + mcp_types.ListResourceTemplatesRequest, + MCPAnalyticsEventType.MCP_RESOURCES_LIST, + ), (mcp_types.ReadResourceRequest, MCPAnalyticsEventType.MCP_RESOURCES_READ), ): _wrap_resource_request(server, data, request_type, event_type) @@ -148,6 +155,7 @@ async def handler(req: Any) -> Any: session_id, event_type=event_type, request=request, + response=resource_listing_response(event_type, result), duration_ms=(time.monotonic() - start) * 1000, client_name=client_name, client_version=client_version, diff --git a/posthog/mcp/_instrument_v2.py b/posthog/mcp/_instrument_v2.py index 8030f4540..649bc606b 100644 --- a/posthog/mcp/_instrument_v2.py +++ b/posthog/mcp/_instrument_v2.py @@ -45,6 +45,7 @@ params_to_request_dict, prepare_request, record_resource_request, + resource_listing_response, resolve_session_and_client, start_tool_call_lifecycle, start_tools_list_lifecycle, @@ -73,6 +74,9 @@ _LIST_METHOD = "tools/list" _RESOURCE_METHODS = { "resources/list": MCPAnalyticsEventType.MCP_RESOURCES_LIST, + # Templates are listings too: the captured request method separates + # `resources/templates/list` from `resources/list` on the same event. + "resources/templates/list": MCPAnalyticsEventType.MCP_RESOURCES_LIST, "resources/read": MCPAnalyticsEventType.MCP_RESOURCES_READ, } @@ -530,6 +534,7 @@ async def handler(ctx: Any, params: Any) -> Any: session_id, event_type=event_type, request=request, + response=resource_listing_response(event_type, result), duration_ms=(time.monotonic() - start) * 1000, client_name=client_name, client_version=client_version, diff --git a/posthog/mcp/_instrumentation.py b/posthog/mcp/_instrumentation.py index b6f0f46e4..52f2b4e71 100644 --- a/posthog/mcp/_instrumentation.py +++ b/posthog/mcp/_instrumentation.py @@ -905,12 +905,23 @@ async def record_tools_list( log(f"record_tools_list failed (event dropped): {err}") +def resource_listing_response(event_type: str, result: Any) -> Any: + """The result an adapter should capture as the event ``response``. A listing + (``resources/list``, ``resources/templates/list``) is metadata — names, uris, + mime types — so it is captured; a read's result is the resource body itself, + which this SDK never captures.""" + if event_type != MCPAnalyticsEventType.MCP_RESOURCES_LIST: + return None + return _to_jsonable(result) + + async def record_resource_request( data: MCPAnalyticsData, session_id: str, *, event_type: str, request: Dict[str, Any], + response: Any = None, error: Any = None, duration_ms: Optional[float] = None, client_name: Optional[str] = None, @@ -918,7 +929,7 @@ async def record_resource_request( protocol_version: Optional[str] = None, extra: Optional[Dict[str, Any]] = None, ) -> None: - """Record resources/list or resources/read without affecting dispatch.""" + """Record a resources listing or read without affecting dispatch.""" try: params = request.get("params") uri = params.get("uri") if isinstance(params, dict) else None @@ -929,6 +940,7 @@ async def record_resource_request( if event_type == MCPAnalyticsEventType.MCP_RESOURCES_READ else None, "parameters": build_captured_mcp_parameters(request), + "response": _wrap_response(response) if response is not None else None, "duration": duration_ms, "client_name": client_name, "client_version": client_version, diff --git a/posthog/mcp/_internal.py b/posthog/mcp/_internal.py index 34e547535..aa16cc687 100644 --- a/posthog/mcp/_internal.py +++ b/posthog/mcp/_internal.py @@ -226,10 +226,15 @@ async def resolve_event_properties( def _get_request_resource_name(request: Any) -> str: + """The thing the request acts on: a tool/prompt ``name``, or the ``uri`` of a + resource read — which is the only name a ``resources/read`` request carries.""" if not isinstance(request, dict): return "Unknown" params = request.get("params") if not isinstance(params, dict): return "Unknown" - name = params.get("name") - return name if isinstance(name, str) else "Unknown" + for key in ("name", "uri"): + value = params.get(key) + if isinstance(value, str): + return value + return "Unknown" diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 6cd5000d6..2409be38d 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -11,11 +11,9 @@ from __future__ import annotations import re -from typing import Any, Dict +from typing import Any, Dict, List, Tuple from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit -from ._event_types import MCPAnalyticsEventType - # SDK-injected arguments stripped from captured $mcp_parameters (they surface as # dedicated properties: $mcp_intent and $mcp_conversation_id). _INJECTED_ARGUMENT_NAMES = ("context", "conversation_id") @@ -30,42 +28,127 @@ re.IGNORECASE, ) -_URL_PATTERN = re.compile(r"\b[a-z][a-z0-9+.-]{0,63}://[^\s<>\"']+", re.IGNORECASE) +# No leading `\b`: a word boundary needs a non-word character before the scheme, +# so `resource_https://user:pw@host` (an `_` before the scheme) would match +# nothing and stay unredacted. Without it the leftmost match wins — `foo.https://x` +# is read as scheme `foo.https`, which redacts the same credentials either way. +_URL_PATTERN = re.compile(r"[a-z][a-z0-9+.-]{0,63}://[^\s<>\"']+", re.IGNORECASE) +# Prose puts URLs in sentences ("see https://x?sig=a, then retry") and in +# parentheses, and the terminal class above swallows the punctuation. It is split +# off before parsing and re-appended to whatever comes back. +_URL_TRAILING_PUNCTUATION_PATTERN = re.compile(r"[.,;:!?)\]}]+$") _MAX_URL_LENGTH = 8192 _MAX_URL_QUERY_FIELDS = 128 +# A query key is sensitive when ANY `-`/`_`/`.`-delimited segment matches, which +# covers the compound names credentials actually travel under: `private_token`, +# `oauth_signature`, `id_token`, `subscription-key`, `X-Amz-Security-Token`. +# Over-redacting a benign `sort_key` is the accepted trade for an analytics payload. +_SENSITIVE_QUERY_SEGMENT_PATTERN = re.compile( + r"(^|[-_.])(auth|token|secret|password|passwd|pwd|credential|signature|sig|" + r"key|hmac|sas|bearer|jwt|session|sessionid)([-_.]|$)", + re.IGNORECASE, +) +# Matched whole rather than per segment: `code` (an OAuth authorization code) as a +# segment would eat `country_code`, `zip_code` and `lang_code`. _SENSITIVE_QUERY_KEY_PATTERN = re.compile( - r"^(auth|key|credential|signature|sig|AWSAccessKeyId|GoogleAccessId|" - r"Policy|Key-Pair-Id|X-Amz-(Credential|Signature|Security-Token)|" - r"X-Goog-(Credential|Signature))$", + r"^(code|AWSAccessKeyId|GoogleAccessId|Policy)$", re.IGNORECASE, ) +_UrlFields = List[Tuple[str, str]] + + +def _should_redact_query_key(key: str) -> bool: + """Whether a URL query/fragment field's value must be dropped: the dict-key + rule, plus the two URL-only rules above.""" + return bool( + _should_redact_key(key) + or _SENSITIVE_QUERY_SEGMENT_PATTERN.search(key) + or _SENSITIVE_QUERY_KEY_PATTERN.match(key) + ) + + +def _sanitize_urls(text: str, *, nested: bool = True) -> str: + return _URL_PATTERN.sub( + lambda match: _sanitize_url(match.group(0), nested=nested), text + ) + -def _sanitize_url(match: re.Match[str]) -> str: - if match.end() - match.start() > _MAX_URL_LENGTH: +def _sanitize_url(value: str, *, nested: bool) -> str: + if len(value) > _MAX_URL_LENGTH: return _REDACTED_VALUE - value = match.group(0) + url_text = _URL_TRAILING_PUNCTUATION_PATTERN.sub("", value) + suffix = value[len(url_text) :] try: - url = urlsplit(value) - query = parse_qsl( - url.query, keep_blank_values=True, max_num_fields=_MAX_URL_QUERY_FIELDS + url = urlsplit(url_text) + query, sanitized_query = _sanitize_url_fields(url.query, nested=nested) + # A fragment is only a field list when it looks like one; `#section-2` is + # left byte-for-byte rather than re-serialized as `section-2=`. + fragment, sanitized_fragment = ( + _sanitize_url_fields(url.fragment, nested=nested) + if "=" in url.fragment + else ([], []) ) - sanitized_query = [ - (key, _REDACTED_VALUE) - if _should_redact_key(key) or _SENSITIVE_QUERY_KEY_PATTERN.fullmatch(key) - else (key, item) - for key, item in query - ] - netloc = url.netloc - if "@" in netloc: - netloc = "%5Bredacted%5D@" + netloc.rsplit("@", 1)[1] - if netloc == url.netloc and sanitized_query == query: + netloc = _redact_userinfo(url.netloc) + if (netloc, sanitized_query, sanitized_fragment) == ( + url.netloc, + query, + fragment, + ): return value - return urlunsplit( - (url.scheme, netloc, url.path, urlencode(sanitized_query), url.fragment) + # Only the part that changed is re-serialized, so an untouched query or + # fragment keeps its original encoding. + return ( + urlunsplit( + ( + url.scheme, + netloc, + url.path, + urlencode(sanitized_query) + if sanitized_query != query + else url.query, + urlencode(sanitized_fragment) + if sanitized_fragment != fragment + else url.fragment, + ) + ) + + suffix ) except ValueError: + return _REDACTED_VALUE + suffix + + +def _redact_userinfo(netloc: str) -> str: + if "@" not in netloc: + return netloc + return "%5Bredacted%5D@" + netloc.rsplit("@", 1)[1] + + +def _sanitize_url_fields(text: str, *, nested: bool) -> Tuple[_UrlFields, _UrlFields]: + """Parse a query (or fragment) and return both the original and the sanitized + fields, so the caller can tell whether anything was redacted. ``;`` is + normalized to ``&``: servers still emit it as a field separator, and a query + split only on ``&`` would hide the credential behind it.""" + fields = parse_qsl( + text.replace(";", "&"), + keep_blank_values=True, + max_num_fields=_MAX_URL_QUERY_FIELDS, + ) + return fields, [ + (key, _sanitize_url_field_value(key, value, nested=nested)) + for key, value in fields + ] + + +def _sanitize_url_field_value(key: str, value: str, *, nested: bool) -> str: + if _should_redact_query_key(key): return _REDACTED_VALUE + # A retained value can carry a URL of its own (a gateway's `?url=`). Sanitize + # that one too, one level deep — a URL nested inside it is already covered by + # the credentials rules applied here. + if nested and "://" in value: + return _sanitize_urls(value, nested=False) + return value # PII redaction for the agent-narrated intent string only. $mcp_intent is free @@ -149,7 +232,7 @@ def _should_redact_key(key: str) -> bool: def _sanitize_string(value: str) -> str: if len(value) >= _SIZE_GATE and _BASE64_PATTERN.match(value): return "[binary data redacted - not supported by PostHog MCP analytics]" - value = _URL_PATTERN.sub(_sanitize_url, value) + value = _sanitize_urls(value) return _redact_secret_tokens(_POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value)) @@ -289,9 +372,8 @@ def sanitize_event(event: Dict[str, Any]) -> Dict[str, Any]: if result.get("parameters") is not None: result["parameters"] = sanitize_captured_value(result["parameters"]) - if result.get("event_type") == MCPAnalyticsEventType.MCP_RESOURCES_READ: - if result.get("resource_name") is not None: - result["resource_name"] = sanitize_captured_value(result["resource_name"]) + if result.get("resource_name") is not None: + result["resource_name"] = sanitize_captured_value(result["resource_name"]) # The intent comes straight from an agent-narrated `context` string, so it # can contain a secret the LLM read aloud or personal data it narrated about diff --git a/posthog/test/mcp/_helpers.py b/posthog/test/mcp/_helpers.py index b4e019524..836ef8ad6 100644 --- a/posthog/test/mcp/_helpers.py +++ b/posthog/test/mcp/_helpers.py @@ -57,3 +57,10 @@ def events_named(source, name): (reads ``.events``) or a raw list of event dicts (PostHogMCP tests).""" events = source.events if hasattr(source, "events") else source return [e for e in events if e["event"] == name] + + +def listed_uris(response): + """The uris a captured ``$mcp_resources_list`` response advertises, whether it + listed static resources or templates.""" + listed = response.get("resources") or response.get("resourceTemplates") or [] + return [item.get("uri") or item.get("uriTemplate") for item in listed] diff --git a/posthog/test/mcp/test_lowlevel.py b/posthog/test/mcp/test_lowlevel.py index abf0754d4..7b012741e 100644 --- a/posthog/test/mcp/test_lowlevel.py +++ b/posthog/test/mcp/test_lowlevel.py @@ -8,14 +8,16 @@ from mcp.server.lowlevel import Server from posthog.mcp import instrument +from posthog.mcp.types import MCPAnalyticsOptions, UserIdentity from posthog.test.mcp._helpers import ( FakeClient, events_named as _events, flush_background as _flush, + listed_uris, ) -def make_server(*, resource_error: bool = False) -> Server: +def make_server(*, resource_error: bool = False, listing: str = "resources") -> Server: server = Server("test-lowlevel") @server.list_tools() @@ -39,9 +41,13 @@ async def call_tool(name, arguments): raise ValueError("boom") async def list_resources(_request): + if listing == "error": + raise ValueError("listing unavailable") return mcp_types.ServerResult( mcp_types.ListResourcesResult( - resources=[ + resources=[] + if listing == "empty" + else [ mcp_types.Resource( name="Guide", uri="file:///guide.md", @@ -51,6 +57,19 @@ async def list_resources(_request): ) ) + async def list_resource_templates(_request): + return mcp_types.ServerResult( + mcp_types.ListResourceTemplatesResult( + resourceTemplates=[ + mcp_types.ResourceTemplate( + name="Profile", + uriTemplate="users://{user_id}/profile", + mimeType="text/markdown", + ) + ] + ) + ) + async def read_resource(request): if resource_error: raise ValueError(f"Cannot read {request.params.uri}") @@ -67,6 +86,9 @@ async def read_resource(request): ) server.request_handlers[mcp_types.ListResourcesRequest] = list_resources + server.request_handlers[mcp_types.ListResourceTemplatesRequest] = ( + list_resource_templates + ) server.request_handlers[mcp_types.ReadResourceRequest] = read_resource return server @@ -201,6 +223,95 @@ async def test_resource_discovery_and_read_are_captured( assert secret not in json.dumps(client.events) +@pytest.mark.parametrize( + "method, listing, listed", + [ + ("resources/list", "resources", ["file:///guide.md"]), + ("resources/list", "empty", []), + ("resources/templates/list", "resources", ["users://{user_id}/profile"]), + ], +) +async def test_resource_listing_event_carries_the_listing( + method: str, listing: str, listed: list +) -> None: + server = make_server(listing=listing) + client = FakeClient() + instrument(server, client) + + request_type = ( + mcp_types.ListResourcesRequest + if method == "resources/list" + else mcp_types.ListResourceTemplatesRequest + ) + await server.request_handlers[request_type](request_type()) + await _flush() + + events = _events(client, "$mcp_resources_list") + assert len(events) == 1 + props = events[0]["properties"] + assert props["$mcp_parameters"]["request"]["method"] == method + assert listed_uris(props["$mcp_response"]) == listed + # An empty listing is a legitimate answer (a template-only server lists no + # static resources), unlike an empty tools/list. + assert props["$mcp_is_error"] is False + assert props["$mcp_duration_ms"] >= 0 + assert "$mcp_resource_name" not in props + assert not _events(client, "$exception") + + +async def test_failed_resource_listing_is_captured() -> None: + server = make_server(listing="error") + client = FakeClient() + instrument(server, client) + + with pytest.raises(ValueError, match="listing unavailable"): + await server.request_handlers[mcp_types.ListResourcesRequest]( + mcp_types.ListResourcesRequest() + ) + await _flush() + + events = _events(client, "$mcp_resources_list") + assert len(events) == 1 + props = events[0]["properties"] + assert props["$mcp_is_error"] is True + assert props["$mcp_duration_ms"] >= 0 + assert "$mcp_response" not in props + assert "$mcp_resource_name" not in props + exceptions = _events(client, "$exception") + assert len(exceptions) == 1 + assert "listing unavailable" in json.dumps(exceptions[0]["properties"]) + + +async def test_identify_on_a_resource_read_is_named_by_the_uri() -> None: + server = make_server() + client = FakeClient() + instrument( + server, + client, + MCPAnalyticsOptions( + identify=lambda request, extra: UserIdentity(distinct_id="user_42") + ), + ) + + await server.request_handlers[mcp_types.ReadResourceRequest]( + mcp_types.ReadResourceRequest( + params=mcp_types.ReadResourceRequestParams( + uri="https://fakeuser:fakepass@example.com/guide" + ) + ) + ) + await _flush() + + identified = _events(client, "$identify") + assert len(identified) == 1 + # A resources/read request carries no `name`, so the uri is the only thing + # that can name it — sanitized like any other captured URL. + assert ( + identified[0]["properties"]["$mcp_resource_name"] + == "https://%5Bredacted%5D@example.com/guide" + ) + + async def test_tool_call_success_captures_intent(): server = make_server() client = FakeClient() diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 755f2c3fd..5bbcd3155 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -106,6 +106,52 @@ def test_sanitize_redacts_large_base64(): "Cannot read https://%5Bredacted%5D@example.com/guide or https://example.com/guide?token=%5Bredacted%5D", ), ("https://fakeuser:fakepass@[invalid/guide?token=fakesecret", "[redacted]"), + ( + "https://app.example.com/cb#access_token=fakeaccess&token_type=bearer", + "https://app.example.com/cb#access_token=%5Bredacted%5D&token_type=%5Bredacted%5D", + ), + ("https://example.com/doc#section-2", "https://example.com/doc#section-2"), + ( + "https://example.com/x?a=1;token=fakesecret", + "https://example.com/x?a=1&token=%5Bredacted%5D", + ), + ( + "https://example.com/x?jwt=fakejwt&sessionid=fakesession&code=fakecode&country_code=BR", + "https://example.com/x?jwt=%5Bredacted%5D&sessionid=%5Bredacted%5D&code=%5Bredacted%5D&country_code=BR", + ), + ( + "See https://example.com/x?sig=fakesignature, then retry.", + "See https://example.com/x?sig=%5Bredacted%5D, then retry.", + ), + ( + "Failed (https://example.com/x?sig=fakesignature).", + "Failed (https://example.com/x?sig=%5Bredacted%5D).", + ), + ("Failed (https://example.com/x?a=b).", "Failed (https://example.com/x?a=b)."), + ( + "resource_https://fakeuser:fakepass@example.com/doc", + "resource_https://%5Bredacted%5D@example.com/doc", + ), + ( + "https://gitlab.example.com/api?private_token=fakesecret&oauth_signature=fakesignature" + "&id_token=fakeaccess&subscription-key=fakekey&sort_key=name", + "https://gitlab.example.com/api?private_token=%5Bredacted%5D&oauth_signature=%5Bredacted%5D" + "&id_token=%5Bredacted%5D&subscription-key=%5Bredacted%5D&sort_key=%5Bredacted%5D", + ), + ( + "https://gateway.example.com/fetch?url=https://svc:fakepass@internal.example.com/doc%3Ftoken%3Dfakesecret", + "https://gateway.example.com/fetch?url=https%3A%2F%2F%255Bredacted%255D%40internal.example.com" + "%2Fdoc%3Ftoken%3D%255Bredacted%255D", + ), + ( + "https://en.wikipedia.org/wiki/Foo_(bar)", + "https://en.wikipedia.org/wiki/Foo_(bar)", + ), + ( + "https://fakeuser:fakepass@en.wikipedia.org/wiki/Foo_(bar).", + "https://%5Bredacted%5D@en.wikipedia.org/wiki/Foo_(bar).", + ), + ("file:///guide.md", "file:///guide.md"), ], ) def test_sanitize_url_credentials(value: str, expected: str) -> None: diff --git a/posthog/test/mcp/test_resources.py b/posthog/test/mcp/test_resources.py index 28caf7f39..bb6bf24a4 100644 --- a/posthog/test/mcp/test_resources.py +++ b/posthog/test/mcp/test_resources.py @@ -11,6 +11,7 @@ FakeClient, events_named, flush_background, + listed_uris, ) @@ -19,6 +20,7 @@ def server(request): if request.param == "fastmcp": if MCP_MAJOR >= 2: pytest.skip("jlowin FastMCP requires MCP SDK v1") + pytest.importorskip("fastmcp") from fastmcp import FastMCP as Server elif MCP_MAJOR < 2: from mcp.server.fastmcp import FastMCP as Server @@ -28,6 +30,27 @@ def server(request): return Server("resource-test") +_V1_REQUEST_TYPES = { + "resources/list": mcp_types.ListResourcesRequest, + "resources/templates/list": mcp_types.ListResourceTemplatesRequest, + "resources/read": mcp_types.ReadResourceRequest, +} + + +async def dispatch(server, method, params=None): + """Send one request through the server's own handler registry, whichever MCP + SDK major is installed.""" + if MCP_MAJOR < 2: + request = _V1_REQUEST_TYPES[method](params=params) + handler = server._mcp_server.request_handlers[type(request)] + return (await handler(request)).root + + from posthog.test.mcp._helpers_v2 import fake_ctx + + entry = server._lowlevel_server.get_request_handler(method) + return await entry.handler(fake_ctx(method=method), params) + + @pytest.mark.parametrize("register_late", [False, True]) @pytest.mark.parametrize("resource_error", [False, True]) async def test_highlevel_resource_tracking( @@ -46,25 +69,12 @@ async def guide() -> str: instrument(server, client) - async def dispatch(method, params=None): - if MCP_MAJOR < 2: - request = ( - mcp_types.ListResourcesRequest() - if method == "resources/list" - else mcp_types.ReadResourceRequest(params=params) - ) - handler = server._mcp_server.request_handlers[type(request)] - return (await handler(request)).root - - from posthog.test.mcp._helpers_v2 import fake_ctx - - entry = server._lowlevel_server.get_request_handler(method) - return await entry.handler(fake_ctx(method=method), params) - - listing = await dispatch("resources/list") + listing = await dispatch(server, "resources/list") assert str(listing.resources[0].uri) == "file:///guide.md" read = dispatch( - "resources/read", mcp_types.ReadResourceRequestParams(uri="file:///guide.md") + server, + "resources/read", + mcp_types.ReadResourceRequestParams(uri="file:///guide.md"), ) if resource_error: message = "resource unavailable" if MCP_MAJOR < 2 else "Error reading resource" @@ -76,7 +86,15 @@ async def dispatch(method, params=None): assert str(result.contents[0].uri) == "file:///guide.md" await flush_background() - assert len(events_named(client, "$mcp_resources_list")) == 1 + lists = events_named(client, "$mcp_resources_list") + assert len(lists) == 1 + list_props = lists[0]["properties"] + assert list_props["$mcp_parameters"]["request"]["method"] == "resources/list" + assert listed_uris(list_props["$mcp_response"]) == ["file:///guide.md"] + assert list_props["$mcp_is_error"] is False + assert list_props["$mcp_duration_ms"] >= 0 + assert "$mcp_resource_name" not in list_props + reads = events_named(client, "$mcp_resource_read") assert len(reads) == 1 props = reads[0]["properties"] @@ -86,3 +104,25 @@ async def dispatch(method, params=None): assert "$mcp_response" not in props assert len(events_named(client, "$exception")) == int(resource_error) assert body not in json.dumps(client.events) + + +async def test_highlevel_resource_templates_listing_is_captured(server) -> None: + client = FakeClient() + + @server.resource("users://{user_id}/profile") + async def profile(user_id: str) -> str: + return f"profile for {user_id}" + + instrument(server, client) + + await dispatch(server, "resources/templates/list") + await flush_background() + + lists = events_named(client, "$mcp_resources_list") + assert len(lists) == 1 + props = lists[0]["properties"] + # Same event as resources/list; the captured request method is what tells a + # template listing apart. + assert props["$mcp_parameters"]["request"]["method"] == "resources/templates/list" + assert listed_uris(props["$mcp_response"]) == ["users://{user_id}/profile"] + assert props["$mcp_is_error"] is False diff --git a/posthog/test/mcp/test_v2_lowlevel.py b/posthog/test/mcp/test_v2_lowlevel.py index ce3d50332..a223d742a 100644 --- a/posthog/test/mcp/test_v2_lowlevel.py +++ b/posthog/test/mcp/test_v2_lowlevel.py @@ -20,11 +20,12 @@ FakeClient, events_named as _events, flush_background as _flush, + listed_uris, ) from posthog.test.mcp._helpers_v2 import fake_ctx -def make_server(*, resource_error: bool = False) -> Server: +def make_server(*, resource_error: bool = False, listing: str = "resources") -> Server: async def on_call_tool(ctx, params): if params.name == "boom": raise ValueError("explode") @@ -68,14 +69,29 @@ async def on_list_tools(ctx, params): ) async def on_list_resources(ctx, params): + if listing == "error": + raise ValueError("listing unavailable") return mcp_types.ListResourcesResult( - resources=[ + resources=[] + if listing == "empty" + else [ mcp_types.Resource( name="Guide", uri="file:///guide.md", mime_type="text/markdown" ) ] ) + async def on_list_resource_templates(ctx, params): + return mcp_types.ListResourceTemplatesResult( + resource_templates=[ + mcp_types.ResourceTemplate( + name="Profile", + uri_template="users://{user_id}/profile", + mime_type="text/markdown", + ) + ] + ) + async def on_read_resource(ctx, params): if resource_error: raise ValueError(f"Cannot read {params.uri}") @@ -92,6 +108,11 @@ async def on_read_resource(ctx, params): server.add_request_handler( "resources/list", mcp_types.PaginatedRequestParams, on_list_resources ) + server.add_request_handler( + "resources/templates/list", + mcp_types.PaginatedRequestParams, + on_list_resource_templates, + ) server.add_request_handler( "resources/read", mcp_types.ReadResourceRequestParams, on_read_resource ) @@ -239,6 +260,58 @@ async def test_resource_discovery_and_read_are_captured( assert props["$mcp_protocol_version"] == "2026-07-28" +@pytest.mark.parametrize( + "method, listing, listed", + [ + ("resources/list", "resources", ["file:///guide.md"]), + ("resources/list", "empty", []), + ("resources/templates/list", "resources", ["users://{user_id}/profile"]), + ], +) +async def test_resource_listing_event_carries_the_listing( + method: str, listing: str, listed: list +) -> None: + server = make_server(listing=listing) + client = FakeClient() + instrument(server, client) + + await _resource_request(server, method) + await _flush() + + events = _events(client, "$mcp_resources_list") + assert len(events) == 1 + props = events[0]["properties"] + assert props["$mcp_parameters"]["request"]["method"] == method + assert listed_uris(props["$mcp_response"]) == listed + # An empty listing is a legitimate answer (a template-only server lists no + # static resources), unlike an empty tools/list. + assert props["$mcp_is_error"] is False + assert props["$mcp_duration_ms"] >= 0 + assert "$mcp_resource_name" not in props + assert not _events(client, "$exception") + + +async def test_failed_resource_listing_is_captured() -> None: + server = make_server(listing="error") + client = FakeClient() + instrument(server, client) + + with pytest.raises(ValueError, match="listing unavailable"): + await _resource_request(server, "resources/list") + await _flush() + + events = _events(client, "$mcp_resources_list") + assert len(events) == 1 + props = events[0]["properties"] + assert props["$mcp_is_error"] is True + assert props["$mcp_duration_ms"] >= 0 + assert "$mcp_response" not in props + assert "$mcp_resource_name" not in props + exceptions = _events(client, "$exception") + assert len(exceptions) == 1 + assert "listing unavailable" in json.dumps(exceptions[0]["properties"]) + + # --- tools/call -------------------------------------------------------------- From 21f9f5566b589c1c40e0ef1a19d5859c240dc1dd Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 10:41:28 -0300 Subject: [PATCH 07/33] fix(mcp): strip trailing URL punctuation in linear time What changed `_URL_TRAILING_PUNCTUATION_PATTERN` (`[.,;:!?)\]}]+$`) backtracks quadratically over an interior run of punctuation, and the URL comes from an attacker- influenceable request, so one message can carry many of them. It is now a plain character set stripped with `str.rstrip`, which is the same operation in linear time. Behavior is unchanged: `rstrip` removes exactly the trailing run the anchored pattern matched. How tested - `https://x/` + 8000 `.` + `a`: 122 ms before, 0.1 ms after - `.venv/bin/pytest posthog/test/mcp -q` -> 358 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 330 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Notes for the reviewer The existing vectors cover the behavior (prose comma, `).`, `Foo_(bar)`), so no new row was added; the JS sibling should make the same swap on its own `replace(/[.,;:!?)\]}]+$/, '')`. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 2409be38d..ea196f1cd 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -35,8 +35,11 @@ _URL_PATTERN = re.compile(r"[a-z][a-z0-9+.-]{0,63}://[^\s<>\"']+", re.IGNORECASE) # Prose puts URLs in sentences ("see https://x?sig=a, then retry") and in # parentheses, and the terminal class above swallows the punctuation. It is split -# off before parsing and re-appended to whatever comes back. -_URL_TRAILING_PUNCTUATION_PATTERN = re.compile(r"[.,;:!?)\]}]+$") +# off before parsing and re-appended to whatever comes back. A character set +# stripped with `rstrip` rather than an anchored `[...]+$` pattern: backtracking +# that pattern over an interior run of punctuation is quadratic, and the URL +# comes from an attacker-influenceable request. +_URL_TRAILING_PUNCTUATION = ".,;:!?)]}" _MAX_URL_LENGTH = 8192 _MAX_URL_QUERY_FIELDS = 128 # A query key is sensitive when ANY `-`/`_`/`.`-delimited segment matches, which @@ -77,7 +80,7 @@ def _sanitize_urls(text: str, *, nested: bool = True) -> str: def _sanitize_url(value: str, *, nested: bool) -> str: if len(value) > _MAX_URL_LENGTH: return _REDACTED_VALUE - url_text = _URL_TRAILING_PUNCTUATION_PATTERN.sub("", value) + url_text = value.rstrip(_URL_TRAILING_PUNCTUATION) suffix = value[len(url_text) :] try: url = urlsplit(url_text) From e5892c9abbf332b88d2bd66bdf426e06fb282eb0 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 10:45:27 -0300 Subject: [PATCH 08/33] fix(mcp): keep apostrophes inside captured URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed `'` is a valid URI sub-delimiter, but the URL pattern's terminal class treated it as a terminator: `https://example.com/o'reilly?token=fakesecret` matched only up to the `o`, so the token shipped unredacted in `$mcp_resource_name` and `$mcp_parameters`, and `https://user:pa'ss@example.com/doc` kept its userinfo. The class is now `[^\s<>\"]+` — `"`, `<` and `>` cannot appear unencoded in a URI so they still terminate a match — and `'` joins the trailing-punctuation set, so a single-quoted URL in prose still has its closing quote split off and re-appended. How tested Three rows added to the parametrized `test_sanitize_url_credentials`: the two vectors above and `Read 'https://example.com/x?sig=fakesignature' first.`, which must keep both quotes and redact the signature. The test also re-runs the sanitizer over each expected value, so idempotence is covered. - `.venv/bin/pytest posthog/test/mcp -q` -> 361 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 333 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Notes for the reviewer The userinfo row is spelled `fakeuser:fake'pass` rather than `user:pa'ss` to match the fake-credential naming the rest of the table uses; it asserts the same redaction. The JS sibling gets the identical pattern change. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 20 ++++++++++++-------- posthog/test/mcp/test_pipeline.py | 12 ++++++++++++ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index ea196f1cd..a59adfe3c 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -32,14 +32,18 @@ # so `resource_https://user:pw@host` (an `_` before the scheme) would match # nothing and stay unredacted. Without it the leftmost match wins — `foo.https://x` # is read as scheme `foo.https`, which redacts the same credentials either way. -_URL_PATTERN = re.compile(r"[a-z][a-z0-9+.-]{0,63}://[^\s<>\"']+", re.IGNORECASE) -# Prose puts URLs in sentences ("see https://x?sig=a, then retry") and in -# parentheses, and the terminal class above swallows the punctuation. It is split -# off before parsing and re-appended to whatever comes back. A character set -# stripped with `rstrip` rather than an anchored `[...]+$` pattern: backtracking -# that pattern over an interior run of punctuation is quadratic, and the URL -# comes from an attacker-influenceable request. -_URL_TRAILING_PUNCTUATION = ".,;:!?)]}" +# `'` is a valid URI sub-delimiter, so it stays IN the match (`/o'reilly?token=x` +# must not be cut short of its query); `"`, `<` and `>` cannot appear unencoded in +# a URI, so they still terminate it. +_URL_PATTERN = re.compile(r"[a-z][a-z0-9+.-]{0,63}://[^\s<>\"]+", re.IGNORECASE) +# Prose puts URLs in sentences ("see https://x?sig=a, then retry"), in parentheses +# and in single quotes, and the terminal class above swallows the punctuation that +# closes them. It is split off before parsing and re-appended to whatever comes +# back — including the `'` the pattern now keeps, since only a trailing one closes +# a quote. A character set stripped with `rstrip` rather than an anchored +# `[...]+$` pattern: backtracking that pattern over an interior run of punctuation +# is quadratic, and the URL comes from an attacker-influenceable request. +_URL_TRAILING_PUNCTUATION = ".,;:!?)]}'" _MAX_URL_LENGTH = 8192 _MAX_URL_QUERY_FIELDS = 128 # A query key is sensitive when ANY `-`/`_`/`.`-delimited segment matches, which diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 5bbcd3155..9f78dcb91 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -152,6 +152,18 @@ def test_sanitize_redacts_large_base64(): "https://%5Bredacted%5D@en.wikipedia.org/wiki/Foo_(bar).", ), ("file:///guide.md", "file:///guide.md"), + ( + "https://example.com/o'reilly?token=fakesecret", + "https://example.com/o'reilly?token=%5Bredacted%5D", + ), + ( + "https://fakeuser:fake'pass@example.com/doc", + "https://%5Bredacted%5D@example.com/doc", + ), + ( + "Read 'https://example.com/x?sig=fakesignature' first.", + "Read 'https://example.com/x?sig=%5Bredacted%5D' first.", + ), ], ) def test_sanitize_url_credentials(value: str, expected: str) -> None: From 2b11efc7ac31250d75c86fee3a322f81d04103a7 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 10:53:21 -0300 Subject: [PATCH 09/33] fix(mcp): close three URL-redaction leaks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed - Depth exhaustion. A value that still carried a URL after the one-level nested pass was returned untouched, so a doubly nested gateway uri (`?url=`) shipped the token. The budget is one level; past it a URL-bearing value is now dropped rather than trusted. - Restored punctuation as a credential's tail. `?password=fakepass!!!` came back as `password=%5Bredacted%5D!!!`. Two rules: a string that IS a single URL (a `$mcp_resource_name`, a `params.uri`, a nested query value) has no prose, so nothing is split off it at all; and in prose, when the last field of the part the URL ends in was rewritten, the punctuation goes with it instead of being re-appended. A sentence loses its comma when it ends in a redacted credential — the accepted cost. - Pass ordering. URLs were rewritten before the PostHog-token pass, and re-serializing a query percent-encodes `/`, so `?ref=/phx_...` became `ref=%2Fphx_...` where the token pattern's `\bph` boundary no longer matched. Tokens are now redacted first, then URLs, then the entropy pass as before (that one still runs last: it works on whitespace-separated words and must see the final text). How tested Nine rows added to / adjusted in the parametrized `test_sanitize_url_credentials`, including the double-nested gateway uri, `?password=fakepass!!!`, the suffix-dropped prose rows, the two suffix-KEPT rows (credential not last; tail is a prose fragment), and the `?ref=/phx_...` row. Each expected value is re-sanitized by the test, so all of them are idempotent. - `.venv/bin/pytest posthog/test/mcp -q` -> 368 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 340 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Notes for the reviewer The token-first ordering changes one existing expectation in both low-level resource tests: `?token=phx_...` now captures as `?token=[redacted]` rather than `?token=%5Bredacted%5D`. The token pass has already redacted the value by the time the URL is parsed, so the URL rewrite finds nothing changed and returns the string as-is. Still fully redacted, only the encoding differs; the JS sibling will land on the same form. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 38 +++++++++++++++++------- posthog/test/mcp/test_lowlevel.py | 7 +++-- posthog/test/mcp/test_pipeline.py | 43 ++++++++++++++++++++++++++-- posthog/test/mcp/test_v2_lowlevel.py | 7 +++-- 4 files changed, 78 insertions(+), 17 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index a59adfe3c..ef9ffd0af 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -76,15 +76,20 @@ def _should_redact_query_key(key: str) -> bool: def _sanitize_urls(text: str, *, nested: bool = True) -> str: + # A string that IS one URL — a `$mcp_resource_name`, a `params.uri`, a query + # field's value — has no prose around it, so nothing at its end is punctuation + # closing a sentence: `?password=hunter2!!!` ends in the password itself. + if _URL_PATTERN.fullmatch(text): + return _sanitize_url(text, nested=nested, in_prose=False) return _URL_PATTERN.sub( - lambda match: _sanitize_url(match.group(0), nested=nested), text + lambda match: _sanitize_url(match.group(0), nested=nested, in_prose=True), text ) -def _sanitize_url(value: str, *, nested: bool) -> str: +def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: if len(value) > _MAX_URL_LENGTH: return _REDACTED_VALUE - url_text = value.rstrip(_URL_TRAILING_PUNCTUATION) + url_text = value.rstrip(_URL_TRAILING_PUNCTUATION) if in_prose else value suffix = value[len(url_text) :] try: url = urlsplit(url_text) @@ -103,6 +108,16 @@ def _sanitize_url(value: str, *, nested: bool) -> str: fragment, ): return value + # The split-off punctuation can be the tail of the credential rather than + # the sentence's: `?password=hunter2!!!` would come back as + # `?password=[redacted]!!!`. So when the last field of the part the URL + # ends in was rewritten, its punctuation goes with it. Losing a comma from + # the surrounding prose is the accepted cost. + tail, sanitized_tail = ( + (fragment, sanitized_fragment) if url.fragment else (query, sanitized_query) + ) + if tail and sanitized_tail[-1] != tail[-1]: + suffix = "" # Only the part that changed is re-serialized, so an untouched query or # fragment keeps its original encoding. return ( @@ -150,11 +165,11 @@ def _sanitize_url_fields(text: str, *, nested: bool) -> Tuple[_UrlFields, _UrlFi def _sanitize_url_field_value(key: str, value: str, *, nested: bool) -> str: if _should_redact_query_key(key): return _REDACTED_VALUE - # A retained value can carry a URL of its own (a gateway's `?url=`). Sanitize - # that one too, one level deep — a URL nested inside it is already covered by - # the credentials rules applied here. - if nested and "://" in value: - return _sanitize_urls(value, nested=False) + # A retained value can carry a URL of its own (a gateway's `?url=`). The budget + # for that is one level: sanitize the first, and drop any value still carrying + # a URL past it rather than trusting what we did not look inside. + if "://" in value: + return _sanitize_urls(value, nested=False) if nested else _REDACTED_VALUE return value @@ -239,8 +254,11 @@ def _should_redact_key(key: str) -> bool: def _sanitize_string(value: str) -> str: if len(value) >= _SIZE_GATE and _BASE64_PATTERN.match(value): return "[binary data redacted - not supported by PostHog MCP analytics]" - value = _sanitize_urls(value) - return _redact_secret_tokens(_POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value)) + # PostHog tokens before URLs: rewriting a query percent-encodes `/`, and a + # `?ref=/phx_...` token would then sit behind `%2F` where the pattern's `\bph` + # boundary no longer matches it. + value = _POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value) + return _redact_secret_tokens(_sanitize_urls(value)) def _redact_secret_tokens(value: str) -> str: diff --git a/posthog/test/mcp/test_lowlevel.py b/posthog/test/mcp/test_lowlevel.py index 7b012741e..dd7d3bb35 100644 --- a/posthog/test/mcp/test_lowlevel.py +++ b/posthog/test/mcp/test_lowlevel.py @@ -163,14 +163,17 @@ async def test_list_tools_injects_optional_context_and_captures(): "ui://guide/page?token=%5Bredacted%5D&TOKEN=%5Bredacted%5D&chapter=intro#section", True, ), + # The PostHog-token pass runs before the URL is parsed, so the token is + # already `[redacted]` by then and the URL rewrite finds nothing left to + # change — the value keeps that literal form instead of being re-encoded. ( "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", - "https://example.com/guide?token=%5Bredacted%5D", + "https://example.com/guide?token=[redacted]", False, ), ( "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", - "https://example.com/guide?token=%5Bredacted%5D", + "https://example.com/guide?token=[redacted]", True, ), ], diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 9f78dcb91..85c048a6c 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -119,13 +119,28 @@ def test_sanitize_redacts_large_base64(): "https://example.com/x?jwt=fakejwt&sessionid=fakesession&code=fakecode&country_code=BR", "https://example.com/x?jwt=%5Bredacted%5D&sessionid=%5Bredacted%5D&code=%5Bredacted%5D&country_code=BR", ), + # A redacted LAST field takes the split-off punctuation with it: the + # punctuation may be the credential's own tail (`?password=hunter2!!!`). ( "See https://example.com/x?sig=fakesignature, then retry.", - "See https://example.com/x?sig=%5Bredacted%5D, then retry.", + "See https://example.com/x?sig=%5Bredacted%5D then retry.", ), ( "Failed (https://example.com/x?sig=fakesignature).", - "Failed (https://example.com/x?sig=%5Bredacted%5D).", + "Failed (https://example.com/x?sig=%5Bredacted%5D", + ), + ( + "See https://example.com/x?password=fakepass!, then retry.", + "See https://example.com/x?password=%5Bredacted%5D then retry.", + ), + # ... but only the last field: anything after it proves where the URL ended. + ( + "See https://example.com/x?sig=fakesignature&page=2, then retry.", + "See https://example.com/x?sig=%5Bredacted%5D&page=2, then retry.", + ), + ( + "See https://example.com/x?sig=fakesignature#intro, then retry.", + "See https://example.com/x?sig=%5Bredacted%5D#intro, then retry.", ), ("Failed (https://example.com/x?a=b).", "Failed (https://example.com/x?a=b)."), ( @@ -160,9 +175,31 @@ def test_sanitize_redacts_large_base64(): "https://fakeuser:fake'pass@example.com/doc", "https://%5Bredacted%5D@example.com/doc", ), + # A string that is nothing but a URL has no prose, so its tail belongs to + # the URL: `!!!` is part of the password, `.` is part of the path. + ( + "https://example.com/login?password=fakepass!!!", + "https://example.com/login?password=%5Bredacted%5D", + ), + ("https://example.com/x?a=b.", "https://example.com/x?a=b."), + # One level of nesting is sanitized; a value still carrying a URL past that + # is dropped rather than trusted. + ( + "https://gateway.example.com/fetch?url=https%3A%2F%2Fgateway2.example.com%2Ffetch" + "%3Furl%3Dhttps%253A%252F%252Finternal.test%252Fdoc%253Ftoken%253Dfakesecret", + "https://gateway.example.com/fetch?url=https%3A%2F%2Fgateway2.example.com%2Ffetch" + "%3Furl%3D%255Bredacted%255D", + ), + # PostHog tokens are redacted before the URL is rewritten: percent-encoding + # the `/` would put `%2F` where the token pattern's `\bph` boundary needs a + # word boundary. + ( + "https://example.com/?ref=/phx_EXAMPLEONLYFAKEVALUE00000000000&token=fakesecret", + "https://example.com/?ref=%2F%5Bredacted%5D&token=%5Bredacted%5D", + ), ( "Read 'https://example.com/x?sig=fakesignature' first.", - "Read 'https://example.com/x?sig=%5Bredacted%5D' first.", + "Read 'https://example.com/x?sig=%5Bredacted%5D first.", ), ], ) diff --git a/posthog/test/mcp/test_v2_lowlevel.py b/posthog/test/mcp/test_v2_lowlevel.py index a223d742a..48845e77b 100644 --- a/posthog/test/mcp/test_v2_lowlevel.py +++ b/posthog/test/mcp/test_v2_lowlevel.py @@ -202,14 +202,17 @@ async def test_list_tools_injects_optional_context_and_captures(): "ui://guide/page?token=%5Bredacted%5D&TOKEN=%5Bredacted%5D&chapter=intro#section", True, ), + # The PostHog-token pass runs before the URL is parsed, so the token is + # already `[redacted]` by then and the URL rewrite finds nothing left to + # change — the value keeps that literal form instead of being re-encoded. ( "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", - "https://example.com/guide?token=%5Bredacted%5D", + "https://example.com/guide?token=[redacted]", False, ), ( "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", - "https://example.com/guide?token=%5Bredacted%5D", + "https://example.com/guide?token=[redacted]", True, ), ], From 1d3798100a024bd1dfaf7302cfcbc1350d72746e Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 10:57:52 -0300 Subject: [PATCH 10/33] fix(mcp): strip intent PII before the generic pass rewrites URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed `sanitize_event` ran `redact_pii(sanitize_captured_value(intent))`. The generic pass rewrites any URL it finds and a rewritten query percent-encodes `@`, so `Open https://example.com/?email=alice@example.com&token=fakesecret` reached `redact_pii` as `email=alice%40example.com` and the email pattern no longer matched it — an address main would have redacted. The two passes are now `sanitize_captured_value(redact_pii(intent))`: PII first, while the narration is still the raw string the agent wrote, then the generic redaction. The comment above it says why the order matters. How tested The intent composition test is now parametrized, with the existing token+email row and the new URL row asserting the exact captured value: `Open https://example.com/?email=%5Bredacted%5D&token=%5Bredacted%5D` — the address and the token are gone, the host is still there. - `.venv/bin/pytest posthog/test/mcp -q` -> 369 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 341 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 15 +++++++++------ posthog/test/mcp/test_pipeline.py | 29 +++++++++++++++++++++++------ 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index ef9ffd0af..e7572f4c8 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -402,13 +402,16 @@ def sanitize_event(event: Dict[str, Any]) -> Dict[str, Any]: # The intent comes straight from an agent-narrated `context` string, so it # can contain a secret the LLM read aloud or personal data it narrated about - # the user. Redact it like any other captured value, then strip structured - # PII (emails, phone numbers, IPs, cards, SSNs) rather than shipping it raw - # as $mcp_intent. PII redaction is scoped to the intent only — structured - # tool parameters and responses often hold the same shapes as legitimate data. + # the user. Strip structured PII (emails, phone numbers, IPs, cards, SSNs) + # first, while the narration is still raw: the generic pass rewrites any URL + # it finds, and a rewritten query percent-encodes `@`, which would hide + # `?email=alice@example.com` from the email pattern. Then redact it like any + # other captured value. PII redaction is scoped to the intent only — + # structured tool parameters and responses often hold the same shapes as + # legitimate data. if result.get("user_intent") is not None: - result["user_intent"] = redact_pii( - sanitize_captured_value(result["user_intent"]) + result["user_intent"] = sanitize_captured_value( + redact_pii(result["user_intent"]) ) if result.get("llm_model") is not None: diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 85c048a6c..a79b3f2d8 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -464,12 +464,29 @@ def test_sanitize_event_redacts_pii_from_intent(): ) -def test_sanitize_event_composes_pii_and_token_redaction_on_intent(): - event = { - "user_intent": "Rotating token phc_123456789012345678901234567890 for user carol@example.org." - } - result = sanitize_event(event) - assert result["user_intent"] == "Rotating token [redacted] for user [redacted]." +@pytest.mark.parametrize( + "label, intent, expected", + [ + ( + "posthog-token-and-email", + "Rotating token phc_123456789012345678901234567890 for user carol@example.org.", + "Rotating token [redacted] for user [redacted].", + ), + # PII is stripped before the generic pass rewrites the URL: a rewritten + # query percent-encodes `@`, and `email=alice%40example.com` no longer + # looks like an email address. The host survives either way. + ( + "email-inside-a-url", + "Open https://example.com/?email=alice@example.com&token=fakesecret", + "Open https://example.com/?email=%5Bredacted%5D&token=%5Bredacted%5D", + ), + ], +) +def test_sanitize_event_composes_pii_and_token_redaction_on_intent( + label: str, intent: str, expected: str +) -> None: + result = sanitize_event({"user_intent": intent}) + assert result["user_intent"] == expected def test_sanitize_event_does_not_redact_pii_shapes_from_structured_data(): From 65ccc768c1ebaa462092609cd9042b05a49f5175 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:02:32 -0300 Subject: [PATCH 11/33] fix(mcp): keep tool names out of the entropy detector What changed Widening the `resource_name` gate in `sanitize_event` (df1c814) sent tool names through `sanitize_captured_value`, whose entropy detector reads a legitimate identifier as a credential: a call to `Get_Organization_Memberships` reported `$mcp_tool_name: "[redacted]"`, which breaks per-tool attribution. A `resource_name` is only ever an identifier or a uri, so it now runs through `_sanitize_resource_name`: PostHog-token redaction then the URL pass, and neither the entropy detector nor the base64 gate. A name with no url in it passes through untouched. How tested New parametrized `test_sanitize_event_resource_name_keeps_identifiers_and_redacts_uris` covers all three shapes: a `$mcp_tool_call` name kept verbatim, an `$identify` uri with its userinfo redacted, and a read uri with its `?token=` redacted. `test_identify_on_a_resource_read_is_named_by_the_uri` still passes. - `.venv/bin/pytest posthog/test/mcp -q` -> 372 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 344 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Notes for the reviewer No parity change for @posthog/mcp: it has no entropy pass, so its `resourceName` was never at risk. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 14 +++++++++++++- posthog/test/mcp/test_pipeline.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index e7572f4c8..63744c79b 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -261,6 +261,18 @@ def _sanitize_string(value: str) -> str: return _redact_secret_tokens(_sanitize_urls(value)) +def _sanitize_resource_name(value: Any) -> Any: + """Sanitize a ``resource_name``: either an identifier (a tool or prompt name) + or a resource uri, so only the passes that matter for a uri run. The entropy + detector ``sanitize_captured_value`` applies to free text is deliberately left + out — it reads a name like ``Get_Organization_Memberships`` as a credential, + and a redacted name costs every per-tool metric the event exists for. A name + with no url in it comes back untouched.""" + if not isinstance(value, str): + return value + return _sanitize_urls(_POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value)) + + def _redact_secret_tokens(value: str) -> str: """Redact credential-looking words, leaving the surrounding text intact. @@ -398,7 +410,7 @@ def sanitize_event(event: Dict[str, Any]) -> Dict[str, Any]: result["parameters"] = sanitize_captured_value(result["parameters"]) if result.get("resource_name") is not None: - result["resource_name"] = sanitize_captured_value(result["resource_name"]) + result["resource_name"] = _sanitize_resource_name(result["resource_name"]) # The intent comes straight from an agent-narrated `context` string, so it # can contain a secret the LLM read aloud or personal data it narrated about diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index a79b3f2d8..e33647862 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -453,6 +453,36 @@ def test_redact_pii_is_not_quadratic_on_pathological_input(): assert time.monotonic() - start < 1.0 +@pytest.mark.parametrize( + "event_type, resource_name, expected", + [ + # A tool name is an identifier, not free text: the entropy detector that + # guards captured values reads this one as a credential, and a redacted + # name costs every per-tool metric the event exists for. + ( + MCPAnalyticsEventType.MCP_TOOLS_CALL, + "Get_Organization_Memberships", + "Get_Organization_Memberships", + ), + ( + MCPAnalyticsEventType.IDENTIFY, + "https://fakeuser:fakepass@example.com/doc", + "https://%5Bredacted%5D@example.com/doc", + ), + ( + MCPAnalyticsEventType.MCP_RESOURCES_READ, + "https://example.com/guide?token=fakesecret", + "https://example.com/guide?token=%5Bredacted%5D", + ), + ], +) +def test_sanitize_event_resource_name_keeps_identifiers_and_redacts_uris( + event_type: str, resource_name: str, expected: str +) -> None: + result = sanitize_event({"event_type": event_type, "resource_name": resource_name}) + assert result["resource_name"] == expected + + def test_sanitize_event_redacts_pii_from_intent(): event = { "user_intent": "Looking up orders for jane.doe@acme.com and calling +1 (415) 555-0142 about a refund.", From 5f5a65ef4a0fb42dd0917b16e36688498a73482d Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:05:47 -0300 Subject: [PATCH 12/33] fix(mcp): redact credentials in resource uris with no authority MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed An MCP resource uri need not have an authority, so `resource:guide?token=...` and `file:/guide.md?token=...` never reached the URL pass and shipped their token in `$mcp_resource_name` and `$mcp_parameters`. The `//` is now optional in `_URL_PATTERN` (`[^\s<>"]+` absorbs a `//host` when there is one), and the nested-value check in `_sanitize_url_field_value` asks the pattern instead of looking for `://`, so `?url=resource:guide?token=x` is covered too. The looser pattern over-matches prose (`Error:foo`, `at12:30`, `C:\path`); a comment says why that is harmless — a match with nothing to redact is returned byte-for-byte and never re-serialized. How tested Rows added to `test_sanitize_url_credentials` for both uri forms and for the three byte-for-byte prose cases, a `resource:` row added to the resource_name test, and a focused read test in both low-level suites asserting the token is absent from every capture. - `.venv/bin/pytest posthog/test/mcp -q` -> 379 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 351 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Notes for the reviewer Two expectations differ from the ones proposed, and both are asserted as observed: - `file:/guide.md?token=...` re-serializes as `file:///guide.md?token=...`; `urlunsplit` restores the empty authority, and JS's `new URL()` does the same. - `resource:guide?token=fakesecret` through `sanitize_captured_value` (the `$mcp_parameters` path) comes back as a bare `[redacted]`: the URL pass rewrites it to `resource:guide?token=%5Bredacted%5D`, and the entropy detector that runs after it for free-text values reads that rewritten string as a credential. `$mcp_resource_name` skips that pass and keeps the readable `resource:guide?token=%5Bredacted%5D`. The token is gone on both paths, but @posthog/mcp has no entropy pass, so its `$mcp_parameters` will keep the readable form where Python drops the value. Flagged for a parity decision. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 10 ++++++++-- posthog/test/mcp/test_lowlevel.py | 24 ++++++++++++++++++++++++ posthog/test/mcp/test_pipeline.py | 23 +++++++++++++++++++++++ posthog/test/mcp/test_v2_lowlevel.py | 22 ++++++++++++++++++++++ 4 files changed, 77 insertions(+), 2 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 63744c79b..2d9c82552 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -35,7 +35,13 @@ # `'` is a valid URI sub-delimiter, so it stays IN the match (`/o'reilly?token=x` # must not be cut short of its query); `"`, `<` and `>` cannot appear unencoded in # a URI, so they still terminate it. -_URL_PATTERN = re.compile(r"[a-z][a-z0-9+.-]{0,63}://[^\s<>\"]+", re.IGNORECASE) +# +# The authority is optional — an MCP resource uri need not have one +# (`resource:guide?token=...`, `file:/guide.md?token=...`), and `[^\s<>"]+` +# absorbs a `//host` when there is one. That over-matches prose (`Error:foo`, +# `at12:30`, `C:\path`), which is harmless: a match with nothing to redact is +# returned byte-for-byte, never re-serialized. +_URL_PATTERN = re.compile(r"[a-z][a-z0-9+.-]{0,63}:[^\s<>\"]+", re.IGNORECASE) # Prose puts URLs in sentences ("see https://x?sig=a, then retry"), in parentheses # and in single quotes, and the terminal class above swallows the punctuation that # closes them. It is split off before parsing and re-appended to whatever comes @@ -168,7 +174,7 @@ def _sanitize_url_field_value(key: str, value: str, *, nested: bool) -> str: # A retained value can carry a URL of its own (a gateway's `?url=`). The budget # for that is one level: sanitize the first, and drop any value still carrying # a URL past it rather than trusting what we did not look inside. - if "://" in value: + if _URL_PATTERN.search(value): return _sanitize_urls(value, nested=False) if nested else _REDACTED_VALUE return value diff --git a/posthog/test/mcp/test_lowlevel.py b/posthog/test/mcp/test_lowlevel.py index dd7d3bb35..c5842fe61 100644 --- a/posthog/test/mcp/test_lowlevel.py +++ b/posthog/test/mcp/test_lowlevel.py @@ -285,6 +285,30 @@ async def test_failed_resource_listing_is_captured() -> None: assert "listing unavailable" in json.dumps(exceptions[0]["properties"]) +async def test_authority_less_resource_uri_is_redacted() -> None: + """An MCP resource uri need not have an authority. The captured name keeps the + redacted uri; the captured parameter is dropped whole, because the entropy + detector that guards free-text values reads the rewritten `resource:...` string + as a credential. Either way the token never reaches PostHog.""" + server = make_server() + client = FakeClient() + instrument(server, client) + + await server.request_handlers[mcp_types.ReadResourceRequest]( + mcp_types.ReadResourceRequest( + params=mcp_types.ReadResourceRequestParams( + uri="resource:guide?token=fakesecret" + ) + ) + ) + await _flush() + + props = _events(client, "$mcp_resource_read")[0]["properties"] + assert props["$mcp_resource_name"] == "resource:guide?token=%5Bredacted%5D" + assert props["$mcp_parameters"]["request"]["params"]["uri"] == "[redacted]" + assert "fakesecret" not in json.dumps(client.events) + + async def test_identify_on_a_resource_read_is_named_by_the_uri() -> None: server = make_server() client = FakeClient() diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index e33647862..7d1ebf7a4 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -167,6 +167,23 @@ def test_sanitize_redacts_large_base64(): "https://%5Bredacted%5D@en.wikipedia.org/wiki/Foo_(bar).", ), ("file:///guide.md", "file:///guide.md"), + # An MCP resource uri need not have an authority, so the `//` is optional. + # `urlunsplit` re-serializes an authority-less `file:` uri as `file:///`, + # which is also what JS's `new URL()` produces. + ( + "file:/guide.md?token=fakesecret", + "file:///guide.md?token=%5Bredacted%5D", + ), + # Same rewrite for `resource:guide?token=...` — but the entropy pass that + # follows for free-text values judges the rewritten string a credential and + # drops it whole. The token is gone either way; `$mcp_resource_name`, which + # skips that pass, keeps the readable form (see the resource_name test). + ("resource:guide?token=fakesecret", "[redacted]"), + # An optional authority over-matches prose, which costs nothing: a match + # with nothing to redact is returned byte-for-byte. + ("Error: see resource:guide.", "Error: see resource:guide."), + ("Meet at12:30 today", "Meet at12:30 today"), + ("resource:guide", "resource:guide"), ( "https://example.com/o'reilly?token=fakesecret", "https://example.com/o'reilly?token=%5Bredacted%5D", @@ -474,6 +491,12 @@ def test_redact_pii_is_not_quadratic_on_pathological_input(): "https://example.com/guide?token=fakesecret", "https://example.com/guide?token=%5Bredacted%5D", ), + # An authority-less resource uri is redacted the same way. + ( + MCPAnalyticsEventType.MCP_RESOURCES_READ, + "resource:guide?token=fakesecret", + "resource:guide?token=%5Bredacted%5D", + ), ], ) def test_sanitize_event_resource_name_keeps_identifiers_and_redacts_uris( diff --git a/posthog/test/mcp/test_v2_lowlevel.py b/posthog/test/mcp/test_v2_lowlevel.py index 48845e77b..f865edf51 100644 --- a/posthog/test/mcp/test_v2_lowlevel.py +++ b/posthog/test/mcp/test_v2_lowlevel.py @@ -263,6 +263,28 @@ async def test_resource_discovery_and_read_are_captured( assert props["$mcp_protocol_version"] == "2026-07-28" +async def test_authority_less_resource_uri_is_redacted() -> None: + """An MCP resource uri need not have an authority. The captured name keeps the + redacted uri; the captured parameter is dropped whole, because the entropy + detector that guards free-text values reads the rewritten `resource:...` string + as a credential. Either way the token never reaches PostHog.""" + server = make_server() + client = FakeClient() + instrument(server, client) + + await _resource_request( + server, + "resources/read", + mcp_types.ReadResourceRequestParams(uri="resource:guide?token=fakesecret"), + ) + await _flush() + + props = _events(client, "$mcp_resource_read")[0]["properties"] + assert props["$mcp_resource_name"] == "resource:guide?token=%5Bredacted%5D" + assert props["$mcp_parameters"]["request"]["params"]["uri"] == "[redacted]" + assert "fakesecret" not in json.dumps(client.events) + + @pytest.mark.parametrize( "method, listing, listed", [ From 36e175e636df71c8221e17e76a5c5deaffad14e4 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:08:58 -0300 Subject: [PATCH 13/33] fix(mcp): keep the URL length bound off data uris MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed With the authority optional, a long unspaced `data:...;base64,...` string that is not valid base64 (so the binary-data branch deliberately keeps it) matched `_URL_PATTERN`, blew the 8192 bound and came back as `[redacted]`. The bound now applies only to a match that opens with an authority — the case it exists for, capping parsing work on an attacker-shaped URL. An over-long authority-less match is sanitized normally; parse_qsl is already bounded by its field count. How tested A 10,000+ char `data:application/octet-stream;base64,AAAA%ZZ...` row added to `test_sanitize_url_bounds`, asserted unchanged both standalone and inside prose; the existing over-length `https://...` row still yields `[redacted]`. - `.venv/bin/pytest posthog/test/mcp -q` -> 380 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 352 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 7 ++++++- posthog/test/mcp/test_pipeline.py | 7 +++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 2d9c82552..69a0ed309 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -50,6 +50,11 @@ # `[...]+$` pattern: backtracking that pattern over an interior run of punctuation # is quadratic, and the URL comes from an attacker-influenceable request. _URL_TRAILING_PUNCTUATION = ".,;:!?)]}'" +# The length bound below caps parsing work on an attacker-shaped authority URL, so +# it only applies to a match that opens with one. A match this long with no +# authority is a data uri, which the bound must not eat — the binary-data branch +# deliberately keeps those, and the field count is already bounded when parsing. +_URL_AUTHORITY_PATTERN = re.compile(r"^[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) _MAX_URL_LENGTH = 8192 _MAX_URL_QUERY_FIELDS = 128 # A query key is sensitive when ANY `-`/`_`/`.`-delimited segment matches, which @@ -93,7 +98,7 @@ def _sanitize_urls(text: str, *, nested: bool = True) -> str: def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: - if len(value) > _MAX_URL_LENGTH: + if len(value) > _MAX_URL_LENGTH and _URL_AUTHORITY_PATTERN.match(value): return _REDACTED_VALUE url_text = value.rstrip(_URL_TRAILING_PUNCTUATION) if in_prose else value suffix = value[len(url_text) :] diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 7d1ebf7a4..5f5a9c183 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -247,6 +247,13 @@ def test_sanitize_url_credentials(value: str, expected: str) -> None: True, id="empty-fields", ), + # The length bound guards authority parsing, so it must not swallow a long + # data uri — not valid base64, so the binary-data branch keeps it too. + pytest.param( + "data:application/octet-stream;base64,AAAA%ZZ" + "A" * 10_000, + False, + id="authority-less-over-limit", + ), ], ) def test_sanitize_url_bounds(uri: str, oversized: bool) -> None: From 98abc7ba825de9fc5125f5bc0af543af41551df4 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:11:22 -0300 Subject: [PATCH 14/33] fix(mcp): start the address at the authority, not at a prose word What changed With the authority optional, `Failed URL:https://alice:hunter2@example.com/doc` matched as one URL with scheme `URL`, so urlsplit put the whole address in the path and the userinfo was never redacted. `_sanitize_url` now looks for the first authority-bearing scheme in the match: when it starts past index 0, everything before it is prose (`URL:`, `a:b:`) and is handed back verbatim with only the remainder sanitized. Matches that already start at the authority, and authority-less uris, take the path they take today. How tested Rows added to `test_sanitize_url_credentials` for the userinfo case, the `?token=` case and the byte-for-byte `Note:https://example.com/doc`. - `.venv/bin/pytest posthog/test/mcp -q` -> 384 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 356 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Notes for the reviewer `see:resource:guide?token=fakesecret` is asserted in the resource_name test rather than the URL table: the URL pass produces the expected `see:resource:guide?token=%5Bredacted%5D`, but the entropy detector that runs after it for free-text values drops that rewritten string whole, the same known behavior as the plain `resource:guide?token=...` case. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 10 ++++++++++ posthog/test/mcp/test_pipeline.py | 19 ++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 69a0ed309..ffbb6012c 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -55,6 +55,7 @@ # authority is a data uri, which the bound must not eat — the binary-data branch # deliberately keeps those, and the field count is already bounded when parsing. _URL_AUTHORITY_PATTERN = re.compile(r"^[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) +_URL_AUTHORITY_SEARCH = re.compile(r"[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) _MAX_URL_LENGTH = 8192 _MAX_URL_QUERY_FIELDS = 128 # A query key is sensitive when ANY `-`/`_`/`.`-delimited segment matches, which @@ -100,6 +101,15 @@ def _sanitize_urls(text: str, *, nested: bool = True) -> str: def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: if len(value) > _MAX_URL_LENGTH and _URL_AUTHORITY_PATTERN.match(value): return _REDACTED_VALUE + # A colon-suffixed word in front of a real URL (`Failed URL:https://...`) is + # absorbed by the authority-less pattern, and parsing the whole thing puts the + # address — userinfo included — in the path, where nothing redacts it. The + # address starts where the authority does; anything before that is prose. + authority = _URL_AUTHORITY_SEARCH.search(value) + if authority and authority.start() > 0: + return value[: authority.start()] + _sanitize_url( + value[authority.start() :], nested=nested, in_prose=in_prose + ) url_text = value.rstrip(_URL_TRAILING_PUNCTUATION) if in_prose else value suffix = value[len(url_text) :] try: diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 5f5a9c183..daa776e04 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -184,6 +184,17 @@ def test_sanitize_redacts_large_base64(): ("Error: see resource:guide.", "Error: see resource:guide."), ("Meet at12:30 today", "Meet at12:30 today"), ("resource:guide", "resource:guide"), + # A colon-suffixed word in front of a URL is prose, not part of the + # address: the authority is where the address starts. + ( + "Failed URL:https://fakeuser:fakepass@example.com/doc", + "Failed URL:https://%5Bredacted%5D@example.com/doc", + ), + ( + "URL:https://example.com/x?token=fakesecret", + "URL:https://example.com/x?token=%5Bredacted%5D", + ), + ("Note:https://example.com/doc", "Note:https://example.com/doc"), ( "https://example.com/o'reilly?token=fakesecret", "https://example.com/o'reilly?token=%5Bredacted%5D", @@ -498,12 +509,18 @@ def test_redact_pii_is_not_quadratic_on_pathological_input(): "https://example.com/guide?token=fakesecret", "https://example.com/guide?token=%5Bredacted%5D", ), - # An authority-less resource uri is redacted the same way. + # An authority-less resource uri is redacted the same way, whatever prose + # the authority-less pattern absorbed in front of it. ( MCPAnalyticsEventType.MCP_RESOURCES_READ, "resource:guide?token=fakesecret", "resource:guide?token=%5Bredacted%5D", ), + ( + MCPAnalyticsEventType.MCP_RESOURCES_READ, + "see:resource:guide?token=fakesecret", + "see:resource:guide?token=%5Bredacted%5D", + ), ], ) def test_sanitize_event_resource_name_keeps_identifiers_and_redacts_uris( From b50cd5626032fcc590d55c0771feacf8441c16b7 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:15:29 -0300 Subject: [PATCH 15/33] fix(mcp): only skip a prefix that is really prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed The prefix skip took any authority found past index 0 as prose, so `file:/guide?password=hunter2&url=https://example.com` treated its own outer URI as the prefix and returned the password raw. The skip now requires the prefix to be a run of colon-suffixed words (`URL:`, `a:b:`). Anything with a `?`, `/` or `=` in it means the match is an outer URI, which is parsed whole — its query pass redacts its own credentials, and a retained value carrying the inner URL goes through the nested pass. How tested Rows added to `test_sanitize_url_credentials` for the outer-URI case, the `token=+` case and the `a:b:` prose run; the `Failed URL:` / `URL:` / `Note:` rows are unchanged. - `.venv/bin/pytest posthog/test/mcp -q` -> 387 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 359 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Notes for the reviewer One byte differs from the proposed row and is asserted as observed: `resource:g?token=fakesecret+https://fakeuser:fakepass@b` comes back as a bare `[redacted]`, not `resource:g?token=%5Bredacted%5D`. The URL pass does produce that value — the entropy detector that runs after it for free-text values then drops the rewritten authority-less string whole, the same behavior already noted for `resource:guide?token=...`. The credential and the inner userinfo are gone on either path. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 14 ++++++++++++-- posthog/test/mcp/test_pipeline.py | 15 +++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index ffbb6012c..2fa701acd 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -56,6 +56,9 @@ # deliberately keeps those, and the field count is already bounded when parsing. _URL_AUTHORITY_PATTERN = re.compile(r"^[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) _URL_AUTHORITY_SEARCH = re.compile(r"[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) +# A prose prefix is a run of colon-suffixed words: `URL:`, `a:b:`. Anything else +# in front of an authority (a `?`, `/`, `=`) belongs to an outer URI, not to prose. +_PROSE_PREFIX_PATTERN = re.compile(r"(?:[a-z][a-z0-9+.-]{0,63}:)+", re.IGNORECASE) _MAX_URL_LENGTH = 8192 _MAX_URL_QUERY_FIELDS = 128 # A query key is sensitive when ANY `-`/`_`/`.`-delimited segment matches, which @@ -104,9 +107,16 @@ def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: # A colon-suffixed word in front of a real URL (`Failed URL:https://...`) is # absorbed by the authority-less pattern, and parsing the whole thing puts the # address — userinfo included — in the path, where nothing redacts it. The - # address starts where the authority does; anything before that is prose. + # address starts where the authority does. This only holds when what precedes + # it really is prose: an outer URI whose query carries a URL + # (`file:/guide?password=x&url=https://...`) is no prefix at all, and parsing + # it whole is what redacts its own credentials. authority = _URL_AUTHORITY_SEARCH.search(value) - if authority and authority.start() > 0: + if ( + authority + and authority.start() > 0 + and _PROSE_PREFIX_PATTERN.fullmatch(value[: authority.start()]) + ): return value[: authority.start()] + _sanitize_url( value[authority.start() :], nested=nested, in_prose=in_prose ) diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index daa776e04..bc244f851 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -195,6 +195,21 @@ def test_sanitize_redacts_large_base64(): "URL:https://example.com/x?token=%5Bredacted%5D", ), ("Note:https://example.com/doc", "Note:https://example.com/doc"), + ( + "a:b:https://fakeuser:fakepass@example.com/doc", + "a:b:https://%5Bredacted%5D@example.com/doc", + ), + # ... but a URI whose own query carries a URL is not a prefix: parsing it + # whole is what redacts its password, and the nested pass handles the + # retained `url=` value. + ( + "file:/guide?password=fakepass&url=https://example.com", + "file:///guide?password=%5Bredacted%5D&url=https%3A%2F%2Fexample.com", + ), + # Same shape, and the entropy pass then drops the rewritten authority-less + # string whole (as above); the credential and the inner userinfo are gone + # either way. + ("resource:g?token=fakesecret+https://fakeuser:fakepass@b", "[redacted]"), ( "https://example.com/o'reilly?token=fakesecret", "https://example.com/o'reilly?token=%5Bredacted%5D", From 1999e40e37bdadf067142d47fa60fc15d56421f8 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:17:32 -0300 Subject: [PATCH 16/33] fix(mcp): split a match that runs two addresses together MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed Replaces the prose-prefix rule from b50cd56, which only covered a colon-suffixed word and missed URLs joined without whitespace: `https://example.com/doc,https://user:pw@other.example.com/doc` parsed as one address with the second one — userinfo and all — buried in the first one's path. One rule covers both shapes: split the match at the first authority that starts before its first `?` or `#`, and sanitize each part on its own. An authority after `?`/`#` is a query or fragment value, so the outer URI is parsed whole and its own field pass redacts it (a sensitive key, or the nested pass). `_PROSE_PREFIX_PATTERN` is gone; `_split_at_second_address` replaces it, and each half is strictly shorter so the recursion terminates. How tested Rows added for the joined-addresses case and for two markdown links run together; the `a:b:` / `Failed URL:` / `URL:` / `Note:` rows, the `file:/guide?password=` row and both gateway rows are unchanged. - `.venv/bin/pytest posthog/test/mcp -q` -> 389 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 361 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Notes for the reviewer `resource:g?token=fakesecret+https://fakeuser:fakepass@b` still asserts a bare `[redacted]`: the URL pass produces `resource:g?token=%5Bredacted%5D` and the entropy detector that follows for free-text values drops the rewritten authority-less string whole, as already noted for `resource:guide?token=...`. Every other proposed row matches byte for byte. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 49 ++++++++++++++++++------------- posthog/test/mcp/test_pipeline.py | 20 +++++++++---- 2 files changed, 44 insertions(+), 25 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 2fa701acd..1bc00380f 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -11,7 +11,7 @@ from __future__ import annotations import re -from typing import Any, Dict, List, Tuple +from typing import Any, Dict, List, Optional, Tuple from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit # SDK-injected arguments stripped from captured $mcp_parameters (they surface as @@ -56,9 +56,6 @@ # deliberately keeps those, and the field count is already bounded when parsing. _URL_AUTHORITY_PATTERN = re.compile(r"^[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) _URL_AUTHORITY_SEARCH = re.compile(r"[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) -# A prose prefix is a run of colon-suffixed words: `URL:`, `a:b:`. Anything else -# in front of an authority (a `?`, `/`, `=`) belongs to an outer URI, not to prose. -_PROSE_PREFIX_PATTERN = re.compile(r"(?:[a-z][a-z0-9+.-]{0,63}:)+", re.IGNORECASE) _MAX_URL_LENGTH = 8192 _MAX_URL_QUERY_FIELDS = 128 # A query key is sensitive when ANY `-`/`_`/`.`-delimited segment matches, which @@ -104,22 +101,18 @@ def _sanitize_urls(text: str, *, nested: bool = True) -> str: def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: if len(value) > _MAX_URL_LENGTH and _URL_AUTHORITY_PATTERN.match(value): return _REDACTED_VALUE - # A colon-suffixed word in front of a real URL (`Failed URL:https://...`) is - # absorbed by the authority-less pattern, and parsing the whole thing puts the - # address — userinfo included — in the path, where nothing redacts it. The - # address starts where the authority does. This only holds when what precedes - # it really is prose: an outer URI whose query carries a URL - # (`file:/guide?password=x&url=https://...`) is no prefix at all, and parsing - # it whole is what redacts its own credentials. - authority = _URL_AUTHORITY_SEARCH.search(value) - if ( - authority - and authority.start() > 0 - and _PROSE_PREFIX_PATTERN.fullmatch(value[: authority.start()]) - ): - return value[: authority.start()] + _sanitize_url( - value[authority.start() :], nested=nested, in_prose=in_prose - ) + # One match can hold a prose word in front of the address (`URL:https://...`, + # `a:b:https://...`) or two addresses run together (`/doc,https://...`). Either + # way the second address begins inside what would parse as the first one's + # path, where nothing — its userinfo least of all — is redacted. So the match + # is split at that authority and each part sanitized on its own. An authority + # AFTER the first `?` or `#` is a query or fragment value instead, which the + # field pass already handles (a sensitive key, or the nested pass). + split = _split_at_second_address(value) + if split is not None: + return _sanitize_url( + value[:split], nested=nested, in_prose=in_prose + ) + _sanitize_url(value[split:], nested=nested, in_prose=in_prose) url_text = value.rstrip(_URL_TRAILING_PUNCTUATION) if in_prose else value suffix = value[len(url_text) :] try: @@ -171,6 +164,22 @@ def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: return _REDACTED_VALUE + suffix +def _split_at_second_address(value: str) -> Optional[int]: + """Where a second address starts inside ``value``, or None. Each half is + strictly shorter than the whole, so the split recursion terminates.""" + boundary = min( + (value.index(char) for char in "?#" if char in value), default=len(value) + ) + return next( + ( + match.start() + for match in _URL_AUTHORITY_SEARCH.finditer(value) + if 0 < match.start() < boundary + ), + None, + ) + + def _redact_userinfo(netloc: str) -> str: if "@" not in netloc: return netloc diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index bc244f851..fd9c38b3e 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -184,8 +184,9 @@ def test_sanitize_redacts_large_base64(): ("Error: see resource:guide.", "Error: see resource:guide."), ("Meet at12:30 today", "Meet at12:30 today"), ("resource:guide", "resource:guide"), - # A colon-suffixed word in front of a URL is prose, not part of the - # address: the authority is where the address starts. + # A match that holds a prose word in front of the address, or two addresses + # run together, is split at the second address and each part sanitized on + # its own — otherwise the second one hides in the first one's path. ( "Failed URL:https://fakeuser:fakepass@example.com/doc", "Failed URL:https://%5Bredacted%5D@example.com/doc", @@ -199,9 +200,18 @@ def test_sanitize_redacts_large_base64(): "a:b:https://fakeuser:fakepass@example.com/doc", "a:b:https://%5Bredacted%5D@example.com/doc", ), - # ... but a URI whose own query carries a URL is not a prefix: parsing it - # whole is what redacts its password, and the nested pass handles the - # retained `url=` value. + ( + "https://example.com/doc,https://fakeuser:fakepass@other.example.com/doc", + "https://example.com/doc,https://%5Bredacted%5D@other.example.com/doc", + ), + # The closing `)` goes with the redacted trailing field, by the rule above: + # punctuation after a rewritten last field may be the credential's own. + ( + "[a](https://example.com/a)[b](https://example.com/b?token=fakesecret)", + "[a](https://example.com/a)[b](https://example.com/b?token=%5Bredacted%5D", + ), + # An authority after the first `?` is a query value, not a second address: + # the outer URI is parsed whole, which is what redacts its own password. ( "file:/guide?password=fakepass&url=https://example.com", "file:///guide?password=%5Bredacted%5D&url=https%3A%2F%2Fexample.com", From f0c6c57c8377748d558cbf690ff968155352ccc7 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:24:58 -0300 Subject: [PATCH 17/33] fix(mcp): run the credential detectors before the URL pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed The entropy detector scans a 200-char window, and rewriting a URL can grow a word past it: `https://example.com/<120 chars>?ref=ghp_<36>&token=x` was `[redacted]` on main and kept its GitHub token after the URL pass moved ahead of it. Both credential passes now run first (PostHog tokens, then the detector), and the URL pass runs last — it only redacts or percent-encodes, so it never exposes anything the detectors could have matched. `_is_secret` now strips this sanitizer's own redaction markers before judging a word. A value can be sanitized twice (a response's content blocks are), and the marker's character mix alone pushed a short uri like `resource:guide?token=%5Bredacted%5D` over the entropy bar, dropping a value we had already made safe. Stripping the marker rather than skipping the word keeps a real credential written around one detectable. Together these restore the readable form for authority-less uris through `sanitize_captured_value` — `resource:guide?token=%5Bredacted%5D` rather than a bare `[redacted]` — which is byte parity with @posthog/mcp, and makes the sanitizer idempotent on every vector in the table. How tested The three `resource:`/`see:resource:` rows now assert the readable form, the Codex `ghp_` row asserts `[redacted]`, and the authority-less read case moved back into the parametrized uri tables in both low-level suites (its standalone test is gone, name and parameters agree again). - `.venv/bin/pytest posthog/test/mcp -q` -> 391 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 363 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 19 ++++++++++++++---- posthog/test/mcp/test_lowlevel.py | 30 ++++++---------------------- posthog/test/mcp/test_pipeline.py | 27 ++++++++++++++++--------- posthog/test/mcp/test_v2_lowlevel.py | 28 ++++++-------------------- 4 files changed, 45 insertions(+), 59 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 1bc00380f..45db59a66 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -18,6 +18,7 @@ # dedicated properties: $mcp_intent and $mcp_conversation_id). _INJECTED_ARGUMENT_NAMES = ("context", "conversation_id") _REDACTED_VALUE = "[redacted]" +_ENCODED_REDACTED_VALUE = "%5Bredacted%5D" _BASE64_PATTERN = re.compile(r"^[A-Za-z0-9+/\n\r]+=*$") _SIZE_GATE = 10_240 _POSTHOG_TOKEN_PATTERN = re.compile(r"\bph[a-z]_[A-Za-z0-9_-]{20,}\b") @@ -294,11 +295,14 @@ def _should_redact_key(key: str) -> bool: def _sanitize_string(value: str) -> str: if len(value) >= _SIZE_GATE and _BASE64_PATTERN.match(value): return "[binary data redacted - not supported by PostHog MCP analytics]" - # PostHog tokens before URLs: rewriting a query percent-encodes `/`, and a - # `?ref=/phx_...` token would then sit behind `%2F` where the pattern's `\bph` - # boundary no longer matches it. + # Both credential passes run before the URL pass, because rewriting a URL + # changes the text they match on: it percent-encodes `/`, hiding a + # `?ref=/phx_...` token behind `%2F` from the `\bph` boundary, and it can grow + # a word past the length window the entropy detector scans. Running the URL + # pass last loses nothing — it only redacts or percent-encodes, so it never + # exposes a credential the detectors could have matched. value = _POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value) - return _redact_secret_tokens(_sanitize_urls(value)) + return _sanitize_urls(_redact_secret_tokens(value)) def _sanitize_resource_name(value: Any) -> Any: @@ -336,6 +340,13 @@ def _redact_secret_tokens(value: str) -> str: def _is_secret(word: str) -> bool: + # Judge the word without this sanitizer's own markers. A value can be + # sanitized twice (a response's content blocks are), and the marker's + # character mix is enough to push a short uri like + # `resource:guide?token=%5Bredacted%5D` over the entropy bar — dropping a + # value we had already made safe. Stripping the marker rather than skipping + # the word keeps a real credential written around one detectable. + word = word.replace(_ENCODED_REDACTED_VALUE, "").replace(_REDACTED_VALUE, "") try: from posthog.exception_utils import _looks_like_secret diff --git a/posthog/test/mcp/test_lowlevel.py b/posthog/test/mcp/test_lowlevel.py index c5842fe61..992c558a3 100644 --- a/posthog/test/mcp/test_lowlevel.py +++ b/posthog/test/mcp/test_lowlevel.py @@ -176,6 +176,12 @@ async def test_list_tools_injects_optional_context_and_captures(): "https://example.com/guide?token=[redacted]", True, ), + # An MCP resource uri need not have an authority. + ( + "resource:guide?token=fakesecret", + "resource:guide?token=%5Bredacted%5D", + False, + ), ], ) async def test_resource_discovery_and_read_are_captured( @@ -285,30 +291,6 @@ async def test_failed_resource_listing_is_captured() -> None: assert "listing unavailable" in json.dumps(exceptions[0]["properties"]) -async def test_authority_less_resource_uri_is_redacted() -> None: - """An MCP resource uri need not have an authority. The captured name keeps the - redacted uri; the captured parameter is dropped whole, because the entropy - detector that guards free-text values reads the rewritten `resource:...` string - as a credential. Either way the token never reaches PostHog.""" - server = make_server() - client = FakeClient() - instrument(server, client) - - await server.request_handlers[mcp_types.ReadResourceRequest]( - mcp_types.ReadResourceRequest( - params=mcp_types.ReadResourceRequestParams( - uri="resource:guide?token=fakesecret" - ) - ) - ) - await _flush() - - props = _events(client, "$mcp_resource_read")[0]["properties"] - assert props["$mcp_resource_name"] == "resource:guide?token=%5Bredacted%5D" - assert props["$mcp_parameters"]["request"]["params"]["uri"] == "[redacted]" - assert "fakesecret" not in json.dumps(client.events) - - async def test_identify_on_a_resource_read_is_named_by_the_uri() -> None: server = make_server() client = FakeClient() diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index fd9c38b3e..99e9af67c 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -174,11 +174,11 @@ def test_sanitize_redacts_large_base64(): "file:/guide.md?token=fakesecret", "file:///guide.md?token=%5Bredacted%5D", ), - # Same rewrite for `resource:guide?token=...` — but the entropy pass that - # follows for free-text values judges the rewritten string a credential and - # drops it whole. The token is gone either way; `$mcp_resource_name`, which - # skips that pass, keeps the readable form (see the resource_name test). - ("resource:guide?token=fakesecret", "[redacted]"), + ("resource:guide?token=fakesecret", "resource:guide?token=%5Bredacted%5D"), + ( + "see:resource:guide?token=fakesecret", + "see:resource:guide?token=%5Bredacted%5D", + ), # An optional authority over-matches prose, which costs nothing: a match # with nothing to redact is returned byte-for-byte. ("Error: see resource:guide.", "Error: see resource:guide."), @@ -216,10 +216,19 @@ def test_sanitize_redacts_large_base64(): "file:/guide?password=fakepass&url=https://example.com", "file:///guide?password=%5Bredacted%5D&url=https%3A%2F%2Fexample.com", ), - # Same shape, and the entropy pass then drops the rewritten authority-less - # string whole (as above); the credential and the inner userinfo are gone - # either way. - ("resource:g?token=fakesecret+https://fakeuser:fakepass@b", "[redacted]"), + # The `+` decodes to a space, so the inner address is part of the token's + # value and goes with it. + ( + "resource:g?token=fakesecret+https://fakeuser:fakepass@b", + "resource:g?token=%5Bredacted%5D", + ), + # The credential detectors run before the URL pass: rewriting a URL can + # push a word past the window the detector scans, and a known token format + # in a long URL would survive. + ( + "https://example.com/" + "a" * 120 + "?ref=ghp_" + "A" * 36 + "&token=x", + "[redacted]", + ), ( "https://example.com/o'reilly?token=fakesecret", "https://example.com/o'reilly?token=%5Bredacted%5D", diff --git a/posthog/test/mcp/test_v2_lowlevel.py b/posthog/test/mcp/test_v2_lowlevel.py index f865edf51..f55b95f95 100644 --- a/posthog/test/mcp/test_v2_lowlevel.py +++ b/posthog/test/mcp/test_v2_lowlevel.py @@ -215,6 +215,12 @@ async def test_list_tools_injects_optional_context_and_captures(): "https://example.com/guide?token=[redacted]", True, ), + # An MCP resource uri need not have an authority. + ( + "resource:guide?token=fakesecret", + "resource:guide?token=%5Bredacted%5D", + False, + ), ], ) async def test_resource_discovery_and_read_are_captured( @@ -263,28 +269,6 @@ async def test_resource_discovery_and_read_are_captured( assert props["$mcp_protocol_version"] == "2026-07-28" -async def test_authority_less_resource_uri_is_redacted() -> None: - """An MCP resource uri need not have an authority. The captured name keeps the - redacted uri; the captured parameter is dropped whole, because the entropy - detector that guards free-text values reads the rewritten `resource:...` string - as a credential. Either way the token never reaches PostHog.""" - server = make_server() - client = FakeClient() - instrument(server, client) - - await _resource_request( - server, - "resources/read", - mcp_types.ReadResourceRequestParams(uri="resource:guide?token=fakesecret"), - ) - await _flush() - - props = _events(client, "$mcp_resource_read")[0]["properties"] - assert props["$mcp_resource_name"] == "resource:guide?token=%5Bredacted%5D" - assert props["$mcp_parameters"]["request"]["params"]["uri"] == "[redacted]" - assert "fakesecret" not in json.dumps(client.events) - - @pytest.mark.parametrize( "method, listing, listed", [ From 3419fe315d1279bba7a5fbb13624ee3aca994c41 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:25:34 -0300 Subject: [PATCH 18/33] fix(mcp): redact credentials in hash-routed fragments What changed `https://example.com/#/callback?token=fakesecret` parsed its whole fragment as a field list, so the only key was `/callback?token` and nothing matched. A hash-routed URL keeps its route in the fragment: everything up to and including the first `?` is now held back verbatim by `_split_fragment_route` and only the remainder is parsed as fields, with the route restored on re-serialization. The "fragment must contain `=`" gate is unchanged, so `#/callback` and `#section-2` still pass through untouched. How tested Rows added to `test_sanitize_url_credentials` for the routed callback and for the two byte-for-byte cases; the existing `#access_token=...` and `#section-2` rows are unchanged. - `.venv/bin/pytest posthog/test/mcp -q` -> 394 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 366 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 18 +++++++++++++++--- posthog/test/mcp/test_pipeline.py | 8 ++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 45db59a66..2b43f6756 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -121,9 +121,10 @@ def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: query, sanitized_query = _sanitize_url_fields(url.query, nested=nested) # A fragment is only a field list when it looks like one; `#section-2` is # left byte-for-byte rather than re-serialized as `section-2=`. + route, fragment_fields = _split_fragment_route(url.fragment) fragment, sanitized_fragment = ( - _sanitize_url_fields(url.fragment, nested=nested) - if "=" in url.fragment + _sanitize_url_fields(fragment_fields, nested=nested) + if "=" in fragment_fields else ([], []) ) netloc = _redact_userinfo(url.netloc) @@ -154,7 +155,7 @@ def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: urlencode(sanitized_query) if sanitized_query != query else url.query, - urlencode(sanitized_fragment) + route + urlencode(sanitized_fragment) if sanitized_fragment != fragment else url.fragment, ) @@ -165,6 +166,17 @@ def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: return _REDACTED_VALUE + suffix +def _split_fragment_route(fragment: str) -> Tuple[str, str]: + """Split a fragment into its route prefix and its fields. A hash-routed URL + (`#/callback?token=...`) puts the route in the fragment, and parsing the whole + thing as fields yields one key of `/callback?token` that matches nothing. The + route, up to and including the first `?`, stays verbatim.""" + if "?" not in fragment: + return "", fragment + route, _, fields = fragment.partition("?") + return route + "?", fields + + def _split_at_second_address(value: str) -> Optional[int]: """Where a second address starts inside ``value``, or None. Each half is strictly shorter than the whole, so the split recursion terminates.""" diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 99e9af67c..5b2cbaa80 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -111,6 +111,14 @@ def test_sanitize_redacts_large_base64(): "https://app.example.com/cb#access_token=%5Bredacted%5D&token_type=%5Bredacted%5D", ), ("https://example.com/doc#section-2", "https://example.com/doc#section-2"), + # A hash-routed URL puts the route in the fragment: it stays verbatim, and + # only what follows the first `?` is a field list. + ( + "https://example.com/#/callback?token=fakesecret", + "https://example.com/#/callback?token=%5Bredacted%5D", + ), + ("https://example.com/#/docs?page=2", "https://example.com/#/docs?page=2"), + ("https://example.com/#/callback", "https://example.com/#/callback"), ( "https://example.com/x?a=1;token=fakesecret", "https://example.com/x?a=1&token=%5Bredacted%5D", From 757aa3c83d71ac2a764d777b56357cebb35fc4a6 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:28:03 -0300 Subject: [PATCH 19/33] fix(mcp): only treat a leading fragment segment as a route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed `#access_token=fakesecret&next=https://other.test/?page=1` split at the `?` inside the `next` value, so everything before it — the access token included — was held back as a verbatim route. A route comes first or not at all, so the split now only happens when no `=` precedes the `?`; otherwise the fragment is already a field list and is parsed whole. How tested A row for that fragment added to `test_sanitize_url_credentials`; the three hash-route rows and the two plain-fragment rows are unchanged. - `.venv/bin/pytest posthog/test/mcp -q` -> 395 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 367 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 9 ++++++--- posthog/test/mcp/test_pipeline.py | 6 ++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 2b43f6756..1781bc566 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -171,10 +171,13 @@ def _split_fragment_route(fragment: str) -> Tuple[str, str]: (`#/callback?token=...`) puts the route in the fragment, and parsing the whole thing as fields yields one key of `/callback?token` that matches nothing. The route, up to and including the first `?`, stays verbatim.""" - if "?" not in fragment: + route, separator, fields = fragment.partition("?") + # A route comes first or not at all. Once a `=` has appeared the fragment is + # already a field list, and the `?` belongs to one of its values + # (`#access_token=x&next=https://other.test/?page=1`). + if "=" in route: return "", fragment - route, _, fields = fragment.partition("?") - return route + "?", fields + return route + separator, fields def _split_at_second_address(value: str) -> Optional[int]: diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 5b2cbaa80..a7a0f71ad 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -119,6 +119,12 @@ def test_sanitize_redacts_large_base64(): ), ("https://example.com/#/docs?page=2", "https://example.com/#/docs?page=2"), ("https://example.com/#/callback", "https://example.com/#/callback"), + # ... but a `?` that follows a `=` is inside a field's value, not a route. + ( + "https://example.com/#access_token=fakesecret&next=https://other.test/?page=1", + "https://example.com/#access_token=%5Bredacted%5D" + "&next=https%3A%2F%2Fother.test%2F%3Fpage%3D1", + ), ( "https://example.com/x?a=1;token=fakesecret", "https://example.com/x?a=1&token=%5Bredacted%5D", From e9e82dd5e19042abd6665019cc35c97239c60de3 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:28:50 -0300 Subject: [PATCH 20/33] fix(mcp): gate binary intents before redacting their PII What changed `redact_pii` ran first on the intent, so a base64 blob holding a Luhn-valid run got a `[redacted]` spliced into it, stopped matching the base64 pattern, and was captured almost whole instead of as the binary marker. `_sanitize_string` is now split into the size/base64 gate (`_is_binary_blob`) and the text passes (`_sanitize_text`), and the new `sanitize_intent` composes them in the order that holds: binary gate, then PII, then the text passes. `sanitize_event` uses it for `user_intent`; non-string intents take the same path they took before. The docstring records why each step sits where it does: the gate first because splicing a redaction into a blob stops it looking like base64, PII before the URL pass because a rewritten URL percent-encodes the `@` the email pattern needs. How tested The blob intent added as a row to the parametrized intent test; the email-inside- a-URL row and the non-string intent test are unchanged. - `.venv/bin/pytest posthog/test/mcp -q` -> 396 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 368 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 44 ++++++++++++++++++++++--------- posthog/test/mcp/test_pipeline.py | 7 +++++ 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 1781bc566..3a3d41259 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -18,6 +18,7 @@ # dedicated properties: $mcp_intent and $mcp_conversation_id). _INJECTED_ARGUMENT_NAMES = ("context", "conversation_id") _REDACTED_VALUE = "[redacted]" +_BINARY_DATA_MARKER = "[binary data redacted - not supported by PostHog MCP analytics]" _ENCODED_REDACTED_VALUE = "%5Bredacted%5D" _BASE64_PATTERN = re.compile(r"^[A-Za-z0-9+/\n\r]+=*$") _SIZE_GATE = 10_240 @@ -308,8 +309,16 @@ def _should_redact_key(key: str) -> bool: def _sanitize_string(value: str) -> str: - if len(value) >= _SIZE_GATE and _BASE64_PATTERN.match(value): - return "[binary data redacted - not supported by PostHog MCP analytics]" + if _is_binary_blob(value): + return _BINARY_DATA_MARKER + return _sanitize_text(value) + + +def _is_binary_blob(value: str) -> bool: + return len(value) >= _SIZE_GATE and bool(_BASE64_PATTERN.match(value)) + + +def _sanitize_text(value: str) -> str: # Both credential passes run before the URL pass, because rewriting a URL # changes the text they match on: it percent-encodes `/`, hiding a # `?ref=/phx_...` token behind `%2F` from the `\bph` boundary, and it can grow @@ -320,6 +329,22 @@ def _sanitize_string(value: str) -> str: return _sanitize_urls(_redact_secret_tokens(value)) +def sanitize_intent(value: Any) -> Any: + """Sanitize the agent-narrated intent: the binary gate, then structured PII, + then the same passes every captured string gets. + + Order matters in both places. The binary gate runs first because splicing a + redaction into a base64 blob stops it looking like base64, and the blob would + then be captured almost whole instead of as the marker. PII runs before the + URL pass because a rewritten URL percent-encodes the `@` the email pattern + needs to see.""" + if not isinstance(value, str): + return sanitize_captured_value(value) + if _is_binary_blob(value): + return _BINARY_DATA_MARKER + return _sanitize_text(redact_pii(value)) + + def _sanitize_resource_name(value: Any) -> Any: """Sanitize a ``resource_name``: either an identifier (a tool or prompt name) or a resource uri, so only the passes that matter for a uri run. The entropy @@ -480,17 +505,12 @@ def sanitize_event(event: Dict[str, Any]) -> Dict[str, Any]: # The intent comes straight from an agent-narrated `context` string, so it # can contain a secret the LLM read aloud or personal data it narrated about - # the user. Strip structured PII (emails, phone numbers, IPs, cards, SSNs) - # first, while the narration is still raw: the generic pass rewrites any URL - # it finds, and a rewritten query percent-encodes `@`, which would hide - # `?email=alice@example.com` from the email pattern. Then redact it like any - # other captured value. PII redaction is scoped to the intent only — - # structured tool parameters and responses often hold the same shapes as - # legitimate data. + # the user. `sanitize_intent` redacts it like any other captured value and + # additionally strips structured PII (emails, phone numbers, IPs, cards, + # SSNs). PII redaction is scoped to the intent only — structured tool + # parameters and responses often hold the same shapes as legitimate data. if result.get("user_intent") is not None: - result["user_intent"] = sanitize_captured_value( - redact_pii(result["user_intent"]) - ) + result["user_intent"] = sanitize_intent(result["user_intent"]) if result.get("llm_model") is not None: result["llm_model"] = sanitize_captured_value(result["llm_model"]) diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index a7a0f71ad..cf5b34805 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -605,6 +605,13 @@ def test_sanitize_event_redacts_pii_from_intent(): "Open https://example.com/?email=alice@example.com&token=fakesecret", "Open https://example.com/?email=%5Bredacted%5D&token=%5Bredacted%5D", ), + # The binary gate runs before PII: splicing a redaction into a base64 blob + # would stop it looking like base64, and the blob would be captured whole. + ( + "base64-blob-with-a-card-shaped-run", + "AAAA/" * 2052 + "4111111111111111/AAA", + "[binary data redacted - not supported by PostHog MCP analytics]", + ), ], ) def test_sanitize_event_composes_pii_and_token_redaction_on_intent( From 916eadf1692b673c5122e44397ef6371d0690c26 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:33:53 -0300 Subject: [PATCH 21/33] fix(mcp): sanitize an address carried in a plain fragment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed A match ends at the first `#`, so a second address inside the fragment is never split off as one — and a fragment with no `=` was skipped entirely, publishing `[a](https://public.test/#intro)[b](https://user:password@private.test/doc)` with its credentials intact. A fragment that is not a field list is now run through the URL text pass one level deep, and counts as a change when it comes back different. Field-list fragments keep today's handling, and the trailing-suffix rule is untouched: no field was rewritten, so the suffix is re-appended. How tested The credential row and its clean counterpart added to `test_sanitize_url_credentials`; `#section-2`, the hash-route rows and the `#access_token=` row are unchanged. - `.venv/bin/pytest posthog/test/mcp -q` -> 398 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 370 passed, 18 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 16 +++++++++++++--- posthog/test/mcp/test_pipeline.py | 10 ++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 3a3d41259..427f8f517 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -123,16 +123,26 @@ def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: # A fragment is only a field list when it looks like one; `#section-2` is # left byte-for-byte rather than re-serialized as `section-2=`. route, fragment_fields = _split_fragment_route(url.fragment) + is_field_list = "=" in fragment_fields fragment, sanitized_fragment = ( _sanitize_url_fields(fragment_fields, nested=nested) - if "=" in fragment_fields + if is_field_list else ([], []) ) + # A fragment that is not a field list is plain text, and text can carry an + # address of its own. A match runs to the first `#`, so such an address is + # never split off as a second one and this is the only pass that sees it. + plain_fragment = ( + url.fragment + if is_field_list + else _sanitize_urls(url.fragment, nested=False) + ) netloc = _redact_userinfo(url.netloc) - if (netloc, sanitized_query, sanitized_fragment) == ( + if (netloc, sanitized_query, sanitized_fragment, plain_fragment) == ( url.netloc, query, fragment, + url.fragment, ): return value # The split-off punctuation can be the tail of the credential rather than @@ -158,7 +168,7 @@ def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: else url.query, route + urlencode(sanitized_fragment) if sanitized_fragment != fragment - else url.fragment, + else plain_fragment, ) ) + suffix diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index cf5b34805..3cf80ad52 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -111,6 +111,16 @@ def test_sanitize_redacts_large_base64(): "https://app.example.com/cb#access_token=%5Bredacted%5D&token_type=%5Bredacted%5D", ), ("https://example.com/doc#section-2", "https://example.com/doc#section-2"), + # A plain fragment is text, and text can carry an address: a match ends at + # the first `#`, so this pass is the only one that sees that address. + ( + "[a](https://public.test/#intro)[b](https://fakeuser:fakepass@private.test/doc)", + "[a](https://public.test/#intro)[b](https://%5Bredacted%5D@private.test/doc)", + ), + ( + "[a](https://public.test/#intro)[b](https://private.test/doc)", + "[a](https://public.test/#intro)[b](https://private.test/doc)", + ), # A hash-routed URL puts the route in the fragment: it stays verbatim, and # only what follows the first `?` is a field list. ( From 19a696c0e9893f466b47bf54a81899f523431933 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:36:11 -0300 Subject: [PATCH 22/33] fix(mcp): report what a failing resource read actually raised MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed mcp 2.x wraps a failing read twice — `UnexpectedResourceError`, re-raised as the shared `MCPError` — and masks the handler's message out of both, so a `TimeoutError('storage backend timed out')` reached PostHog as `$mcp_error_type: MCPError` / `$mcp_error_message: Error reading resource file:///guide.md`. `_primary_exception` already steps past consecutive tool dispatch wrappers; the wrapper table now covers the resource pair too, so the scalars land on the handler's own exception. The `$exception` sibling keeps the whole chain as before, and the wrapper is still re-raised to the caller, so dispatch semantics are unchanged. mcp 1.x's `ResourceError` is deliberately NOT in the table: it keeps the handler's message in its own text, so reporting it loses nothing. How tested New `test_failed_read_reports_the_handler_failure` in `test_resources.py`, which runs against every high-level adapter on both SDK majors: the caller still gets the SDK wrapper, the message carries `storage backend timed out` on both, and the type is `TimeoutError` on v2 / the unchanged `ResourceError` on v1. Verified it fails on v2 without the wrapper-table change. - `.venv/bin/pytest posthog/test/mcp -q` -> 400 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 371 passed, 19 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_posthog_events.py | 37 ++++++++++++++++++++---------- posthog/test/mcp/test_resources.py | 29 +++++++++++++++++++++++ 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/posthog/mcp/_posthog_events.py b/posthog/mcp/_posthog_events.py index 689746e84..e380d598b 100644 --- a/posthog/mcp/_posthog_events.py +++ b/posthog/mcp/_posthog_events.py @@ -161,22 +161,33 @@ def _add_common_properties(event: Event, properties: Dict[str, Any]) -> None: properties["$set"] = {**identify_actor_data} -_TOOL_DISPATCH_WRAPPERS = ("ToolError", "UnexpectedToolError") +# The dispatch wrappers that say nothing the event does not already say: the +# exception names each one surfaces as, and the message it stamps. The message is +# what makes a match specific — `MCPError` is the shared base class mcp 2.x +# re-raises a failed read as, and the resource path stacks two wrappers. mcp 1.x's +# own `ResourceError` is deliberately absent: it keeps the handler's message in +# its text, so reporting it loses nothing. +_DISPATCH_WRAPPERS = ( + (("ToolError", "UnexpectedToolError"), "Error executing tool"), + (("MCPError", "UnexpectedResourceError"), "Error reading resource"), +) # Where the SDKs define their dispatch wrappers: mcp.server.fastmcp.exceptions -# (mcp 1.x), mcp.server.mcpserver.exceptions (mcp 2.x), fastmcp.exceptions -# (standalone fastmcp). An application's own exception carries its own module, -# so a matching name alone must not unwrap it. +# (mcp 1.x), mcp.server.mcpserver.exceptions and mcp.shared.exceptions (mcp 2.x), +# fastmcp.exceptions (standalone fastmcp). An application's own exception carries +# its own module, so a matching name alone must not unwrap it. _SDK_MODULE_PREFIXES = ("mcp.", "fastmcp.") def _is_dispatch_wrapper(entry: Any) -> bool: if not isinstance(entry, dict): return False - return ( - entry.get("type") in _TOOL_DISPATCH_WRAPPERS - and str(entry.get("module") or "").startswith(_SDK_MODULE_PREFIXES) - and str(entry.get("value", "")).startswith("Error executing tool") + if not str(entry.get("module") or "").startswith(_SDK_MODULE_PREFIXES): + return False + value = str(entry.get("value", "")) + return any( + entry.get("type") in names and value.startswith(message) + for names, message in _DISPATCH_WRAPPERS ) @@ -186,10 +197,12 @@ def _primary_exception(error: Any) -> Dict[str, Any]: The MCP SDK's tool dispatch re-raises whatever a tool raised as a ``ToolError`` whose message starts ``Error executing tool ``, and mcp >= 2.1 masks the original text out of that message entirely, keeping - it only on ``__cause__`` — the next entry of the chain here. The wrapper - says nothing the event's tool name does not already say, so the scalars - step past every consecutive wrapper (a tool invoking a failing tool is - wrapped once per dispatch) to the first real exception. + it only on ``__cause__`` — the next entry of the chain here. Resource + dispatch does the same on mcp 2.x, with two stacked wrappers and the uri in + place of the tool name. The wrapper says nothing the event's tool or resource + name does not already say, so the scalars step past every consecutive wrapper + (a tool invoking a failing tool is wrapped once per dispatch) to the first + real exception. """ if not isinstance(error, dict): return {} diff --git a/posthog/test/mcp/test_resources.py b/posthog/test/mcp/test_resources.py index bb6bf24a4..19a0100b5 100644 --- a/posthog/test/mcp/test_resources.py +++ b/posthog/test/mcp/test_resources.py @@ -126,3 +126,32 @@ async def profile(user_id: str) -> str: assert props["$mcp_parameters"]["request"]["method"] == "resources/templates/list" assert listed_uris(props["$mcp_response"]) == ["users://{user_id}/profile"] assert props["$mcp_is_error"] is False + + +async def test_failed_read_reports_the_handler_failure(server) -> None: + """The SDK wraps a failing read in its own error before it reaches the caller. + The captured failure detail follows that chain to what the handler actually + raised, while the caller still receives the SDK's wrapper unchanged.""" + client = FakeClient() + + @server.resource("file:///guide.md") + async def guide() -> str: + raise TimeoutError("storage backend timed out") + + instrument(server, client) + + with pytest.raises(Exception, match="Error reading resource"): + await dispatch( + server, + "resources/read", + mcp_types.ReadResourceRequestParams(uri="file:///guide.md"), + ) + await flush_background() + + props = events_named(client, "$mcp_resource_read")[0]["properties"] + assert props["$mcp_is_error"] is True + assert "storage backend timed out" in props["$mcp_error_message"] + # v2 masks the handler's message out of its wrapper, so the scalars step past + # it. v1's wrapper keeps that message, and is reported as it always was. + expected_type = "TimeoutError" if MCP_MAJOR >= 2 else "ResourceError" + assert props["$mcp_error_type"] == expected_type From 6c980c6b0f7fb4325a39fc06215d6cce25d5352a Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:42:27 -0300 Subject: [PATCH 23/33] fix(mcp): sanitize route prefixes and bound both URL recursions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed - Route prefix leak. `#https://user:password@private.test/doc?page=1` splits at a `?` that precedes any `=`, and the route before it was kept verbatim — with its credentials. The route now goes through the same text pass a plain fragment gets, and the fragment is re-serialized when either the route or the fields changed. `_split_fragment_route` returns the route, the `?` and the fields separately, so the route is sanitized as text while the fields are re-encoded. - Fragment recursion. A `#`-chained uri (`resource:x#resource:x#...`) recursed once per `#`, to RecursionError. The fragment text passes now take the same one-level budget as a nested field value: past it, text still carrying an address is replaced with the marker instead of descended into. Depth is at most two. - Address-split recursion. Splitting a match at its second address recursed once per address. `_split_addresses` now cuts the whole match into pieces in one pass and `_sanitize_single_url` (the old non-splitting body) handles each. No piece can need splitting again: every piece but the last ends before the first `?`/`#`, and in the last piece a remaining authority sits in field data. How tested Two rows for the route prefix, and the two pathological chains asserted for their exact output. Before this commit the `#`-chain raised RecursionError; the address-chain returned a bare `[redacted]` because the length bound fired ahead of the recursion (a shorter one recursed ~470 deep and worked), so Python never crashed on that one — JS, with its own stack limit, is the reason both are covered. Every existing row is unchanged. - `.venv/bin/pytest posthog/test/mcp -q` -> 404 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 375 passed, 19 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Notes for the reviewer The length bound moved from the whole match to the individual piece, which is what lets a long run of short addresses be sanitized rather than dropped whole. Work stays linear in the input: each piece is parsed once and its field count is still bounded. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 114 +++++++++++++++++------------- posthog/test/mcp/test_pipeline.py | 22 ++++++ 2 files changed, 86 insertions(+), 50 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 427f8f517..e9fe884a0 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -11,7 +11,7 @@ from __future__ import annotations import re -from typing import Any, Dict, List, Optional, Tuple +from typing import Any, Dict, List, Tuple from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit # SDK-injected arguments stripped from captured $mcp_parameters (they surface as @@ -101,48 +101,65 @@ def _sanitize_urls(text: str, *, nested: bool = True) -> str: def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: + return "".join( + _sanitize_single_url(piece, nested=nested, in_prose=in_prose) + for piece in _split_addresses(value) + ) + + +def _split_addresses(value: str) -> List[str]: + """Cut a match into the addresses it runs together. + + One match can hold a prose word in front of the address (`URL:https://...`, + `a:b:https://...`) or several addresses joined without whitespace + (`/doc,https://...`). Each address after the first begins inside what would + parse as its predecessor's path, where nothing — its userinfo least of all — + is redacted. An authority AFTER the first `?` or `#` is a query or fragment + value instead, which the field and fragment passes already handle. + + One pass is enough: every piece but the last ends before the first `?`/`#`, so + it holds neither, and in the last piece every remaining authority sits in + field data. + """ + boundary = min( + (value.index(char) for char in "?#" if char in value), default=len(value) + ) + starts = [ + match.start() + for match in _URL_AUTHORITY_SEARCH.finditer(value) + if 0 < match.start() < boundary + ] + if not starts: + return [value] + return [value[begin:end] for begin, end in zip([0] + starts, starts + [len(value)])] + + +def _sanitize_single_url(value: str, *, nested: bool, in_prose: bool) -> str: if len(value) > _MAX_URL_LENGTH and _URL_AUTHORITY_PATTERN.match(value): return _REDACTED_VALUE - # One match can hold a prose word in front of the address (`URL:https://...`, - # `a:b:https://...`) or two addresses run together (`/doc,https://...`). Either - # way the second address begins inside what would parse as the first one's - # path, where nothing — its userinfo least of all — is redacted. So the match - # is split at that authority and each part sanitized on its own. An authority - # AFTER the first `?` or `#` is a query or fragment value instead, which the - # field pass already handles (a sensitive key, or the nested pass). - split = _split_at_second_address(value) - if split is not None: - return _sanitize_url( - value[:split], nested=nested, in_prose=in_prose - ) + _sanitize_url(value[split:], nested=nested, in_prose=in_prose) url_text = value.rstrip(_URL_TRAILING_PUNCTUATION) if in_prose else value suffix = value[len(url_text) :] try: url = urlsplit(url_text) query, sanitized_query = _sanitize_url_fields(url.query, nested=nested) # A fragment is only a field list when it looks like one; `#section-2` is - # left byte-for-byte rather than re-serialized as `section-2=`. - route, fragment_fields = _split_fragment_route(url.fragment) + # left byte-for-byte rather than re-serialized as `section-2=`. What is not + # a field list — a route prefix, or the whole fragment — is plain text. + route, separator, fragment_fields = _split_fragment_route(url.fragment) is_field_list = "=" in fragment_fields fragment, sanitized_fragment = ( _sanitize_url_fields(fragment_fields, nested=nested) if is_field_list else ([], []) ) - # A fragment that is not a field list is plain text, and text can carry an - # address of its own. A match runs to the first `#`, so such an address is - # never split off as a second one and this is the only pass that sees it. - plain_fragment = ( - url.fragment - if is_field_list - else _sanitize_urls(url.fragment, nested=False) - ) + text = route if is_field_list else url.fragment + sanitized_text = _sanitize_fragment_text(text, nested=nested) netloc = _redact_userinfo(url.netloc) - if (netloc, sanitized_query, sanitized_fragment, plain_fragment) == ( + if (netloc, sanitized_query, sanitized_fragment, sanitized_text) == ( url.netloc, query, fragment, - url.fragment, + text, ): return value # The split-off punctuation can be the tail of the credential rather than @@ -166,9 +183,9 @@ def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: urlencode(sanitized_query) if sanitized_query != query else url.query, - route + urlencode(sanitized_fragment) + sanitized_text + separator + urlencode(sanitized_fragment) if sanitized_fragment != fragment - else plain_fragment, + else sanitized_text + separator + fragment_fields, ) ) + suffix @@ -177,34 +194,31 @@ def _sanitize_url(value: str, *, nested: bool, in_prose: bool) -> str: return _REDACTED_VALUE + suffix -def _split_fragment_route(fragment: str) -> Tuple[str, str]: - """Split a fragment into its route prefix and its fields. A hash-routed URL - (`#/callback?token=...`) puts the route in the fragment, and parsing the whole - thing as fields yields one key of `/callback?token` that matches nothing. The - route, up to and including the first `?`, stays verbatim.""" +def _sanitize_fragment_text(text: str, *, nested: bool) -> str: + """Sanitize the plain-text part of a fragment: a route prefix, or a fragment + that is not a field list. Text can carry an address of its own, and a match + ends at the first `#`, so this is the only pass that sees it. It gets the same + one-level budget as a nested field value — past it, text still carrying an + address is dropped rather than trusted, which is also what stops a + `#`-chained uri from recursing without end.""" + if nested: + return _sanitize_urls(text, nested=False) + return _REDACTED_VALUE if _URL_PATTERN.search(text) else text + + +def _split_fragment_route(fragment: str) -> Tuple[str, str, str]: + """Split a fragment into its route, the `?` that ends the route, and its + fields. A hash-routed URL (`#/callback?token=...`) puts the route in the + fragment, and parsing the whole thing as fields yields one key of + `/callback?token` that matches nothing. The three parts concatenate back to + the fragment, so the route keeps its own text while the fields are re-encoded.""" route, separator, fields = fragment.partition("?") # A route comes first or not at all. Once a `=` has appeared the fragment is # already a field list, and the `?` belongs to one of its values # (`#access_token=x&next=https://other.test/?page=1`). if "=" in route: - return "", fragment - return route + separator, fields - - -def _split_at_second_address(value: str) -> Optional[int]: - """Where a second address starts inside ``value``, or None. Each half is - strictly shorter than the whole, so the split recursion terminates.""" - boundary = min( - (value.index(char) for char in "?#" if char in value), default=len(value) - ) - return next( - ( - match.start() - for match in _URL_AUTHORITY_SEARCH.finditer(value) - if 0 < match.start() < boundary - ), - None, - ) + return "", "", fragment + return route, separator, fields def _redact_userinfo(netloc: str) -> str: diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 3cf80ad52..3106ac27a 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -121,6 +121,28 @@ def test_sanitize_redacts_large_base64(): "[a](https://public.test/#intro)[b](https://private.test/doc)", "[a](https://public.test/#intro)[b](https://private.test/doc)", ), + # A route prefix is that same plain text: it is kept verbatim, so it has to + # be sanitized too. + ( + "https://public.test/#https://fakeuser:fakepass@private.test/doc?page=1", + "https://public.test/#https://%5Bredacted%5D@private.test/doc?page=1", + ), + ( + "[a](https://public.test/#intro)[b](https://fakeuser:fakepass@private.test/doc?page=1)", + "[a](https://public.test/#intro)[b](https://%5Bredacted%5D@private.test/doc?page=1)", + ), + # Neither the fragment text pass nor the address split may recurse per URL: + # both of these are one match, and both used to grow the stack with it. + pytest.param( + "resource:x#" * 10_000 + "intro", + "resource:x#resource:x#[redacted]", + id="fragment-chain", + ), + pytest.param( + "https://a.test/x," * 10_000 + "https://fakeuser:fakepass@b.test/doc", + "https://a.test/x," * 10_000 + "https://%5Bredacted%5D@b.test/doc", + id="address-chain", + ), # A hash-routed URL puts the route in the fragment: it stays verbatim, and # only what follows the first `?` is a field list. ( From 84655d0f81009beb827c63960a1ce338d3e5506d Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:48:19 -0300 Subject: [PATCH 24/33] fix(mcp): recognize a hash route that carries its own `=` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed `#/docs/id=1?token=fakesecret` was read as one field named `/docs/id` with a value of `1?token=fakesecret`, so the token survived. A fragment is now a route when it reads as a path (starts with `/`) and has a `?`, or when nothing field-shaped precedes that `?` — the previous rule, which still covers `#/callback?k=v` and `#https://user:pw@host/doc?page=1`. Failing both, it is a field list when it holds a `=`, and plain text otherwise. A query key's segments are now also `/`-delimited, so the `/token` of a `#/token=fakesecret` fragment is recognized as the credential name it is. This widens redaction for every key with a `/` in it, in queries as well as fragments (`a/token`, `sort/key`) — the same over-redaction trade the segment rule already documents. How tested Rows added for the route with a `=`, its byte-for-byte counterpart without a `?`, and the leading-slash field list; the existing route, field-list and plain-fragment rows are unchanged. - `.venv/bin/pytest posthog/test/mcp -q` -> 407 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 378 passed, 19 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Notes for the reviewer `#/token=fakesecret` comes back as `#%2Ftoken=%5Bredacted%5D`, not `#/token=%5Bredacted%5D`: re-serializing a field percent-encodes a `/` in its key. `URLSearchParams` does the same, so the two SDKs still agree byte for byte. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 20 ++++++++++++-------- posthog/test/mcp/test_pipeline.py | 13 +++++++++++++ 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index e9fe884a0..0f18a8ba9 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -60,13 +60,13 @@ _URL_AUTHORITY_SEARCH = re.compile(r"[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) _MAX_URL_LENGTH = 8192 _MAX_URL_QUERY_FIELDS = 128 -# A query key is sensitive when ANY `-`/`_`/`.`-delimited segment matches, which +# A query key is sensitive when ANY `-`/`_`/`.`/`/`-delimited segment matches, which # covers the compound names credentials actually travel under: `private_token`, # `oauth_signature`, `id_token`, `subscription-key`, `X-Amz-Security-Token`. # Over-redacting a benign `sort_key` is the accepted trade for an analytics payload. _SENSITIVE_QUERY_SEGMENT_PATTERN = re.compile( - r"(^|[-_.])(auth|token|secret|password|passwd|pwd|credential|signature|sig|" - r"key|hmac|sas|bearer|jwt|session|sessionid)([-_.]|$)", + r"(^|[-_./])(auth|token|secret|password|passwd|pwd|credential|signature|sig|" + r"key|hmac|sas|bearer|jwt|session|sessionid)([-_./]|$)", re.IGNORECASE, ) # Matched whole rather than per segment: `code` (an OAuth authorization code) as a @@ -213,12 +213,16 @@ def _split_fragment_route(fragment: str) -> Tuple[str, str, str]: `/callback?token` that matches nothing. The three parts concatenate back to the fragment, so the route keeps its own text while the fields are re-encoded.""" route, separator, fields = fragment.partition("?") - # A route comes first or not at all. Once a `=` has appeared the fragment is - # already a field list, and the `?` belongs to one of its values - # (`#access_token=x&next=https://other.test/?page=1`). - if "=" in route: + # A route ends at the first `?`, and there is one when the fragment either + # reads as a path (`#/docs/id=1?token=...`) or has nothing field-shaped in + # front of that `?` (`#/callback?token=...`). Otherwise the `?` belongs to a + # field's value (`#access_token=x&next=https://other.test/?page=1`), and the + # fragment is a field list — or plain text when it holds no fields at all. + if separator and (fragment.startswith("/") or "=" not in route): + return route, separator, fields + if "=" in fragment: return "", "", fragment - return route, separator, fields + return fragment, "", "" def _redact_userinfo(netloc: str) -> str: diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 3106ac27a..d1c54342f 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -151,6 +151,19 @@ def test_sanitize_redacts_large_base64(): ), ("https://example.com/#/docs?page=2", "https://example.com/#/docs?page=2"), ("https://example.com/#/callback", "https://example.com/#/callback"), + # A route can hold a `=` of its own: what makes it a route is the path + # shape, not the absence of one. + ( + "https://example.com/#/docs/id=1?token=fakesecret", + "https://example.com/#/docs/id=1?token=%5Bredacted%5D", + ), + ("https://example.com/#/docs/id=1", "https://example.com/#/docs/id=1"), + # No `?`, so this is a field list whose key happens to start with a `/`; + # re-serializing the redacted field percent-encodes that `/`. + ( + "https://example.com/#/token=fakesecret", + "https://example.com/#%2Ftoken=%5Bredacted%5D", + ), # ... but a `?` that follows a `=` is inside a field's value, not a route. ( "https://example.com/#access_token=fakesecret&next=https://other.test/?page=1", From 67a76c6a7828747d7c628384762d202d34532eef Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:53:08 -0300 Subject: [PATCH 25/33] fix(mcp): split an adjacent address wherever it sits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed `_split_addresses` stopped looking at the first `?` or `#`, so an address that followed one — `[a](https://public.test/?download)[b](https://alice:pw@private.test/doc)` — was absorbed into a query KEY, and keys are never sanitized. The boundary is gone: every authority start past index 0 splits, except one preceded by `=`. An address in value position belongs to the field that holds it and the nested pass sanitizes it there; anywhere else it is simply the next address. The single-pass structure and `_sanitize_single_url` are unchanged, and the docstring now explains value position vs adjacent address. How tested Three rows added (query key, after a comma inside a query, straight after the `?`). Every existing row is unchanged, including both gateway `?url=https://...` rows, `#access_token=…&next=https://…` and `?token=…+https://…` — all value position, none split — and the two pathological inputs still finish in ~10ms. - `.venv/bin/pytest posthog/test/mcp -q` -> 410 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 381 passed, 19 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 21 +++++++++------------ posthog/test/mcp/test_pipeline.py | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 0f18a8ba9..06576c3ac 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -112,22 +112,19 @@ def _split_addresses(value: str) -> List[str]: One match can hold a prose word in front of the address (`URL:https://...`, `a:b:https://...`) or several addresses joined without whitespace - (`/doc,https://...`). Each address after the first begins inside what would - parse as its predecessor's path, where nothing — its userinfo least of all — - is redacted. An authority AFTER the first `?` or `#` is a query or fragment - value instead, which the field and fragment passes already handle. - - One pass is enough: every piece but the last ends before the first `?`/`#`, so - it holds neither, and in the last piece every remaining authority sits in - field data. + (`/doc,https://...`, `/?download)[b](https://...`). Each address after the + first begins inside what would parse as its predecessor's path, query key or + fragment, none of which is redacted. + + The exception is an address in value position, right after a `=` + (`?url=https://...`): it belongs to the field that holds it, and the nested + pass sanitizes it there. Everywhere else an authority starts an adjacent + address, and one pass over the match finds them all. """ - boundary = min( - (value.index(char) for char in "?#" if char in value), default=len(value) - ) starts = [ match.start() for match in _URL_AUTHORITY_SEARCH.finditer(value) - if 0 < match.start() < boundary + if match.start() > 0 and value[match.start() - 1] != "=" ] if not starts: return [value] diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index d1c54342f..ff37f722f 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -263,6 +263,20 @@ def test_sanitize_redacts_large_base64(): "https://example.com/doc,https://fakeuser:fakepass@other.example.com/doc", "https://example.com/doc,https://%5Bredacted%5D@other.example.com/doc", ), + # An adjacent address is adjacent wherever it sits: a query key holds one + # as readily as a path, and a key is never redacted on its own. + ( + "[a](https://public.test/?download)[b](https://fakeuser:fakepass@private.test/doc)", + "[a](https://public.test/?download)[b](https://%5Bredacted%5D@private.test/doc)", + ), + ( + "https://example.com/?q=see,https://fakeuser:fakepass@x.test/doc", + "https://example.com/?q=see,https://%5Bredacted%5D@x.test/doc", + ), + ( + "https://example.com/?https://fakeuser:fakepass@x.test/doc", + "https://example.com/?https://%5Bredacted%5D@x.test/doc", + ), # The closing `)` goes with the redacted trailing field, by the rule above: # punctuation after a rewritten last field may be the credential's own. ( From 2502fb8dc5a091f9276891f4acd212c8e514b08b Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 11:55:17 -0300 Subject: [PATCH 26/33] fix(mcp): judge each half of a fragment on its own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed `#/token=fakesecret&next=https://other.test/?page=1` starts with `/` and holds a `?`, so the route heuristic took everything before that `?` — the token included — as text to keep verbatim. Shape cannot separate that from `#/docs/id=1?token=x`, so the heuristics are gone: a fragment splits at its first `?`, and each half gets the field pass when it holds a `=` and the text pass when it does not. The halves are reassembled around the `?`, each keeping its own encoding. `_split_fragment_route` is replaced by `_sanitize_fragment_part`, which also reports whether it rewrote its last field, and `_rewrote_the_last_field` keeps the trailing-punctuation rule readable now that it has two halves to consider. How tested The new fragment row asserts the leading-slash field list with a nested address; the existing `#access_token=...&next=...` row changes as expected (its half is re-serialized on its own, so the `?page=1` after it stays verbatim rather than being encoded into a value). Every other fragment, route, markdown and pathological row keeps its expectation, and the long inputs still run in ~15ms. - `.venv/bin/pytest posthog/test/mcp -q` -> 411 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 382 passed, 19 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 84 ++++++++++++++++--------------- posthog/test/mcp/test_pipeline.py | 19 ++++--- 2 files changed, 56 insertions(+), 47 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 06576c3ac..34bc7e06c 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -12,7 +12,7 @@ import re from typing import Any, Dict, List, Tuple -from urllib.parse import parse_qsl, urlencode, urlsplit, urlunsplit +from urllib.parse import SplitResult, parse_qsl, urlencode, urlsplit, urlunsplit # SDK-injected arguments stripped from captured $mcp_parameters (they surface as # dedicated properties: $mcp_intent and $mcp_conversation_id). @@ -139,24 +139,21 @@ def _sanitize_single_url(value: str, *, nested: bool, in_prose: bool) -> str: try: url = urlsplit(url_text) query, sanitized_query = _sanitize_url_fields(url.query, nested=nested) - # A fragment is only a field list when it looks like one; `#section-2` is - # left byte-for-byte rather than re-serialized as `section-2=`. What is not - # a field list — a route prefix, or the whole fragment — is plain text. - route, separator, fragment_fields = _split_fragment_route(url.fragment) - is_field_list = "=" in fragment_fields - fragment, sanitized_fragment = ( - _sanitize_url_fields(fragment_fields, nested=nested) - if is_field_list - else ([], []) + # A `?` inside a fragment splits it in two, and each half stands on its + # own: `#/callback?token=x` is text then fields, `#a=1?b=2` is fields + # twice. Shape cannot tell them apart — `#/token=x` looks like a route and + # is a field list — so each half is judged only on whether it holds a `=`. + head, separator, fragment_tail = url.fragment.partition("?") + sanitized_head, head_rewrote_last = _sanitize_fragment_part(head, nested=nested) + sanitized_tail, tail_rewrote_last = _sanitize_fragment_part( + fragment_tail, nested=nested ) - text = route if is_field_list else url.fragment - sanitized_text = _sanitize_fragment_text(text, nested=nested) + fragment = sanitized_head + separator + sanitized_tail netloc = _redact_userinfo(url.netloc) - if (netloc, sanitized_query, sanitized_fragment, sanitized_text) == ( + if (netloc, sanitized_query, fragment) == ( url.netloc, query, - fragment, - text, + url.fragment, ): return value # The split-off punctuation can be the tail of the credential rather than @@ -164,10 +161,9 @@ def _sanitize_single_url(value: str, *, nested: bool, in_prose: bool) -> str: # `?password=[redacted]!!!`. So when the last field of the part the URL # ends in was rewritten, its punctuation goes with it. Losing a comma from # the surrounding prose is the accepted cost. - tail, sanitized_tail = ( - (fragment, sanitized_fragment) if url.fragment else (query, sanitized_query) - ) - if tail and sanitized_tail[-1] != tail[-1]: + if _rewrote_the_last_field( + url, query, sanitized_query, head_rewrote_last, tail_rewrote_last + ): suffix = "" # Only the part that changed is re-serialized, so an untouched query or # fragment keeps its original encoding. @@ -180,9 +176,7 @@ def _sanitize_single_url(value: str, *, nested: bool, in_prose: bool) -> str: urlencode(sanitized_query) if sanitized_query != query else url.query, - sanitized_text + separator + urlencode(sanitized_fragment) - if sanitized_fragment != fragment - else sanitized_text + separator + fragment_fields, + fragment, ) ) + suffix @@ -191,6 +185,33 @@ def _sanitize_single_url(value: str, *, nested: bool, in_prose: bool) -> str: return _REDACTED_VALUE + suffix +def _rewrote_the_last_field( + url: SplitResult, + query: _UrlFields, + sanitized_query: _UrlFields, + head_rewrote_last: bool, + tail_rewrote_last: bool, +) -> bool: + """Whether the URL ends in a field whose value was just redacted — the part it + ends in being its fragment when it has one, its query otherwise.""" + if not url.fragment: + return bool(query) and sanitized_query[-1] != query[-1] + return tail_rewrote_last if "?" in url.fragment else head_rewrote_last + + +def _sanitize_fragment_part(text: str, *, nested: bool) -> Tuple[str, bool]: + """Sanitize one half of a fragment: the field pass when it holds a `=`, the + text pass otherwise. Also reports whether its last field was rewritten, which + is what tells the caller that trailing punctuation may belong to the + credential rather than to the surrounding prose.""" + if "=" not in text: + return _sanitize_fragment_text(text, nested=nested), False + fields, sanitized = _sanitize_url_fields(text, nested=nested) + if sanitized == fields: + return text, False + return urlencode(sanitized), bool(fields) and sanitized[-1] != fields[-1] + + def _sanitize_fragment_text(text: str, *, nested: bool) -> str: """Sanitize the plain-text part of a fragment: a route prefix, or a fragment that is not a field list. Text can carry an address of its own, and a match @@ -203,25 +224,6 @@ def _sanitize_fragment_text(text: str, *, nested: bool) -> str: return _REDACTED_VALUE if _URL_PATTERN.search(text) else text -def _split_fragment_route(fragment: str) -> Tuple[str, str, str]: - """Split a fragment into its route, the `?` that ends the route, and its - fields. A hash-routed URL (`#/callback?token=...`) puts the route in the - fragment, and parsing the whole thing as fields yields one key of - `/callback?token` that matches nothing. The three parts concatenate back to - the fragment, so the route keeps its own text while the fields are re-encoded.""" - route, separator, fields = fragment.partition("?") - # A route ends at the first `?`, and there is one when the fragment either - # reads as a path (`#/docs/id=1?token=...`) or has nothing field-shaped in - # front of that `?` (`#/callback?token=...`). Otherwise the `?` belongs to a - # field's value (`#access_token=x&next=https://other.test/?page=1`), and the - # fragment is a field list — or plain text when it holds no fields at all. - if separator and (fragment.startswith("/") or "=" not in route): - return route, separator, fields - if "=" in fragment: - return "", "", fragment - return fragment, "", "" - - def _redact_userinfo(netloc: str) -> str: if "@" not in netloc: return netloc diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index ff37f722f..6a43d9e41 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -151,24 +151,31 @@ def test_sanitize_redacts_large_base64(): ), ("https://example.com/#/docs?page=2", "https://example.com/#/docs?page=2"), ("https://example.com/#/callback", "https://example.com/#/callback"), - # A route can hold a `=` of its own: what makes it a route is the path - # shape, not the absence of one. + # Shape says nothing: this half of the fragment holds a `=`, so it is read + # as fields even though it looks like a path. ( "https://example.com/#/docs/id=1?token=fakesecret", "https://example.com/#/docs/id=1?token=%5Bredacted%5D", ), ("https://example.com/#/docs/id=1", "https://example.com/#/docs/id=1"), - # No `?`, so this is a field list whose key happens to start with a `/`; - # re-serializing the redacted field percent-encodes that `/`. + # A field list whose key happens to start with a `/`; re-serializing the + # redacted field percent-encodes that `/`. ( "https://example.com/#/token=fakesecret", "https://example.com/#%2Ftoken=%5Bredacted%5D", ), - # ... but a `?` that follows a `=` is inside a field's value, not a route. + # A `?` splits a fragment in two, and each half is a field list or plain + # text on its own terms — the half before the `?` here is fields, and the + # half after keeps its own encoding. ( "https://example.com/#access_token=fakesecret&next=https://other.test/?page=1", "https://example.com/#access_token=%5Bredacted%5D" - "&next=https%3A%2F%2Fother.test%2F%3Fpage%3D1", + "&next=https%3A%2F%2Fother.test%2F?page=1", + ), + ( + "https://example.com/#/token=fakesecret&next=https://other.test/?page=1", + "https://example.com/#%2Ftoken=%5Bredacted%5D" + "&next=https%3A%2F%2Fother.test%2F?page=1", ), ( "https://example.com/x?a=1;token=fakesecret", From d867631e63bd622b89570cdf4f7add30017a1b83 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 12:00:11 -0300 Subject: [PATCH 27/33] fix(mcp): decide value position by the nearest structural character MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed `?token=foo%20https://secret.test/private` split at the inner authority because only the character right before it was checked, cutting the token's value in two and publishing the tail beside the redaction. The decision now scans back to the nearest character of `=&;?#/`: a `=` means the authority is inside a field's value, so the nested pass handles it there; a field separator, a `/`, or nothing at all means a new address begins. `/` is in the set so a `=` inside a path (`/a=b/c,https://...`) does not read as a field. How tested Rows added for the split value, for the path-with-`=`, and for the comma inside a query — that last one is now value position, so it goes through the nested pass and is re-encoded as part of its field (`q=see%2Chttps%3A%2F%2F%255Bredacted...`) rather than being split off. Every other row is unchanged, gateway and pathological inputs included; the long inputs still run in ~16ms, since each backward scan stops at the previous address's own `/`. - `.venv/bin/pytest posthog/test/mcp -q` -> 413 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 384 passed, 19 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 25 ++++++++++++++++++++----- posthog/test/mcp/test_pipeline.py | 14 +++++++++++++- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 34bc7e06c..fd9616c97 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -58,6 +58,8 @@ # deliberately keeps those, and the field count is already bounded when parsing. _URL_AUTHORITY_PATTERN = re.compile(r"^[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) _URL_AUTHORITY_SEARCH = re.compile(r"[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) +# What separates one field, or one path segment, from the next. +_FIELD_STRUCTURE_CHARACTERS = "=&;?#/" _MAX_URL_LENGTH = 8192 _MAX_URL_QUERY_FIELDS = 128 # A query key is sensitive when ANY `-`/`_`/`.`/`/`-delimited segment matches, which @@ -116,21 +118,34 @@ def _split_addresses(value: str) -> List[str]: first begins inside what would parse as its predecessor's path, query key or fragment, none of which is redacted. - The exception is an address in value position, right after a `=` - (`?url=https://...`): it belongs to the field that holds it, and the nested - pass sanitizes it there. Everywhere else an authority starts an adjacent - address, and one pass over the match finds them all. + The exception is an address in value position (`?url=https://...`, + `?token=foo%20https://...`): it belongs to the field that holds it, and the + nested pass sanitizes it there — splitting it off would cut the field's value + in two and publish the tail. What decides is the nearest structural character + before the authority: a `=` means the authority is inside a field's value, + while a `&`, `;`, `?`, `#`, `/` — or nothing at all — means a new address + begins. `/` is in that set so a `=` in a path (`/a=b/c,https://...`) does not + read as a field. One pass over the match finds them all. """ starts = [ match.start() for match in _URL_AUTHORITY_SEARCH.finditer(value) - if match.start() > 0 and value[match.start() - 1] != "=" + if match.start() > 0 and not _in_value_position(value, match.start()) ] if not starts: return [value] return [value[begin:end] for begin, end in zip([0] + starts, starts + [len(value)])] +def _in_value_position(value: str, start: int) -> bool: + """Whether the authority at ``start`` sits inside a field's value: the nearest + structural character before it is a `=`.""" + for index in range(start - 1, -1, -1): + if value[index] in _FIELD_STRUCTURE_CHARACTERS: + return value[index] == "=" + return False + + def _sanitize_single_url(value: str, *, nested: bool, in_prose: bool) -> str: if len(value) > _MAX_URL_LENGTH and _URL_AUTHORITY_PATTERN.match(value): return _REDACTED_VALUE diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 6a43d9e41..d48439a17 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -276,9 +276,21 @@ def test_sanitize_redacts_large_base64(): "[a](https://public.test/?download)[b](https://fakeuser:fakepass@private.test/doc)", "[a](https://public.test/?download)[b](https://%5Bredacted%5D@private.test/doc)", ), + # ... unless it sits in a field's value, where splitting it off would cut + # that value in two and publish the tail. The nearest structural character + # before the authority decides: `=` means value, `/` and the field + # separators mean a new address. + ( + "https://host/x?token=foo%20https://secret.test/private", + "https://host/x?token=%5Bredacted%5D", + ), ( "https://example.com/?q=see,https://fakeuser:fakepass@x.test/doc", - "https://example.com/?q=see,https://%5Bredacted%5D@x.test/doc", + "https://example.com/?q=see%2Chttps%3A%2F%2F%255Bredacted%255D%40x.test%2Fdoc", + ), + ( + "https://host/a=b/c,https://fakeuser:fakepass@x.test/doc", + "https://host/a=b/c,https://%5Bredacted%5D@x.test/doc", ), ( "https://example.com/?https://fakeuser:fakepass@x.test/doc", From b7179b90bd69b21a70cae8d18d04278e5ac0c0e3 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 12:02:36 -0300 Subject: [PATCH 28/33] fix(mcp): bound value position to the fields region, fail closed on a split credential MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed - A `=` in the path is not a field. `https://example.com/redirect=https://user:pw@host/doc` kept the inner address attached, so nothing sanitized it. Value position now only exists past the first `?` or `#`: before that, an authority always starts its own address. With the region check in place, `/` leaves `_FIELD_STRUCTURE_CHARACTERS` — the path case it was there for is covered. - A `?` can be a character of a credential. `#password=prefix?fakesecret` split into a redacted head and a tail that published the rest of the password. Nothing can tell that `?` from a real boundary, so when the head ends in a value just redacted, the whole tail is replaced with the marker rather than sanitized. How tested Rows added for the path `=`, and for a fragment credential split by a `?` both with and without field-shaped text after it; `#/docs/id=1?token=...` still sanitizes its tail normally, since that head's last field is not sensitive. Every other row is unchanged and the pathological inputs still run in ~15ms. - `.venv/bin/pytest posthog/test/mcp -q` -> 416 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 387 passed, 19 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 32 +++++++++++++++++++++---------- posthog/test/mcp/test_pipeline.py | 17 ++++++++++++++++ 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index fd9616c97..c142c813e 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -58,8 +58,8 @@ # deliberately keeps those, and the field count is already bounded when parsing. _URL_AUTHORITY_PATTERN = re.compile(r"^[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) _URL_AUTHORITY_SEARCH = re.compile(r"[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) -# What separates one field, or one path segment, from the next. -_FIELD_STRUCTURE_CHARACTERS = "=&;?#/" +# What separates one field from the next inside a query or fragment. +_FIELD_STRUCTURE_CHARACTERS = "=&;?#" _MAX_URL_LENGTH = 8192 _MAX_URL_QUERY_FIELDS = 128 # A query key is sensitive when ANY `-`/`_`/`.`/`/`-delimited segment matches, which @@ -121,16 +121,23 @@ def _split_addresses(value: str) -> List[str]: The exception is an address in value position (`?url=https://...`, `?token=foo%20https://...`): it belongs to the field that holds it, and the nested pass sanitizes it there — splitting it off would cut the field's value - in two and publish the tail. What decides is the nearest structural character - before the authority: a `=` means the authority is inside a field's value, - while a `&`, `;`, `?`, `#`, `/` — or nothing at all — means a new address - begins. `/` is in that set so a `=` in a path (`/a=b/c,https://...`) does not - read as a field. One pass over the match finds them all. + in two and publish the tail. Only the fields region has values, so an address + before the first `?` or `#` is always its own, `=` in the path or not + (`/redirect=https://user:pw@host/doc`). Inside that region the nearest + structural character decides: a `=` puts the authority in a value, a field + separator — or nothing at all — starts a new address. One pass over the match + finds them all. """ + fields_start = min( + (value.index(char) for char in "?#" if char in value), default=len(value) + ) starts = [ match.start() for match in _URL_AUTHORITY_SEARCH.finditer(value) - if match.start() > 0 and not _in_value_position(value, match.start()) + if match.start() > 0 + and ( + match.start() < fields_start or not _in_value_position(value, match.start()) + ) ] if not starts: return [value] @@ -160,8 +167,13 @@ def _sanitize_single_url(value: str, *, nested: bool, in_prose: bool) -> str: # is a field list — so each half is judged only on whether it holds a `=`. head, separator, fragment_tail = url.fragment.partition("?") sanitized_head, head_rewrote_last = _sanitize_fragment_part(head, nested=nested) - sanitized_tail, tail_rewrote_last = _sanitize_fragment_part( - fragment_tail, nested=nested + # A `?` can fall inside a credential (`#password=pre?fix`), and nothing + # here can tell that from a real boundary. When the head ends in a value + # just redacted, the tail may be the rest of it, so it goes too. + sanitized_tail, tail_rewrote_last = ( + (_REDACTED_VALUE, True) + if head_rewrote_last and fragment_tail + else _sanitize_fragment_part(fragment_tail, nested=nested) ) fragment = sanitized_head + separator + sanitized_tail netloc = _redact_userinfo(url.netloc) diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index d48439a17..a8fbd8e7b 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -157,6 +157,17 @@ def test_sanitize_redacts_large_base64(): "https://example.com/#/docs/id=1?token=fakesecret", "https://example.com/#/docs/id=1?token=%5Bredacted%5D", ), + # ... but when the half before the `?` ends in a value we just redacted, + # that `?` may be a character of the credential rather than a boundary, so + # what follows it goes too. + ( + "https://example.com/#password=prefix?fakesecret", + "https://example.com/#password=%5Bredacted%5D?[redacted]", + ), + ( + "https://example.com/#password=prefix?token=x&page=1", + "https://example.com/#password=%5Bredacted%5D?[redacted]", + ), ("https://example.com/#/docs/id=1", "https://example.com/#/docs/id=1"), # A field list whose key happens to start with a `/`; re-serializing the # redacted field percent-encodes that `/`. @@ -292,6 +303,12 @@ def test_sanitize_redacts_large_base64(): "https://host/a=b/c,https://fakeuser:fakepass@x.test/doc", "https://host/a=b/c,https://%5Bredacted%5D@x.test/doc", ), + # Only the fields region holds values, so a `=` in the path never makes + # the address that follows it part of one. + ( + "https://example.com/redirect=https://fakeuser:fakepass@private.example.com/doc", + "https://example.com/redirect=https://%5Bredacted%5D@private.example.com/doc", + ), ( "https://example.com/?https://fakeuser:fakepass@x.test/doc", "https://example.com/?https://%5Bredacted%5D@x.test/doc", From d3ec6c84786fe14edb4512ec25afd5875f8edfd5 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 12:07:21 -0300 Subject: [PATCH 29/33] fix(mcp): find value position in one forward pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed `_in_value_position` scanned backwards from every authority to the nearest structural character. Once `/` left that set, a value whose addresses all sit after the same `?` made every scan run back to it: `"https://a.test/?" + "https://b.test/x," * 4000` (68 KB) took 2.2 s, synchronous on the server's event loop. `_authority_starts` now walks the value once, pairing each authority start with the last structural character seen before it, and `_split_addresses` reads value position off that. Same decisions, linear time — the same input is 7 ms. How tested `test_sanitize_url_is_not_quadratic_on_many_addresses` asserts that input comes back unchanged in under a second, next to the existing quadratic-PII test. Every row in the URL table keeps its expectation, and the two other pathological inputs still run in ~10ms and ~19ms. - `.venv/bin/pytest posthog/test/mcp -q` -> 417 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 388 passed, 19 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 35 ++++++++++++++++++------------- posthog/test/mcp/test_pipeline.py | 13 ++++++++++++ 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index c142c813e..2f43c21bc 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -125,32 +125,37 @@ def _split_addresses(value: str) -> List[str]: before the first `?` or `#` is always its own, `=` in the path or not (`/redirect=https://user:pw@host/doc`). Inside that region the nearest structural character decides: a `=` puts the authority in a value, a field - separator — or nothing at all — starts a new address. One pass over the match - finds them all. + separator — or nothing at all — starts a new address. One forward pass over + the match finds them all. """ fields_start = min( (value.index(char) for char in "?#" if char in value), default=len(value) ) starts = [ - match.start() - for match in _URL_AUTHORITY_SEARCH.finditer(value) - if match.start() > 0 - and ( - match.start() < fields_start or not _in_value_position(value, match.start()) - ) + start + for start, structural in _authority_starts(value) + if start > 0 and (start < fields_start or structural != "=") ] if not starts: return [value] return [value[begin:end] for begin, end in zip([0] + starts, starts + [len(value)])] -def _in_value_position(value: str, start: int) -> bool: - """Whether the authority at ``start`` sits inside a field's value: the nearest - structural character before it is a `=`.""" - for index in range(start - 1, -1, -1): - if value[index] in _FIELD_STRUCTURE_CHARACTERS: - return value[index] == "=" - return False +def _authority_starts(value: str) -> List[Tuple[int, str]]: + """Every authority start in ``value``, each paired with the last structural + character seen before it. One forward pass: the cursor never moves backwards, + so a value carrying thousands of addresses costs the same per character as one + carrying a single address.""" + starts: List[Tuple[int, str]] = [] + cursor = 0 + structural = "" + for match in _URL_AUTHORITY_SEARCH.finditer(value): + for index in range(cursor, match.start()): + if value[index] in _FIELD_STRUCTURE_CHARACTERS: + structural = value[index] + cursor = match.start() + starts.append((match.start(), structural)) + return starts def _sanitize_single_url(value: str, *, nested: bool, in_prose: bool) -> str: diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index a8fbd8e7b..e3828ad0c 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -619,6 +619,19 @@ def test_redact_pii_redacts_multiple_identifiers(): ) +def test_sanitize_url_is_not_quadratic_on_many_addresses(): + # Every address here sits after the same `?`, which is the worst case for a + # value-position check that scans backwards from each one. The forward pass + # sees each character once; a regression to a per-address scan would spend + # seconds on this, on the server's own event loop. + import time + + pathological = "https://a.test/?" + "https://b.test/x," * 4_000 + start = time.monotonic() + assert sanitize_captured_value(pathological) == pathological + assert time.monotonic() - start < 1.0 + + def test_redact_pii_is_not_quadratic_on_pathological_input(): # A 100k-char run with an `@` but no valid TLD is the worst case for an # unbounded email pattern. With bounded quantifiers this stays linear; a From b760dc6c572aa02098bf77cd91a507bb130ba859 Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 12:09:43 -0300 Subject: [PATCH 30/33] fix(mcp): tell a URL's delimiters from the same characters inside a value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed - `?token=prefix?https://secret.example/private` split at the inner authority, because the forward pass counted every `?` as structural — including the one inside the token's own value. Only three positions actually divide a URL: the query's `?`, the fragment's `#`, and the `?` that splits the fragment into head and tail. `_structural_delimiters` computes them per value, and every other `?`/`#` is ordinary text, so the address after it stays with its field and the field pass redacts the lot. - `#password=phx_...?private-suffix` kept its tail: the PostHog-token pass had already rewritten that value, so comparing before and after found no change and the fail-closed rule never fired. A field list now reports whether its last field is SENSITIVE — its key is a credential name, or its value changed — and that flag drives both the fail-closed fragment tail and the trailing-punctuation rule. How tested A row for each: the value-internal `?`, and the already-redacted password whose head is byte-identical. Every existing row keeps its expectation, and all three pathological inputs still run in ~20ms or less. - `.venv/bin/pytest posthog/test/mcp -q` -> 419 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 390 passed, 19 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 43 +++++++++++++++++++++++++------ posthog/test/mcp/test_pipeline.py | 12 +++++++++ 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 2f43c21bc..a3205143b 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -11,7 +11,7 @@ from __future__ import annotations import re -from typing import Any, Dict, List, Tuple +from typing import Any, Dict, List, Set, Tuple from urllib.parse import SplitResult, parse_qsl, urlencode, urlsplit, urlunsplit # SDK-injected arguments stripped from captured $mcp_parameters (they surface as @@ -58,8 +58,10 @@ # deliberately keeps those, and the field count is already bounded when parsing. _URL_AUTHORITY_PATTERN = re.compile(r"^[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) _URL_AUTHORITY_SEARCH = re.compile(r"[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) -# What separates one field from the next inside a query or fragment. -_FIELD_STRUCTURE_CHARACTERS = "=&;?#" +# What separates one field from the next inside a query or fragment. `?` and `#` +# are not here: whether one of those divides a URL depends on where it sits, and +# `_structural_delimiters` works that out per value. +_FIELD_SEPARATORS = "=&;" _MAX_URL_LENGTH = 8192 _MAX_URL_QUERY_FIELDS = 128 # A query key is sensitive when ANY `-`/`_`/`.`/`/`-delimited segment matches, which @@ -146,18 +148,32 @@ def _authority_starts(value: str) -> List[Tuple[int, str]]: character seen before it. One forward pass: the cursor never moves backwards, so a value carrying thousands of addresses costs the same per character as one carrying a single address.""" + delimiters = _structural_delimiters(value) starts: List[Tuple[int, str]] = [] cursor = 0 structural = "" for match in _URL_AUTHORITY_SEARCH.finditer(value): for index in range(cursor, match.start()): - if value[index] in _FIELD_STRUCTURE_CHARACTERS: + if value[index] in _FIELD_SEPARATORS or index in delimiters: structural = value[index] cursor = match.start() starts.append((match.start(), structural)) return starts +def _structural_delimiters(value: str) -> Set[int]: + """The positions where a `?` or `#` actually divides the URL: the query's `?`, + the fragment's `#`, and the `?` that splits the fragment into head and tail. + Every other one is a character inside a value — `?token=pre?fix` has one `?` + of structure and one of credential.""" + fragment = value.find("#") + query = value.find("?") + if fragment != -1 and (query == -1 or query > fragment): + query = -1 + fragment_tail = value.find("?", fragment + 1) if fragment != -1 else -1 + return {index for index in (query, fragment, fragment_tail) if index != -1} + + def _sanitize_single_url(value: str, *, nested: bool, in_prose: bool) -> str: if len(value) > _MAX_URL_LENGTH and _URL_AUTHORITY_PATTERN.match(value): return _REDACTED_VALUE @@ -227,7 +243,7 @@ def _rewrote_the_last_field( """Whether the URL ends in a field whose value was just redacted — the part it ends in being its fragment when it has one, its query otherwise.""" if not url.fragment: - return bool(query) and sanitized_query[-1] != query[-1] + return _last_field_is_sensitive(query, sanitized_query) return tail_rewrote_last if "?" in url.fragment else head_rewrote_last @@ -239,9 +255,20 @@ def _sanitize_fragment_part(text: str, *, nested: bool) -> Tuple[str, bool]: if "=" not in text: return _sanitize_fragment_text(text, nested=nested), False fields, sanitized = _sanitize_url_fields(text, nested=nested) - if sanitized == fields: - return text, False - return urlencode(sanitized), bool(fields) and sanitized[-1] != fields[-1] + return ( + urlencode(sanitized) if sanitized != fields else text, + _last_field_is_sensitive(fields, sanitized), + ) + + +def _last_field_is_sensitive(fields: _UrlFields, sanitized: _UrlFields) -> bool: + """Whether the last field of a list carries a credential: its key is one of + the sensitive names, or its value was just rewritten. The key alone has to + count — an earlier pass may already have redacted the value, leaving the + comparison nothing to catch.""" + if not fields: + return False + return _should_redact_query_key(fields[-1][0]) or sanitized[-1] != fields[-1] def _sanitize_fragment_text(text: str, *, nested: bool) -> str: diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index e3828ad0c..f36fd2305 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -168,6 +168,12 @@ def test_sanitize_redacts_large_base64(): "https://example.com/#password=prefix?token=x&page=1", "https://example.com/#password=%5Bredacted%5D?[redacted]", ), + # The head is byte-identical here — the PostHog-token pass had already + # redacted that value — so what marks the tail as suspect is the key. + ( + "https://example.com/#password=phx_EXAMPLEONLYFAKEVALUE00000000000?private-suffix", + "https://example.com/#password=[redacted]?[redacted]", + ), ("https://example.com/#/docs/id=1", "https://example.com/#/docs/id=1"), # A field list whose key happens to start with a `/`; re-serializing the # redacted field percent-encodes that `/`. @@ -295,6 +301,12 @@ def test_sanitize_redacts_large_base64(): "https://host/x?token=foo%20https://secret.test/private", "https://host/x?token=%5Bredacted%5D", ), + # A second `?` inside a value is a character of that value, not a + # delimiter, so the address after it belongs to the token. + ( + "https://example.com/?token=prefix?https://secret.example/private", + "https://example.com/?token=%5Bredacted%5D", + ), ( "https://example.com/?q=see,https://fakeuser:fakepass@x.test/doc", "https://example.com/?q=see%2Chttps%3A%2F%2F%255Bredacted%255D%40x.test%2Fdoc", From 7371ec717fec4ef0ee7855e21280c7dfb54f813c Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 12:13:21 -0300 Subject: [PATCH 31/33] fix(mcp): stop splitting a credential at a semicolon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed Every `;` was normalized to `&` before parsing, so `?password=prefix;remainingsecret` was captured as `password=%5Bredacted%5D&remainingsecret=` — the tail of the password published as a field of its own. A `;` is a legacy field separator to some servers and an ordinary character to others, and the value cannot say which, so parsing now splits on `&` alone and the decision fails closed: a sensitive key redacts its whole value (the `;` tail with it), and a value under a non-sensitive key is redacted whole when any `;`-separated piece of it names a credential. The 128-field bound counts `&` only, which is what `parse_qsl` was already doing. How tested Rows for the split password, the legacy `a=1;token=...` field (now redacted as one value rather than re-serialized into two) and a benign `a=1;b=2` that stays byte-for-byte. - `.venv/bin/pytest posthog/test/mcp -q` -> 421 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 392 passed, 19 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Notes for the reviewer The README's URL paragraph never described the `;` normalization, so nothing there needed changing. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 22 ++++++++++++++++++---- posthog/test/mcp/test_pipeline.py | 11 ++++++++++- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index a3205143b..c2de8e473 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -291,11 +291,11 @@ def _redact_userinfo(netloc: str) -> str: def _sanitize_url_fields(text: str, *, nested: bool) -> Tuple[_UrlFields, _UrlFields]: """Parse a query (or fragment) and return both the original and the sanitized - fields, so the caller can tell whether anything was redacted. ``;`` is - normalized to ``&``: servers still emit it as a field separator, and a query - split only on ``&`` would hide the credential behind it.""" + fields, so the caller can tell whether anything was redacted. Only ``&`` + separates fields — see ``_sanitize_url_field_value`` for the ``;`` a value can + carry — so the field bound counts ``&`` too.""" fields = parse_qsl( - text.replace(";", "&"), + text, keep_blank_values=True, max_num_fields=_MAX_URL_QUERY_FIELDS, ) @@ -308,6 +308,12 @@ def _sanitize_url_fields(text: str, *, nested: bool) -> Tuple[_UrlFields, _UrlFi def _sanitize_url_field_value(key: str, value: str, *, nested: bool) -> str: if _should_redact_query_key(key): return _REDACTED_VALUE + # A `;` is a legacy field separator to some servers and an ordinary character + # to others, and the value alone cannot say which. Splitting on it would cut + # `password=pre;fix` in two and publish the second half, so instead the whole + # value goes whenever any `;`-separated piece of it names a credential. + if ";" in value and _names_a_credential(value): + return _REDACTED_VALUE # A retained value can carry a URL of its own (a gateway's `?url=`). The budget # for that is one level: sanitize the first, and drop any value still carrying # a URL past it rather than trusting what we did not look inside. @@ -316,6 +322,14 @@ def _sanitize_url_field_value(key: str, value: str, *, nested: bool) -> str: return value +def _names_a_credential(value: str) -> bool: + """Whether any ``;``-separated piece of a field's value reads as a field of + its own with a sensitive name (``a=1;token=x``).""" + return any( + _should_redact_query_key(piece.partition("=")[0]) for piece in value.split(";") + ) + + # PII redaction for the agent-narrated intent string only. $mcp_intent is free # text the calling LLM writes into the injected `context` argument, so it can # carry personal data the model read aloud despite being told not to. We redact diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index f36fd2305..6654e8ebd 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -194,10 +194,19 @@ def test_sanitize_redacts_large_base64(): "https://example.com/#%2Ftoken=%5Bredacted%5D" "&next=https%3A%2F%2Fother.test%2F?page=1", ), + # A `;` is a field separator to some servers and a value character to + # others, so a value holding one goes whole rather than being split: the + # legacy field is still redacted, and `password=pre;fix` keeps its tail + # out of the payload. ( "https://example.com/x?a=1;token=fakesecret", - "https://example.com/x?a=1&token=%5Bredacted%5D", + "https://example.com/x?a=%5Bredacted%5D", ), + ( + "https://example.com/guide?password=prefix;remainingsecret", + "https://example.com/guide?password=%5Bredacted%5D", + ), + ("https://example.com/x?a=1;b=2", "https://example.com/x?a=1;b=2"), ( "https://example.com/x?jwt=fakejwt&sessionid=fakesession&code=fakecode&country_code=BR", "https://example.com/x?jwt=%5Bredacted%5D&sessionid=%5Bredacted%5D&code=%5Bredacted%5D&country_code=BR", From d05b16e4d15dcb8d8d84fd8eaec832ac18487e5f Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 12:14:06 -0300 Subject: [PATCH 32/33] fix(mcp): redact an intent's credentials before its PII What changed `sanitize_intent` ran `redact_pii` first, and a PII pattern can cut a token in half: the phone pattern reads the middle of `phx_AAAAAAAA-415-555-0142-AAAAAAAAAAAAAAAAAAAA` as a number, so the intent kept `phx_AAAAAAAA-[redacted]-AAAAAAAAAAAAAAAAAAAA` where the token pass would have taken the whole thing. `_sanitize_text` is split into `_redact_credentials` (the PostHog-token pass, then the entropy detector) and the URL pass, and the intent now runs binary gate, credentials, PII, URLs. Every other captured string keeps credentials-then-URLs, unchanged. The ordering comment moves onto `_redact_credentials` and `sanitize_intent`: credentials before PII because a PII pattern can cut a token in half, PII before the URL pass because a rewritten URL percent-encodes the `@` the email pattern needs. How tested A row for that token; the email-inside-a-URL, PostHog-token-plus-email, phone-and-email and binary-blob intent rows are unchanged. - `.venv/bin/pytest posthog/test/mcp -q` -> 422 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 393 passed, 19 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 42 ++++++++++++++++++------------- posthog/test/mcp/test_pipeline.py | 8 ++++++ 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index c2de8e473..c6ca9fd2f 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -419,30 +419,38 @@ def _is_binary_blob(value: str) -> bool: def _sanitize_text(value: str) -> str: - # Both credential passes run before the URL pass, because rewriting a URL - # changes the text they match on: it percent-encodes `/`, hiding a - # `?ref=/phx_...` token behind `%2F` from the `\bph` boundary, and it can grow - # a word past the length window the entropy detector scans. Running the URL - # pass last loses nothing — it only redacts or percent-encodes, so it never - # exposes a credential the detectors could have matched. - value = _POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value) - return _sanitize_urls(_redact_secret_tokens(value)) + return _sanitize_urls(_redact_credentials(value)) + + +def _redact_credentials(value: str) -> str: + """Redact PostHog tokens, then any other word that reads as a credential. + + Both run before the URL pass, because rewriting a URL changes the text they + match on: it percent-encodes `/`, hiding a `?ref=/phx_...` token behind `%2F` + from the `\bph` boundary, and it can grow a word past the length window the + entropy detector scans. Running the URL pass last loses nothing — it only + redacts or percent-encodes, so it never exposes a credential the detectors + could have matched.""" + return _redact_secret_tokens(_POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value)) def sanitize_intent(value: Any) -> Any: - """Sanitize the agent-narrated intent: the binary gate, then structured PII, - then the same passes every captured string gets. - - Order matters in both places. The binary gate runs first because splicing a - redaction into a base64 blob stops it looking like base64, and the blob would - then be captured almost whole instead of as the marker. PII runs before the - URL pass because a rewritten URL percent-encodes the `@` the email pattern - needs to see.""" + """Sanitize the agent-narrated intent: the binary gate, the credential passes, + structured PII, then URLs. + + Every step sits where it does for a reason. The binary gate runs first + because splicing a redaction into a base64 blob stops it looking like base64, + and the blob would then be captured almost whole instead of as the marker. + Credentials run before PII because a PII pattern can cut a token in half — the + phone pattern reads `phx_AAAA-415-555-0142-AAAA` as a number and redacts only + the middle, leaving the token's halves in the payload. PII runs before the URL + pass because a rewritten URL percent-encodes the `@` the email pattern needs + to see.""" if not isinstance(value, str): return sanitize_captured_value(value) if _is_binary_blob(value): return _BINARY_DATA_MARKER - return _sanitize_text(redact_pii(value)) + return _sanitize_urls(redact_pii(_redact_credentials(value))) def _sanitize_resource_name(value: Any) -> Any: diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 6654e8ebd..8e04d026a 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -734,6 +734,14 @@ def test_sanitize_event_redacts_pii_from_intent(): "Open https://example.com/?email=alice@example.com&token=fakesecret", "Open https://example.com/?email=%5Bredacted%5D&token=%5Bredacted%5D", ), + # Credentials are redacted before PII: the phone pattern reads the middle + # of this token as a number, and redacting that first would leave the + # token's two halves behind. + ( + "token-a-pii-pattern-would-cut-in-half", + "Rotating phx_AAAAAAAA-415-555-0142-AAAAAAAAAAAAAAAAAAAA", + "Rotating [redacted]", + ), # The binary gate runs before PII: splicing a redaction into a base64 blob # would stop it looking like base64, and the blob would be captured whole. ( From f70061d4f105b65f2e64084a702110de6d9dfe8f Mon Sep 17 00:00:00 2001 From: Lucas Faria Date: Thu, 10 Sep 2026 12:19:15 -0300 Subject: [PATCH 33/33] fix(mcp): keep a credential whole across `;` and a fragment's `?` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed - A `;` inside a KEY named nothing. `?download;token=fakesecret` parses to the key `download;token`, and only values were checked for `;`-separated credentials. `;` joins the segment separators of the sensitive-key pattern, so such a key is recognized and its field redacted. - A credential's suffix was split away before anything could fail closed. `#password=prefix?https://private.example/remainingsecret` treated the fragment's `?` as structure and the address behind it as adjacent, so the fail-closed tail rule never saw it; `?password=prefix;https://...` did the same through `;`. In `_authority_starts`, `;` is no longer a field separator (fields parse on `&` alone, so a `;` belongs to whatever value holds it), and the fragment's `?` divides only when no value is already open — straight after a `=` the address stays with its field, where the fragment split and its fail-closed rule can see the whole of it. How tested Five rows: the `;` key, the `;` and `?` suffixes, a fragment head with no credential (so its tail is sanitized as text rather than dropped), and a `?` that opens no value, which still divides. Every existing row is unchanged and all three pathological inputs still run in ~20ms or less. - `.venv/bin/pytest posthog/test/mcp -q` -> 427 passed - `.venv-mcp-v2/bin/pytest posthog/test/mcp -q` -> 398 passed, 19 skipped - `.venv/bin/ruff check posthog/mcp posthog/test/mcp` -> All checks passed! - `.venv/bin/ruff format --check posthog/mcp posthog/test/mcp` -> 60 files already formatted - `uv run mypy --no-site-packages --config-file mypy.ini . | uv run mypy-baseline filter` -> Success: no issues found in 231 source files Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 --- posthog/mcp/_sanitization.py | 37 +++++++++++++++++++------------ posthog/test/mcp/test_pipeline.py | 27 ++++++++++++++++++++++ 2 files changed, 50 insertions(+), 14 deletions(-) diff --git a/posthog/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index c6ca9fd2f..90db4ebd0 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -11,7 +11,7 @@ from __future__ import annotations import re -from typing import Any, Dict, List, Set, Tuple +from typing import Any, Dict, List, Tuple from urllib.parse import SplitResult, parse_qsl, urlencode, urlsplit, urlunsplit # SDK-injected arguments stripped from captured $mcp_parameters (they surface as @@ -60,17 +60,18 @@ _URL_AUTHORITY_SEARCH = re.compile(r"[a-z][a-z0-9+.-]{0,63}://", re.IGNORECASE) # What separates one field from the next inside a query or fragment. `?` and `#` # are not here: whether one of those divides a URL depends on where it sits, and -# `_structural_delimiters` works that out per value. -_FIELD_SEPARATORS = "=&;" +# `_structural_delimiters` works that out per value. Neither is `;` — fields are +# parsed on `&` alone, so a `;` is a character of whatever value holds it. +_FIELD_SEPARATORS = "=&" _MAX_URL_LENGTH = 8192 _MAX_URL_QUERY_FIELDS = 128 -# A query key is sensitive when ANY `-`/`_`/`.`/`/`-delimited segment matches, which +# A query key is sensitive when ANY `-`/`_`/`.`/`/`/`;`-delimited segment matches, which # covers the compound names credentials actually travel under: `private_token`, # `oauth_signature`, `id_token`, `subscription-key`, `X-Amz-Security-Token`. # Over-redacting a benign `sort_key` is the accepted trade for an analytics payload. _SENSITIVE_QUERY_SEGMENT_PATTERN = re.compile( - r"(^|[-_./])(auth|token|secret|password|passwd|pwd|credential|signature|sig|" - r"key|hmac|sas|bearer|jwt|session|sessionid)([-_./]|$)", + r"(^|[-_./;])(auth|token|secret|password|passwd|pwd|credential|signature|sig|" + r"key|hmac|sas|bearer|jwt|session|sessionid)([-_./;]|$)", re.IGNORECASE, ) # Matched whole rather than per segment: `code` (an OAuth authorization code) as a @@ -148,30 +149,38 @@ def _authority_starts(value: str) -> List[Tuple[int, str]]: character seen before it. One forward pass: the cursor never moves backwards, so a value carrying thousands of addresses costs the same per character as one carrying a single address.""" - delimiters = _structural_delimiters(value) + query, fragment, fragment_tail = _structural_delimiters(value) starts: List[Tuple[int, str]] = [] cursor = 0 structural = "" for match in _URL_AUTHORITY_SEARCH.finditer(value): for index in range(cursor, match.start()): - if value[index] in _FIELD_SEPARATORS or index in delimiters: + # The fragment's own `?` divides it only when a value is not already + # open: straight after a `=` that `?` may be a character of the value, + # and the address behind it belongs to the field — where the fragment + # split and its fail-closed rule can see the whole of it. + if ( + value[index] in _FIELD_SEPARATORS + or index in (query, fragment) + or (index == fragment_tail and structural != "=") + ): structural = value[index] cursor = match.start() starts.append((match.start(), structural)) return starts -def _structural_delimiters(value: str) -> Set[int]: - """The positions where a `?` or `#` actually divides the URL: the query's `?`, - the fragment's `#`, and the `?` that splits the fragment into head and tail. - Every other one is a character inside a value — `?token=pre?fix` has one `?` - of structure and one of credential.""" +def _structural_delimiters(value: str) -> Tuple[int, int, int]: + """Where a `?` or `#` can divide the URL: the query's `?`, the fragment's `#`, + and the `?` that splits the fragment into head and tail (``-1`` for each one + the value does not have). Every other `?` or `#` is a character inside a value + — `?token=pre?fix` has one `?` of structure and one of credential.""" fragment = value.find("#") query = value.find("?") if fragment != -1 and (query == -1 or query > fragment): query = -1 fragment_tail = value.find("?", fragment + 1) if fragment != -1 else -1 - return {index for index in (query, fragment, fragment_tail) if index != -1} + return query, fragment, fragment_tail def _sanitize_single_url(value: str, *, nested: bool, in_prose: bool) -> str: diff --git a/posthog/test/mcp/test_pipeline.py b/posthog/test/mcp/test_pipeline.py index 8e04d026a..cab7cb9c1 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -207,6 +207,33 @@ def test_sanitize_redacts_large_base64(): "https://example.com/guide?password=%5Bredacted%5D", ), ("https://example.com/x?a=1;b=2", "https://example.com/x?a=1;b=2"), + # A `;` inside a KEY names the credential just as a `-` or `_` would. + ( + "https://example.com/?download;token=fakesecret", + "https://example.com/?download%3Btoken=%5Bredacted%5D", + ), + # Neither a `;` nor the fragment's own `?` ends a value that is already + # open, so the address behind one stays with its field and goes with it. + ( + "https://example.com/?password=prefix;https://private.example/remainingsecret", + "https://example.com/?password=%5Bredacted%5D", + ), + ( + "https://example.com/#password=prefix?https://private.example/remainingsecret", + "https://example.com/#password=%5Bredacted%5D?[redacted]", + ), + # Attached the same way, but this head holds no credential, so the tail is + # sanitized as the text it is. + ( + "https://example.com/#/docs/id=1?https://fakeuser:fakepass@x.test/doc", + "https://example.com/#/docs/id=1?https://%5Bredacted%5D@x.test/doc", + ), + # ... while a `?` that opens no value still divides: the address after it + # is its own. + ( + "https://example.com/?a=1#b?https://fakeuser:fakepass@x.test/doc", + "https://example.com/?a=1#b?https://%5Bredacted%5D@x.test/doc", + ), ( "https://example.com/x?jwt=fakejwt&sessionid=fakesession&code=fakecode&country_code=BR", "https://example.com/x?jwt=%5Bredacted%5D&sessionid=%5Bredacted%5D&code=%5Bredacted%5D&country_code=BR",