fix(mcp): run free-text PII redaction before URL rewriting in feedback capture

Main's URL-credential sanitizer (#928) percent-encodes the @ the email
pattern anchors on, so sanitize-then-redact let PII inside URLs through
the feedback fields. Free text (summary, details, friction_points,
suggested_improvement, tool_name) now uses the $mcp_intent pass
(credentials -> PII -> URLs), and extras use a new sanitize_free_text_value
walker that applies it per string leaf while keeping key-based redaction.
sanitize_intent is renamed sanitize_free_text: the pass is no longer
intent-specific. Ports posthog-js#4870 commits 9d3b3933, f4a69ea8,
270c4630.

Generated-By: PostHog Desktop
Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0

Author: gesh

posthog/mcp/_sanitization.py

@@ -443,9 +443,9 @@ def _redact_credentials(value: str) -> str:
     return _redact_secret_tokens(_POSTHOG_TOKEN_PATTERN.sub(_REDACTED_VALUE, value))
 
 
-def sanitize_intent(value: Any) -> Any:
-    """Sanitize the agent-narrated intent: the binary gate, the credential passes,
-    structured PII, then URLs.
+def sanitize_free_text(value: Any) -> Any:
+    """Sanitize agent-narrated free text (the intent, the send_feedback fields):
+    the binary gate, the credential passes, structured PII, then URLs.
 
     Every step sits where it does for a reason. The binary gate runs first
     because splicing a redaction into a base64 blob stops it looking like base64,
@@ -586,12 +586,23 @@ def redact_pii(value: Any) -> Any:
 
 
 def sanitize_captured_value(value: Any) -> Any:
+    return _sanitize_value_with(value, _sanitize_string)
+
+
+def sanitize_free_text_value(value: Any) -> Any:
+    """:func:`sanitize_captured_value` with the free-text string pass
+    (:func:`sanitize_free_text`) on every string leaf, so nested agent-narrated
+    values get structured-PII redaction in the load-bearing order too."""
+    return _sanitize_value_with(value, sanitize_free_text)
+
+
+def _sanitize_value_with(value: Any, sanitize_string_fn: Any) -> Any:
     if value is None:
         return value
     if isinstance(value, str):
-        return _sanitize_string(value)
+        return sanitize_string_fn(value)
     if isinstance(value, list):
-        return [sanitize_captured_value(item) for item in value]
+        return [_sanitize_value_with(item, sanitize_string_fn) for item in value]
     # bool is an int subclass; both pass through unchanged.
     if not isinstance(value, dict):
         return value
@@ -601,7 +612,7 @@ def sanitize_captured_value(value: Any) -> Any:
         result[key] = (
             _REDACTED_VALUE
             if _should_redact_key(str(key))
-            else sanitize_captured_value(nested)
+            else _sanitize_value_with(nested, sanitize_string_fn)
         )
     return result
 
@@ -622,12 +633,13 @@ def sanitize_event(event: Dict[str, Any]) -> Dict[str, Any]:
 
     # The intent comes straight from an agent-narrated `context` string, so it
     # can contain a secret the LLM read aloud or personal data it narrated about
-    # the user. `sanitize_intent` redacts it like any other captured value and
+    # the user. `sanitize_free_text` redacts it like any other captured value and
     # additionally strips structured PII (emails, phone numbers, IPs, cards,
-    # SSNs). PII redaction is scoped to the intent only — structured tool
-    # parameters and responses often hold the same shapes as legitimate data.
+    # SSNs). PII redaction is scoped to agent-narrated free text only —
+    # structured tool parameters and responses often hold the same shapes as
+    # legitimate data.
     if result.get("user_intent") is not None:
-        result["user_intent"] = sanitize_intent(result["user_intent"])
+        result["user_intent"] = sanitize_free_text(result["user_intent"])
 
     if result.get("llm_model") is not None:
         result["llm_model"] = sanitize_captured_value(result["llm_model"])

posthog/mcp/feedback.py

@@ -10,7 +10,7 @@
 from typing import Any, Dict, Optional, Union
 
 from ._internal import _maybe_await
-from ._sanitization import redact_pii, sanitize_captured_value
+from ._sanitization import sanitize_free_text, sanitize_free_text_value
 from .constants import PostHogMCPAnalyticsProperty
 from .logger import log
 from .types import CollectFeedbackOptions, FeedbackReport, JsonRecord
@@ -257,25 +257,26 @@ def _truncate_feedback_text(value: str, max_length: int) -> str:
 
 def _capture_free_text(value: str) -> str:
     """Agent-narrated free text can contain a secret the LLM read aloud or personal
-    data it narrated, so it gets the ``$mcp_intent`` treatment: sanitize, strip
-    structured PII, then bound the length. The event pipeline does not process
+    data it narrated, so it gets exactly the ``$mcp_intent`` pass
+    (``sanitize_free_text``: credentials -> structured PII -> URLs — the order is
+    load-bearing, the URL rewrite would percent-encode the ``@`` the email pattern
+    anchors on), then a length bound. The event pipeline does not process
     ``event["properties"]``, so this happens here."""
