fix: bound the logo download, and stop malformed input reading as a fault

Review findings on the coverage branch.

The download size cap I added checked len(response.content), which has
already buffered the whole body -- it stopped the bytes reaching disk but
not memory, which was the point. A server that omits Content-Length and
never stops sending would still exhaust the process. Stream it instead,
counting as it arrives, into a sibling .part file that is replaced over
the target only once it decodes. A transfer that dies midway now leaves
nothing behind rather than a truncated logo for load_logo() to cache.

The follower's control-message handler caught three exception types, but
two reachable UDP payloads raise others: a bare JSON scalar makes
msg.get() raise AttributeError, and an "sx" carrying a non-numeric x
raises ValueError or TypeError from float(). Those escaped to the outer
handler, skipping the legacy-PNG fallback and -- since this branch added
a backoff there -- charging one malformed packet a 0.1s stall on the
receive path. The legacy-PNG path also decoded without the dimension cap
its TCP counterpart applies, so a crafted 65KB frame could force a large
allocation on the render thread; both paths now share one constant.

Three repo_url handlers called .strip() on client input without checking
it was a string, so {"repo_url": 12345} answered 500. The credentials
upload parsed the same file twice, the second time inside a bare except
that a preceding parse had already made unreachable. And both raw-config
handlers kept a json.JSONDecodeError arm that get_json(silent=True) had
turned into dead code, collapsing "sent something unparseable" into "sent
nothing" -- they now say which.

Two of the new tests were not testing what they claimed. The pruning
round-trip wrote ten backups inside one second, so all ten landed on the
same int(time.time()) filename and overwrote each other; it never reached
the limit it asserted. And the sync clock helper patched attributes on the
stdlib time module, freezing time process-wide for every daemon thread
earlier tests had left running.

Full suite: 3352 passed, coverage 54%.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh

test/test_api_v3_calendar_credentials.py

@@ -22,6 +22,7 @@ import stat
 import sys
 import time
 from pathlib import Path
+from types import SimpleNamespace
 
 import pytest
 
@@ -192,14 +193,25 @@ class TestBackupPruning:
         upload(api_v3_client, VALID_CREDENTIALS)
         assert len(backups(plugin_dir)) == 3  # 2 seeded + 1 new
 
-    def test_repeated_uploads_stay_bounded(self, api_v3_client, plugin_dir):
+    def test_repeated_uploads_stay_bounded(
+            self, api_v3_client, plugin_dir, api_v3_module, monkeypatch):
+        # The backup filename carries int(time.time()), so uploads inside
+        # the same second all write the same name and overwrite each other.
+        # Advance a fake clock a second per round — otherwise this never
+        # reaches six backups and the bound holds for the wrong reason.
+        clock = {"now": int(time.time())}
+        monkeypatch.setattr(
+            api_v3_module, "time", SimpleNamespace(time=lambda: clock["now"]))
         for i in range(10):
+            clock["now"] += 1
             upload(api_v3_client, {"installed": {"round": i}})
-            # Distinct mtimes so ordering is well-defined between rounds.
-            for path in backups(plugin_dir):
-                os.utime(path, (path.stat().st_mtime, path.stat().st_mtime))
-            time.sleep(0.01)
-        assert len(backups(plugin_dir)) <= 5
+            os.utime(plugin_dir / "credentials.json",
+                     (clock["now"], clock["now"]))
+        remaining = backups(plugin_dir)
+        assert len(remaining) == 5
+        # And they are the five most recent rounds, not an arbitrary five.
+        kept = sorted(int(p.name.rsplit(".", 1)[1]) for p in remaining)
+        assert kept == [clock["now"] - 4 + i for i in range(5)]
 
     def test_unremovable_backup_does_not_fail_the_upload(
             self, api_v3_client, plugin_dir, monkeypatch):

test/test_api_v3_registry_endpoints.py

@@ -166,9 +166,14 @@ class TestRegistryFromUrl:
         assert body["message"] == "An error occurred; see logs for details"
         assert "Traceback" not in str(body)
 
