Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions slack-bridge/bridge.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import {
validateSendParams,
validateReactParams,
createRateLimiter,
sanitizeOutboundText,
markdownToMrkdwn,
} from "./security.mjs";

// ── Config ──────────────────────────────────────────────────────────────────
Expand Down Expand Up @@ -467,14 +469,17 @@ function startApiServer() {
}

const { channel, text, thread_ts } = apiRequestBody;
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,
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) {
Expand Down Expand Up @@ -511,14 +516,17 @@ function startApiServer() {
return;
}

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,
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);
Expand Down
7 changes: 6 additions & 1 deletion slack-bridge/broker-bridge.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
validateReactParams,
createRateLimiter,
sanitizeOutboundText,
markdownToMrkdwn,
} from "./security.mjs";
import {
canonicalizeEnvelope,
Expand Down Expand Up @@ -698,7 +699,11 @@ 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-...) → <xoxb-...|text>).
const converted = markdownToMrkdwn(text);
const sanitized = sanitizeOutboundText(converted);
if (sanitized.blocked) {
logWarn(`🛡️ outbound message blocked (${contextLabel}): ${sanitized.reasons.join(", ")}`);
} else if (sanitized.redacted) {
Expand Down
79 changes: 79 additions & 0 deletions slack-bridge/security.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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) → <url|text>
* - ![alt](url) → <url|alt> (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) → <url|alt> (must come before link conversion)
processed = processed.replace(/!\[([^\]]*)\]\(([^)]+)\)/g, (_, alt, url) => {
return alt ? `<${url}|${alt}>` : `<${url}>`;
});

// Links: [text](url) → <url|text>
processed = processed.replace(/\[([^\]]+)\]\(([^)]+)\)/g, (_, linkText, url) => {
Copy link

Choose a reason for hiding this comment

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

Bug: The regex for converting markdown links incorrectly parses URLs that contain parentheses, leading to broken links in Slack messages.
Severity: MEDIUM

Suggested Fix

Update the link conversion regex to correctly handle parentheses within the URL part. A common approach is to allow for balanced parentheses within the URL capture group or to make the text part non-greedy and capture until the final parenthesis of the markdown link syntax.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: slack-bridge/security.mjs#L307

Potential issue: The regular expression `/\s*-\s*\[([xX\s])\]/g` used to convert
markdown links to Slack's `mrkdwn` format is flawed. The URL capture group `([^)]+)`
greedily matches characters until it encounters the first closing parenthesis. This
causes incorrect parsing for URLs that legitimately contain parentheses, such as many
Wikipedia links (e.g., `https://en.wikipedia.org/wiki/Set_(mathematics))`). The
resulting link in Slack will be malformed and broken, degrading the user experience when
an agent provides such a link.

Did we get this right? 👍 / 👎 to inform future reviews.

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 ───────────────────────────────────────────────────

/**
Expand Down
192 changes: 192 additions & 0 deletions slack-bridge/security.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
isAllowed,
cleanMessage,
formatForSlack,
markdownToMrkdwn,
validateSendParams,
validateReactParams,
safeEqualSecret,
Expand Down Expand Up @@ -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 <url|text>", () => {
assert.equal(
markdownToMrkdwn("See [the docs](https://example.com) here"),
"See <https://example.com|the docs> here",
);
});

it("converts image ![alt](url) to <url|alt>", () => {
assert.equal(
markdownToMrkdwn("Check ![screenshot](https://img.example.com/pic.png)"),
"Check <https://img.example.com/pic.png|screenshot>",
);
});

it("converts image with empty alt to bare URL", () => {
assert.equal(
markdownToMrkdwn("![](https://img.example.com/pic.png)"),
"<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("<https://example.com|click>"), "<https://example.com|click>");
});

// 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 <https://x.com|link>",
);
});

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", () => {
Expand Down