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

BUG: Fail on wrong state of reserved permission bits during encryption #2421

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

stefan6419846
Copy link
Collaborator

Fixes #2401.

This change is a bit tricky, as it can be interpreted as a breaking one. We previously did not ensure that the user would use this correctly and this will throw a hard error now. On the other hand, users which would get an error now have been using this the wrong way the whole time anyway.

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.15%. Comparing base (4a41c53) to head (5f7d018).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2421   +/-   ##
=======================================
  Coverage   95.14%   95.15%           
=======================================
  Files          51       51           
  Lines        8551     8565   +14     
  Branches     1706     1711    +5     
=======================================
+ Hits         8136     8150   +14     
  Misses        261      261           
  Partials      154      154           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pubpub-zz
Copy link
Collaborator

pubpub-zz commented Jan 22, 2024

I dislike this change. as @stefan6419846 has stated it this introduced some possible errors that did not used to be. overloading __init__ or __new__ isn't there a way to force the default flags?

@stefan6419846
Copy link
Collaborator Author

overloading init or new isn't there a way to force the default flags?

I do not think that this suits the IntFlag usage, where you tend to just concatenate existing values like UAP.PRINT | UAP.ABC. With the proposed code, you can already start with UAP.minimal() and just add the desired bits/flags to it.

The general alternative would be to silently and auto-magically fix the corresponding bits in the background, but personally I prefer clean user input which gets validated and fixed by the user.

@MartinThoma
Copy link
Member

The general alternative would be to silently and auto-magically fix the corresponding bits in the background

Adding a warning as a first step would be a no-brainer. I'm undecided if R13-R32 should then (a) be auto-fixed (b) just get the warning (c) get an exception.

@pubpub-zz
Copy link
Collaborator

Adding a warning as a first step would be a no-brainer. I'm undecided if R13-R32 should then (a) be auto-fixed (b) just get the warning (c) get an exception.

First your reading of R13-R32 is correct and should be set to 1
Second, not everybody knows the PDF specification sof all defaults (bits 1-2 to 0, bits 7,8,13-32 set to 1) should be set to default values without warning nor exception.

@stefan6419846
Copy link
Collaborator Author

Second, not everybody knows the PDF specification sof all defaults (bits 1-2 to 0, bits 7,8,13-32 set to 1) should be set to default values without warning nor exception.

While this might be a breaking change, I tend to still disagree about this: With UAP.minimal() and adding the desired permissions or UAP.all() and removing the not desired permissions everyone is able to generate the correct permission flags - we should just document this properly. Whoever mangles with the reserved bits (R* values) should be aware of what this means, id est that it contradicts the official specs.

@stefan6419846 stefan6419846 added the on-hold PR requests that need clarification before they can be merged.A comment must give details label Feb 23, 2024
@stefan6419846
Copy link
Collaborator Author

Given the last feedback, I would probably go with the following approach:

  • Validate the permission bits.
  • In case of an error: Report a warning with a dedicated class and automatically fix it.

This way everyone can decide whether to propagate the warning as an error through a warning filter or whether to silence it. I have seen no indication that a general deprecation of wrong permission bits is desired for now to avoid requiring too deep knowledge of PDF internals.

@MartinThoma
Copy link
Member

MartinThoma commented Jul 20, 2024

Given the last feedback, I would probably go with the following approach:

  • Validate the permission bits.
  • In case of an error: Report a warning with a dedicated class and automatically fix it.

That doesn't fit our definition of what warnings are used for: https://pypdf.readthedocs.io/en/stable/user/suppress-warnings.html (but besides that, it sounds reasonable to me).

Maybe we should just check if self.strict is True?

That would also give the user control + we have a default that works for most people / cases.

@stefan6419846
Copy link
Collaborator Author

This would mostly apply to the writer, which AFAIK does not have a strict mode at the moment. We would have to add this if required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on-hold PR requests that need clarification before they can be merged.A comment must give details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PDF encryption: Reserved permission bits might have wrong state
3 participants