fix(startup): warn when an installed systemd unit has drifted from the repo's

Nothing re-applies systemd units after the first install. `git pull` -- which
is what the web UI's update button runs -- brings a new template into the
checkout, but no code in web_interface/ or src/ copies it to
/etc/systemd/system, and nothing anywhere runs `systemctl daemon-reload`. The
unit that actually runs is whatever first_time_install.sh wrote on day one.

So every hardening added to a unit is inert on existing installs, silently.
Measured on a live rig:

    installed  /etc/systemd/system/ledmatrix.service   2026-08-06
    template   systemd/ledmatrix.service               2026-08-19
    contents                                           differ

with the practical result that the MemoryMax=85% the repo's template specifies
was not being enforced at all -- `systemctl show` reported
MemoryMax=infinity. Anyone reading the template would reasonably believe the
service was capped.

Startup now compares each installed unit against its substituted template and
warns when they differ, naming install_service.sh as the remedy.

A warning, not an error, and deliberately not a silent rewrite: editing files
under /etc and restarting services is the installer's job, not something a
display process should do to a machine while it is booting. Making it fatal
would also brick every development checkout whose unit is legitimately absent
or hand-edited.

Comparison ignores comments, blank lines and ordering. The template carries
explanatory comments the installed copy will not have, and systemd does not
care about order within a section, so a literal comparison would warn on every
boot and be ignored within a week.

Mutation-checked three ways: never reporting drift fails, making it fatal
fails, and -- after the first attempt missed it -- comparing raw text now fails
too. That last gap is worth noting: the comment-insensitivity tests originally
exercised the helper directly, so a comparison that stopped calling the helper
passed them all. The test that catches it goes through _validate_systemd_units.

29 startup-validator tests pass.

scripts/install/configure_wifi_permissions.sh

@@ -37,10 +37,6 @@ 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) || {
@@ -66,36 +62,6 @@ $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/startup_validator.py

@@ -62,6 +62,9 @@ class StartupValidator:
         # Validate plugins if plugin manager is available
         if self.plugin_manager:
             self._validate_plugins()
+
+        # Warn when the running systemd unit no longer matches the repo's
+        self._validate_systemd_units()
         
         is_valid = len(self.errors) == 0
         
@@ -74,6 +77,71 @@ class StartupValidator:
         
         return (is_valid, self.errors.copy(), self.warnings.copy())
     
+    #: Units this project installs, and where each is installed to.
+    _UNITS = (
+        ("systemd/ledmatrix.service", "/etc/systemd/system/ledmatrix.service"),
+        ("systemd/ledmatrix-web.service", "/etc/systemd/system/ledmatrix-web.service"),
+    )
+
+    def _validate_systemd_units(self) -> None:
+        """Warn when an installed unit has drifted from the repo's template.
+
+        Nothing re-applies these after the first install. `git pull` -- which is
+        what the web UI's update button runs -- brings a new template into the
+        checkout, but nothing copies it to /etc/systemd/system and nothing runs
+        `systemctl daemon-reload`, so the unit that actually runs is whatever
+        first_time_install.sh wrote on day one.
+
+        That makes every hardening added to a unit inert on existing installs.
+        Measured on one rig: the installed unit was thirteen days older than the
+        repo's and differed in content, so a MemoryMax the repo had specified
+        was not being enforced at all -- `systemctl show` reported
+        MemoryMax=infinity.
+
+        A warning rather than an error, and certainly not a silent rewrite:
+        editing files under /etc and restarting services is the installer's job,
+        not something a display process should do to a machine while it boots.
+        The remedy is to re-run scripts/install/install_service.sh.
+        """
+        try:
+            project_root = Path(__file__).resolve().parent.parent
+            for template_rel, installed_path in self._UNITS:
+                template = project_root / template_rel
+                installed = Path(installed_path)
+                if not template.is_file() or not installed.is_file():
+                    continue
+
+                # The template carries placeholders the installer substitutes,
+                # so compare the substituted form rather than the raw file.
+                expected = template.read_text(encoding="utf-8")
+                expected = expected.replace("__PROJECT_ROOT_DIR__", str(project_root))
+                expected = expected.replace("__USER__", "root")
+
+                try:
+                    actual = installed.read_text(encoding="utf-8")
+                except PermissionError:
+                    continue
+
+                if self._unit_body(expected) != self._unit_body(actual):
+                    self.warnings.append(
+                        f"{installed.name} differs from {template_rel}; the "
+                        "installed unit is not refreshed by an update, so "
+                        "settings added to the template are not in effect. "
+                        "Re-run scripts/install/install_service.sh to apply them."
+                    )
+        except OSError as e:
+            self.logger.debug("Could not compare systemd units: %s", e)
+
+    @staticmethod
+    def _unit_body(text: str) -> str:
+        """A unit's meaningful lines: no comments, no blanks, no ordering noise."""
+        lines = []
+        for line in text.splitlines():
+            line = line.strip()
+            if line and not line.startswith("#"):
+                lines.append(line)
+        return "\n".join(sorted(lines))
+
     def _validate_config(self) -> None:
         """Validate configuration files."""
         try:

test/test_sudo_allowlist_covers_calls.py

@@ -1,120 +0,0 @@
-"""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))

test/test_systemd_unit_drift.py

@@ -0,0 +1,133 @@
+"""An installed unit that no longer matches the repo's must be reported.
+
+Nothing re-applies systemd units after the first install. `git pull` -- what
+the web UI's update button runs -- brings a new template into the checkout, but
+no code in web_interface/ or src/ copies it to /etc/systemd/system or runs
+`systemctl daemon-reload`. The unit that actually runs is whatever
+first_time_install.sh wrote on day one.
+
+So every hardening added to a unit is inert on existing installs. Measured on a
+live rig: the installed unit was dated 2026-08-06 and the repo's 2026-08-19,
+and they differed -- with the result that a MemoryMax=85% present in the repo's
+template was not being enforced at all. `systemctl show` reported
+MemoryMax=infinity.
+
+This is a warning, not an error, and deliberately not a silent rewrite:
+editing files under /etc and restarting services is the installer's job, not
+something a display process should do to a machine while it boots.
+"""
+import logging
+from pathlib import Path
+from unittest.mock import MagicMock
+
+import pytest
+
+from src.startup_validator import StartupValidator
+
+
+@pytest.fixture
+def validator():
+    v = StartupValidator(config_manager=MagicMock())
+    v.logger = logging.getLogger("test")
+    v.warnings = []
+    v.errors = []
+    return v
+
+
+def test_a_matching_unit_produces_no_warning(validator, tmp_path):
+    """The installed unit, substituted exactly as the installer would."""
+    project_root = Path("src/startup_validator.py").resolve().parent.parent
+    template_rel = "systemd/ledmatrix.service"
+    template = project_root / template_rel
+    if not template.is_file():
+        pytest.skip("repo unit template not present")
+
+    installed = tmp_path / "ledmatrix.service"
+    installed.write_text(
+        template.read_text(encoding="utf-8")
+        .replace("__PROJECT_ROOT_DIR__", str(project_root))
+        .replace("__USER__", "root"),
+        encoding="utf-8")
+
+    validator._UNITS = ((template_rel, str(installed)),)
+    validator._validate_systemd_units()
+    assert not validator.warnings, f"a matching unit warned: {validator.warnings}"
+    assert not validator.errors
+
+
+def test_comments_and_blank_lines_are_not_drift():
+    """Otherwise every comment the repo adds would look like a changed unit."""
+    a = "[Service]\n# explains a setting\nExecStart=/x\nRestart=always\n"
+    b = "[Service]\nExecStart=/x\n\nRestart=always\n"
+    assert StartupValidator._unit_body(a) == StartupValidator._unit_body(b)
+
+
+def test_a_changed_directive_is_drift():
+    a = "[Service]\nExecStart=/x\nMemoryMax=85%\n"
+    b = "[Service]\nExecStart=/x\n"
+    assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b)
+
+
+def test_reordered_directives_are_not_drift():
+    """systemd does not care about order within a section, so neither should this."""
+    a = "[Service]\nExecStart=/x\nRestart=always\n"
+    b = "[Service]\nRestart=always\nExecStart=/x\n"
+    assert StartupValidator._unit_body(a) == StartupValidator._unit_body(b)
+
+
+def test_cosmetic_differences_do_not_warn(validator, tmp_path):
+    """Through the real comparison, not the helper.
+
+    The repo's template carries explanatory comments the installed copy may not
+    have, and the installer does not preserve ordering or blank lines. If those
+    counted as drift, every boot would warn and the warning would be ignored.
+    Asserting this on _unit_body alone would not catch a comparison that stopped
+    calling it -- which is exactly what a careless edit does.
+    """
+    project_root = Path("src/startup_validator.py").resolve().parent.parent
+    template_rel = "systemd/ledmatrix.service"
+    template = project_root / template_rel
+    if not template.is_file():
+        pytest.skip("repo unit template not present")
+
+    substituted = (template.read_text(encoding="utf-8")
+                   .replace("__PROJECT_ROOT_DIR__", str(project_root))
+                   .replace("__USER__", "root"))
+    # Same directives, stripped of comments and blank lines and reordered.
+    directives = sorted(line.strip() for line in substituted.splitlines()
+                        if line.strip() and not line.strip().startswith("#"))
+    installed = tmp_path / "ledmatrix.service"
+    installed.write_text("\n".join(reversed(directives)) + "\n", encoding="utf-8")
+
+    validator._UNITS = ((template_rel, str(installed)),)
+    validator._validate_systemd_units()
+    assert not validator.warnings, (
+        f"cosmetic-only difference reported as drift: {validator.warnings}")
+
+
+def test_drift_is_reported_as_a_warning(validator, tmp_path):
+    """The whole point: a real difference must surface, and only as a warning."""
+    installed = tmp_path / "ledmatrix.service"
+    installed.write_text("[Service]\nExecStart=/usr/bin/python3 /x/run.py\n")
+
+    project_root = Path("src/startup_validator.py").resolve().parent.parent
+    template_rel = "systemd/ledmatrix.service"
+    template = project_root / template_rel
+    if not template.is_file():
+        pytest.skip("repo unit template not present")
+
+    validator._UNITS = ((template_rel, str(installed)),)
+    validator._validate_systemd_units()
+
+    assert validator.warnings, "a differing unit produced no warning"
+    assert "install_service.sh" in validator.warnings[0], (
+        "the warning does not tell the user how to fix it")
+    assert not validator.errors, "drift must not be fatal at startup"
+
+
+def test_a_missing_installed_unit_is_silent(validator, tmp_path):
+    """Development checkouts have no /etc/systemd unit; that is not drift."""
+    validator._UNITS = (("systemd/ledmatrix.service", str(tmp_path / "absent.service")),)
+    validator._validate_systemd_units()
+    assert not validator.warnings
+    assert not validator.errors