-    def test_non_string_repo_url_is_a_500_not_a_crash(
-            self, api_v3_client, api_v3_module):
-        # .strip() on a non-string raises; the handler's catch-all turns
-        # that into a 500 rather than propagating.
+    def test_non_string_repo_url_is_rejected(self, api_v3_client, api_v3_module):
+        # Regression: .strip() on a non-string raised, and the catch-all
+        # reported the caller's own mistake as a server fault.
         response = api_v3_client.post(self.URL, json={"repo_url": 12345})
-        assert response.status_code == 500
+        assert response.status_code == 400
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry_from_url.assert_not_called()
+
+    def test_blank_repo_url_is_rejected(self, api_v3_client, api_v3_module):
+        response = api_v3_client.post(self.URL, json={"repo_url": "   "})
+        assert response.status_code == 400
+        api_v3_module.api_v3.plugin_store_manager.fetch_registry_from_url.assert_not_called()

test/test_logo_helper.py

@@ -21,7 +21,7 @@ from unittest.mock import MagicMock, patch
 
 import pytest
 import requests
-from PIL import Image
+from PIL import Image, UnidentifiedImageError
 
 from src.common.logo_helper import MAX_LOGO_BYTES, LogoHelper
 
@@ -46,10 +46,44 @@ def write_logo(path: Path, size=(20, 20), color=(255, 0, 0), fmt="PNG") -> Path:
     return path
 
 
-def fake_response(content: bytes):
+def fake_response(content: bytes, chunk_size: int = 64 * 1024):
+    """Stand-in for a streamed requests.Response.
+
+    _download_logo opens `with session.get(..., stream=True)` and reads
+    through iter_content(), so the fake has to be a context manager that
+    yields the body in pieces rather than exposing it as .content.
+    Chunking is the fake's own, not the caller's, so a test can dribble a
+    body out in small pieces.
+    """
     response = MagicMock()
-    response.content = content
+    response.__enter__.return_value = response
+    response.__exit__.return_value = False
     response.raise_for_status = MagicMock()
+
+    def _iter_content(*_args, **_kwargs):
+        for i in range(0, len(content), chunk_size):
+            yield content[i:i + chunk_size]
+
+    response.iter_content = _iter_content
+    return response
+
+
+def endless_response(chunk: bytes = b"\x00" * 65536):
+    """A server that declares no length and never stops sending.
+
+    This is the case response.content could not survive: it buffers to
+    completion, so the size check never got a chance to run.
+    """
+    response = MagicMock()
+    response.__enter__.return_value = response
+    response.__exit__.return_value = False
+    response.raise_for_status = MagicMock()
+
+    def _iter_content(*_args, **_kwargs):
+        while True:
+            yield chunk
+
+    response.iter_content = _iter_content
     return response
 
 
@@ -169,7 +203,10 @@ class TestLoadLogoWithDownload:
         logo = helper.load_logo_with_download("PHI", path, "http://x/logo.png")
         assert logo is not None
         assert path.exists()
