Fix key reuse bug (Hopding#1078)

* Prevent new XObject keys from conflicting with existing ones

* Add test for XObject key reuse

* Prevent Font key reuse

* Fix other instances of key reuse

* Fix another instance

* Add PDFDict.uniqueKey() test

Author: Hopding

Diff for: scratchpad/index.ts

@@ -3,38 +3,23 @@ import { openPdf, Reader } from './open';
 import { PDFDocument } from 'src/index';
 
 (async () => {
-  // This should be a Uint8Array or ArrayBuffer
-  // This data can be obtained in a number of different ways
-  // If your running in a Node environment, you could use fs.readFile()
-  // In the browser, you could make a fetch() call and use res.arrayBuffer()
-  const existingPdfBytes = fs.readFileSync('assets/pdfs/with_annots.pdf');
+  const pdfDoc1 = await PDFDocument.create();
+  const image1 = await pdfDoc1.embedPng(
+    fs.readFileSync('assets/images/mario_emblem.png'),
+  );
+  const page1 = pdfDoc1.addPage();
+  page1.drawImage(image1, { ...image1.scale(1.0) });
 
-  // Load a PDFDocument without updating its existing metadata
-  const pdfDoc = await PDFDocument.load(existingPdfBytes);
-  const viewerPrefs = pdfDoc.catalog.getOrCreateViewerPreferences();
+  const pdfDoc1Bytes = await pdfDoc1.save();
 
-  // Print all available viewer preference fields
-  console.log('HideToolbar:', viewerPrefs.getHideToolbar());
-  console.log('HideMenubar:', viewerPrefs.getHideMenubar());
-  console.log('HideWindowUI:', viewerPrefs.getHideWindowUI());
-  console.log('FitWindow:', viewerPrefs.getFitWindow());
-  console.log('CenterWindow:', viewerPrefs.getCenterWindow());
-  console.log('DisplayDocTitle:', viewerPrefs.getDisplayDocTitle());
-  console.log('NonFullScreenPageMode:', viewerPrefs.getNonFullScreenPageMode());
-  console.log('ReadingDirection:', viewerPrefs.getReadingDirection());
-  console.log('PrintScaling:', viewerPrefs.getPrintScaling());
-  console.log('Duplex:', viewerPrefs.getDuplex());
-  console.log('PickTrayByPDFSize:', viewerPrefs.getPickTrayByPDFSize());
-  console.log('PrintPageRange:', viewerPrefs.getPrintPageRange());
-  console.log('NumCopies:', viewerPrefs.getNumCopies());
+  const pdfDoc2 = await PDFDocument.load(pdfDoc1Bytes);
+  const image2 = await pdfDoc2.embedPng(
+    fs.readFileSync('assets/images/minions_banana_alpha.png'),
+  );
+  const page2 = pdfDoc2.getPage(0);
+  page2.drawImage(image2, { ...image2.scale(0.5), x: 100, y: 100 });
 
-  // Serialize the PDFDocument to bytes (a Uint8Array)
-  const pdfBytes = await pdfDoc.save();
-
-  // For example, `pdfBytes` can be:
-  //   • Written to a file in Node
-  //   • Downloaded from the browser
-  //   • Rendered in an <iframe>
+  const pdfBytes = await pdfDoc2.save();
 
   fs.writeFileSync('out.pdf', pdfBytes);
   openPdf('out.pdf', Reader.Preview);

Diff for: src/api/PDFPage.ts

@@ -100,7 +100,7 @@ export default class PDFPage {
   /** The document to which this page belongs. */
   readonly doc: PDFDocument;
 
-  private fontKey?: string;
+  private fontKey?: PDFName;
   private font?: PDFFont;
   private fontSize = 24;
   private fontColor = rgb(0, 0, 0) as Color;
@@ -699,8 +699,7 @@ export default class PDFPage {
     // TODO: Reuse image Font name if we've already added this image to Resources.Fonts
     assertIs(font, 'font', [[PDFFont, 'PDFFont']]);
     this.font = font;
-    this.fontKey = this.doc.context.addRandomSuffix(this.font.name);
-    this.node.setFontDictionary(PDFName.of(this.fontKey), this.font.ref);
+    this.fontKey = this.node.newFontDictionary(this.font.name, this.font.ref);
   }
 
   /**
@@ -1059,8 +1058,7 @@ export default class PDFPage {
     assertRangeOrUndefined(options.opacity, 'opacity.opacity', 0, 1);
     assertIsOneOfOrUndefined(options.blendMode, 'options.blendMode', BlendMode);
 
-    const xObjectKey = this.doc.context.addRandomSuffix('Image', 10);
-    this.node.setXObject(PDFName.of(xObjectKey), image.ref);
+    const xObjectKey = this.node.newXObject('Image', image.ref);
 
     const graphicsStateKey = this.maybeEmbedGraphicsState({
       opacity: options.opacity,
@@ -1134,8 +1132,10 @@ export default class PDFPage {
     assertRangeOrUndefined(options.opacity, 'opacity.opacity', 0, 1);
     assertIsOneOfOrUndefined(options.blendMode, 'options.blendMode', BlendMode);
 
-    const xObjectKey = this.doc.context.addRandomSuffix('EmbeddedPdfPage', 10);
-    this.node.setXObject(PDFName.of(xObjectKey), embeddedPage.ref);
+    const xObjectKey = this.node.newXObject(
+      'EmbeddedPdfPage',
+      embeddedPage.ref,
+    );
 
     const graphicsStateKey = this.maybeEmbedGraphicsState({
       opacity: options.opacity,
@@ -1549,7 +1549,7 @@ export default class PDFPage {
     return { oldFont, oldFontKey, newFont, newFontKey };
   }
 
-  private getFont(): [PDFFont, string] {
+  private getFont(): [PDFFont, PDFName] {
     if (!this.font || !this.fontKey) {
       const font = this.doc.embedStandardFont(StandardFonts.Helvetica);
       this.setFont(font);
@@ -1580,7 +1580,7 @@ export default class PDFPage {
     opacity?: number;
     borderOpacity?: number;
     blendMode?: BlendMode;
-  }): string | undefined {
+  }): PDFName | undefined {
     const { opacity, borderOpacity, blendMode } = options;
 
     if (
@@ -1591,16 +1591,14 @@ export default class PDFPage {
       return undefined;
     }
 
-    const key = this.doc.context.addRandomSuffix('GS', 10);
-
     const graphicsState = this.doc.context.obj({
       Type: 'ExtGState',
       ca: opacity,
       CA: borderOpacity,
       BM: blendMode,
     });
 
-    this.node.setExtGState(PDFName.of(key), graphicsState);
+    const key = this.node.newExtGState('GS', graphicsState);
 
     return key;
   }

Diff for: src/api/form/PDFForm.ts

@@ -550,11 +550,9 @@ export default class PDFForm {
         const page = this.findWidgetPage(widget);
         const widgetRef = this.findWidgetAppearanceRef(field, widget);
 
-        const xObjectKey = this.doc.context.addRandomSuffix('FlatWidget', 10);
-        page.node.setXObject(PDFName.of(xObjectKey), widgetRef);
+        const xObjectKey = page.node.newXObject('FlatWidget', widgetRef);
 
         const rectangle = widget.getRectangle();
-
         const operators = [
           pushGraphicsState(),
           translate(rectangle.x, rectangle.y),

Diff for: src/core/objects/PDFDict.ts

@@ -159,6 +159,16 @@ class PDFDict extends PDFObject {
     return new Map(this.dict);
   }
 
+  /** Generate a random key that doesn't exist in current key set */
+  uniqueKey(tag = ''): PDFName {
+    const existingKeys = this.keys();
+    let key = PDFName.of(this.context.addRandomSuffix(tag, 10));
+    while (existingKeys.includes(key)) {
+      key = PDFName.of(this.context.addRandomSuffix(tag, 10));
+    }
+    return key;
+  }
+
   clone(context?: PDFContext): PDFDict {
     const clone = PDFDict.withContext(context || this.context);
     const entries = this.entries();

Diff for: src/core/structures/PDFPageLeaf.ts

@@ -150,16 +150,49 @@ class PDFPageLeaf extends PDFDict {
     Font.set(name, fontDictRef);
   }
 
+  newFontDictionaryKey(tag: string): PDFName {
+    const { Font } = this.normalizedEntries();
+    return Font.uniqueKey(tag);
+  }
+
+  newFontDictionary(tag: string, fontDictRef: PDFRef): PDFName {
+    const key = this.newFontDictionaryKey(tag);
+    this.setFontDictionary(key, fontDictRef);
+    return key;
+  }
+
   setXObject(name: PDFName, xObjectRef: PDFRef): void {
     const { XObject } = this.normalizedEntries();
     XObject.set(name, xObjectRef);
   }
 
+  newXObjectKey(tag: string): PDFName {
+    const { XObject } = this.normalizedEntries();
+    return XObject.uniqueKey(tag);
+  }
+
+  newXObject(tag: string, xObjectRef: PDFRef): PDFName {
+    const key = this.newXObjectKey(tag);
+    this.setXObject(key, xObjectRef);
+    return key;
+  }
+
   setExtGState(name: PDFName, extGStateRef: PDFRef | PDFDict): void {
     const { ExtGState } = this.normalizedEntries();
     ExtGState.set(name, extGStateRef);
   }
 
+  newExtGStateKey(tag: string): PDFName {
+    const { ExtGState } = this.normalizedEntries();
+    return ExtGState.uniqueKey(tag);
+  }
+
+  newExtGState(tag: string, extGStateRef: PDFRef | PDFDict): PDFName {
+    const key = this.newExtGStateKey(tag);
+    this.setExtGState(key, extGStateRef);
+    return key;
+  }
+
   ascend(visitor: (node: PDFPageTree | PDFPageLeaf) => any): void {
     visitor(this);
     const Parent = this.Parent();

Diff for: tests/api/PDFPage.spec.ts

@@ -1,17 +1,20 @@
-import { PDFArray, PDFDocument, PDFName } from 'src/index';
+import fs from 'fs';
+import { PDFArray, PDFDocument, PDFName, StandardFonts } from 'src/index';
+
+const birdPng = fs.readFileSync('assets/images/greyscale_bird.png');
 
 describe(`PDFDocument`, () => {
-  describe.only(`getSize() method`, () => {
-    it(`Returns the width and height of the the page's MediaBox`, async () => {
+  describe(`getSize() method`, () => {
+    it(`returns the width and height of the the page's MediaBox`, async () => {
       const pdfDoc = await PDFDocument.create();
       const page = pdfDoc.addPage();
       page.node.set(PDFName.MediaBox, pdfDoc.context.obj([5, 5, 20, 50]));
       expect(page.getSize()).toEqual({ width: 15, height: 45 });
     });
   });
 
-  describe.only(`setSize() method`, () => {
-    it(`Sets the width and height of only the the page's MediaBox when the other boxes are not defined`, async () => {
+  describe(`setSize() method`, () => {
+    it(`sets the width and height of only the the page's MediaBox when the other boxes are not defined`, async () => {
       const pdfDoc = await PDFDocument.create();
       const page = pdfDoc.addPage();
 
@@ -33,7 +36,7 @@ describe(`PDFDocument`, () => {
       expect(page.node.ArtBox()).toBeUndefined();
     });
 
-    it(`Sets the width and height of the the page's CropBox, BleedBox, TrimBox, and ArtBox when they equal the MediaBox`, async () => {
+    it(`sets the width and height of the the page's CropBox, BleedBox, TrimBox, and ArtBox when they equal the MediaBox`, async () => {
       const pdfDoc = await PDFDocument.create();
       const page = pdfDoc.addPage();
 
@@ -64,7 +67,7 @@ describe(`PDFDocument`, () => {
       expect(page.getArtBox()).toEqual({ x: 5, y: 5, width: 15, height: 45 });
     });
 
-    it(`Does not set the width and height of the the page's CropBox, BleedBox, TrimBox, or ArtBox when they do not equal the MediaBox`, async () => {
+    it(`does not set the width and height of the the page's CropBox, BleedBox, TrimBox, or ArtBox when they do not equal the MediaBox`, async () => {
       const pdfDoc = await PDFDocument.create();
       const page = pdfDoc.addPage();
 
@@ -95,4 +98,54 @@ describe(`PDFDocument`, () => {
       expect(page.getArtBox()).toEqual({ x: 0, y: 5, width: 20, height: 25 });
     });
   });
+
+  // https://github.com/Hopding/pdf-lib/issues/1075
+  it(`drawImage() does not reuse existing XObject keys`, async () => {
+    const pdfDoc1 = await PDFDocument.create();
+    const image1 = await pdfDoc1.embedPng(birdPng);
+    const page1 = pdfDoc1.addPage();
+
+    expect(page1.node.normalizedEntries().XObject.keys().length).toEqual(0);
+    page1.drawImage(image1);
+    expect(page1.node.normalizedEntries().XObject.keys().length).toEqual(1);
+
+    const key1 = page1.node.normalizedEntries().XObject.keys()[0];
+
+    const pdfDoc2 = await PDFDocument.load(await pdfDoc1.save());
+    const image2 = await pdfDoc2.embedPng(birdPng);
+    const page2 = pdfDoc2.getPage(0);
+
+    expect(page2.node.normalizedEntries().XObject.keys().length).toEqual(1);
+    page2.drawImage(image2);
+    expect(page2.node.normalizedEntries().XObject.keys().length).toEqual(2);
+
+    const key2 = page2.node.normalizedEntries().XObject.keys()[1];
+    expect(key1).not.toEqual(key2);
+    expect(page2.node.normalizedEntries().XObject.keys()).toEqual([key1, key2]);
+  });
+
+  // https://github.com/Hopding/pdf-lib/issues/1075
+  it(`setFont() does not reuse existing Font keys`, async () => {
+    const pdfDoc1 = await PDFDocument.create();
+    const font1 = await pdfDoc1.embedFont(StandardFonts.Helvetica);
+    const page1 = pdfDoc1.addPage();
+
+    expect(page1.node.normalizedEntries().Font.keys().length).toEqual(0);
+    page1.setFont(font1);
+    expect(page1.node.normalizedEntries().Font.keys().length).toEqual(1);
+
+    const key1 = page1.node.normalizedEntries().Font.keys()[0];
+
+    const pdfDoc2 = await PDFDocument.load(await pdfDoc1.save());
+    const font2 = await pdfDoc2.embedFont(StandardFonts.Helvetica);
+    const page2 = pdfDoc2.getPage(0);
+
+    expect(page2.node.normalizedEntries().Font.keys().length).toEqual(1);
+    page2.setFont(font2);
+    expect(page2.node.normalizedEntries().Font.keys().length).toEqual(2);
+
+    const key2 = page2.node.normalizedEntries().Font.keys()[1];
+    expect(key1).not.toEqual(key2);
+    expect(page2.node.normalizedEntries().Font.keys()).toEqual([key1, key2]);
+  });
 });

Diff for: tests/api/form/PDFRadioGroup.spec.ts

@@ -57,7 +57,7 @@ describe(`PDFRadioGroup`, () => {
     expect(historicalFigures.isOffToggleable()).toBe(false);
   });
 
-  it.only(`supports mutualExclusion=true`, async () => {
+  it(`supports mutualExclusion=true`, async () => {
     const pdfDoc = await PDFDocument.create();
     const page = pdfDoc.addPage();
     const form = pdfDoc.getForm();
@@ -103,7 +103,7 @@ describe(`PDFRadioGroup`, () => {
     expect((opt.get(3) as PDFHexString).decodeText()).toBe('qux');
   });
 
-  it.only(`supports mutualExclusion=false`, async () => {
+  it(`supports mutualExclusion=false`, async () => {
     const pdfDoc = await PDFDocument.create();
     const page = pdfDoc.addPage();
     const form = pdfDoc.getForm();

Diff for: tests/core/objects/PDFDict.spec.ts

@@ -138,7 +138,7 @@ describe(`PDFDict`, () => {
     );
   });
 
-  it(`return "undefined" if the underlying value is "PDFNull"`, () => {
+  it(`returns "undefined" if the underlying value is "PDFNull"`, () => {
     const dict = context.obj({ foo: null });
     dict.set(PDFName.of('Bar'), PDFNull);
     context.assign(PDFRef.of(21), PDFNull);
@@ -164,4 +164,32 @@ describe(`PDFDict`, () => {
     expect(dict.lookupMaybe(PDFName.of('Bar'), PDFDict)).toBe(undefined);
     expect(dict.lookupMaybe(PDFName.of('qux'), PDFDict)).toBe(undefined);
   });
+
+  // https://github.com/Hopding/pdf-lib/issues/1075
+  it(`can generate new keys that don't conflict with existing ones`, () => {
+    const anotherContext = PDFContext.create();
+    const anotherDict = anotherContext.obj({});
+    const anotherKey = anotherDict.uniqueKey();
+
+    const dict = context.obj({});
+    expect(dict.keys().length).toBe(0);
+
+    dict.set(anotherKey, context.obj('boing'));
+    expect(dict.keys().length).toBe(1);
+
+    const key1 = dict.uniqueKey();
+    dict.set(key1, context.obj('beep'));
+    expect(dict.keys().length).toBe(2);
+
+    const key2 = dict.uniqueKey();
+    dict.set(key2, context.obj('boop'));
+    expect(dict.keys().length).toBe(3);
+
+    const key3 = dict.uniqueKey();
+    dict.set(key3, context.obj('baap'));
+    expect(dict.keys().length).toBe(4);
+
+    expect(new Set(dict.keys()).size).toBe(4);
+    expect(dict.keys()).toEqual([anotherKey, key1, key2, key3]);
+  });
 });