chore: resolve 20 audit MEDIUM + LOW findings#96
Merged
Conversation
nelsonduarte
added
bug
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import os | ||
|
|
||
| from PySide6.QtCore import Qt, QSize, Signal | ||
| from PySide6.QtCore import Qt, QSize, Signal, QTimer |
| _logging.getLogger("pdfapps").warning( | ||
| "Pipeline save destination is a symlink: %s -> %s", | ||
| path, os.path.realpath(path)) | ||
| except Exception: |
The compress utility raised raw English RuntimeError ("Install pypdf
and/or PyMuPDF...") and ValueError ("No gain: ... KB") strings that
surfaced unchanged in error dialogs — Chinese / Russian / German users
saw English. Route both through t() with new tool.compress.deps_missing
and tool.compress.no_gain_detail keys (8 languages each).
The finally-clause unconditionally cleared every QLineEdit, so a wrong password or owner/confirm mismatch forced the user to retype all four fields. Gate the clear with a `success` flag — only sticky errors via show_error keep the input; wrong_pass / mismatch warnings preserve it.
Pre-existing rows kept their start/end values when the user swapped a 50-page PDF for a 10-page one, then Apply tripped the row_invalid error on every row. _load_input now calls _clamp_rows_to_total so spinbox values are capped to the new total and end>=start is preserved.
pypdf.PdfReader.decrypt returns 0 on wrong password and silently exposes a reader whose .pages is empty. Every downstream tool then wrote an empty PDF over the user's destination. BasePage._open_reader, the editor's _load_form_fields + _apply_forms paths, watermark (source + watermark file) and merge per-input now raise a translated "wrong password" error instead.
_save_pipeline used shutil.copy2 directly, so a crash mid-copy could truncate a pre-existing destination PDF. Switched to mkstemp + os.replace (same-volume) with a copy2 fallback when mkstemp can't write next to the target. Also defer the startup update check via QTimer.singleShot(2000) so it never races showMaximized, scope the PgUp/PgDown/Ctrl+S/Ctrl+W shortcuts to WidgetWithChildrenShortcut so editing a QTextEdit no longer pages the viewer behind, default unsaved-pipeline prompts to Cancel, and log a warning when the save destination is a symlink (audit trail for J3).
PR-A migrated 8 tools to BasePage._atomic_pdf_write, but nup.py and the 8 converters in import_pdf.py kept calling doc.save(out_path) directly. A kill / crash mid-save could truncate a pre-existing output PDF and none of them benefited from the same-source-path guard. The bulk doc.save migration uses the helper's save_opts plumbing (garbage=4, deflate=True for nup). Also wire the previously dead _IMG_EXTS tuple in import_pdf as an input filter for the image-to-PDF converter.
Default Qt tooltips inherited the OS chrome (white-on-yellow on Windows) which clashed with the dark palette and was barely readable on top of the light theme too. Add a QToolTip block to STYLE and STYLE_LIGHT using the existing card / border / text-primary tokens so tooltips track the active theme.
_SignatureDialog has 5+ focusable widgets (tabs, type input, font combo, save checkbox, cancel + ok). Focus jumped erratically between the tabs widget and the buttons depending on layout add order. Lock the path: tabs -> type input -> font combo -> save cb -> cancel -> ok.
…atin The "replace existing page numbers" QMessageBox.question lacked an explicit defaultButton — an Enter keystroke would Yes-confirm a destructive overwrite with no undo (the tool routes through saveIncr). Set defaultButton=No. Also detect non-Latin chars in the selected format template and surface a status-bar warning, since the built-in PyMuPDF "helv" font is Type-1 Latin-1 and renders CJK / Cyrillic / Arabic / Hebrew as ? glyphs.
If the search loop in the right-click delete handler exits with target_annot is None (annotation already wiped by another tool, or the bbox / content match drifts after an external edit), the code still bumped the incremental-update offset by calling saveIncr() — a no-op write that changed the file hash for no user benefit. Show a warning and return early instead. New translation key: viewer.delete_no_match across all 8 languages.
Bitdefender + heuristic AV engines flag PyInstaller binaries with no version-info resource as suspicious. Add a version='version_info.txt' kwarg to the EXE() call on Windows builds only, and regenerate version_info.txt from app.constants.APP_VERSION on each spec evaluation so the PE header stays in sync with the macOS bundle. The checked-in version_info.txt acts as a seed / fallback.
26 assertions covering M1-M12 and L1/L4/L5/L8: i18n parity for the new keys, atomic write migration in nup/import_pdf/pipeline save, decrypt return checks across 5 tools, QShortcut WidgetWithChildren scope, themed QToolTip rule, defaultButton wiring on destructive prompts, _SignatureDialog setTabOrder, _IMG_EXTS usage in import_pdf, non-Latin warnings, version_info.txt + spec wiring, and the viewer note-delete no-match no-op.
nelsonduarte
force-pushed
the
chore/audit-mediums-lows
branch
from
97f2197 to
1252498
Compare
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 20 polish/quality bugs from audit Rounds 5-11 (12 MEDIUM + 8 LOW). Low risk, focused improvements covering security hardening, i18n correctness, atomic IO, and UX consistency.
MEDIUM (12)
LOW (8 - 6 applied + 2 already implemented)
i18n
5 new keys x 8 languages: `tool.compress.deps_missing`, `tool.compress.no_gain_detail`, `tool.err.wrong_password`, `tool.warn.font_latin_only`, `viewer.delete_no_match`. Parity verified: 8 x 602 keys.
Tests
`tests/test_audit_mediums_lows.py` — 26 source-level + behavioral tests covering all 18 applied fixes. All pass.
Adversarial review
Review verdict: SAFE TO MERGE. Three cosmetic non-blocking notes:
These are tracked for a follow-up if needed; no dataloss / security risk.
Validation