Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plainAddPlaceholder breaks the PDF file with existing annotations #198

Open
symbianm opened this issue Oct 20, 2023 · 4 comments
Open

plainAddPlaceholder breaks the PDF file with existing annotations #198

symbianm opened this issue Oct 20, 2023 · 4 comments

Comments

@symbianm
Copy link

Describe the bug and the expected behaviour
When using a file with the existing form annotations and adding a placeholder using plainAddPlaceholder it breaks the PDF file.
Acrobat Reader gives an error. MacO's preview misses all text. But Chrome opens it okay.
Results are random and it depends on which annotations it has.

Screenshot 2023-10-20 at 5 52 21 Screenshot 2023-10-20 at 5 51 36

Is it a bug in signing or in the helpers?
Issue with @signpdf/placeholder-plain package

To Reproduce
Using the slightly modified example from here:
https://github.com/vbuch/node-signpdf/blob/develop/packages/examples/src/javascript.js

const fs = require('fs');
const signpdf = require('@signpdf/signpdf').default;
const { plainAddPlaceholder } = require('@signpdf/placeholder-plain');
const { P12Signer } = require('@signpdf/signer-p12');

const signPdf = async () => {
  const p12Buffer = fs.readFileSync(`${__dirname}/certificate.p12`);
  const pdfBuffer = fs.readFileSync(`${__dirname}/document2.pdf`);

  // The PDF needs to have a placeholder for a signature to be signed.
  const pdfWithPlaceholder = plainAddPlaceholder({
    pdfBuffer,
    reason: 'Digitally signing PDF by Me.',
    contactInfo: 'test@test.com',
    name: 'Test',
    location: 'Fake Address',
  });

  // pdfWithPlaceholder is now a modified buffer that is ready to be signed.
  const signer = new P12Signer(p12Buffer);
  const signedPDfBuffer = await signpdf.sign(pdfWithPlaceholder, signer);

  fs.writeFileSync(`${__dirname}/signed-document.pdf`, signedPDfBuffer);
};

signPdf();

The certificate.p12 is from your repo https://github.com/vbuch/node-signpdf/blob/develop/resources/certificate.p12
PDF files:
document2.pdf
document.pdf

@vbuch
Copy link
Owner

vbuch commented Oct 20, 2023

Hi @symbianm ,
Thank you for the repro. Your document is PDF-1.7 which placeholder-plain notoriusly cannot handle as it includes streams ( see PDF > 1.3 ). There are some workarounds that I can think of:

  1. If you have control over the generation of that PDF, you may generate it with a version <= 1.3 or 1.7 but without streams. This will allow placeholder-plain to work with it.
  2. Try and work with pdf-lib. This example was written with the previous version of @signpdf node-signpdf but things will generally work the same way. Additionally a @signpdf/placeholder-pdf-lib could be a great addition to @signpdf so... PRs are welcome.

@vbuch vbuch linked a pull request Nov 8, 2023 that will close this issue
@vbuch vbuch removed a link to a pull request Nov 8, 2023
@vbuch
Copy link
Owner

vbuch commented Nov 10, 2023

#79 (comment)

@symbianm
Copy link
Author

symbianm commented Nov 13, 2023

Hi @symbianm , Thank you for the repro. Your document is PDF-1.7 which placeholder-plain notoriusly cannot handle as it includes streams ( see PDF > 1.3 ). There are some workarounds that I can think of:

  1. If you have control over the generation of that PDF, you may generate it with a version <= 1.3 or 1.7 but without streams. This will allow placeholder-plain to work with it.
  2. Try and work with pdf-lib. This example was written with the previous version of @signpdf node-signpdf but things will generally work the same way. Additionally a @signpdf/placeholder-pdf-lib could be a great addition to @signpdf so... PRs are welcome.

Hi @vbuch sorry for the delay, and thanks for the details.
Unfortunately, I have no control over the PDF generator and can't change the PDF version. And yes, I found that example too, but it required some adjustments because it removes all existing annotations when it adds signature annotation.

Here is my version if someone is looking for a resolution:

const fs = require('fs');
const signpdf = require('@signpdf/signpdf').default;
const utils = require('@signpdf/utils');
const { P12Signer } = require('@signpdf/signer-p12');
const {
  PDFDocument,
  PDFName,
  PDFNumber,
  PDFHexString,
  PDFString,
  PDFArray,
} = require('pdf-lib');
// Custom code to add Byterange to PDF
const PDFArrayCustom = require('./pdfArrayCustom.helper');

