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

Encrypt can also be a Dictionary #783

Closed
wants to merge 2 commits into from
Closed

Encrypt can also be a Dictionary #783

wants to merge 2 commits into from

Conversation

samalloing
Copy link
Collaborator

Hi @carlwilson

Encrypt can be also be a PdfDictionary (also used Arlington PDF model to verify)

A testfile: https://github.com/mozilla/pdf.js/blob/master/test/pdfs/issue6010_1.pdf

Sam

@carlwilson carlwilson added this to the JHOVE 1.28 milestone Nov 30, 2022
@carlwilson carlwilson self-assigned this Nov 30, 2022
@carlwilson carlwilson added bug A product defect that needs fixing P2 Medium priority issues to be scheduled in a future release P1 High priority issues to be scheduled in the upcoming release and removed P2 Medium priority issues to be scheduled in a future release labels Nov 30, 2022
@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Base: 46.33% // Head: 46.32% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (7f84e7b) compared to base (eadc199).
Patch has no changes to coverable lines.

❗ Current head 7f84e7b differs from pull request most recent head 2f22a39. Consider uploading reports for the commit 2f22a39 to get more accurate results

Additional details and impacted files
@@                Coverage Diff                @@
##             integration     #783      +/-   ##
=================================================
- Coverage          46.33%   46.32%   -0.02%     
+ Complexity          1054     1053       -1     
=================================================
  Files                 57       57              
  Lines               9079     9079              
  Branches            1612     1609       -3     
=================================================
- Hits                4207     4206       -1     
- Misses              4333     4335       +2     
+ Partials             539      538       -1     
Impacted Files Coverage Δ
...a/edu/harvard/hul/ois/jhove/NisoImageMetadata.java 75.99% <0.00%> (-0.20%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

_encryptDictRef = (PdfIndirectObj) _trailerDict
.get(DICT_KEY_ENCRYPT); // This is at least v. 1.1
_encrypted = (_encryptDictRef != null);
PdfObject encryptObj = _trailerDict.get(DICT_KEY_ENCRYPT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a solid approach, but the if/else leaves the possibility that the instance returned here isn't "tested" at all. Put another way, what happens when encryptObj ISN'T an instance of PdfIndirectObj OR PdfDictionary? This may be an error that gets picked up elsewhere, but I wonder if there shouldn't be a default condition here that marks an error?

@carlwilson
Copy link
Member

carlwilson commented Nov 30, 2022

@samalloing I've also created PR #803, which sets things up for this change. I've added you as a reviewer anyway, but the two want merging before trying to merge into integration. I'm good to handle that but want to keep them separate until the review is resolved. I actually merged them up instead, I'll fix the tests once the changes are made.

BTW this appears to be a duplicate of #743 ? Please confirm and I'll close that PR.

@carlwilson
Copy link
Member

carlwilson commented Jan 9, 2023

@samalloing I think I've fixed these issues in #810 where I've added the changes needed to report encryption in the new case. Finally, I've added a couple of examples to the corpora and patched the tests to work.

If you could review #810 we can get these merged up.

@carlwilson
Copy link
Member

Closed as contained in #810 now, thanks @samalloing

@carlwilson carlwilson closed this Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A product defect that needs fixing P1 High priority issues to be scheduled in the upcoming release
Projects
Development

Successfully merging this pull request may close these issues.

2 participants