From 438ef091f966d0d8d9708952877f685ff3a4835b Mon Sep 17 00:00:00 2001 From: agent_coder Date: Thu, 2 Jul 2026 15:46:44 +0300 Subject: [PATCH] fix(#288 review): markdown-safe-escape the untrusted page title in chat export MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../core/ai-chat/chat-markdown.util.spec.ts | 124 ++++++++++++++++++ .../src/core/ai-chat/chat-markdown.util.ts | 32 ++++- 2 files changed, 154 insertions(+), 2 deletions(-) diff --git a/apps/server/src/core/ai-chat/chat-markdown.util.spec.ts b/apps/server/src/core/ai-chat/chat-markdown.util.spec.ts index 105ecdaf..e53b5dfa 100644 --- a/apps/server/src/core/ai-chat/chat-markdown.util.spec.ts +++ b/apps/server/src/core/ai-chat/chat-markdown.util.spec.ts @@ -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` ` 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` ', + 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(''); + }); + + // #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', diff --git a/apps/server/src/core/ai-chat/chat-markdown.util.ts b/apps/server/src/core/ai-chat/chat-markdown.util.ts index 7b69b765..1888f781 100644 --- a/apps/server/src/core/ai-chat/chat-markdown.util.ts +++ b/apps/server/src/core/ai-chat/chat-markdown.util.ts @@ -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')}`); }