refactor(markdown): single canonical sanitize schema for both renderers (MUL-4922)

packages/ui/markdown/Markdown.tsx

@@ -1,8 +1,8 @@
 import * as React from 'react'
-import ReactMarkdown, { type Components, defaultUrlTransform } from 'react-markdown'
+import ReactMarkdown, { type Components } from 'react-markdown'
 import rehypeKatex from 'rehype-katex'
 import rehypeRaw from 'rehype-raw'
-import rehypeSanitize, { defaultSchema } from 'rehype-sanitize'
+import rehypeSanitize from 'rehype-sanitize'
 import remarkBreaks from 'remark-breaks'
 import remarkGfm from 'remark-gfm'
 import remarkMath from 'remark-math'
@@ -14,6 +14,7 @@ import { isAllowedFileCardHref, preprocessFileCards } from './file-cards'
 import { preprocessLinks } from './linkify'
 import { preprocessIssueIdentifiers } from './issue-identifiers'
 import { preprocessMentionShortcodes } from './mentions'
+import { markdownSanitizeSchema, markdownUrlTransform } from './sanitize'
 import 'katex/dist/katex.min.css'
 import './markdown.css'
 
@@ -87,64 +88,6 @@ export interface MarkdownProps {
   autolinkIssueIdentifiers?: boolean
 }
 
