chore: 10 polish LOW fixes from audit cluster#94
Merged
Conversation
Uncaught exceptions raised from queued Qt slots in PyInstaller --windowed builds (console=False on Windows/macOS) were written to a stderr stream pointed at NUL/dev/null and never reached pdfapps.log. Users got a silent UI freeze with zero diagnostic trail (R7 I2). Route them through the logging framework with exc_info= so the rotating handler captures the traceback, and surface a best-effort QMessageBox so the failure is visible. KeyboardInterrupt keeps the default behaviour so Ctrl-C in console builds still exits cleanly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
A corrupt config.json was silently overwritten with {} on the next
_update_config call, wiping language / dark_mode / recents with no
warning to the user or support trail (R8 N1).
Distinguish 'missing file' (normal first run) from 'unparseable file'
and, in the latter case, copy the broken file aside to
<config>.corrupt.bak before resetting. Use contextlib.suppress so a
read-only profile or permission error still lets the app start with
default settings instead of failing hard.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Unicode strings flowing into pypdf PdfReader.decrypt and PyMuPDF Document.authenticate were used as-is. A password typed on macOS (NFD-leaning) and copied to a Windows machine (NFC-leaning) failed to authenticate even though the on-screen glyphs were identical (R6 C1). Add a static _nfc helper and run every cached password through it at the three entry points (prompt-and-cache, _open_reader, _open_fitz). NFC matches what Windows/Linux clipboards produce and what most PDF producers wrote pre-PDF-2.0; we still don't SASLprep (PDF 2.0 only) — that's a separate, higher-risk change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The compact-mode 'Change source' link installs a lambda that calls self.set_compact_mode(False). If the page is destroyed between the Qt click event being queued and the slot actually running, the lambda touches a dead C++ QWidget and Qt logs 'Internal C++ object already deleted' / crashes (R6 O2). PySide6 has no QPointer; gate the slot on shiboken6.isValid(self), which is already imported in this module. Cost is one isValid call per compact-mode exit — negligible. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Without setTabOrder Qt walked the dialog's children in construction order (icon, title, password, Cancel, OK) and on some platforms also landed initial focus on the Cancel button. A keyboard-only user had to press Tab three times to reach the password field they came here to fill in (R11 G1). Anchor focus on the password field and force the cycle password -> OK -> Cancel so Enter / Tab match the expected flow. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
_TextEditDialog asked the user for replacement text via a QTextEdit (multi-line), but page.insert_text in PyMuPDF only supports single-line strings — any newline got rasterized as a '?' glyph in the output PDF (R11 B2). Switch to QLineEdit so the input is restricted to what the writer can actually emit; wire returnPressed to accept the dialog for consistency with the password dialog. Simpler than implementing a per-line iteration on insert_text in the editor and matches the existing _TextDialog (insert) behavior. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Dropping a folder onto the main window used to silently open every .pdf inside (R10 review nit). A folder with 200 PDFs flooded the tab bar, locked the UI for several seconds while each tab loaded, and gave the user no way to back out. Show a Yes/No QMessageBox when the recursive list exceeds 20 files so the user can confirm or skip. Default is No so accidental drops on archive folders do nothing surprising. i18n: add viewer.drop_many_pdfs_confirm and widgets.drop_first_only across all 8 supported languages. The latter ships with this commit so the JSON stays parity-clean; it is consumed by the next commit (DropFileEdit multi-URL warning). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
DropFileEdit is a single-file widget but used to silently take only urls[0] when the user dragged multiple files in. The extra files vanished with no feedback (R11 N3). Filter to URLs that pass the extension filter first so dragging one PDF plus an unrelated screenshot still counts as 'one PDF dropped' and skips the warning. When more than one accepted file is present, surface a QMessageBox.information with the count using the new widgets.drop_first_only key (added in the previous commit). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The R10 #6 guard rejected the 'no /AcroForm dict at all' case but missed the rarer one where /AcroForm exists with zero widget entries — e.g. files whose fields were flattened by a third-party tool but the dict was left behind. update_page_form_field_values then ran a silent no-op and the user got no feedback that nothing changed (R10 review follow-up). Reuse _r.get_fields() (already cached by pypdf when the form table was first populated) and surface the same editor.forms.no_fields status the plain-PDF branch uses. Avoids importing fitz just to count widgets and keeps the messaging consistent for the user. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
parse_pages used to raise ValueError on inputs like '3-' because
int('') failed inside the split-on-'-' branch. PR-G surfaced the
friendly bad_page_input message but the underlying behaviour stayed
unhelpful — most users typed '3-' expecting 'from page 3 to the
end', matching pdftk's familiar 1- syntax (R7 E5 follow-up).
Restructure the per-part loop so missing-side ranges fall back to
1 (left) or total (right). '-' on its own is still invalid (no
bounds at all). Update the existing open-range test in
test_audit_r11.py to use 'abc-' as the regression target since
'1-' is now intentionally valid.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
16 tests covering all ten PR-H bug fixes: #1 pdfapps.py installs sys.excepthook #2 i18n backs up corrupt config.json before reset #3 base.py NFC-normalizes passwords #4 _PdfPasswordDialog explicit tab order #5 window confirms folder drop >20 PDFs #6 editor zero-widget AcroForm short-circuits #7 _TextEditDialog uses QLineEdit #8 DropFileEdit warns on multiple URLs #9 set_compact_mode lambda guarded by shiboken6.isValid #10 parse_pages accepts '3-' / '-5' open ranges, still rejects '-' Plus two i18n parity guards verifying both new translation keys (viewer.drop_many_pdfs_confirm, widgets.drop_first_only) land in all eight supported languages with the {count} placeholder intact. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Round 11 review caught that the previous NFC fix only normalized at
read time in the BasePage helpers (_maybe_prompt_password, _open_reader,
_open_fitz). The cached self._pdf_password attribute was still set raw
at 2-3 sites (editor/tab.py, viewer/panel.py, plus the prompt path in
base.py itself), so any tool that reads self._pdf_password directly
(merge, watermark, ocr, page_numbers, convert ×8, nup, plus ~17 other
call sites) still saw the un-normalized value.
The fix lives in app.utils.normalize_password (single source of truth)
and gets applied at every WRITE site: BasePage._maybe_prompt_password,
EditorTab._load_pdf, PdfViewerPanel._open_path. BasePage._nfc is kept
as a thin delegator so subclasses calling self._nfc(...) keep working
and the three read-side helpers stay defensive against externally-set
caches. By normalizing at write time the attribute becomes deterministic
and every consumer is covered without instrumenting each tool.
Also pulled in two adjacent polish items the same review flagged:
B5: corrupt config.json backups now suffixed with a wall-clock
timestamp (.corrupt-YYYYMMDD_HHMMSS.bak) so successive corruption
events do not clobber prior evidence.
F6: _TextEditDialog now collapses \r/\n in the pre-filled old_text
before QLineEdit.setText, since QLineEdit silently truncates at
the first newline and would hide the rest of the detected string
from the user.
Tests: test_polish_lows.py grows from 19 to 22 cases — three new
write-site source assertions (base/editor/viewer) plus a behavioral
NFD→NFC round-trip through the cache; the existing _nfc(pwd) source
assertion was tightened to require all three defensive read-side
calls plus the normalize_password write call. F6 is covered by two
new substring assertions inside the _TextEditDialog source block.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
nelsonduarte
added
bug
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic Password | 1f3d31a | app/utils.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "config.json appeared corrupt; saved backup " | ||
| "to %s before reset", backup_path, | ||
| ) | ||
| except OSError: |
| None, "Unexpected error", | ||
| f"{exc_type.__name__}: {exc_value}\n\nDetails logged.", | ||
| ) | ||
| except Exception: |
| import unicodedata | ||
|
|
||
| nfd = "passé" # 'e' + combining acute | ||
| nfc = "passé".encode("utf-8") |
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
2 participants
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.
Summary
10 polish/LOW-severity fixes accumulated across audit Rounds 6-11. Low risk, focused improvements: 1 security helper (excepthook), 1 dataloss prevention (config backup), 1 i18n correctness (NFC normalization), and 7 UX/accessibility nits.
Fixes
sys.excepthook(pdfapps.py) — Uncaught Qt exceptions in--windowedbuilds went to closed stderr. Now logged vialogging.critical(exc_info=...)with best-effort QMessageBox.config.jsonbackup on corruption (app/i18n.py) — Previously silently reset to{}on JSON parse failure. Now preserved as.corrupt-{timestamp}.bakbefore reset.app/base.py,app/editor/tab.py,app/viewer/panel.py,app/utils.py) — Passwords copied from macOS (NFD) failed on Windows. Now normalized at every cache write-site via new module-levelnormalize_password()so all 17 downstream consumers (merge, watermark, ocr, etc.) see the consistent NFC form without instrumenting each tool._PdfPasswordDialogtab order — Was Cancel -> OK -> password. Now password -> OK -> Cancel withsetFocus()on the edit._TextEditDialogsingle-line —QTextEditreplaced withQLineEditto match the single-line fitzinsert_textcapability. Pre-extracted text with newlines now collapsed to spaces so user can still edit.app/window.py) — Drops with >20 PDFs prompt for confirmation (default=No) to avoid flooding the UI.DropFileEditmulti-URL warning — Was silently discarding extra URLs. Now shows info dialog explaining only the first was loaded.app/editor/tab.py) — Forms with/AcroFormdict but no fields now surfaceeditor.forms.no_fieldscleanly instead of running a no-op silently.set_compact_modelambda isValid guard (app/base.py) — Lambda capturedself; could hit destroyed C++ object if page closed before click. Nowshiboken6.isValid(self)guarded.parse_pagesopen-ended ranges (app/utils.py) — Input "3-" now means "3 to total" (was raisingValueError). Standalone "-" still raises friendly translated error.i18n
2 new keys x 8 languages:
viewer.drop_many_pdfs_confirm(with{count}placeholder)widgets.drop_first_only(with{count}placeholder)Parity verified: 8 x 599 keys.
Tests
tests/test_polish_lows.py— 19 tests (16 initial + 3 added during review for NFC write-site coverage). All pass.Updated tests:
test_audit_r11.py::test_parse_pages_raises_friendly_message_for_open_rangenow uses "abc-" target since "3-" became valid.Final: 246 passed, 1 failed (
test_flatpak_manifest_tag_is_currentpre-existing), 1 skipped.Adversarial review notes
Review caught that the original NFC fix only normalized at the 3
BasePagehelper read-sites. The cache attributeself._pdf_passwordwas still being written raw ateditor/tab.py:624andviewer/panel.py:513, leaving ~17 downstream call sites exposed. Fixed in commit 1f3d31a by promoting_nfcto module-levelnormalize_password()and normalizing at write time. Two minor polish items were also applied: backup file is now timestamped (preserves history of multiple corruptions), and_TextEditDialogcollapses newlines so pre-extracted multi-line text remains editable.Validation
APP_VERSIONbump