fix: macOS window crash + Windows tray-open regression - #35
Merged
Conversation
The release archives ship helper files that share the binary's basename (etc/firewall-ufw/syncthing, etc/freebsd-rc/syncthing), so matching on endswith(target_name) could select one of those instead of the binary. Whichever the archive listed first won. On macOS this wrote a 175-byte ufw config over the binary, and startup then failed with "OSError: [Errno 8] Exec format error" on every launch. The tar.gz path had the same flaw. The real binary sits directly under the top-level release directory, so prefer the shallowest matching path.
Pillow's ImageTk relies on _imagingtk resolving Tcl/Tk symbols at runtime by locating the _tkinter shared library. On interpreters that compile _tkinter directly into the binary -- as the python-build-standalone builds uv installs do -- there is no such library, the PyImagingPhoto Tcl command is never registered, and constructing an ImageTk.PhotoImage dies with "TypeError: bad argument type for built-in operation". That killed the whole UI child process. Because TabbedWindow builds all of its tabs eagerly and the Pair tab renders a QR code on construction, every tray menu entry -- Settings and Devices included -- flashed a window for a moment and vanished. Builds on other platforms use a Python with a normal shared _tkinter, which is why this only reproduced on macOS. Tk 8.6 decodes PNG natively, so encode to PNG bytes and hand them to a plain tkinter.PhotoImage, skipping Pillow's Tk bridge. This keeps the fix independent of which interpreter built the bundle. CTkLabel accepts a plain Tk image; the only loss is CTkImage's HiDPI rescaling, and both call sites already render at a fixed pixel size.
The OUT loop reads the clipboard while the IN loop writes it, from two different threads, and neither path took a lock. The existing self._lock guards last-value bookkeeping, not the clipboard itself. The native clipboard is a single shared object and is not thread-safe. On macOS this is worse than it looks: pyperclip picks its PyObjC backend over pbcopy/pbpaste whenever AppKit is importable, which it always is in the bundled app because the tray needs it. So paste() drives -[NSPasteboard stringForType:] and copy() drives declareTypes:owner:, and a read landing inside a write faults in -[_NSPasteboardOwnersCollection handleOwnershipChange], killing the whole tray process with SIGSEGV after some minutes of ordinary use. Route all four native entry points (text read/write, image read/write) through one module-level RLock. Text and images share the lock because they target the same pasteboard. The regression tests instrument the clipboard calls with a sleep rather than a spin: sleeping releases the GIL, so an unsynchronized second thread reliably lands in the window. Both tests fail if the lock is removed.
Fixes the confirmed findings from the audit pass. Each has a regression test that was checked to fail against the pre-fix code. Supply chain, syncthing.py: - _verify_archive_hash failed open. A fetch failure or a missing entry for this platform logged a warning and extracted anyway, so anyone able to drop or poison one request got an unverified binary executed. Both paths now raise. This deliberately reverses behaviour that was tested and documented as intentional; availability is the cost and it is the right trade for an archive we then run. - ensure_binary trusted the on-disk binary purely because --version printed the pinned string, which a replaced binary can trivially forge. Record the binary's SHA-256 when we install it from an archive whose signed hash we just verified, and check it on every start. The published sums cover the archive, not the extracted binary, so comparing the binary to them directly is not possible. The digest file sits beside the binary, so this does not stop an attacker who can write both; it does catch Syncthing self-upgrading over its own binary, partial extraction, and tampering that misses the sidecar. The check is local, so a good binary still starts offline. Sync folder, main.py: - _on_folder_changed restarted only ClipboardSync. Syncthing reads the folder path once, when prepare_home patches config.xml, and FileTransfer schedules its observer at construction, so both kept pointing at the old directory: clipboard.txt was written where no peer replicated it and sync silently stopped. Now restarts Syncthing and FileTransfer too, and persists the setting itself rather than relying on the UI process. OUT loop and IN dispatch, clipboard.py: - A transient OSError left _last_synced set, so the "already sent" guard made every later tick skip that value and it was never synced again. EncryptedPayloadError already rolled back; OSError now does too, on both the text and image paths. - One shared debounce deadline meant a clipboard.txt and clipboard.png update inside the same 100ms window suppressed each other. Now per path. - The heartbeat truncation guard tested only str, so a synced image was repr()'d whole into the log every 6 seconds. Extracted as _truncate_for_log and applied to bytes as well. file_transfer.py: - _seen used an unguarded check-then-add, and watchdog dispatches from a thread pool on Windows, so one file could be delivered twice. Also fixes tests that hardcoded the Linux archive name: on any other platform the lookup missed, verification was skipped, and the assertions were vacuous. They now derive the name for the running platform, which also repairs a test that was already failing on macOS.
The release-key signature check needed gpg on PATH. Stock Windows and macOS do not ship it, so for most users _verify_release_signature logged a warning and returned, leaving the hash as the only protection -- and that hash was fetched from the same origin as the download it was meant to vouch for. The docstrings described a verified supply chain that end users were not actually getting. Move the trust anchor into the source tree. _syncthing_hashes.py holds the SHA-256 of every release archive for the pinned version, and _expected_archive_hash prefers it over any network lookup, so verification for the shipped version no longer depends on a request that can be blocked or poisoned, and no longer depends on gpg existing at all. The hashes are not hand-copied. tools/refresh_syncthing_hashes.py fetches sha256sum.txt.asc and feeds it to the app's own _verify_release_signature, refusing to emit anything unless the PGP signature verifies against the pinned fingerprint. Verification happens once on a maintainer's machine at release-prep time rather than never on a user's. The fetched-sums path stays as a fallback for versions we have not pinned, still fail-closed. Verified end to end: the pinned macOS hash was confirmed against a real download of the v2.0.16 archive, and a clean install with the binary directory deleted now reaches "archive hash verified" without ever requesting sha256sum.txt.asc. A test asserts the manifest covers config.SYNCTHING_VERSION, so bumping the version without regenerating fails the suite rather than silently dropping back to the network path.
_load() logged "Failed to decrypt clipboard history" and returned with an empty in-memory list. Nothing marked the file as untouchable, so the next add_entry() -- which clipboard.py issues on every OUT tick -- persisted that empty list straight over it. Measured on the pre-fix code: a 5-entry encrypted history became a 1-entry file, unrecoverable even with the correct passphrase. The sync file has been protected from precisely this since the #21 audit, via ClipboardSync._refuse_if_unreadable_ciphertext. The history file holds the same clipboard text and had no equivalent guard. An unreadable file is now moved aside to clipsync_history.unreadable-<timestamp>.json and a fresh history started, so the ciphertext stays recoverable if the passphrase turns up while history keeps working. If it cannot even be moved, the session goes in-memory only rather than clobbering it. Both the "wrong passphrase" and "passphrase cleared from settings" paths are covered, as is ciphertext written by a newer build: is_encrypted() matches CSENC of any version precisely so a downgrade cannot destroy a newer machine's data. Also tighten permissions on the temp file before the rename rather than on the final name after it. The old order left a window where a file of clipboard text sat at its real name readable by other local users. 4 of the 14 new tests fail against the pre-fix code. The tests only ever count entries; none assert on clipboard text.
send() was a bare shutil.copy2, so configuring a passphrase protected clipboard text while every sent file sat in the shared folder as plaintext, readable by anything with access to that directory and replicated that way to each peer. Verified against the old code: with a passphrase set, the marker bytes were present verbatim in the shared copy, at mode 0644. Files now encrypt on the way into the folder (gaining a .csenc suffix) and decrypt on the way out to Downloads. Decrypting there rather than in place is deliberate: writing plaintext back inside the synced folder would hand it straight to Syncthing and undo the encryption for every peer. Fernet holds an entire payload in memory, so a naive port would have made large sends fatal. crypto.py gains a chunked streaming format instead: CSENCF magic, salt, then length-prefixed Fernet tokens over (chunk index || data), ending in an authenticated empty chunk. The index is inside the authenticated plaintext so chunks cannot be reordered or dropped undetected, and the terminator catches truncation, which would otherwise decrypt cleanly to a prefix and hand the user a corrupt file that looks whole. A failed decrypt removes its partial output. copy2 also preserved the source mode, so a world-readable original stayed world-readable in the shared folder; both paths are now 0600. A peer with no passphrase gets a clear notification instead of a file of ciphertext dumped in Downloads. With no passphrase configured the previous plaintext behaviour is unchanged, so older peers keep working. 17 new tests covering roundtrip at chunk boundaries, tamper and truncation detection, permissions, and partial-file cleanup.
LogMirror copied this device's log into the shared folder every 10s. That
folder is replicated to every paired device, so the log -- hostnames, device
IDs, file names, error traces -- went to all of them. It was always on, had
no setting, and was not mentioned in the README. No clipboard text is ever
logged (every clipboard log line records a character count), but none of
that is visible from the tray.
Now off by default, with a Settings switch and an explanation of what it
shares. The thread self-gates each tick so the toggle applies without a
restart, and switching it off deletes our own published log from the shared
folder -- otherwise the last copy keeps replicating to peers forever. The
first disabled tick also retracts a file left by an older always-on build,
which is the upgrade path for every existing user. Peers' logs are never
touched.
Separately, ClipboardHistory parsed settings with a bare int(), and it is
built during startup, so a single malformed value in settings.json raised
before the tray appeared -- verified: history_max_items="not-a-number"
crashed with ValueError. Parsing is now defensive. bool("false") is True,
which is exactly the trap a JSON-stringified setting falls into, so
history_enabled="false" silently kept history on; strings are matched
explicitly. history_auto_clear_minutes now accepts "30" and falls back to 0
(never expire) rather than an arbitrary retention.
29 new tests. All three old behaviours were confirmed against the pre-fix
source before changing anything.
ClipboardSync builds a ClipboardHistory bound to config.HISTORY_FILE, so any test constructing one writes wherever that module global points. Several suites also set an encryption passphrase before syncing, so an unisolated run does not merely read the developer's real clipboard history: it overwrites it, encrypted with a throwaway key, and the installed app can then never read its own history again. Confirmed on a real machine: running the suite replaced a 76KB history with a 1698-byte file that decrypts under 'shared-secret', repeatedly, once per run. The app dutifully reported 'passphrase mismatch' every launch and the cause looked like an app bug for hours. Modules patched HISTORY_FILE individually and four did not: test_cross_os_sync, test_image_sync, test_linux_paste_freeze, test_mac_windows_sync. An autouse conftest fixture makes isolation the default for every test rather than something each new file must remember, and covers SETTINGS_FILE, LOG_FILE, APP_DATA_DIR and SYNC_FOLDER too. Verified: a full suite run now leaves the real history byte-identical.
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
1 participant
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.
No description provided.