fix(install): parse the sudoers rules before installing them (#602)

Both installers generated the ledmatrix_web rules and copied them straight
into /etc/sudoers.d without ever parsing them. Every rule is built from
`which` lookups, so an empty or surprising path produces a malformed
drop-in -- and a malformed file in /etc/sudoers.d makes sudo refuse every
command for every user. On a headless Pi that is unrecoverable over SSH.

first_time_install.sh now runs `visudo -c` on the generated file and, if it
does not parse, prints what visudo said and leaves the installed file
untouched rather than replacing it with a broken one. configure_web_sudo.sh
does the same before it offers the rules for confirmation.

first_time_install.sh also built the file at a fixed /tmp path as root;
mktemp now picks the name.

test/test_sudoers_is_validated.py renders the installer's own sudoers
heredoc and checks the result with visudo -- the check neither installer
had -- and asserts the install stays gated on it.


Claude-Session: https://claude.ai/code/session_01Dby94z9PV3zVM25fqGNXTt

Co-authored-by: Claude <noreply@anthropic.com>

first_time_install.sh

@@ -1504,6 +1504,9 @@ echo "------------------------------------------------"
 # Create sudoers configuration for the web interface
 echo "Creating sudoers configuration..."
 SUDOERS_FILE="/etc/sudoers.d/ledmatrix_web"
+# A predictable name in a world-writable directory is a symlink target;
+# root writes the rules here, so let mktemp pick the name.
+SUDOERS_TMP=$(mktemp "${TMPDIR:-/tmp}/ledmatrix_web_sudoers.XXXXXX")
 
 # Get command paths
 PYTHON_PATH=$(which python3)
@@ -1514,7 +1517,7 @@ BASH_PATH=$(which bash)
 JOURNALCTL_PATH=$(which journalctl 2>/dev/null || true)
 
 # Create sudoers content
-cat > /tmp/ledmatrix_web_sudoers << EOF
+cat > "$SUDOERS_TMP" << EOF
 # LED Matrix Web Interface passwordless sudo configuration
 # This allows the web interface user to run specific commands without a password
 
@@ -1541,7 +1544,7 @@ $ACTUAL_USER ALL=(ALL) NOPASSWD: $BASH_PATH $PROJECT_ROOT_DIR/scripts/fix_perms/
 $ACTUAL_USER ALL=(ALL) NOPASSWD: $BASH_PATH $PROJECT_ROOT_DIR/scripts/fix_perms/safe_pip_install.sh *
 EOF
 if [ -n "$JOURNALCTL_PATH" ]; then
-    cat >> /tmp/ledmatrix_web_sudoers << EOF
+    cat >> "$SUDOERS_TMP" << EOF
 # NOEXEC, because these rules end in a wildcard and journalctl starts a pager
 # when its output is a terminal. From that pager (less) a "!sh" is a root
 # shell -- the standard journalctl escalation. The web interface always passes
@@ -1555,17 +1558,38 @@ $ACTUAL_USER ALL=(ALL) NOPASSWD:NOEXEC: $JOURNALCTL_PATH -t ledmatrix *
 EOF
 fi
 
-if [ -f "$SUDOERS_FILE" ] && cmp -s /tmp/ledmatrix_web_sudoers "$SUDOERS_FILE"; then
+# Never install rules we have not parsed. A malformed drop-in in
-    echo "Sudoers configuration already up to date"
+# /etc/sudoers.d makes sudo refuse every command for every user, which on a
-    rm /tmp/ledmatrix_web_sudoers
+# headless Pi leaves no way in at all. If the rules do not parse, say so and
+# keep whatever is already installed.
+SUDOERS_VALID=1
+if command -v visudo >/dev/null 2>&1; then
+    if ! visudo -c -f "$SUDOERS_TMP" >/dev/null 2>&1; then
+        SUDOERS_VALID=0
+        echo "⚠ The generated sudoers rules did not parse:" >&2
+        visudo -c -f "$SUDOERS_TMP" >&2 || true
+        echo "⚠ Leaving $SUDOERS_FILE unchanged. The web interface cannot control" >&2
+        echo "  the display service until this is fixed." >&2
+    fi
 else
-    echo "Installing/updating sudoers configuration..."
+    echo "⚠ visudo not found; installing the sudoers rules unvalidated"
-    cp /tmp/ledmatrix_web_sudoers "$SUDOERS_FILE"
-    chmod 440 "$SUDOERS_FILE"
-    rm /tmp/ledmatrix_web_sudoers
 fi
 
-echo "✓ Passwordless sudo access configured"
+if [ "$SUDOERS_VALID" = "0" ]; then
+    rm -f "$SUDOERS_TMP"
+elif [ -f "$SUDOERS_FILE" ] && cmp -s "$SUDOERS_TMP" "$SUDOERS_FILE"; then
+    echo "Sudoers configuration already up to date"
+    rm -f "$SUDOERS_TMP"
+else
+    echo "Installing/updating sudoers configuration..."
+    cp "$SUDOERS_TMP" "$SUDOERS_FILE"
+    chmod 440 "$SUDOERS_FILE"
+    rm -f "$SUDOERS_TMP"
+fi
+
+if [ "$SUDOERS_VALID" = "1" ]; then
+    echo "✓ Passwordless sudo access configured"
+fi
 echo ""
 
 CURRENT_STEP="Configure WiFi management permissions"

scripts/install/configure_web_sudo.sh

@@ -130,6 +130,19 @@ TEMP_SUDOERS="/tmp/ledmatrix_web_sudoers_$$"
     echo "$WEB_USER ALL=(ALL) NOPASSWD: $BASH_PATH $SAFE_PIP_INSTALL_PATH *"
 } > "$TEMP_SUDOERS"
 
+# Never offer to install rules we have not parsed. A malformed drop-in in
+# /etc/sudoers.d makes sudo refuse every command for every user.
+if command -v visudo >/dev/null 2>&1; then
+    if ! visudo -c -f "$TEMP_SUDOERS" >/dev/null 2>&1; then
+        echo ""
+        echo "✗ The generated sudoers rules did not parse:" >&2
+        visudo -c -f "$TEMP_SUDOERS" >&2 || true
+        echo "Nothing was changed." >&2
+        rm -f "$TEMP_SUDOERS"
+        exit 1
+    fi
+fi
+
 echo ""
 echo "Generated sudoers configuration:"
 echo "--------------------------------"

test/test_sudoers_is_validated.py

@@ -0,0 +1,120 @@
+"""The generated sudoers rules must parse before they reach /etc/sudoers.d.
+
+A malformed drop-in there makes sudo refuse every command for every user. On a
+headless Pi that is unrecoverable without pulling the SD card, so both
+installers run `visudo -c` on the file they generated before installing it.
+
+The render test also gives us the check neither installer had: that the rules
+they actually emit are valid sudoers syntax on a real Linux box.
+"""
+
+import os
+import shutil
+import subprocess
+import sys
+import tempfile
+
+import pytest
+
+REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+FIRST_TIME = os.path.join(REPO_ROOT, "first_time_install.sh")
+CONFIGURE = os.path.join(REPO_ROOT, "scripts", "install", "configure_web_sudo.sh")
+
+VISUDO = shutil.which("visudo") or (
+    "/usr/sbin/visudo" if os.path.exists("/usr/sbin/visudo") else None
+)
+
+
+def _read(path):
+    with open(path, "r", encoding="utf-8") as handle:
+        return handle.read()
+
+
+def test_first_time_install_validates_before_installing():
+    body = _read(FIRST_TIME)
+    assert 'visudo -c -f "$SUDOERS_TMP"' in body
+    install = body.index('cp "$SUDOERS_TMP" "$SUDOERS_FILE"')
+    validate = body.index('visudo -c -f "$SUDOERS_TMP"')
+    assert validate < install, "the rules must be checked before they are installed"
+
+
+def test_the_install_is_gated_on_the_check():
+    """Checking and then installing anyway would be worse than not checking."""
+    body = _read(FIRST_TIME)
+    assert "SUDOERS_VALID=0" in body
+    gate = body.index('if [ "$SUDOERS_VALID" = "0" ]')
+    install = body.index('cp "$SUDOERS_TMP" "$SUDOERS_FILE"')
+    assert gate < install
+
+
+def test_first_time_install_does_not_use_a_predictable_temp_file():
+    body = _read(FIRST_TIME)
+    assert "mktemp" in body
+    assert "> /tmp/ledmatrix_web_sudoers" not in body
+    assert ">> /tmp/ledmatrix_web_sudoers" not in body
+
+
+def test_configure_web_sudo_validates_before_installing():
+    body = _read(CONFIGURE)
+    assert 'visudo -c -f "$TEMP_SUDOERS"' in body
+    install = body.index('cp "$TEMP_SUDOERS" /etc/sudoers.d/ledmatrix_web')
+    validate = body.index('visudo -c -f "$TEMP_SUDOERS"')
+    assert validate < install, "the rules must be checked before they are installed"
+
+
+def _render_first_time_sudoers(project_root, user):
+    """Run the installer's own sudoers heredoc with realistic values."""
+    body = _read(FIRST_TIME)
+    start = body.index("# Create sudoers content")
+    end = body.index("# Never install rules we have not parsed.")
+    block = body[start:end]
+    out = os.path.join(project_root, "rendered")
+    script = "\n".join(
+        [
+            "set -euo pipefail",
+            f"ACTUAL_USER={user}",
+            f"PROJECT_ROOT_DIR={project_root}",
+            'SUDOERS_TMP="$(mktemp)"',
+            "PYTHON_PATH=$(which python3)",
+            "SYSTEMCTL_PATH=/usr/bin/systemctl",
+            "REBOOT_PATH=/usr/sbin/reboot",
+            "POWEROFF_PATH=/usr/sbin/poweroff",
+            "BASH_PATH=$(which bash)",
+            "JOURNALCTL_PATH=/usr/bin/journalctl",
+            block,
+            f'cp "$SUDOERS_TMP" {out}',
+        ]
+    )
+    subprocess.run(["bash", "-c", script], check=True)
+    return out
+
+
+@pytest.mark.skipif(sys.platform == "win32", reason="visudo is POSIX only")
+@pytest.mark.skipif(VISUDO is None, reason="visudo not installed")
+def test_the_rules_the_installer_emits_actually_parse():
+    with tempfile.TemporaryDirectory() as tmp:
+        rendered = _render_first_time_sudoers(tmp, "ledmatrix")
+        os.chmod(rendered, 0o440)
+        result = subprocess.run(
+            [VISUDO, "-c", "-f", rendered], capture_output=True, text=True
+        )
+        assert result.returncode == 0, result.stdout + result.stderr
+
+
+@pytest.mark.skipif(sys.platform == "win32", reason="visudo is POSIX only")
+@pytest.mark.skipif(VISUDO is None, reason="visudo not installed")
+def test_a_broken_rule_is_caught_rather_than_installed():
+    """The guard is only worth having if visudo rejects what it should."""
+    with tempfile.TemporaryDirectory() as tmp:
+        rendered = _render_first_time_sudoers(tmp, "ledmatrix")
+        with open(rendered, "r", encoding="utf-8") as handle:
+            good = handle.read()
+        broken = os.path.join(tmp, "broken")
+        with open(broken, "w", encoding="utf-8") as handle:
+            # An empty command path is what an unset $BASH_PATH would produce.
+            handle.write(good + "\nledmatrix ALL=(ALL) NOPASSWD:\n")
+        os.chmod(broken, 0o440)
+        result = subprocess.run(
+            [VISUDO, "-c", "-f", broken], capture_output=True, text=True
+        )
+        assert result.returncode != 0