Repository navigation
fix(mcp): bound intent and parameter capture work, redact feedback extra keys and numbers - #1217
Open
lucasheriques wants to merge 4 commits into
Open
lucasheriques wants to merge 4 commits into
lucasheriques wants to merge 4 commits into
Conversation
…edback extras Each fails on main: a 1MB separated-digit intent scans for seconds, captured parameters read every argument and recurse past the depth limit (a 1000-deep argument raises RecursionError), a top-level credential-named argument is captured whole, and object feedback extras keep identifiers in keys and numbers. Tested: pytest posthog/test/mcp/test_pipeline.py -k 'intent_scan or captured_parameters' and posthog/test/mcp/test_feedback.py -k 'extras_redact or scalar_extras'. The new cases fail; the guard cases pass.
…tra keys and numbers - sanitize_free_text redacts only a head of 2x the intent limit (4096 chars), cut at a space no pass matches across: not after a digit or a card/SSN/phone separator. A head with no such space is one long word whose entropy verdict depends on all of it, so it is captured as [redacted]. 12MB of "1-1." went from ~41s to under 1ms. - build_captured_mcp_parameters strips the injected arguments, applies the 100-entry and depth-10 limits (normalize, strings whole), then sanitizes. A credential-named key is redacted without reading its value. This also redacts a top-level argument named e.g. "password", which main captured whole, and fixes a RecursionError on deeply nested arguments. - Feedback object extras: keys and numbers get the free-text pass, a number whose text changes becomes "[redacted]", and the extra is limited before sanitizing (a 5000-deep extra raised RecursionError). Top-level number and boolean extras are captured as sent. Tested: pytest posthog/test/mcp (711 passed, 11 skipped); ruff format --check, ruff check, mypy | mypy-baseline filter all clean. The straddle tests fail when the cut ignores digits and separators. Reviewer notes: two existing tests now use 4000-char words instead of 5000 so they stay below the scanned head; a no-space intent over 4096 chars is [redacted] rather than truncated. Card and SSN matching in redact_pii is unchanged. Large string leaves in $mcp_parameters are still scanned in full.
Contributor
posthog-python Compliance ReportDate: 2026-11-09T15:22:12.918936+00:00 ✅ All Tests Passed!121/121 tests passed Capture_V1 Tests✅ 95/95 tests passed View Details
Capture_Ai Tests✅ 5/5 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
Feature_Flags_Local_Evaluation Tests✅ 4/4 tests passed View Details
|
…ck extras - The head cut read the character before an ASCII space, but redact_pii first turns Unicode spaces into ASCII ones, so "+44<NBSP> 7911..." was cut mid-number and its digits leaked once earlier redactions shrank the head. The check now reads the normalized text. - Free text up to 8192 chars (4x the limit) is redacted whole, as on main, so CJK, newline- or tab-separated text and short prose before a long path keep their content. The head cut applies only above that; worst case is bounded. - Feedback extra keys skip the entropy detector, as resource names do: it read names like Get_Organization_Memberships as credentials, and two such keys collapsed into one "[redacted]" entry. Keys still get the token, PII and URL passes, bounded the same way. - An int past Python's 4300-digit str() limit inside an extra is "[redacted]" instead of raising, and _capture_extra_value keeps sanitizing inside its try, so one extra can no longer drop the whole $mcp_feedback event. Tested: pytest posthog/test/mcp (717 passed, 11 skipped); ruff and mypy baseline clean; the adversaries' repro scripts print the required output and their differential fuzzer finds 0 head-vs-whole mismatches on seeds 1, 3, 5, 7.
lucasheriques
marked this pull request as ready for review
November 9, 2026 15:17
PR overviewThe PR bounds intent and parameter capture work in MCP and updates feedback sanitization to redact extra keys and numbers. One issue remains open in feedback sanitization: token-shaped keys in declared object extras can pass through intact to the outbound feedback metadata property, even though the same strings are redacted as values. This creates a credential-disclosure path when credentials appear as keys; known credential-format checks should be retained while omitting only the entropy heuristic. No issues have been addressed yet. Open issues (1)
Fixed/addressed: 0 · PR risk: 5/10 Reviews
|
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| return _redact_bounded( | ||
| key, | ||
| lambda text: _sanitize_urls( | ||
| redact_pii(_POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, text)) |
There was a problem hiding this comment.
Medium: Preserve known credential checks for feedback keys
This path skips known credential-format checks as well as entropy detection: for a declared object extra, meta={"sk-proj-" + "Ab9z" * 8: 1} reaches the outbound $mcp_feedback_meta property with the entire token-shaped key intact, although the same string as a value is redacted. Retain the known-format credential checks for keys while omitting only the entropy heuristic that misclassifies ordinary names.
Contributor
SDK performance: posthog-pythonBenchmarks completed. No changes flagged for review. Flags require at least a 5% change in the median and all six pairs agreeing on direction. This is a noise filter, not proof of a performance change or a CI gate. View all results
Values are medians of paired measurements. Not flagged does not mean unchanged. Unavailable means a measurement could not be provided, not zero. Estimated allocation measurements retain their instrumentation uncertainty. RSS measures the whole process, not just SDK allocations; observed peaks are sampled checkpoints. Paired ranges in the diagnostics describe variability, not confidence intervals. Each shard uses its own runner; compare Before PR against With PR within a row, not across shards. Compared 102 metric rows; 102 rows shown in all results. Omitted or shortened data remain in |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
The credential-disclosure concern from @veria-ai remains unaddressed despite the reported no-findings review. Retain known credential-format checks for feedback keys, add regression coverage, and request review from Georgis Andonis or a security-aware teammate before re-requesting.
- Author wrote 69% of the modified lines and has 9 merged PRs in these paths (familiarity MODERATE).
- PostHog Review (Standard) reviewed the current head: no findings.
- @veria-ai on posthog/mcp/_sanitization.py: the feedback-key sanitizer still applies only the PostHog-specific token pattern, personal-data redaction, and URL sanitization, bypassing the broader credential checks used for string values. Non-PostHog credential-shaped keys can therefore remain in outbound feedback metadata. The discussion from @veria-ai[bot] repeats this still-unfixed concern.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 272L, 3F substantive, 469L/7F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (469L, 7F, two-areas, fix) |
| stamphog 2.7.0 | .stamphog/policy.yml @ 311d11b · reviewed head 311d11b |
dustinbyrne
reviewed
Nov 9, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
The bounded traversal and feedback key/number redaction fixes are verified. One intentional long-text retention change still needs reconciliation with the current MCP Payload privacy contract; details inline.
AI-assisted review.
| if len(value) <= _MAX_WHOLE_FREE_TEXT_LENGTH: | ||
| return redact(value) | ||
| head = _free_text_head(value) | ||
| return redact(head) + _TRUNCATION_SUFFIX if head else _REDACTED_VALUE |
Contributor
There was a problem hiding this comment.
[P2/design] Reconcile blanket long-text replacement with the canonical bounded-prefix contract
Disposition: REQUIRED acceptance decision.
Relationship: Intentional, introduced behavior change.
When benign intent exceeds 8,192 characters and has no qualifying ASCII-space boundary in its first 4,096 characters, this fallback replaces all its content with [redacted]. For example:
client.capture_tool_call(
"search_docs",
intent="请帮我查询" * 1800, # 9,000 characters
duration_ms=1,
)
client.flush()Base delivered a 2,048-character prefix; this head delivers the ten-character placeholder. The same sanitizer also serves feedback text and unknown-tool names.
The current canonical MCP specification requires keeping the start and prescribes a bounded limit-plus-1,024 / longest-common-prefix algorithm. A missing ASCII-space boundary does not itself make CJK prose a credential.
Both inspected independent peers retain text here: JS falls back to whole-string sanitation, while Go retains a bounded UTF-8 prefix. Their existing algorithm differences do not override the specification.
This is an atypically long input, and the replacement is privacy-conservative: no new disclosure, tool failure or broad event loss is demonstrated. Nevertheless, affected users lose all useful intent content. Backend intent clustering and session display consume the supplied string and cannot restore it.
Consider aligning this changed free-text path with the canonical bounded-prefix contract while retaining the existing straddle/privacy tests. Alternatively, explicitly resolve the policy divergence against that specification before accepting it. The PR and changeset disclose the tradeoff, but disclosure alone does not establish acceptance. This does not require expanding the PR to unrelated sanitizer debt.
Executed public regression:
def test_public_cjk_intent_retains_prefix():
from unittest.mock import patch
from posthog.mcp import PostHogMCP
text = "请帮我查询" * 1800
client = PostHogMCP("phc_test", enable_exception_autocapture=False)
try:
with patch.object(client, "capture") as captured:
client.capture_tool_call("search_docs", intent=text, duration_ms=1)
client.flush()
value = captured.call_args.kwargs["properties"]["$mcp_intent"]
assert value.startswith(text[:1000])
assert len(value) <= 2048
finally:
client.shutdown()Run through the isolated pytest runner with:
run_pytest.py <snapshot> --rootdir=<validation> \
<validation>/test_regressions.py \
-k public_cjk_intent_retains_prefix -q --tb=short --timeout=30
It passes on merge base and fails on reviewed head specifically because the value is [redacted]. A separate real-HTTP smoke confirms the same delivered-content change; its ordinary short-intent control is unchanged. No fix was applied or retested.
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
Three fixes to what
posthog.mcpcaptures, ported from PostHog/posthog-js#5607. Each one is shown below as before (main) and after (this PR). The outputs come from running one scratch script against the merge base (2c35f04) and this branch. Timings are single runs on a laptop.On
main:$mcp_intentis redacted in full before truncation keeps 2048 characters, so a long intent stalls capture for seconds.RecursionError, and a top-level argument namedpasswordis captured whole.$mcp_feedback_<name>object extras send emails and card numbers that sit in keys or in number values.This PR adds no personal-data redaction to
$mcp_parameters,$mcp_responseor error text. It does not change which identifiersredact_piimatches.What changes, by example
1. Free text is redacted only as far as truncation can keep it
sanitize_free_text(used for$mcp_intent, thesend_feedbacktext fields, and unknown tool names) still redacts text of up to 8192 characters (4x what truncation keeps) whole, as onmain. Past that it redacts a head of at most 4096 characters and appends.... The head ends at an ASCII space no redaction pattern matches across. A space after a digit, after one of./()+-, or after a Unicode space doesn't count, because a card, SSN or phone number can continue across it. So the head redacts exactly as it would inside the whole string. A longer text with no such space in its first 4096 characters is one long word, and the entropy check judges a word as a whole. Judging it would cost the full scan, so it is captured as[redacted].$mcp_intentmain"1-1." * 2048(8 KB, worst case still redacted whole)1-1.1-1.1-1.…(2048 chars)"1-1." * 250_000(1 MB)1-1.1-1.1-1.…(2048 chars)[redacted]"1-1." * 3_000_000(12 MB)1-1.1-1.1-1.…(2048 chars)[redacted]"lorem ipsum " * 87_382(1 MB)lorem ipsum lorem…(2048 chars)"lorem ipsum " * 1_000_000(12 MB)lorem ipsum lorem…(2048 chars)"s" * 5000or"请帮我查询" * 900"s" * 9000(one 9000-char word)sss…(2048 chars)[redacted]Refund for jane.doe@acme.com, card 4111 1111 1111 1111Refund for [redacted], card [redacted]Tests pin the cut rule. An intent whose start shrinks by 3000 characters when redacted has a card, an SSN, a phone number (one with an NBSP before a space too) or a URL with userinfo straddling character 4096. None of their digits or credentials reach the capture. Those tests fail if the cut ignores digits and separators.
2. Tool arguments are limited before they are sanitized
build_captured_mcp_parametersnow strips the injected arguments (context,conversation_id, and the SDK-ownedllm_model). Then it applies the 100-entry and depth-10 limits throughnormalize, with strings kept whole. Only after that does it sanitize. A credential-named key is written as[redacted]without its value being read. Read counts come from adictsubclass that counts entries read throughitems().main"...": "[MaxProperties ~]"RecursionError{"deep": {"child": … "[Object]"}}, cut at depth 10 of$mcp_parameters{"password": "hunter2", "q": "x", "context": "…"}{"password": "hunter2", "q": "x"}{"password": "[redacted]", "q": "x"}{"context": …, "conversation_id": …, a0 … a99}On
mainthe credential-key rule only ran inside nested objects, so a top-level argument namedpassword,tokenorapi_keyreached$mcp_parameterswhole.3. Feedback object extras redact identifiers in keys and numbers
A host can declare extra report fields for the
send_feedbacktool. An object extra is sent as JSON text, andmainsanitized only its string values. Now each string and number inside it gets the same free-text pass as$mcp_intent. A number whose text that pass changes becomes the string"[redacted]", so the JSON stays valid. Keys get the token, PII and URL passes but skip the entropy detector, as_sanitize_resource_namedoes for names. Otherwise names likeGet_Organization_Membershipsread as credentials, and two such keys collapse into one[redacted]entry. The extra is also limited to 100 entries and depth 10 before it is sanitized.Declared extras
meta(object) andscore(number), sent as:{ "meta": {"ada@example.com": "x", "card\n4111111111111111": 1, "n": 4111111111111111}, "score": 4111111111111111 }main$mcp_feedback_meta{"ada@example.com": "x", "card\n4111111111111111": 1, "n": 4111111111111111}{"[redacted]": "x", "card\n[redacted]": 1, "n": "[redacted]"}$mcp_feedback_score41111111111111114111111111111111(top-level numbers are sent as is, per spec)metanested 5000 deepRecursionError, whole$mcp_feedbackevent dropped{"child": … "[Object]"}, cut at depth 10meta={"Get_Organization_Memberships": 1, "ISO_8601_timestamp_UTC": 5}meta={"n": 10**5000}json.dumpsfails){"n": "[redacted]"}Keys are sanitized before the object is stringified. Sanitizing the JSON text instead would miss the first key: in
card\n4111…the escape'snis a word character, so the card has no word boundary.What does not change
Identifier matching in
redact_piistays as it is, including card and SSN overlap. Both runs printed the same output:redact_piiinputmainand this PR4111 1111 1111 1111 12/30[redacted] 12/30SSN 078-05-1120SSN [redacted]4111-1111-1111-1111 and 078-05-1120[redacted] and [redacted]id 4111111111111112(fails Luhn)id 4111111111111112ip 10.0.0.1, +44 20 7946 0958ip [redacted], [redacted]$mcp_parametersstill gets no personal-data redaction.{"email": "ada@example.com", "note": "call 415-555-0142"}is captured as is on both.main.$mcp_parametersand responses are still scanned in full, andsanitize_eventsanitizesparametersa second time, as onmain. A 12 MB string argument costs about 3 s across the two passes. This PR bounds entries and depth only. Bounding captured strings the way posthog-jsredactCapturedHeaddoes is a follow-up.Tradeoffs
[redacted]instead of its first 2048 characters. That covers long CJK text, minified JSON, and newline- or tab-only text.$mcp_parametersdoes, wheremaindropped the extra."[redacted]". Two keys that both redact to[redacted]collapse into the last one.Where to look
posthog/mcp/_sanitization.py_redact_bounded,_free_text_headand_CONTINUES_ACROSS_A_SPACE(the cut rule, next to the PII patterns it depends on)posthog/mcp/_sanitization.pybuild_captured_mcp_parameters,_pick_captured_fields,_kept_arguments,limit_entries: pick fields, strip, limit, then sanitizeposthog/mcp/_sanitization.py_sanitize_value_withwith its leaf and key functions;_sanitize_free_text_leaf(the number rule),_sanitize_free_text_keyposthog/mcp/_truncation.pynormalize's optionalredacts_key, and_Walkcarrying itposthog/mcp/feedback.py_capture_extra_value: scalars as is, objects limited and then walkedposthog/test/mcp/test_pipeline.py,test_feedback.py💚 How did you test it?
Each fix has a parameterized test that failed on
mainfirst. The test commit lands before the fix.pytest posthog/test/mcp: 717 passed, 11 skipped.ruff format --check .,ruff check ., andmypy --no-site-packages --config-file mypy.ini . | mypy-baseline filterare all clean, using the tool versions pinned inuv.lock._CONTINUES_ACROSS_A_SPACEis set to""(4 cases).📝 Checklist
If releasing new changes
sampo addto generate a changeset file (.sampo/changesets/mcp-capture-bounds.md)🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Opus 5.5) wrote this from a brief that came out of the posthog-js#5607 review. Those review rounds rejected every change to card and SSN overlap handling, so
redact_piimatching is left alone here.The Go SDK pre-truncates to 2x the limit at a fixed index. I chose a space-aligned cut instead. A fixed cut can split a URL's userinfo or a multi-group card, and once redaction shrinks the head by more than 2048 characters, the split piece lands in the capture. Four Sonnet reviewers made a simplify pass. The interrogate gate ran an Opus adversary and a Sonnet adversary, each in its own checkout. Both reproduced an NBSP phone-number leak at the cut and the loss of long space-free text. The Sonnet adversary also found key collisions from the entropy detector and a 4300-digit int that dropped the whole feedback event. All four are fixed in the last fix commit, each with a test, and the adversaries' scripts now print the required output. Their differential fuzzer (head redaction against whole-string redaction) finds 0 mismatches on four seeds. Both also flagged that large string arguments are still scanned in full; that is out of scope here and listed above.