fix: declare namespace prefixes that are not in scope when saving (#456)

.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.

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,

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';

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,30 +81,83 @@ 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<string, string>;
+
+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<string, string> | null => {
+  let added: Map<string, string> | 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<string, string>, 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<string, string>): 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) {
     parts.push('/>');
     return;
   }
   parts.push('>');
-  for (const child of e.children) writeNode(child, parts);
+  for (const child of e.children) writeNode(child, parts, childScope);
   parts.push('</');
   parts.push(name);
   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 = '<?xml version="1.0" encoding="UTF-8" standalone="yes"?>';
+
 const writeDeclaration = (doc: XmlDocument, parts: string[]): void => {
   if (!doc.decl) return;
   parts.push('<?xml version="');
@@ -163,19 +220,19 @@ export const serializeXml = (
   } else if (options.standaloneDeclaration ?? true) {
     // OOXML parts MUST start with an XML declaration. Add a sensible default
     // when the AST doesn't carry one (typical for hand-built trees).
-    parts.push('<?xml version="1.0" encoding="UTF-8" standalone="yes"?>');
+    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('');
 };
 
 // Serialize a single element with no declaration / document wrapper. Useful in
 // 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('');
 };
 

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(/<p:(sldMasterId|sldId) /g, `<p:$1 xmlns:r="${R}" `);
+
+describe('issue-456: undeclared r: prefix on new elements', () => {
+  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<typeof elem>) => ({
+    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(
+      '<p:root xmlns:p="urn:p"><p:c xmlns:r="urn:r" r:id="rId1"/><p:c xmlns:r="urn:r" r:id="rId1"/></p:root>',
+    );
+    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(
+      '<x:root xmlns:x="urn:one"><x:c xmlns:x="urn:two"/></x:root>',
+    );
+  });
+
+  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(
+      '<Types xmlns="urn:ct"><Default/></Types>',
+    );
+  });
+
+  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"/);
+  });
+});