Skip to content

fix(channels): escape Slack reserved chars before mrkdwn conversion#4197

Open
DaoyuanLi2816 wants to merge 2 commits into
bytedance:mainfrom
DaoyuanLi2816:fix/slack-html-entity-escaping
Open

fix(channels): escape Slack reserved chars before mrkdwn conversion#4197
DaoyuanLi2816 wants to merge 2 commits into
bytedance:mainfrom
DaoyuanLi2816:fix/slack-html-entity-escaping

Conversation

@DaoyuanLi2816

Copy link
Copy Markdown
Contributor

Summary

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>, <http://url|label>). SlackChannel.send() never did this, so any outbound message containing those characters -- code snippets, shell conditionals, URL query strings -- reaches Slack's API unescaped.

The bug

kwargs: dict[str, Any] = {
    "channel": msg.chat_id,
    "text": _slack_md_converter.convert(msg.text),
}

msg.text goes through the markdown-to-mrkdwn converter (_slack_md_converter, from the markdown_to_mrkdwn library) and the result is handed straight to chat_postMessage. The converter only rewrites markdown syntax (**bold**, [text](url), headings, lists, ...) -- it passes &, <, and > through completely unchanged (added test_converter_passes_reserved_characters_through_unchanged to pin this).

Concretely, a reply containing if a < b && b > c: is sent to Slack's API byte-for-byte as-is. Slack's renderer sees a literal < and tries to parse <@U12345>-style mention/link syntax out of whatever follows it, so the text renders broken or misinterpreted instead of the literal code the agent wrote. The same applies to any raw <@...>/<#...>-shaped substring in agent output (e.g. quoting a user's own message back).

The fix

Escaping has to run before the mrkdwn conversion, not after. The converter itself emits mrkdwn link syntax -- [text](url) becomes <url|text>, ![alt](url) becomes <url> -- and that generated <...> is exactly what must reach Slack unescaped. If escaping ran after conversion, it would re-escape the converter's own output and turn every real link into a dead &lt;url|text&gt;.

So the fix escapes the raw input first, then lets the converter run on the escaped text and emit its own unescaped link syntax on top:

"text": _slack_md_converter.convert(_escape_slack_text(msg.text)),

_escape_slack_text is html.escape(text, quote=False) -- stdlib html.escape already replaces & before </> internally, which matters here: escaping & first avoids double-escaping the & inside the &amp;/&lt;/&gt; entities being introduced. quote=False skips quote-escaping, since Slack's own escaping rule only covers &/</>.

This ordering is safe for real links: markdown link syntax ([label](url)) uses [, ], (, ), none of which are touched by escaping, so the converter's link-detection regex still matches the escaped text unchanged. A literal & inside a URL's query string (e.g. ?a=1&b=2) is still escaped to &amp; per Slack's rule, including inside the converter's generated <url|label> -- Slack unescapes entities anywhere in the message when rendering, links included.

Testing

New TestSlackTextEscaping in backend/tests/test_channels.py drives SlackChannel.send() end-to-end against a mocked chat_postMessage and asserts on the actual text payload:

  • test_raw_angle_brackets_and_ampersand_are_escaped -- "if a < b && b > c:" arrives as "if a &lt; b &amp;&amp; b &gt; c:".
  • test_bot_mention_syntax_is_neutralized_not_interpreted -- a raw <@U12345>-shaped substring no longer survives as live mention syntax.
  • test_real_markdown_link_still_converts_without_double_escaping -- critical non-regression case: [DeerFlow docs](https://example.com/docs) still converts to <https://example.com/docs|DeerFlow docs> with no extra escaping corrupting it.
  • test_ampersand_in_link_url_is_escaped_before_conversion -- a literal & inside a link URL's query string still comes through as &amp; inside the generated <url|label>.

Verification performed:

  • Fail-before/pass-after via patch-file revert: reverted just the source fix (tests untouched) and reran -- the three bug-covering tests failed on unfixed main-equivalent code; reapplying the fix turned them green again.
  • Confirmed the non-regression test actually discriminates the ordering choice: temporarily swapped the implementation to escape after conversion (the wrong order) and reran -- test_real_markdown_link_still_converts_without_double_escaping and the URL-ampersand test both failed as expected (<https://example.com/docs|DeerFlow docs> corrupted into &lt;https://example.com/docs|DeerFlow docs&gt;), confirming the test isn't a tautology.
  • Full backend/tests/test_channels.py (all channels, not just Slack): 256 passed.
  • backend/tests/test_slack_channel_connections.py: 3 passed.
  • ruff check --line-length 240 and ruff format --check --line-length 240 on both changed files: clean.

…ersion

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>,
<http://url|label>). 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 <url|label> 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.
@github-actions github-actions Bot added area:backend Gateway / runtime / core backend under backend/ risk:medium Medium risk: regular code changes size/M PR changes 100-300 lines labels Jul 15, 2026

@willem-bd willem-bd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DaoyuanLi2816. I found one issue that should be addressed before this is ready.

[P2] Escaping > neutralizes markdown blockquotes that previously rendered as Slack blockquotes

  • Location: backend/app/channels/slack.py_escape_slack_text (applied to the text kwarg in SlackChannel.send)
  • Problem: html.escape(text, quote=False) escapes every > to &gt; before the mrkdwn conversion. A markdown blockquote — > at the start of a line — previously passed through the converter unchanged and reached Slack as a literal >, which Slack renders as a block quote. After this change it arrives as &gt;, which Slack renders as a literal > character instead of a block quote. Agent output that uses > for quoting/callouts loses its blockquote styling. (The same mechanism neutralizes raw <https://url> autolinks too, since the converter passes those through unchanged — though that one is harder to avoid, because < must be escaped to prevent mention/link injection.)
  • Evidence: SlackMarkdownConverter().convert("> quoted text") == "> quoted text" — the converter passes > through unchanged, so before this PR a line-start > reached the API unescaped and rendered as a blockquote. Escaping > to &gt; is exactly what Slack's escaping guidance prescribes to prevent > being treated as a blockquote, so the blanket escape flips previously-correct blockquote rendering to literal text. The new TestSlackTextEscaping cases cover &/</> inside code and links, but none assert that blockquote rendering is preserved.
  • Suggested fix: & and < are what neutralize Slack's <...> mention/link syntax (the injection this PR targets); > is only special at line start, where it is the intended blockquote marker. Narrow the escape so line-start > survives — either don't escape > (a targeted text.replace("&", "&amp;").replace("<", "&lt;") in that order, instead of html.escape(..., quote=False)), or keep html.escape and restore line-start markers after escaping (e.g. re.sub(r"(?m)^&gt;", ">", text)). The [label](url) link path stays unaffected either way, since escaping runs before conversion and []() contains no </>.
  • Test: add a TestSlackTextEscaping case asserting a blockquote still produces a line starting with > (e.g. send("> quoted text") yields "> quoted text") while </& elsewhere in the same input remain escaped.

_escape_slack_text's html.escape(text, quote=False) replaces every ">"
with "&gt;", 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 "&gt; " 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Gateway / runtime / core backend under backend/ risk:medium Medium risk: regular code changes size/M PR changes 100-300 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants