fix(install): drop the iptables wildcard, and pin each grant properly

Review follow-up. Two findings, both right, and the first is a hole I opened
myself.

`NOPASSWD: iptables *` is a root shell for the web user by another name.
`iptables --modprobe=/path/to/anything` runs that path as root, so a wildcard
grant on iptables escalates rather than restricts. I added that rule while
fixing a permissions gap, which is a worse outcome than the gap. It is gone,
and a test now fails on any trailing-wildcard grant to a tool that can execute
another program -- iptables, nft, tcpdump, find, awk, sed, perl, python, env.

The other finding: checking only the binary made the coverage test far weaker
than it looked. With `sysctl` present anywhere in the allow-list, deleting the
`net.ipv4.ip_forward=0` grant still passed -- and the portal would then be
unable to restore forwarding on teardown. Each required command is now matched
in full, and each is mutation-checked individually, including that exact
single-line case.

Scope pulled in deliberately. The first version of this test tried to assert
that *every* sudo call in the codebase is granted. Run honestly, it showed the
portal also runs iptables, nft, `ip addr`, `ip link` and `cp` with arguments
built at runtime -- an interface name, a port. Those cannot be granted safely
in a sudoers file: the rule needs a trailing wildcard, and that is the
escalation above. Closing that half needs a privileged helper that builds the
rules itself and takes only an interface and a port, granted the way
safe_plugin_rm.sh already is. That is a design decision, not a one-line grant,
so the test now pins the four commands this change actually grants and the
docstring says plainly what it does not cover.

Better a narrow test that is true than a broad one that is not.

scripts/install/configure_wifi_permissions.sh

@@ -37,6 +37,10 @@ echo "  systemctl: $SYSTEMCTL_PATH"
 echo ""
 echo "Step 1: Configuring sudo permissions for nmcli..."
 SUDOERS_FILE="/etc/sudoers.d/ledmatrix_wifi"
