From 13037c7b93a1e86f6ccf6595e9f8eadef0b8026c Mon Sep 17 00:00:00 2001 From: baseballyama Date: Mon, 12 Oct 2026 23:42:55 +0900 Subject: [PATCH 1/2] fix: declare namespace prefixes that are not in scope when saving (#456) Elements built in code carry no namespace declarations and relied on an ancestor declaring their prefixes. Namespaces in XML lets a producer declare a prefix on the element that uses it instead of the part root, so new `` / `` elements could be written with an unbound prefix and the saved part no longer parsed. The serializer now tracks the prefixes in scope and declares any that an element's name or attributes need. Parsed trees are unaffected: their declarations already cover every prefix. getShapeXmlString keeps writing the shape as a fragment so its output does not change. Closes #456 Co-Authored-By: Claude Opus 5.5 --- .changeset/undeclared-namespace-prefix.md | 5 + src/api/fn/shape-slide-read.ts | 13 +-- src/internal/xml/index.ts | 2 +- src/internal/xml/serialize.ts | 85 ++++++++++++--- test/issue-456-undeclared-prefix.test.ts | 124 ++++++++++++++++++++++ 5 files changed, 207 insertions(+), 22 deletions(-) create mode 100644 .changeset/undeclared-namespace-prefix.md create mode 100644 test/issue-456-undeclared-prefix.test.ts 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..98493d5ae 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, @@ -399,14 +399,11 @@ export const getShapeSlide = (shape: SlideShapeData): SlideData => shape[SHAPE_S * sibling of `getSlideXmlString`; useful for snapshot tests, bug * reports, and before/after dumps during transformations. */ +// 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`. 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..01eedcdb5 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,68 @@ 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; + const require = (prefix: string, uri: string): void => { + const own = e.prefixDecls.get(prefix) ?? added?.get(prefix); + if (own !== undefined) { + if (own !== uri) { + throw new Error( + `prefix "${prefix}" on <${rawName(e.name.prefix, e.name.localName)}> is bound to both ${own} and ${uri}`, + ); + } + return; + } + if ((scope.get(prefix) ?? '') === uri) return; + added ??= new Map(); + added.set(prefix, uri); + }; + require(e.name.prefix, e.name.namespaceURI); + // Unprefixed attributes are in no namespace whatever the default is. + for (const a of e.attrs) if (a.name.prefix !== '') require(a.name.prefix, a.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 +150,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 +192,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 +234,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"/); + }); +}); From aadc489908d79273214941368b02231553fc62ee Mon Sep 17 00:00:00 2001 From: baseballyama Date: Mon, 12 Oct 2026 23:52:15 +0900 Subject: [PATCH 2/2] perf: check element and attribute prefixes in one loop Co-Authored-By: Claude Opus 5.5 --- src/api/fn/shape-slide-read.ts | 6 +++--- src/internal/xml/serialize.ts | 30 ++++++++++++++---------------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/api/fn/shape-slide-read.ts b/src/api/fn/shape-slide-read.ts index 98493d5ae..8044609a3 100644 --- a/src/api/fn/shape-slide-read.ts +++ b/src/api/fn/shape-slide-read.ts @@ -394,14 +394,14 @@ 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. */ -// 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`. export const getShapeXmlString = (shape: SlideShapeData): string => XML_DECLARATION + serializeFragment(shape[SHAPE_ELEMENT]); diff --git a/src/internal/xml/serialize.ts b/src/internal/xml/serialize.ts index 01eedcdb5..144b2e531 100644 --- a/src/internal/xml/serialize.ts +++ b/src/internal/xml/serialize.ts @@ -94,23 +94,21 @@ const DOCUMENT_SCOPE: Scope = new Map([['xml', NS.xml]]); // 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; - const require = (prefix: string, uri: string): void => { - const own = e.prefixDecls.get(prefix) ?? added?.get(prefix); - if (own !== undefined) { - if (own !== uri) { - throw new Error( - `prefix "${prefix}" on <${rawName(e.name.prefix, e.name.localName)}> is bound to both ${own} and ${uri}`, - ); - } - return; + 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}`, + ); } - if ((scope.get(prefix) ?? '') === uri) return; - added ??= new Map(); - added.set(prefix, uri); - }; - require(e.name.prefix, e.name.namespaceURI); - // Unprefixed attributes are in no namespace whatever the default is. - for (const a of e.attrs) if (a.name.prefix !== '') require(a.name.prefix, a.name.namespaceURI); + } return added; };