feat(mcp): capture resource discovery and reads - #4830
Conversation
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 7 · PR risk: 0/10 |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (preparedEvent) { | ||
| preparedEvent.event.response = result |
There was a problem hiding this comment.
A successful resources/read request copies its complete result into $mcp_response. MCP resources can return files, configuration, or credentials in contents[].text, but the sanitizer only applies content-block redaction to a top-level content array. Sensitive resource text can therefore be sent to PostHog unless every adopter adds a custom beforeSend filter. Resource contents should be redacted by default or captured only through explicit opt-in.
How this was verified: The resource handler result flows directly into $mcp_response, and the sanitizer does not apply content-block redaction to the MCP contents response field.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/mcp/src/extensions/instrumentation.ts
Line: 458
Comment:
**Resource contents leak**
A successful `resources/read` request copies its complete result into `$mcp_response`. MCP resources can return files, configuration, or credentials in `contents[].text`, but the sanitizer only applies content-block redaction to a top-level `content` array. Sensitive resource text can therefore be sent to PostHog unless every adopter adds a custom `beforeSend` filter. Resource contents should be redacted by default or captured only through explicit opt-in.
**How this was verified:** The resource handler result flows directly into `$mcp_response`, and the sanitizer does not apply content-block redaction to the MCP `contents` response field.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Graphite Automations"sdk release label" took an action on this PR • (09/07/26)1 label was added to this PR based on Adam Bowker's automation. "Add graphite merge queue [copy]" took an action on this PR • (09/07/26)2 labels were added to this PR based on Lucas Faria's automation. |
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-handler test with successful and failing reads containing an invented token. Both new cases fail before the fix. Document the capture boundary and beforeSend. Validation: 717 MCP unit tests pass; dependency/package builds and MCP lint/format checks pass. MCP App wire probes pass 10/10 checks on each protocol generation.
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. All 732 MCP tests passed; lint, formatting, and dependency/package build passed. MCP Apps wire probes passed 10/10 on both v1 and v2. Regression coverage includes encoded keys, duplicate query parameters, malformed URLs, and success/error events.
Resolve the architecture documentation conflict by preserving resource-body exclusion and the current model-metadata behavior. Keep upstream SDK and build changes intact. Validation of the resolved resource code: 742 MCP tests, MCP/dependency builds, lint, package formatting, and 20/20 MCP App wire checks passed. Latest main changes are outside MCP. CodeScene merge-wide findings include upstream browser/replay complexity; unrelated refactors are deferred.
Redact URLs over 8,192 characters or 128 query fields before parsing credentials. Bound empty-query-field handling and preserve the original request passed to resource handlers. Document the limits. Parameterized length/field boundary cases fail before the fix and pass afterward. Validation: 742 MCP tests; builds, lint, package formatting; MCP App wire probes 20/20 across SDK v1 and v2. The scoped CodeScene pre-commit gate passes with stable sanitizer health.
gesh
left a comment
gesh
left a comment
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
QA Swarm review complete. See inline comments.
| const URL_PATTERN = /\b[a-z][a-z0-9+.-]{0,63}:\/\/[^\s<>"']+/gi | ||
| const MAX_URL_LENGTH = 8192 | ||
| const MAX_URL_QUERY_FIELDS = 128 | ||
| const SENSITIVE_QUERY_KEY_PATTERN = |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[security-audit/sensitive-data-exposure] 🟡 MEDIUM
Description: SENSITIVE_QUERY_KEY_PATTERN and SENSITIVE_KEY_PATTERN are fully anchored (^(...)$), so only exact parameter names redact. Compound credential names pass through unredacted: private_token (GitLab PAT), oauth_token / oauth_signature (OAuth 1.0a), id_token (OIDC), auth_token, session_token, secret_key, subscription-key (Azure APIM). Coverage looks arbitrary to a caller: token redacts but auth_token does not; signature redacts but oauth_signature does not. Verified by executing the shipped sanitizeUrl against each name.
Data flow: resources/read {uri: "https://gitlab.example.com/...?private_token=glpat-..."} → captureResourceRequest sets resourceName and parameters → sanitizeEvent → sanitizeUrl → shouldRedactQueryKey("private_token") returns false → the live token lands in $mcp_resource_name and $mcp_parameters.request.params.uri, and on the $exception sibling when the read fails.
Exploit: one resource read with ?private_token=glpat-... writes a live bearer credential verbatim into retained analytics that every member of the host's PostHog project can read.
Fix: match separator-delimited segments instead of the whole key, e.g. test each key against /(^|[-_.])(auth|token|secret|password|passwd|pwd|credential|signature|sig|key|hmac|sas|bearer|jwt)([-_.]|$)/i in shouldRedactQueryKey, and keep the exact presigned-URL names as-is. Over-redacting a benign sort_key is the correct trade for an analytics payload. Note: every query-key test in the new suites is a positive test for a name already in the pattern; one negative test for a compound name would have caught this.
Confidence: High — confirmed by executing the exact shipped code.
There was a problem hiding this comment.
Fixed in 9d8b88b. Query keys are matched per -/_/. segment (auth|token|secret|password|passwd|pwd|credential|signature|sig|key|hmac|sas|bearer|jwt|session|sessionid), so private_token, oauth_token, oauth_signature, id_token, auth_token, session_token, secret_key, subscription-key all redact; the exact list keeps code, AWSAccessKeyId, GoogleAccessId, Policy (code stays exact so country_code survives). The table now includes the compound names and a sort_key over-redaction row. Same change shipped to the Python SDK.
| if (!changed) { | ||
| return value | ||
| } | ||
| url.search = query.toString() |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[security-audit/sensitive-data-exposure] 🟢 LOW
Description: sanitizeUrl rewrites url.search but never touches url.hash, so a credential in the fragment survives even when the same parameter name in the query would redact. OAuth 2.0 implicit-flow responses put access_token and id_token in the fragment by specification, so this is a standard credential shape, not only "application-specific" fragment data.
Exploit: resources/read {"uri": "https://example.com/guide#access_token=ya29....&token_type=bearer"} — the captured $mcp_resource_name keeps the full token. Verified by execution.
Fix: after the query rebuild, parse url.hash.slice(1) with URLSearchParams, apply the same shouldRedactQueryKey pass, and reassign url.hash only when a key matched, so plain anchors like #section stay byte-for-byte.
Confidence: Medium — the leak is confirmed by execution, but ARCHITECTURE.md documents fragments as out of scope with beforeSend as the escape hatch, so this may be an accepted risk. Filed because the fix is a few lines and reuses vocabulary already present.
There was a problem hiding this comment.
Fixed in 9d8b88b. A fragment containing = gets the same key pass; url.hash is reassigned only when a key matched, so #section-2 stays byte-for-byte. Covered by the #access_token=... row.
| query.append(key, REDACTED_VALUE) | ||
| changed = true | ||
| } else { | ||
| query.append(key, item) |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[convergent: router + security-audit/sensitive-data-exposure] 🟢 LOW
Description: retained query values are appended verbatim and never re-examined, so a URL nested inside a non-sensitive query parameter keeps its own credentials. The greedy [^\s<>"']+ in URL_PATTERN swallows the inner URL as part of the outer match, so the inner URL never gets its own sanitizeUrl pass. Both reviewers flagged this independently.
Exploit: resources/read {"uri": "https://gateway.example.com/fetch?url=https://svc:s3cr3t@internal.example.com/doc"} — the inner basic-auth password lands in $mcp_resource_name unchanged (also verified for the percent-encoded form, and for the case where another key sets changed — re-serialization only percent-encodes the inner credential, it stays readable).
Fix: for each retained query value, re-run the value through sanitizeUrl when it matches URL_PATTERN (after one guarded decodeURIComponent attempt, one level of recursion to bound the work).
Confidence: Medium — the redaction failure is confirmed by execution; prevalence is uncertain since it needs a gateway/proxy-shaped resource URI. The ARCHITECTURE.md caveat partially discloses this, but userinfo redaction is the PR's headline control, so the gap is worth closing.
There was a problem hiding this comment.
Fixed in 9d8b88b (tightened in 57f9aca). A retained query or fragment value containing :// is run through the URL sanitizer one level deep; past that budget a URL-bearing value is replaced with [redacted] rather than trusted, so a gateway wrapping a gateway cannot carry credentials through. Covered by the single and double nested gateway rows.
| const SENSITIVE_KEY_PATTERN = | ||
| /^(authorization|cookie|set-cookie|x-api-key|api[-_]?key|api[-_]?token|access[-_]?token|refresh[-_]?token|token|password|secret|client[-_]?secret|private[-_]?key)$/i | ||
|
|
||
| const URL_PATTERN = /\b[a-z][a-z0-9+.-]{0,63}:\/\/[^\s<>"']+/gi |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[security-audit/sensitive-data-exposure] 🟢 LOW
Description: the leading \b in URL_PATTERN requires a word boundary before the scheme, but _ is a word character and is not in the scheme class [a-z0-9+.-]. A URL immediately preceded by an underscore therefore matches nowhere, so sanitizeUrl is never invoked and nothing redacts at all — a total bypass of the control when hit.
Exploit: any captured string of the form ..._https://user:pass@host/..., e.g. an error message built as `read failed for uri_${uri}` — $exception_list[].value keeps the raw userinfo credential. Verified: resource_https://user:s3cr3t@example.com/doc returns unchanged, while a digit prefix redacts fine.
Fix: replace the leading \b with a negative lookbehind that excludes only scheme-legal characters plus underscore: /(?<![A-Za-z0-9+.\-_])[a-z][a-z0-9+.-]{0,63}:\/\/[^\s<>"']+/gi. Node 18+ supports lookbehind and this package is Node-only.
Confidence: Medium — the bypass is confirmed by execution against the shipped pattern, but it only fires when a captured string concatenates an underscore directly onto a URI.
There was a problem hiding this comment.
Fixed in 9d8b88b by dropping the leading \b entirely. The proposed lookbehind that excludes _ still fails this case (at the h the preceding _ is excluded, and no earlier start can span _), verified by execution. With no anchor, leftmost matching reads foo.https://x as scheme foo.https, which redacts the same URL. Covered by the resource_https://user:pw@... row.
| } | ||
| stampClientIdentity(event, request, extra, server) | ||
| stampTransportIdentity(event, extra) | ||
| const identity = await handleIdentify(server, data, sessionId, request, sessionInfo, extra) |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[router] 🟢 LOW
captureResourceRequest calls handleIdentify, which builds its own $identify event using getRequestResourceName(request) — a helper that only reads params.name, never params.uri. So an $identify event fired during a resources/list / resources/read call (when options.identify is configured and identity is first-seen or changes) reports resourceName: "Unknown" instead of the resource. The primary $mcp_resource_read / $mcp_resources_list events are unaffected. Consider teaching getRequestResourceName about params.uri as a fallback — note that if you do, the sanitization gate at sanitization.ts:36 (eventType === mcpResourcesRead) must widen too, since it currently relies on this exact "Unknown" behavior to keep unredacted URIs out of $identify.
There was a problem hiding this comment.
Fixed in 5242a24. getRequestResourceName falls back to params.uri, and the gate in sanitization.ts was widened so resourceName is sanitized on every event type, so $identify from a read is named by the (redacted) uri. Test added in string-method-registration.test.ts. Python had the same gap and got the same fix.
| } | ||
|
|
||
| /** Captures a non-tool MCP request without changing its result or error semantics. */ | ||
| export async function captureResourceRequest(params: TraceRequestParams): Promise<unknown> { |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[router] 🟢 LOW
captureResourceRequest re-implements the prepare → execute → publish-success / publish-failure lifecycle already factored into prepareToolCallEvent / publishSuccessfulToolEvent / publishFailedToolEvent for tool calls (understandably — resources have no ownership/conversation/context concerns). Correct and well tested today, but ~60 lines of near-duplicate logic that can silently drift from the tool-call lifecycle's error/duration/logging conventions in future changes.
There was a problem hiding this comment.
Partly addressed in 5242a24. The two resource publish blocks collapsed into publishResourceEvent, and the adapter tables share a traceResourceRequest factory (about 35 lines removed). I stopped short of folding in publishSuccessfulToolEvent / publishFailedToolEvent: both compute their outcome (isToolResultError, the minted-conversation reset) inside the same guard as the publish, so a shared helper would either narrow that guard or nest two trys, and neither read simpler.
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated reviewers (qa-team, paul-reviewer, xp-reviewer, security-audit as warranted) Verdict: 💬 APPROVE WITH NITS (round 1 @ a3e4034)The instrumentation itself is correct, idiomatic, and verified end-to-end (742/742 unit tests, lint clean, dual-era harness 10/10 on SDK v1 and v2, with the pre-existing Key findings
Convergence
Reviewer summaries
Automated by QA Swarm — not a human review |
gesh
left a comment
gesh
left a comment
There was a problem hiding this comment.
Looks great!
Approving to unblock, have a look at the inline comments
`sanitizeUrl` missed several shapes a reviewer found: - The pattern's leading `\b` meant `resource_https://user:pw@host` matched nothing at all — `_` is a word character but not scheme-legal, so no boundary exists there. Dropped it. - The terminal class absorbs prose punctuation, so `See <url>, then...` parsed the comma as part of the address. A trailing run of `.,;:!?)]}` is now split off before parsing and re-appended verbatim. Split by a linear backward scan rather than a `$`-anchored pattern: that pattern backtracks quadratically (~40ms) on a long punctuation run that does not end the match, on an attacker-influenceable string. - Credential-named query keys were matched whole, so `private_token`, `oauth_signature`, `subscription-key` and friends were captured in the clear. A key is now sensitive when any `-`/`_`/`.`-delimited segment is; `code` stays whole-key-only so it does not eat `country_code`. - `;` is now normalized to `&` before splitting fields, fragments get the same key pass when they look like a field list, and a URL nested in a retained value is sanitized one level deep. Untouched parts are returned byte-for-byte — only a part that was actually redacted is re-serialized. Tested: `pnpm --filter @posthog/mcp test:unit` (770 passed), extending the parametrized `mcp-payloads.test.ts` tables with the review's vectors plus a must-not-redact `country_code` row and a quadratic-backtracking guard. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
`resources/templates/list` was not instrumented, so a server whose resources are all templated looked like it served no resources at all. Both adapters now patch it, publishing the same `$mcp_resources_list` event — the captured `request.method` is what separates a static listing from a templated one. List events now carry the handler result as `$mcp_response`. A listing is discovery metadata (names, uris, uri templates, mime types, next cursor), not a resource body, and it answers "what did this client actually see?". Reads still capture no response. An empty listing is not flagged as an error the way an empty `tools/list` is: a template-only server legitimately advertises no static resources. Two smaller review findings, both about the resource name: - `getRequestResourceName` now falls back to `params.uri`, so an `$identify` published from a `resources/read` names the resource instead of reporting `Unknown`. - `sanitizeEvent` redacts `resourceName` on every event type, not only `resources/read` — `$identify` and the `$exception` sibling carry the same name, and it used to reach PostHog with its credentials intact. The two near-identical publish blocks in `captureResourceRequest` are now one `publishResourceEvent`. The tool-call publishers were left alone: they wrap outcome computation in the same `try`, so sharing the helper would either narrow that guard or nest two `try`s — neither is simpler. Tested: `pnpm --filter @posthog/mcp test:unit` (770 passed), including a new `resource-listings.test.ts` that runs both adapters against the real SDK (high-level `ResourceTemplate`, low-level v1 Zod registration), and list-property/failing-list/empty-list cases in `string-method-registration.test.ts`. `node harness/dual-era/probe-mcp-apps.mjs` 20/20 on both eras. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
The changeset now warns that URL credential redaction applies to every captured string, so URLs already flowing through `$mcp_tool_call` parameters, responses, and error messages will show `%5Bredacted%5D` values after upgrading — a visible change to existing data, not just to the new resource events. ARCHITECTURE: the patched-handler list gains `resources/templates/list`, the `$mcp_resources_list` row now names the properties it carries, and the redaction note covers fragments, segment-matched keys (with the `sort_key` over-redaction called out), and one-level nested URLs. The App probe's closing line does assert resource capture now, so the harness README no longer says it does not. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
`'` is a valid URI sub-delimiter, but the pattern's terminal class excluded it, so `https://example.com/o'reilly?token=fakesecret` matched only as far as `https://example.com/o` — the query was never parsed and the token shipped in the clear in `$mcp_resource_name` and `$mcp_parameters`. The same truncation left userinfo intact on `https://user:pa'ss@example.com/doc`. The class now excludes only what is never valid unencoded in a URI (whitespace, `"`, `<`, `>`), and `'` joins the trailing-punctuation set so a URL single-quoted in prose still has its closing quote split off and re-appended. `new URL()` leaves an apostrophe unencoded in a path, so a rewritten URL keeps it verbatim. Tested: `pnpm --filter @posthog/mcp test:unit` (774 passed) with both reported vectors plus a quoted-in-prose row added to the parametrized table, `pnpm turbo run build --filter=@posthog/mcp...`, `pnpm --filter @posthog/mcp lint`, oxfmt check, and `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
Four findings from a cross-model review of the resource-tracking branch: 1. Depth exhaustion leaked. A retained value carrying a URL was appended untouched once the one-level nesting budget was spent, so a gateway address wrapping a gateway address shipped the innermost token. Past the budget such a value is now dropped whole rather than trusted. 2. Restored prose punctuation could be a credential's own tail: `?password=fakepass!!!` was captured as `password=%5Bredacted%5D!!!`. Two rules now. A match that IS the whole string is an address, not prose — the case for `$mcp_resource_name` and `params.uri` — so nothing is split off its end. In prose, the suffix is dropped rather than restored when the last field of the URL's trailing part was rewritten; losing a comma from the sentence is the accepted cost. 3. The PostHog-token pass ran after the URL pass, so rewriting a query percent-encoded the `/` in `ref=/phx_...` and erased the `\b` boundary the token pattern needs — leaking a token that main redacts. Tokens are now redacted first. A field is counted as changed only when its value actually differs, so a value the token pass already replaced is not re-serialized (and matches what the Python sibling produces). 4. `publishResourceEvent` built the exception outside its try/catch. `captureException` reads the thrown value's `stack`, which an application error may define as a throwing getter, so analytics could replace the resource error the caller was waiting on with its own. Stamping now happens inside the try, as in `publishFailedToolEvent`. Tested: `pnpm --filter @posthog/mcp test:unit` (783 passed) with the new vectors in the parametrized URL table (double-nested gateway, token-pass ordering, and the six punctuation cases) and a resource-read test whose error has a throwing `stack` getter; `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
`sanitizeEvent` ran the generic captured-value pass over `$mcp_intent` first and `redactPii` second. The URL rewrite percent-encodes `@`, so an email narrated inside a query parameter reached `redactPii` as `email=alice%40example.com`, where the email pattern no longer matches — a regression against main, which shipped no URL rewrite at all. PII is now stripped from the raw narration first, then the generic pass runs. `Open https://example.com/?email=alice@example.com&token=fakesecret` becomes `Open https://example.com/?email=%5Bredacted%5D&token=%5Bredacted%5D`: both the address and the credential are gone, and the host stays so the intent is still readable. Tested: `pnpm --filter @posthog/mcp test:unit` (784 passed) with a new `sanitization.test.ts` case asserting the email and the token are both absent while the host survives; `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
An MCP resource URI need not have an authority. `resource:guide?token=x` and `file:/guide.md?token=x` never matched a pattern that required `//`, so the token shipped in the clear in `$mcp_resource_name` and `$mcp_parameters`. The authority is now optional — the terminal class absorbs `//host` when there is one — and the nested-value check asks the same pattern instead of looking for a literal `://`. That sweeps up ordinary prose (`Error:foo`, `at12:30`, `C:\path`, a log line's `ERROR:root:`, an ISO timestamp's `T13:40:`). It costs nothing: a match with nothing to redact is returned byte-for-byte, `new URL()` only rejects on host and port problems that an authority-less string cannot have, and 200KB of colon-dense log text sanitizes in 11ms. One case did need a rule. Over `MAX_URL_LENGTH` the match was dropped whole, which is right for an address but destroyed long unspaced blobs the widened pattern now reaches — a 10KB non-base64 `data:` URI, which `sanitizeString` deliberately passes through, became `[redacted]`. The length bound now drops only a match that opens with an authority; an over-long authority-less match is left alone. The narrow leak that leaves (an 8KB+ authority-less URI carrying a credential) is worth less than the payload fidelity it buys, but it is a judgement call worth a reviewer's eye. Tested: `pnpm --filter @posthog/mcp test:unit` (794 passed), with both reported vectors and six byte-for-byte prose rows in the parametrized table, plus a real-SDK `resources/read` of `resource:guide?token=...` on both adapters asserting the token reaches no capture; `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
…them Follow-up to 9a7494a, which returned an over-long authority-less match raw so a long `data:` URI would survive the length bound. Raw was the wrong fallback: an authority-less URI over 8,192 characters could still carry a credential in its query. The bound now guards only what it was written for — the parsing an attacker-shaped address can force, which needs an authority. An authority-less match falls through to normal sanitization: `new URL()` is linear, the 128-field bound still applies, and a match with nothing to redact comes back byte-for-byte, so the long non-base64 data URI `sanitizeString` deliberately passes through is still untouched while a 9,000-character `resource:...?token=...` now loses its token. Tested: `pnpm --filter @posthog/mcp test:unit` (795 passed), adding that 9,000-character row to the parametrized table and keeping the data-URI row in the byte-for-byte table; `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
The authority-optional pattern sweeps a colon-suffixed prose word into the same match as the URL that follows it, so `Failed URL:https://alice:hunter2@example.com/doc` parsed as scheme `URL` with the real address as its opaque path — no username, no password, and the userinfo redaction never fired. `sanitizeUrl` now looks for the first scheme that brings an authority before parsing. Found past position 0, the leading run is prose: it is kept verbatim and the rest is sanitized as the address. A match with no authority anywhere (`see:resource:guide?token=x`) is still parsed whole, so authority-less resource URIs keep working. The recursion is one deep by construction — the slice starts at the authority. Tested: `pnpm --filter @posthog/mcp test:unit` (799 passed), with the three redaction rows and the byte-for-byte `Note:https://example.com/doc` row added to the parametrized table; `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Colon-dense log text still sanitizes at ~9ms per 186KB. Reviewer note: the prose run before the authority is returned verbatim, so a contrived unspaced value that packs a credential-bearing authority-less URI *before* a real URL — `resource:g?token=s+https://a@b` — keeps that first token. Sanitizing the prefix through `sanitizeUrlsInString` would close it; left alone here to stay in step with the Python sibling, which takes the same shape. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
`[a](https://public.test/#intro)[b](https://user:password@private.test/doc)` is one match: the address split stops at the first `#`, so the second link ends up inside the first one's fragment. That fragment has no `=`, so it was skipped as prose and the credentials were published. A fragment with no `=` is not a field list, but it is still text — and text can carry an address. It now goes through the URL pass one level deep, and the hash is rewritten only if that changed something, so a real prose fragment stays byte-for-byte. No field was rewritten either way, so the trailing-punctuation rule is unaffected. Tested: `pnpm --filter @posthog/mcp test:unit` (811 passed), with the reported input and its credential-free twin added to the parametrized tables, and every other fragment row re-verified unchanged (`#section-2`, `#part`, `#section`, the prose `#intro,` tail, the OAuth field list, the `#/callback?` route, and the `#k=v&next=https://…?p=1` row); `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
|
One more, pushed on top (same in posthog-python#928): a plain fragment (no |
…ddress Three findings on 7aa9ee9, all in the same code path: 1. The text in front of a fragment's fields was preserved verbatim as a route, so `#https://user:password@private.test/doc?page=1` — where the `?` precedes the first `=` — published the credentials. That text gets the same URL pass a plain fragment gets; the hash is rewritten when either the text or the fields changed, and an untouched field list goes back verbatim rather than re-serialized. 2. The fragment text pass recursed once per `#`, because a fragment is itself a URL whose fragment is itself a URL — `resource:x#` ten thousand times overflowed the stack after ~1.4s. It is now capped like a field value: at the nested level a URL-bearing text is dropped whole instead of descended into, so depth is at most two. 3. The address split recursed once per address, so ten thousand URLs run together did the same. It is one pass now: the cut points are collected up front and each piece sanitized on its own. No piece can need cutting again — every cut precedes the value's own `?`/`#`, so only the last piece holds field data and every authority inside it sits in that data. The length bound moves onto the individual piece, which is what lets a long run of joined addresses be sanitized rather than dropped whole. `;`/`&` normalization now lives in one place, so the field bound and the field parse cannot disagree about what a separator is. Tested: `pnpm --filter @posthog/mcp test:unit` (815 passed), with both fragment rows in the parametrized table and a parametrized pair for the two pathological inputs — `resource:x#`×10,000 -> `resource:x#resource:x#[redacted]` in 3ms, and 10,000 joined addresses -> the same string with only the userinfo redacted in 11ms, both under a 1s budget where they previously threw or crawled. Every existing row re-verified unchanged. `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
|
One more hardening commit, pushed on top (same in posthog-python#928): a fragment route prefix that is itself an address ( |
`#/docs/id=1?token=fakesecret` was left alone. Route detection keyed only on the fragment's first `?` preceding its first `=`, and here the route itself holds an `=` — so the whole thing was read as one field named `/docs/id` with the token buried in its value, where no credential name could match. A fragment is now a route when it has a `?` and either starts with `/` or puts that `?` ahead of any `=`. The second half is the old rule, which still covers `#callback?k=v` and `#https://user:pw@host/doc?page=1`. Failing both, it is a field list if it holds an `=`, and plain text otherwise. Tested: `pnpm --filter @posthog/mcp test:unit` (817 passed), with the route row and a byte-for-byte `#/docs/id=1` row added to the parametrized tables and every existing fragment row re-verified unchanged; `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Reviewer note: `#/token=fakesecret` is NOT redacted, and this change does not affect it either way — it is a field list whose key is `/token`, and `/` is not one of the `-`/`_`/`.` segment delimiters the credential-name rules split on, so `/token` matches nothing (`/access_token` does match, via the `_`). Fixing that means widening the segment delimiter, which is a shared rule and wants deciding for both SDKs at once, so it is left here rather than guessed at. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
Follow-up to 23d911a, which left `#/token=fakesecret` untouched: that fragment is a field list whose key is `/token`, and the segment rule knew only `-`, `_` and `.` as separators, so the leading slash hid the name. `/` is now a separator on both sides of the rule. It over-redacts `sort/key` the way it already over-redacts `sort_key` — the same accepted trade, now noted in the comment. Re-serializing a key percent-encodes the slash, so the fragment comes back as `#%2Ftoken=%5Bredacted%5D`. Tested: `pnpm --filter @posthog/mcp test:unit` (818 passed), with that row added to the parametrized table and every existing key row re-verified — `#/docs/id=1?token=…` still redacts through its route, `#/docs/id=1`, `#/docs?page=2` and `#/callback` are still byte-for-byte; `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
|
Two more, pushed on top (same in posthog-python#928): a hash route that carries its own |
`[a](https://public.test/?download)[b](https://alice:pw@private.test/doc)` published its credentials: the split stopped at the first `?`, so the second link landed inside the first one's query — where `URLSearchParams` made it part of a KEY, and keys are never sanitized. The `?`/`#` boundary is gone. The match is cut at every authority past position 0 whose preceding character is not `=`; an authority right after `=` is a field's value and stays with its field, which the field pass already hands to the nested pass. That is one rule instead of a boundary plus a key special case, and it still needs no recursion: after the cuts every authority left inside a piece is either at its start or in value position. Tested: `pnpm --filter @posthog/mcp test:unit` (821 passed), adding the three reported inputs and re-verifying every existing row byte-for-byte — both gateway `?url=https://…` rows (value position, uncut), `Failed URL:`, `a:b:`, `a:b://c@d`, the comma-joined pair, the markdown pairs, the fragment-route rows, `file:/guide?password=…&url=`, and the two pathological inputs (still 3ms and 11ms). `resource:g?token=…+https://…@b` also holds, because `+` is scheme-legal: the leftmost authority match starts at `fakesecret+https://`, whose preceding character is the `=`. `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
`#/token=fakesecret&next=https://other.test/?page=1` leaked its token: the fragment starts with `/` and holds a `?`, so the route heuristic took everything before that `?` as text — but it is a field list whose `next` value merely contains a `/`. Shape cannot separate that from `#/docs/id=1?token=…`, whose route really does carry an `=`. So the heuristics are gone. The fragment is split at its first `?`, and each part is read as credential-named fields when it holds an `=` and as text otherwise, then reassembled around the `?`. A part is re-serialized only if something in it was rewritten, so an untouched side keeps its own encoding. One existing expectation moves with this: the tail of `#access_token=…&next=https://other.test/?page=1` is now its own field list rather than part of the head's `next` value, so it comes back as `?page=1` instead of `%3Fpage%3D1`. Nothing is redacted differently. Tested: `pnpm --filter @posthog/mcp test:unit` (822 passed), with the reported input added and every other fragment row re-verified byte-for-byte — `#/docs/id=1?token=…`, `#/callback?token=…`, `#/token=…`, `#https://user:pw@host/doc?page=1`, `#/docs?page=2`, `#/callback`, `#/docs/id=1`, `#section-2`, `#part`, `#section`, the OAuth field list, the prose `#intro,` tail rule, and both markdown pairs; `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
|
Two more, pushed on top (same in posthog-python#928): an adjacent address is now split off wherever it sits unless it directly follows |
`https://host/x?token=foo%20https://secret.test/private` cut the inner address out of the token's value, so the tail `https://secret.test/private` survived beside the `%5Bredacted%5D` the field pass produced. Checking only the character immediately before an authority is too narrow: what matters is whether a field name opened the run it sits in. Value position is now decided by reading backwards to the nearest `=`, `&`, `;`, `?` or `#` — an `=` means a field name came first, so the address belongs to that field's value and goes through the nested pass with it. The scan only applies inside the fields region: before the first `?`/`#` an `=` is a path character, so `…/redirect=https://user:pw@host` and `…/a=b/c,https://user:pw@host` are adjacent addresses and are still cut apart. One existing expectation moves with this: in `?q=see,https://fakeuser:fakepass@x.test/doc` the address is now part of `q`'s value, so it is redacted through the nested pass and the field is re-serialized. Nothing is left unredacted either way. Tested: `pnpm --filter @posthog/mcp test:unit` (825 passed), with the three reported inputs added and every other row re-verified byte-for-byte — `?download)[b](https://`, `?https://`, `#intro)[b](`, `#https://…?page=1`, `#/token=…&next=https://o/?page=1`, `Failed URL:`, `a:b:`, `a:b://c@d`, the comma-joined pair, both gateway rows, `file:/guide?password=…&url=`, `resource:g?token=…+https://…@b`, and the two pathological inputs (3ms and 11ms); `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
`#password=prefix?fakesecret` split into a head that was redacted and a tail that was read as text and kept, publishing the rest of the password. A `?` inside a credential is indistinguishable from the one between a route and its fields, so shape cannot tell them apart. When the head's last field was rewritten, the tail may be the remainder of that value, so it is replaced whole instead of sanitized. An empty tail has nothing to hide and stays empty. Either case counts as a rewritten trailing field, so prose punctuation after such a URL is dropped too — it could equally have been part of the credential. A head whose last field was untouched is unaffected, so `#/docs/id=1?token=…` and `#/callback?token=…` still redact only the tail's own fields. Tested: `pnpm --filter @posthog/mcp test:unit` (829 passed), with both reported inputs plus the empty-tail and prose-punctuation cases added to the parametrized table, and every other fragment row re-verified unchanged. `new URL()` leaves the brackets literal in a fragment, so the output is `#password=%5Bredacted%5D?[redacted]` as expected. `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
|
Two more, pushed on top (same in posthog-python#928): 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 |
Since `/` left the structural set, an authority in the fields region scans back to the nearest `=&;?#` — which for `https://a.test/?` + `https://b.test/x,` x4000 is the same `?` for every one of the four thousand addresses. That is quadratic: 68 KB took 1573 ms on the event loop, synchronously, on attacker-influenceable input. The backward scan is replaced by a single forward walk that tracks the last structural character seen and whether the fields region has opened, deciding each authority from that state as it passes. Same rule, same results, linear: the same input now takes 7 ms. Tested: `pnpm --filter @posthog/mcp test:unit` (830 passed) — every existing row keeps its expectation, and the 68 KB input joins the timed pathological set (returns unchanged, no credentials in it), alongside the `resource:x#` x10,000 and joined-addresses cases at 3 ms and 12 ms. `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
`https://example.com/?token=prefix?https://secret.example/private` cut the token's value in half: the second `?` sits inside that value, but the forward pass counted every `?` as structural, so the address after it was split off and survived beside the `%5Bredacted%5D`. A URL has exactly three delimiters — the query's, the fragment's, and the one between the fragment's own head and tail, matching how the fragment is split. Only those, plus the field separators `=`, `&` and `;`, are structural now; any other `?` or `#` is ordinary text inside a value. Tested: `pnpm --filter @posthog/mcp test:unit` (830 passed), with that input and a companion whose fragment delimiter does still make the address adjacent, and every existing row re-verified — the two path-`=` rows, both gateway rows, `?q=see,https://`, `?token=foo%20https://`, `?download)[b](`, `?https://`, `#intro)[b](`, `#https://…?page=1`, and the three pathological inputs, the 68 KB one still at 9 ms. `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
`#password=phx_…?private-suffix` kept its tail. The PostHog-token pass runs before the URL pass, so by the time the field list was read the value already said `[redacted]`; comparing it found no change, and the fail-closed tail rule never fired. Whether a field list ends in a credential is a separate question from whether anything was rewritten. A field list now reports `lastFieldSensitive` — the last field's key is credential-named, or its value changed — and that flag drives both the fragment tail and the trailing-punctuation rule. `changed` keeps its own meaning, so an untouched field list is still put back verbatim rather than re-serialized: the head above keeps its literal brackets. Tested: `pnpm --filter @posthog/mcp test:unit` (832 passed), with that input added and every existing row re-verified — `#password=prefix?…`, the empty-tail and prose-punctuation cases, `#/docs/id=1?token=…`, `?sig=…&page=2, then retry.` (comma kept, last field benign), and `Failed (…?a=b).`; `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
`?password=prefix;remainingsecret` was captured as `?password=%5Bredacted%5D&remainingsecret=`: normalizing `;` to `&` before parsing cut the password in two and republished its tail as a bare field name. Nothing is normalized now — fields parse on `&`, and the 128-field bound counts `&` too. Legacy `;`-separated fields are still handled, but by failing closed rather than by splitting: a value carrying a `;` is dropped whole whenever any of its `;`-separated pieces names a credential. A sensitive key already took its whole value, `;` and all. One existing expectation moves with this: `?a=1;token=fakesecret` now redacts the whole `a` value rather than reinterpreting the tail as a `token` field. Nothing is left unredacted either way, and `?a=1;b=2` is still returned byte-for-byte. Tested: `pnpm --filter @posthog/mcp test:unit` (834 passed), with the reported input added, that row updated, and every other row unchanged; `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. ARCHITECTURE.md needed no change — it never described the normalization. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
`sanitizeIntent` ran `redactPii` first, so a PostHog key whose middle happens to be phone-shaped — `phx_AAAAAAAA-415-555-0142-AAAA…` — lost only that run and shipped as `phx_AAAAAAAA-[redacted]-AAAA…`, with both halves of the token intact. Main removed the whole thing. `sanitizeText` is split into its two passes, `redactCredentials` and `redactUrls`, and the intent now runs binary gate, credentials, PII, URLs. Every ordering constraint is written down beside them: credentials before PII because a PII pattern can cut a token in half, PII before URLs because the URL rewrite percent-encodes the `@` the email pattern needs, and the binary gate first because splicing `[redacted]` into a blob stops it looking like base64. Ordinary captured strings are unchanged — credentials, then URLs. Tested: `pnpm --filter @posthog/mcp test:unit` (835 passed), with that intent added and the existing intent cases re-verified — the email inside a URL, the token-plus-email compose case, the base64 blob, and the plain-PII rows; `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
|
Four more, pushed on top (same in posthog-python#928): value position is decided in one forward pass instead of a backward scan per address, which was quadratic on thousands of joined URLs (a42ee10); only a URL's real delimiters count as structural, so a second |
Two ways a credential's tail escaped the field rules: A legacy `;`-separated pair parses into a single key, and `?download;token=fakesecret` matched nothing — the key was `download;token`, and the `;` check only looked at values. `;` joins `-`, `_`, `.` and `/` as a segment separator in a credential name, so that key is now sensitive and takes its whole value. And a credential's own suffix was being split away before anything could fail closed on it. In `#password=prefix?https://private.example/rest` the fragment-tail `?` was read as a delimiter, so the address after it was cut off as adjacent and the tail rule never saw `rest`; `;` did the same in a query. `;` is no longer structural in the address split — fields parse on `&`, so it is a value character — and the fragment-tail `?` counts as a delimiter only when the last structural character before it is not `=`. Where it is, the address stays attached and the fragment split plus its fail-closed tail rule decide, which they can, because they see the whole credential. A head that proves benign still lets its tail through: `#/docs/id=1?https://user:pw@x.test/doc` keeps the route and sanitizes the address as the tail's own text. Tested: `pnpm --filter @posthog/mcp test:unit` (839 passed), with the four reported inputs added and every existing row re-verified — `?a=1#b?https://…` still splits (its fragment delimiter follows a `#`, not an `=`), `?token=prefix?https://…`, `?a=1;b=2`, `?a=1;token=…`, `#/docs/id=1?token=…`, `#https://user:pw@host/doc?page=1`, and the 68 KB input still at 8 ms; `pnpm turbo run build --filter=@posthog/mcp...`; `pnpm --filter @posthog/mcp lint`; oxfmt check; `node harness/dual-era/probe-mcp-apps.mjs` 20/20. Claude-Session: https://claude.ai/code/session_01VGVQTsHUk5dmQC2rGPEgc7
|
One more, pushed on top (same in posthog-python#928): |
…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
2 participants
Problem
MCP server authors cannot tell whether clients discover or read their resources, including MCP App UI resources.
Changes
Resource discovery now emits
$mcp_resources_list.Resource reads emit
$mcp_resource_readwith the URI, duration, and error state without capturing resource bodies.The wrappers preserve original results and errors across MCP SDK 1.x and 2.x server shapes.
The dual-era MCP App probe now verifies resource analytics under both protocol generations.
resources/templates/listis instrumented alongsideresources/list; listings carry their metadata as$mcp_response.Captured URLs redact credentials: userinfo, credential-named query and fragment fields (matched per
-/_/.///;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.Credential and PII redaction run before URL rewriting so percent-encoding cannot hide a token or an email; the narrated intent is gated for binary blobs first. Work is linear in the input: no per-address backward scans, no recursion per fragment or address.
$identifyfrom a resource read is named by its (redacted) URI;captureExceptionfor a failed resource request runs inside the publish guard so analytics can never replace the handler's error.The same behavior ships in PostHog/posthog-python#928; both sanitizers produce byte-identical output on 78 shared vectors.
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto 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 design uses the existing request-handler seam and keeps analytics failures isolated from MCP behavior.