-    return _truncate_feedback_text(
-        redact_pii(sanitize_captured_value(value)), _MAX_FEEDBACK_TEXT_LENGTH
-    )
+    return _truncate_feedback_text(sanitize_free_text(value), _MAX_FEEDBACK_TEXT_LENGTH)
 
 
 def _capture_extra_value(value: Any) -> Any:
-    """A declared extra is agent-supplied like the core free-text fields, so it
-    gets the same treatment. Non-scalars are JSON-stringified first so the
-    redaction sees the full text; unserializable values are dropped."""
-    sanitized = sanitize_captured_value(value)
+    """A declared extra is agent-supplied like the core free-text fields, so its
+    string leaves get the same free-text pass (with the key-based redaction
+    ``sanitize_free_text_value`` keeps for nested objects), then non-scalars are
+    JSON-stringified and everything is bounded. Unserializable values are dropped."""
+    sanitized = sanitize_free_text_value(value)
     if isinstance(sanitized, str):
-        return _capture_free_text(sanitized)
+        return _truncate_feedback_text(sanitized, _MAX_FEEDBACK_TEXT_LENGTH)
     if sanitized is None or isinstance(sanitized, (bool, int, float)):
         return sanitized
     try:
-        return _capture_free_text(json.dumps(sanitized))
+        return _truncate_feedback_text(json.dumps(sanitized), _MAX_FEEDBACK_TEXT_LENGTH)
     except Exception:  # noqa: BLE001 - capture must never raise into the tool path
         return None
 
@@ -306,10 +307,10 @@ def build_feedback_event_properties(report: FeedbackReport) -> JsonRecord:
         )
     if report.tool_name:
         # Nominally an identifier, but the schema can't stop an agent from
-        # writing prose into it — so it gets the same PII redaction as the
-        # other free text.
+        # writing prose into it — so it gets the same free-text pass as the
+        # other fields.
         properties[PostHogMCPAnalyticsProperty.FEEDBACK_TOOL] = _truncate_feedback_text(
-            redact_pii(sanitize_captured_value(report.tool_name)),
+            sanitize_free_text(report.tool_name),
             _MAX_FEEDBACK_TOOL_NAME_LENGTH,
         )
     if report.task_completed is not None:

posthog/test/mcp/test_feedback.py

@@ -312,20 +312,55 @@ def test_tool_name_gets_pii_redaction():
     assert props["$mcp_feedback_tool"] == "ask [redacted]"
 
 
-def test_pii_inside_urls_is_redacted():
-    # Guards the ordering of sanitize vs redact: a sanitizer that re-serializes
-    # URLs (percent-encoding "@") before redaction would hide the email from the
-    # redaction pattern (the TS SDK's veria finding on this feature).
+def test_pii_inside_urls_is_redacted_on_every_surface():
+    # Guards the ordering of the free-text pass: running the URL rewrite before
+    # PII redaction percent-encodes the "@" the email pattern anchors on, letting
+    # the email through (the TS SDK's veria finding on this feature).
+    url = "https://example.com/?email=jane@example.com"
+    options = CollectFeedbackOptions(extra_properties={"area": {"type": "string"}})
     report = parse_feedback_report(
         {
             "feedback_type": "issue",
-            "summary": "Login fails at https://example.com/?email=jane@example.com",
-        }
+            "summary": f"Login fails at {url}",
+            "details": f"see {url} too",
+            "friction_points": f"url {url} slow",
+            "suggested_improvement": f"fix {url}",
+            "tool_name": url,
+            "area": url,
+        },
+        options,
+    )
+    props = build_feedback_event_properties(report)
+    for key in (
+        "$mcp_feedback_summary",
+        "$mcp_feedback_details",
+        "$mcp_feedback_friction_points",
+        "$mcp_feedback_suggested_improvement",
+        "$mcp_feedback_tool",
+        "$mcp_feedback_area",
+    ):
+        assert "jane@example.com" not in props[key], key
+        assert "jane%40example.com" not in props[key], key
+        assert "[redacted]" in props[key], key
+
+
+def test_nested_extras_keep_key_based_redaction():
+    # The feedback path walks extras with the free-text pass; nothing else
+    # asserts that credential-named keys inside a nested extra still redact by
+    # key name, so a stringify-first refactor could drop that protection silently.
+    options = CollectFeedbackOptions(extra_properties={"meta": {"type": "object"}})
+    report = parse_feedback_report(
+        {
+            "feedback_type": "issue",
+            "summary": "s",
+            "meta": {"note": "ping jane@example.com", "password": "hunter2"},
+        },
+        options,
     )
     props = build_feedback_event_properties(report)
-    assert "jane@example.com" not in props["$mcp_feedback_summary"]
-    assert "jane%40example.com" not in props["$mcp_feedback_summary"]
-    assert "[redacted]" in props["$mcp_feedback_summary"]
+    assert '"password": "[redacted]"' in props["$mcp_feedback_meta"]
+    assert "hunter2" not in props["$mcp_feedback_meta"]
+    assert '"note": "ping [redacted]"' in props["$mcp_feedback_meta"]
 
 
 def test_intent_joins_summary_and_details():