-        helper.session.get.assert_called_once_with("http://x/logo.png", timeout=30)
+        # stream=True is load-bearing: it is what lets the size cap apply
+        # before the body is buffered.
+        helper.session.get.assert_called_once_with(
+            "http://x/logo.png", timeout=30, stream=True)
 
     def test_download_failure_falls_back_to_placeholder(self, helper, tmp_path):
         helper.session.get = MagicMock(
@@ -215,18 +252,56 @@ class TestDownloadLogo:
         path = tmp_path / "huge.png"
         helper.session.get = MagicMock(
             return_value=fake_response(b"\x00" * (MAX_LOGO_BYTES + 1)))
-        with pytest.raises(ValueError, match="over the"):
+        with pytest.raises(ValueError, match="exceeds the"):
             helper._download_logo("http://x/huge.png", path)
         assert not path.exists()
 
+    def test_unbounded_response_is_aborted_at_the_cap(self, helper, tmp_path):
+        # Regression: the cap used to be checked against response.content,
+        # which buffers the whole body first — so a server that omits
+        # Content-Length and never stops sending exhausted memory before
+        # the check could run. Streaming counts bytes as they arrive, so
+        # this terminates instead of hanging.
+        path = tmp_path / "endless.png"
+        helper.session.get = MagicMock(return_value=endless_response())
+        with pytest.raises(ValueError, match="exceeds the"):
+            helper._download_logo("http://x/endless.png", path)
+        assert not path.exists()
+
+    def test_no_partial_file_is_left_when_the_stream_dies(self, helper, tmp_path):
+        # A transfer that fails midway must not leave a truncated logo
+        # where the real one belongs — load_logo() would cache it.
+        path = tmp_path / "cut.png"
+        real = png_bytes()
+
+        def _dies_midway(*_args, **_kwargs):
+            yield real[:20]
+            raise OSError("connection reset")
+
+        response = MagicMock()
+        response.__enter__.return_value = response
+        response.__exit__.return_value = False
+        response.raise_for_status = MagicMock()
+        response.iter_content = _dies_midway
+        helper.session.get = MagicMock(return_value=response)
+
+        with pytest.raises(OSError):
+            helper._download_logo("http://x/cut.png", path)
+        assert not path.exists()
+        assert list(tmp_path.glob("*.part")) == []
+
     def test_non_image_response_is_deleted_and_raises(self, helper, tmp_path):
         # Regression: undecodable bytes stayed on disk, so every later
         # load_logo() call hit the corrupt file instead of re-downloading.
         path = tmp_path / "bad.png"
         helper.session.get = MagicMock(return_value=fake_response(b"<html>404</html>"))
-        with pytest.raises(Exception):
+        # Specifically Pillow's identify failure, not any OSError: the
+        # point is that the bytes did not decode, and OSError alone would
+        # also admit unrelated filesystem faults.
+        with pytest.raises(UnidentifiedImageError):
             helper._download_logo("http://x/bad.png", path)
         assert not path.exists()
+        assert list(tmp_path.glob("*.part")) == []
 
     def test_decompression_bomb_is_deleted_and_raises(self, helper, tmp_path, monkeypatch):
         path = tmp_path / "bomb.png"

test/test_sync_manager.py

@@ -31,6 +31,7 @@ import socket
 import threading
 import time
 from pathlib import Path
+from types import SimpleNamespace
 from unittest.mock import MagicMock, patch
 
 import numpy as np
@@ -118,11 +119,27 @@ def raise_n_then_stop(mgr, exc, count):
     return _side_effect
 
 
+def fake_clock(monkeypatch, *, time_fn=None, sleep_fn=None):
+    """Swap sync_manager's own `time` reference for a private stand-in.
+
+    sync_manager.time IS the stdlib module, so patching attributes on it
+    would freeze the clock and no-op sleep for the whole process —
+    including the daemon threads earlier tests left running, which is a
+    hard-to-trace source of cross-test flakiness. Rebinding the module's
+    reference keeps the patch scoped to the code under test. Anything not
+    overridden falls through to the real functions.
+    """
+    monkeypatch.setattr(sync_manager, "time", SimpleNamespace(
+        time=time_fn or time.time,
+        sleep=sleep_fn or time.sleep,
+    ))
+
+
 def run_watchdog_once(monkeypatch, mgr, watchdog, now):
     """Run exactly one watchdog iteration at a frozen wall-clock time."""
-    monkeypatch.setattr(sync_manager.time, "time", lambda: now)
-    monkeypatch.setattr(
-        sync_manager.time, "sleep", lambda _: setattr(mgr, "_running", False))
+    fake_clock(monkeypatch,
+               time_fn=lambda: now,
+               sleep_fn=lambda _: setattr(mgr, "_running", False))
     mgr._running = True
     watchdog()
 
@@ -334,11 +351,11 @@ class TestLeaderRecvLoop:
         assert mgr._leader_state is LeaderState.CONNECTED
         assert mgr._peer_ip == "10.0.0.8"
 
-    def test_heartbeat_from_known_peer_refreshes_timer(self):
+    def test_heartbeat_from_known_peer_refreshes_timer(self, monkeypatch):
         mgr = make_manager(role=SyncRole.LEADER)
         mgr._peer_ip = "10.0.0.8"
-        with patch.object(sync_manager.time, "time", return_value=12345.0):
-            self._drive(mgr, json.dumps({"t": "hb"}).encode())
+        fake_clock(monkeypatch, time_fn=lambda: 12345.0)
+        self._drive(mgr, json.dumps({"t": "hb"}).encode())
         assert mgr._last_heartbeat_time == 12345.0
 
     def test_heartbeat_from_stranger_is_ignored(self):
@@ -370,7 +387,7 @@ class TestLeaderRecvLoop:
         mgr._recv_sock = MagicMock()
         mgr._recv_sock.recvfrom.side_effect = raise_n_then_stop(mgr, OSError("boom"), 3)
         sleeps = MagicMock()
-        monkeypatch.setattr(sync_manager.time, "sleep", sleeps)
+        fake_clock(monkeypatch, sleep_fn=sleeps)
         mgr._running = True
         mgr._leader_recv_loop()
         assert sleeps.call_count == 3
@@ -470,6 +487,45 @@ class TestFollowerRecvLoop:
         self._drive(mgr, json.dumps({"t": "nc"}).encode())
         assert calls == [1]
 
+    def test_non_object_json_does_not_reach_the_outer_handler(self):
+        # A bare JSON scalar parses, then msg.get() raises AttributeError.
+        # That has to be caught here so the payload still gets its shot at
+        # the legacy-PNG fallback; escaping to the outer handler would also
+        # charge one malformed packet the 0.1s error backoff.
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+        sleeps = MagicMock()
+        with patch.object(sync_manager, "time",
+                          SimpleNamespace(time=time.time, sleep=sleeps)):
+            self._drive(mgr, b"12345")
+        assert mgr.get_latest_frame() is None
+        sleeps.assert_not_called()
+
+    def test_non_numeric_scroll_x_does_not_reach_the_outer_handler(self):
+        # float("a") raises ValueError; {"x": null} raises TypeError.
+        for payload in ({"t": "sx", "x": "a"}, {"t": "sx", "x": None}):
+            mgr = make_manager(role=SyncRole.FOLLOWER)
+            sleeps = MagicMock()
+            with patch.object(sync_manager, "time",
+                              SimpleNamespace(time=time.time, sleep=sleeps)):
+                self._drive(mgr, json.dumps(payload).encode())
+            assert mgr.get_latest_scroll_x() is None
+            sleeps.assert_not_called()
+
+    def test_oversized_legacy_frame_is_rejected_before_decode(self, monkeypatch):
+        # The UDP path is reachable by any host on the LAN, so it caps
+        # dimensions before load() just as the TCP image server does.
+        mgr = make_manager(role=SyncRole.FOLLOWER)
+
+        class Huge:
+            width, height = 10, sync_manager._MAX_FRAME_H + 1
+
+            def load(self):
+                raise AssertionError("load() must not run past the cap")
+
+        monkeypatch.setattr(sync_manager.Image, "open", lambda *a, **kw: Huge())
+        self._drive(mgr, b"\x89PNG not really but not JSON either")
+        assert mgr.get_latest_frame() is None
+
     def test_scroll_x_missing_key_is_swallowed(self):
         mgr = make_manager(role=SyncRole.FOLLOWER)
         self._drive(mgr, json.dumps({"t": "sx"}).encode())  # no "x"
@@ -480,7 +536,7 @@ class TestFollowerRecvLoop:
         mgr._recv_sock = MagicMock()
         mgr._recv_sock.recvfrom.side_effect = raise_n_then_stop(mgr, OSError("boom"), 3)
         sleeps = MagicMock()
-        monkeypatch.setattr(sync_manager.time, "sleep", sleeps)
+        fake_clock(monkeypatch, sleep_fn=sleeps)
         mgr._running = True
         mgr._follower_recv_loop()
         assert sleeps.call_count == 3
@@ -797,23 +853,71 @@ class TestStop:
         make_manager(role=SyncRole.STANDALONE).stop()  # all sockets None
 
 
-class TestLoopbackHandshake:
+def _broadcast_works(port):
+    """True when this environment can actually deliver a UDP broadcast.
+
+    The handshake below depends on it: the follower announces itself to
+    ("<broadcast>", port). Sandboxes and some CI networks drop or refuse
+    broadcast, and sync_manager swallows the sendto error, so without
+    this probe the test would just wait out its deadline and fail for a
+    reason that has nothing to do with the code.
+    """
+    recv = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
+    send = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
+    try:
+        recv.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+        recv.bind(("", port))
+        recv.settimeout(0.5)
+        send.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1)
+        send.sendto(b"probe", ("<broadcast>", port))
+        return recv.recvfrom(64)[0] == b"probe"
+    except OSError:
+        return False
+    finally:
+        recv.close()
+        send.close()
+
+
+class TestRealSocketHandshake:
     def test_leader_and_follower_negotiate_over_real_sockets(self, monkeypatch):
         # One end-to-end check that the wire format actually round-trips:
         # every other test drives the loops with mocked sockets.
+        #
+        # Not loopback-only, despite the free-port probe below: the manager
+        # binds UDP and TCP on all interfaces and the follower announces by
+        # broadcast. That is the behaviour under test, so the environment
+        # has to support it.
         monkeypatch.setattr(sync_manager, "HELLO_INTERVAL", 0.02)
         monkeypatch.setattr(sync_manager, "HEARTBEAT_INTERVAL", 0.02)
 
-        # Pick a free port by binding one on loopback and releasing it.
-        # Loopback, not "", so this test never opens a port to the network.
-        probe = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
-        probe.bind(("127.0.0.1", 0))
-        port = probe.getsockname()[1]
-        probe.close()
-
         hw = {"rows": 32, "cols": 64, "chain_length": 1}
-        leader = DisplaySyncManager("leader", {"port": port}, hw, MagicMock())
-        follower = DisplaySyncManager("follower", {"port": port}, hw, MagicMock())
+        leader = follower = None
+        # The free-port probe is inherently racy — the port can be taken
+        # between release and rebind — so retry rather than fail on it.
+        for attempt in range(5):
+            probe = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
+            probe.bind(("", 0))
+            port = probe.getsockname()[1]
+            probe.close()
+
+            if not _broadcast_works(port):
+                pytest.skip("environment cannot deliver UDP broadcast")
+
+            try:
+                leader = DisplaySyncManager("leader", {"port": port}, hw, MagicMock())
+                follower = DisplaySyncManager("follower", {"port": port}, hw, MagicMock())
+                break
+            except OSError:
+                # Port taken between probe and bind, or the TCP image
+                # server could not bind port+1. Tear down whichever end
+                # came up before retrying with a fresh port.
+                for mgr in (leader, follower):
+                    if mgr is not None:
+                        mgr.stop()
+                leader = follower = None
+        else:
+            pytest.skip("could not obtain a free port pair for the handshake")
+
         try:
             deadline = time.time() + 5.0
             while time.time() < deadline:

test/web_interface/test_api_v3_config_raw.py

@@ -112,6 +112,11 @@ class TestSaveRawMain:
         assert response.status_code == 400
         body = response.get_json()
         assert body["status"] == "error"
+        # A body that was sent but does not parse is a distinct mistake
+        # from sending none, and says so. Previously the handler's own
+        # json.JSONDecodeError arm was unreachable — Werkzeug raised
+        # first — so this collapsed into "No data provided".
+        assert "Invalid JSON in request body" in body["message"]
 
     def test_config_error_is_a_500_with_context(self, env, monkeypatch):
         def refuse(kind, data):