Repository navigation
fix(mcp): bound intent and parameter capture work, redact feedback extra keys and numbers - #1217
Open
lucasheriques wants to merge 6 commits into
Open
lucasheriques wants to merge 6 commits into
lucasheriques wants to merge 6 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-12T14:58:04.037509+00:00 ✅ All Tests Passed!270/270 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_V4 Tests✅ 149/149 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 sanitization and redacts additional feedback keys and numbers. One issue has been addressed, but an open scan-boundary gap can leave sensitive text unredacted in captured intent. Long signed JWTs with credential-bearing claims and URL userinfo can escape recognition when their identifying delimiters fall beyond both scan limits, allowing secrets to remain in Open issues (1)
Fixed/addressed: 1 · PR risk: 5/10 Reviews |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
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.
…scan; keep known credential formats in feedback keys Long free text (intent, feedback fields, feedback keys) now follows the sdk-specs bounded scan: redact the text cut at limit+1024 and at the limit, keep their common prefix. Text with no ASCII-space boundary (CJK, one long word) keeps its start instead of becoming [redacted]. Feedback keys keep the known-format credential checks (sk-proj-..., PEM) and still skip entropy. Tested: pytest posthog/test/mcp + test_exception_capture (756 passed, 11 skipped); straddle tests fail under either one-scan mutation; ruff, mypy baseline clean.
![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 d526a20: 1 Must 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
+433
to
+437
| kept = os.path.commonprefix( | ||
| [ | ||
| redact(value[: _MAX_USER_INTENT_LENGTH + _SCAN_MARGIN]), | ||
| redact(value[:_MAX_USER_INTENT_LENGTH]), | ||
| ] |
Contributor
There was a problem hiding this comment.
P1 · Redact URL credentials that extend past both scan cuts
With intent "https://admin:" + "p" * 4000 + "@localhost/x", both slices end before @, so neither URL pass recognizes userinfo. _redact_bounded retains their raw common prefix and captures roughly 2 KB of the password in $mcp_intent; the previous full pass removes it. Preserve the privacy contract (AGENTS.md:21) by redacting incomplete URLs at the scan boundary.
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 current-head review raises a security concern that the code still exhibits; the discussion bot's all-clear claim does not resolve it. Fix @PostHog's inline finding in posthog/mcp/_sanitization.py, add a regression test for credentials extending beyond both scan cuts, and request fresh review from a teammate or agent; Georgis Andonis or Aleksander Błaszkiewicz are suggested reviewers.
- Author wrote 61% of the modified lines and has 9 merged PRs in these paths (familiarity MODERATE).
- PostHog Review (Standard) reviewed the current head: 1 finding, posted on the PR.
- posthog[bot] reviewed the current head.
- @PostHog, posthog/mcp/_sanitization.py: The URL-credential exposure remains unfixed. _redact_bounded sanitizes only the first 2048 and 3072 characters and retains their common prefix. For an HTTPS URL with username admin and a 4000-character repeated-letter password, both cuts precede the userinfo delimiter; the URL sanitizer leaves these incomplete authorities unchanged, retaining password text in captured intent. The full URL would have its userinfo removed.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 309L, 4F substantive, 500L/8F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (500L, 8F, two-areas, fix) |
| stamphog 2.8.0 | .stamphog/policy.yml @ d526a20 · reviewed head d526a20 |
| return redact(value) | ||
| kept = os.path.commonprefix( | ||
| [ | ||
| redact(value[: _MAX_USER_INTENT_LENGTH + _SCAN_MARGIN]), |
There was a problem hiding this comment.
Medium: Incomplete prefix scans can bypass redaction
Both scans can agree on raw sensitive text: a verified 1,631-character signed JWT starting at offset 1,550 has its signature separator at 3,137, so neither prefix recognizes it, and the final $mcp_intent contains a decodable db_password claim that the pre-change sanitizer redacted. Fail closed on unfinished credential-like spans at the scan boundary rather than treating their common prefix as safe; URL userinfo whose @ falls beyond both cuts is affected too.
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 keys, and unknown tool names) follows the sdk-specs bounded scan. Text up to 3072 characters (the 2048 limit plus a 1024 margin) is redacted whole. Longer text is redacted twice, once cut at 3072 and once cut at 2048. Capture keeps the common prefix of the two results and appends.... A match split by the 2048 cut differs between the two results. A match split by the 3072 cut lies past the end of the second result. So neither reaches the capture, and the cost no longer grows with the input.|
$mcp_intent|main| This PR ||
"1-1." * 2048(8 KB) | not measured separately | 14 ms,1-1.1-1.1-1.…(2048 chars) ||
"1-1." * 250_000(1 MB) | 3.6 s,1-1.1-1.1-1.…(2048 chars) | 14 ms, same output ||
"1-1." * 3_000_000(12 MB) | 41.2 s,1-1.1-1.1-1.…(2048 chars) | 14 ms, same output ||
"1-1." * 3_000_000(12 MB) | 41.2 s,1-1.1-1.1-1.…(2048 chars) | under 0.1 ms,[redacted]||
"lorem ipsum " * 87_382(1 MB) | 218 ms,lorem ipsum lorem…(2048 chars) | 0.7 ms, same output ||
"s" * 5000,"s" * 9000or"请帮我查询" * 1800| first 2048 chars | same ||
"s" * 5000or"请帮我查询" * 900| first 2048 chars | same ||
"s" * 9000(one 9000-char word) |sss…(2048 chars) |[redacted]||
Refund for jane.doe@acme.com, card 4111 1111 1111 1111|Refund for [redacted], card [redacted]| same |Tests pin the bounded scan. A card, an SSN, a phone number (one with an NBSP before a space too) or a URL with userinfo straddles the 2048 cut or the 3072 cut, after a start that shrinks by 1500 characters when redacted. None of their digits or credentials reach the capture. Dropping either of the two redactions fails three of those cases.
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 pass, the known-format credential check (sk-proj-…, PEM and the other vendor formats), and the PII and URL passes. They skip only the entropy detector. 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. Longer intents keep their start, as onmain.$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
$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_boundedand_SCAN_MARGIN(the bounded scan), and_sanitize_free_text_keywith_is_known_secret(keys)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 posthog/test/test_exception_capture.py: 756 passed, 11 skipped, after mergingmain.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._redact_boundedis dropped (3 cases each).📝 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 first version cut at an ASCII space that no pattern matches across. Review pointed to the sdk-specs bounded scan (cut at limit + 1024 and at the limit, keep the common prefix), and this PR now follows it. The space rule turned long space-free text into
[redacted], and it depended on a hand-kept list of characters a number continues across, which the NBSP leak showed is easy to get wrong. 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.