From eca8bd75f431d7719c8ea5b1e9a18d4461c2ef08 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Wed, 25 Feb 2026 10:42:48 -0500 Subject: [PATCH 1/3] bridge: convert Markdown to Slack mrkdwn in outbound messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LLMs naturally write Markdown (**bold**, [links](url), # headings, etc.) but Slack uses mrkdwn (*bold*, , no headings). Messages were sent verbatim, rendering incorrectly in Slack. Add markdownToMrkdwn() converter in security.mjs and apply it in the outbound path of both broker-bridge and legacy bridge. Conversions: - **bold** / __bold__ → *bold* - ~~strike~~ → ~strike~ - [text](url) → - ![alt](url) → - # Heading → *Heading* (bolded line) - ```lang → ``` (strip language hints from fenced code) - --- / *** / ___ → ───────── (unicode horizontal rule) Code blocks and inline code are protected from transformation. Already-valid mrkdwn passes through unchanged. 29 new tests covering all conversion paths plus a real-world agent message example. --- slack-bridge/bridge.mjs | 11 +- slack-bridge/broker-bridge.mjs | 8 +- slack-bridge/security.mjs | 79 ++++++++++++++ slack-bridge/security.test.mjs | 192 +++++++++++++++++++++++++++++++++ 4 files changed, 284 insertions(+), 6 deletions(-) diff --git a/slack-bridge/bridge.mjs b/slack-bridge/bridge.mjs index cb436e6..5eb4f75 100644 --- a/slack-bridge/bridge.mjs +++ b/slack-bridge/bridge.mjs @@ -34,6 +34,7 @@ import { validateSendParams, validateReactParams, createRateLimiter, + markdownToMrkdwn, } from "./security.mjs"; // ── Config ────────────────────────────────────────────────────────────────── @@ -467,14 +468,15 @@ function startApiServer() { } const { channel, text, thread_ts } = apiRequestBody; + const safeText = markdownToMrkdwn(text); const result = await app.client.chat.postMessage({ token: process.env.SLACK_BOT_TOKEN, channel, - text, + text: safeText, ...(thread_ts && { thread_ts }), }); - console.log(`📤 Sent to ${channel}: ${text.slice(0, 80)}${text.length > 80 ? "..." : ""}`); + console.log(`📤 Sent to ${channel}: ${safeText.slice(0, 80)}${safeText.length > 80 ? "..." : ""}`); // If this is a threaded reply, check for a pending ✅ ack reaction. if (thread_ts) { @@ -511,14 +513,15 @@ function startApiServer() { return; } + const safeText = markdownToMrkdwn(text); const result = await app.client.chat.postMessage({ token: process.env.SLACK_BOT_TOKEN, channel: thread.channel, - text, + text: safeText, thread_ts: thread.thread_ts, }); - console.log(`📤 Reply to ${thread_id} (${thread.channel}): ${text.slice(0, 80)}${text.length > 80 ? "..." : ""}`); + console.log(`📤 Reply to ${thread_id} (${thread.channel}): ${safeText.slice(0, 80)}${safeText.length > 80 ? "..." : ""}`); // Check for a pending ✅ ack reaction on the /reply path too. resolveAckReaction(thread.channel, thread.thread_ts); diff --git a/slack-bridge/broker-bridge.mjs b/slack-bridge/broker-bridge.mjs index 4f8a2b3..ddf2997 100755 --- a/slack-bridge/broker-bridge.mjs +++ b/slack-bridge/broker-bridge.mjs @@ -22,6 +22,7 @@ import { validateReactParams, createRateLimiter, sanitizeOutboundText, + markdownToMrkdwn, } from "./security.mjs"; import { canonicalizeEnvelope, @@ -701,10 +702,13 @@ function sanitizeOutboundMessage(text, contextLabel) { const sanitized = sanitizeOutboundText(text); if (sanitized.blocked) { logWarn(`🛡️ outbound message blocked (${contextLabel}): ${sanitized.reasons.join(", ")}`); - } else if (sanitized.redacted) { + return sanitized.text; + } + if (sanitized.redacted) { logWarn(`🧼 outbound message redacted (${contextLabel}): ${sanitized.reasons.join(", ")}`); } - return sanitized.text; + // Convert Markdown → Slack mrkdwn so agent output renders correctly. + return markdownToMrkdwn(sanitized.text); } async function handleUserMessage(userMessage, event) { diff --git a/slack-bridge/security.mjs b/slack-bridge/security.mjs index 3c3917a..e357c80 100644 --- a/slack-bridge/security.mjs +++ b/slack-bridge/security.mjs @@ -252,6 +252,85 @@ export function formatForSlack(text) { return text; } +/** + * Convert Markdown to Slack mrkdwn. + * + * The agent (LLM) naturally writes Markdown but Slack uses mrkdwn, which has + * different syntax. This converts the most common divergences: + * + * - **bold** / __bold__ → *bold* + * - *italic* / _italic_ → _italic_ (already valid — but must not + * collide with bold conversion) + * - ~~strikethrough~~ → ~strikethrough~ + * - [text](url) → + * - ![alt](url) → (image links → plain link) + * - # Headings (h1–h6) → *Heading* (bolded line) + * - ```lang\ncode\n``` → ```\ncode\n``` (strip language hint) + * - `inline code` → `inline code` (already valid) + * - > blockquote → > blockquote (already valid) + * - Ordered list (1. item) → kept as-is (Slack renders plain text lists fine) + * - Horizontal rules (---, ***) → ───────── (unicode line) + * + * Designed to be safe/idempotent: already-valid mrkdwn passes through unchanged. + */ +export function markdownToMrkdwn(text) { + if (typeof text !== "string") return String(text); + + // Protect fenced code blocks from transformation by extracting them first. + // Use a placeholder unlikely to appear in real text (U+FFFC OBJECT REPLACEMENT CHARACTER). + const PH = "\uFFFC"; + const codeBlocks = []; + let processed = text.replace(/```[^\n]*\n([\s\S]*?)```/g, (_match, code) => { + const index = codeBlocks.length; + codeBlocks.push(`\`\`\`\n${code}\`\`\``); + return `${PH}CODEBLOCK_${index}${PH}`; + }); + + // Protect inline code spans from transformation. + const inlineCode = []; + processed = processed.replace(/`[^`\n]+`/g, (match) => { + const index = inlineCode.length; + inlineCode.push(match); + return `${PH}INLINE_${index}${PH}`; + }); + + // Headings: "# Heading" → "*Heading*" (bold line) + // Only match at line start; support h1–h6. + processed = processed.replace(/^#{1,6}\s+(.+)$/gm, (_, heading) => `*${heading.trim()}*`); + + // Images: ![alt](url) → (must come before link conversion) + processed = processed.replace(/!\[([^\]]*)\]\(([^)]+)\)/g, (_, alt, url) => { + return alt ? `<${url}|${alt}>` : `<${url}>`; + }); + + // Links: [text](url) → + processed = processed.replace(/\[([^\]]+)\]\(([^)]+)\)/g, (_, linkText, url) => { + return `<${url}|${linkText}>`; + }); + + // Bold: **text** or __text__ → *text* + // Use negative lookbehind/ahead to avoid converting already-single-star patterns. + // Process ** first, then __ . + processed = processed.replace(/\*\*(.+?)\*\*/g, (_, content) => `*${content}*`); + processed = processed.replace(/__(.+?)__/g, (_, content) => `*${content}*`); + + // Strikethrough: ~~text~~ → ~text~ + processed = processed.replace(/~~(.+?)~~/g, (_, content) => `~${content}~`); + + // Horizontal rules: lines that are just ---, ***, or ___ (with optional spaces) + processed = processed.replace(/^[ \t]*([-*_])\1{2,}[ \t]*$/gm, "─────────"); + + // Restore inline code spans. + const inlineRe = new RegExp(`${PH}INLINE_(\\d+)${PH}`, "g"); + processed = processed.replace(inlineRe, (_, index) => inlineCode[Number(index)]); + + // Restore code blocks. + const blockRe = new RegExp(`${PH}CODEBLOCK_(\\d+)${PH}`, "g"); + processed = processed.replace(blockRe, (_, index) => codeBlocks[Number(index)]); + + return processed; +} + // ── Bridge API Validation ─────────────────────────────────────────────────── /** diff --git a/slack-bridge/security.test.mjs b/slack-bridge/security.test.mjs index 6798c5b..b79f4a4 100644 --- a/slack-bridge/security.test.mjs +++ b/slack-bridge/security.test.mjs @@ -14,6 +14,7 @@ import { isAllowed, cleanMessage, formatForSlack, + markdownToMrkdwn, validateSendParams, validateReactParams, safeEqualSecret, @@ -316,6 +317,197 @@ describe("formatForSlack", () => { }); }); +// ── markdownToMrkdwn ──────────────────────────────────────────────────────── + +describe("markdownToMrkdwn", () => { + // Bold + it("converts **bold** to *bold*", () => { + assert.equal(markdownToMrkdwn("This is **bold** text"), "This is *bold* text"); + }); + + it("converts __bold__ to *bold*", () => { + assert.equal(markdownToMrkdwn("This is __bold__ text"), "This is *bold* text"); + }); + + it("converts multiple bold spans in one line", () => { + assert.equal( + markdownToMrkdwn("**first** and **second**"), + "*first* and *second*", + ); + }); + + // Strikethrough + it("converts ~~strikethrough~~ to ~strikethrough~", () => { + assert.equal(markdownToMrkdwn("This is ~~removed~~ text"), "This is ~removed~ text"); + }); + + // Links + it("converts [text](url) to ", () => { + assert.equal( + markdownToMrkdwn("See [the docs](https://example.com) here"), + "See here", + ); + }); + + it("converts image ![alt](url) to ", () => { + assert.equal( + markdownToMrkdwn("Check ![screenshot](https://img.example.com/pic.png)"), + "Check ", + ); + }); + + it("converts image with empty alt to bare URL", () => { + assert.equal( + markdownToMrkdwn("![](https://img.example.com/pic.png)"), + "", + ); + }); + + // Headings + it("converts # Heading to *Heading*", () => { + assert.equal(markdownToMrkdwn("# Main Title"), "*Main Title*"); + }); + + it("converts ## Heading to *Heading*", () => { + assert.equal(markdownToMrkdwn("## Section"), "*Section*"); + }); + + it("converts ### through ###### headings", () => { + assert.equal(markdownToMrkdwn("### Sub"), "*Sub*"); + assert.equal(markdownToMrkdwn("###### Deep"), "*Deep*"); + }); + + it("does not convert # mid-line", () => { + assert.equal(markdownToMrkdwn("Issue #123 is fixed"), "Issue #123 is fixed"); + }); + + // Code blocks + it("strips language hint from fenced code blocks", () => { + const input = "```javascript\nconsole.log('hi');\n```"; + const expected = "```\nconsole.log('hi');\n```"; + assert.equal(markdownToMrkdwn(input), expected); + }); + + it("preserves content inside fenced code blocks", () => { + const input = "```\n**not bold** [not a link](foo)\n```"; + assert.equal(markdownToMrkdwn(input), input); + }); + + it("preserves content inside code blocks with language hint", () => { + const input = "```sql\nSELECT **col** FROM tbl;\n```"; + const expected = "```\nSELECT **col** FROM tbl;\n```"; + assert.equal(markdownToMrkdwn(input), expected); + }); + + // Inline code + it("preserves inline code spans untouched", () => { + assert.equal( + markdownToMrkdwn("Use `**not bold**` in code"), + "Use `**not bold**` in code", + ); + }); + + it("does not transform markdown inside inline code", () => { + assert.equal( + markdownToMrkdwn("Run `rm -rf /` carefully and `[link](url)` there"), + "Run `rm -rf /` carefully and `[link](url)` there", + ); + }); + + // Horizontal rules + it("converts --- horizontal rule", () => { + assert.equal(markdownToMrkdwn("above\n---\nbelow"), "above\n─────────\nbelow"); + }); + + it("converts *** horizontal rule", () => { + assert.equal(markdownToMrkdwn("above\n***\nbelow"), "above\n─────────\nbelow"); + }); + + it("converts ___ horizontal rule", () => { + assert.equal(markdownToMrkdwn("above\n___\nbelow"), "above\n─────────\nbelow"); + }); + + // Pass-through (already valid mrkdwn) + it("leaves single *bold* unchanged", () => { + assert.equal(markdownToMrkdwn("This is *bold* already"), "This is *bold* already"); + }); + + it("leaves _italic_ unchanged", () => { + assert.equal(markdownToMrkdwn("This is _italic_ text"), "This is _italic_ text"); + }); + + it("leaves > blockquotes unchanged", () => { + assert.equal(markdownToMrkdwn("> quoted text"), "> quoted text"); + }); + + it("leaves bullet lists unchanged", () => { + assert.equal(markdownToMrkdwn("• item one\n• item two"), "• item one\n• item two"); + }); + + it("leaves - bullet lists unchanged", () => { + assert.equal(markdownToMrkdwn("- item one\n- item two"), "- item one\n- item two"); + }); + + it("leaves Slack-native links unchanged", () => { + assert.equal(markdownToMrkdwn(""), ""); + }); + + // Non-string input + it("converts non-string to string", () => { + assert.equal(markdownToMrkdwn(42), "42"); + assert.equal(markdownToMrkdwn(null), "null"); + }); + + // Complex real-world example (like the one from the issue) + it("handles a realistic agent message", () => { + const input = [ + "That's a much better approach. Authors are immutable, so the count only needs to increment.", + "", + "Right now the expensive queries are:", + "• **Persons**: person_identities → messages (COUNT DISTINCT on messages table)", + "• **Companies**: company_persons → person_identities → messages (even worse — 3-way join)", + "", + "With message_count on authors instead:", + "• **Persons**: person_identities → authors then SUM(authors.message_count)", + "• **Companies**: company_persons → person_identities → authors then SUM(...)", + "• **Write path is trivial**: just INCREMENT message_count", + "", + "I'll close both PRs (#2439 and #2440) and open a new one. Want me to go ahead?", + ].join("\n"); + + const output = markdownToMrkdwn(input); + + // **bold** should become *bold* + assert.ok(!output.includes("**Persons**")); + assert.ok(output.includes("*Persons*")); + assert.ok(!output.includes("**Companies**")); + assert.ok(output.includes("*Companies*")); + assert.ok(!output.includes("**Write path is trivial**")); + assert.ok(output.includes("*Write path is trivial*")); + + // Bullet structure and other text should be preserved + assert.ok(output.includes("• *Persons*: person_identities")); + assert.ok(output.includes("(#2439 and #2440)")); + }); + + // Mixed formatting + it("handles bold + link in same line", () => { + assert.equal( + markdownToMrkdwn("See **bold** and [link](https://x.com)"), + "See *bold* and ", + ); + }); + + it("handles multiple code blocks with surrounding markdown", () => { + const input = "**Title**\n```js\nconst x = 1;\n```\nSome **more** text\n```\nplain\n```"; + const output = markdownToMrkdwn(input); + assert.ok(output.startsWith("*Title*")); + assert.ok(output.includes("```\nconst x = 1;\n```")); + assert.ok(output.includes("Some *more* text")); + assert.ok(output.includes("```\nplain\n```")); + }); +}); + // ── sanitizeOutboundText ──────────────────────────────────────────────────── describe("sanitizeOutboundText", () => { From 84f7741c4ca5d878669dbd18c70b96e594705e11 Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Wed, 25 Feb 2026 10:51:39 -0500 Subject: [PATCH 2/3] bridge: add sanitizeOutboundText to legacy bridge outbound path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The legacy bridge (bridge.mjs) was missing outbound security sanitization that broker-bridge.mjs already had. Apply sanitizeOutboundText() before markdownToMrkdwn() on both /send and /reply, matching the broker-bridge pattern: block → return fallback text, otherwise sanitize then convert. --- slack-bridge/bridge.mjs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/slack-bridge/bridge.mjs b/slack-bridge/bridge.mjs index 5eb4f75..c557579 100644 --- a/slack-bridge/bridge.mjs +++ b/slack-bridge/bridge.mjs @@ -34,6 +34,7 @@ import { validateSendParams, validateReactParams, createRateLimiter, + sanitizeOutboundText, markdownToMrkdwn, } from "./security.mjs"; @@ -468,7 +469,8 @@ function startApiServer() { } const { channel, text, thread_ts } = apiRequestBody; - const safeText = markdownToMrkdwn(text); + const sanitized = sanitizeOutboundText(text); + const safeText = sanitized.blocked ? sanitized.text : markdownToMrkdwn(sanitized.text); const result = await app.client.chat.postMessage({ token: process.env.SLACK_BOT_TOKEN, channel, @@ -513,7 +515,8 @@ function startApiServer() { return; } - const safeText = markdownToMrkdwn(text); + const sanitized = sanitizeOutboundText(text); + const safeText = sanitized.blocked ? sanitized.text : markdownToMrkdwn(sanitized.text); const result = await app.client.chat.postMessage({ token: process.env.SLACK_BOT_TOKEN, channel: thread.channel, From 65b1b0444977dd86356165aee00c284ee65e4f0d Mon Sep 17 00:00:00 2001 From: Ben Vinegar Date: Wed, 25 Feb 2026 10:54:45 -0500 Subject: [PATCH 3/3] bridge: reorder to convert markdown before sanitization Sanitization should run last so it sees the exact text that will be sent to Slack. Markdown conversion can reshape content (e.g. a token inside a markdown link gets restructured), so running sanitization after conversion ensures nothing slips through. --- slack-bridge/bridge.mjs | 10 ++++++---- slack-bridge/broker-bridge.mjs | 13 +++++++------ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/slack-bridge/bridge.mjs b/slack-bridge/bridge.mjs index c557579..130ec71 100644 --- a/slack-bridge/bridge.mjs +++ b/slack-bridge/bridge.mjs @@ -469,8 +469,9 @@ function startApiServer() { } const { channel, text, thread_ts } = apiRequestBody; - const sanitized = sanitizeOutboundText(text); - const safeText = sanitized.blocked ? sanitized.text : markdownToMrkdwn(sanitized.text); + const converted = markdownToMrkdwn(text); + const sanitized = sanitizeOutboundText(converted); + const safeText = sanitized.text; const result = await app.client.chat.postMessage({ token: process.env.SLACK_BOT_TOKEN, channel, @@ -515,8 +516,9 @@ function startApiServer() { return; } - const sanitized = sanitizeOutboundText(text); - const safeText = sanitized.blocked ? sanitized.text : markdownToMrkdwn(sanitized.text); + const converted = markdownToMrkdwn(text); + const sanitized = sanitizeOutboundText(converted); + const safeText = sanitized.text; const result = await app.client.chat.postMessage({ token: process.env.SLACK_BOT_TOKEN, channel: thread.channel, diff --git a/slack-bridge/broker-bridge.mjs b/slack-bridge/broker-bridge.mjs index ddf2997..7b0526e 100755 --- a/slack-bridge/broker-bridge.mjs +++ b/slack-bridge/broker-bridge.mjs @@ -699,16 +699,17 @@ async function _react(channel, threadTs, emoji) { } function sanitizeOutboundMessage(text, contextLabel) { - const sanitized = sanitizeOutboundText(text); + // Convert Markdown → Slack mrkdwn first, then sanitize the final text. + // Sanitization runs last so it sees the exact text that will be sent to Slack + // (markdown conversion can reshape tokens, e.g. [text](xoxb-...) → ). + const converted = markdownToMrkdwn(text); + const sanitized = sanitizeOutboundText(converted); if (sanitized.blocked) { logWarn(`🛡️ outbound message blocked (${contextLabel}): ${sanitized.reasons.join(", ")}`); - return sanitized.text; - } - if (sanitized.redacted) { + } else if (sanitized.redacted) { logWarn(`🧼 outbound message redacted (${contextLabel}): ${sanitized.reasons.join(", ")}`); } - // Convert Markdown → Slack mrkdwn so agent output renders correctly. - return markdownToMrkdwn(sanitized.text); + return sanitized.text; } async function handleUserMessage(userMessage, event) {