-// Sanitization schema — extends GitHub defaults to allow code highlighting classes
-// and Multica's internal mention/slash protocols.
-const sanitizeSchema = {
-  ...defaultSchema,
-  protocols: {
-    ...defaultSchema.protocols,
-    href: [...(defaultSchema.protocols?.href ?? []), 'mention', 'slash'],
-    // Permit inline data-URI images (QR codes, charts, base64 screenshots).
-    // The scheme gate only allows `data:` through here; attributes.img below
-    // narrows it to image/* so non-image data URIs are still rejected.
-    src: [...(defaultSchema.protocols?.src ?? []), 'data'],
-  },
-  attributes: {
-    ...defaultSchema.attributes,
-    div: [
-      ...(defaultSchema.attributes?.div ?? []),
-      'dataType',
-      'dataHref',
-      'dataFilename',
-    ],
-    code: [
-      ...(defaultSchema.attributes?.code ?? []),
-      ['className', /^language-/],
-      ['className', /^math-/],
-      ['className', /^hljs/],
-    ],
-    img: [
-      // Drop the default plain `src` entry so the value allow-list below is the
-      // one findDefinition resolves — it returns the first match by name, so a
-      // bare `src` string would otherwise shadow (and disable) the allow-list.
-      ...(defaultSchema.attributes?.img ?? []).filter(
-        (attr) => (typeof attr === 'string' ? attr : attr[0]) !== 'src',
-      ),
-      'alt',
-      // Allow inline data:image/* URIs while leaving every other src form
-      // (http/https/site-relative) exactly as before: the negative lookahead
-      // keeps all non-data values, and data: is narrowed to images only.
-      ['src', /^data:image\//i, /^(?!data:)/i],
-    ],
-  },
-}
-
-/**
- * Custom URL transform that allows Multica internal protocols while keeping
- * the default security for all other URLs.
- */
-function urlTransform(url: string): string {
-  if (url.startsWith('mention://')) return url
-  if (url.startsWith('slash://skill/')) return url
-  // Allow inline data:image/* URIs — defaultUrlTransform strips every data: URL
-  // to '', which would blank the src even after rehype-sanitize keeps it. Kept
-  // in sync with the image/* narrowing in sanitizeSchema (protocols.src +
-  // attributes.img) so both gates agree on what a valid inline image is.
-  if (/^data:image\//i.test(url)) return url
-  return defaultUrlTransform(url)
-}
-
-
 // File path detection regex - matches paths starting with /, ~/, or ./
 const FILE_PATH_REGEX =
   /^(?:\/|~\/|\.\/)[\w\-./@]+\.(?:ts|tsx|js|jsx|mjs|cjs|md|json|yaml|yml|py|go|rs|css|scss|less|html|htm|txt|log|sh|bash|zsh|swift|kt|java|c|cpp|h|hpp|rb|php|xml|toml|ini|cfg|conf|env|sql|graphql|vue|svelte|astro|prisma)$/i
@@ -483,8 +426,8 @@ export function Markdown({
           remarkBreaks,
           [remarkGfm, { singleTilde: false }],
         ]}
-        rehypePlugins={[rehypeRaw, [rehypeSanitize, sanitizeSchema], rehypeKatex]}
-        urlTransform={urlTransform}
+        rehypePlugins={[rehypeRaw, [rehypeSanitize, markdownSanitizeSchema], rehypeKatex]}
+        urlTransform={markdownUrlTransform}
         components={components}
       >
         {processedContent}

packages/ui/markdown/index.ts

@@ -8,6 +8,7 @@ export {
   ISSUE_IDENTIFIER_PATTERN,
 } from './issue-identifiers'
 export { preprocessMentionShortcodes } from './mentions'
+export { markdownSanitizeSchema, markdownUrlTransform } from './sanitize'
 export {
   preprocessFileCards,
   isCdnUrl,

packages/ui/markdown/sanitize.ts

@@ -0,0 +1,75 @@
+import { defaultUrlTransform } from 'react-markdown'
+import { defaultSchema, type Options } from 'rehype-sanitize'
+
+/**
+ * Canonical sanitize schema + URL transform for every product-level Markdown
+ * renderer (Chat via ui/markdown/Markdown.tsx, Issue/Comment via
+ * views/editor/readonly-content.tsx).
+ *
+ * This is the ONLY copy. Both renderers previously carried a verbatim fork of
+ * this schema, which had already drifted — the readonly fork whitelisted <mark>
+ * for `==highlight==` and the chat fork did not. A security-relevant allow-list
+ * maintained in two places means any future XSS fix has to land twice, and
+ * missing one is a hole. Adding a surface means importing this, never copying
+ * it (MUL-4922).
+ *
+ * The two gates below must agree on what a valid inline image is:
+ * `protocols.src` + `attributes.img` (sanitize) and `markdownUrlTransform`
+ * (react-markdown). Change them together.
+ */
+export const markdownSanitizeSchema: Options = {
+  ...defaultSchema,
+  // Allow <mark> (text highlight) — emitted by highlightToHtml from `==text==`.
+  // It carries no attributes, so only the tag name needs whitelisting.
+  tagNames: [...(defaultSchema.tagNames ?? []), 'mark'],
+  protocols: {
+    ...defaultSchema.protocols,
+    href: [...(defaultSchema.protocols?.href ?? []), 'mention', 'slash'],
+    // Permit inline data-URI images (QR codes, charts, base64 screenshots).
+    // The scheme gate only allows `data:` through here; attributes.img below
+    // narrows it to image/* so non-image data URIs are still rejected.
+    src: [...(defaultSchema.protocols?.src ?? []), 'data'],
+  },
+  attributes: {
+    ...defaultSchema.attributes,
+    div: [
+      ...(defaultSchema.attributes?.div ?? []),
+      'dataType',
+      'dataHref',
+      'dataFilename',
+    ],
+    code: [
+      ...(defaultSchema.attributes?.code ?? []),
+      ['className', /^language-/],
+      ['className', /^math-/],
+      ['className', /^hljs/],
+    ],
+    img: [
+      // Drop the default plain `src` entry so the value allow-list below is the
+      // one findDefinition resolves — it returns the first match by name, so a
+      // bare `src` string would otherwise shadow (and disable) the allow-list.
+      ...(defaultSchema.attributes?.img ?? []).filter(
+        (attr) => (typeof attr === 'string' ? attr : attr[0]) !== 'src',
+      ),
+      'alt',
+      // Allow inline data:image/* URIs while leaving every other src form
+      // (http/https/site-relative) exactly as before: the negative lookahead
+      // keeps all non-data values, and data: is narrowed to images only.
+      ['src', /^data:image\//i, /^(?!data:)/i],
+    ],
+  },
+}
+
+/**
+ * Allows Multica internal protocols and inline images through react-markdown's
+ * URL gate while keeping default security for everything else.
+ */
+export function markdownUrlTransform(url: string): string {
+  if (url.startsWith('mention://')) return url
+  if (url.startsWith('slash://skill/')) return url
+  // defaultUrlTransform strips every data: URL to '', which would blank the src
+  // even after rehype-sanitize keeps it. Kept in sync with the image/* narrowing
+  // in markdownSanitizeSchema so both gates agree on what a valid inline image is.
+  if (/^data:image\//i.test(url)) return url
+  return defaultUrlTransform(url)
+}

packages/views/common/rich-content-sanitize-contract.test.tsx

@@ -0,0 +1,184 @@
+/**
+ * Cross-surface sanitize contract (MUL-4922).
+ *
+ * The two product-level Markdown chains — Chat (ui/markdown/Markdown.tsx) and
+ * Issue/Comment (views/editor/readonly-content.tsx) — used to carry a verbatim
+ * fork of the sanitize schema and urlTransform each, and had already drifted:
+ * readonly whitelisted <mark>, chat did not. Both now import the single
+ * canonical base from @multica/ui/markdown.
+ *
+ * This suite runs one set of security fixtures through BOTH surfaces and
+ * asserts the same outcome. It is the mechanism that stops a third fork from
+ * growing back: a schema change that lands on only one chain fails here.
+ */
+import { describe, expect, it, vi } from "vitest";
+import { render } from "@testing-library/react";
+import {
+  Markdown as MarkdownBase,
+  markdownSanitizeSchema,
+} from "@multica/ui/markdown";
+
+vi.mock("@multica/core/config", () => ({
+  useConfigStore: (selector: (state: { cdnDomain: string }) => unknown) =>
+    selector({ cdnDomain: "" }),
+  configStore: { getState: () => ({ cdnDomain: "" }) },
+}));
+
+vi.mock("../issues/hooks", () => ({
+  useResolveIssueIdentifier: () => null,
+}));
+
+vi.mock("../i18n", async () => {
+  const editor = (await import("../locales/en/editor.json")).default;
+  return {
+    useT: () => ({
+      t: (select: (bundle: typeof editor) => string) => select(editor),
+    }),
+    useTimeAgo: () => "just now",
+  };
+});
+
+vi.mock("@multica/core/api", () => ({
+  api: { getAttachmentTextContent: vi.fn() },
+  PreviewTooLargeError: class extends Error {},
+  PreviewUnsupportedError: class extends Error {},
+}));
+
+vi.mock("@multica/core/paths", () => ({
+  useWorkspacePaths: () => ({
+    issueDetail: (id: string) => `/test/issues/${id}`,
+    projectDetail: (id: string) => `/test/projects/${id}`,
+  }),
+  useWorkspaceSlug: () => "test",
+}));
+
+vi.mock("../navigation", () => ({
+  useNavigation: () => ({ push: vi.fn(), openInNewTab: vi.fn() }),
+  AppLink: ({ href, children }: { href: string; children: React.ReactNode }) => (
+    <a href={href}>{children}</a>
+  ),
+}));
+
+vi.mock("../issues/components/issue-mention-card", () => ({
+  IssueMentionCard: ({ issueId }: { issueId: string }) => <span>{issueId}</span>,
+}));
+
+vi.mock("../projects/components/project-chip", () => ({
+  ProjectChip: ({ projectId }: { projectId: string }) => <span>{projectId}</span>,
+}));
+
+vi.mock("../editor/link-hover-card", () => ({
+  useLinkHover: () => ({}),
+  LinkHoverCard: () => null,
+}));
+
+vi.mock("../editor/utils/link-handler", () => ({
+  openLink: vi.fn(),
+  isMentionHref: (href?: string) => Boolean(href?.startsWith("mention://")),
+}));
+
+// Expose the sanitized url/filename that survived the schema, so image
+// fixtures can be asserted identically on both surfaces (readonly routes
+// images through <Attachment>, the ui base renders a plain <img>).
+vi.mock("../editor/attachment", () => ({
+  Attachment: ({
+    attachment,
+  }: {
+    attachment: { url: string; filename: string };
+  }) => <img src={attachment.url} alt={attachment.filename} />,
+}));
+
+import { ReadonlyContent } from "../editor/readonly-content";
+
+const PNG_1X1 =
+  "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==";
+
+const SURFACES: ReadonlyArray<{
+  name: string;
+  render: (markdown: string) => HTMLElement;
+}> = [
+  {
+    name: "Chat (ui/markdown)",
+    render: (markdown) => render(<MarkdownBase>{markdown}</MarkdownBase>).container,
+  },
+  {
+    name: "Issue/Comment (ReadonlyContent)",
+    render: (markdown) => render(<ReadonlyContent content={markdown} />).container,
+  },
+];
+
+describe.each(SURFACES)("sanitize contract — $name", ({ render: renderSurface }) => {
+  it("strips <script>", () => {
+    const container = renderSurface("hi\n\n<script>alert(1)</script>");
+
+    expect(container.querySelector("script")).toBeNull();
+    expect(container.textContent).not.toContain("alert(1)");
+  });
+
+  it("strips event-handler attributes from raw HTML", () => {
+    const container = renderSurface('<img src="x" onerror="alert(1)">');
+
+    expect(container.querySelector("[onerror]")).toBeNull();
+    expect(container.innerHTML).not.toContain("onerror");
+  });
+
+  it("neutralizes javascript: hrefs", () => {
+    const container = renderSurface("[click](javascript:alert(1))");
+
+    const href = container.querySelector("a")?.getAttribute("href") ?? "";
+    expect(href.toLowerCase()).not.toContain("javascript:");
+  });
+
+  it("preserves an inline data:image/* src", () => {
+    const container = renderSurface(`![demo](${PNG_1X1})`);
+
+    expect(container.querySelector("img")).toHaveAttribute("src", PNG_1X1);
+  });
+
+  it("blanks a non-image data: URI", () => {
+    const container = renderSurface("![x](data:text/html,<script>alert(1)</script>)");
+
+    expect(container.querySelector("img")?.getAttribute("src") ?? "").toBe("");
+  });
+
+  it("keeps http(s) image src intact", () => {
+    const container = renderSurface("![cat](https://cdn.example.com/cat.png)");
+
+    expect(container.querySelector("img")).toHaveAttribute(
+      "src",
+      "https://cdn.example.com/cat.png",
+    );
+  });
+
+  // The drift this sweep closes: <mark> was whitelisted on readonly only, so
+  // the same content highlighted in a comment and rendered in chat disagreed.
+  it("allows <mark> (==highlight== lowering target)", () => {
+    const container = renderSurface("<mark>hi</mark>");
+
+    expect(container.querySelector("mark")?.textContent).toBe("hi");
+  });
+
+  it("allows the slash:// protocol", () => {
+    const container = renderSurface("[/deploy](slash://skill/abc-123)");
+
+    expect(container.querySelector(".slash-command")?.textContent).toBe("/deploy");
+  });
+});
+
+// Code-block *rendering* is deliberately not asserted cross-surface yet: chat
+// highlights with Shiki and readonly with lowlight, so the emitted class tokens
+// still differ. Converging them is the RichCodeBlock phase of MUL-4922; until
+// the highlight engine is picked, only the schema-level allow-list is shared.
+describe("canonical sanitize schema", () => {
+  it("permits language-/math-/hljs class tokens on <code>", () => {
+    const codeAttrs = markdownSanitizeSchema.attributes?.code ?? [];
+    const patterns = codeAttrs
+      .filter((attr): attr is [string, RegExp] => Array.isArray(attr))
+      .filter(([name]) => name === "className")
+      .map(([, pattern]) => pattern.source);
+
+    expect(patterns).toEqual(
+      expect.arrayContaining(["^language-", "^math-", "^hljs"]),
+    );
+  });
+});