fix: three high-severity audit bugs (#21, #22, #23) - #34
Merged
Conversation
#21 Encrypted clipboard payloads were destroyed on version/passphrase skew. crypto.decrypt() returns None rather than raising on every failure path, so _read_file() returned None for a CSENC payload we could not decrypt, _seed_from_file() left _last_synced as None, and the first OUT tick wrote local plaintext straight over the peer's ciphertext. The guard lives in _write_file/_write_image_file rather than in a sticky flag: reading the destination at write time is authoritative and recovers on its own the moment a matching passphrase is configured, with no state to reset. _out_tick now catches EncryptedPayloadError separately from OSError and reverts _last_synced under the lock, so the value is retried rather than silently marked as synced. is_encrypted() now matches the version-agnostic b"CSENC" prefix instead of the three known magics. Previously a payload from a newer build (a v3 we do not know yet) was not recognized as ciphertext at all, so the new guard would have sailed past the exact mixed-version case that motivates it. This is what makes the v2 payload from 18065f4 safe to ship alongside 1.0.0 peers. #22 History persistence was not thread-safe. add_entry/clear/set_max_items all released self._lock before calling _persist(), and _persist() serialized self._entries with no lock at all, so one thread's stale snapshot could overwrite another's appended entry. _persist_locked() now requires the lock and every mutator holds it across the file write. #23 settings.json and the history file both wrote through a fixed .tmp name, so the tray process and UI subprocesses could clobber each other's temp file and commit a corrupted mix. Both now use a pid-unique temp name and clean it up on failure. Regression tests verified to fail against the unfixed code: the five guard tests fail without the write-time check, and the concurrency test fails against the unlocked persist. The two success-path cases are controls that pass either way.
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.
Closes #21, closes #22, closes #23.
#21 Encrypted payloads destroyed on version or passphrase skew (data loss)
crypto.decrypt()returnsNonerather than raising on every failure path. So for a CSENC payload we cannot decrypt:_read_file()returnsNone→_seed_from_file()leaves_last_syncedasNone→ the first OUT tick sees a difference and writes local plaintext over the peer's ciphertext. The other machine's data is gone.The guard is in
_write_file/_write_image_file, not a sticky instance flag. Reading the destination at write time is authoritative and self-correcting: it starts allowing writes again the moment the user configures the right passphrase, with no state to reset and no added stat() on the per-tick hot path._out_tickcatchesEncryptedPayloadErrorseparately fromOSErrorand reverts_last_syncedunder the lock, so the value is retried instead of being silently recorded as synced.is_encrypted()now matches the version-agnosticb"CSENC"prefix rather than the three known magics. Previously a payload written by a newer build was not recognized as ciphertext at all, so the new guard would have walked straight past the mixed-version case that motivates it. This is the part that makes the v2 payload from 18065f4 safe to ship while 1.0.0 peers (v0/v1 only) are still in the field.#22 History persistence not thread-safe
add_entry/clear/set_max_itemsreleasedself._lockbefore calling_persist(), and_persist()serializedself._entriesunder no lock._persist_locked()now requires the lock, held continuously from mutation through the write.#23 Fixed temp filename
settings.jsonand the history file both wrote through a single shared.tmpname, so the tray and UI subprocesses could clobber each other. Both now use a pid-unique temp name with cleanup on failure.Verification
tests/integration/test_real_clipboard.pyare pre-existing onmainand need a real X clipboard.