fix(slack): stop double-decoding HTML entities when escaping message text - #64748
fix(slack): stop double-decoding HTML entities when escaping message text#64748briandevans wants to merge 1 commit into
Conversation
…text
format_message unescapes already-escaped input before re-escaping, so that
pre-escaped text doesn't get double-escaped. That unescape was three
sequential str.replace calls, which re-scan each other's output:
"&lt;" --(& -> &)--> "<" --(< -> <)--> "<"
The & produced by the first replace pairs with the following "lt;" and
decodes a second time. "&lt;" is the wire form of the literal text
"<", so the text is silently destroyed: Slack receives "<" and renders
"<". Anyone writing about HTML or markup ("&lt;b&gt;" -> "<b>")
loses their literal text, with no error.
re.sub scans left-to-right and never re-scans its own replacements, so a
single pass fixes it. The escape pass on the next line is left untouched --
it is correctly ordered (& first, so the &s it inserts aren't re-escaped).
Only the double-decode cases change; every other input is byte-identical
before and after. This is the same round-trip invariant the neighbouring
test_pre_escaped_{ampersand,lt,gt}_not_double_escaped tests already assert,
extended to the case they miss. Affects the plain mrkdwn path (send,
edit_message) and Block Kit sections, which route section text through
format_message.
There was a problem hiding this comment.
Pull request overview
Fixes a Slack message-formatting bug where the unescape pass could decode entity text twice (e.g., &lt; collapsing to <), causing users’ literal entity strings to be silently altered before sending to Slack.
Changes:
- Replace chained
str.replaceunescape logic informat_message()with a single left-to-rightre.subpass to avoid re-processing replacement output. - Add a regression test ensuring
&lt;/&gt;(wire forms for literal entity text) round-trip unchanged.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| plugins/platforms/slack/adapter.py | Changes the entity-unescape step to a single-pass regex substitution to prevent double-decoding before re-escaping. |
| tests/gateway/test_slack.py | Adds a regression test covering the double-decode case for literal entity text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for the focused regression fix. Current GitHub reports the branch cleanly mergeable, and required CI checks—including all Python test slices—passed. Automated hermes-sweeper review. |
alt-glitch
added
type/bug
teknium1
added
sweeper:risk-compatibility
|
Merged via #70191 — your commit was cherry-picked/reapplied onto current main with your authorship preserved in git history: your single-pass entity decode was cherry-picked (verified live: main double-decoded). Thanks for the contribution! |
What does this PR do?
format_messagesilently destroys literal HTML-entity text before it reaches Slack.Step 6 unescapes already-escaped input so it doesn't get double-escaped — a real invariant the test suite already guards. But the unescape is three sequential
str.replacecalls, and each re-scans the previous one's output:The
&produced by the first replace pairs with the followinglt;and decodes a second time.&lt;is the wire form of the literal text<— so Slack receives<and renders<. The user's literal text is gone, with no error.Observed on
main(buggy) vs. patched (fixed):main&lt;<&lt;&gt;>&gt;&lt;b&gt;<b>→ renders<b>&lt;b&gt;&amp;&amp;&amp;</>/&a & b/<x>/AT&T < 5 > 3Anyone writing about markup — a code review comment, a docs snippet, an agent explaining HTML — hits this. Reaches Slack via
send()(L1414),edit_message()(L1531), and Block Kit sections (L2046 routes section text throughformat_message), so both the plain mrkdwn and Block Kit paths are affected.re.subscans left-to-right and never re-scans its own replacements, so a single pass fixes it. Only the double-decode cases change; every other input is byte-identical before and after.This is not a new invariant. It's the same contract
test_pre_escaped_ampersand_not_double_escaped,test_pre_escaped_lt_not_double_escaped, andtest_pre_escaped_gt_not_double_escapedalready assert (L2510-2520) — extended to the case they miss.Related Issue
None — found by reading the escaping passes in
format_messageagainst the round-trip invariant its own tests assert.Type of Change
Changes Made
plugins/platforms/slack/adapter.py— replace the three chainedstr.replaceunescape calls with a singlere.subpass over&(amp|lt|gt);. The escape line below is deliberately left as-is: it's correctly ordered (&first, so the&s it inserts aren't re-escaped).reis already imported (L16).tests/gateway/test_slack.py— regression test alongside the three existing pre-escape tests inTestFormatMessage.How to Test
main:adapter.format_message("&lt;")returns'<'instead of'&lt;'.pytest tests/gateway/test_slack.py -k escaped_entity_text_not_double_decodedon unpatched code → fails withassert '<' == '&lt;'.tests/gateway/test_slack.py; 400 passed across the Slack surface (test_slack.py,test_slack_block_kit.py,test_slack_block_kit_adapter.py,test_slack_approval_buttons.py,test_slack_mention.py,test_slack_plugin_action_handlers.py,test_slack_channel_skills.py).test_pre_escaped_*_not_double_escapedtests andtest_mixed_raw_and_escaped_entitiesstay green — the fix doesn't over-correct.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AContract Protected
Invariant: unescaping already-escaped input must decode each entity exactly once — an escaped entity's own text must round-trip intact.
&lt;and&gt;(the wire forms of the literal texts<and>). Both previously decoded twice and lost their literal meaning.&immediately followed bylt;/gt;triggers it.re.sub's single left-to-right pass removes the class of bug rather than the two instances, so a future entity added to the map can't reintroduce it.test_pre_escaped_*_not_double_escapedtests plustest_mixed_raw_and_escaped_entitiesandtest_escapes_control_charactersconfirm the fix doesn't under-decode — genuinely pre-escaped single entities and raw&/</>are handled exactly as before.