combine_pdf icon indicating copy to clipboard operation
combine_pdf copied to clipboard

Published 20 hours ago • verified publisher icon boazsegev

raise_on_encrypted: true

Open leviwilson opened this issue 4 years ago • 6 comments

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 avatar Jul 30 '20 18:07 leviwilson

@boazsegev any feedback on these changes?

leviwilson avatar Aug 18 '20 14:08 leviwilson

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

boazsegev avatar Sep 10 '20 21:09 boazsegev

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.

  • 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.
  • 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.
    • Ditto for adding the .travis.yml.

takahashim avatar Oct 01 '20 06:10 takahashim

  • 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 avatar Oct 01 '20 06:10 leviwilson

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

leviwilson avatar Nov 17 '20 15:11 leviwilson

@boazsegev @leviwilson that would be a great feature.

reiz avatar Oct 12 '21 07:10 reiz

@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

kimyu92 avatar Apr 01 '23 01:04 kimyu92

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 avatar Apr 04 '23 00:04 boazsegev

released @ 1.0.23

boazsegev avatar Apr 04 '23 01:04 boazsegev