search

boazsegev / combine_pdf

A Pure ruby library to merge PDF files, number pages and maybe more...
MIT License
734 stars 155 forks source link

raise_on_encrypted: true #177

Closed leviwilson closed 1 year ago

leviwilson commented 4 years ago

This pull request adds a raise_on_encrypted: true option to CombinePDF.load. In this manner, a consumer can choose to have CombinePDF throw an exception in the event that the PDF they are trying to open is encrypted.

In addition to this, sometimes during the CombinePDF::PDFParser#parse, it can come across a scenario where it raises a Zlib::DataError but was still able to obtain some information about the file in the root_object such as whether or not there is root_object[:Encrypt] information in it. For those conditions, rather than raising a Zlib::DataError it will raise the CombinePDF::EncryptionError instead but only if the raise_on_encrypted: true has been set. Otherwise, it'll allow the Zlib::DataError to bubble up.

leviwilson commented 4 years ago

@boazsegev any feedback on these changes?

boazsegev commented 4 years ago

Sorry, busy days... I'll look it over soon... I hope.

takahashim commented 3 years ago

This PR is great! 👏 The :raise_on_encrypted option and CombinePDF::EncryptionError are exactly what I was looking for. I'm very happy to see these two features merged.

On the other hand, there are some things I've been wondering about in the PR.

leviwilson commented 3 years ago
  • test/combine_pdf/load_test.rb is spec style, but test/combine_pdf/renderer_test.rb is Minitest::Test style. I don't think it's very desirable to have two specs with different styles in there.

I've never used Minitest before but the load_test.rb had more verifications which left me seeking additional Minitest functionality, but I still stuck within Minitest

  • Fixing the Rakefile for test runs is great, but it solves a different problem than encryption and EncryptionError. I think it would be better to separate it into another Pull Request.

It's still related to the PR since when I went to run specs, there was no way to run all of them. Thought it would be best to make it a default rake task like other projects.

  • Ditto for adding the .travis.yml. I'm indifferent. Seems innocuous enough to not warrant another PR.

Thanks for the feedback / looking at it!

leviwilson commented 3 years ago

@boazsegev just checking in on this. It look good?

reiz commented 2 years ago

@boazsegev @leviwilson that would be a great feature.

kimyu92 commented 1 year ago

@boazsegev Do you mind to take a look on this feature to at least allow user to stop proceeding until a better decryption is implemented?

There is no way user can stop proceeding encrypted pdf without able to ingest flag

boazsegev commented 1 year ago

Thank you every one for the work you put in on this :)

I'll test and push the patch as soon as I have a moment.

boazsegev commented 1 year ago

released @ 1.0.23