From cc258aaffd23b4636a198e3e229b0c1ecd242334 Mon Sep 17 00:00:00 2001
From: Chuck <33324927+ChuckBuilds@users.noreply.github.com>
Date: Fri, 21 Aug 2026 14:28:23 -0400
Subject: [PATCH] fix(install): tag the secondary installer's journalctl grants
 too (#491)

Review was right on all three counts, and the first is the one that matters:
scripts/install/configure_web_sudo.sh writes the same three wildcard
journalctl rules as first_time_install.sh and none of them carried NOEXEC. So
this PR closed the pager escape on one installer path and left it open on the
other, which is close to no fix at all -- a rig configured through that script
still hands out a root shell via less's "!command".

The test could not have caught it, for two independent reasons. INSTALLERS
did not list the file. And even listed, _grant_lines() kept the raw source
line: that installer echoes its rules, so each one ends in a quote rather
than the wildcard, and the trailing-* check skipped every one of them. Either
alone would have hidden it.

Both fixed: the file is covered, and an echoed rule is unwrapped to the
sudoers line it actually emits.

The selector test now covers -t ledmatrix as well. It asserted only the two
-u forms, so deleting the -t rule would have passed.

Verified by removing NOEXEC again from the secondary installer: four of the
six tests fail, where before the suite passed with the vulnerability present.


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

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 scripts/install/configure_web_sudo.sh | 11 ++++++++---
 test/test_sudoers_noexec_on_pagers.py | 24 ++++++++++++++++++------
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/scripts/install/configure_web_sudo.sh b/scripts/install/configure_web_sudo.sh
index feccc3e7..bbcc14ed 100644
--- a/scripts/install/configure_web_sudo.sh
+++ b/scripts/install/configure_web_sudo.sh
@@ -100,10 +100,15 @@ TEMP_SUDOERS="/tmp/ledmatrix_web_sudoers_$$"
     echo "$WEB_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart ledmatrix-web.service"
 
     # Optional: journalctl (non-critical — skip if not found)
+    #
+    # NOEXEC, matching first_time_install.sh. These rules end in a wildcard and
+    # journalctl starts a pager, so without it the caller can reach a shell:
+    # less runs "!command" as the user the pager belongs to, which here is
+    # root. NOEXEC stops the granted command executing anything of its own.
     if [ -n "$JOURNALCTL_PATH" ]; then
-        echo "$WEB_USER ALL=(ALL) NOPASSWD: $JOURNALCTL_PATH -u ledmatrix.service *"
-        echo "$WEB_USER ALL=(ALL) NOPASSWD: $JOURNALCTL_PATH -u ledmatrix *"
-        echo "$WEB_USER ALL=(ALL) NOPASSWD: $JOURNALCTL_PATH -t ledmatrix *"
+        echo "$WEB_USER ALL=(ALL) NOPASSWD:NOEXEC: $JOURNALCTL_PATH -u ledmatrix.service *"
+        echo "$WEB_USER ALL=(ALL) NOPASSWD:NOEXEC: $JOURNALCTL_PATH -u ledmatrix *"
+        echo "$WEB_USER ALL=(ALL) NOPASSWD:NOEXEC: $JOURNALCTL_PATH -t ledmatrix *"
     fi
 
     # Required: python3, bash
diff --git a/test/test_sudoers_noexec_on_pagers.py b/test/test_sudoers_noexec_on_pagers.py
index 623622d7..ca657785 100644
--- a/test/test_sudoers_noexec_on_pagers.py
+++ b/test/test_sudoers_noexec_on_pagers.py
@@ -30,6 +30,10 @@ ROOT = Path(__file__).resolve().parent.parent
 INSTALLERS = (
     ROOT / "first_time_install.sh",
     ROOT / "scripts" / "install" / "configure_wifi_permissions.sh",
+    # Writes the same journalctl grants as first_time_install.sh. It was
+    # missing here, and because of that this suite passed while three
+    # ungranted wildcard rules sat in it.
+    ROOT / "scripts" / "install" / "configure_web_sudo.sh",
 )
 
 #: Commands that will start another program of their own accord -- a pager, an
@@ -44,8 +48,14 @@ def _grant_lines():
             continue
         for line in installer.read_text(encoding="utf-8", errors="replace").splitlines():
             stripped = line.strip()
-            if "NOPASSWD" in stripped and not stripped.startswith("#"):
-                lines.append(stripped)
+            if "NOPASSWD" not in stripped or stripped.startswith("#"):
+                continue
+            # Installers emit rules two ways: written literally into a heredoc,
+            # or echoed into a file. An echoed rule ends in a quote, so the
+            # trailing-wildcard check below would skip it and the rule would
+            # never be examined at all.
+            echoed = re.fullmatch(r"""echo\s+(['"])(.*)\1""", stripped)
+            lines.append(echoed.group(2) if echoed else stripped)
     return lines
 
 
@@ -78,10 +88,12 @@ def test_journalctl_is_granted_at_all():
         "no journalctl grant remains; the web interface reads logs through it")
 
 
-@pytest.mark.parametrize("unit", ["ledmatrix.service", "ledmatrix"])
-def test_each_journalctl_rule_is_tagged(unit):
+@pytest.mark.parametrize("selector", ["-u ledmatrix.service", "-u ledmatrix",
+                                      "-t ledmatrix"])
+def test_each_journalctl_rule_is_tagged(selector):
+    """Every selector, so removing one cannot pass by the others' presence."""
     matching = [r for r in _grant_lines()
-                if "JOURNALCTL_PATH" in r and f"-u {unit} " in r]
-    assert matching, f"no journalctl rule for -u {unit}"
+                if "JOURNALCTL_PATH" in r and f"{selector} " in r]
+    assert matching, f"no journalctl rule for {selector}"
     untagged = [r for r in matching if "NOEXEC" not in r]
     assert not untagged, f"untagged journalctl rule(s): {untagged}"