fix(#288 review): markdown-safe-escape the untrusted page title in chat export

F1: pc.title (untrusted cross-user page title) was interpolated raw into the markdown export heading. Reusing escapeAttr alone (the prompt sink's XML-attribute sanitizer, strips < > ") is insufficient here because the sink is MARKDOWN: link /image syntax survives, so a title like ![x](http://evil) or [phish](http://evil) injects a remote image / clickable link into the downloaded .md disguised as a trusted system annotation. Add markdownHeadingSafe() = escapeAttr() + backslash- escape [ and ] (disables both [text](url) and ![text](url); a bare (url) is inert). F2: cover the title branch — a title that collapses to empty via escapeAttr falls to the bare heading (no ("")), and a link/image-injection title is neutralized (non-vacuous vs the escapeAttr-only version). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-02 15:46:44 +03:00
parent c39fab70c1
commit 438ef091f9
2 changed files with 154 additions and 2 deletions
@@ -307,6 +307,130 @@ describe('buildChatMarkdown (server) — structure', () => {
    );
  });

+  // #288 F1/F2: an empty page title must render the BARE heading with no
+  // `("…")` suffix (the `pc.title ? … : …` false branch).
+  it('renders the page-change heading with no title suffix when title is empty', () => {
+    const md = buildChatMarkdown({
+      title: 'T',
+      chatId: 'c',
+      rows: [
+        row({
+          role: 'assistant',
+          content: 'answer',
+          metadata: {
+            pageChanged: { title: '', diff: '@@ -1 +1 @@\n-old\n+new' },
+          } as never,
+        }),
+      ],
+    });
+    // Bare heading, single line, no parenthesized title.
+    expect(md).toContain(
+      '> **📝 The user edited this page before this turn; the diff the agent saw:**',
+    );
+    expect(md).not.toContain('("');
+    expect(md).toContain('-old');
+  });
+
+  // #288 F1: the page title is UNTRUSTED cross-user data, so a title carrying a
+  // newline / backtick / `"` / `<`/`>` must be neutralized by escapeAttr before
+  // it is interpolated into the `> **…**` blockquote heading — otherwise it
+  // could break the blockquote onto multiple lines or inject markup/HTML into
+  // the downloaded .md. escapeAttr strips `<>"` and collapses whitespace runs to
+  // a single space, so `Ev"il\n> `x` <b>` becomes ``Evil `x` b``.
+  it('escapes an untrusted page title in the page-change heading', () => {
+    const md = buildChatMarkdown({
+      title: 'T',
+      chatId: 'c',
+      rows: [
+        row({
+          role: 'assistant',
+          content: 'answer',
+          metadata: {
+            pageChanged: {
+              title: 'Ev"il\n> `x` <b>',
+              diff: '@@ -1 +1 @@\n-old\n+new',
+            },
+          } as never,
+        }),
+      ],
+    });
+    // The heading stays a single blockquote line with the escaped title.
+    expect(md).toContain(
+      '> **📝 The user edited this page before this turn; the diff the agent saw: ("Evil `x` b")**',
+    );
+    // No raw attribute/markup breakers survived from the title.
+    expect(md).not.toContain('Ev"il');
+    expect(md).not.toContain('<b>');
+  });
+
+  // #288 review F1: escapeAttr ALONE is insufficient for this MARKDOWN sink —
+  // link/image syntax survives it. A cross-user title with `![x](url)` /
+  // `[phish](url)` must NOT become a working remote image or clickable link in
+  // the downloaded .md; markdownHeadingSafe backslash-escapes `[`/`]` so both are
+  // inert. (Non-vacuous: fails against the escapeAttr-only version, which left
+  // `](https://` intact.)
+  it('neutralizes markdown link/image syntax in an untrusted page title', () => {
+    const md = buildChatMarkdown({
+      title: 'T',
+      chatId: 'c',
+      rows: [
+        row({
+          role: 'assistant',
+          content: 'answer',
+          metadata: {
+            pageChanged: {
+              title:
+                '![x](https://attacker.example/t.png) and [click](https://phish.example)',
+              diff: '@@ -1 +1 @@\n-old\n+new',
+            },
+          } as never,
+        }),
+      ],
+    });
+    // No WORKING image/link syntax survives — the `[…]` sits escaped as `\[…\]`,
+    // so the unescaped `![x](` image and `[click](` link markers are gone. (We
+    // deliberately do NOT assert `not.toContain('](https://')`: after escaping the
+    // literal `\](https://` still contains `](https://` as a raw substring — that
+    // check would false-fail even though the link is inert.)
+    expect(md).not.toContain('![x](');
+    expect(md).not.toContain('[click](');
+    // The brackets are backslash-escaped, so `[text](url)`/`![text](url)` are inert.
+    expect(md).toContain('\\[');
+    expect(md).toContain('\\]');
+    // The heading stays a SINGLE blockquote line (no newline injected).
+    const headingLine = md
+      .split('\n')
+      .find((l) => l.includes('the diff the agent saw:'));
+    expect(headingLine).toBeDefined();
+    expect(headingLine).toContain('\\[x\\]');
+    expect(headingLine).toContain('\\[click\\]');
+  });
+
+  // #288 internal review Finding 2: a NON-empty title made up entirely of
+  // escapeAttr breakers (`<>"`) escapes to '' — the ternary must then fall to the
+  // BARE heading with NO `("…")` suffix. Locks the ternary-on-escaped-value
+  // behavior (distinct from the empty-string input test above).
+  it('renders the bare heading for a title that escapes to empty', () => {
+    const md = buildChatMarkdown({
+      title: 'T',
+      chatId: 'c',
+      rows: [
+        row({
+          role: 'assistant',
+          content: 'answer',
+          metadata: {
+            pageChanged: { title: '<>"', diff: '@@ -1 +1 @@\n-old\n+new' },
+          } as never,
+        }),
+      ],
+    });
+    expect(md).toContain(
+      '> **📝 The user edited this page before this turn; the diff the agent saw:**',
+    );
+    expect(md).not.toContain('("');
+    expect(md).toContain('-old');
+  });
+
  it('escapes embedded triple-backtick fences with a longer delimiter', () => {
    const md = buildChatMarkdown({
      title: 'T',
@@ -15,6 +15,7 @@
 */

 import type { AiChatMessage } from '@docmost/db/types/entity.types';
+import { escapeAttr } from './ai-chat.prompt';

 /** Supported export label languages. Defaults to English. */
 export type ExportLang = 'en' | 'ru';
@@ -110,6 +111,24 @@ const LABELS: Record<
  },
 };

+/**
+ * Make an untrusted title safe to interpolate into a Markdown blockquote
+ * HEADING. escapeAttr() neutralizes the XML/HTML breakers (`<` `>` `"`) and
+ * collapses whitespace for the PROMPT sink (`page="…"`), but this export sink is
+ * MARKDOWN — link/image syntax survives escapeAttr. So additionally backslash-
+ * escape `[` and `]`: that disables both `[text](url)` links and `![text](url)`
+ * images, so a cross-user title like `![x](http://evil)` or `[phish](http://evil)`
+ * cannot inject a remote (auto-loading) image or a clickable link into the
+ * downloaded .md disguised as a trusted system annotation. A bare `(url)` with no
+ * preceding `[]` is inert Markdown, so brackets are the only security-critical
+ * characters here. (We leave backticks to escapeAttr's whitespace pass — a title
+ * shown as inline code cannot escape the blockquote line or load a resource, so
+ * it is not a security concern for this sink.)
+ */
+function markdownHeadingSafe(title: string): string {
+  return escapeAttr(title).replace(/[[\]]/g, (m) => `\\${m}`);
+}
+
 /** True for AI SDK tool parts (static `tool-*` or `dynamic-tool`). */
 function isToolPart(type: string): boolean {
  return type.startsWith('tool-') || type === 'dynamic-tool';
@@ -293,8 +312,17 @@ export function buildChatMarkdown(args: {
    // warning the model received.
    const pc = pageChangedOf(row);
    if (pc) {
-      const heading = pc.title
-        ? `${L.pageEditedByUser} ("${pc.title}")`
+      // The page title is UNTRUSTED cross-user data (a collaborative page's title
+      // controllable by another user). escapeAttr() alone (the prompt sink) is
+      // INSUFFICIENT here: this is a MARKDOWN sink, so we neutralize link/image
+      // syntax too (backslash-escaping `[`/`]`) before interpolating it into this
+      // `> **…**` blockquote heading — otherwise `![x](url)` / `[phish](url)` would
+      // inject a remote image or clickable link into the downloaded .md. An
+      // all-`<>"` title escapes to empty and correctly falls to the bare heading.
+      // The diff body is already safe via fence(). (#288 review F1.)
+      const safeTitle = markdownHeadingSafe(pc.title);
+      const heading = safeTitle
+        ? `${L.pageEditedByUser} ("${safeTitle}")`
        : L.pageEditedByUser;
      blocks.push(`> **📝 ${heading}**\n\n${fence(pc.diff, 'diff')}`);
    }