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}")