Skip to content

fix: three high-severity audit bugs (#21, #22, #23) - #34

Merged
offbyonebit merged 1 commit into
mainfrom
fix/audit-high-severity
Jul 31, 2026
Merged

fix: three high-severity audit bugs (#21, #22, #23)#34
offbyonebit merged 1 commit into
mainfrom
fix/audit-high-severity

Conversation

@offbyonebit

Copy link
Copy Markdown
Owner

Closes #21, closes #22, closes #23.

#21 Encrypted payloads destroyed on version or passphrase skew (data loss)

crypto.decrypt() returns None rather than raising on every failure path. So for a CSENC payload we cannot decrypt: _read_file() returns None_seed_from_file() leaves _last_synced as None → 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_tick catches EncryptedPayloadError separately from OSError and reverts _last_synced under the lock, so the value is retried instead of being silently recorded as synced.

is_encrypted() now matches the version-agnostic b"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_items released self._lock before calling _persist(), and _persist() serialized self._entries under no lock. _persist_locked() now requires the lock, held continuously from mutation through the write.

#23 Fixed temp filename

settings.json and the history file both wrote through a single shared .tmp name, so the tray and UI subprocesses could clobber each other. Both now use a pid-unique temp name with cleanup on failure.

Verification

  • 146 unit tests pass; ruff and mypy clean.
  • The 5 failures in tests/integration/test_real_clipboard.py are pre-existing on main and need a real X clipboard.
  • Each regression test was confirmed to fail against the unfixed code: the 5 guard tests fail with the write-time check removed, and the concurrency test fails with persistence moved back outside the lock. The 2 success-path cases are controls that pass either way and are labelled as such.

#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.
@offbyonebit
offbyonebit merged commit 242ebcb into main Jul 31, 2026
1 check passed
@offbyonebit
offbyonebit deleted the fix/audit-high-severity branch July 31, 2026 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant