fix(editor): 13 audit findings (1 CRIT + 7 HIGH + 5 MED + LOWs)#82
Merged
Conversation
Twelve new keys added across all 8 shipping locales for the encryption
preservation prompt, forms / pending warnings, signature dialog empty
validation, note-deletion label, and form-load failure feedback. Parity
preserved at 8 locales x 595 keys.
Keys: editor.encrypt.warning_*, editor.encrypt.save_*,
editor.forms.has_pending, editor.forms.undo_unavailable,
editor.forms.load_failed, editor.forms.no_fields,
editor.signature.empty_{draw,type,import}, edit.label.note_delete.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three signature dialog UX fixes:
- _on_accept previously returned silently when Draw was empty, Type was
blank, or Import was unset. The dialog now surfaces a QMessageBox
warning per case (editor.signature.empty_{draw,type,import}) so the
user understands why nothing happened.
- _SignatureCanvas.is_empty / to_image now include the in-progress
stroke. Clicking OK with the mouse button still held no longer drops
the final stroke.
- File-open filter for the Import tab now lists *.tif and *.tiff so
scanned signatures imported from older capture devices show up.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Canvas-side Round 9 fixes: - Overlay image/signature pixmaps now go through an LRU cache keyed by (path, mtime). paintEvent previously rebuilt QPixmap(path) per overlay per repaint -- with 30+ stamps the cost dominated scroll latency. The mtime in the cache key auto-invalidates when the signature file is regenerated on disk. - _rect_to_pdf now intersects the dragged rect with the start page's bbox and returns None for a zero-area / fully off-page result. Cross-page redact drags previously mapped to the start page only and PyMuPDF silently truncated the off-page portion. - mouseReleaseEvent skips the rect_selected emit when the clamped rect is None so the redact pipeline never sees a degenerate Rect. - Notes now also persist the source annotation type + bbox when discovered late via context-menu, so the tab can register a stable delete_annot pending edit (xref isn't preserved across the release_doc/fitz.open round-trip). - paintEvent uses enumerate() for the overlay index instead of an O(n) list.index call (LOW polish). - _style_inline_edit picks a theme-aware background instead of the hardcoded #FFFFFFE6 which clashed on dark pages. - _commit_inline / _cancel_inline reset insert-mode style state so the next insert doesn't inherit stale font/size/colour. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The save path stripped encryption silently, downgrading password- protected input files to plaintext on every save. Both the fitz path (_run) and the pypdf forms path (_apply_forms) now detect encrypted input and prompt the user with three explicit choices: - Keep protection (re-encrypt the output with AES-256, the captured user password used as both user_pw and owner_pw) - Save without password (previous behaviour, but now opt-in) - Cancel (abort the save, nothing is written) Documented limitation: owner == user. We only captured a single password at load time; the original owner password is not recoverable from the input file. Future enhancement would be a separate prompt for the owner password. Adds shared mode-index constants (_MODE_REDACT etc.) for readability — call-sites can now write `if idx == _MODE_FORMS:` instead of `== 5`. New i18n keys: editor.encrypt.warning_title, editor.encrypt.warning_text, editor.encrypt.save_protected, editor.encrypt.save_unprotected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When the user opened a PDF, made overlay/redact/note edits, then switched to Forms mode and hit Save, the Forms apply path ran first and the pending edits were silently discarded. _run now checks for pending edits before delegating to _apply_forms in Forms mode and pops a Yes/No QMessageBox explaining the trade-off. Default button is No so accidental Enter doesn't lose data. New i18n key: editor.forms.has_pending. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two related note bugs: 1) Existing-annotation deletes evaporated on save. The canvas mutated _canvas._doc in-memory but _run did release_doc() + fitz.open(path), so the deletion was lost. Existing notes now carry their annot type + bbox at load time; deleting one registers a `delete_annot` pending edit which _run applies against the freshly-reopened doc by matching on (type, bbox) instead of xref (xref is not preserved across reopen). 2) Pending-note delete didn't push to _redo_stack -- Ctrl+Y could not restore the note, and a subsequent Ctrl+Z recovered some other arbitrary edit. _on_note_deleted now appends the removed entry to _redo_stack (without clearing it, so unrelated redo state is kept). Also adds a label entry + KeyError-safe .get() fallback in _add so unknown future edit types don't crash the QListWidget update. New i18n key: edit.label.note_delete. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three UX fixes around mode switching and undo: - Image mode (#4): Clicking the Image mode button no longer reopens the file picker every single time. It now mirrors the signature flow and only fires QFileDialog if no image is chosen yet (or the chosen file has since vanished). - Inline edit (#9): Switching modes while _inline_edit is open used to leave the typed text in limbo. _on_mode_btn now cancels the in-flight inline edit explicitly. - Forms undo (#8): Forms-mode edits live in the QTableWidget itself and are intentionally not tracked in _pending. Ctrl+Z previously did nothing silently; now _undo emits a status hint, and the undo/redo button tooltips swap to a "not available in Forms mode" string while the mode is active. New i18n key: editor.forms.undo_unavailable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two related Forms-mode polish items: - _load_form_fields previously swallowed every exception silently, so a malformed PDF read produced an empty table that looked identical to a PDF with no form fields. The except path now logs the exception and shows editor.forms.load_failed in a new status label below the table. A successful-but-empty load shows editor.forms.no_fields so the user can tell the two states apart. - _apply_forms now passes auto_regenerate=True to pypdf update_page_form_field_values so the rendered widget appearance picks up the new value in third-party viewers that don't honour NeedAppearances. New i18n keys: editor.forms.load_failed, editor.forms.no_fields. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Thirty source-level + behavioural tests covering each of the 12 fixes plus the LOW polish items. Pattern mirrors tests/test_round8_fixes.py: read the touched source, assert the new wiring is present, so a future refactor that drops a guard produces a visible test failure. Behavioural coverage (no full Qt event loop required): - _on_note_deleted pushes the popped entry onto _redo_stack and keeps the QListWidget in lock-step. - _load_overlay_pixmap LRU cache returns the same QPixmap instance for a repeated (path, mtime) call. i18n parity: explicitly verifies all 12 new keys exist in every shipping locale + asserts the global key set is consistent across locales. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Note delete persistence was only triggered on late-discovery via _annot_note_at. The common case (notes loaded in _load_existing_annotations and queued into _pending with _existing=True) never reached the delete_annot enqueue branch — the first for loop in _on_note_deleted matched on text+page and returned early, leaving the dedicated _existing block unreachable. Now both paths enqueue a delete_annot edit, fully persisting the deletion to the output file. The original note dict is stashed on the edit (_original_note) so _undo can restore the canvas overlay if the user reverses the action. Additionally: - _undo now re-adds the original note to _pending when a delete_annot is rolled back, keeping the canvas in sync with the pending queue. - New test_note_deleted_existing_enqueues_delete_annot covers the enqueue + undo-restore behaviour end-to-end with a Qt-light stub. - Optional cleanup: dropped the dead pass-only elif branch in canvas.mouseReleaseEvent and removed the redundant try/except around _fitz_permissions_of (already returns -1 on any failure). 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}
- Remove dead _MODE_* constants in editor/tab.py (declared but never read). - Drop orphan `import re` in tests/test_editor_audit_r9.py. - Rename `app = QApplication(...)` to `_app = ...` in 3 test bodies to match the suppression pattern used elsewhere.
| tests/test_editor_undo.py.""" | ||
| pytest.importorskip("PySide6.QtWidgets") | ||
| from PySide6.QtWidgets import QApplication, QListWidget | ||
| _app = QApplication.instance() or QApplication([]) |
| after it was never reached for the common case.""" | ||
| pytest.importorskip("PySide6.QtWidgets") | ||
| from PySide6.QtWidgets import QApplication, QListWidget | ||
| _app = QApplication.instance() or QApplication([]) |
| instance — proves the LRU is actually wired.""" | ||
| pytest.importorskip("PySide6.QtWidgets") | ||
| from PySide6.QtWidgets import QApplication | ||
| _app = QApplication.instance() or QApplication([]) |
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 13 editor-tool bugs from Round 9 audit. The CRIT is silent dataloss of encryption; 7 HIGHs cover persistence, performance, UX and lifecycle; 5 MEDs and several LOWs are polish.
Critical
editor/tab.py) —doc.save(...)andwriter.write(f)lackedencryption=. PDFs protected with a password were saved as plaintext. Now prompts withQMessageBox.question(3 options: Keep / Save unprotected / Cancel; default = Keep). Re-encrypts with AES-256 usingself._pdf_passwordas both user and owner (documented limitation: owner pwd not recoverable from a single load prompt). Permissions preserved via_fitz_permissions_ofhelper.HIGH
_runbypassed_pending. Now warns user withQMessageBox.question(defaultButton=No).delete_annotmutated_canvas._docbut_runreopened from disk. Now enqueues adelete_annotedit via_pendingfor BOTH common path (notes loaded at init with_existing=True) AND late-discovery path. Save loop processes them via bbox+type match. Undo restores via stashed_original_note._pick_image()was called every mode-switch. Now guarded byos.path.isfile.QPixmaploaded from disk per paintEvent — N overlays * 60fps = I/O thrash. Now cached via@lru_cache(maxsize=64)keyed by(path, mtime)._redo_stack—Ctrl+Zrecovered an arbitrary prior edit. Now pushes the original to redo stack symmetrically.QMessageBox.warning+ early return.MEDIUM
_pending-> tooltip swap warning Ctrl+Z unavailable in Forms mode._inline_editleft typed text in limbo -> now_cancel_inline()on mode change.pdf_rect.intersect(page_rect)+ reject empty.is_emptyandto_imagenow include_current._load_form_fieldsswallowed exceptions -> logged via_log.warning+ status feedback.LOWs (polish bundle)
labels.get(edit['type'], edit['type'])— KeyError safetyenumerate(self._overlays)— O(n^2) avoided_inline_editbackground uses theme color (was hardcoded#FFFFFFE6)*.tif/*.tiff_apply_forms(..., auto_regenerate=True)for viewer compatibility_inline_insert_size/_colorreset on commitpassbranch incanvas.py:914_fitz_permissions_ofi18n
12 new keys * 8 languages (en/pt/es/fr/de/zh/it/nl):
editor.encrypt.warning_title/text/save_protected/save_unprotectededitor.forms.has_pending/undo_unavailable/load_failed/no_fieldseditor.signature.empty_draw/empty_type/empty_importedit.label.note_deleteParity verified: 8 * 595 keys = 4760 total.
Tests
tests/test_editor_audit_r9.py— 31 source-level + behavioral tests (30 initial + 1 added for review finding). Behavioral: LRU pixmap identity, note-deleted redo sync (both pending + existing branches).Final: 226 passed, 1 failed (Flatpak tag pre-existing), 1 skipped.
Audit context
Round 9 was a focused drill-down per editor tool (vs the cross-cutting Rounds 5-8). After this PR, the editor reaches CRIT/HIGH zero from the catalogued audit. Round 10 (suggested by reviewer): viewer tools for similar persistence/encryption strip gaps.
Limitations documented
delete_annotmatching by (annot_type, bbox) with 1pt tolerance — two identical overlapping annots would match only the first._pending).Validation
APP_VERSIONbump