fix: authenticate PDF passwords with the spelling that actually unlocks the file - #174
Merged
Merged
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
…ks the file A correct password was accepted by the unlock prompt and then rejected by every tool that touched the same document. The viewer showed the document open with a blank thumbnail strip, and each tool reported a wrong password for a password the user had just typed correctly. Root cause ---------- The app normalised passwords to NFC before handing them to either engine, and NFC is a third spelling that neither engine uses. ISO 32000-2 section 7.6.4.3.3 mandates SASLprep (RFC 4013) for R6/AES-256, and the Normalize step of SASLprep is NFKC, not NFC. pypdf implements SASLprep in full since 6.12.0 and applies it whenever V >= 5 and the argument is a str. MuPDF does not normalise at all: pdf_saslprep_from_utf8 in pdf_crypt.c is a stub carrying a "TODO: stringprep with SASLprep profile" comment and copies the UTF-8 bytes verbatim. So the prompt (fitz) and the tools (pypdf) could disagree about the very same password, and NFC matched neither of them. Approach -------- New pure module app/pdf_password.py (no PySide6, no app.* import) that: * expands a typed password into a small ordered, de-duplicated candidate list: raw, SASLprep(raw), NFC(raw). NFD is deliberately absent; it is not a spelling any engine produces. * caches the spelling that actually authenticated, so every later operation reuses the exact bytes that worked instead of guessing again. * feeds both engines the same UTF-8 byte sequence, so pypdf and fitz can no longer diverge by construction for R >= 5. The encrypt tool no longer predicts the spelling its own output will carry. pypdf normalises on the write side, so the written file is now probed and the spelling that opens it is what gets cached. Password clearing was reaching a path that real documents never take, so the cache outlived the document it belonged to; it now clears on the paths documents actually follow. Wrong passwords are presented as warnings ----------------------------------------- The sites that raise on a bad password all reached show_error, which put the generic "something went wrong, see the log" text in the primary slot and hid the real translated sentence behind "Show Details", prefixed with a Python class name. A dedicated WrongPasswordError is now routed to a warning with the translated message in the primary text and no traceback pane. The generic crash path is unchanged and still logs. R <= 4 is best effort and documented in the module: the spec defers to the host code page there, so no conformance target exists and no single spelling is guaranteed to satisfy both engines. Tests: tests/test_unicode_passwords.py and tests/test_wrong_password_dialog.py cover candidate expansion and ordering, the cached-winner path, the encrypt probe, clearing, and both dialog presentations. The obsolete source-text NFC assertion in tests/test_polish_lows.py is replaced by behavioural round-trip tests against really encrypted files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nelsonduarte
force-pushed
the
fix/unicode-password-normalization
branch
from
September 2, 2026 21:09
d1f265c to
67bb98d
Compare
Deploying pdfapps with
|
| Latest commit: |
88d93c8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://91b177cd.pdfapps.pages.dev |
| Branch Preview URL: | https://fix-unicode-password-normali.pdfapps.pages.dev |
The secret scanner flags password literals in the PDF encryption tests
("s3cret", "merge-secret", "correct-horse" and friends). None of them are
credentials. Each is a throwaway string typed inline to encrypt a PDF that
the same test function generates under a pytest tmp_path and discards when
it finishes. They unlock nothing outside that temporary file and reach no
service.
The new .gitguardian.yaml lists the five files that actually contain such
literals rather than excluding tests/ wholesale. A blanket directory rule
would blind the scanner across a tree where a real secret could still land
by accident, which is the case the tool has to keep catching; naming the
files keeps it useful everywhere else and makes any future addition an
explicit decision.
Also fixes the py/import-and-import-from warning in
tests/test_unicode_passwords.py, where the encrypt module was pulled in as
"import app.tools.encrypt" while the neighbouring line used the from-import
form. The _Poison docstring now records why its __getattr__ raises
RuntimeError instead of AttributeError: AttributeError is the one exception
the getattr default swallows, so it would let the test pass against the very
regression the test exists to catch.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
A correct password was accepted by the unlock prompt and then rejected by every tool that touched the same document. The user saw the document visibly open in the viewer, with a blank thumbnail strip, while each tool insisted the password was wrong.
Root cause
The app normalised passwords to NFC before handing them to either engine. NFC is a third spelling that neither engine uses.
_encryption._saslprep, ending inunicodedata.normalize("NFKC", ...)), applied in_encode_passwordwheneverV >= 5and the argument is astr.pdf_saslprep_from_utf8inpdf_crypt.cis a stub carrying a/* TODO: stringprep with SASLprep profile */comment and copies the UTF-8 bytes verbatim.So the prompt (fitz) and the tools (pypdf) could disagree about the very same password, and NFC matched neither.
What changed
New pure module
app/pdf_password.py(noPySide6, noapp.*import, fully headless-testable):raw,SASLprep(raw),NFC(raw). NFD is deliberately absent, it is not a spelling any engine produces.bytes, which skips its SASLprep branch; fitz gets thestr, which MuPDF encodes to the same bytes), so they can no longer diverge by construction forR >= 5.Encrypt tool probes instead of predicting. pypdf normalises on the write side, so the tool no longer tries to guess the spelling its own output will carry: it probes the written file and caches the spelling that opens it.
Password clearing reaches real paths. The clearing hook was on a path real documents never take, so the cache outlived the document it belonged to.
Wrong passwords are warnings, not crashes. Every raising site reached
show_error, which put the generic "something went wrong, see the log" text in the primary slot and hid the real, already-translated sentence behind "Show Details", prefixed with a Python class name, in eight locales. A dedicatedWrongPasswordErrornow routes to a warning with the translated message in the primary text and no traceback pane. The generic crash path is unchanged and still logs.R <= 4is best effort, and documented in the module. The spec defers to the host system code page there, so no conformance target exists and no single spelling is guaranteed to satisfy both engines.How it was verified
QT_QPA_PLATFORM=offscreen.app/window.pyfrom 0% to 52%.tests/test_unicode_passwords.py(candidate expansion and ordering, cached-winner reuse, encrypt probe, clearing, real encrypted-file round trips) andtests/test_wrong_password_dialog.py(asserts what the dialog is told to show: primary text, icon, presence or absence of the details pane, plus the generic path pinned so "warn on wrong password" cannot degrade into "warn on everything").tests/test_polish_lows.pywas replaced by behavioural round-trip tests against genuinely encrypted files, since a source-text assertion cannot distinguish the two presentations.What is deliberately NOT in this PR
pypdf/cryptographybump together with the Flatpak pins inflatpak/requirements-pinned.txt.requirements.txtwas reverted here on purpose so this PR stays a behaviour fix; the bump gets its own PR._open_fitzfail-open behaviour, left as is.R <= 4non-Latin-1 passwords, unfixable without a conformance target (see above).fitztruncating at NUL, an upstream behaviour, not worked around here._passwordsites in the render classes, a pure naming refactor with a real blast radius, kept separate.CI
Only CodeQL runs on this PR.
security-deps.ymlis path-filtered torequirements.txt,flatpak/requirements-pinned.txtand its own workflow file, none of which this PR touches, so its strictpip-audit --strictgate does not fire.