Cover the next tier of untested modules and endpoints, and fix the 43 bugs that surfaced

.github/workflows/test.yml

@@ -72,4 +72,4 @@ jobs:
             --ignore=test/plugins \
             --cov=src --cov=web_interface \
             --cov-report=term \
-            --cov-fail-under=48
+            --cov-fail-under=52

src/common/logo_helper.py

@@ -6,6 +6,8 @@
 """
 
 import logging
+import os
+import tempfile
 from pathlib import Path
 from typing import Dict, List, Optional, Union
 
@@ -19,6 +21,10 @@
 )
 
 
+# Well above any real team logo; bounds what a remote URL can write to disk.
+MAX_LOGO_BYTES = 10 * 1024 * 1024
+
+
 class LogoHelper:
     """
     Helper class for logo loading, caching, and resizing.
@@ -226,7 +232,10 @@ def get_cache_stats(self) -> Dict[str, int]:
         return {
             'cached_logos': len(self._logo_cache),
             'cache_size_limit': self.cache_size,
-            'cache_usage_percent': (len(self._logo_cache) / self.cache_size) * 100
+            'cache_usage_percent': (
+                (len(self._logo_cache) / self.cache_size) * 100
+                if self.cache_size else 0
+            ),
         }
 
     def _resize_logo(self, logo: Image.Image, max_width: Optional[int] = None, 
@@ -258,21 +267,64 @@ def _cache_logo(self, cache_key: str, logo: Image.Image) -> None:
         self._cache_order.append(cache_key)
 
     def _download_logo(self, url: str, file_path: Path) -> None:
-        """Download logo from URL."""
+        """Download logo from URL.
+
+        The response size is capped and the saved file is verified as a
+        decodable image before it is left on disk: a logo URL is remote
+        input, and without this an oversized or malformed response would
+        be cached for every later load_logo() call to trip over.
+
+        The body is streamed and counted as it arrives rather than read
+        through response.content, which buffers the whole thing first —
+        a server that omits Content-Length and never stops sending would
+        exhaust memory before any size check could run. Nothing lands at
+        file_path until the download completes and decodes, so a failed
+        download cannot leave a truncated logo behind either.
+        """
         # Ensure directory exists with proper permissions
         ensure_directory_permissions(file_path.parent, get_assets_dir_mode())
-
-        # Download with timeout
-        response = self.session.get(url, timeout=30)
-        response.raise_for_status()
-
-        # Save to file
-        with open(file_path, 'wb') as f:
-            f.write(response.content)
-
+
+        # A unique temp name, not a fixed "<name>.part": two plugins can
+        # ask for the same logo at once, and a shared name would let them
+        # interleave writes into one file, publish the mixture, or delete
+        # each other's partial. Same directory, so os.replace stays atomic.
+        fd, tmp_name = tempfile.mkstemp(
+            dir=str(file_path.parent), prefix=file_path.name + '.', suffix='.part')
+        tmp_path = Path(tmp_name)
+        try:
+            # fdopen outermost so the descriptor mkstemp handed back is
+            # always adopted and closed, including when the request itself
+            # raises — load_logo_with_download swallows that, so a leak
+            # here would accumulate quietly on a URL that keeps failing.
+            with os.fdopen(fd, 'wb') as f:
+                with self.session.get(url, timeout=30, stream=True) as response:
+                    response.raise_for_status()
+                    downloaded = 0
+                    for chunk in response.iter_content(chunk_size=64 * 1024):
+                        if not chunk:
+                            continue
+                        downloaded += len(chunk)
+                        if downloaded > MAX_LOGO_BYTES:
+                            raise ValueError(
+                                f"Logo at {url} exceeds the "
+                                f"{MAX_LOGO_BYTES}-byte limit; not saved")
+                        f.write(chunk)
+
+            # Verify it decodes before it becomes the cached logo. PIL
+            # raises DecompressionBombError past its own pixel limit; a
+            # partial or non-image response raises UnidentifiedImageError
+            # (an OSError subclass).
+            with Image.open(tmp_path) as probe:
+                probe.load()
+
+            os.replace(tmp_path, file_path)
+        except BaseException:
+            tmp_path.unlink(missing_ok=True)
+            raise
+
         # Set proper file permissions after saving
         ensure_file_permissions(file_path, get_assets_file_mode())
-        
+
         self.logger.debug(f"Downloaded logo to {file_path}")
 
     def _create_placeholder_logo(self, team_abbr: str, 

src/common/sync_manager.py

@@ -19,6 +19,7 @@
 
 import io
 import json
+import math
 import os
 import socket
 import struct
@@ -37,6 +38,13 @@
 _RAW_HEADER = struct.Struct('<HH')  # width, height (uint16 LE)
 
 
+# Upper bound on a decoded frame/scroll image. Generous for any real scroll
+# image (a leader's full cycle is long but only panel-height tall), and low
+# enough that a crafted image from any host on the LAN cannot force a large
+# allocation on the render thread. Applied on both receive paths — the TCP
+# image server and the follower's legacy-PNG UDP fallback.
+_MAX_FRAME_W, _MAX_FRAME_H = 100_000, 256
+
 SYNC_PORT = 5765
 HELLO_INTERVAL = 5.0       # follower broadcasts hello every 5 s
 HEARTBEAT_INTERVAL = 2.0   # follower sends heartbeat every 2 s
@@ -101,6 +109,7 @@ def __init__(
         self._peer_chain: int = 0
         self._last_heartbeat_time: float = 0.0
         self._leader_width: int = 0  # set by display_controller after init
+        self._oversized_frame_warned: bool = False
 
         # Follower state
         self._follower_state = FollowerState.STANDALONE
@@ -174,6 +183,10 @@ def _leader_recv_loop(self) -> None:
                 continue
             except Exception as exc:
                 self.logger.debug("Sync leader recv error: %s", exc)
+                # Brief backoff: a socket left in a bad state raises
+                # immediately, which would otherwise spin this thread at
+                # 100% CPU logging the same error.
+                time.sleep(0.1)
 
     def _handle_hello(self, msg: dict, sender_ip: str) -> None:
         hw = self._hw_config
@@ -273,11 +286,10 @@ def _image_server_loop(self) -> None:
                             break
                         data.extend(chunk)
                     img = Image.open(io.BytesIO(data))
-                    _MAX_W, _MAX_H = 100_000, 256  # generous for any real scroll image
-                    if img.width > _MAX_W or img.height > _MAX_H:
+                    if img.width > _MAX_FRAME_W or img.height > _MAX_FRAME_H:
                         self.logger.warning(
                             "Sync: rejected oversized scroll image %dx%d (max %dx%d) from %s",
-                            img.width, img.height, _MAX_W, _MAX_H, addr,
+                            img.width, img.height, _MAX_FRAME_W, _MAX_FRAME_H, addr,
                         )
                         continue
                     try:
@@ -396,7 +408,7 @@ def send_frame(self, image: Image.Image) -> None:
             data = header + arr.tobytes()
             if len(data) <= 65000:
                 self._send_sock.sendto(data, (self._peer_ip, self.port))
-            elif not getattr(self, '_oversized_frame_warned', False):
+            elif not self._oversized_frame_warned:
                 self._oversized_frame_warned = True
                 self.logger.warning(
                     "Sync: frame too large for UDP (%d bytes, max 65000) — "
@@ -451,43 +463,76 @@ def _start_follower(self) -> None:
         )
         self.write_status_file()
 
+    def _handle_received_frame(self, img: Image.Image, sender_ip: str) -> None:
+        """Record a decoded leader frame and enter follower mode if needed."""
+        with self._frame_lock:
+            self._latest_frame = img
+        self._last_leader_frame_time = time.time()
+        self._leader_ip = sender_ip
+
+        if self._follower_state == FollowerState.STANDALONE:
+            self._follower_state = FollowerState.FOLLOWER
+            self.logger.info(
+                "Sync: leader active at %s — switching to follower mode",
+                sender_ip,
+            )
+            self.write_status_file()
+
     def _follower_recv_loop(self) -> None:
         while self._running:
             try:
                 data, addr = self._recv_sock.recvfrom(65535)
                 sender_ip = addr[0]
 
-                if data[:8] == _RAW_MAGIC or len(data) > 512:
-                    # Frame data: prefer magic-tagged raw RGB; fall back to legacy PNG
+                if data[:8] == _RAW_MAGIC:
+                    # Magic-tagged raw RGB frame — self-describing, no guessing.
                     try:
-                        if data[:8] == _RAW_MAGIC:
-                            w, h = _RAW_HEADER.unpack(data[8:12])
-                            raw = data[12:]
-                            img = Image.frombuffer(
-                                "RGB", (w, h), raw, "raw", "RGB", 0, 1
-                            )
-                        else:
-                            # Fallback: try legacy PNG
-                            img = Image.open(io.BytesIO(data))
-                            img.load()
-                        with self._frame_lock:
-                            self._latest_frame = img
-                        self._last_leader_frame_time = time.time()
-                        self._leader_ip = sender_ip
-
-                        if self._follower_state == FollowerState.STANDALONE:
-                            self._follower_state = FollowerState.FOLLOWER
-                            self.logger.info(
-                                "Sync: leader active at %s — switching to follower mode",
-                                sender_ip,
-                            )
-                            self.write_status_file()
+                        w, h = _RAW_HEADER.unpack(data[8:12])
+                        raw = data[12:]
+                        img = Image.frombuffer(
+                            "RGB", (w, h), raw, "raw", "RGB", 0, 1
+                        )
+                        self._handle_received_frame(img, sender_ip)
                     except Exception as exc:
                         self.logger.debug("Sync: frame decode error: %s", exc)
                 else:
-                    # Control message
+                    # No magic prefix. Whether the payload parses as JSON
+                    # decides between a control message and a legacy
+                    # (pre-magic) PNG frame — both wire formats are
+                    # self-describing, so no size heuristic is needed. A
+                    # >512-byte control message used to be misrouted into
+                    # image decode and silently dropped.
                     try:
                         msg = json.loads(data.decode("utf-8"))
+                    except (json.JSONDecodeError, UnicodeDecodeError):
+                        # Not JSON — try a legacy PNG frame.
+                        try:
+                            img = Image.open(io.BytesIO(data))
+                            if img.width > _MAX_FRAME_W or img.height > _MAX_FRAME_H:
+                                # Same cap the TCP image path applies: decode
+                                # is deferred until load(), so check first.
+                                self.logger.debug(
+                                    "Sync: rejected oversized legacy frame %dx%d from %s",
+                                    img.width, img.height, sender_ip,
+                                )
+                                continue
+                            img.load()
+                            self._handle_received_frame(img, sender_ip)
+                        except Exception as exc:
+                            self.logger.debug("Sync: frame decode error: %s", exc)
+                        continue
+
+                    # It parsed, so it is a control message and never a
+                    # frame. Read and validate its fields under a guard —
+                    # a UDP payload is attacker-shaped, so a non-object
+                    # body makes .get() raise AttributeError and an "sx"
+                    # carrying a non-numeric x raises ValueError/TypeError
+                    # — but dispatch the callback *outside* it. Running
+                    # the callback in here would let a fault in someone
+                    # else's code read as a malformed packet and be
+                    # logged as one.
+                    fire_new_cycle = False
+                    try:
                         t = msg.get("t")
                         if t == "hello_ack":
                             self._leader_ip = sender_ip
@@ -501,7 +546,17 @@ def _follower_recv_loop(self) -> None:
                             self.write_status_file()
                         elif t == "sx":
                             # Vegas scroll-position sync — tiny message, renders locally
-                            self._latest_scroll_x = float(msg["x"])
+                            scroll_x = float(msg["x"])
+                            if not math.isfinite(scroll_x):
+                                # json.loads accepts the NaN/Infinity literals,
+                                # and float("nan") accepts the strings, so a
+                                # non-finite x reaches here intact. Left alone
+                                # it poisons every offset computed from it —
+                                # NaN comparisons are all false, so the
+                                # follower renders a frame it can never scroll
+                                # back from. Treat it as malformed.
+                                raise ValueError(f"non-finite scroll x: {msg['x']!r}")
+                            self._latest_scroll_x = scroll_x
                             self._last_leader_frame_time = time.time()
                             self._leader_ip = sender_ip
                             if self._follower_state == FollowerState.STANDALONE:
@@ -511,19 +566,22 @@ def _follower_recv_loop(self) -> None:
                                     sender_ip,
                                 )
                                 self.write_status_file()
-                                if self._on_new_cycle:
-                                    self._on_new_cycle()  # build initial scroll image
+                                fire_new_cycle = True  # build initial scroll image
                         elif t == "nc":
                             # Leader started a new scroll cycle — rebuild local image
-                            if self._on_new_cycle:
-                                self._on_new_cycle()
-                    except (json.JSONDecodeError, UnicodeDecodeError, KeyError):
-                        pass
+                            fire_new_cycle = True
+                    except (KeyError, AttributeError, TypeError, ValueError) as exc:
+                        self.logger.debug("Sync: malformed control message: %s", exc)
+                        continue
+
+                    if fire_new_cycle and self._on_new_cycle:
+                        self._on_new_cycle()
 
             except socket.timeout:
                 continue
             except Exception as exc:
                 self.logger.debug("Sync follower recv error: %s", exc)
+                time.sleep(0.1)
 
     def _follower_announce_loop(self) -> None:
         hw = self._hw_config

src/web_interface/api_helpers.py

@@ -29,18 +29,16 @@ def success_response(
         Flask jsonify response
     """
     response_data = create_success_response(data, message, metadata)
-
-    # Add request metadata if available
-    if metadata is None:
-        metadata = {}
-
-    # Add timing if request start time is available
+
+    # Timing is merged into whatever the caller passed, without inventing a
+    # metadata block for responses that have neither.
+    enriched = dict(metadata) if metadata is not None else {}
     if hasattr(request, 'start_time'):
-        metadata['response_time_ms'] = int((time.time() - request.start_time) * 1000)
-    
-    if metadata:
-        response_data['metadata'] = metadata
-    
+        enriched['response_time_ms'] = int((time.time() - request.start_time) * 1000)
+
+    if metadata is not None or enriched:
+        response_data['metadata'] = enriched
+
     return jsonify(response_data)
 
 

src/web_interface/error_handler.py

@@ -142,14 +142,17 @@ def create_success_response(
         "status": "success"
     }
 
+    # All three use `is not None` rather than truthiness: "" and {} are
+    # values a caller chose to send, and dropping them silently would make
+    # the response shape depend on the data.
     if data is not None:
         response["data"] = data
-    
-    if message:
+
+    if message is not None:
         response["message"] = message
-    
-    if metadata:
+
+    if metadata is not None:
         response["metadata"] = metadata
-    
+
     return response
 

src/web_interface/errors.py

@@ -89,7 +89,11 @@ def __init__(
         self.category = category or self._infer_category(error_code)
         self.details = details
         self.context = context or {}
-        self.suggested_fixes = suggested_fixes or self._get_default_suggestions(error_code)
+        # `is None`, not truthiness: an explicit [] means "this caller has
+        # no suggestions to offer", which the default list would override.
+        self.suggested_fixes = (
+            suggested_fixes if suggested_fixes is not None
+            else self._get_default_suggestions(error_code))
         self.original_error = original_error
 
     def _infer_category(self, error_code: ErrorCode) -> ErrorCategory: