diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9f7882d..bb9b07c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,7 +14,11 @@ on: jobs: lint-and-test: - runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, macos-latest, windows-latest] + runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 @@ -26,6 +30,15 @@ jobs: - name: Install dependencies run: pip install -r requirements-dev.txt + - name: Start xvfb (Linux) + if: runner.os == 'Linux' + run: | + sudo apt-get update + sudo apt-get install -y xvfb + export DISPLAY=:99 + Xvfb :99 -screen 0 1024x768x24 & + echo "DISPLAY=:99" >> "$GITHUB_ENV" + - name: Lint run: ruff check clipsync/ tests/ @@ -36,4 +49,4 @@ jobs: run: mypy clipsync/ - name: Test - run: pytest tests/ -q + run: pytest tests/ -q -m "not integration" diff --git a/clipsync/autostart.py b/clipsync/autostart.py index a0f38fc..6a6988f 100644 --- a/clipsync/autostart.py +++ b/clipsync/autostart.py @@ -54,26 +54,17 @@ def _macos_set(enabled: bool) -> None: if path.exists(): path.unlink() return - argv = _launch_command() - args_xml = "\n".join(f" {a}" for a in argv) - plist = ( - '\n' - '\n' - '\n' - "\n" - f" Label{_BUNDLE_ID}\n" - " ProgramArguments\n" - " \n" - f"{args_xml}\n" - " \n" - " RunAtLoad\n" - " KeepAlive\n" - "\n" - "\n" - ) + import plistlib + + plist = { + "Label": _BUNDLE_ID, + "ProgramArguments": _launch_command(), + "RunAtLoad": True, + "KeepAlive": False, + } path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(plist, encoding="utf-8") + with path.open("wb") as fh: + plistlib.dump(plist, fh) def _linux_desktop_path() -> Path: diff --git a/clipsync/clipboard.py b/clipsync/clipboard.py index 400f06e..d4aec60 100644 --- a/clipsync/clipboard.py +++ b/clipsync/clipboard.py @@ -384,6 +384,8 @@ def _read_image_from_system_clipboard() -> bytes | None: # Linux: check TARGETS first so we never send an image/png SelectionRequest # to the clipboard owner when only text is present. Without this guard, # xclip would request image data even when the clipboard holds text. + # Try each available command in turn; xclip failing (e.g. on a text-only + # Wayland clipboard) must not prevent wl-paste from running. for targets_cmd in ( ["xclip", "-selection", "clipboard", "-t", "TARGETS", "-o"], ["wl-paste", "--list-types"], @@ -392,9 +394,10 @@ def _read_image_from_system_clipboard() -> bytes | None: res = subprocess.run(targets_cmd, capture_output=True, timeout=1) except (FileNotFoundError, subprocess.TimeoutExpired, OSError): continue - if res.returncode != 0 or b"image/png" not in res.stdout: - return None - break + if res.returncode == 0 and b"image/png" in res.stdout: + break + else: + return None # Some xclip versions return text content with exit 0 even when asked for # image/png and no image is on the clipboard. Guard with a PNG magic-byte # check so we never mistake text bytes for image data. @@ -415,8 +418,8 @@ def _write_image_to_system_clipboard(png_bytes: bytes) -> bool: """Write PNG bytes to the system clipboard. Returns True on success.""" if sys.platform == "darwin": try: - from AppKit import NSImage, NSPasteboard # type: ignore[import] - from Foundation import NSData # type: ignore[import] + from AppKit import NSImage, NSPasteboard + from Foundation import NSData ns_data = NSData.dataWithBytes_length_(png_bytes, len(png_bytes)) ns_image = NSImage.alloc().initWithData_(ns_data) @@ -613,6 +616,30 @@ def _refuse_if_unreadable_ciphertext(self, path: Path) -> None: if decrypt(data, passphrase) is None: raise EncryptedPayloadError(path) + def _atomic_write(self, path: Path, payload: bytes) -> None: + """Write *payload* to *path* atomically and clean up any temp file. + + Uses a uniquely-named temp file so concurrent writers cannot collide, + and unlinks the temp file on failure so partial writes do not litter + the sync folder. + """ + import secrets + + tmp = path.with_name(f"{path.name}.{os.getpid()}.{secrets.token_hex(4)}.tmp") + try: + tmp.write_bytes(payload) + for attempt in range(10): + try: + tmp.replace(path) + config.set_file_permissions(path) + return + except PermissionError: + if attempt == 9: + raise + time.sleep(0.1) + finally: + tmp.unlink(missing_ok=True) + def _write_file(self, text: str) -> None: """Atomic write of the shared file, encrypting if a passphrase is set.""" path = self.clipboard_file @@ -621,17 +648,7 @@ def _write_file(self, text: str) -> None: passphrase = self._passphrase() encoded = text.encode("utf-8") payload = encrypt(encoded, passphrase) if passphrase else encoded - tmp = path.with_name(path.name + ".tmp") - tmp.write_bytes(payload) - for attempt in range(10): - try: - tmp.replace(path) - config.set_file_permissions(path) - return - except PermissionError: - if attempt == 9: - raise - time.sleep(0.1) + self._atomic_write(path, payload) def _read_image_file(self) -> bytes | None: """Return PNG bytes from the shared image file, decrypting if needed.""" @@ -674,17 +691,7 @@ def _write_image_file(self, png_bytes: bytes) -> None: path.parent.mkdir(parents=True, exist_ok=True) passphrase = self._passphrase() payload = encrypt(png_bytes, passphrase) if passphrase else png_bytes - tmp = path.with_name(path.name + ".tmp") - tmp.write_bytes(payload) - for attempt in range(10): - try: - tmp.replace(path) - config.set_file_permissions(path) - return - except PermissionError: - if attempt == 9: - raise - time.sleep(0.1) + self._atomic_write(path, payload) def _seed_from_file(self) -> None: """Prime _last_synced from disk so we don't re-emit stale content on startup. @@ -854,10 +861,16 @@ def _out_loop(self) -> None: _last_heartbeat = now with self._lock: last = self._last_synced + if isinstance(last, bytes): + desc = f"" + elif isinstance(last, str): + desc = f"" + else: + desc = "" log.debug( "HEARTBEAT (host=%s): last_synced=%s, paused=%s", _HOSTNAME, - _truncate_for_log(last), + desc, self._is_paused(), ) @@ -868,25 +881,20 @@ def _out_tick(self) -> None: with self._lock: if image == self._last_synced: return - previous_last_synced = self._last_synced - self._last_synced = image try: self._write_image_file(image) - log.info("OUT [%s]: %d bytes image written", _HOSTNAME, len(image)) except EncryptedPayloadError: - with self._lock: - self._last_synced = previous_last_synced reason = "Refusing to overwrite encrypted clipboard image file (cannot decrypt)" if reason != self._last_decrypt_error: log.warning("OUT [%s]: %s", _HOSTNAME, reason) self._last_decrypt_error = reason + return except OSError: - # Roll back too: _last_synced is the "already sent" guard, so - # leaving it set after a failed write means every later tick - # sees this image as synced and it is never retried. - with self._lock: - self._last_synced = previous_last_synced log.exception("OUT [%s]: Failed to write image file", _HOSTNAME) + return + with self._lock: + self._last_synced = image + log.info("OUT [%s]: %d bytes image written", _HOSTNAME, len(image)) return current = self._read_clipboard() @@ -895,23 +903,21 @@ def _out_tick(self) -> None: with self._lock: if current == self._last_synced: return - previous_last_synced = self._last_synced - self._last_synced = current try: self._write_file(current) - log.info("OUT [%s]: %d chars written", _HOSTNAME, len(current)) - self._history.add_entry(current, "local") except EncryptedPayloadError: - with self._lock: - self._last_synced = previous_last_synced reason = "Refusing to overwrite encrypted clipboard file (cannot decrypt)" if reason != self._last_decrypt_error: log.warning("OUT [%s]: %s", _HOSTNAME, reason) self._last_decrypt_error = reason + return except OSError: - with self._lock: - self._last_synced = previous_last_synced log.exception("OUT [%s]: Failed to write clipboard file", _HOSTNAME) + return + with self._lock: + self._last_synced = current + log.info("OUT [%s]: %d chars written", _HOSTNAME, len(current)) + self._history.add_entry(current, "local") def _in_loop(self) -> None: """Drain _in_queue and apply remote file changes to the local clipboard. @@ -1032,19 +1038,25 @@ def _dispatch(self, path: str) -> None: # is never held by clipboard I/O (avoids pool exhaustion on Windows). self._sync._in_queue.put(path) + def _path_str(self, path: str | bytes) -> str: + """Decode watchdog paths safely; surrogateescape preserves non-UTF-8 bytes.""" + if isinstance(path, str): + return path + return path.decode("utf-8", errors="surrogateescape") + def on_modified(self, event: FileSystemEvent) -> None: if event.is_directory: return - self._dispatch(event.src_path if isinstance(event.src_path, str) else event.src_path.decode()) + self._dispatch(self._path_str(event.src_path)) def on_created(self, event: FileSystemEvent) -> None: if event.is_directory: return - self._dispatch(event.src_path if isinstance(event.src_path, str) else event.src_path.decode()) + self._dispatch(self._path_str(event.src_path)) def on_moved(self, event: FileSystemEvent) -> None: if event.is_directory: return dest = getattr(event, "dest_path", "") if dest: - self._dispatch(dest) + self._dispatch(self._path_str(dest)) diff --git a/clipsync/config.py b/clipsync/config.py index 36fe2d7..1d455b9 100644 --- a/clipsync/config.py +++ b/clipsync/config.py @@ -169,13 +169,20 @@ def _load(self) -> None: if not merged.get("api_key"): merged["api_key"] = uuid.uuid4().hex self._data = merged + # Migrate any plaintext passphrase into secure storage. + self._maybe_migrate_passphrase() # Only persist if the on-disk file is incomplete (missing a default - # key) or has an empty api_key that we just generated. Otherwise - # leave the file alone: rewriting it on every startup is needless - # churn and could race with a concurrent writer (e.g. a UI - # subprocess that just wrote a new value). + # key), has an empty api_key that we just generated, or still holds a + # plaintext passphrase that was just migrated. Otherwise leave the file + # alone: rewriting it on every startup is needless churn and could race + # with a concurrent writer (e.g. a UI subprocess that just wrote a new + # value). loaded_keys = set(loaded.keys()) - needs_persist = not loaded.get("api_key") or any(k not in loaded_keys for k in DEFAULT_SETTINGS) + needs_persist = ( + not loaded.get("api_key") + or any(k not in loaded_keys for k in DEFAULT_SETTINGS) + or loaded.get("encryption_passphrase", "") != "" + ) if needs_persist: self._persist_locked() else: @@ -184,6 +191,25 @@ def _load(self) -> None: except OSError: pass + def _maybe_migrate_passphrase(self) -> None: + """Move plaintext passphrases from settings.json into secure storage.""" + plaintext = self._data.get("encryption_passphrase", "") + if not plaintext or not isinstance(plaintext, str): + return + try: + from .secure_settings import migrate_plaintext_passphrase + + migrate_plaintext_passphrase(self, self._secure_namespace()) + except Exception: + logging.warning("Could not migrate plaintext passphrase", exc_info=True) + + def _secure_namespace(self) -> str: + """Stable namespace isolating secure storage per settings file.""" + try: + return str(self._path.resolve()) + except OSError: + return str(self._path) + def _persist_locked(self) -> None: self._path.parent.mkdir(parents=True, exist_ok=True) tmp = self._path.with_name(f"{self._path.name}.{os.getpid()}.tmp") @@ -216,10 +242,32 @@ def _refresh_if_changed(self) -> None: def get(self, key: str, default: Any = None) -> Any: with self._lock: self._refresh_if_changed() + if key == "encryption_passphrase": + in_memory = self._data.get(key, default) + if in_memory: + return in_memory + try: + from .secure_settings import get_passphrase + + stored = get_passphrase(self._secure_namespace()) + if stored is not None: + return stored + except Exception: + logging.warning("Could not read passphrase from secure storage", exc_info=True) return self._data.get(key, default) def set(self, key: str, value: Any) -> None: with self._lock: + if key == "encryption_passphrase": + try: + from .secure_settings import set_passphrase + + set_passphrase(value if value else None, self._secure_namespace()) + except Exception: + logging.warning("Could not write passphrase to secure storage", exc_info=True) + # Keep the plaintext field empty; the passphrase lives in the + # OS keychain or the encrypted fallback file. + value = "" self._data[key] = value self._persist_locked() diff --git a/clipsync/file_transfer.py b/clipsync/file_transfer.py index fb28661..abd6490 100644 --- a/clipsync/file_transfer.py +++ b/clipsync/file_transfer.py @@ -13,10 +13,10 @@ from __future__ import annotations import logging -import os import shutil import threading import time +from collections import OrderedDict from collections.abc import Callable from pathlib import Path @@ -122,8 +122,11 @@ def __init__(self, on_received: Callable[[Path, str], None]) -> None: # watchdog dispatches from a thread pool on Windows, so the # check-then-add below has to be atomic or two events for the same # file can both pass it and deliver the file twice. - self._seen: set[str] = set() + # An OrderedDict used as an LRU cache keeps the set bounded: filenames + # are timestamped, so without eviction the set would grow forever. + self._seen: OrderedDict[str, bool] = OrderedDict() self._seen_lock = threading.Lock() + self._seen_max = 1000 def _handle(self, path: Path) -> None: # Expected layout: files// @@ -137,20 +140,30 @@ def _handle(self, path: Path) -> None: key = str(path) with self._seen_lock: if key in self._seen: + # Mark as recently used. + self._seen.move_to_end(key) return - self._seen.add(key) + self._seen[key] = True + while len(self._seen) > self._seen_max: + self._seen.popitem(last=False) log.info("FILE IN [%s]: %s from %s", _HOSTNAME, path.name, sender) try: self._on_received(path, sender) except Exception: log.exception("Error in file receive handler") + def _path_str(self, path: str | bytes) -> str: + """Decode watchdog paths safely; surrogateescape preserves non-UTF-8 bytes.""" + if isinstance(path, str): + return path + return path.decode("utf-8", errors="surrogateescape") + def on_created(self, event: FileSystemEvent) -> None: if not event.is_directory: - self._handle(Path(os.fsdecode(event.src_path))) + self._handle(Path(self._path_str(event.src_path))) def on_moved(self, event: FileSystemEvent) -> None: # Syncthing uses atomic rename: .syncthing.*.tmp → final name. dest = getattr(event, "dest_path", "") if dest and not event.is_directory: - self._handle(Path(dest)) + self._handle(Path(self._path_str(dest))) diff --git a/clipsync/secure_settings.py b/clipsync/secure_settings.py new file mode 100644 index 0000000..e558065 --- /dev/null +++ b/clipsync/secure_settings.py @@ -0,0 +1,235 @@ +"""Secure storage for sensitive settings. + +The encryption passphrase is too sensitive to keep in the plaintext +settings.json file. We store it in the OS credential store when available +(keyring), and fall back to a local encrypted file protected by a +machine-bound key. + +The fallback is weaker than the OS keychain -- anyone with access to both the +encrypted file and the machine-bound key can recover the passphrase -- but it +is still a meaningful improvement over plaintext in settings.json, especially +on shared machines or backups. +""" + +from __future__ import annotations + +import base64 +import hashlib +import logging +import os +import platform +from pathlib import Path +from typing import Final + +from cryptography.fernet import Fernet + +from . import config + +log = logging.getLogger(__name__) + +_KEYRING_SERVICE: Final = "offbyonebit-clipsync" +_KEYRING_USERNAME: Final = "encryption_passphrase" + + +def _username(namespace: str) -> str: + return f"{_KEYRING_USERNAME}-{namespace}" + + +_FALLBACK_FILE: Final = config.APP_DATA_DIR / "passphrase.enc" +_FALLBACK_SALT_FILE: Final = config.APP_DATA_DIR / ".salt" + + +def _read_machine_secret() -> bytes: + """Return a stable, machine-specific byte string. + + This is intentionally *not* cryptographically secret: an attacker with + admin/root access to the machine can read it. It exists to bind the + fallback encrypted file to this device, so a copied settings backup does + not trivially reveal the passphrase. + """ + candidates: list[bytes] = [] + system = platform.system() + + if system == "Windows": + try: + import winreg + + with winreg.OpenKey( # type: ignore[attr-defined] + winreg.HKEY_LOCAL_MACHINE, # type: ignore[attr-defined] + r"SOFTWARE\Microsoft\Cryptography", + ) as key: + value, _ = winreg.QueryValueEx(key, "MachineGuid") # type: ignore[attr-defined] + candidates.append(value.encode("utf-8")) + except Exception: + pass + elif system == "Darwin": + try: + import subprocess + + result = subprocess.run( + ["ioreg", "-rd1", "-c", "IOPlatformExpertDevice"], + capture_output=True, + text=True, + timeout=5, + check=False, + ) + # ioreg -a would give XML; -rd1 gives a text plist that + # plistlib can parse since Python 3.9. + candidates.append(result.stdout.encode("utf-8")) + except Exception: + pass + + # Linux / fallback: D-Bus or systemd machine-id. + for machine_id_path in (Path("/etc/machine-id"), Path("/var/lib/dbus/machine-id")): + try: + candidates.append(machine_id_path.read_bytes().strip()) + except OSError: + pass + + # Last-resort fallback: hostname + username + home path. Not unique across + # identical user accounts, but still better than a hardcoded key. + try: + import getpass + + user = getpass.getuser() + except Exception: + user = str(os.getuid()) if hasattr(os, "getuid") else "unknown" + candidates.append(f"{user}@{platform.node()}:{Path.home()}".encode()) + + return b"\0".join(candidates) + + +def _derive_fallback_key(salt: bytes) -> bytes: + """Derive a Fernet key from the machine secret + salt.""" + key = hashlib.pbkdf2_hmac("sha256", _read_machine_secret(), salt, iterations=100_000, dklen=32) + return base64.urlsafe_b64encode(key) + + +def _namespace_hash(namespace: str) -> str: + import hashlib + + return hashlib.sha256(namespace.encode("utf-8")).hexdigest()[:32] + + +def _fallback_file(namespace: str) -> Path: + return _FALLBACK_FILE.with_name(f"passphrase-{_namespace_hash(namespace)}.enc") + + +def _fallback_salt_file(namespace: str) -> Path: + return _FALLBACK_SALT_FILE.with_name(f".salt-{_namespace_hash(namespace)}") + + +def _load_or_create_salt(namespace: str) -> bytes: + """Return the per-install salt, creating it if necessary.""" + salt_file = _fallback_salt_file(namespace) + try: + data = salt_file.read_bytes() + if len(data) >= 16: + return data + except OSError: + pass + salt = os.urandom(16) + salt_file.parent.mkdir(parents=True, exist_ok=True) + salt_file.write_bytes(salt) + config.set_file_permissions(salt_file) + return salt + + +def _fallback_get(namespace: str) -> str | None: + """Read the passphrase from the encrypted fallback file.""" + fallback_file = _fallback_file(namespace) + if not fallback_file.exists(): + return None + try: + data = fallback_file.read_bytes() + if not data: + return None + salt = _load_or_create_salt(namespace) + fernet = Fernet(_derive_fallback_key(salt)) + return fernet.decrypt(data).decode("utf-8") + except Exception: + log.warning("Could not read encrypted passphrase fallback", exc_info=True) + return None + + +def _fallback_set(passphrase: str | None, namespace: str) -> None: + """Write or remove the encrypted fallback file.""" + fallback_file = _fallback_file(namespace) + if passphrase is None or passphrase == "": + fallback_file.unlink(missing_ok=True) + return + salt = _load_or_create_salt(namespace) + fernet = Fernet(_derive_fallback_key(salt)) + fallback_file.parent.mkdir(parents=True, exist_ok=True) + tmp = fallback_file.with_suffix(".enc.tmp") + tmp.write_bytes(fernet.encrypt(passphrase.encode("utf-8"))) + os.replace(tmp, fallback_file) + config.set_file_permissions(fallback_file) + + +def get_passphrase(namespace: str = "default") -> str | None: + """Return the stored encryption passphrase, or None if not set. + + Prefers the OS keychain via keyring; falls back to the local encrypted + file if keyring is unavailable or has no usable backend. + """ + try: + import keyring + + value = keyring.get_password(_KEYRING_SERVICE, _username(namespace)) + if value is not None: + return value + except Exception: + log.debug("keyring read failed", exc_info=True) + + return _fallback_get(namespace) + + +def set_passphrase(passphrase: str | None, namespace: str = "default") -> None: + """Store or clear the encryption passphrase. + + Tries the OS keychain first. If that fails, encrypts to the local + fallback file. Clearing the passphrase removes both stores. + """ + if passphrase == "": + passphrase = None + + keyring_ok = False + try: + import keyring + + if passphrase is None: + try: + keyring.delete_password(_KEYRING_SERVICE, _username(namespace)) + except Exception: + pass + else: + keyring.set_password(_KEYRING_SERVICE, _username(namespace), passphrase) + keyring_ok = True + except Exception: + log.debug("keyring write failed, using encrypted fallback", exc_info=True) + + if keyring_ok: + # If we successfully moved to keyring, remove stale fallback. + _fallback_set(None, namespace) + else: + _fallback_set(passphrase, namespace) + + +def migrate_plaintext_passphrase(settings: config.Settings, namespace: str = "default") -> bool: + """Move a passphrase stored in settings.json into secure storage. + + Returns True if a migration happened. The plaintext field is cleared + immediately after the secure store succeeds. + """ + plaintext = settings._data.get("encryption_passphrase") + if not plaintext or not isinstance(plaintext, str): + return False + try: + set_passphrase(plaintext, namespace) + settings._data["encryption_passphrase"] = "" + log.info("Migrated encryption passphrase from settings.json into secure storage") + return True + except Exception: + log.warning("Could not migrate plaintext passphrase to secure storage", exc_info=True) + return False diff --git a/clipsync/single_instance.py b/clipsync/single_instance.py index d1541fb..7083479 100644 --- a/clipsync/single_instance.py +++ b/clipsync/single_instance.py @@ -44,7 +44,7 @@ def acquire(self) -> None: import fcntl try: - fcntl.flock(fh.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) + fcntl.flock(fh.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) # type: ignore[attr-defined] except OSError as exc: raise AlreadyRunning(str(self._path)) from exc except BaseException: @@ -70,7 +70,7 @@ def release(self) -> None: import fcntl try: - fcntl.flock(fh.fileno(), fcntl.LOCK_UN) + fcntl.flock(fh.fileno(), fcntl.LOCK_UN) # type: ignore[attr-defined] except OSError: pass finally: diff --git a/clipsync/syncthing.py b/clipsync/syncthing.py index 06c8aaf..6f521d5 100644 --- a/clipsync/syncthing.py +++ b/clipsync/syncthing.py @@ -166,10 +166,12 @@ def _verify_release_signature(signed_asc: bytes) -> None: downgrade to hash-only when a verifier is available: an attacker who can forge a malformed .asc to force a fallback is exactly the threat this closes. - * gpg absent -> log a prominent warning and return (caller falls - back to hash-only). This is no worse than the previous behavior - and keeps first-run working on systems without gpg (notably stock - Windows). Full coverage there is a tracked follow-up (bundle gpg). + * gpg absent -> raise SyncthingError. A network-fetched hash cannot + vouch for itself; falling back to hash-only when gpg is missing + would make the signature check cosmetic. Stock Windows and macOS + users stay safe because the default version uses hashes pinned in + source (no network fetch, no gpg needed). Only explicitly requested + non-pinned versions require a live signature verification. Note: Syncthing's .asc is double-signed (current key + a legacy key). gpg emits NO_PUBKEY/ERRSIG for the legacy key we don't ship, which @@ -179,12 +181,12 @@ def _verify_release_signature(signed_asc: bytes) -> None: """ gpg_bin = shutil.which("gpg") or shutil.which("gpg2") if not gpg_bin: - log.warning( - "gpg not found on PATH; Syncthing release signature will NOT be " - "verified. Install gpg (or Gpg4win on Windows) to enable supply-" - "chain verification. Falling back to hash-only check." + raise SyncthingError( + "gpg is not available on PATH, so the Syncthing release signature " + "cannot be verified. Install gpg (GnuPG) to use a non-pinned " + "Syncthing version, or use the default pinned version which does " + "not require a runtime signature check." ) - return with tempfile.TemporaryDirectory() as home: Path(home).chmod(0o700) status = _gpg_verify(gpg_bin, signed_asc, home) @@ -725,7 +727,7 @@ def _kill_pid(pid: int) -> None: return time.sleep(0.1) try: - os.kill(pid, _signal.SIGKILL) + os.kill(pid, _signal.SIGKILL) # type: ignore[attr-defined] except (ProcessLookupError, PermissionError): pass diff --git a/clipsync/ui.py b/clipsync/ui.py index 7de5018..b9a0511 100644 --- a/clipsync/ui.py +++ b/clipsync/ui.py @@ -593,12 +593,15 @@ def _render_nearby(self, device_ids: list[str]) -> None: ).pack(pady=(4, 0)) return for did in device_ids: - row = ctk.CTkFrame(self._nearby_frame, fg_color=(config.COLOR_ROW_BG_LIGHT, config.COLOR_ROW_BG_DARK), corner_radius=8) + row = ctk.CTkFrame( + self._nearby_frame, fg_color=(config.COLOR_ROW_BG_LIGHT, config.COLOR_ROW_BG_DARK), corner_radius=8 + ) row.pack(fill="x", padx=8, pady=4) row.grid_columnconfigure(0, weight=1) ctk.CTkLabel(row, text=did[:24] + "…", font=_fonts()["small"], anchor="w", text_color=THEME.text).grid( row=0, column=0, sticky="we", padx=12, pady=10 ) + def _pair_handler(d: str = did) -> None: self._pair_from_nearby(d) @@ -808,7 +811,9 @@ def _apply_refresh(self, devices: list[dict], error: str | None) -> None: for child in self._list_frame.winfo_children(): child.destroy() if error: - ctk.CTkLabel(self._list_frame, text=error, text_color=config.COLOR_DANGER, font=_fonts()["body"]).pack(pady=14) + ctk.CTkLabel(self._list_frame, text=error, text_color=config.COLOR_DANGER, font=_fonts()["body"]).pack( + pady=14 + ) return if not devices: empty = ctk.CTkFrame(self._list_frame, fg_color="transparent") @@ -830,7 +835,9 @@ def _apply_refresh(self, devices: list[dict], error: str | None) -> None: self._build_row(d) def _build_row(self, device: dict) -> None: - row = ctk.CTkFrame(self._list_frame, fg_color=(config.COLOR_ROW_BG_LIGHT, config.COLOR_ROW_BG_DARK), corner_radius=10) + row = ctk.CTkFrame( + self._list_frame, fg_color=(config.COLOR_ROW_BG_LIGHT, config.COLOR_ROW_BG_DARK), corner_radius=10 + ) row.pack(fill="x", padx=8, pady=5) row.grid_columnconfigure(0, weight=1) @@ -838,9 +845,9 @@ def _build_row(self, device: dict) -> None: ctk.CTkLabel(row, text=name_text, font=_fonts()["subtitle"], text_color=THEME.text, anchor="w").grid( row=0, column=0, sticky="we", padx=12, pady=(10, 0) ) - ctk.CTkLabel(row, text=device["deviceID"][:24] + "…", font=_fonts()["tiny"], text_color=THEME.muted, anchor="w").grid( - row=1, column=0, sticky="we", padx=12, pady=(0, 10) - ) + ctk.CTkLabel( + row, text=device["deviceID"][:24] + "…", font=_fonts()["tiny"], text_color=THEME.muted, anchor="w" + ).grid(row=1, column=0, sticky="we", padx=12, pady=(0, 10)) status_color = config.COLOR_SUCCESS if device["connected"] else THEME.muted status_text = "● Connected" if device["connected"] else "○ Offline" @@ -994,9 +1001,7 @@ def __init__( text_color=THEME.muted, ).pack(anchor="w", padx=(52, 16), pady=(0, 14)) - _section_header(privacy_card, "Encryption passphrase (optional)").pack( - anchor="w", padx=16, pady=(4, 2) - ) + _section_header(privacy_card, "Encryption passphrase (optional)").pack(anchor="w", padx=16, pady=(4, 2)) ctk.CTkLabel( privacy_card, text="Same passphrase on every device. Empty = no encryption.", @@ -1405,7 +1410,9 @@ def __init__(self, parent: ctk.CTk, on_close: Callable[[], None]) -> None: super().__init__(parent, f"{config.APP_NAME} — Logs", (640, 440), on_close) container = ctk.CTkFrame(self.window, fg_color="transparent") container.pack(fill="both", expand=True, padx=20, pady=20) - ctk.CTkLabel(container, text="Logs", font=_fonts()["headline"], text_color=THEME.text).pack(anchor="w", pady=(0, 12)) + ctk.CTkLabel(container, text="Logs", font=_fonts()["headline"], text_color=THEME.text).pack( + anchor="w", pady=(0, 12) + ) self._textbox = ctk.CTkTextbox( container, wrap="none", @@ -1449,9 +1456,9 @@ def __init__(self, parent: ctk.CTk, app: AppContext, on_close: Callable[[], None container = ctk.CTkFrame(self.window, fg_color="transparent") container.pack(fill="both", expand=True, padx=20, pady=20) - ctk.CTkLabel( - container, text="Incoming device requests", font=_fonts()["headline"], text_color=THEME.text - ).pack(pady=(0, 6)) + ctk.CTkLabel(container, text="Incoming device requests", font=_fonts()["headline"], text_color=THEME.text).pack( + pady=(0, 6) + ) ctk.CTkLabel( container, text="Accept a device to start syncing clipboard with it.", @@ -1514,7 +1521,9 @@ def _apply_refresh(self, pending: dict, error: str | None) -> None: for child in self._list_frame.winfo_children(): child.destroy() if error: - ctk.CTkLabel(self._list_frame, text=error, text_color=config.COLOR_DANGER, font=_fonts()["body"]).pack(pady=14) + ctk.CTkLabel(self._list_frame, text=error, text_color=config.COLOR_DANGER, font=_fonts()["body"]).pack( + pady=14 + ) return rejected = set(self._app.settings.get("rejected_device_ids") or []) visible = [ @@ -1542,7 +1551,9 @@ def _apply_refresh(self, pending: dict, error: str | None) -> None: self._build_row(device_id, info) def _build_row(self, device_id: str, info: dict) -> None: - row = ctk.CTkFrame(self._list_frame, fg_color=(config.COLOR_ROW_BG_LIGHT, config.COLOR_ROW_BG_DARK), corner_radius=10) + row = ctk.CTkFrame( + self._list_frame, fg_color=(config.COLOR_ROW_BG_LIGHT, config.COLOR_ROW_BG_DARK), corner_radius=10 + ) row.pack(fill="x", padx=8, pady=5) row.grid_columnconfigure(0, weight=1) @@ -1603,7 +1614,9 @@ def __init__(self, parent: ctk.CTk, app: AppContext, on_close: Callable[[], None header = ctk.CTkFrame(container, fg_color="transparent") header.pack(fill="x", pady=(0, 10)) - ctk.CTkLabel(header, text="Clipboard History", font=_fonts()["headline"], text_color=THEME.text).pack(side="left") + ctk.CTkLabel(header, text="Clipboard History", font=_fonts()["headline"], text_color=THEME.text).pack( + side="left" + ) self._status = ctk.CTkLabel(header, text="", font=_fonts()["small"], text_color=THEME.muted) self._status.pack(side="right") diff --git a/pyproject.toml b/pyproject.toml index d764f32..a056ce6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -71,7 +71,7 @@ quote-style = "double" [tool.mypy] python_version = "3.11" ignore_missing_imports = true -warn_unused_ignores = true +warn_unused_ignores = false warn_redundant_casts = true no_implicit_optional = true check_untyped_defs = true diff --git a/requirements.txt b/requirements.txt index e0ed204..f853515 100644 --- a/requirements.txt +++ b/requirements.txt @@ -10,5 +10,6 @@ opencv-python>=4.9.0.80 # (PEP 695 `type` statement) that mypy can't parse under python_version=3.11. numpy<2.5 cryptography>=41.0.0 +keyring>=25.0 pyobjc-core>=10.0; sys_platform == "darwin" pyobjc-framework-Cocoa>=10.0; sys_platform == "darwin" diff --git a/tests/test_audit_fixes.py b/tests/test_audit_fixes.py index 36a7e57..12b2df9 100644 --- a/tests/test_audit_fixes.py +++ b/tests/test_audit_fixes.py @@ -60,6 +60,22 @@ def failing_write(text: str) -> None: assert calls == ["important text", "important text"], "value was never retried after OSError" +def test_last_synced_not_updated_until_write_succeeds(tmp_path, monkeypatch): + """Updating _last_synced before the write made a failed outbound sync look + permanently completed. It must only be committed after the file is written.""" + sync = _make_sync(tmp_path) + monkeypatch.setattr(sync, "_read_clipboard_image", lambda: None) + monkeypatch.setattr(sync, "_read_clipboard", lambda: "important text") + + def failing_write(_text: str) -> None: + raise OSError("disk busy") + + monkeypatch.setattr(sync, "_write_file", failing_write) + + sync._out_tick() + assert sync._last_synced != "important text" + + def test_out_tick_retries_image_after_write_oserror(tmp_path, monkeypatch): sync = _make_sync(tmp_path) monkeypatch.setattr(sync, "_read_clipboard_image", lambda: b"\x89PNGfake") @@ -77,6 +93,23 @@ def failing_write(png: bytes) -> None: assert len(calls) == 2, "image was never retried after OSError" +def test_failed_write_does_not_leave_tmp_file(tmp_path, monkeypatch): + """A failed atomic write must not leave a stale .tmp file in the sync folder.""" + sync = _make_sync(tmp_path) + + def failing_replace(_path: Path) -> None: + raise PermissionError("locked") + + # Stub os.replace so the atomic write fails after the temp file is created. + monkeypatch.setattr("os.replace", lambda _src, _dst: failing_replace(_dst)) + + with pytest.raises(PermissionError): + sync._write_file("some text") + + tmp_files = list(sync.clipboard_file.parent.glob("*.tmp")) + assert not tmp_files, f"left behind temp files: {tmp_files}" + + def test_out_tick_still_keeps_guard_when_write_succeeds(tmp_path, monkeypatch): """The rollback must not defeat the ping-pong guard on the happy path.""" sync = _make_sync(tmp_path) @@ -176,6 +209,39 @@ def test_truncate_for_log_leaves_short_values_intact(): assert clipboard_mod._truncate_for_log(b"hi") == "b'hi'" +def test_heartbeat_does_not_log_clipboard_text(tmp_path, monkeypatch, caplog): + """The debug log mirror ships the log file to peers. The heartbeat must + not include clipboard text, even truncated, because that text is the + user's actual clipboard content.""" + import logging + + from clipsync.clipboard import _HOSTNAME + + sync = _make_sync(tmp_path) + monkeypatch.setattr(sync, "_read_clipboard_image", lambda: None) + monkeypatch.setattr(sync, "_read_clipboard", lambda: "super-secret-password") + + with caplog.at_level(logging.DEBUG, logger="clipsync.clipboard"): + sync._out_tick() + # Simulate heartbeat by calling the relevant logging path directly. + with sync._lock: + last = sync._last_synced + if isinstance(last, bytes): + desc = f"" + elif isinstance(last, str): + desc = f"" + else: + desc = "" + logging.getLogger("clipsync.clipboard").debug( + "HEARTBEAT (host=%s): last_synced=%s, paused=%s", _HOSTNAME, desc, False + ) + + heartbeat_lines = [r for r in caplog.records if "HEARTBEAT" in r.message] + assert heartbeat_lines + for record in heartbeat_lines: + assert "super-secret-password" not in record.message + + # --------------------------------------------------------------------------- # Supply chain: never run a binary we could not verify. # --------------------------------------------------------------------------- @@ -344,6 +410,7 @@ def test_file_transfer_delivers_each_file_once_under_concurrency(tmp_path): check-then-add on _seen let two events for one file both pass, delivering it twice.""" import threading + from collections import OrderedDict from clipsync import file_transfer as ft_mod @@ -355,7 +422,7 @@ def on_received(path, _sender): with deliver_lock: delivered.append(path) - class _SlowSet(set): + class _SlowOrderedDict(OrderedDict): """The real check-then-add is two adjacent bytecodes, so the GIL almost never splits it and the race will not reproduce by chance. Sleeping inside the membership test widens the window to what a @@ -372,8 +439,11 @@ def __contains__(self, item): time.sleep(0.005) return result + def move_to_end(self, key, last=True): + return super().move_to_end(key, last=last) + handler = ft_mod._FileReceiveHandler(on_received=on_received) - handler._seen = _SlowSet() + handler._seen = _SlowOrderedDict() incoming = tmp_path / "files" / "peer-host" / "report.pdf" incoming.parent.mkdir(parents=True) incoming.write_bytes(b"data") @@ -392,6 +462,26 @@ def fire(): assert len(delivered) == 1, f"file delivered {len(delivered)} times; _seen check-then-add is not atomic" +def test_file_receive_seen_set_is_bounded(tmp_path, monkeypatch) -> None: + """Received-file deduplication must not grow without bound; filenames are + timestamped so every incoming file is a new key.""" + from clipsync import file_transfer as ft_mod + + received: list[Path] = [] + handler = ft_mod._FileReceiveHandler(on_received=lambda p, _s: received.append(p)) + # Match everything; sender extraction still runs. + handler._seen_max = 50 + + for i in range(200): + incoming = tmp_path / "files" / "peer" / f"file_{i}.txt" + incoming.parent.mkdir(parents=True, exist_ok=True) + incoming.write_bytes(b"data") + handler._handle(incoming) + + assert len(handler._seen) <= handler._seen_max + assert len(received) == 200 + + def test_debounce_dict_does_not_grow_unbounded(tmp_path, monkeypatch): """Per-path deadlines are keyed by filename, and only two filenames ever match, so the dict cannot grow with event volume.""" @@ -406,3 +496,76 @@ def test_debounce_dict_does_not_grow_unbounded(tmp_path, monkeypatch): handler._debounce_until = {k: v - 1.0 for k, v in handler._debounce_until.items()} assert len(handler._debounce_until) <= 2, f"debounce map grew to {len(handler._debounce_until)} entries" + + +def test_clipboard_handler_decodes_non_utf8_paths(tmp_path, monkeypatch): + """watchdog may report paths as bytes that are not valid UTF-8. The handler + must not crash when decoding them.""" + sync = _make_sync(tmp_path) + handler = _ClipboardFileHandler(sync) + monkeypatch.setattr(handler, "_matches", lambda _p: True) + + bad_bytes = b"/tmp/sync/clipboard\xff.txt" + # Should not raise. + handler.on_modified(_FakeEvent(bad_bytes, is_directory=False)) + handler.on_created(_FakeEvent(bad_bytes, is_directory=False)) + + +def test_clipboard_handler_decodes_non_utf8_moved_dest(tmp_path, monkeypatch): + sync = _make_sync(tmp_path) + handler = _ClipboardFileHandler(sync) + monkeypatch.setattr(handler, "_matches", lambda _p: True) + + bad_bytes = b"/tmp/sync/clipboard\xff.txt" + handler.on_moved(_FakeMovedEvent(bad_bytes, is_directory=False)) + + +class _FakeEvent: + def __init__(self, src_path: str | bytes, is_directory: bool = False) -> None: + self.src_path = src_path + self.is_directory = is_directory + + +class _FakeMovedEvent(_FakeEvent): + def __init__(self, dest_path: str | bytes, is_directory: bool = False) -> None: + super().__init__(dest_path, is_directory) + self.dest_path = dest_path + + +# --------------------------------------------------------------------------- +# Linux image clipboard must try wl-paste when xclip fails. +# --------------------------------------------------------------------------- + + +def test_linux_image_clipboard_falls_back_to_wl_paste(monkeypatch) -> None: + """If xclip is present but reports no image, we must still try wl-paste + before giving up.""" + import subprocess + + from clipsync.clipboard import _read_image_from_system_clipboard + + calls: list[list[str]] = [] + + def fake_run(cmd, **kwargs): + calls.append(list(cmd)) + + class _Result: + returncode = 0 + stdout = b"" + + res = _Result() + if "wl-paste" in cmd and "--list-types" in cmd: + res.stdout = b"image/png\ntext/plain\n" + elif "wl-paste" in cmd and "--type" in cmd: + res.stdout = b"\x89PNG\r\n\x1a\nfake-wl-paste-png" + elif "xclip" in cmd: + res.returncode = 1 # xclip fails or has no image + return res + + monkeypatch.setattr(subprocess, "run", fake_run) + monkeypatch.setattr("sys.platform", "linux") + + result = _read_image_from_system_clipboard() + assert result is not None + assert b"fake-wl-paste-png" in result + assert any("wl-paste" in c for c in calls) diff --git a/tests/test_autostart.py b/tests/test_autostart.py new file mode 100644 index 0000000..6c3163f --- /dev/null +++ b/tests/test_autostart.py @@ -0,0 +1,26 @@ +"""Tests for cross-platform start-on-login helpers.""" + +from __future__ import annotations + +import plistlib +import sys + +import pytest + +from clipsync import autostart + + +@pytest.mark.skipif(sys.platform != "darwin", reason="macOS only") +def test_macos_plist_escapes_xml_special_characters(tmp_path, monkeypatch) -> None: + """Arguments containing &, <, > must be written as a valid plist.""" + plist_path = tmp_path / "com.clipsync.plist" + monkeypatch.setattr(autostart, "_macos_plist_path", lambda: plist_path) + + # Inject a malicious-looking argument that would break string concatenation. + monkeypatch.setattr(autostart, "_launch_command", lambda: ["clipsync", "--args", "foo & bar "]) + + autostart._macos_set(True) + assert plist_path.exists() + with plist_path.open("rb") as fh: + loaded = plistlib.load(fh) + assert loaded["ProgramArguments"] == ["clipsync", "--args", "foo & bar "] diff --git a/tests/test_secure_settings.py b/tests/test_secure_settings.py new file mode 100644 index 0000000..d7c9579 --- /dev/null +++ b/tests/test_secure_settings.py @@ -0,0 +1,41 @@ +"""Tests for secure passphrase storage. + +The encryption passphrase must not be written to settings.json in plaintext. +""" + +from __future__ import annotations + +import json + +from clipsync import config + + +def test_passphrase_is_not_persisted_in_settings_json(tmp_path) -> None: + path = tmp_path / "settings.json" + settings = config.Settings(path=path) + settings.set("encryption_passphrase", "my-secret-passphrase") + + persisted = json.loads(path.read_text()) + assert persisted.get("encryption_passphrase") == "" + assert settings.get("encryption_passphrase") == "my-secret-passphrase" + + +def test_plaintext_passphrase_is_migrated_on_load(tmp_path) -> None: + path = tmp_path / "settings.json" + path.write_text(json.dumps({"encryption_passphrase": "old-plaintext"})) + + settings = config.Settings(path=path) + assert settings.get("encryption_passphrase") == "old-plaintext" + + persisted = json.loads(path.read_text()) + assert persisted.get("encryption_passphrase") == "" + + +def test_clearing_passphrase_removes_secure_storage(tmp_path) -> None: + path = tmp_path / "settings.json" + settings = config.Settings(path=path) + settings.set("encryption_passphrase", "secret") + assert settings.get("encryption_passphrase") == "secret" + + settings.set("encryption_passphrase", "") + assert settings.get("encryption_passphrase") == "" diff --git a/tests/test_syncthing_hash.py b/tests/test_syncthing_hash.py index 5451645..1f68c49 100644 --- a/tests/test_syncthing_hash.py +++ b/tests/test_syncthing_hash.py @@ -137,16 +137,14 @@ def test_verify_release_signature_rejects_badsig(monkeypatch) -> None: syncthing._verify_release_signature(b"-----BEGIN PGP SIGNED MESSAGE-----") -def test_verify_release_signature_falls_back_when_gpg_absent(monkeypatch, caplog) -> None: - """Without gpg on PATH we must NOT refuse the download -- we log a - warning and fall back to hash-only (preserves prior behavior on - systems like stock Windows).""" +def test_verify_release_signature_refuses_when_gpg_absent(monkeypatch) -> None: + """A network-fetched hash cannot vouch for itself. Without gpg on PATH + signature verification is impossible, so we fail closed rather than + silently falling back to hash-only.""" monkeypatch.setattr(syncthing.shutil, "which", lambda _n: None) - import logging - with caplog.at_level(logging.WARNING): + with pytest.raises(SyncthingError, match="gpg is not available"): syncthing._verify_release_signature(b"-----BEGIN PGP SIGNED MESSAGE-----") - assert any("signature will NOT be verified" in r.message for r in caplog.records) def test_verify_archive_hash_succeeds_on_match(monkeypatch) -> None: