fix: 12 Round 10 audit findings (2 CRIT + 5 HIGH + 3 MED + 2 LOW)#84
Merged
Conversation
…IT-1, A2, A3) Comment delete from the viewer context menu now: - copies the PDF to a same-directory .bak before delete_annot, restoring it on saveIncr failure so power loss or read-only writes cannot corrupt the user's file (CRIT-1); - separates the in-memory delete from saveIncr so a disk write error reopens the doc to discard the stale in-memory mutation, preventing inconsistent state across the next paint (A2); - breaks ties between annotations with identical content using bbox proximity (1pt) before falling back to content-only match, so two notes with the same text no longer cause the wrong one to be deleted (A3). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PR-D's regression made _run() release the canvas's fitz.Document BEFORE prompting the user how to save an encrypted file. Cancelling the prompt then left the canvas with _doc=None, stranding the editor on the placeholder until the user manually reloaded the PDF. Open a short-lived peek doc to read needs_pass / drive the encryption prompt. Only release the canvas once the user has confirmed the choice; the keep-protection and save-unprotected paths are unchanged because they reach the existing release_doc call. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…#11) The module-level @lru_cache(maxsize=64) for _load_overlay_pixmap had no clear() hook the tab close path could use — across long sessions with multiple tabs each cache slot kept a multi-MB QPixmap, leaking until process exit. Replace it with: - an explicit FIFO dict + manual eviction so size is still capped at 64 but close_doc() can drop everything via the new clear_overlay_pixmap_cache() helper; - an uncached fallback path when os.path.getmtime() raises OSError, so a transient FS error no longer poisons the cache with a null QPixmap that stays for the rest of the process lifetime. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The recents list was built once in PdfViewerPanel.__init__ from get_recent_files() and never refreshed. add_recent_file() updates the config.json behind the scenes, but the user kept seeing the stale UI list until the next app launch. Extract the population code into _refresh_recents() and call it from window._load_and_track / _open_in_new_tab across every viewer tab, so returning to the placeholder always shows the up-to-date list. No new Qt signals required — the parent already knows when a load happens. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Clicking Apply in Forms mode on a PDF with no form fields raised
pypdf's PyPdfError("No /AcroForm dictionary in PDF") — a cryptic
internal error from update_page_form_field_values that looked like
a bug. Detect the missing /AcroForm root entry on the prepared
PdfWriter and short-circuit with the existing editor.forms.no_fields
status string instead, leaving the input file untouched.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The viewer's note-delete context menu already prompts the user before destroying a comment; the editor's equivalent silently deleted on left-click, which was inconsistent and easy to trigger by accident on _existing notes that persist to the saved file. Add the same QMessageBox.question(default=No) using the existing viewer.confirm_delete_comment / msg.confirm i18n strings — no new keys required. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The release version-bump step rewrote ``v{ANY}`` and
``pdfapps-{ANY}`` unanchored in both .SRCINFO files. A future
historical comment such as ``# since v1.10.0`` would have been
silently rewritten when the next release bumped versions.
Anchor every replacement to the surrounding token (``.tar.gz``
suffix, ``/v.../`` path segment). Verified that both current AUR
.SRCINFOs still receive every required substitution.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The drop handler previously checked path.lower().endswith('.pdf')
on the local file. Folder drops returned a directory path that
failed the suffix check and were silently ignored; web URL drops
(http/https/ftp) resolved to an empty toLocalFile() and were also
silently swallowed.
Handle both:
- folder drops iterate every *.pdf (and uppercase *.PDF) and open
each via _load_and_track;
- web URL drops surface a one-shot translated warning telling the
user to download the file first.
dragEnterEvent now accepts folder URLs too so the cursor reflects
the drop target.
Adds viewer.drop_url_not_supported (8 locales) — parity verified.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
19 source-level tests covering every fix in PR-F. Where the production change is observable at module import time (cache name, helper functions, regex anchors), we hit it directly; where it lives inside Qt event-loop code (context menus, drop handlers), we read the source and assert the new wiring is in place. Pair with manual QA on the next release candidate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
nelsonduarte
added
bug
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if backup_path: | ||
| with contextlib.suppress(Exception): | ||
| os.unlink(backup_path) | ||
| backup_path = None |
| if backup_path: | ||
| with contextlib.suppress(Exception): | ||
| shutil.move(backup_path, self._path) | ||
| backup_path = None |
| import json | ||
| from pathlib import Path | ||
|
|
||
| import pytest |
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 12 bugs from audit Round 10. 11 fixed in code, 1 (tab close OCR race) audited and locked-in via test invariant. Includes 2 CRITs (viewer dataloss + editor regression from PR-D) and 5 HIGHs.
Critical fixes
app/viewer/canvas.py) — Delete comment via right-click wrote directly to the user's PDF viasaveIncr()with no backup, no atomic write, no confirmation. Power loss = file corruption. PR-A fixed the equivalent in the editor; this completes the parity for the viewer. New flow: confirmation dialog (default=No), tempfile backup, two-step delete/save with restore-on-failure, and document reopen if save fails (to discard the in-memory mutation).app/editor/tab.py) —_canvas.release_doc()was called BEFORE_prompt_encryption_choice(). Cancelling the encryption dialog left the canvas in a dead state with placeholder pages permanently until manual reload. Now uses a short-lived peek-doc to drive the prompt;release_doc()happens only AFTER the user confirms.HIGH
app/editor/canvas.py) — Replaced@lru_cache(maxsize=64)module-level with explicit FIFO_OVERLAY_PIXMAP_CACHE+clear_overlay_pixmap_cache()wired intoclose_doc(). Long sessions no longer accumulate dead QPixmap references.delete_annot + saveIncrseparated into two try blocks. If save fails after delete, the doc is reopened to discard the stale in-memory state (re-authenticated if needed).app/viewer/panel.py+app/window.py) — Extracted_refresh_recentsand wired from_load_and_track/_open_in_new_tab. Multi-tab panels all refresh.app/editor/tab.py) —_apply_formsnow checks"/AcroForm" not in writer._root_objectand surfaceseditor.forms.no_fieldscleanly.MEDIUM
app/editor/canvas.py) — UX consistency with viewer;QMessageBox.question(default=No)reusingviewer.confirm_delete_comment..github/workflows/release.yml) — Anchored to tarball context (pdfapps-{ANY}\.tar\.gz,/v{ANY}\.tar\.gz,/v{ANY}/); dropped unanchoredv{ANY}. Comments and changelog refs no longer corruptible in future bumps.window.closeEventalready callswait_for_workers()beforesuper().closeEvent(). Editor tab has no background workers (renders use gen-bumping). New testtest_window_closeevent_waits_for_workers_before_destroylocks the invariant.LOW
app/editor/canvas.py) — OSError path now returnsQPixmap(path)directly instead of caching an empty pixmap under the bogus mtime key.app/window.py) — Drops of folders iterate*.pdf/*.PDFand open each as a tab. Web URL drops surface a translated warning (viewer.drop_url_not_supported).i18n
1 new key × 8 languages:
viewer.drop_url_not_supported. Parity verified by test.Tests
tests/test_audit_r10.py— 19 new tests covering all 12 fixes (source-level + behavioral where feasible).Final: 249 passed, 1 failed (
test_flatpak_manifest_tag_is_currentpre-existing — manifest in v1.13.9 vs APP_VERSION 1.13.15), 1 skipped.Audit context
Round 10 was the focused viewer drill-down (suggested by R9 reviewer) plus PR-D regression check. After this PR, CRIT/HIGH-zero across the catalogued audit. Total bugs mapped: 78; closed: 37; remaining: 41 (24 MEDIUM + 17 LOW) — all polish, none release-blocking.
Non-blocking observations (from review)
viewer/canvas.py:716-719— backup+saveIncr cycle fires even whentarget_annot is None. Minor IO redundancy.editor/canvas.py:525—clear_overlay_pixmap_cache()invalidates globally not per-tab. Functional; consider docstring clarification.window.py:1015-1018— folder drop opens all PDFs without prompt. Considerif len > 20: confirm.viewer/canvas.py:630-639— backup file briefly visible in same dir; OneDrive/Dropbox sync noise possible. Cross-FS trade-off.Validation
APP_VERSIONbump