Repository navigation
feat(mcp): redact personal data in tool arguments, results and errors by default - #5603
Closed
lucasheriques wants to merge 8 commits into
Closed
lucasheriques wants to merge 8 commits into
lucasheriques wants to merge 8 commits into
Conversation
… by default We told OpenAI's plugin directory review that the MCP SDKs redact personal data in tool payloads by default. sdk-specs#190 specifies it; this is the JS reference port. What: - @posthog/core: redactPayloadPii, the intent pass narrowed for payloads (public, unglued IPv4 only; phones only with +/00/011 or a parenthesized area code; a bare JSON-number card is written "[redacted]" quoted). sanitizeCapturedValue runs it, when redactPayloadPersonalData is set, inside the existing redactCapturedHead cut between the credential and URL passes, on object keys (collisions keep the key that sorts first by code point), and on Luhn-valid card numbers. - @posthog/mcp: new redactPayloadPersonalData option (default true) on instrument() and PostHogMCP, applied to $mcp_parameters, $mcp_response, exception values (cut at the 2,048 error limit) and resource read URIs. $mcp_input_keys drops names the token, URL or payload pass would change. $mcp_feedback_<name> JSON text is sanitized as free text, keys included. Tested: - pnpm --filter @posthog/mcp test:unit: 1049 passed (the new tests failed first) - pnpm --filter @posthog/mcp lint, check-types: clean - pnpm --filter @posthog/mcp build && test:integration: green (2 expected failures) - packages/core test:unit: 2356 passed; browser webmcp.test.ts: 46 passed Reviewer notes: - Per the spec's order (personal data before URLs), a URL with user:pass@host reads as an email and is redacted whole, read URIs included. Two existing expectations changed accordingly. - @posthog/core is listed in the changeset: mcp depends on it unbundled.
Contributor
Coherence📄 0 spec file errors · ⛔ 0 broken rules · 🚪 1 unchecked input · 🐛 0 bugs that came back What is Coherence?Coherence compares the code with its spec files ( This check read 44 parts of the code and 7 rules. Note These findings do not make this check fail. None of these findings are in files that this pull request changes. You do not need to fix them in this pull request. 🚪 Unchecked inputs (1)
What this means and how to fix itInput from outside this code comes in at these places. The spec does not say what checks that input. 10 more places were already like this when Coherence was added. This list does not show them. To close one of these, find the place by its name in the
To get the line to add, run |
Contributor
Contributor
|
Size Change: +66.1 kB (+0.25%) Total Size: 26.5 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
Node SDK acceptance v2Commit: 2b5415c Capture v0✅ 83 passed / ❌ 4 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. Capture v1✅ 148 passed / ❌ 4 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. |
Contributor
posthog-js Compliance ReportDate: 2026-11-06 22:29:47 UTC ✅ All Tests Passed!26/26 tests passed Capture Tests✅ 26/26 tests passed View Details
|
Contributor
SDK performance: posthog-nodeBenchmarks 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 |
…yload cost The adversarial gate on the payload personal data pass found data it leaked or corrupted. This fixes them. What and why: - JSON escapes: read as text, the letter of `\n`, `\t`, `\uXXXX` is a word character, so cards, SSNs, IPs and phones after it leaked and an email swallowed the `n` (invalid `\[` escape). A string that parses as a JSON object or array, within the length the passes already read (4x the string limit), is now sanitized as the structure it encodes (the same passes as structuredContent: leaves, keys, card-number integers) and written back with JSON.stringify; unchanged text comes back byte for byte. Free text (intent, feedback) gets the same path with its own passes, which covers feedback extras' keys. JSON nested too deep to walk falls back to the text pass instead of dropping the event. - Floats: the payload card candidate no longer takes `.` as a separator or starts after `-`/`.` or ends before `.digit`/an exponent; 95 in 1,000 random floats produced invalid JSON. The `00`/`011` phone prefix no longer matches after `.` (it ate `620.0119256973267`), found while verifying. - Card then SSN: a card is one run of 13-19 digits or a run grouped 4-4-4-4, 4-4-4-4-3, 4-6-5 or 4-6-4, so `4111111111111111 078-05-1120` no longer swallows `078`. Shared with the intent pass. - Cost: sanitizeEvent applies the entry and nesting limits first, as the spec orders; strings are still cut by truncateEvent after the passes. Gate probe, processMcpEvent with 100k string leaves: on 249-262 ms -> 0.4-0.5 ms, off 89-96 ms -> 0.2 ms (3 runs each, median of 5). - Changeset names the remaining default false positives. Tested: - pnpm --filter @posthog/mcp test:unit: 1131 passed; the 61 new cases failed first (the escapes x patterns grid, floats, card+SSN, feedback keys, deep nesting, the 100k-leaf cost bound) - pnpm --filter @posthog/core test:unit: 2356 passed; browser webmcp.test.ts: 46 passed - lint and check-types clean for mcp and core - pnpm --filter @posthog/mcp build && test:integration: green (same 2 expected failures per suite as before) Reviewer notes: - Redacted JSON text is re-serialized compactly; integers past 2^53 in it round as they do in structuredContent. A card written `4111111111111111e0` is redacted by value in parsed JSON. - JSON text longer than the parse cut keeps the text pass, so an escape there can still hide an identifier.
…ializing JSON What: the text pass (intent, payload, and the PostHog token pass) now matches every pattern on a same-length copy of the string in which each JSON/JS escape (\n \t \r \b \f \/ \\ \" \uXXXX) reads as NUL and Unicode spaces read as ASCII spaces, and splices each redaction into the original at the same offsets. sanitizeJsonText and the parse/re-serialize path are gone. Cards: the CARD_GROUPINGS requirement is gone; each start takes the shortest Luhn-valid 13-19 digit run of whole groups, so a trailing SSN is left for its own pattern. The payload card candidate accepts a card glued to a letter by - or . and still rejects negatives and fractions. A card that is a bare JSON number outside any string (quote parity on the masked text) is written "[redacted]" in both passes. Object walks (core sanitizeValueWith, mcp normalize) build results with Object.fromEntries, so a parsed __proto__ member survives. buildCapturedMcpParameters applies the entry and nesting limits (limitEntries) before sanitizing. Why: two rounds of parse-and-reserialize kept failing the re-gate (duplicate keys, integers past 2^53, __proto__, NDJSON, embedded JSON, BOM, lost formatting). Masking needs none of those special cases: no match contains an escape, so JSON stays valid and clean text stays byte for byte. The mask char is not a separator, so 4111\n1111\n... is not joined into a spaced card. The PostHog token pass reads the masked text too, because "x\nphc_..." leaked a token after the parse path was removed (and leaks on main). Tested: - pnpm --filter @posthog/core test:unit: 2356 passed, 1 skipped - pnpm --filter @posthog/mcp test:unit: 1702 passed (325 of them fail on 7c0527a) - browser vitest src/__tests__/webmcp.test.ts: 46 passed - lint and check-types for core and mcp, pnpm format: clean - pnpm --filter @posthog/mcp build && test:integration: 37/39, the 2 declared expected failures - re-gate probes probe1-3 and perf, plus zz-regate-* tests: no leaks, intent card table matches main row for row; 100k-leaf arguments build in 8.9 ms vs 0.1 ms for 100, the rest being V8 key enumeration (Object.keys on 100k keys alone is 8.2 ms) Reviewer notes: the context argument is now dropped after the entry limit, so it counts toward the 100 kept arguments. A payload card after a non-letter - or . (": -4111...") is read as a negative number and kept.
… numbers What: - Payload cards: an unseparated 13-19 digit run, or groups printed as cards are (4-4-4-4, 4-4-4-4-3, 4-6-4, 4-6-5), Luhn-valid. The intent keeps the shortest Luhn run of any whole groups. Both are found by a loop instead of a backtracking pattern. The payload 00/011 phone prefix no longer counts after a dash (874-426-0063). - Escapes: \uXXXX reads as the character it encodes (a Unicode space as a space, a non-ASCII character as %, a control, quote or backslash as NUL). The same applies to literal non-ASCII text. A match maps back over whole escapes, so the output stays valid JSON. - JSON numbers in value position are kept or replaced whole by "[redacted]", whatever the indent. Payload: a non-negative token whose value is an integer card (4111...0, 4111...e0). Intent: that, or any token the card pass would touch. A text that is only a card becomes [redacted]. - The head scanned for a long string is cut at the window edge unless a URL crosses it; then it is cut where the URL starts. - buildCapturedMcpParameters drops `context` before the argument limit, copying at most 101 arguments. Why: the gate found payload false positives (date pairs 20%, phone pairs 42%, ZIP+4 26%, id lists 10%), escape-hidden emails and spaced cards, a fixed 64-char lookback that broke indented JSON, partial number tokens (-[redacted], [redacted].0), whole-value scans of whitespace-free input (745ms at 8MB), a stack overflow at 12MB, and a lost 100th argument. Tested: - pnpm --filter @posthog/mcp test:unit: 1775 passed (64 new cases fail on 7c039ad) - pnpm --filter @posthog/core test:unit: 2356 passed, 1 skipped - browser vitest src/__tests__/webmcp.test.ts: 46 passed - lint, check-types (core, mcp), pnpm format: clean - pnpm --filter @posthog/mcp build && test:integration: exit 0, nest 37/39 with the 2 declared expected failures - gate3 probes: p4_fuzz 0 regressions in 200k; intent output identical to 7c039ad on 300k escape-free strings; p11_fp payload 0.0% on all six generators; p12_indent valid at 2/4/10; payload `1-1.` 25-42ms from 1MB to 20MB Reviewer notes: a URL that starts the window and runs past it is still judged on the whole value, as on main, so `a:` plus 12MB costs seconds. Direct redactPii/redactPayloadPii calls on 12MB no longer throw but take seconds. Negative payload cards stay kept.
…ding it whole What: when a URL crosses the 131072-character scan window, sanitizeString and sanitizeFreeText now replace it whole with `[redacted]` and stop at the window edge. Before, the cut moved back to where that URL started, and a value opening with one fell back to processing the whole string. Object keys keep the plain window (they get no URL pass). Why: cost must follow the property limits, not the payload. `a:` + 12MB took 9.8-16s through the intent pass and 3-3.6s through the payload pass. Cutting the URL at the edge instead would leak, since `https:secretuser...` without its `@` reads as having no userinfo. The trailing URL is found after the personal-data pass, so an email written into a URL's host (`john@ex.com:...`) is still redacted. Behavior change from main (#5186), noted in the changeset: a URL longer than the rest of the window is now `[redacted]` and nothing after it is captured. A `data:` image inlined in a long HTML page is the visible case. Tested: - New tests failed on 707642d: 8MB/1MB cost ratio 5.3-9.4 (limit 3) for a value opening with a URL whose userinfo and token sit past the window, on the intent, payload-on and payload-off passes; window outputs for that value and for the four pinned URL cases (credential URL at the edge, credentials past the window, field count past the window, pre-signed URLs) plus the inlined-image test. - pnpm --filter @posthog/core test:unit: 2356 passed, 1 skipped - pnpm --filter @posthog/mcp test:unit: 1779 passed - browser webmcp.test.ts: 46 passed - pnpm format, lint (core, mcp, oxlint), check-types (core, mcp): clean - pnpm --filter @posthog/mcp build && test:integration: exit 0 (2 declared expected failures) - a: + 12MB now 26ms intent, 23ms payload on, 2ms payload off; p6/p8/p9 probes flat across 1-20MB. Reviewer note: the shrink fallback (head under 2x the limit after redaction, cut on whitespace) still reads the whole string. 8MB of 1024-char pre-signed URL lines costs 2.2s vs 0.3s for 1MB. Not addressed here.
…s the head What: when redacting the 131072-character window leaves a head under twice the string limit, sanitizeString, sanitizeFreeText and object keys now redact a window twice as long, up to four times the first (524288 characters), instead of the whole string. At that cap a head still under twice the limit keeps all but its last maxStringLength characters, so a match cut at the window edge is never captured. Why: cost must follow the property limits, not the payload. 1024-character pre-signed URL lines shrink about tenfold, so 8MB fell back to whole-string redaction: 2.2s vs 0.3s for 1MB on the intent pass. Keeping the cap head whole would leak: tokens that shrink 27x leave a cut `john.doe@exa` inside the kept part. Tested: - New tests failed on ed74ee2: 8MB/1MB cost ratio 7.0-7.3 (limit 3) for pre-signed URL lines on the payload, payload-off and intent passes; the window output for pre-signed URL lines past the widest window; and the split email at the widest window edge on the payload and intent passes. - pnpm --filter @posthog/core test:unit: 2356 passed, 1 skipped - pnpm --filter @posthog/mcp test:unit: 1785 passed - browser webmcp.test.ts: 46 passed - pnpm format, lint (core, mcp, oxlint), check-types (core, mcp): clean - pnpm --filter @posthog/mcp build && test:integration: exit 0 (2 declared expected failures) - p14 aligned: 1MB intent 250ms, 8MB 251ms (was 0.3s vs 2.2s); p6/p8/p9/p13 flat. Reviewer note: at the cap, a head that redaction shrank more than 16x keeps nothing ('...'). Before, it read the whole string and kept up to the limit.
…ce, cut keys to 256 What: - The JSON-number card rule writes "[redacted]" with quotes only when the scanned text parses as JSON (leading BOM allowed), checked lazily on the first card-like number. Elsewhere the card pass redacts it unquoted, as main does. - buildCapturedMcpParameters applies only the entry and nesting limits; sanitizeEvent sanitizes arguments once, with the payload option. - Object keys are cut to the 256-character key limit after redaction (CapturedValueOptions.maxKeyLength), with the option on or off. Why: the final gate found three leaks. The inserted quote ended the URL match, so `?ids=[4111...]&access_token=x` and userinfo after a card shipped (intent regression vs main, and payload with the option on). Arguments were cut at the scan window by a first pass with personal data off, so the second pass saw a split identifier's prefix. Keys were redacted on a window head but never truncated, so a split identifier in a key shipped. Tested: - 21 new cases fail on e7cb535: URL credentials after a card in brackets, commas, colon, a `[INFO]` log line, and userinfo, intent and payload; the unquoted card in prose; five identifiers split at the window in arguments through processMcpEvent; keys crossing the window in response and parameters, option on and off. The gate's zz-final-gate.test.ts: 8/8 pass. - pnpm --filter @posthog/mcp test:unit: 1813 passed - pnpm --filter @posthog/core test:unit: 2356 passed, 1 skipped - browser vitest src/__tests__/webmcp.test.ts: 46 passed - lint (core, mcp, oxlint), check-types (core, mcp), pnpm format: clean - pnpm --filter @posthog/mcp build && test:integration: exit 0, 37/39 with the 2 declared expected failures - gfinal probes on this build: f1/f2 clean and match main; f8_window 0 leaks in 36360; f4c_json invalid 0, cleanChanged 0; f5_parity, f4c leaks and f6_fp identical to e7cb535; f3_cost flat from 1MB to 20MB. Reviewer notes: NDJSON is not JSON text, so a bare card number in it is now unquoted. A URL-shaped text with cards now reaches the URL parser whole (url-ish probe 9ms to 25ms at the window, flat across sizes).
# Conflicts: # packages/mcp/src/extensions/sanitization.ts
![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 2b5415c: 1 Must fix, 5 Should fix. This pull request is large, so the review ran in 4 parts with less depth than usual.
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.
| if (value.length <= windowLength) { | ||
| return redactUrls(redact(value)) | ||
| } | ||
| const head = redact(value.slice(0, windowLength)) |
Contributor
There was a problem hiding this comment.
P1 · Preserve card-number redaction when windowing JSON text
A valid JSON result exceeding 131,072 characters with an early "card":4111111111111111.0 leaks the complete card number. redactCapturedText cuts the document before jsonNumberRule validates it, so JSON detection fails and the fallback card pass ignores the decimal form. Keep numeric-card recognition effective on incomplete scan windows.
Comment on lines
+772
to
+778
| for (const grouping of CARD_GROUPINGS) { | ||
| const end = first + grouping.reduce((sum, length) => sum + length + 1, -1) | ||
| if ( | ||
| grouping.every((length, index) => lengths[index] === length) && | ||
| passesLuhn(run.slice(first, end).replace(/\D/g, '')) | ||
| ) { | ||
| return end |
Contributor
There was a problem hiding this comment.
P2 · Redact the complete card when its prefix also passes Luhn
For 4111 1111 1111 1111 003, both the full 19-digit card and its 16-digit prefix pass Luhn. PAYLOAD_CARDS accepts the shorter grouping first and returns [redacted] 003, exposing part of the card; the revised intent matcher does the same, regressing its previous coverage. Resolve overlapping valid groupings before returning so the complete card is redacted.
| function payloadPasses(options: CapturedValueOptions): ValuePasses { | ||
| return { | ||
| string: (value) => sanitizeString(value, options), | ||
| key: (key) => cutKey(redactCapturedHead(key, redactPayloadPii, options), options), |
Contributor
There was a problem hiding this comment.
P2 · Size object-key scan windows from the key limit
With 100 long object keys, redactCapturedHead uses maxStringLength (32,768) to scan 131,072 characters per key, although cutKey retains only 256. A read-only reproduction with nonmatching email-shaped keys blocks synchronously for 6.2 seconds, delaying other requests. Bound the key scan window using maxKeyLength instead.
| event: T, | ||
| { redactPayloadPersonalData }: PayloadSanitizationOptions = { redactPayloadPersonalData: true } | ||
| ): T { | ||
| const result = limitPayloadEntries(event) |
Contributor
There was a problem hiding this comment.
P2 · Discard redacted subtrees before normalizing payloads
When a result contains nested objects under secret or image metadata, limitPayloadEntries clones subtrees that sanitization previously discarded immediately. A four-level, 50-way shared array under structuredContent.secret allocates roughly 98 MB and blocks the event loop for 92 ms. Apply sensitive-key and non-text block redaction before the limiting walk.
| // `-`, where it is the last group of a dashed number (`874-426-0063`). | ||
| const PAYLOAD_PHONE_INTL_PATTERN = /(^|[^\w.-])((?:\+|00|011)[ .-]?[1-9]\d{0,2}(?:[ ./()-]{0,2}\d){7,13})(?![\w])/g | ||
| // A number in JSON value position: after `:`, `,` or `[`, and before `,`, `}` or `]`. | ||
| const JSON_NUMBER_PATTERN = /([:,[]\s*)(-?\d+(?:\.\d+)?(?:[eE][+-]?\d+)?)(?=\s*[,}\]])/g |
Contributor
There was a problem hiding this comment.
P2 · Recognize card numbers at the JSON document root
A tool text block containing only 4111111111111111.0 or 4111111111111111e0 passes through payload sanitization unchanged. Both are valid JSON numbers equal to a Luhn-valid card, but JSON_NUMBER_PATTERN requires surrounding container delimiters and the fallback rejects decimals and exponents. Include document boundaries in numeric-token matching.
| ]) | ||
| } | ||
|
|
||
| export function sanitizeCapturedValue(value: unknown, options: CapturedValueOptions): unknown { |
Contributor
There was a problem hiding this comment.
P2 · Keep existing sanitization options source-compatible
Existing TypeScript calls to sanitizeCapturedValue(value, { maxStringLength, truncationSuffix }) stop compiling because CapturedValueOptions requires maxKeyLength. The function is publicly re-exported from @posthog/core, so the planned minor release breaks its existing options contract. Make the new field optional and supply a default.
![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.
All six @PostHog concerns remain supported by the current code, and its current-head review therefore does not supply concern-free assurance for these sensitive changes. Address those inline findings and request a fresh review from Georgis Andonis, Paul D'Ambra, or another teammate before re-requesting.
- Author wrote 41% of the modified lines and has 22 merged PRs in these paths (familiarity MODERATE).
- PostHog Review (Standard) reviewed the current head: 6 findings, posted on the PR.
- posthog[bot] reviewed the current head.
- @PostHog, packages/core/src/privacy.ts: redactCapturedText slices long documents before jsonNumberRule calls JSON.parse. An incomplete JSON window fails validation, while PAYLOAD_CARDS excludes decimal suffixes, leaving an early decimal-form card number exposed.
- @PostHog, packages/core/src/privacy.ts: PAYLOAD_CARDS tries the 16-digit grouping before the overlapping 19-digit grouping and returns immediately on a valid checksum; INTENT_CARDS likewise returns the first valid prefix. A valid longer card can retain its final group.
- @PostHog, packages/core/src/privacy.ts: payloadPasses scans keys through redactCapturedHead using maxStringLength, not maxKeyLength. With MCP defaults this scans 131,072 characters per initial key window before retaining only 256, so the reported excessive synchronous work remains possible.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 853L, 10F substantive, 1791L/19F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (1791L, 19F, two-areas, feat) |
| stamphog 2.7.0 | .stamphog/policy.yml @ 2b5415c · reviewed head 2b5415c |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| return BINARY_REDACTED_VALUE | ||
| } | ||
| const redactPersonalData = options.redactPayloadPersonalData ? redactPayloadPii : (head: string) => head | ||
| return redactCapturedText(value, (head) => redactPersonalData(redactCredentials(head)), options) |
There was a problem hiding this comment.
Medium: PII replacement can bypass URL credential scrubbing
With the default option, the valid custom URI dead::beef?access_token=secret becomes [redacted]?access_token=secret: the IPv6 pass removes its scheme before the URL pass can recognize it, leaving a credential that the previous payload sanitizer scrubbed in outgoing parameters, responses, resource names, and exception messages. Preserve the original URL spans for credential sanitization before identifier replacements can destroy their boundaries; simply swapping passes also needs care to avoid hiding PII through URL encoding.
| function payloadPasses(options: CapturedValueOptions): ValuePasses { | ||
| return { | ||
| string: (value) => sanitizeString(value, options), | ||
| key: (key) => cutKey(redactCapturedHead(key, redactPayloadPii, options), options), |
There was a problem hiding this comment.
Medium: Object keys skip credential sanitization
The new key pass runs only redactPayloadPii, so a URL-keyed result such as { "https://service.test/file?access_token=secret": "ok" } retains the credential in $mcp_response or $mcp_parameters, although the same URL as a string value is scrubbed. Apply credential and URL sanitization to keys before truncation, including in the option-off branch at line 1090, so disabling personal-data redaction does not bypass credential protection.
PR overviewThe PR adds default personal-data redaction to MCP tool arguments, results, and errors, including processing of object keys. Two open issues leave credentials exposed in some sanitized payloads: personal-data replacement can disrupt URL recognition, and object keys do not receive credential sanitization. These gaps can allow access tokens to remain in outgoing parameters, responses, resource names, or exception messages when the affected URL forms are present. No issues have been addressed yet. Open issues (2)
Fixed/addressed: 0 · PR risk: 5/10 |
lucasheriques
marked this pull request as draft
November 6, 2026 22:33
Contributor
Author
|
Closing. We're not adding a pattern-based personal data pass to tool payloads in the SDKs. sdk-specs#194 reverts the spec requirement. Tool arguments and results are what the server's own tools already hold, so the SDK relays them as captured, after the credential passes. Hosts that must not send them use parameter or response capture, or Some fixes here apply to main independently of the payload pass, and may come back in a small PR:
|
6 of 24 tasks
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.
Problem
@posthog/mcpshould not capture personal data in tool arguments, results and errors by default. It redacted it only in intent and feedback text, so a customer-lookup tool sent the customer's email in$mcp_parametersand$mcp_response. sdk-specs#190 (merged) specifies the behavior. This is the reference port, and Python, Go, Ruby and Rust follow it.Changes
redactPayloadPiiin@posthog/coreprivacy.tsis narrower than the intent pass:+,00,011or a parenthesized area coderedactPayloadPersonalData: false:$mcp_parametersand$mcp_response$mcp_error_message$exceptionvalues\nor__proto__need no special cases."[redacted]", only in text that parses as JSON. In other text a card is redacted unquoted, so a URL around it stays one URL for the credential pass.$mcp_input_keysdrops a name the pass would change, and feedback JSON extras are sanitized as free text.@posthog/mcpand@posthog/coreminor. It lists what is redacted, the opt-out, and the known false positives:ddd-dd-ddddreferences:or@00-prefixed ids+-signed numbers and coordinatesFor reviewers:
xxx:as a URL scheme.How I tested
pnpm --filter @posthog/mcp test:unit: 1837 passed.@posthog/core: 2355 passed.webmcp.test.ts: 46 passed.test:integrationpasses, apart from the 2 expected failures.