fix: hardening fixes from full-codebase review#25
Open
KristianP26 wants to merge 21 commits into
Open
Conversation
The 1024-token ceiling silently dropped rewrites of dictations over roughly 2700 characters (finish_reason=length discards the result). Scale the guard up to 4096 so long dictations still get cleaned up.
Per-character injection issued two syscalls per UTF-16 unit, which is slow for long transcripts and lets concurrent user keystrokes interleave mid-text. Build one INPUT array and inject it atomically; also lift _utf16_units to module level so the splitting logic is unit-testable.
Each dictation parented a new _WorkerThread to the tray app without ever deleting it, leaking one QThread object per recording for the process lifetime. Connect finished to deleteLater per the Qt ownership pattern.
Unchecking Enabled during a recording ran the full pipeline and typed the transcription into the focused window. The user intent is to stop, so cancel the recording and return to idle instead.
Every launch rewrote settings.ini and keys.enc even when the .env import was a no-op. Compare against the loaded config and persist only when the import actually changed something.
A frozen exe launched from the HKCU Run key inherits an unpredictable working directory, so the cwd-based .env lookup silently missed the file shipped next to the binary. Resolve it relative to the executable when frozen; dev runs keep the cwd behaviour.
Custom headers routinely carry credentials (X-Api-Key, proxy tokens) but were persisted plaintext in settings.ini while API keys went through DPAPI. Treat all four header fields as secrets, and purge any plaintext value an older version wrote so it cannot shadow the encrypted one on load. Non-Windows dev runs lose header persistence, matching the existing behaviour for API keys.
Only the OK button went through validation; a direct accept() call (keyboard default, programmatic close) skipped both validation and the startup-registration sync. Fold the checks into accept() itself.
Clicking into a key field exposed the secret immediately — an easy slip during screen shares. Keep the field masked and add a checkable trailing eye action that reveals the value only on demand.
Calibration recorded two seconds of audio synchronously on the Qt main thread, freezing the whole dialog. Move the measurement into a small QThread, disable the button while it runs, and have done() wait for an in-flight run so the thread never outlives the dialog.
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
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
A batch of correctness, security, and responsiveness fixes from a full review of the codebase. Each change is an isolated commit.
Security
settings.ini(they routinely carry tokens such asX-Api-Key). Stale plaintext values from older versions are purged on the next save so they cannot shadow the encrypted ones.has_plaintext_secrets()detects secret keys an older version left insettings.iniand forces a save at startup (values are preserved, just re-persisted via DPAPI), so the purge no longer waits for the first manual settings save.Correctness
accept()call bypassed both validation and the startup-registration sync.done()disconnects the result slots before waiting on the calibration thread, so a slow calibration can no longer deliver into a closing dialog..envnext to the exe — apps launched from the Run key inherit an unpredictable cwd, so the file shipped next to the binary was silently missed.finish_reason=lengthand silently lost their cleanup.Resource & responsiveness
finished→deleteLater); previously oneQThreadobject leaked per recording.SendInputcall instead of two syscalls per UTF-16 unit: faster for long transcripts and atomic with respect to concurrent user keystrokes.settings.ini/keys.encwhen the.envimport changed nothing (unless plaintext secrets need purging, see above).Minor
SendInputsemantics: a failed batch means a prefix of the text was already typed; there is no rollback.config.stt_languageaccess inrewrite.py(defensivegetattrremoved).QSignalSpy+ release event instead of aprocessEvents/sleeppoll loop).Test plan
python -m compileall src/ tests/clean.python -m unittest discover -s tests, offscreen Qt, Windows): 105 tests pass, including new coverage for the cap scaling, UTF-16 splitting, discard-on-disable, cancel-on-disable-while-processing, startup-save skip, startup plaintext-secret purge, frozen.envpath, header encryption + stale-ini purge, accept-path validation, password-field toggle, threaded calibration, and dropped late calibration results on dialog close.SendInputtypes correctly (python -m src.injector),python -m src.configDPAPI roundtrip OK.