diff --git a/scripts/install/configure_wifi_permissions.sh b/scripts/install/configure_wifi_permissions.sh index cfba4bc4..37e2c7db 100755 --- a/scripts/install/configure_wifi_permissions.sh +++ b/scripts/install/configure_wifi_permissions.sh @@ -25,9 +25,44 @@ if [ "$EUID" -eq 0 ]; then exit 1 fi +# Resolve command paths against a fixed PATH, and check what we resolved. +# +# Every path found here is written into a sudoers file as a NOPASSWD grant, so +# whoever controls the binary at that path controls root. first_time_install.sh +# re-execs itself with `sudo -E`, which preserves the invoking user's +# environment -- PATH included -- so without pinning it, `which nmcli` can +# resolve to anything on that PATH: a writable directory early in it turns a +# compromise of the low-privilege web user into permanent root. +PATH=/usr/sbin:/usr/bin:/sbin:/bin +export PATH + +# A binary named in a sudoers rule must be root-owned and writable by nobody +# else, or the grant hands root to whoever can rewrite it. +require_trusted_binary() { + local label="$1" path="$2" + if [ ! -x "$path" ]; then + echo "✗ $label: $path is not an executable file" + exit 1 + fi + local owner perms + owner=$(stat -c '%u' "$path") || exit 1 + perms=$(stat -c '%a' "$path") || exit 1 + if [ "$owner" != "0" ]; then + echo "✗ $label: $path is not owned by root (uid $owner); refusing to" + echo " grant it NOPASSWD sudo." + exit 1 + fi + # Group- or world-writable means someone other than root can replace it. + case "$perms" in + *[2367]) echo "✗ $label: $path is writable by group or other ($perms);" + echo " refusing to grant it NOPASSWD sudo." + exit 1 ;; + esac +} + # Get the full paths to commands -NMCLI_PATH=$(which nmcli || echo "/usr/bin/nmcli") -SYSTEMCTL_PATH=$(which systemctl) +NMCLI_PATH=$(command -v nmcli || echo "/usr/bin/nmcli") +SYSTEMCTL_PATH=$(command -v systemctl) echo "Command paths:" echo " nmcli: $NMCLI_PATH" @@ -37,6 +72,18 @@ 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) + +# Checked before any of them reaches the sudoers file. +require_trusted_binary "nmcli" "$NMCLI_PATH" +require_trusted_binary "systemctl" "$SYSTEMCTL_PATH" +require_trusted_binary "sysctl" "$SYSCTL_PATH" +require_trusted_binary "nft" "$NFT_PATH" +require_trusted_binary "rfkill" "$RFKILL_PATH" +require_trusted_binary "mkdir" "$MKDIR_PATH" # Create a temporary sudoers file using mktemp (handles permissions better) TEMP_SUDOERS=$(mktemp) || { @@ -62,6 +109,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 diff --git a/src/startup_validator.py b/src/startup_validator.py index 9f2e0a51..79275dc2 100644 --- a/src/startup_validator.py +++ b/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,80 @@ 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, in order: no comments, no blanks. + + Order is preserved deliberately. This used to sort, which made the + comparison insensitive to two changes that matter in a systemd unit: + repeated directives such as ExecStartPre= and ExecStartPost= run in + the order they appear, and a directive that moves between [Unit], + [Service] and [Install] means something different -- or nothing -- + where it lands. A drift check that normalises those away reports no + drift for a unit that has genuinely changed. + """ + lines = [] + for line in text.splitlines(): + line = line.strip() + if line and not line.startswith("#"): + lines.append(line) + return "\n".join(lines) + def _validate_config(self) -> None: """Validate configuration files.""" try: diff --git a/systemd/ledmatrix.service b/systemd/ledmatrix.service index d5f064b6..ab5ddf4d 100644 --- a/systemd/ledmatrix.service +++ b/systemd/ledmatrix.service @@ -8,6 +8,18 @@ Type=simple User=root WorkingDirectory=__PROJECT_ROOT_DIR__ Environment=PYTHONDONTWRITEBYTECODE=1 +# glibc gives each allocating thread its own malloc arena, up to 8 x CPU count, +# and an arena that has grown is never handed back to the OS. This process runs +# 9 threads on a 3-core Pi, so the ceiling is 24 arenas -- and a rig measured at +# 1030 MB resident held 23 large anonymous mappings on 64 MB-aligned addresses, +# 920 MB of them, while the live data it was actually holding (widest scroll +# strip seen: 35,746 x 64) accounts for roughly 15 MB. That gap is arena bloat, +# not leaked objects: RSS was flat across repeated sampling, not climbing. +# +# Capping the arenas trades a little allocator concurrency for a large amount of +# resident memory on a device that has neither to spare. 2 is the usual value; +# raise it if frame times regress. +Environment=MALLOC_ARENA_MAX=2 ExecStart=/usr/bin/python3 __PROJECT_ROOT_DIR__/run.py # Restart=always, not on-failure: run.py exiting 0 (a clean shutdown path taken # for a reason that no longer applies, e.g. a config reload) would otherwise leave diff --git a/test/test_sudo_allowlist_covers_calls.py b/test/test_sudo_allowlist_covers_calls.py new file mode 100644 index 00000000..bec2486b --- /dev/null +++ b/test/test_sudo_allowlist_covers_calls.py @@ -0,0 +1,147 @@ +"""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 _normalise(rule): + """One rule with binary-path variables reduced to bare tool names. + + Rules are written as `$SYSCTL_PATH -w ...` or `${NFT_PATH} ...`, 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. Both spellings are handled: shell expands them + identically, and a check that understood only one silently skipped the + other. + """ + rule = re.sub(r"\$\{?([A-Z][A-Z0-9_]*)_PATH\}?", + lambda m: m.group(1).lower(), rule) + return re.sub(r"/usr/(?:s?bin)/", "", rule) + + +def _granted_commands(): + """The command each NOPASSWD rule actually grants, normalised. + + _grant_lines() already returns everything after "NOPASSWD:", so what + arrives here is the command, possibly preceded by the NOEXEC tag and + possibly still carrying the closing quote of an `echo "..."` that wrote + it. Both are stripped so the result is comparable to a plain command. + """ + commands = [] + for rule in _grant_lines(): + command = _normalise(rule).strip() + command = re.sub(r"^NOEXEC:\s*", "", command) + command = command.rstrip('"').rstrip("'").strip() + if command: + commands.append(" ".join(command.split())) + return commands + + +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. + """ + wanted = " ".join(command) + granted = _granted_commands() + # Exact match, not a prefix. A substring search was satisfied by + # `sysctl -w net.ipv4.ip_forward=0 *`, and that trailing wildcard lets the + # caller append whatever they like to a command running as root -- a far + # wider grant than the one this test is meant to be confirming. + assert wanted in granted, ( + f"no installer grants exactly `{wanted}`; closest matches: " + + str([g for g in granted if g.startswith(command[0])])[:200]) + + +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 + # Normalised the same way as everything else: `${NFT_PATH} *` left a + # brace before the tool name, and the word-boundary check below does + # not treat "{" as a boundary, so that spelling slipped through. + haystack = _normalise(rule).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)) diff --git a/test/test_systemd_malloc_arenas.py b/test/test_systemd_malloc_arenas.py new file mode 100644 index 00000000..bb4bc8f0 --- /dev/null +++ b/test/test_systemd_malloc_arenas.py @@ -0,0 +1,95 @@ +"""The display unit must cap glibc's malloc arenas. + +glibc hands each allocating thread its own malloc arena, up to 8 x CPU count, +and an arena that has grown is never returned to the OS. This process runs +threads for the render loop, the update workers and the background fetchers, so +on a 3-core Pi the ceiling is 24 arenas. + +Measured on a live rig, 2.5 hours in: + + RSS 1030 MB + Private_Dirty 988 MB + anonymous mappings > 10 MB 23 (ceiling is 8 x 3 = 24) + largest few 104, 79, 66, 63, 63 MB, on 64 MB-aligned addresses + +against live data that accounts for perhaps 15 MB -- the widest scroll strip +observed was 35,746 x 64, about 7 MB as RGB and the same again for its numpy +mirror. Repeated sampling showed RSS flat between 990 and 1030 MB rather than +climbing, so this is arena bloat rather than a leak: memory Python has freed +but glibc is holding per-arena. + +The device had 59 MB free at the time. + +Capping the arena count trades a little allocator concurrency for that resident +memory. The render loop is latency-sensitive, so if p99 frame time regresses the +right response is to raise this rather than remove it. +""" +import re +from pathlib import Path + +import pytest + +UNIT = (Path(__file__).resolve().parent.parent / "systemd" / "ledmatrix.service") + +#: The value the unit is expected to carry. 2 is the usual choice for a +#: threaded Python process; 1-4 all keep some of the saving, but only one of +#: them is what this project ships. +EXPECTED_ARENA_MAX = 2 + + +def _environment(unit_text): + return dict( + line.split("=", 2)[1:3] if line.count("=") >= 2 else (line.split("=", 1)[1], "") + for line in unit_text.splitlines() + if line.startswith("Environment=") + ) + + +def test_the_unit_exists(): + assert UNIT.is_file(), f"{UNIT} is missing" + + +def test_malloc_arena_max_is_capped(): + env = _environment(UNIT.read_text(encoding="utf-8")) + assert "MALLOC_ARENA_MAX" in env, ( + "the display unit does not cap glibc arenas; on a 3-core Pi the default " + "ceiling is 24 and a measured rig held 23 of them, 920 MB" + ) + value = int(env["MALLOC_ARENA_MAX"]) + # Pinned, not a range. A range let a change to 4 -- which hands most of the + # saving back -- pass unnoticed, which was the point of the finding that + # prompted this. Raising it is a legitimate response to a frame-time + # regression, but it should be a visible edit here rather than a silent + # drift, so the number lives in one place and changing it shows up in + # review. + assert value == EXPECTED_ARENA_MAX, ( + f"MALLOC_ARENA_MAX={value}, expected {EXPECTED_ARENA_MAX}. If this was " + "raised deliberately because frame times regressed, update " + "EXPECTED_ARENA_MAX here and say so in the commit." + ) + + +def test_the_reason_is_recorded_next_to_it(): + """A bare tuning knob invites removal by whoever meets it next.""" + text = UNIT.read_text(encoding="utf-8") + index = text.index("Environment=MALLOC_ARENA_MAX") + preamble = text[:index].splitlines()[-12:] + comment = "\n".join(line for line in preamble if line.startswith("#")) + assert "arena" in comment.lower(), "no explanation precedes the setting" + assert re.search(r"\d", comment), ( + "the explanation cites no measurement, so a reader cannot tell whether " + "it still applies to their hardware" + ) + + +@pytest.mark.parametrize("unit", ["ledmatrix.service"]) +def test_the_unit_still_parses_as_ini(unit): + """systemd will refuse a malformed unit, and the panel stays dark.""" + import configparser + + path = UNIT.parent / unit + parser = configparser.ConfigParser(strict=False) + # systemd allows repeated keys; ConfigParser needs them merged, not rejected. + parser.read_string(path.read_text(encoding="utf-8")) + assert parser.has_section("Service") + assert parser.has_option("Service", "ExecStart") diff --git a/test/test_systemd_unit_drift.py b/test/test_systemd_unit_drift.py new file mode 100644 index 00000000..21cf56e0 --- /dev/null +++ b/test/test_systemd_unit_drift.py @@ -0,0 +1,152 @@ +"""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_drift(): + """Order is not noise in a systemd unit. + + Repeated directives -- ExecStartPre=, ExecStartPost= -- run in the order + they appear, and a directive that moves between [Unit], [Service] and + [Install] means something different, or nothing, where it lands. This + check used to sort the lines before comparing, which reported no drift for + a unit that had genuinely changed. + """ + a = "[Service]\nExecStartPre=/first\nExecStartPre=/second\n" + b = "[Service]\nExecStartPre=/second\nExecStartPre=/first\n" + assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b), ( + "swapping two ExecStartPre= lines changes what runs first, and was " + "being normalised away") + + +def test_a_directive_moved_between_sections_is_drift(): + a = "[Unit]\nDescription=x\n[Service]\nExecStart=/x\n" + b = "[Unit]\nDescription=x\nExecStart=/x\n[Service]\n" + assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b), ( + "ExecStart= in [Unit] is not the same unit, and sorting hid it") + + +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")) + # Cosmetic means comments, blank lines and stray indentation -- the things + # the installer really does drop. Not reordering: that changes the unit, + # and is asserted as drift above. + directives = [line.strip() for line in substituted.splitlines() + if line.strip() and not line.strip().startswith("#")] + installed = tmp_path / "ledmatrix.service" + installed.write_text( + "\n\n".join(" " + d for d in 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