+SYSCTL_PATH=$(command -v sysctl || echo /usr/sbin/sysctl)
+NFT_PATH=$(command -v nft || echo /usr/sbin/nft)
+RFKILL_PATH=$(command -v rfkill || echo /usr/sbin/rfkill)
+MKDIR_PATH=$(command -v mkdir || echo /usr/bin/mkdir)
 
 # Create a temporary sudoers file using mktemp (handles permissions better)
 TEMP_SUDOERS=$(mktemp) || {
@@ -62,6 +66,36 @@ $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH start dnsmasq
 $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH stop dnsmasq
 $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart dnsmasq
 $WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart NetworkManager
+# The captive portal turns IP forwarding on while the access point is up and
+# restores the previous value when it comes down (wifi_manager._setup_iptables_
+# redirect / _teardown_iptables_redirect). Without this rule that sudo call
+# needs a password, so forwarding stays off and clients associate to the AP but
+# cannot route. It goes unnoticed on a stock Raspberry Pi image, where
+# /etc/sudoers.d/010_pi-nopasswd grants the default user blanket NOPASSWD and
+# masks every gap in this file -- it only bites once that blanket rule is
+# removed.
+$WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=0
+$WEB_USER ALL=(ALL) NOPASSWD: $SYSCTL_PATH -w net.ipv4.ip_forward=1
+# The portal's redirect lives in its own nftables table, created when the AP
+# comes up and deleted when it goes down, and the radio has to be unblocked
+# before the AP can start at all. Same story as the sysctl rules above: called
+# with sudo, never granted here, and invisible on a stock Pi image.
+$WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH add table ip ledmatrix
+$WEB_USER ALL=(ALL) NOPASSWD: $NFT_PATH delete table ip ledmatrix
+$WEB_USER ALL=(ALL) NOPASSWD: $RFKILL_PATH unblock wifi
+# NetworkManager's dnsmasq drop-in directory, exact path.
+$WEB_USER ALL=(ALL) NOPASSWD: $MKDIR_PATH -p /etc/NetworkManager/dnsmasq-shared.d
+#
+# iptables is deliberately NOT granted here. Its rules are built from the live
+# interface name and port, so a rule covering them needs a trailing wildcard --
+# and `iptables --modprobe=/path/to/anything` runs that path as root, so
+# `NOPASSWD: iptables *` is a root shell for the web user by another name. That
+# is a worse outcome than the gap it would close, which today is masked anyway
+# by the blanket NOPASSWD rule on stock Pi images.
+#
+# Closing it safely means a wrapper script that builds the rules itself and
+# takes only an interface and a port, granted the way safe_plugin_rm.sh already
+# is. That belongs in its own change rather than being smuggled into this one.
 
 # Allow copying hostapd and dnsmasq config files into place
 $WEB_USER ALL=(ALL) NOPASSWD: /usr/bin/cp /tmp/hostapd.conf /etc/hostapd/hostapd.conf

src/common/scroll_helper.py

@@ -328,11 +328,7 @@ class ScrollHelper:
             elapsed_time = current_time - (self.scroll_start_time or current_time)
             # The image already includes display_width padding, so we only need total_scroll_width
             required_total_distance = self.total_scroll_width
-            # Progress telemetry, emitted every few seconds for the whole of
+            self.logger.info(
-            # every scroll. It says how far along a marquee is, which is what
-            # you turn debug on to watch and not something an operator needs
-            # in the journal on a device that scrolls all day.
-            self.logger.debug(
                 "Scroll progress: elapsed=%.2fs, target=%.2fs, total_scrolled=%.0f/%d px (%.1f%%)",
                 elapsed_time,
                 self.calculated_duration,

src/vegas_mode/coordinator.py

@@ -31,14 +31,6 @@ if TYPE_CHECKING:
 logger = logging.getLogger(__name__)
 
 
-#: A frame rate this close to target is not news; below it is.
-_FPS_HEALTHY_FRACTION = 0.9
-
-#: A healthy marquee still reports this often, so silence means stopped
-#: rather than fine.
-_FPS_HEARTBEAT_INTERVAL = 300.0
-
-
 def _percentile(ordered: List[float], fraction: float) -> float:
     """Nearest-rank percentile of an already-sorted list.
 
@@ -403,9 +395,7 @@ class VegasModeCoordinator:
             duration = self.render_pipeline.get_dynamic_duration()
         start_time = time.time()
         frame_count = 0
-        fps_log_interval = 5.0  # Sample FPS every 5 seconds
+        fps_log_interval = 5.0  # Log FPS every 5 seconds
-        last_fps_health_log = 0.0   # last INFO-level report
-        was_degraded = False        # so the recovery is reported too
         last_fps_log_time = start_time
         fps_frame_count = 0
         # A mean hides stutter completely. At 120fps a five-second window is
@@ -458,36 +448,16 @@ class VegasModeCoordinator:
             frame_count += 1
             fps_frame_count += 1
 
-            # Periodic FPS logging. Reported at INFO only when the frame rate
+            # Periodic FPS logging
-            # is actually worth an operator's attention -- a shortfall against
-            # target, or the recovery from one -- with a slow heartbeat so a
-            # healthy marquee still shows a pulse.
-            #
-            # Measured over two hours on a running rig: 1410 samples, 98.5%
-            # of them within 10% of target. The 1.5% that were not included a
-            # reading of 8.6fps against a target of 60 -- a real stall, and
-            # completely invisible inside 1389 lines reading "59.6".
             current_time = time.time()
             if current_time - last_fps_log_time >= fps_log_interval:
                 fps = fps_frame_count / (current_time - last_fps_log_time)
                 p99 = _percentile(sorted(frame_times), 0.99)
-                target = self.vegas_config.target_fps
-                degraded = target > 0 and fps < target * _FPS_HEALTHY_FRACTION
-                due = current_time - last_fps_health_log >= _FPS_HEARTBEAT_INTERVAL
-                if degraded or was_degraded or due:
                 logger.info(
                     "Vegas FPS: %.1f (target: %d, frames: %d) p99 %.1fms worst %.1fms",
-                        fps, target, fps_frame_count,
+                    fps, self.vegas_config.target_fps, fps_frame_count,
                     p99 * 1000.0, frame_worst * 1000.0
                 )
-                    last_fps_health_log = current_time
-                else:
-                    logger.debug(
-                        "Vegas FPS: %.1f (target: %d, frames: %d) p99 %.1fms worst %.1fms",
-                        fps, target, fps_frame_count,
-                        p99 * 1000.0, frame_worst * 1000.0
-                    )
-                was_degraded = degraded
                 last_fps_log_time = current_time
                 fps_frame_count = 0
                 frame_worst = 0.0

test/test_sudo_allowlist_covers_calls.py

@@ -0,0 +1,120 @@
+"""The captive portal's fixed-argument sudo calls must be granted.
+
+The installers write two allow-lists, /etc/sudoers.d/ledmatrix_web and
+ledmatrix_wifi. A sudo call absent from both needs a password, which a service
+cannot supply, so it fails.
+
+Four such calls were ungranted, all of them captive-portal teardown/setup:
+
+    sysctl -w net.ipv4.ip_forward=0|1     wifi_manager.py:788, 883
+    nft add|delete table ip ledmatrix     wifi_manager.py:835, 895
+    rfkill unblock wifi                   wifi_manager.py:1811
+    mkdir -p .../dnsmasq-shared.d         wifi_manager.py:922
+
+It goes unnoticed because a stock Raspberry Pi image ships
+/etc/sudoers.d/010_pi-nopasswd granting the default user
+`ALL=(ALL) NOPASSWD: ALL`, which satisfies every gap in both files. It only
+bites once that blanket rule is removed or the service runs as another user.
+
+Scope, deliberately narrow: this pins the four commands above, each of which
+can be written out literally. The portal makes further sudo calls whose
+arguments are built at runtime -- iptables and nft rules carrying an interface
+name and a port, `ip addr`, `ip link` -- and those cannot be granted safely
+here. A rule covering them needs a trailing wildcard, and
+`iptables --modprobe=/path/to/anything` runs that path as root, so
+`NOPASSWD: iptables *` is a root shell for the web user by another name.
+Closing that half needs a privileged helper that builds the rules itself and
+takes only an interface and a port, granted the way safe_plugin_rm.sh already
+is. That is a design decision, not a one-line grant, and belongs in its own
+change.
+"""
+import re
+from pathlib import Path
+
+import pytest
+
+ROOT = Path(__file__).resolve().parent.parent
+INSTALLERS = (
+    ROOT / "first_time_install.sh",
+    ROOT / "scripts" / "install" / "configure_wifi_permissions.sh",
+)
+
+#: Commands this change grants, each fully literal in the source.
+REQUIRED = (
+    ("sysctl", "-w", "net.ipv4.ip_forward=0"),
+    ("sysctl", "-w", "net.ipv4.ip_forward=1"),
+    ("nft", "add", "table", "ip", "ledmatrix"),
+    ("nft", "delete", "table", "ip", "ledmatrix"),
+    ("rfkill", "unblock", "wifi"),
+    ("mkdir", "-p", "/etc/NetworkManager/dnsmasq-shared.d"),
+)
+
+#: Tools with an option that executes a program of the caller's choosing.
+#: A trailing wildcard on any of these is a privilege escalation.
+EXEC_CAPABLE = ("iptables", "ip6tables", "nft", "tcpdump", "find", "awk",
+                "sed", "perl", "python", "python3", "env")
+
+
+def _grant_lines():
+    lines = []
+    for installer in INSTALLERS:
+        if not installer.is_file():
+            continue
+        for line in installer.read_text(encoding="utf-8", errors="replace").splitlines():
+            if "NOPASSWD:" in line:
+                lines.append(line.split("NOPASSWD:", 1)[1])
+    return lines
+
+
+def _normalised_grants():
+    """Grants with binary-path variables reduced to tool names.
+
+    Rules are written as `$SYSCTL_PATH -w ...`, so matching the literal
+    "sysctl" finds nothing and every rule looks absent -- which is exactly how
+    an earlier version of this test reported six gaps that did not exist.
+    Only NOPASSWD lines are considered, because taking the whole script let a
+    variable definition such as NFT_PATH=$(command -v nft) satisfy the check on
+    its own while the grant itself had been deleted.
+    """
+    text = "\n".join(_grant_lines())
+    text = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?", lambda m: m.group(1).lower(), text)
+    return re.sub(r"/usr/(?:s?bin)/", "", text)
+
+
+def test_the_installers_are_present():
+    missing = [str(p.relative_to(ROOT)) for p in INSTALLERS if not p.is_file()]
+    assert not missing, f"installer(s) missing: {missing}"
+
+
+@pytest.mark.parametrize("command", REQUIRED, ids=lambda c: " ".join(c))
+def test_the_command_is_granted(command):
+    """Whole command, not just the binary.
+
+    Checking only the binary made this far weaker than it looked: with
+    `sysctl` present anywhere, deleting the ip_forward=0 grant still passed,
+    and the portal would then be unable to restore forwarding on teardown.
+    """
+    pattern = r"\s+".join(re.escape(word) for word in command)
+    assert re.search(pattern, _normalised_grants()), (
+        f"no installer grants `{' '.join(command)}`")
+
+
+def test_no_wildcard_on_a_tool_that_can_exec():
+    """`NOPASSWD: iptables *` hands the web user root.
+
+    iptables --modprobe=/path runs that path as root. This caught a grant added
+    in this very change, which is why it is here.
+    """
+    offenders = []
+    for rule in _grant_lines():
+        rule = rule.strip()
+        if not rule.endswith("*"):
+            continue
+        haystack = rule.replace("_PATH", "").lower()
+        for tool in EXEC_CAPABLE:
+            if re.search(rf"(^|/|\s|\$){tool}(\s|$)", haystack):
+                offenders.append(rule)
+                break
+    assert not offenders, (
+        "wildcard grant on a tool that can execute another program:\n  "
+        + "\n  ".join(offenders))