diff --git a/CHANGELOG.md b/CHANGELOG.md index e33e1149..53dfdcd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -392,6 +392,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `@docmost/token-estimate` package) so they can never diverge. Deferred-tool activation is also cached in the chat metadata to avoid re-resolving it each turn. (#490) +- **Cyrillic (and any non-ASCII) draw.io labels no longer turn into mojibake + when a diagram is opened in the draw.io editor.** Agent-created diagrams + (`drawioCreate`) and Confluence-imported diagrams stored their model in the + SVG's `content=` attribute as base64; the draw.io editor decodes that via + Latin-1 `atob` (no UTF-8 step), so every non-ASCII char (e.g. `Старт-бит`, + `ё`, `—`) split into garbage and the editor's autosave then persisted the + corrupted model, breaking the page preview too. Both write paths + (`buildDrawioSvg`, the import service's `createDrawioSvg`) now write `content=` + 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:* 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 new file mode 100644 index 00000000..ebe67431 --- /dev/null +++ b/apps/server/src/integrations/import/services/import-attachment.service.drawio-svg.spec.ts @@ -0,0 +1,131 @@ +// p-limit and @sindresorhus/slugify are ESM-only and not in jest's transform +// allowlist; both are irrelevant to createDrawioSvg (a pure fs + string method), +// so they are mocked out to keep the module graph loadable under ts-jest. +jest.mock('p-limit', () => ({ + __esModule: true, + default: () => (fn: () => unknown) => fn(), +})); +jest.mock('@sindresorhus/slugify', () => ({ + __esModule: true, + default: (input: string) => String(input), +})); + +import { promises as fs } from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { ImportAttachmentService } from './import-attachment.service'; + +/** + * Unit test for ImportAttachmentService.createDrawioSvg (issue #507). + * + * The Confluence import wraps a `.drawio` file into a `.drawio.svg` attachment. + * The `content=` payload MUST be the mxfile XML entity-escaped (draw.io's native + * form), NOT base64 — draw.io's editor decodes a base64 content= via Latin-1 + * atob, mangling every non-ASCII char into mojibake. createDrawioSvg touches no + * injected dependency, so the service is built with placeholder deps. + */ +describe('ImportAttachmentService.createDrawioSvg (#507)', () => { + const service = new ImportAttachmentService( + {} as any, + {} as any, + {} as any, + ); + const call = (p: string): Promise => + (service as any).createDrawioSvg(p); + + let tmpDir: string; + beforeAll(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'drawio-507-')); + }); + afterAll(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }); + }); + + const writeDrawio = async (name: string, xml: string): Promise => { + const p = path.join(tmpDir, name); + await fs.writeFile(p, xml, 'utf-8'); + return p; + }; + + it('writes content= as entity-encoded XML, not base64', async () => { + const drawio = + '' + + '' + + '' + + ''; + const p = await writeDrawio('cyrillic.drawio', drawio); + + const svg = (await call(p)).toString('utf-8'); + const content = /content="([^"]*)"/.exec(svg)?.[1]; + expect(content).toBeDefined(); + + // Entity-encoded XML form, starting with <mxfile — never a base64 blob. + expect(content).toMatch(/^<mxfile/); + expect(content).toContain('<'); + // Non-ASCII survives as raw UTF-8, with no Latin-1 mojibake. + expect(content).toContain('Старт-бит'); + expect(content).toContain('Схема — ёж'); + expect(content).not.toContain('Ð'); + + // Decoding the attribute (un-escaping) yields the original drawio file. + const decoded = content! + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/&/g, '&'); + 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); + + const svg = (await call(p)).toString('utf-8'); + const content = /content="([^"]*)"/.exec(svg)?.[1]; + expect(content).toBeDefined(); + // The attribute value must contain no bare `<`, `>` or `"` that would break + // out of the content="..." attribute or the SVG element. + expect(content).not.toMatch(/[<>"]/); + const decoded = content! + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/&/g, '&'); + expect(decoded).toBe(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 9100149b..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'; @@ -849,7 +850,12 @@ export class ImportAttachmentService { ): Promise { try { const drawioContent = await fs.readFile(drawioPath, 'utf-8'); - const drawioBase64 = Buffer.from(drawioContent).toString('base64'); + // Write the mxfile XML XML-entity-escaped (draw.io's native content= form), + // NOT base64. draw.io's editor decodes a base64 content= via Latin-1 atob + // (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.xmlEscapeContent(drawioContent); let imageElement = ''; // If we have a PNG, include it in the SVG @@ -875,7 +881,7 @@ export class ImportAttachmentService { width="600" height="400" viewBox="0 0 600 400" - content="${drawioBase64}">${imageElement}`; + content="${drawioEscaped}">${imageElement}`; return Buffer.from(svgContent, 'utf-8'); } catch (error) { @@ -884,6 +890,24 @@ 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`). 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 xmlEscapeContent(s: string): string { + return htmlEscape(s) + .replace(/\t/g, ' ') + .replace(/\n/g, ' ') + .replace(/\r/g, ' '); + } + private async uploadWithRetry(opts: { abs: string; storageFilePath: string; diff --git a/packages/mcp/src/lib/drawio-xml.ts b/packages/mcp/src/lib/drawio-xml.ts index 84b2c5a3..620b12ea 100644 --- a/packages/mcp/src/lib/drawio-xml.ts +++ b/packages/mcp/src/lib/drawio-xml.ts @@ -178,10 +178,11 @@ function sliceModel(xml: string): string | null { // --- decode chain ---------------------------------------------------------- /** - * Read the `content=` attribute out of a `.drawio.svg` string. Docmost stores a - * base64 payload there (createDrawioSvg); draw.io's own SVG export may store the - * XML entity-encoded instead. The DOM decodes entities for us, so the caller - * only has to distinguish "starts with '<'" (raw XML) from base64. + * Read the `content=` attribute out of a `.drawio.svg` string. Docmost writes + * the mxfile XML entity-encoded there (buildDrawioSvg / createDrawioSvg), which + * is also how draw.io's own SVG export stores it; older attachments stored a + * base64 payload instead. The DOM decodes entities for us, so the caller only + * has to distinguish "starts with '<'" (raw XML) from base64. */ export function extractContentAttr(svg: string): string { const { doc, error } = parseXml(svg); @@ -195,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"); @@ -307,9 +319,16 @@ export function encodeDrawioFile(modelXml: string, title = "Page-1"): string { /** * Build the `diagram.drawio.svg` attachment. Mirrors the import service's * createDrawioSvg contract exactly: - * ${inner} + * ${inner} * plus width/height/viewBox from the diagram bounding box and the schematic * preview as the visible children (`inner`). + * + * The `content=` value is the mxfile XML XML-entity-escaped (draw.io's own + * native form), NOT base64. draw.io's editor decodes a base64 content= via + * Latin-1 atob (no UTF-8 step), turning every non-ASCII char (e.g. Cyrillic, + * ё, —) into mojibake; the entity-encoded form is decoded by the DOM as UTF-8 + * and opens intact. Our decoder (decodeDrawioSvg) reads both forms, so old + * base64 attachments still round-trip. */ export function buildDrawioSvg( modelXml: string, @@ -318,14 +337,14 @@ export function buildDrawioSvg( title = "Page-1", ): string { const file = encodeDrawioFile(modelXml, title); - const base64 = Buffer.from(file, "utf-8").toString("base64"); + const content = xmlEscape(file); const w = Math.max(1, Math.round(bbox.width)); const h = Math.max(1, Math.round(bbox.height)); return ( `${inner}` + `content="${content}">${inner}` ); } @@ -334,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 8d6e83ec..3295b0f8 100644 --- a/packages/mcp/test/unit/drawio-xml.test.mjs +++ b/packages/mcp/test/unit/drawio-xml.test.mjs @@ -51,6 +51,14 @@ const hasRule = (issues, rule, cellId) => (i) => i.rule === rule && (cellId === undefined || i.cellId === cellId), ); +// Reverse the attribute-value XML escaping used in the `content=` payload. +const unescapeAttr = (s) => + s + .replace(/</g, "<") + .replace(/>/g, ">") + .replace(/"/g, '"') + .replace(/&/g, "&"); + // --- style parsing --------------------------------------------------------- test("parseStyle: base stylename + key=value pairs", () => { @@ -335,16 +343,20 @@ test("encode/build: a title with < > \" & round-trips without corrupting the SVG // The outer content="..." attribute must not be broken by the title: the raw // title metacharacters never appear literally in the SVG markup (they are - // base64-encoded inside content=, and escaped inside the file XML). + // entity-escaped inside content=, doubly so where the file XML already escaped + // them inside name="..."). const contentMatch = /content="([^"]*)"/.exec(svg); assert.ok(contentMatch, "SVG has a single well-formed content= attribute"); // The diagram model still decodes losslessly despite the exotic title. assert.equal(decodeDrawioSvg(svg), model); + // content= is now entity-encoded XML (draw.io's native form), never base64. + assert.match(contentMatch[1], /^<mxfile/); + // The file XML is well-formed: the title lives in name="..." as escaped // entities, so unescaping recovers the original title byte-for-byte. - const fileXml = Buffer.from(contentMatch[1], "base64").toString("utf-8"); + const fileXml = unescapeAttr(contentMatch[1]); const nameMatch = //.exec(fileXml); assert.ok(nameMatch, "the diagram name attribute is intact and quote-safe"); const decodedTitle = nameMatch[1] @@ -358,3 +370,112 @@ test("encode/build: a title with < > \" & round-trips without corrupting the SVG const file = encodeDrawioFile(model, title); assert.match(file, /name="A < B > C " D & E">/); }); + +// --- #507: content= is entity-encoded XML, never base64 -------------------- + +// A model whose cell values carry Cyrillic, ё and an em dash — exactly the +// characters draw.io's Latin-1 atob mangles when content= is base64. +const CYRILLIC_MODEL = + "" + + '' + + '' + + '' + + ""; + +test("#507: buildDrawioSvg writes content= as entity-encoded XML (not base64)", () => { + const model = normalizeXml(CYRILLIC_MODEL); + const svg = buildDrawioSvg(model, "", { width: 200, height: 120 }, "Диаграмма"); + + const contentMatch = /content="([^"]*)"/.exec(svg); + assert.ok(contentMatch, "SVG has a single well-formed content= attribute"); + const content = contentMatch[1]; + + // The content= value is the entity-encoded mxfile XML — starts with `<mxfile`. + assert.match(content, /^<mxfile/, "content= is entity-encoded mxfile XML"); + // It must NOT be a base64 blob: base64 has no XML entities and no literal `<`. + assert.ok(content.includes("<"), "content= carries XML entities, not base64"); + + // Cyrillic / ё / — survive verbatim in the attribute (raw UTF-8, not atob-mangled). + assert.ok(content.includes("Старт-бит — ёж"), "non-ASCII value is raw UTF-8 in content="); + assert.ok(content.includes("Диаграмма"), "non-ASCII title is raw UTF-8 in content="); + // The mojibake that base64+atob would have produced must be absent. + assert.ok(!content.includes("Ð"), "no Latin-1 mojibake in content="); +}); + +test("#507: Cyrillic model round-trips byte-stable through buildDrawioSvg -> decodeDrawioSvg", () => { + const model = normalizeXml(CYRILLIC_MODEL); + const svg = buildDrawioSvg(model, "", { width: 200, height: 120 }, "Заголовок — ё"); + assert.equal(decodeDrawioSvg(svg), model); +}); + +test("#507 back-compat: an OLD base64-form .drawio.svg still decodes losslessly", () => { + const model = normalizeXml(CYRILLIC_MODEL); + // Reproduce the pre-fix write path: encodeDrawioFile -> base64 in content=. + const file = encodeDrawioFile(model, "Старая диаграмма"); + const base64 = Buffer.from(file, "utf-8").toString("base64"); + const svg = + ``; + // No XML entities, purely base64 alphabet — this is the legacy form. + assert.ok(!base64.includes("<") && !base64.includes("&")); + assert.equal(decodeDrawioSvg(svg), model); +}); + +test("#507 negative: non-ASCII in a cell value AND in the title round-trip clean", () => { + const model = normalizeXml(CYRILLIC_MODEL); + const title = "Тест — ёмкость № 5"; + const svg = buildDrawioSvg(model, "", { width: 200, height: 120 }, title); + + // Model recovered byte-for-byte. + assert.equal(decodeDrawioSvg(svg), model); + + // Title lands in the of the decoded file XML, intact. + const content = /content="([^"]*)"/.exec(svg)[1]; + const fileXml = unescapeAttr(content); + const nameMatch = //.exec(fileXml); + 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); +});