From 8153e68eb85e92be893253e90077da45f38f9732 Mon Sep 17 00:00:00 2001 From: Daoyuan Li <94409450+DaoyuanLi2816@users.noreply.github.com> Date: Mon, 20 Jul 2026 19:02:49 -0700 Subject: [PATCH] fix(channels): escape Slack reserved chars before mrkdwn conversion (#4197) * fix(channels): escape &/ in Slack outbound text before mrkdwn conversion Slack requires callers to replace &, <, and > with their HTML entity equivalents before sending message text, since an unescaped <...> triggers Slack's own mention/link syntax (e.g. <@USERID>, ). SlackChannel.send() ran msg.text through the markdown-to-mrkdwn converter and sent the result straight to chat_postMessage with no escaping step, so technical/code content like "if a < b && b > c:" would arrive as literal Slack markup and render broken or misinterpreted. Escaping must happen before the mrkdwn conversion, not after: the converter emits its own syntax for real markdown links ([text](url)), and that generated syntax must reach Slack unescaped. Escaping raw input first (via html.escape(text, quote=False), which replaces & before ) and leaving the converter's output alone satisfies both requirements. * fix(channels): preserve a line-leading > as Slack's blockquote marker _escape_slack_text's html.escape(text, quote=False) replaces every ">" with ">", including a ">" at the start of a line -- Slack's own mrkdwn blockquote marker, which the converter otherwise passes through unchanged. Agent output using markdown blockquotes for quoting/callouts lost its blockquote styling and arrived as a literal "> " prefix instead of a rendered blockquote. Only "&" and "<" neutralize Slack's "<...>" mention/link syntax; ">" is special to Slack only at the start of a line. Restore a line-leading ">" to a literal character after escaping (re.sub with MULTILINE), so the mrkdwn converter still renders it as a blockquote; a "<"/"&" anywhere, and a ">" that is not at a line start, still escape. New tests cover the line-start preservation, that the exemption does not widen into "never escape '>'" (a mid-line/non-leading ">" still escapes), and that restoration applies per line in multiline text; all three revert cleanly against the unconditional html.escape() to reproduce the blockquote-corruption bug. Full test_channels.py (259 tests) plus ruff check/format are clean. --- backend/app/channels/slack.py | 33 ++++++++++- backend/tests/test_channels.py | 104 +++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 1 deletion(-) diff --git a/backend/app/channels/slack.py b/backend/app/channels/slack.py index 2da95d466..6e0470d5c 100644 --- a/backend/app/channels/slack.py +++ b/backend/app/channels/slack.py @@ -3,7 +3,9 @@ from __future__ import annotations import asyncio +import html import logging +import re from typing import Any from markdown_to_mrkdwn import SlackMarkdownConverter @@ -18,6 +20,35 @@ logger = logging.getLogger(__name__) _slack_md_converter = SlackMarkdownConverter() +def _escape_slack_text(text: str) -> str: + """Escape Slack's reserved characters (``&``, ``<``, ``>``) in raw message text, + except a ``>`` at the very start of a line -- Slack's own blockquote marker. + + Slack requires callers to replace these with their HTML entity equivalents + (``&``, ``<``, ``>``) before sending message text -- an unescaped + ``<...>`` triggers Slack's own mention/link syntax (e.g. ``<@USERID>``, + ````). See: + https://api.slack.com/reference/surfaces/formatting#escaping + + This MUST run before ``_slack_md_converter.convert()``, not after: the + converter emits its own mrkdwn link syntax (````) for real + markdown links, and that generated syntax must reach Slack unescaped. + Escaping the raw input first -- and leaving the converter's own output + alone -- satisfies both requirements. ``html.escape(..., quote=False)`` + replaces ``&`` before ``<``/``>``, so the entities it introduces are never + re-escaped. + + Only ``&`` and ``<`` neutralize Slack's ``<...>`` mention/link syntax; a + ``>`` is special to Slack only at the start of a line, where the mrkdwn + converter passes it through unchanged as a blockquote marker. Escaping + every ``>`` would turn a quoted line into visible ``>`` text instead of + a rendered blockquote, so a line-leading ``>`` is restored to a literal + ``>`` after escaping; a ``>`` anywhere else in the text still escapes. + """ + escaped = html.escape(text, quote=False) + return re.sub(r"(?m)^>", ">", escaped) + + def _normalize_allowed_users(allowed_users: Any) -> set[str]: if allowed_users is None: return set() @@ -128,7 +159,7 @@ class SlackChannel(Channel): kwargs: dict[str, Any] = { "channel": msg.chat_id, - "text": _slack_md_converter.convert(msg.text), + "text": _slack_md_converter.convert(_escape_slack_text(msg.text)), } if msg.thread_ts: kwargs["thread_ts"] = msg.thread_ts diff --git a/backend/tests/test_channels.py b/backend/tests/test_channels.py index 94fc5a526..65abbcca8 100644 --- a/backend/tests/test_channels.py +++ b/backend/tests/test_channels.py @@ -7153,6 +7153,110 @@ class TestSlackMarkdownConversion: assert "*Title*" in result assert "#" not in result + def test_converter_passes_reserved_characters_through_unchanged(self): + # The library itself never escapes Slack's reserved characters -- this + # pins that assumption so SlackChannel.send() knows it must do so itself. + from app.channels.slack import _slack_md_converter + + result = _slack_md_converter.convert("if a < b && b > c:") + assert result == "if a < b && b > c:" + + +# --------------------------------------------------------------------------- +# Slack outbound text escaping tests (Slack's &/ HTML-entity requirement) +# +# Slack requires callers to replace &, <, and > with their HTML entity +# equivalents before sending message text, because an unescaped `<...>` +# triggers Slack's own mention/link syntax (e.g. `<@USERID>`, +# ``). See: +# https://api.slack.com/reference/surfaces/formatting#escaping +# --------------------------------------------------------------------------- + + +class TestSlackTextEscaping: + @staticmethod + def _sent_text(text: str) -> str: + """Send *text* through SlackChannel.send() and return the resulting + Slack API ``text`` kwarg, without actually hitting the network.""" + from app.channels.slack import SlackChannel + + captured: dict[str, object] = {} + + async def go(): + bus = MessageBus() + ch = SlackChannel(bus=bus, config={"bot_token": "xoxb-test", "app_token": "xapp-test"}) + + mock_web = MagicMock() + + def post_message(**kwargs): + captured.update(kwargs) + return MagicMock() + + mock_web.chat_postMessage = post_message + ch._web_client = mock_web + + msg = OutboundMessage(channel_name="slack", chat_id="C123", thread_id="t1", text=text) + await ch.send(msg) + + _run(go()) + return captured["text"] + + def test_raw_angle_brackets_and_ampersand_are_escaped(self): + # Realistic technical/code content containing all three reserved + # characters must arrive escaped, so Slack renders it as literal text + # instead of attempting to parse a broken mention/link. + sent = self._sent_text("if a < b && b > c:") + assert sent == "if a < b && b > c:" + assert "<" not in sent + assert ">" not in sent + + def test_bot_mention_syntax_is_neutralized_not_interpreted(self): + # Raw text that happens to look like a mention must not survive as + # live `<@...>` syntax -- Slack would otherwise try to resolve it. + sent = self._sent_text("please ask <@U12345> for review") + assert sent == "please ask <@U12345> for review" + + def test_real_markdown_link_still_converts_without_double_escaping(self): + # Critical non-regression case: escaping must run BEFORE mrkdwn + # conversion, not after. The converter's own generated `` + # syntax for a real markdown link must survive untouched -- if + # escaping ran after conversion instead, this would corrupt into + # `<url|label>` and Slack would render a dead link. + sent = self._sent_text("See [DeerFlow docs](https://example.com/docs) for more.") + assert "" in sent + assert "<" not in sent + assert ">" not in sent + + def test_ampersand_in_link_url_is_escaped_before_conversion(self): + # & must be escaped first (before < and >) so it doesn't double-escape + # the &/</> entities being introduced, and a literal '&' in a + # URL must still come through as & per Slack's escaping rule -- + # even inside the converter's own generated syntax. + sent = self._sent_text("[Search](https://example.com?a=1&b=2)") + assert "" in sent + + def test_blockquote_marker_at_line_start_is_preserved(self): + # A ">" at the very start of a line is Slack's own blockquote marker + # (the mrkdwn converter passes it through unchanged), not part of the + # <...> mention/link syntax that & and < neutralize. Escaping it would + # turn a quoted line into visible ">" text instead of a rendered + # blockquote. + sent = self._sent_text("> quoted text") + assert sent == "> quoted text" + + def test_blockquote_marker_exemption_is_line_start_only(self): + # The line-start exemption must not widen into "never escape '>'": + # a "<"/"&" anywhere, and a ">" that is NOT at the start of a line, + # still escape -- only the leading marker is restored. + sent = self._sent_text("> a < b & c > d") + assert sent == "> a < b & c > d" + + def test_blockquote_marker_restored_on_every_line(self): + # The restoration must apply per-line (re.MULTILINE), not just once + # at the start of the whole string. + sent = self._sent_text("intro\n> first quote\nmiddle\n> second quote") + assert sent == "intro\n> first quote\nmiddle\n> second quote" + # --------------------------------------------------------------------------- # Telegram streaming tests