From e1ce7189f1c6a6934cecc11f3fc7822c8ce1f1d3 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Wed, 23 Sep 2026 10:34:20 -0400 Subject: [PATCH] fix(install): make one-shot retry() retry, and drop root grants on user files (#606) retry() in one-shot-install.sh used `if ! "$@"; then status=$?`, where $? is the status of the negation -- always 0. A failed command was never retried and retry() reported success, so a failed `git clone` carried on until a later check noticed the missing checkout. It now retries (3 attempts) and returns the command's status. The two apt steps stay non-fatal: warning and continuing is what they effectively did before, and making them fatal would stop installs that work today. A clone that keeps failing stops the install, as it already did, just sooner and with the one-shot's own error message. Both installers granted the web user NOPASSWD root on display_controller.py, start_display.sh and stop_display.sh. Those files are owned by the user after Step 11's chown, so the grant let the web user rewrite them and run them as root, and nothing ever ran them through sudo. Removed from both installers, with a test that every project file granted as root is a root-owned fix_perms helper. Co-authored-by: Claude Opus 5.5 --- first_time_install.sh | 3 -- scripts/install/configure_web_sudo.sh | 9 ---- scripts/install/one-shot-install.sh | 35 +++++++----- test/test_one_shot_retry.py | 65 +++++++++++++++++++++++ test/test_web_sudoers_installers_agree.py | 15 ++++++ 5 files changed, 102 insertions(+), 25 deletions(-) create mode 100644 test/test_one_shot_retry.py diff --git a/first_time_install.sh b/first_time_install.sh index 5749f533..98fab2fd 100755 --- a/first_time_install.sh +++ b/first_time_install.sh @@ -1535,9 +1535,6 @@ $ACTUAL_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH is-active ledmatrix.service $ACTUAL_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH start ledmatrix-web.service $ACTUAL_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH stop ledmatrix-web.service $ACTUAL_USER ALL=(ALL) NOPASSWD: $SYSTEMCTL_PATH restart ledmatrix-web.service -$ACTUAL_USER ALL=(ALL) NOPASSWD: $PYTHON_PATH $PROJECT_ROOT_DIR/display_controller.py -$ACTUAL_USER ALL=(ALL) NOPASSWD: $BASH_PATH $PROJECT_ROOT_DIR/start_display.sh -$ACTUAL_USER ALL=(ALL) NOPASSWD: $BASH_PATH $PROJECT_ROOT_DIR/stop_display.sh $ACTUAL_USER ALL=(ALL) NOPASSWD: $BASH_PATH $PROJECT_ROOT_DIR/scripts/fix_perms/safe_plugin_rm.sh * # Install a requirements.txt as root via vetted helper, so packages are visible # to root-run ledmatrix.service (not just the web interface's own user). diff --git a/scripts/install/configure_web_sudo.sh b/scripts/install/configure_web_sudo.sh index e73f5916..762327e9 100755 --- a/scripts/install/configure_web_sudo.sh +++ b/scripts/install/configure_web_sudo.sh @@ -111,13 +111,6 @@ TEMP_SUDOERS="/tmp/ledmatrix_web_sudoers_$$" echo "$WEB_USER ALL=(ALL) NOPASSWD:NOEXEC: $JOURNALCTL_PATH -t ledmatrix *" fi - # Required: python3, bash - # NOTE: display_controller.py/start_display.sh/stop_display.sh live at the - # project root, not under scripts/install/ (where this script lives) — - # must use PROJECT_ROOT here, not PROJECT_DIR. - echo "$WEB_USER ALL=(ALL) NOPASSWD: $PYTHON_PATH $PROJECT_ROOT/display_controller.py" - echo "$WEB_USER ALL=(ALL) NOPASSWD: $BASH_PATH $PROJECT_ROOT/start_display.sh" - echo "$WEB_USER ALL=(ALL) NOPASSWD: $BASH_PATH $PROJECT_ROOT/stop_display.sh" echo "" echo "# Allow web user to remove plugin directories via vetted helper script" echo "# The helper validates that the target path resolves inside plugin-repos/ or plugins/" @@ -155,8 +148,6 @@ echo "- Start/stop/restart the ledmatrix service" echo "- Enable/disable the ledmatrix service" echo "- Check service status" echo "- View system logs via journalctl" -echo "- Run display_controller.py directly" -echo "- Execute start_display.sh and stop_display.sh" echo "- Reboot and shutdown the system" echo "- Remove plugin directories (for update/uninstall when root-owned files block deletion)" echo "- Install plugin/base requirements.txt as root (so ledmatrix.service can see them)" diff --git a/scripts/install/one-shot-install.sh b/scripts/install/one-shot-install.sh index 52921ec6..37276027 100755 --- a/scripts/install/one-shot-install.sh +++ b/scripts/install/one-shot-install.sh @@ -65,15 +65,14 @@ retry() { local delay_seconds=5 local status while true; do - # Run command in a context that disables errexit so we can capture exit code - # This prevents errexit from triggering before status=$? runs - if ! "$@"; then - status=$? - else - status=0 - fi - if [ $status -eq 0 ]; then + # The condition of an if doesn't trip errexit, and in the else branch + # $? is the command's own exit status. (This used to be `if ! "$@"; + # then status=$?`, where $? is the status of the negation -- always 0 -- + # so a failure never retried and was reported as success.) + if "$@"; then return 0 + else + status=$? fi if [ $attempt -ge $max_attempts ]; then print_error "Command failed after $attempt attempts: $*" @@ -259,22 +258,32 @@ main() { # Update package list first. first_time_install.sh is told the lists are # already fresh so it does not repeat this a minute later. + # A refresh that still fails after retries (say one unreachable mirror) + # only warns: that is what this step effectively did before retry() + # could report a failure, and making it fatal would stop installs that + # work today. if [ "$EUID" -eq 0 ]; then - retry apt-get update -qq + retry apt-get update -qq || print_warning "apt-get update failed; continuing with the existing package lists" else - retry sudo apt-get update -qq + retry sudo apt-get update -qq || print_warning "apt-get update failed; continuing with the existing package lists" fi export LEDMATRIX_APT_UPDATED=1 # Install git and curl (needed for cloning and the script itself) if ! command -v git >/dev/null 2>&1 || ! command -v curl >/dev/null 2>&1; then print_warning "git or curl not found, installing..." + # Not fatal here, for the same reason: without git the clone below + # fails and stops the install with its own error. if [ "$EUID" -eq 0 ]; then - retry apt-get install -y git curl + retry apt-get install -y git curl || true else - retry sudo apt-get install -y git curl + retry sudo apt-get install -y git curl || true + fi + if command -v git >/dev/null 2>&1 && command -v curl >/dev/null 2>&1; then + print_success "git and curl installed" + else + print_warning "Could not install git and curl" fi - print_success "git and curl installed" else print_success "git and curl already installed" fi diff --git a/test/test_one_shot_retry.py b/test/test_one_shot_retry.py new file mode 100644 index 00000000..7682e804 --- /dev/null +++ b/test/test_one_shot_retry.py @@ -0,0 +1,65 @@ +"""retry() in scripts/install/one-shot-install.sh retries, and reports failure. + +It used `if ! "$@"; then status=$?`, where $? is the status of the negation, +always 0: a failed command was never retried and retry() returned success, +so a failed `git clone` carried on until a later check noticed the missing +checkout. The apt steps now retry for real but stay non-fatal, as they +effectively were; a clone that keeps failing stops the install. +""" +import re +import subprocess +import sys +from pathlib import Path + +import pytest + +ONE_SHOT = Path(__file__).resolve().parent.parent / "scripts" / "install" / "one-shot-install.sh" + +pytestmark = pytest.mark.skipif( + not sys.platform.startswith("linux"), reason="runs the installer's bash under Linux" +) + + +def _function(name): + text = ONE_SHOT.read_text(encoding="utf-8") + m = re.search(rf"^{name}\(\) \{{\n.*?^\}}\n", text, re.S | re.M) + assert m, f"{name}() not found in one-shot-install.sh" + return m.group(0) + + +def _run(snippet): + script = ( + "set -Eeuo pipefail\n" + "trap 'echo ERR_TRAP_FIRED >&2; exit 99' ERR\n" + "print_error() { echo \"E: $*\" >&2; }\n" + "print_warning() { echo \"W: $*\" >&2; }\n" + "sleep() { :; }\n" + f"{_function('retry')}\n" + f"{snippet}\n" + ) + return subprocess.run(["bash", "-c", script], capture_output=True, text=True) + + +def test_failure_is_retried_and_reported(): + r = _run("n=0; f() { n=$((n+1)); return 7; }\n" + "if retry f; then echo OK; else echo \"FAILED $? after $n\"; fi") + assert r.stdout.strip() == "FAILED 7 after 3", r.stdout + r.stderr + + +def test_success_on_a_later_attempt(): + r = _run("n=0; f() { n=$((n+1)); [ $n -ge 2 ]; }\n" + "retry f && echo \"OK after $n\"") + assert r.stdout.strip() == "OK after 2", r.stdout + r.stderr + + +def test_a_plain_call_that_keeps_failing_stops_the_script(): + r = _run("retry false\necho SHOULD_NOT_RUN") + assert "SHOULD_NOT_RUN" not in r.stdout + assert "ERR_TRAP_FIRED" in r.stderr + + +def test_apt_steps_stay_non_fatal(): + text = ONE_SHOT.read_text(encoding="utf-8") + for line in text.splitlines(): + if re.search(r"\bretry (sudo )?apt-get ", line): + assert "||" in line, f"apt step would now abort the install: {line.strip()}" diff --git a/test/test_web_sudoers_installers_agree.py b/test/test_web_sudoers_installers_agree.py index 67e858e0..7a6d10d7 100644 --- a/test/test_web_sudoers_installers_agree.py +++ b/test/test_web_sudoers_installers_agree.py @@ -129,3 +129,18 @@ def test_every_granted_helper_is_hardened_in_first_time_install_after_chown(): assert loop.start() > project_chown, ( "helper hardening runs before Step 11's project-wide chown, which undoes it") assert _granted_helpers() <= set(loop.group(1).split()) + + +def test_no_grant_runs_a_file_the_web_user_can_edit(): + """Every project file granted as root must be a fix_perms helper, which + both installers chown root:root (checked above). Anything else under the + project root is owned by the user after Step 11's chown, so a NOPASSWD + rule for it lets the web user rewrite the file and run it as root. The + grants for display_controller.py, start_display.sh and stop_display.sh + were exactly that, and nothing ever ran them through sudo.""" + for installer in (FIRST_TIME, CONFIGURE): + for _, command in _grants(installer): + for token in command.split(): + if token.startswith("$PROJECT_ROOT/"): + assert token.startswith("$PROJECT_ROOT/scripts/fix_perms/"), ( + f"{installer.name} grants root on a user-owned file: {command}")