Avoid password prompt on attachment-only encryption with no attachments #20140
Conversation
Aditi-1400
force-pushed
the
fix-password
branch
from
74bf518 to
ae2ccf9
Compare
|
The PR title and test name are wrong, right? It should be more like "Avoid password prompt when encryption only affects attachments and there are none" |
Hmm, let me frame it better. |
src/core/xref.js
Outdated
| stmF === "Identity" && | ||
| strF === "Identity" && | ||
| eff !== "Identity" && | ||
| cryptFilters.get(eff)?.get("CFM") !== "None" |
There was a problem hiding this comment.
If cryptFilters.get(eff) is undefined, this last line will result in true and not false. Is it intended?
There was a problem hiding this comment.
No, it wasn't, fixed it
| const names = root.get("Names"); | ||
| if (names instanceof Dict && names.has("EmbeddedFiles")) { | ||
| const nameTree = new NameTree(names.getRaw("EmbeddedFiles"), this); | ||
| const attachments = nameTree.getAll(); |
There was a problem hiding this comment.
Not sure about how much performance matters in this case, but we could have a nameTree.isEmpty() method to avoid having to extract all the entries just to check if any exists.
Aditi-1400
force-pushed
the
fix-password
branch
from
ae2ccf9 to
0d430b7
Compare
Aditi-1400
changed the title
Aditi-1400
changed the title
| @@ -143,6 +128,52 @@ class XRef { | |||
| } | |||
| warn(`XRef.parse - Invalid "Root" reference: "${ex}".`); | |||
| } | |||
|
|
|||
| if (encrypt instanceof Dict) { | |||
There was a problem hiding this comment.
As far as I can tell all this stuff is useless and only the AuthEvent entry should be checked:
https://christianhaider.de/dokuwiki/lib/exe/fetch.php?media=pdf:pdf32000_2008.pdf#page=76&zoom=auto,-60,624
There was a problem hiding this comment.
From what I understand, It looks like AuthEvent checks for when to check for encryption but doesn't really tell us what parts are encrypted, which is what we need to check if attachments are the only thing that's encrypted
There was a problem hiding this comment.
According to the AuthEvent value we should only ask for the password when the user wants to access to the attachments (EFOpen) or when the doc is open (DocOpen).
So if the value is EFOpen and there are no attachments, then the user won't be prompted.
There was a problem hiding this comment.
I spent some time digging into this today, and based on what the spec says, I agree we can simplify things here. For PDFs with encrypted attachments, there appear to be three relevant cases:
- The PDF has
/AuthEvent /EFOpen, and includes attachments: (Issue [Bug]: File Attachments do not open when they are encrypted #20139) - It has
/AuthEvent /EFOpenbut no attachments, so we still need to check if any are actually present. - There's No
/AuthEvent, but it uses/StmF /Identity,/StrF /Identity, and/EFF /StdCF, this is a very specific case and not something we see in practice, I tried to generate a PDF that hits this edge-case, but I ended up with a PDF where the entries were/StmF /Identity,/StrF /Identity, and/EFF /StdCFand there was no/AuthEvent, but the content of the attachments was not actually encrypted.
So now, I’ve removed the extra checks for /StmF, /StrF, and /EFF. We're only checking for /AuthEvent and whether the file actually has attachments.
Aditi-1400
force-pushed
the
fix-password
branch
4 times, most recently
from
dc57108 to
0f1a284
Compare
… attachments PDF.js wrongly prompted for passwords when a PDF had no attachments but had an encryption enabled that was limited to only attachments leaving all page, image and metadata streams in clear text. The encryption dictionary is now discarded for this case. This prevents unnecessary prompts for PDFs with no protected content beyond attachments
Aditi-1400
force-pushed
the
fix-password
branch
from
0f1a284 to
995f6f2
Compare
|
Added the test PDF separately, since the CI wasn't downloading the PDF from the link for some reason? |
| ) { | ||
| let hasEncryptedAttachments = false; | ||
| if (root instanceof Dict) { | ||
| const names = root.get("Names"); |
There was a problem hiding this comment.
I don't understand why you're doing that.
If there's no attachment then the user cannot click on them and the encryption stuff will never be used.
And if there are attachments then we've to only decrypt them when the user request to read them.
There is a bug (we talked about with @timvandermeij) about having to only load (and decrypt) the attachments when the user requests them, because right now we get them all the time.
Fixes: #20049
PDF.js wrongly prompted for passwords when a PDF had no attachments but had an encryption enabled that was limited to only attachments leaving all page, image and metadata streams in clear text.
The encryption dictionary is now discarded for this case. This prevents unnecessary prompts for PDFs with no protected content beyond attachments.
Related issues: #20139