fix: 7 HIGH audit findings (memory, security, updater, print)#78
Merged
Conversation
Long editing sessions could grow _pending unbounded; edits referencing temp signature/image files were never released until the tab closed. Add _MAX_PENDING=500 constant; _add() now trims oldest edits and unlinks their orphan temp files (only paths inside the system tempdir are touched, so user-picked source images are left alone). Undo paths already handle empty stacks correctly. The visible QListWidget shadow is trimmed in lockstep so labels stay aligned with edit indices. Addresses R5/A2.
Previously the persistent ~/.pdfapps_signature.png (and the in-flight /tmp copy produced by _SignatureDialog) were saved with the default umask (typically 0o644), allowing other users on multi-user POSIX hosts to read the cached signature. Apply os.chmod(path, 0o600) after both writes. Windows is unchanged because NTFS ACLs already inherit owner-only access from the user profile. Addresses R5/C1.
Cached _pdf_password persisted in memory until the BasePage / EditorTab / ViewerPanel object was GC'd. A heap inspection or memory dump after the user finished viewing a confidential PDF could surface the password as plain text. Add _clear_pdf_password() helper on BasePage (and a mirror on EditorTab, which does not inherit from BasePage). Wire it into: - EditorTab._close_pdf (already committed alongside the editor fix) - PdfViewerPanel.load (when switching files) - PdfViewerPanel.closeEvent (teardown) Limitation documented: Python str immutability prevents a real zero-scrub of the original bytes; the helper drops the only reachable reference and uses a ctypes zero-buffer hint as a defensive marker. Addresses R5/D2.
…ion on retry Two related lifetime bugs: * _apply_update_unix wraps the apply body in try/finally so failed applies no longer leak the ~100 MB installer tempfile. Pre-fix os.remove(downloaded) only ran on the success path, leaving the artefact in /tmp until the OS cleaned it up (R5/F3). * UpdateDialog now creates a fresh _Signals instance per _start_download call. Pre-fix the dialog reused a single _Signals across retries, reconnecting finished/error/cancelled on top of prior thread connections. When the second download completed Qt delivered the signal to both the live AND already-deleted historical threads, producing warnings or crashes (R6/B6-B7). The prior _signals instance is deleteLater'd before reassignment so historical connections are released atomically.
QPrintDialog exposes fromPage/toPage/copyCount/pageOrder but the print loop ignored all four — always printing every page once in forward order regardless of the user's choices. Build the iteration range from printer.fromPage()..toPage() (treating 0,0 as "all pages" per Qt's contract), repeat the loop copyCount() times, and reverse the page list when pageOrder() is LastPageFirst. AttributeError-guard the enum access so PySide6 < 6.4 bindings still print (worst case: forward order, same as before this fix). Page-break placement adjusted so neither the first page of the first copy nor the boundary between copies produces a blank leading page. Preserves the CRIT-3 pixmap safety fixes (alpha=False / csRGB fallback / .copy()) which live in the same loop body. Addresses R7/N7-H1.
Several tests opened "app/base.py" / "installer.py" / etc. with relative paths, breaking when pytest ran from any cwd other than the repo root (IDEs, CI matrix configurations, sub-directories, or /tmp). Migrate the affected open() calls to absolute paths anchored at _REPO_ROOT = Path(__file__).resolve().parent.parent so the suite passes from any working directory. Addresses R7/N7-H2.
The cap-trim block computed `extra = _pending_list.count() - len(_pending)` BEFORE the new edit's addItem ran. In steady state at the cap that diff was 0, so no takeItem ever fired, and the subsequent addItem left the QListWidget with +1 row vs the underlying _pending list. The next _undo then removed the wrong label (the visible most-recent row, not the row matching the popped edit). Switch the takeItem loop to use len(to_drop), which is the correct trim count regardless of whether addItem has happened yet. Adds a regression test that drives the real TabEditar._add against a real QListWidget and asserts the widget rows stay locked to the _pending list both after the cap-trim and after a subsequent _undo. Caught in PR-B adversarial review. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- wipe_pdf_password is the single source of truth in app/utils.py; BasePage._clear_pdf_password, TabEditar._clear_pdf_password and PdfViewerPanel._clear_pdf_password are now thin wrappers that delegate to it. Used to be three near-identical copies — flagged in PR-B review as DRY rot. - Remove duplicate `import contextlib` from updater.py:268 (already imported at module top). Existing password-lifecycle tests still pass (they exercise the _clear_pdf_password method on BasePage, which now routes through the shared helper). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from PySide6.QtWidgets import QApplication | ||
| except ImportError: | ||
| pytest.skip("PySide6 unavailable") | ||
| app = QApplication.instance() or QApplication([]) |
| # test has something to actually narrow. | ||
| src = tmp_path / "source.png" | ||
| src.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 32) | ||
| os.chmod(src, 0o644) |
| # APP_VERSION at any given point. | ||
| import re | ||
| const = open("app/constants.py", encoding="utf-8").read() | ||
| const = open(_REPO_ROOT / "app" / "constants.py", encoding="utf-8").read() |
| # tab.py builds the stroke as plain (float, float) tuples now; | ||
| # this test fails if anyone reintroduces fitz.Point wrapping. | ||
| src = open("app/editor/tab.py", encoding="utf-8").read() | ||
| src = open(_REPO_ROOT / "app" / "editor" / "tab.py", encoding="utf-8").read() |
| # call on the dialog's thread (main), which mutates the flag | ||
| # immediately. Pin this so it can't be "simplified" back. | ||
| worker_src = open("app/worker.py", encoding="utf-8").read() | ||
| worker_src = open(_REPO_ROOT / "app" / "worker.py", encoding="utf-8").read() |
| """ | ||
| os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") | ||
| from PySide6.QtWidgets import QApplication, QListWidget | ||
| _app = QApplication.instance() or QApplication([]) # noqa: F841 |
|
|
||
| import os | ||
| import sys | ||
| import tempfile |
| try: | ||
| os.unlink(p) | ||
| self.dropped_paths.append(p) | ||
| except OSError: |
| if self._signals is not None: | ||
| try: | ||
| self._signals.deleteLater() | ||
| except RuntimeError: |
| # case via the profile owner; chmod there is a no-op. | ||
| try: | ||
| os.chmod(tmp, 0o600) | ||
| except OSError: |
This was referenced Jun 5, 2026
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
Closes 7 HIGH-severity bugs from audit rounds 5, 6, and 7.
Fixes
_pendingunbounded (editor/tab.py) — long sessions grew the edit history indefinitely; temp signature/image files never released. Cap at 500 + FIFO drop with temp-file cleanup. Test guards_pending_listwidget sync (regression for off-by-one drift caught in review).i18n.py,editor/dialogs.py) —~/.pdfapps_signature.pngand tmp images saved with default umask (0o644). Nowos.chmod(path, 0o600)after save.base.py,editor/tab.py,viewer/panel.py) — cached_pdf_passwordpersisted until GC. Heap dumps could expose. Best-effort wipe viawipe_pdf_password()inutils.py(Python str immutability documented as limitation). Wired into close/load paths.updater.py) —_apply_update_unixleft ~100 MB installer tempfile on failed apply. Wrapped in try/finally.updater.py) — retry reconnected_signals.finished/error/cancelledto a new thread without disconnecting old. Qt deliveredfinishedto all historical threads. Now fresh_Signalsinstance per download, olddeleteLater'd.viewer/panel.py) —QPrintDialogexposesfromPage/toPage/copyCount/pageOrder; loop ignored all four. Now respects user choice. Preserves the CRIT-3 pixmap fix inside the loop.tests/test_pdfapps.py) — 11 tests opened"app/base.py"etc. with relative paths. Migrated to_REPO_ROOT = Path(__file__).resolve().parent.parent.Tests
tests/test_editor_undo.py(+4 tests including widget-sync regression)tests/test_signature_perms.py(3 tests; functional 0o600 check skipped on Windows)tests/test_password_lifecycle.py(8 tests)tests/test_updater.py(+6 tests for cleanup + signal lifecycle)tests/test_viewer_safety.py(+4 tests for print range/copies/reverse)Validation
cd /tmp && pytest <repo>/tests/passesSupersedes #73.