Repository navigation
fix(mcp): keep identifiers split by the intent scan cut out of events - #452
Open
lucasheriques wants to merge 1 commit into
Open
lucasheriques wants to merge 1 commit into
lucasheriques wants to merge 1 commit into
Conversation
posthogmcp cut $mcp_intent (and the unknown tool name) to twice its limit before redacting, then kept the first limit bytes. An identifier the cut split no longer matched its pattern, and once earlier redactions shrank the text, its raw fragment (`ada.lovelace@exampl`, `phc_Ab3dEf6hIj9`, a partial card or phone) landed inside the kept limit. Same leak as posthog-ruby#349 and posthog-python#1217. boundedFreeText now follows the sdk-specs bounded scan: text over limit + 1,024 bytes is redacted cut at limit + 1,024 and cut at limit, and only the common prefix is kept. The binary gate runs on the uncut text, so a base64 intent becomes the binary placeholder instead of 2,045 raw characters (the 4,096-byte pre-cut kept it under the 10,240-byte gate). Tested: go test ./posthogmcp/ (plus posthogmcpgo, posthogmcpsdk) pass. TestCaptureFreeTextNeverKeepsAnIdentifierSplitByAScanCut fails 10 cases on main (token, email, card, SSN, phone at the old 2x-limit cut, for intent and unknown tool name); the binary case of TestCaptureToolCallBoundsALongIntent fails on main too. Long prose keeps byte-identical output. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PR overviewThe PR changes MCP intent sanitization to keep identifiers split by scan cutoffs out of captured events. One issue remains open, with none addressed so far: URL userinfo spanning both scan cutoffs can escape redaction and leave credentials in the captured $mcp_intent event. This is a concrete credential-exposure regression, though the reported impact is limited to specially structured intent URLs. Open issues (1)
Fixed/addressed: 0 · PR risk: 6/10 Reviews
|
Contributor
posthog-go Compliance ReportDate: 2026-11-13 23:13:53 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
|
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 8b4d917: 1 Must fix, 1 Should fix.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
Comment on lines
+503
to
+505
| withMargin := sanitizeFreeText(prefixUTF8(value, limit+scanMarginBytes)) | ||
| atLimit := sanitizeFreeText(prefixUTF8(value, limit)) | ||
| return truncateUTF8(commonPrefix(withMargin, atLimit)+truncationSuffix, limit) |
Contributor
There was a problem hiding this comment.
P1 · Prevent URL userinfo spanning both scan cuts from leaking
With a URL starting at intent byte 1900 and a 1,300-byte password before @localhost, both prefixes end before @. sanitizeFreeText leaves both URLs unchanged, so commonPrefix emits https://ada: followed by 133 password bytes. The previous implementation redacts this input. Conservatively discard URL authorities that cross both scan boundaries.
| if len(value) <= limit+scanMarginBytes { | ||
| return truncateUTF8(sanitizeFreeText(value), limit) | ||
| } | ||
| if isBinaryBlob(value) { |
Contributor
There was a problem hiding this comment.
P2 · Bound the binary check before scanning large intents
A multi-megabyte base64 intent now sends its entire value through isBase64Like's regexes synchronously; 16 MiB takes about 400 ms in isolation. This defeats the spec's limit-proportional privacy checks, required by AGENTS.md:12. Implement the binary gate using the spec's first/last 1,024-character samples.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| if isBinaryBlob(value) { | ||
| return binaryRedactedValue | ||
| } | ||
| withMargin := sanitizeFreeText(prefixUTF8(value, limit+scanMarginBytes)) |
There was a problem hiding this comment.
Medium: Both scan cuts can hide URL userinfo
For an intent of "https://ada:hunter2" + strings.Repeat("x", 3080) + "@internal/", both prefixes end before @, so neither pass recognizes the userinfo: the captured $mcp_intent starts with https://ada:hunter2xxxx..., whereas the previous implementation returns https://***@internal/. Redact or omit URL authorities that cross the scan boundary rather than treating agreement between two incomplete scans as evidence that their common prefix is safe.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 Motivation and Context
posthogmcpcut$mcp_intentto twice its limit (4,096 bytes) before redacting, then kept the first 2,048. An identifier that the first cut split in half no longer matches its pattern. Redactions earlier in the text shrink it (an email of 39 bytes becomes[redacted], 10 bytes), so the raw fragment lands inside the kept limit. The unknown tool name (CaptureUnknownTool) had the same shape at 512/256 bytes. This is the leak fixed in PostHog/posthog-ruby#349 and PostHog/posthog-python#1217. It reachesposthogmcpgoandposthogmcpsdkusers through the core.What
maincaptures when a start of about 100 emails precedes the identifier at the cut:$mcp_intentonmainends withphc_Ab3dEf6hIj9lMn2pQr5tUv8xYz1bCd… phc_Ab3dEf6hIj9...ada.lovelace@example.com… ada.lovelace@exampl...4111 1111 1111 1111… 4111 1111 1...123-45-6789… 123-45...(415) 555-0142… (415) 555...boundedFreeTextnow follows the sdk-specs bounded scan (mcp-analyticsspec, "These passes SHALL cost in proportion to the property limits"). Text up to limit + 1,024 bytes is redacted whole. Longer text is redacted twice, once cut at limit + 1,024 and once cut at the limit, and only the common prefix is kept, followed by.... A match split by the cut at the limit is redacted in the first result and raw in the second. A match split by the margin cut lies past the end of the second result. So neither reaches the event, and the cost still follows the limit, not the input.The binary gate also runs on the uncut text now, as the spec and Ruby do. On
main, a 10 KB base64 intent became 2,045 rawAs plus..., because the 4,096-byte pre-cut kept it under the 10,240-byte gate. It now becomes the binary placeholder.truncateUTF8was already a linear pass over the whole intent (ToValidUTF8), so this adds no new cost class.Long benign text keeps its start. The new test pins byte-identical output for long prose and for long CJK text.
💚 How did you test it?
TestCaptureFreeTextNeverKeepsAnIdentifierSplitByAScanCutplaces a PostHog token, email, card, SSN, NANP phone and URL userinfo across the cut at the limit, at limit + margin, and at the old 2x limit, for both$mcp_intentand the unknown tool name. Onmain, 10 cases fail: every identifier except URL userinfo, at the 2x cut, for both fields. With this PR all 36 pass.TestCaptureToolCallBoundsALongIntent: prose and CJK keep their start, and binary data becomes the placeholder. The binary case fails onmain.go test ./...inposthogmcp,posthogmcpgoandposthogmcpsdk, plusgo vetandgofmt: all pass.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
main, text from bytes 2,048 to 4,096 could fill the gap, which is how the leak happened.posthogmcpgoandposthogmcpsdkmodules require corev1.44.1. Users get the fix by upgradingposthog-goonce this releases.🤖 Generated with Claude Code