feat(mcp): capture resource discovery and reads - #928
Conversation
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewThis pull request adds capture of MCP resource discovery and resource read operations, including sanitization of captured values and URLs. Eight issues have been addressed, but two credential-redaction bypasses remain open. Crafted Unicode spacing or nested URLs in fragment tails can allow credentials in captured MCP content to be exported without redaction, creating a clear sensitive-data exposure risk. Open issues (2)
Fixed/addressed: 8 · PR risk: 7/10 |
posthog-python Compliance ReportDate: 2026-09-10 15:25:16 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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, |
There was a problem hiding this comment.
Resource reads now unconditionally include the full returned response in $mcp_response. Text resources can contain private documents, configuration, or credentials, but the sanitizer only masks sensitive dictionary keys and recognizable token patterns; ordinary sensitive text remains unchanged and is sent to PostHog. Capture only resource metadata by default, or require explicit opt-in before exporting resource bodies.
How this was verified: A successful resources/read result flows through _wrap_response into the PostHog capture pipeline, and the added tests confirm that its text content is emitted verbatim.
Knowledge Base Used: MCP event and session processing
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/mcp/_instrumentation.py
Line: 863
Comment:
**Resource bodies leak sensitive data**
Resource reads now unconditionally include the full returned response in `$mcp_response`. Text resources can contain private documents, configuration, or credentials, but the sanitizer only masks sensitive dictionary keys and recognizable token patterns; ordinary sensitive text remains unchanged and is sent to PostHog. Capture only resource metadata by default, or require explicit opt-in before exporting resource bodies.
**How this was verified:** A successful `resources/read` result flows through `_wrap_response` into the PostHog capture pipeline, and the added tests confirm that its text content is emitted verbatim.
**Knowledge Base Used:** [MCP event and session processing](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog-python/-/docs/mcp-event-and-session-processing.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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).
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.
Combine resource URL credential redaction with main's intent PII and model-metadata sanitization. Preserve the current MCP adapters and pipeline. Validation: MCP v1 338 passed; MCP v2 312 passed, 17 expected skips. Ruff lint/format and mypy baseline passed. CodeScene merge-wide findings include upstream MCP complexity; unrelated refactors are deferred.
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.
gesh
left a comment
gesh
left a comment
There was a problem hiding this comment.
QA swarm review (automated multi-agent pass, adversarially verified). The suite passes both CI legs (SDK v1 and v2), the wrapping topology and failure isolation are sound, and events follow the existing tool-call conventions. The nine comments below are the confirmed findings, all minor severity. The two most useful before merge: fragment redaction in the URL sanitizer, and importorskip in test_resources.py.
| if netloc == url.netloc and sanitized_query == query: | ||
| return value | ||
| return urlunsplit( | ||
| (url.scheme, netloc, url.path, urlencode(sanitized_query), url.fragment) |
There was a problem hiding this comment.
URL fragments are never redacted. _sanitize_url examines only netloc and query. url.fragment goes to urlunsplit unchanged, and the unchanged-URL early return also emits it verbatim. A URI such as https://app.example.com/cb#access_token=eyJ... ships the token to PostHog in $mcp_resource_name, $mcp_parameters, and error messages. The same token in a query string is redacted. This is the RFC 6749 implicit-grant shape, so it is a standard pattern. The entropy fallback does not catch it, because _looks_like_path_or_url bails on strings that contain ://. Suggestion: apply the sensitive-key redaction to fragments that parse as key=value pairs, and add a test with a credential-bearing fragment (the only fragment test, test_pipeline.py, uses a benign fragment).
There was a problem hiding this comment.
Fixed in df1c814. A fragment that parses as key=value fields gets the same key rules as the query; a plain anchor like #section-2 stays byte-for-byte. Covered by the #access_token=... and #section-2 rows in test_sanitize_url_credentials. Same change shipped to the JS SDK.
| value = match.group(0) | ||
| try: | ||
| url = urlsplit(value) | ||
| query = parse_qsl( |
There was a problem hiding this comment.
Semicolon-delimited query credentials bypass redaction. parse_qsl splits on & only (Python >= 3.10). For https://example.com/x?a=1;token=sekret123, the whole query becomes one pair ('a', '1;token=sekret123'). No key matches the sensitive patterns, so the secret ships unredacted. Verified by execution on this branch. Real-world likelihood is low (legacy convention), but it contradicts the README claim. Suggestion: also split on ;, or redact URLs whose query contains ;.
There was a problem hiding this comment.
Fixed in df1c814. ; is normalized to & before splitting query and fragment fields, so ?a=1;token=x redacts. When nothing is redacted the original string (with its ;) is returned unchanged; only a rewrite re-serializes with &.
| _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( |
There was a problem hiding this comment.
Some standard credential query keys are missing from the redaction list. code (the OAuth authorization code), jwt, and sessionid pass through verbatim, while token and sig are redacted. Verified by execution: ?jwt=...&sessionid=abc123&code=authcode123 is captured unchanged (short values dodge the entropy fallback). code in a callback URL is as standardized as access_token. Suggestion: add code, jwt, and session/sessionid to _SENSITIVE_QUERY_KEY_PATTERN.
There was a problem hiding this comment.
Fixed in df1c814, and widened: query keys are now matched per -/_/. segment (auth|token|secret|password|passwd|pwd|credential|signature|sig|key|hmac|sas|bearer|jwt|session|sessionid), so jwt, sessionid, session_id, private_token, oauth_signature, id_token, subscription-key all redact. code is matched exactly rather than as a segment so country_code and zip_code survive. Over-redacting a benign sort_key is the accepted trade.
| re.IGNORECASE, | ||
| ) | ||
|
|
||
| _URL_PATTERN = re.compile(r"\b[a-z][a-z0-9+.-]{0,63}://[^\s<>\"']+", re.IGNORECASE) |
There was a problem hiding this comment.
The URL regex absorbs trailing prose punctuation into the rewrite. The terminal class [^\s<>"']+ matches ,, ), and .. When redaction rewrites a URL inside a message, adjacent punctuation is parsed into the query and dropped. Verified by execution: See https://example.com/x?sig=abc, then retry. becomes See https://example.com/x?sig=%5Bredacted%5D then retry. and Failed (https://example.com/x?sig=abc). loses ). entirely. Impact is only on captured analytics text, and only when a rewrite triggers, but it degrades $mcp_error_message fidelity. Suggestion: strip trailing punctuation from the match before parsing, and re-append it after the rewrite.
There was a problem hiding this comment.
Fixed in df1c814 (and made linear in 21f9f55). Trailing .,;:!?)]}' is split off before parsing and re-appended, so See https://x?a=b, then retry. and Failed (https://x?a=b). keep their punctuation. One deliberate exception after Codex review (2b11efc): when the URL's last field was itself redacted, the suffix is dropped, because ?password=hunter2!!! would otherwise come back as [redacted]!!!. So See https://x?sig=abc, then retry. now loses the comma while See https://x?sig=abc&page=2, then retry. keeps it. Structured fields like $mcp_resource_name never strip anything, since there is no prose there.
|
|
||
| def _wrap_resource_requests(server: Any, data: MCPAnalyticsData) -> None: | ||
| for request_type, event_type in ( | ||
| (mcp_types.ListResourcesRequest, MCPAnalyticsEventType.MCP_RESOURCES_LIST), |
There was a problem hiding this comment.
resources/templates/list is not instrumented in any adapter. MCP resource discovery has two methods: resources/list and resources/templates/list. This wrapper covers only ListResourcesRequest/ReadResourceRequest, and _RESOURCE_METHODS in _instrument_v2.py lists only resources/list and resources/read. FastMCP registers a ListResourceTemplatesRequest handler, and templated resources (@mcp.resource("users://{id}")) are advertised only through it. A template-only server therefore shows reads with no matching discovery events, while the README and changeset claim resource discovery is captured. No test registers a templated resource either. Suggestion: wrap ListResourceTemplatesRequest / resources/templates/list in both adapters, or scope the README claim.
There was a problem hiding this comment.
Fixed in df1c814. resources/templates/list is wrapped on every adapter (low-level v1, FastMCP, v2) and emitted as $mcp_resources_list; $mcp_parameters.request.method separates it from resources/list. Tests register a templated users://{user_id}/profile resource on each adapter.
| try: | ||
| params = request.get("params") | ||
| uri = params.get("uri") if isinstance(params, dict) else None | ||
| event: Dict[str, Any] = { |
There was a problem hiding this comment.
$mcp_resources_list records nothing about the listing outcome. Both wrappers await the handler result and then discard it; record_resource_request receives only the request. The event carries no resource names or URIs, no count, no nextCursor, and no empty-list flag — it can only say a listing happened, in N ms. The sibling record_tools_list captures $mcp_listed_tool_names, the full response, and flags an empty listing as an error. Listing metadata (name/uri/mimeType) is not a resource body, so the README exclusion does not apply. Suggestion: pass the result through and capture listed URIs (sanitized) and a count, for parity with $mcp_tools_list.
There was a problem hiding this comment.
Fixed in df1c814. Listing events now carry the listing result as $mcp_response (names, uris, uriTemplates, mimeTypes, nextCursor), which is metadata rather than a body, so it lines up with $mcp_tools_list. Reads still capture no response. An empty listing is intentionally not flagged as an error: a template-only server legitimately lists zero static resources.
| if request.param == "fastmcp": | ||
| if MCP_MAJOR >= 2: | ||
| pytest.skip("jlowin FastMCP requires MCP SDK v1") | ||
| from fastmcp import FastMCP as Server |
There was a problem hiding this comment.
This bare from fastmcp import ... errors instead of skipping when the optional package is absent. The fixture guards only MCP_MAJOR >= 2. On MCP SDK v1 without jlowin fastmcp installed (a supported configuration), the four [fastmcp-*] parametrizations report ERROR at fixture setup with ModuleNotFoundError. Reproduced by running the file in such an environment: 4 passed, 4 errors. The repo convention is pytest.importorskip("fastmcp") (see test_fastmcp_v2.py). Suggestion: add pytest.importorskip("fastmcp") on the fastmcp path.
There was a problem hiding this comment.
Fixed in df1c814 with pytest.importorskip("fastmcp") on that fixture path.
| assert str(result.root.contents[0].uri) == uri | ||
| await _flush() | ||
|
|
||
| assert len(_events(client, "$mcp_resources_list")) == 1 |
There was a problem hiding this comment.
$mcp_resources_list is only asserted by count. All three test files assert len(events(..., "$mcp_resources_list")) == 1 and check no property of the event (here, test_v2_lowlevel.py, and test_resources.py). The resource_error parametrization only makes reads fail, so the wrapper error branch for ListResourcesRequest and the empty-listing case are never exercised. Suggestion: assert the list event's properties ($mcp_is_error, duration, absence of $mcp_resource_name) and add a failing-list and an empty-list case.
There was a problem hiding this comment.
Fixed in df1c814. test_lowlevel.py and test_v2_lowlevel.py now assert the list event's method, $mcp_response uris, $mcp_is_error, $mcp_duration_ms, and absence of $mcp_resource_name, with parametrized static / empty / templates cases plus a failing-listing case that checks the $exception sibling. test_resources.py does the same through the high-level adapters.
| pypi/posthog: minor | ||
| --- | ||
|
|
||
| Capture MCP resource discovery and reads from instrumented servers. |
There was a problem hiding this comment.
The changeset omits a cross-cutting behavior change to existing events. The new _URL_PATTERN.sub(_sanitize_url, ...) in _sanitize_string runs on every captured string of every $mcp_* event, including pre-existing $mcp_tool_call parameters, responses, and error messages, and _SENSITIVE_QUERY_KEY_PATTERN redacts generic keys such as key, sig, and auth. After an upgrade, users will see URLs in existing captured tool-call data rewritten with %5Bredacted%5D values, with no changelog note. Suggestion: add a line that describes the new URL-credential redaction and that it also applies to tool-call events.
There was a problem hiding this comment.
Fixed in df1c814. The changeset now says URL credential redaction applies to every captured string, including existing $mcp_tool_call parameters, responses and error messages, and that URLs in existing tool-call data will show %5Bredacted%5D values after upgrading.
gesh
left a comment
gesh
left a comment
There was a problem hiding this comment.
Approving to unblock, have a look at the inline comments
… listings 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
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
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
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=<gateway2 whose own ?url= carries ...?token=fakesecret>`) 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
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
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
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
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
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
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=<credential>+<inner url>` 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
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
|
Addressed every inline finding above (replies on each thread), then ran an additional cross-model review pass on the result, which surfaced more URL-sanitizer gaps. All fixed on this branch, and every change ships identically in posthog-js#4830 so both SDKs redact the same bytes (verified by diffing 46 shared vectors through both sanitizers):
Two things deliberately left for separate PRs: the low-level v1 adapter does not wrap resource handlers registered after |
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
|
One more hardening commit, pushed on top (same in posthog-js#4830): a fragment route prefix that is itself an address ( |
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
|
One more, pushed on top (same in posthog-js#4830): a hash route that carries its own |
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
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
|
Two more, pushed on top (same in posthog-js#4830): an adjacent address is now split off wherever it sits unless it directly follows |
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
… split credential 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
|
Two more, pushed on top (same in posthog-js#4830): an adjacent address is only kept with its field when it sits in value position inside the query or fragment (nearest structural character before it is |
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
…alue 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
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
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
|
Four more, pushed on top (same in posthog-js#4830): value position is decided in one forward pass instead of a backward scan per address, which was quadratic on thousands of joined URLs (d3ec6c8); only a URL's real delimiters count as structural, so a second |
| return sanitize_captured_value(value) | ||
| if _is_binary_blob(value): | ||
| return _BINARY_DATA_MARKER | ||
| return _sanitize_urls(redact_pii(_redact_credentials(value))) |
There was a problem hiding this comment.
High: Unicode spacing bypasses credential redaction
An LLM can place a credential-looking token after a non-breaking Unicode space and have it exported unchanged. _redact_credentials only splits on ASCII spaces, while the Unicode-space normalization now happens later in redact_pii; normalize first while retaining credential redaction ahead of the PII patterns.
| return _sanitize_urls(redact_pii(_redact_credentials(value))) | |
| return _sanitize_urls(redact_pii(_redact_credentials(_UNICODE_SPACE_PATTERN.sub(" ", value)))) |
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
|
One more, pushed on top (same in posthog-js#4830): |
| if ( | ||
| value[index] in _FIELD_SEPARATORS | ||
| or index in (query, fragment) | ||
| or (index == fragment_tail and structural != "=") |
There was a problem hiding this comment.
High: Fragment-tail URL bypasses credential redaction
An LLM can supply a value such as https://outer.test/#id=1?https://inner.test/path?token=correcthorsebatterystaple. Because the preceding fragment contains =, this condition keeps the inner address attached; _sanitize_fragment_part then sees the inner query's = and parses the entire address as a field whose key is https://inner.test/path?token, so the sensitive-key check does not match and the credential is exported unchanged. Preserve the fail-closed behavior for a sensitive fragment head without preventing an otherwise independent inner URL from going through URL sanitization.
…ction (#20078) * docs(mcp-analytics): document resource events and URL credential redaction The SDKs now emit $mcp_resources_list (resources/list and resources/templates/list, carrying the listing) and $mcp_resource_read (URI, timing, error state, never the body), and redact credentials inside every captured URL. The events reference gains both rows and drops the "not emitted yet" note; the privacy page states that resource bodies never leave the process and describes the URL rule; the SDK v2 page drops the resource gap from its "not instrumented yet" table. Shipped in PostHog/posthog-js#4830 and PostHog/posthog-python#928. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7 * docs(mcp-analytics): name the SDK versions that emit resource events Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
…k capture Main's URL-credential sanitizer (#928) percent-encodes the @ the email pattern anchors on, so sanitize-then-redact let PII inside URLs through the feedback fields. Free text (summary, details, friction_points, suggested_improvement, tool_name) now uses the $mcp_intent pass (credentials -> PII -> URLs), and extras use a new sanitize_free_text_value walker that applies it per string leaf while keeping key-based redaction. sanitize_intent is renamed sanitize_free_text: the pass is no longer intent-specific. Ports posthog-js#4870 commits 9d3b3933, f4a69ea8, 270c4630. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0
* feat(mcp): capture agents feedback Add an opt-in collect_feedback option that injects a send_feedback virtual tool and captures every call as a $mcp_feedback event, porting PostHog/posthog-js#4870 to the Python MCP analytics SDK. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 * fix(mcp): address review findings ported from the TS feedback tool - redact structured PII in tool_name like the other free-text fields - enforce declared type/enum on extras: mismatches stay out of extras and the captured properties (raw keeps everything for the handler) - warn when on_feedback is set on the PostHogMCP path, where it is ignored - align capture_feedback's property precedence with the instrument() path (feedback properties win over caller-supplied ones) - correct the reserved-keys comment: SDK-injected arguments do reach raw because the report is parsed before stripping Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 * test(mcp): pin PII redaction of emails inside URLs in feedback text The TS SDK's sanitizer re-serializes URLs before PII redaction, which percent-encodes "@" and hides emails from the email pattern. The Python sanitizer does no URL rewriting, so the case already redacts correctly; this test keeps it that way if URL scrubbing is ever added. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 * fix(mcp): run free-text PII redaction before URL rewriting in feedback capture Main's URL-credential sanitizer (#928) percent-encodes the @ the email pattern anchors on, so sanitize-then-redact let PII inside URLs through the feedback fields. Free text (summary, details, friction_points, suggested_improvement, tool_name) now uses the $mcp_intent pass (credentials -> PII -> URLs), and extras use a new sanitize_free_text_value walker that applies it per string leaf while keeping key-based redaction. sanitize_intent is renamed sanitize_free_text: the pass is no longer intent-specific. Ports posthog-js#4870 commits 9d3b3933, f4a69ea8, 270c4630. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 * refactor: apply simplify pass Compute _is_sdk_virtual_tool once per mutate_tool_schema call instead of twice per tool per tools/list dispatch. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 * fix(mcp): plug feedback-tool-shadow and error-leak gaps from review - _matches_extra_schema: a declared "integer" extra accepted any float, including fractional ones (3.5), since Python's numeric tower conflates int and float; now requires the value to be whole. - handle_feedback: an on_feedback exception was logged with str(error) verbatim, letting agent-controlled report text (PII, credentials, log-forging newlines) an error message echoes reach host logs; now logs only the exception type, matching the report log beside it. - start_tool_call_lifecycle: conversation-id resolution skipped every call named like the feedback tool regardless of the listing-derived shadow flag, so a real tool that collided with the configured feedback name never got a conversation id even once ownership was known; now mirrors ToolCallLifecycle.is_feedback's fail-open guard. Addresses greptile-apps findings on PR #939. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 * fix(mcp): let original_tool disambiguate a feedback-name collision in prepare_tool_call On the custom-dispatcher path prepare_tool_list skips injecting the virtual tool when a real tool owns the feedback name, but prepare_tool_call still flagged every call by that name as feedback, so the documented dispatch flow suppressed the real tool. A host-supplied original_tool is stateless proof a real tool owns the name (the virtual tool never exists in the host's own list), so it now wins. Without original_tool the name match stands and the documented remedy is a non-colliding tool_name. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0 * fix(mcp): keep a feedback-name collision across paginated tools/list pages Each page of a paginated listing recomputed feedback_tool_shadowed from that page's tools alone, so a real send_feedback tool listed on page 1 was forgotten by page 2 — the SDK appended its virtual tool and then swallowed the real tool's calls. The flag is now sticky for the instrumentation instance's lifetime, and the virtual tool is appended only to the final page (no nextCursor), so an early page can't advertise it before a later page reveals the real tool. Generated-By: PostHog Desktop Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0
2 participants
💡 Motivation and Context
MCP server authors cannot tell whether clients discover or read their resources.
This leaves resource-based integrations absent from MCP Analytics even when tool tracking works.
Resource bodies stay out of analytics because resources may contain private documents or credentials.
🔨 Changes
resources/list,resources/templates/listandresources/readare instrumented on every adapter (low-level v1, FastMCP, jlowin FastMCP, v2). Listings emit$mcp_resources_listwith the listing metadata as$mcp_response; reads emit$mcp_resource_readwith the URI, duration and error state and never capture the body.-/_/.///;segment, plus exact signed-URL names), URLs nested one level inside a retained value, authority-less URIs such asresource:guide?token=x, hash-routed fragments, and adjacent addresses run together without whitespace. Whenever a credential's tail is ambiguous the sanitizer fails closed. This applies to every captured string, so URLs already flowing through$mcp_tool_calldata will show%5Bredacted%5Dvalues after upgrading.$identifyfrom a resource read is named by its (redacted) URI, andresource_nameis sanitized with the URL passes only so tool names are never eaten by the entropy detector.MCPErrorwrapper.The same behavior ships in PostHog/posthog-js#4830; both sanitizers produce byte-identical output on 78 shared vectors.
💚 How did you test it?
test_pipeline.py, including three timed pathological inputs; a cross-model review pass ran to completion with no findings on the final branch.📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Codex implemented this change with the
debugging-mcp-analytics,writing-tests, andwriting-pr-descriptionsskills. The paired JavaScript implementation uses the same event contract to prevent SDK drift.