diff --git a/.changeset/undeclared-namespace-prefix.md b/.changeset/undeclared-namespace-prefix.md new file mode 100644 index 000000000..bd2149e91 --- /dev/null +++ b/.changeset/undeclared-namespace-prefix.md @@ -0,0 +1,5 @@ +--- +'@office-kit/pptx': patch +--- + +Fixed: `duplicateSlide`, `addSlideImage` and other edits could save XML with an unbound `r:` (or other) prefix when the template declared that prefix on individual elements instead of the part root, so the saved file failed to load (#456). The writer now declares any prefix a new element or attribute uses that is not already in scope. diff --git a/src/api/fn/shape-slide-read.ts b/src/api/fn/shape-slide-read.ts index 679634aa5..8044609a3 100644 --- a/src/api/fn/shape-slide-read.ts +++ b/src/api/fn/shape-slide-read.ts @@ -16,7 +16,7 @@ import { type ShapeKind, readSlideLayoutPart, } from '../../internal/presentationml/index.ts'; -import { parseXml, serializeXml } from '../../internal/xml/index.ts'; +import { XML_DECLARATION, parseXml, serializeFragment } from '../../internal/xml/index.ts'; import { INTERNAL_PACKAGE, LAYOUT_DOCUMENT, @@ -394,19 +394,16 @@ export const findShapesByPreset = ( */ export const getShapeSlide = (shape: SlideShapeData): SlideData => shape[SHAPE_SLIDE]; +// The shape's prefixes are declared on its slide's root, so it is written as a +// fragment: declaring them on the shape would make it read differently from +// the same element inside `getSlideXmlString`. /** * Returns the shape's current XML element as a string. Diagnostic * sibling of `getSlideXmlString`; useful for snapshot tests, bug * reports, and before/after dumps during transformations. */ export const getShapeXmlString = (shape: SlideShapeData): string => - serializeXml({ - kind: 'document', - decl: null, - root: shape[SHAPE_ELEMENT], - prolog: [], - epilog: [], - }); + XML_DECLARATION + serializeFragment(shape[SHAPE_ELEMENT]); /** * Returns the 0-based document-order index of `shape` on its slide, diff --git a/src/internal/xml/index.ts b/src/internal/xml/index.ts index 15bc0e49a..1c3aeb066 100644 --- a/src/internal/xml/index.ts +++ b/src/internal/xml/index.ts @@ -39,4 +39,4 @@ export { textContent, walkElements, } from './query.ts'; -export { serializeFragment, serializeXml } from './serialize.ts'; +export { XML_DECLARATION, serializeFragment, serializeXml } from './serialize.ts'; diff --git a/src/internal/xml/serialize.ts b/src/internal/xml/serialize.ts index 01401e5e1..144b2e531 100644 --- a/src/internal/xml/serialize.ts +++ b/src/internal/xml/serialize.ts @@ -6,7 +6,8 @@ // way out match the way in (provided the AST hasn't been mutated to break // that). The serializer never reorders attributes, never reallocates // namespace prefixes, never re-emits xmlns declarations on a different -// element than where the parser placed them. +// element than where the parser placed them. It only adds a declaration +// where an element's or attribute's prefix would otherwise be unbound. // - Predictable output for hand-built ASTs. Authoring-time code can assume // the same input element produces the same byte sequence. // - Escape rules tight enough for the reference desktop app to accept the output: the @@ -21,6 +22,7 @@ // someone really wants it, but the round-trip story is cleaner without. import type { XmlAttr, XmlDocument, XmlElement, XmlNode } from './ast.ts'; +import { NS } from './namespaces.ts'; // XML 1.0 §2.2 excludes C0 controls other than tab / LF / CR, U+FFFE, // U+FFFF, and unpaired surrogates. Numeric references cannot rescue them. @@ -79,13 +81,66 @@ const writeAttr = (a: XmlAttr): string => { return ` ${name}="${escapeAttr(a.value)}"`; }; -const writeElement = (e: XmlElement, parts: string[]): void => { +// Prefix → URI bindings in scope at the element being written. The root of a +// document starts with only the reserved `xml` prefix bound. +type Scope = ReadonlyMap; + +const DOCUMENT_SCOPE: Scope = new Map([['xml', NS.xml]]); + +// Parsed elements carry the declarations their input had, but elements built +// with `elem(...)` carry none and rely on an ancestor declaring their prefixes. +// Producers may declare a prefix on the element that uses it rather than on +// the part root, so that ancestor is not guaranteed; declare whatever the +// element's own name and attributes need that is not already in scope. +const missingDecls = (e: XmlElement, scope: Scope): Map | null => { + let added: Map | null = null; + for (let i = -1; i < e.attrs.length; i++) { + const name = i < 0 ? e.name : e.attrs[i]!.name; + // Unprefixed attributes are in no namespace whatever the default is. + if (i >= 0 && name.prefix === '') continue; + const own = e.prefixDecls.get(name.prefix) ?? added?.get(name.prefix); + if (own === undefined) { + if ((scope.get(name.prefix) ?? '') === name.namespaceURI) continue; + added ??= new Map(); + added.set(name.prefix, name.namespaceURI); + } else if (own !== name.namespaceURI) { + throw new Error( + `prefix "${name.prefix}" on <${rawName(e.name.prefix, e.name.localName)}> is bound to both ${own} and ${name.namespaceURI}`, + ); + } + } + return added; +}; + +const writeDecls = (decls: ReadonlyMap, parts: string[]): void => { + for (const [prefix, uri] of decls) { + const decl = prefix === '' ? 'xmlns' : `xmlns:${prefix}`; + parts.push(` ${decl}="${escapeAttr(uri)}"`); + } +}; + +const withDecls = (scope: Scope, decls: ReadonlyMap): Scope => { + if (decls.size === 0) return scope; + const next = new Map(scope); + for (const [prefix, uri] of decls) next.set(prefix, uri); + return next; +}; + +// `scope` is null for fragments: their context is unknown, so they are written +// with exactly the declarations they carry. +const writeElement = (e: XmlElement, parts: string[], scope: Scope | null): void => { const name = rawName(e.name.prefix, e.name.localName); parts.push('<'); parts.push(name); - for (const [prefix, uri] of e.prefixDecls) { - const decl = prefix === '' ? 'xmlns' : `xmlns:${prefix}`; - parts.push(` ${decl}="${escapeAttr(uri)}"`); + writeDecls(e.prefixDecls, parts); + let childScope = scope; + if (scope) { + const added = missingDecls(e, scope); + childScope = withDecls(scope, e.prefixDecls); + if (added) { + writeDecls(added, parts); + childScope = withDecls(childScope, added); + } } for (const a of e.attrs) parts.push(writeAttr(a)); if (e.children.length === 0) { @@ -93,16 +148,16 @@ const writeElement = (e: XmlElement, parts: string[]): void => { return; } parts.push('>'); - for (const child of e.children) writeNode(child, parts); + for (const child of e.children) writeNode(child, parts, childScope); parts.push(''); }; -const writeNode = (n: XmlNode, parts: string[]): void => { +const writeNode = (n: XmlNode, parts: string[], scope: Scope | null): void => { switch (n.kind) { case 'element': - writeElement(n, parts); + writeElement(n, parts, scope); return; case 'text': parts.push(escapeText(n.data)); @@ -135,6 +190,8 @@ const writeNode = (n: XmlNode, parts: string[]): void => { } }; +export const XML_DECLARATION = ''; + const writeDeclaration = (doc: XmlDocument, parts: string[]): void => { if (!doc.decl) return; parts.push(''); + parts.push(XML_DECLARATION); } - for (const p of doc.prolog) writeNode(p, parts); - writeElement(doc.root, parts); - for (const e of doc.epilog) writeNode(e, parts); + for (const p of doc.prolog) writeNode(p, parts, null); + writeElement(doc.root, parts, DOCUMENT_SCOPE); + for (const e of doc.epilog) writeNode(e, parts, null); return parts.join(''); }; @@ -175,7 +232,7 @@ export const serializeXml = ( // tests and when emitting fragments that get inlined into a larger document. export const serializeFragment = (element: XmlElement): string => { const parts: string[] = []; - writeElement(element, parts); + writeElement(element, parts, null); return parts.join(''); }; diff --git a/test/issue-456-undeclared-prefix.test.ts b/test/issue-456-undeclared-prefix.test.ts new file mode 100644 index 000000000..d402cee86 --- /dev/null +++ b/test/issue-456-undeclared-prefix.test.ts @@ -0,0 +1,124 @@ +import { strFromU8, strToU8, unzipSync, zipSync } from 'fflate'; +import { describe, expect, it } from 'vitest'; +import { + addBlankSlide, + addSlideImage, + createPresentation, + duplicateSlide, + emu, + getSlides, + getSlideShapes, + loadPresentation, + removeSlide, + savePresentation, +} from '../src/api/index.ts'; +import { attr, elem, parseXml, qname, serializeXml } from '../src/internal/xml/index.ts'; + +const R = 'http://schemas.openxmlformats.org/officeDocument/2006/relationships'; +const PNG = Uint8Array.from( + atob( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk+M9QDwADhgGAWjR9awAAAABJRU5ErkJggg==', + ), + (c) => c.charCodeAt(0), +); + +const editPart = (bytes: Uint8Array, name: string, edit: (xml: string) => string) => { + const files = unzipSync(bytes); + files[name] = strToU8(edit(strFromU8(files[name]!))); + return zipSync(files); +}; + +const deckWithOneSlide = async () => { + const pres = createPresentation(); + addBlankSlide(pres); + return savePresentation(pres); +}; + +// Namespaces in XML lets a producer declare a prefix on the element that uses +// it rather than on the part root. +const declareRLocally = (xml: string) => + xml.replace(` xmlns:r="${R}"`, '').replace(/ { + it('duplicateSlide declares r: when presentation.xml declares it per element', async () => { + const bytes = editPart(await deckWithOneSlide(), 'ppt/presentation.xml', declareRLocally); + const pres = await loadPresentation(bytes); + duplicateSlide(pres, getSlides(pres)[0]!); + const reloaded = await loadPresentation(await savePresentation(pres)); + expect(getSlides(reloaded)).toHaveLength(2); + }); + + it('removeSlide after duplicateSlide re-parses presentation.xml cleanly', async () => { + const bytes = editPart(await deckWithOneSlide(), 'ppt/presentation.xml', declareRLocally); + const pres = await loadPresentation(bytes); + duplicateSlide(pres, getSlides(pres)[0]!); + removeSlide(pres, getSlides(pres)[0]!); + expect(getSlides(pres)).toHaveLength(1); + }); + + it('addSlideImage declares r: on a slide whose root omits it', async () => { + const bytes = editPart(await deckWithOneSlide(), 'ppt/slides/slide1.xml', (xml) => + xml.replace(` xmlns:r="${R}"`, ''), + ); + const pres = await loadPresentation(bytes); + addSlideImage(getSlides(pres)[0]!, PNG, { + x: emu(0), + y: emu(0), + w: emu(914400), + h: emu(914400), + }); + const reloaded = await loadPresentation(await savePresentation(pres)); + expect(getSlideShapes(getSlides(reloaded)[0]!)).toHaveLength(1); + }); +}); + +describe('serializeXml namespace scope', () => { + const doc = (root: ReturnType) => ({ + kind: 'document' as const, + decl: null, + prolog: [], + root, + epilog: [], + }); + + it('declares a prefix only where it is not already in scope', () => { + const child = elem(qname('p', 'c', 'urn:p'), { + attrs: [attr(qname('r', 'id', 'urn:r'), 'rId1')], + }); + const root = elem(qname('p', 'root', 'urn:p'), { + prefixDecls: new Map([['p', 'urn:p']]), + children: [child, child], + }); + const out = serializeXml(doc(root), { standaloneDeclaration: false }); + expect(out).toBe( + '', + ); + expect(() => parseXml(out)).not.toThrow(); + }); + + it('redeclares a prefix that is bound to a different URI in scope', () => { + const root = elem(qname('x', 'root', 'urn:one'), { + prefixDecls: new Map([['x', 'urn:one']]), + children: [elem(qname('x', 'c', 'urn:two'))], + }); + expect(serializeXml(doc(root), { standaloneDeclaration: false })).toBe( + '', + ); + }); + + it('declares the default namespace for an unprefixed element in a namespace', () => { + const root = elem(qname('', 'Types', 'urn:ct'), { + children: [elem(qname('', 'Default', 'urn:ct'))], + }); + expect(serializeXml(doc(root), { standaloneDeclaration: false })).toBe( + '', + ); + }); + + it('rejects one prefix bound to two URIs on the same element', () => { + const root = elem(qname('x', 'root', 'urn:one'), { + attrs: [attr(qname('x', 'a', 'urn:two'), 'v')], + }); + expect(() => serializeXml(doc(root))).toThrow(/prefix "x"/); + }); +});