const p12Buffer = fs.readFileSync(`${__dirname}/certificate.p12`);

const signPdfService = async (buffer) => {
  const pdfDoc = await PDFDocument.load(buffer);
  const pages = pdfDoc.getPages();

  const ByteRange = PDFArrayCustom.withContext(pdfDoc.context);
  ByteRange.push(PDFNumber.of(0));
  ByteRange.push(PDFName.of(utils.DEFAULT_BYTE_RANGE_PLACEHOLDER));
  ByteRange.push(PDFName.of(utils.DEFAULT_BYTE_RANGE_PLACEHOLDER));
  ByteRange.push(PDFName.of(utils.DEFAULT_BYTE_RANGE_PLACEHOLDER));

  const signatureDict = pdfDoc.context.obj({
    Type: 'Sig',
    Filter: 'Adobe.PPKLite',
    SubFilter: 'adbe.pkcs7.detached',
    ByteRange,
    Contents: PDFHexString.of('A'.repeat(p12Buffer.byteLength * 2)),
    Reason: PDFString.of('Digitally verifiable PDF document signed by On Q.'),
    M: PDFString.fromDate(new Date()),
  });
  const signatureDictRef = pdfDoc.context.register(signatureDict);

  const lastPage = pages[pages.length - 1];
  const widgetDict = pdfDoc.context.obj({
    Type: 'Annot',
    Subtype: 'Widget',
    FT: 'Sig',
    Rect: [0, 0, 0, 0],
    V: signatureDictRef,
    T: PDFString.of('Signature1'),
    F: 4,
    P: lastPage.ref,
    DR: pdfDoc.context.obj({}),
  });
  const widgetDictRef = pdfDoc.context.register(widgetDict);

  // keep older annotations
  try {
    const annots = lastPage.node.lookup(PDFName.of('Annots'), PDFArray);
    annots.push(widgetDictRef);
    // Add our signature widget to the first page
    lastPage.node.set(PDFName.of('Annots'), pdfDoc.context.obj(annots));
  } catch (err) {
    lastPage.node.set(
      PDFName.of('Annots'),
      pdfDoc.context.obj([widgetDictRef]),
    );
  }

  // Create an AcroForm object containing our signature widget
  pdfDoc.catalog.set(
    PDFName.of('AcroForm'),
    pdfDoc.context.obj({
      SigFlags: 3,
      Fields: [widgetDictRef],
    }),
  );

  const modifiedPdfBytes = await pdfDoc.save({ useObjectStreams: false });
  const modifiedPdfBuffer = Buffer.from(modifiedPdfBytes);

  const signer = new P12Signer(p12Buffer);
  return signpdf.sign(modifiedPdfBuffer, signer);
};

module.exports = signPdfService;

@vbuch
Copy link
Owner

vbuch commented Nov 14, 2023

Thank you for the input @symbianm It was just the thing I was about to do on the placeholder-pdf-lib PR. Here is my take on it and here is how I test this

Once I rewrite the history so it makes more sense, I'll be welcoming reviews on this PR.

I think it's important to note that currently pdf-lib does not provide incremental updates so it is very likely that adding a new signature on top of the previous one invalidates the first one.

vbuch added a commit that referenced this issue Nov 14, 2023
* Use pdf-lib 1.17 types;
* Use the newly introduced FLAG enums;
* Reuse existing AcroForm (refs #198);
* Reuse existing Annots (refs #198);
* Fix mistaken signature length;
vbuch added a commit that referenced this issue Nov 14, 2023
* Use pdf-lib 1.17 types;
* Use the newly introduced FLAG enums;
* Reuse existing AcroForm (refs #198);
* Reuse existing Annots (refs #198);
* Fix mistaken signature length;
vbuch pushed a commit that referenced this issue Nov 21, 2023
* Use pdf-lib 1.17 types;
* Use the newly introduced FLAG enums;
* Reuse existing AcroForm (refs #198);
* Reuse existing Annots (refs #198);
* Fix mistaken signature length;
vbuch pushed a commit that referenced this issue Nov 21, 2023
* Use pdf-lib 1.17 types;
* Use the newly introduced FLAG enums;
* Reuse existing AcroForm (refs #198);
* Reuse existing Annots (refs #198);
* Fix mistaken signature length;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants