diff --git a/CHANGELOG.md b/CHANGELOG.md index 083eb49e..e700f832 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -347,10 +347,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 as XML-entity-escaped mxfile XML — draw.io's own native form, decoded by the DOM as UTF-8 — so labels open intact. The decoder reads both the new entity-encoded form and the old base64 form, so existing diagrams still open. - *Healing pre-fix diagrams:* any agent-created cyrillic diagram written before - this fix is repaired in place by `drawioGet` → `drawioUpdate` with the same - XML (rewrites the attachment in the new form); no migration script is needed. - (#507) + *Healing pre-fix diagrams:* only a diagram that still holds its original + (correct-UTF-8) base64 — i.e. one not yet opened/autosaved in the draw.io + editor — can be repaired in place by `drawioGet` → `drawioUpdate` with the + same XML (rewrites the attachment in the new form); no migration script is + needed. A diagram that was already opened in the editor persisted the + mojibake at rest, so `drawioGet` reads the already-corrupted text and + `drawioUpdate` faithfully rewrites it — that text is lost and is not + recoverable by a rewrite. (#507) - **A chat with one malformed message part no longer 500s on every turn, and a failed send no longer duplicates the user's message.** Incoming client parts are now whitelisted to `text` (a forged tool-result part can no longer reach diff --git a/apps/server/src/integrations/import/services/import-attachment.service.drawio-svg.spec.ts b/apps/server/src/integrations/import/services/import-attachment.service.drawio-svg.spec.ts index 00735ea2..ebe67431 100644 --- a/apps/server/src/integrations/import/services/import-attachment.service.drawio-svg.spec.ts +++ b/apps/server/src/integrations/import/services/import-attachment.service.drawio-svg.spec.ts @@ -76,6 +76,41 @@ describe('ImportAttachmentService.createDrawioSvg (#507)', () => { expect(decoded).toBe(drawio); }); + it('encodes literal tab/newline/CR as numeric char-refs, not literal control chars (#507 F1)', async () => { + // A literal tab/newline/CR inside the mxfile XML would be collapsed to a + // single space by XML attribute-value normalization when the draw.io editor + // reads content=, silently flattening multi-line labels and tab-bearing + // values. They must be emitted as numeric char-refs instead. + const drawio = + '' + + '' + + '' + + '' + + ''; + const p = await writeDrawio('ctrl.drawio', drawio); + + const svg = (await call(p)).toString('utf-8'); + const content = /content="([^"]*)"/.exec(svg)?.[1]; + expect(content).toBeDefined(); + // No literal control chars survive in the attribute value. + expect(content).not.toMatch(/[\t\n\r]/); + // They round-trip as numeric char-refs. + expect(content).toContain(' '); + expect(content).toContain(' '); + expect(content).toContain(' '); + // Decoding (char-refs back to literal, entities back) recovers the file. + const decoded = content! + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, "'") + .replace(/ /gi, '\t') + .replace(/ /gi, '\n') + .replace(/ /gi, '\r') + .replace(/&/g, '&'); + expect(decoded).toBe(drawio); + }); + it('escapes XML metacharacters in the drawio payload', async () => { const drawio = '"q" <x>'; const p = await writeDrawio('meta.drawio', drawio); diff --git a/apps/server/src/integrations/import/services/import-attachment.service.ts b/apps/server/src/integrations/import/services/import-attachment.service.ts index 1b47ce4e..3fa94838 100644 --- a/apps/server/src/integrations/import/services/import-attachment.service.ts +++ b/apps/server/src/integrations/import/services/import-attachment.service.ts @@ -8,6 +8,7 @@ import { createReadStream } from 'node:fs'; import { promises as fs } from 'fs'; import { Readable } from 'stream'; import { getMimeType, sanitizeFileName } from '../../../common/helpers'; +import { htmlEscape } from '../../../common/helpers/html-escaper'; import { v7 } from 'uuid'; import { FileTask } from '@docmost/db/types/entity.types'; import { getAttachmentFolderPath } from '../../../core/attachment/attachment.utils'; @@ -854,7 +855,7 @@ export class ImportAttachmentService { // (no UTF-8 step), turning every non-ASCII char (Cyrillic, ё, —) into // mojibake; the entity-encoded form is decoded by the DOM as UTF-8 and // opens intact. Docmost's own decoder reads both forms. - const drawioEscaped = this.xmlEscapeAttr(drawioContent); + const drawioEscaped = this.xmlEscapeContent(drawioContent); let imageElement = ''; // If we have a PNG, include it in the SVG @@ -891,14 +892,20 @@ export class ImportAttachmentService { /** * Escape a string so it is safe as the value of a double-quoted XML attribute - * (the `content=` payload of a `.drawio.svg`). Order matters: `&` first. + * (the `content=` payload of a `.drawio.svg`). The shared `htmlEscape` covers + * `& < > " '` (a strict superset of what this attribute needs; the extra `'` + * escape is harmless in a `"`-delimited value). On top of that, the numeric + * char-refs for tab/newline/CR are required: a literal tab/newline/CR inside + * an attribute value is collapsed to a single space by XML attribute-value + * normalization on DOM read (both our decoder and the real draw.io editor), + * silently flattening multi-line labels and tab-bearing values. Char-refs + * survive that normalization (#507). */ - private xmlEscapeAttr(s: string): string { - return s - .replace(/&/g, '&') - .replace(//g, '>') - .replace(/"/g, '"'); + private xmlEscapeContent(s: string): string { + return htmlEscape(s) + .replace(/\t/g, ' ') + .replace(/\n/g, ' ') + .replace(/\r/g, ' '); } private async uploadWithRetry(opts: { diff --git a/packages/mcp/src/lib/drawio-xml.ts b/packages/mcp/src/lib/drawio-xml.ts index f0a7e526..620b12ea 100644 --- a/packages/mcp/src/lib/drawio-xml.ts +++ b/packages/mcp/src/lib/drawio-xml.ts @@ -196,12 +196,23 @@ export function extractContentAttr(svg: string): string { // value itself never contains a double-quote (base64 / entity-encoded XML). const m = /content="([^"]*)"/.exec(svg); if (m) { - // Decode the handful of XML entities a raw regex would leave encoded. + // Decode the handful of XML entities a raw regex would leave encoded. The + // numeric char-refs for tab/newline/CR MUST be decoded here too: the DOM + // path above turns them back into the literal control chars, so this + // regex fallback has to agree or the two decode paths diverge (#507). + // `&` is decoded last so an escaped `&#x9;` reads back as the + // literal text ` `, not a tab. return m[1] .replace(/</g, "<") .replace(/>/g, ">") .replace(/"/g, '"') .replace(/'/g, "'") + .replace(/ /gi, "\t") + .replace(/ /g, "\t") + .replace(/ /gi, "\n") + .replace(/ /g, "\n") + .replace(/ /gi, "\r") + .replace(/ /g, "\r") .replace(/&/g, "&"); } throw new Error("drawio: SVG has no content= attribute to decode"); @@ -342,7 +353,15 @@ function xmlEscape(s: string): string { .replace(/&/g, "&") .replace(//g, ">") - .replace(/"/g, """); + .replace(/"/g, """) + // A literal tab/newline/CR inside an attribute value is collapsed to a + // single space by XML attribute-value normalization on DOM read (both jsdom + // here and the real draw.io editor), silently flattening multi-line labels + // and tab-bearing values. Numeric char-refs survive that normalization, so + // emit them the way draw.io's own native export does (#507). + .replace(/\t/g, " ") + .replace(/\n/g, " ") + .replace(/\r/g, " "); } // --- normalization + hash -------------------------------------------------- diff --git a/packages/mcp/test/unit/drawio-xml.test.mjs b/packages/mcp/test/unit/drawio-xml.test.mjs index e5e5f746..3295b0f8 100644 --- a/packages/mcp/test/unit/drawio-xml.test.mjs +++ b/packages/mcp/test/unit/drawio-xml.test.mjs @@ -435,3 +435,47 @@ test("#507 negative: non-ASCII in a cell value AND in the title round-trip clean assert.ok(nameMatch, "diagram name attribute present"); assert.equal(unescapeAttr(nameMatch[1]), title); }); + +// --- #507 F1: literal tab/newline/CR in an attribute value survive the +// content= round-trip. The whole mxfile XML lives in one content="..." attr; +// XML attribute-value normalization collapses a LITERAL tab/newline/CR to a +// single space on DOM read, so the escape must emit numeric char-refs instead. +const CTRL_MODEL = + "" + + '' + + '' + + '' + + '' + + '' + + ""; + +test("#507 F1: literal tab/newline/CR in a value round-trip byte-stable (DOM decode)", () => { + const svg = buildDrawioSvg(CTRL_MODEL, "", { width: 200, height: 200 }); + const content = /content="([^"]*)"/.exec(svg)[1]; + // The tab/newline/CR must be emitted as numeric char-refs, never as literal + // control chars (which DOM attribute-value normalization would eat). + assert.ok( + !/[\t\n\r]/.test(content), + "no literal tab/newline/CR survive in the content= attribute", + ); + assert.ok( + content.includes(" ") && + content.includes(" ") && + content.includes(" "), + "tab/newline/CR are emitted as numeric char-refs", + ); + // Full DOM decode recovers the model byte-for-byte, control chars intact. + assert.equal(decodeDrawioSvg(svg), CTRL_MODEL); +}); + +test("#507 F1: regex fallback decodes tab/newline/CR char-refs (agrees with DOM path)", () => { + const svg = buildDrawioSvg(CTRL_MODEL, "", { width: 200, height: 200 }); + const content = /content="([^"]*)"/.exec(svg)[1]; + // A bare `&` makes the SVG wrapper malformed, forcing extractContentAttr onto + // its regex fallback branch. That branch must decode the tab/newline/CR + // char-refs exactly like the DOM path, or the two decoders diverge. + const malformedSvg = `&`; + assert.equal(decodeDrawioSvg(malformedSvg), CTRL_MODEL); + // The well-formed (DOM) path yields the identical result. + assert.equal(decodeDrawioSvg(svg), CTRL_MODEL); +});