From 449ac2fb8f7b542ea58160e8df94f3fb645ed141 Mon Sep 17 00:00:00 2001 From: Eddie Wang Date: Mon, 21 Sep 2026 15:46:05 -0700 Subject: [PATCH] fix(photon): strip markdown when the sidecar downgrades to the text builder chooseSendFormat() routes markdown containing a raw http(s) URL through spectrum-ts' text() builder, because the markdown builder's iMessage data detection 500s on those messages (#73615). text() ships the payload verbatim, and nothing strips the markers on the way down, so any reply that mentions a link arrives in iMessage as literal markdown source: **Release 1.2.0** is out - **EUR 5** off this month https://example.com/releases/1.2.0 Every ** is visible in the bubble. Remove the URL from that same reply and it renders correctly, which is what makes the URL the trigger rather than the content. spectrum-ts documents markdown() as degrading to readable plain text on platforms without native support "instead of surfacing raw ** markers". Selecting text() ourselves opts out of that guarantee, so the adapter has to honour it instead. Strip in _sidecar_send() when the payload is markdown and the sidecar will downgrade it. The format key is deliberately preserved: the sidecar owns the builder choice (test_rich_links.py pins that contract), and an older sidecar without chooseSendFormat must keep rendering natively. _send_plain_fallback() selects the text builder explicitly via markdown=False and had the same leak, so it strips too. Reuses the shared strip_markdown() helper rather than adding a second implementation, so the PHOTON_MARKDOWN=false path and this path produce identical output and both inherit any future fix to that helper. Diagnosis previously reported in #85733, which was closed unmerged. Stripping must not take the URL with it, though. The shared helper collapsed [label](url) to label alone, which on this path is worse than raw markdown: iMessage auto-links bare URLs and nothing else, so the reply arrives with a description and no way to reach the link. Open the itinerary on Google Flights <- URL gone entirely So strip_markdown() takes keep_link_targets, which rewrites [label](https://url) as "label\nurl" (own line, because these URLs are often long) and leaves non-http targets such as mailto: or relative paths label-only, since iMessage won't linkify those either. The default is unchanged, so the SMS, IRC, Feishu and QQ callers keep dropping the target as before. plugins/platforms/line/adapter.py already carries a private strip_markdown_preserving_urls() for exactly this reason ("LINE auto-links bare URLs only"). This moves the behaviour behind the shared helper instead, so Photon's three plain-text paths -- the downgrade, _send_plain_fallback(), and PHOTON_MARKDOWN=false -- all agree. gateway/platforms/bluebubbles.py is the same iMessage surface with the same loss; left alone here to keep this change to one platform. --- gateway/platforms/helpers.py | 39 +++++-- plugins/platforms/photon/adapter.py | 41 ++++++- tests/gateway/test_strip_markdown_links.py | 39 +++++++ .../plugins/platforms/photon/test_markdown.py | 108 ++++++++++++++++++ 4 files changed, 217 insertions(+), 10 deletions(-) create mode 100644 tests/gateway/test_strip_markdown_links.py diff --git a/gateway/platforms/helpers.py b/gateway/platforms/helpers.py index bc06f4791b..0dec58d6c5 100644 --- a/gateway/platforms/helpers.py +++ b/gateway/platforms/helpers.py @@ -88,8 +88,9 @@ def bounded_put(store: MutableMapping[str, Any], key: str, value: Any, cap: int) del store[next(iter(store))] -# Markdown-stripping rules, applied in order: bold, italic, bold/italic underscore, -# code fence markers, inline code, headings, links, then newline squeeze. +# Inline markdown-stripping rules, applied in order: bold, italic, bold/italic +# underscore, code fence markers, inline code, headings. Links and the newline +# squeeze run after these, in that order (see ``strip_markdown``). _STRIP_RULES = ( (re.compile(r"\*\*(.+?)\*\*", re.DOTALL), r"\1"), (re.compile(r"\*(.+?)\*", re.DOTALL), r"\1"), @@ -98,16 +99,40 @@ _STRIP_RULES = ( (re.compile(r"```[a-zA-Z0-9_+-]*\n?"), ""), (re.compile(r"`(.+?)`"), r"\1"), (re.compile(r"^#{1,6}\s+", re.MULTILINE), ""), - (re.compile(r"\[([^\]]+)\]\([^\)]+\)"), r"\1"), - (re.compile(r"\n{3,}"), "\n\n"), ) +_MD_LINK_RE = re.compile(r"\[([^\]]+)\]\(([^\)]+)\)") +_HTTP_TARGET_RE = re.compile(r"https?://", re.IGNORECASE) +_NEWLINE_SQUEEZE_RE = re.compile(r"\n{3,}") -def strip_markdown(text: str) -> str: - """Strip markdown formatting for plain-text platforms (SMS, iMessage, etc.).""" +def _drop_link_target(match: "re.Match[str]") -> str: + return match.group(1) + + +def _keep_link_target(match: "re.Match[str]") -> str: + r"""``[label](https://url)`` -> ``label\nurl``. + + The bare URL is the only thing a platform with its own data detection + (iMessage) can turn back into a tap target, so dropping it makes the link + unreachable rather than merely unformatted. Non-http targets (``mailto:``, + relative paths) are not auto-linked, so they keep the label-only behaviour. + """ + label, target = match.group(1).strip(), match.group(2).strip() + if not _HTTP_TARGET_RE.match(target): + return match.group(1) + return target if label == target else f"{label}\n{target}" + + +def strip_markdown(text: str, *, keep_link_targets: bool = False) -> str: + r"""Strip markdown formatting for plain-text platforms (SMS, iMessage, etc.). + + ``keep_link_targets`` rewrites ``[label](https://url)`` as ``label\nurl`` + instead of discarding the URL; pass it on platforms that auto-link bare URLs. + """ for pattern, repl in _STRIP_RULES: text = pattern.sub(repl, text) - return text.strip() + text = _MD_LINK_RE.sub(_keep_link_target if keep_link_targets else _drop_link_target, text) + return _NEWLINE_SQUEEZE_RE.sub("\n\n", text).strip() class ThreadParticipationTracker: diff --git a/plugins/platforms/photon/adapter.py b/plugins/platforms/photon/adapter.py index 1975db6ded..ed74e0f349 100644 --- a/plugins/platforms/photon/adapter.py +++ b/plugins/platforms/photon/adapter.py @@ -297,6 +297,32 @@ def _markdown_enabled() -> bool: return _get_scoped_secret("PHOTON_MARKDOWN", "true").strip().lower() not in {"false", "0", "no"} +# Mirrors URL_RE in plugins/platforms/photon/sidecar/send-format.mjs. Keep the +# two in sync: the sidecar owns the builder choice, this owns the payload that +# choice implies. +_SIDECAR_URL_RE = re.compile(r"https?://[^\s)'\"<>]+", re.IGNORECASE) + + +def _strip_for_imessage(text: str) -> str: + """Strip markdown, but keep ``[label](url)`` targets as a bare URL on its own line. + + Every plain-text path here ends at iMessage, which auto-links bare URLs and + nothing else, so discarding a link target makes the link unreachable — see + the ``chooseSendFormat`` comment for why these payloads lose markdown at all. + """ + return strip_markdown(text, keep_link_targets=True) + + +def _sidecar_downgrades_to_text(text: str) -> bool: + """True when the sidecar will route a markdown payload to the text builder. + + ``chooseSendFormat`` sends markdown containing a raw http(s) URL through + spectrum-ts' ``text()`` builder, because the markdown builder's iMessage + data detection 500s on those messages. + """ + return bool(_SIDECAR_URL_RE.search(text or "")) + + def _url_only_candidate(text: str) -> Optional[str]: candidate = (text or "").strip() if not re.fullmatch(r"https?://\S+", candidate, flags=re.IGNORECASE): @@ -1293,7 +1319,7 @@ class PhotonAdapter(BasePlatformAdapter): def format_message(self, content: str) -> str: # Markdown passes through verbatim (sidecar markdown() builder); PHOTON_MARKDOWN=false strips. - return content if _markdown_enabled() else strip_markdown(content) + return content if _markdown_enabled() else _strip_for_imessage(content) @staticmethod def _is_retryable_error(error: Optional[str]) -> bool: @@ -1322,7 +1348,8 @@ class PhotonAdapter(BasePlatformAdapter): """No Markdown banner (replies are markdown or already-stripped plain text); bypass richlink() so a rich-link outage doesn't strand a sendable URL.""" return await self._sidecar_send( - chat_id, self.format_message(content)[: self.MAX_MESSAGE_LENGTH], richlink=False, markdown=False) + chat_id, _strip_for_imessage(self.format_message(content))[: self.MAX_MESSAGE_LENGTH], + richlink=False, markdown=False) async def _post_send(self, path: str, body: Dict[str, Any], *, structured: bool = False) -> SendResult: """POST a send-like body and wrap the outcome as a SendResult. ``structured`` carries @@ -1349,11 +1376,19 @@ class PhotonAdapter(BasePlatformAdapter): return rich_result logger.warning("[photon] rich-link send failed, falling back to plain text: %s", rich_result.error) markdown = False + send_markdown = markdown and _markdown_enabled() + if send_markdown and _sidecar_downgrades_to_text(text): + # spectrum-ts' markdown() degrades to readable plain text on its own; + # text() does not, and ships the source verbatim. Strip before the + # sidecar downgrades, or iMessage renders literal ** markers. + # The format key stays: the sidecar owns the builder choice, and an + # older sidecar without chooseSendFormat must keep rendering natively. + text = _strip_for_imessage(text) if len(text) > self.MAX_MESSAGE_LENGTH: logger.warning("[photon] truncating outbound from %d to %d chars", len(text), self.MAX_MESSAGE_LENGTH) text = text[: self.MAX_MESSAGE_LENGTH] body: Dict[str, Any] = {"spaceId": space_id, "text": text} - if markdown and _markdown_enabled(): # key omitted when disabled: pre-`format` sidecars still accept + if send_markdown: # key omitted when disabled: pre-`format` sidecars still accept body["format"] = "markdown" return await self._post_send("/send", body, structured=True) diff --git a/tests/gateway/test_strip_markdown_links.py b/tests/gateway/test_strip_markdown_links.py new file mode 100644 index 0000000000..e6f316a51e --- /dev/null +++ b/tests/gateway/test_strip_markdown_links.py @@ -0,0 +1,39 @@ +"""``strip_markdown`` link handling. + +The default drops a link target (SMS, IRC, Feishu, QQ all rely on that); +``keep_link_targets=True`` leaves the bare URL behind for platforms whose own +data detection is the only thing that can re-linkify it. +""" +from __future__ import annotations + +from gateway.platforms.helpers import strip_markdown + +_LINK = "[Open the itinerary](https://example.com/i?tfs=CBwQ&hl=en)" + + +def test_link_target_is_dropped_by_default() -> None: + assert strip_markdown(f"see {_LINK} now") == "see Open the itinerary now" + + +def test_keep_link_targets_puts_the_url_on_its_own_line() -> None: + assert strip_markdown(_LINK, keep_link_targets=True) == ( + "Open the itinerary\nhttps://example.com/i?tfs=CBwQ&hl=en" + ) + + +def test_keep_link_targets_does_not_duplicate_a_self_labelled_link() -> None: + url = "https://example.com/x" + assert strip_markdown(f"[{url}]({url})", keep_link_targets=True) == url + + +def test_keep_link_targets_ignores_non_http_targets() -> None: + """``mailto:``/relative targets are not auto-linked, so emitting them is noise.""" + text = "[mail](mailto:a@example.com) and [rel](/docs/page)" + assert strip_markdown(text, keep_link_targets=True) == "mail and rel" + + +def test_keep_link_targets_still_strips_inline_formatting() -> None: + text = f"**Release 1.2.0**\n- `code` here\n{_LINK}" + out = strip_markdown(text, keep_link_targets=True) + assert "**" not in out and "`" not in out + assert out.endswith("https://example.com/i?tfs=CBwQ&hl=en") diff --git a/tests/plugins/platforms/photon/test_markdown.py b/tests/plugins/platforms/photon/test_markdown.py index 2013bbc01f..5fd688aeb7 100644 --- a/tests/plugins/platforms/photon/test_markdown.py +++ b/tests/plugins/platforms/photon/test_markdown.py @@ -103,3 +103,111 @@ async def test_standalone_send_includes_markdown_format( assert result.get("success") is True assert posted[0][1]["format"] == "markdown" + + +_MD_WITH_URL = ( + "**Release 1.2.0** is out\n" + "- **\u20ac5** off this month\n" + "https://example.com/releases/1.2.0" +) +_MD_WITH_LINK = "**Release 1.2.0** is out\n[Read the notes](https://example.com/releases/1.2.0)" + + +@pytest.mark.asyncio +async def test_url_bearing_markdown_is_stripped_before_the_text_builder( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A markdown payload the sidecar downgrades must not ship raw markers. + + ``chooseSendFormat`` routes markdown containing a raw URL through + spectrum-ts' ``text()`` builder, which sends the source verbatim. + """ + adapter = _make_adapter(monkeypatch) + calls = _capture_sidecar(adapter) + + await adapter.send("space-1", _MD_WITH_URL) + + path, body = calls[-1] + assert path == "/send" + # The format key is deliberately preserved: the sidecar owns the builder + # choice (see test_rich_links.py). Only the payload changes. + assert body["format"] == "markdown" + assert "**" not in body["text"] + # Stripping must not damage the parts iMessage still needs. + assert "https://example.com/releases/1.2.0" in body["text"] + assert "\u20ac5" in body["text"] + + +@pytest.mark.asyncio +async def test_url_free_markdown_still_renders_natively( + monkeypatch: pytest.MonkeyPatch, +) -> None: + adapter = _make_adapter(monkeypatch) + calls = _capture_sidecar(adapter) + + await adapter.send("space-1", _MD) + + _, body = calls[-1] + assert body["format"] == "markdown" + assert body["text"] == _MD, "URL-free markdown must reach markdown() untouched" + + +@pytest.mark.asyncio +async def test_plain_fallback_strips_markdown( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The explicit fallback selects the text builder, so it must strip too.""" + adapter = _make_adapter(monkeypatch) + calls = _capture_sidecar(adapter) + + await adapter._send_plain_fallback("space-1", _MD, reply_to=None, metadata=None) + + _, body = calls[-1] + assert "format" not in body + assert "**" not in body["text"] + assert "`" not in body["text"] + + +@pytest.mark.asyncio +async def test_markdown_link_keeps_a_tappable_url( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A ``[label](url)`` link must not lose its URL on the way to iMessage. + + The URL inside ``](...)`` is enough for ``chooseSendFormat`` to pick the text + builder, so this payload gets stripped — and iMessage auto-links bare URLs + only. Dropping the target (the shared default) leaves the link unreachable. + """ + adapter = _make_adapter(monkeypatch) + calls = _capture_sidecar(adapter) + + await adapter.send("space-1", _MD_WITH_LINK) + + _, body = calls[-1] + assert body["text"] == ( + "Release 1.2.0 is out\nRead the notes\nhttps://example.com/releases/1.2.0" + ) + + +@pytest.mark.asyncio +async def test_plain_fallback_keeps_link_urls( + monkeypatch: pytest.MonkeyPatch, +) -> None: + adapter = _make_adapter(monkeypatch) + calls = _capture_sidecar(adapter) + + await adapter._send_plain_fallback("space-1", _MD_WITH_LINK, reply_to=None, metadata=None) + + _, body = calls[-1] + assert "**" not in body["text"] + assert "https://example.com/releases/1.2.0" in body["text"] + + +def test_markdown_disabled_keeps_link_urls(monkeypatch: pytest.MonkeyPatch) -> None: + """``PHOTON_MARKDOWN=false`` strips through the same iMessage-aware helper.""" + monkeypatch.setenv("PHOTON_MARKDOWN", "false") + adapter = _make_adapter(monkeypatch) + + assert adapter.format_message(_MD_WITH_LINK) == ( + "Release 1.2.0 is out\nRead the notes\nhttps://example.com/releases/1.2.0" + )