diff --git a/.sampo/changesets/calm-resource-atlas.md b/.sampo/changesets/calm-resource-atlas.md new file mode 100644 index 000000000..241c60e7c --- /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. 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 a032629f7..a5c38d645 100644 --- a/posthog/mcp/README.md +++ b/posthog/mcp/README.md @@ -1,7 +1,22 @@ # 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. + +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/__init__.py b/posthog/mcp/__init__.py index 633459300..e22a8a1ab 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 @@ -219,9 +219,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 f61f3c361..0b6e7f7bc 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, @@ -58,6 +59,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 d16c698e0..d66337e63 100644 --- a/posthog/mcp/_instrument_lowlevel.py +++ b/posthog/mcp/_instrument_lowlevel.py @@ -22,13 +22,17 @@ 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, + resource_listing_response, resolve_session_and_client, start_tool_call_lifecycle, start_tools_list_lifecycle, @@ -50,6 +54,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: @@ -73,6 +78,94 @@ 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), + # 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) + + +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=resource_listing_response(event_type, 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 c740ae273..649bc606b 100644 --- a/posthog/mcp/_instrument_v2.py +++ b/posthog/mcp/_instrument_v2.py @@ -37,11 +37,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, collect_listed_tools, mutate_tool_schema, params_to_request_dict, + prepare_request, + record_resource_request, + resource_listing_response, resolve_session_and_client, start_tool_call_lifecycle, start_tools_list_lifecycle, @@ -68,6 +72,13 @@ # 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, + # 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, +} def instrument_mcpserver_v2(server: Any, data: MCPAnalyticsData) -> None: @@ -86,6 +97,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) @@ -98,6 +111,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) @@ -132,6 +147,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 @@ -465,6 +482,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=resource_listing_response(event_type, 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 328fc597f..52f2b4e71 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 @@ -904,3 +903,55 @@ 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}") + + +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, + client_version: Optional[str] = None, + protocol_version: Optional[str] = None, + extra: Optional[Dict[str, Any]] = None, +) -> None: + """Record a resources listing or 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/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/_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/mcp/_sanitization.py b/posthog/mcp/_sanitization.py index 47cf76737..90db4ebd0 100644 --- a/posthog/mcp/_sanitization.py +++ b/posthog/mcp/_sanitization.py @@ -11,12 +11,15 @@ from __future__ import annotations import re -from typing import Any, Dict +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 # 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 _POSTHOG_TOKEN_PATTERN = re.compile(r"\bph[a-z]_[A-Za-z0-9_-]{20,}\b") @@ -27,6 +30,315 @@ 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. +# `'` 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. +# +# 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 +# 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 = ".,;:!?)]}'" +# 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) +_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. 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 +# 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"^(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: + # 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, in_prose=True), text + ) + + +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://...`, `/?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 (`?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. 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 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 = [ + 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 _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.""" + 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()): + # 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) -> 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 query, fragment, fragment_tail + + +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 + 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 `?` 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) + # 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) + if (netloc, sanitized_query, fragment) == ( + url.netloc, + query, + url.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. + 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. + return ( + urlunsplit( + ( + url.scheme, + netloc, + url.path, + urlencode(sanitized_query) + if sanitized_query != query + else url.query, + fragment, + ) + ) + + suffix + ) + except ValueError: + 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 _last_field_is_sensitive(query, sanitized_query) + 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) + 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: + """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 _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. Only ``&`` + separates fields — see ``_sanitize_url_field_value`` for the ``;`` a value can + carry — so the field bound counts ``&`` too.""" + fields = parse_qsl( + text, + 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 `;` 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. + if _URL_PATTERN.search(value): + return _sanitize_urls(value, nested=False) if nested else _REDACTED_VALUE + 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 @@ -106,11 +418,62 @@ 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: + 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, 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_urls(redact_pii(_redact_credentials(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. @@ -134,6 +497,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 @@ -247,16 +617,17 @@ 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("resource_name") is not None: + 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 - # 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. `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"] = redact_pii( - sanitize_captured_value(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/_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 eb50ff1e1..992c558a3 100644 --- a/posthog/test/mcp/test_lowlevel.py +++ b/posthog/test/mcp/test_lowlevel.py @@ -1,17 +1,23 @@ """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 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(): +def make_server(*, resource_error: bool = False, listing: str = "resources") -> Server: server = Server("test-lowlevel") @server.list_tools() @@ -34,6 +40,57 @@ 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): + if listing == "error": + raise ValueError("listing unavailable") + return mcp_types.ServerResult( + mcp_types.ListResourcesResult( + resources=[] + if listing == "empty" + else [ + mcp_types.Resource( + name="Guide", + uri="file:///guide.md", + mimeType="text/markdown", + ) + ] + ) + ) + + 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}") + 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.ListResourceTemplatesRequest] = ( + list_resource_templates + ) + server.request_handlers[mcp_types.ReadResourceRequest] = read_resource + return server @@ -62,6 +119,208 @@ async def test_list_tools_injects_optional_context_and_captures(): assert listed and listed[0]["properties"]["$mcp_listed_tool_names"] == ["echo"] +@pytest.mark.parametrize( + "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, + ), + # 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=[redacted]", + False, + ), + ( + "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", + "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( + uri: str, captured_uri: str, resource_error: bool +) -> None: + server = make_server(resource_error=resource_error) + client = FakeClient() + instrument(server, client) + + 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 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 + for secret in ( + "phx_EXAMPLEONLYFAKEVALUE00000000000", + "fakeuser", + "fakepass", + "fakesecret", + "fakeaccess", + "fakecredential", + "fakesignature", + ): + 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 b92e92c5c..cab7cb9c1 100644 --- a/posthog/test/mcp/test_pipeline.py +++ b/posthog/test/mcp/test_pipeline.py @@ -78,6 +78,392 @@ 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]"), + ( + "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"), + # 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 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. + ( + "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"), + # 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", + ), + # ... 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]", + ), + # 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 `/`. + ( + "https://example.com/#/token=fakesecret", + "https://example.com/#%2Ftoken=%5Bredacted%5D", + ), + # 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?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", + ), + # 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=%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"), + # 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", + ), + # 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.", + ), + ( + "Failed (https://example.com/x?sig=fakesignature).", + "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)."), + ( + "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"), + # 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", + ), + ("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."), + ("Meet at12:30 today", "Meet at12:30 today"), + ("resource:guide", "resource:guide"), + # 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", + ), + ( + "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"), + ( + "a:b:https://fakeuser:fakepass@example.com/doc", + "a:b:https://%5Bredacted%5D@example.com/doc", + ), + ( + "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)", + ), + # ... 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", + ), + # 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", + ), + ( + "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", + ), + # 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", + ), + # 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", + ), + ( + "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.", + ), + ], +) +def test_sanitize_url_credentials(value: str, expected: str) -> None: + assert sanitize_captured_value(value) == expected + 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", + ), + # 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: + 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": { @@ -281,6 +667,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 @@ -293,6 +692,48 @@ 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", + ), + # 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( + 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.", @@ -304,12 +745,44 @@ 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", + ), + # 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. + ( + "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( + 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(): diff --git a/posthog/test/mcp/test_resources.py b/posthog/test/mcp/test_resources.py new file mode 100644 index 000000000..19a0100b5 --- /dev/null +++ b/posthog/test/mcp/test_resources.py @@ -0,0 +1,157 @@ +"""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, + listed_uris, +) + + +@pytest.fixture(params=["official", "fastmcp"]) +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 + else: + from mcp.server.mcpserver import MCPServer as Server + + 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( + 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) + + listing = await dispatch(server, "resources/list") + assert str(listing.resources[0].uri) == "file:///guide.md" + read = dispatch( + server, + "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() + + 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"] + 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) + + +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 + + +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 diff --git a/posthog/test/mcp/test_v2_lowlevel.py b/posthog/test/mcp/test_v2_lowlevel.py index eb543f86f..f55b95f95 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 @@ -18,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(): +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") @@ -58,13 +61,63 @@ 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): + if listing == "error": + raise ValueError("listing unavailable") + return mcp_types.ListResourcesResult( + 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}") + 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/templates/list", + mcp_types.PaginatedRequestParams, + on_list_resource_templates, + ) + 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 +130,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 +158,169 @@ async def test_list_tools_injects_optional_context_and_captures(): assert listed[0]["properties"]["$mcp_server_name"] == "test-low-v2" +@pytest.mark.parametrize( + "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, + ), + # 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=[redacted]", + False, + ), + ( + "https://example.com/guide?token=phx_EXAMPLEONLYFAKEVALUE00000000000", + "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( + 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") + 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 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 + 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" + + +@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 -------------------------------------------------------------- @@ -200,13 +421,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():