diff --git a/CHANGELOG.md b/CHANGELOG.md index f77818dc..e516de35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -216,6 +216,30 @@ Core: `ensure_shared_group_ownership()` now returns immediately when `os.geteuid` or `os.chown` is missing. No behaviour change on the Pi. +Automatic updates and Update Code: + +- An update that changes `web_interface/requirements.txt` is no longer rolled + back on every auto-updating device. `safe_pip_install.sh` allowed only the + root `requirements.txt`, so the install Update Code and the health check run + for the web requirements was refused, and the health check rolls back any + update whose dependencies failed (Install Base Requirements failed the same + way). The wrapper now allows both core requirement files; a core requirement + file symlinked out of the project is refused. +- The automatic update's local-change check and Update Code now count changes + the same way (`auto_update.local_changes`): permission-only changes and + anything under `plugins/` or `plugin-repos/` don't count, and a core path + that merely contains `plugins/` does. Such edits used to pass the check and + then be stashed by the pull and never restored, despite "will not stash your + changes". The pull's `--autostash` now carries them across. Update Code + still stashes other edits; the automatic update refuses instead. +- When the automatic update's own rollback fails (a partial pull, or a health + check that never started), plugins are no longer updated and the display is + not restarted, as the 3.4.0 notes promised. +- The health check's dependency reinstall no longer retries pip failures or + timeouts with a second bash path, and all reinstalls share a 10-minute + budget, so a rollback finishes inside the unit's 30-minute limit instead of + being killed mid-way. + ## 3.4.0 Plugin-facing changes since 3.3.0 (tag `v3.3.1`) not covered further down: diff --git a/docs/WEB_INTERFACE_GUIDE.md b/docs/WEB_INTERFACE_GUIDE.md index 10ef082b..f2e077b6 100644 --- a/docs/WEB_INTERFACE_GUIDE.md +++ b/docs/WEB_INTERFACE_GUIDE.md @@ -102,7 +102,9 @@ Configure basic system settings: check are shown under the toggle, and anything other than success raises a banner on **Overview**. - *Checks first:* the code update is skipped, with the reason shown, if - tracked files were edited locally, the checkout has local commits, a + tracked files were edited locally (permission-only changes and edits under + `plugins/` or `plugin-repos/` don't count; the pull carries those across + and puts them back), the checkout has local commits, a rebase/merge is in progress, the branch has no upstream, less than 300 MB is free, or the newest version already failed once. A failed fetch is retried the next day. diff --git a/scripts/fix_perms/safe_pip_install.sh b/scripts/fix_perms/safe_pip_install.sh index ae7d266c..06557e6f 100755 --- a/scripts/fix_perms/safe_pip_install.sh +++ b/scripts/fix_perms/safe_pip_install.sh @@ -1,6 +1,7 @@ #!/bin/bash # safe_pip_install.sh — Install a requirements.txt as root after validating -# that the resolved path is the project's own requirements.txt or a plugin's +# that the resolved path is one of the project's own requirements files +# (requirements.txt, web_interface/requirements.txt) or a plugin's # requirements.txt under plugin-repos/ or plugins/. # # This script is intended to be called via sudo from the web interface, so @@ -25,9 +26,17 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" PROJECT_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" # Allowed locations (resolved, no trailing slash): -# - the project's own requirements.txt +# - the project's own requirements files. Update Code, the automatic +# update's health check and Install Base Requirements install both, so +# one missing here is refused on every device -- and the automatic +# updater rolls back any update that changes it. +# Only their folders are resolved: resolving the files too would follow +# a requirements.txt symlinked out of the project and allow its target. # - any requirements.txt under plugin-repos/ or plugins/ -ALLOWED_EXACT="$(realpath --canonicalize-missing "$PROJECT_ROOT/requirements.txt")" +ALLOWED_EXACT=( + "$(realpath --canonicalize-missing "$PROJECT_ROOT")/requirements.txt" + "$(realpath --canonicalize-missing "$PROJECT_ROOT/web_interface")/requirements.txt" +) ALLOWED_BASES=( "$(realpath --canonicalize-missing "$PROJECT_ROOT/plugin-repos")" "$(realpath --canonicalize-missing "$PROJECT_ROOT/plugins")" @@ -43,9 +52,13 @@ if [ "$(basename "$RESOLVED_TARGET")" != "requirements.txt" ]; then fi ALLOWED=false -if [ "$RESOLVED_TARGET" = "$ALLOWED_EXACT" ]; then - ALLOWED=true -else +for EXACT in "${ALLOWED_EXACT[@]}"; do + if [ "$RESOLVED_TARGET" = "$EXACT" ]; then + ALLOWED=true + break + fi +done +if [ "$ALLOWED" = false ]; then for BASE in "${ALLOWED_BASES[@]}"; do if [[ "$RESOLVED_TARGET" == "$BASE/"* ]]; then ALLOWED=true @@ -56,7 +69,7 @@ fi if [ "$ALLOWED" = false ]; then echo "DENIED: $RESOLVED_TARGET is not an allowed requirements.txt location" >&2 - echo "Allowed: $ALLOWED_EXACT, or any requirements.txt under: ${ALLOWED_BASES[*]}" >&2 + echo "Allowed: ${ALLOWED_EXACT[*]}, or any requirements.txt under: ${ALLOWED_BASES[*]}" >&2 exit 2 fi diff --git a/scripts/utils/auto_update_verify.py b/scripts/utils/auto_update_verify.py index 048721a2..d75fe276 100644 --- a/scripts/utils/auto_update_verify.py +++ b/scripts/utils/auto_update_verify.py @@ -42,10 +42,30 @@ HEALTH_TIMEOUT_SECONDS = 180 #: loop look healthy between attempts, so a single "is-active" proves nothing. STABLE_SECONDS = 45 POLL_SECONDS = 5 +WEB_CHECK_TIMEOUT_SECONDS = 5 +SYSTEMCTL_QUERY_TIMEOUT_SECONDS = 10 +RESTART_TIMEOUT_SECONDS = 90 +GIT_TIMEOUT_SECONDS = 60 +GIT_RESET_TIMEOUT_SECONDS = 120 PIP_TIMEOUT_SECONDS = 600 +#: All of a rollback's dependency reinstalls together. A pip that times out +#: or fails is not retried: systemd stops this unit at TimeoutStartSec, and a +#: rollback killed half-way leaves the update reported as still verifying. +PIP_BUDGET_SECONDS = 600 #: sudoers matches the exact command line, so bash is named by path, the same -#: candidates src/common/permission_utils.install_requirements_file tries. +#: candidates src/common/permission_utils.install_requirements_file tries... BASH_CANDIDATES = ('/usr/bin/bash', '/bin/bash') +#: ...and, like it, moves to the next one only when sudo refused the command +#: line (permission_utils.SUDO_REFUSAL_PHRASES), never after pip itself ran. +SUDO_REFUSAL_PHRASES = ('a password is required', 'is not allowed to run', 'no tty present') + +#: The longest one health check can take: restart and wait, roll back +#: (diff, reset, reinstalls), restart and wait again. A wait's last poll can +#: start just before its deadline and run every query to its timeout. +_WAIT_WORST_SECONDS = (HEALTH_TIMEOUT_SECONDS + STABLE_SECONDS + WEB_CHECK_TIMEOUT_SECONDS + + 2 * SYSTEMCTL_QUERY_TIMEOUT_SECONDS + POLL_SECONDS) +WORST_CASE_SECONDS = (2 * (2 * RESTART_TIMEOUT_SECONDS + _WAIT_WORST_SECONDS) + + GIT_TIMEOUT_SECONDS + GIT_RESET_TIMEOUT_SECONDS + PIP_BUDGET_SECONDS) #: What a command that could not run at all reports: its callers only read #: these three fields, the same ones a completed subprocess has. @@ -83,7 +103,7 @@ def write_pending(path, data): def _web_responds(url=WEB_HEALTH_URL): try: - with urllib.request.urlopen(url, timeout=5) as resp: # nosec B310 - fixed loopback URL + with urllib.request.urlopen(url, timeout=WEB_CHECK_TIMEOUT_SECONDS) as resp: # nosec B310 - fixed loopback URL return resp.status == 200 except (urllib.error.URLError, OSError, ValueError): return False @@ -104,7 +124,7 @@ class Verifier: self.web_responds = web_responds self.log = log or (lambda msg: print(f'[auto-update-verify] {msg}', flush=True)) - def _run(self, args, timeout=60): + def _run(self, args, timeout=GIT_TIMEOUT_SECONDS): try: return self.run(args, cwd=str(self.project_root), capture_output=True, text=True, timeout=timeout) @@ -114,15 +134,17 @@ class Verifier: # -- services --------------------------------------------------------- def service_active(self, unit): - return self._run(['systemctl', 'is-active', unit], timeout=10).stdout.strip() == 'active' + return self._run(['systemctl', 'is-active', unit], + timeout=SYSTEMCTL_QUERY_TIMEOUT_SECONDS).stdout.strip() == 'active' def restart_count(self, unit): out = self._run(['systemctl', 'show', '-p', 'NRestarts', '--value', unit], - timeout=10).stdout.strip() + timeout=SYSTEMCTL_QUERY_TIMEOUT_SECONDS).stdout.strip() return int(out) if out.isdigit() else None def restart(self, unit): - result = self._run(['sudo', '-n', 'systemctl', 'restart', f'{unit}.service'], timeout=90) + result = self._run(['sudo', '-n', 'systemctl', 'restart', f'{unit}.service'], + timeout=RESTART_TIMEOUT_SECONDS) if result.returncode != 0: self.log(f'restarting {unit} failed: {(result.stderr or "").strip()}') return result.returncode == 0 @@ -173,16 +195,28 @@ class Verifier: changed = set(result.stdout.split()) if result.returncode == 0 else set(REQUIREMENT_FILES) return [rel for rel in REQUIREMENT_FILES if rel in changed] - def install_requirements(self, rel): + def install_requirements(self, rel, deadline=None): + """Install one requirements file through the root wrapper, by ``deadline``.""" wrapper = self.project_root / 'scripts' / 'fix_perms' / 'safe_pip_install.sh' req = self.project_root / rel if not req.exists(): return True for bash in BASH_CANDIDATES: - result = self._run(['sudo', '-n', bash, str(wrapper), str(req)], - timeout=PIP_TIMEOUT_SECONDS) + timeout = PIP_TIMEOUT_SECONDS + if deadline is not None: + timeout = min(timeout, deadline - self.clock()) + if timeout <= 0: + self.log(f'no time left to reinstall {rel}') + return False + result = self._run(['sudo', '-n', bash, str(wrapper), str(req)], timeout=timeout) if result.returncode == 0: return True + # Only a refused command line is worth the next candidate. A pip + # that ran and failed, or timed out, would just do it again. + if not any(phrase in (result.stderr or '') for phrase in SUDO_REFUSAL_PHRASES): + # Not pip's output: it can echo an index URL's credentials. + self.log(f'reinstalling {rel} failed (exit {result.returncode})') + return False return False def rollback(self, pending): @@ -191,13 +225,17 @@ class Verifier: if not old: return False, 'the commit to roll back to is unknown' requirements = self.changed_requirements(old, new) if new else list(REQUIREMENT_FILES) - # Safe to --hard: the updater refuses to run with local edits to - # tracked files, so the only thing this discards is the update. - result = self._run(['git', 'reset', '--hard', old], timeout=120) + # --hard: the updater refuses to run with local edits to tracked core + # files (web_interface/auto_update.local_changes), so outside the + # plugin folders the only thing this discards is the update. Edits + # under plugins/ and plugin-repos/, which that check leaves to the + # pull's --autostash, are reset along with it. + result = self._run(['git', 'reset', '--hard', old], timeout=GIT_RESET_TIMEOUT_SECONDS) if result.returncode != 0: return False, (f'"git reset --hard {old}" failed: ' f'{(result.stderr or result.stdout or "").strip()}') - failed = [rel for rel in requirements if not self.install_requirements(rel)] + deadline = self.clock() + PIP_BUDGET_SECONDS + failed = [rel for rel in requirements if not self.install_requirements(rel, deadline)] if failed: return True, ('reinstalling the previous dependencies from ' + ', '.join(failed) + ' failed; run Install Base Requirements from the Tools tab') diff --git a/src/common/permission_utils.py b/src/common/permission_utils.py index 29be2f04..f59b31cc 100644 --- a/src/common/permission_utils.py +++ b/src/common/permission_utils.py @@ -373,6 +373,14 @@ def sudo_remove_directory(path: Path, allowed_bases: Optional[list] = None) -> b return False +#: What sudo prints when it refuses a command line outright (not in sudoers for +#: that exact argv, or it wants a password). Only then is another bash path +#: worth trying: after pip itself ran, a retry just repeats the failure. The +#: automatic update's health check keeps its own copy of this list +#: (scripts/utils/auto_update_verify.py runs without importing src/). +SUDO_REFUSAL_PHRASES = ("a password is required", "is not allowed to run", "no tty present") + + def install_requirements_file(req_file: Path, timeout: int = 300) -> subprocess.CompletedProcess: """ Install a requirements.txt file for a plugin (or the project itself). @@ -434,10 +442,7 @@ def install_requirements_file(req_file: Path, timeout: int = 300) -> subprocess. # Distinguish "sudo rejected this exact command line" (worth # trying the next bash candidate) from "sudo ran it but pip # itself failed" (a real error — stop and surface it). - denied = any( - phrase in result.stderr - for phrase in ("a password is required", "is not allowed to run", "no tty present") - ) + denied = any(phrase in result.stderr for phrase in SUDO_REFUSAL_PHRASES) if not denied: # Deliberately don't interpolate req_file or the pip output here: # this log line is scanner-visible, and a static analyzer can't diff --git a/test/test_auto_update.py b/test/test_auto_update.py index 8b693f5d..46a02230 100644 --- a/test/test_auto_update.py +++ b/test/test_auto_update.py @@ -72,7 +72,7 @@ class Repo: def real_pull(device, **extra): - def core_update(): + def core_update(**kwargs): before = git(device, 'rev-parse', 'HEAD') r = subprocess.run(['git', 'pull', '-q', '--rebase', '--autostash'], cwd=str(device), capture_output=True, text=True) @@ -339,6 +339,140 @@ class TestPreflightRefuses: assert repo.head() == old and h.sudo == [] +# -- what counts as a local change --------------------------------------------- + +def _device_with_plugin_and_script(tmp_path): + """A clone carrying a bundled plugin and an executable script, as LEDMatrix does.""" + repo = Repo(tmp_path) + for rel, text in (('plugin-repos/web-ui-info/manager.py', 'x = 1\n'), + ('web_interface/static/v3/js/plugins/store.js', 'var a;\n'), + ('scripts/run.sh', 'echo hi\n')): + (repo.seed / rel).parent.mkdir(parents=True, exist_ok=True) + (repo.seed / rel).write_text(text) + git(repo.seed, 'add', '.') + git(repo.seed, 'update-index', '--chmod=+x', 'scripts/run.sh') + git(repo.seed, 'commit', '-qm', 'layout') + git(repo.seed, 'push', '-q', 'origin', 'main') + git(repo.device, 'pull', '-q') + return repo + + +def _chmod_like_the_installer(device): + """first_time_install.sh sets tracked 100755 files to 644: a mode-only change. + + Windows has no exec bit, so there core.fileMode=true alone shows it.""" + import os + git(device, 'config', 'core.fileMode', 'true') + os.chmod(device / 'scripts' / 'run.sh', 0o644) + assert 'mode change 100755 => 100644 scripts/run.sh' in git(device, 'diff', '--summary') + + +@needs_git +class TestLocalChangesAreCountedOnce: + """The preflight promises the pull will not stash. That holds only if both + count local changes the same way (auto_update.local_changes).""" + + def test_mode_changes_and_plugin_folders_do_not_count(self, tmp_path): + repo = _device_with_plugin_and_script(tmp_path) + _chmod_like_the_installer(repo.device) + (repo.device / 'plugin-repos/web-ui-info/manager.py').write_text('x = 2 # store update\n') + assert au.local_changes(repo.device) == [] + + def test_a_core_folder_named_plugins_still_counts(self, tmp_path): + """The old filter matched 'plugins/' anywhere in the line.""" + repo = _device_with_plugin_and_script(tmp_path) + (repo.device / 'web_interface/static/v3/js/plugins/store.js').write_text('var mine;\n') + assert au.local_changes(repo.device) == ['web_interface/static/v3/js/plugins/store.js'] + + def test_a_staged_rename_is_reported_by_its_new_name(self, tmp_path): + repo = Repo(tmp_path) + git(repo.device, 'mv', 'app.py', 'main.py') + assert au.local_changes(repo.device) == ['main.py'] + + def test_the_preflight_refuses_a_core_edit_under_a_plugins_folder(self, tmp_path): + repo = _device_with_plugin_and_script(tmp_path) + old = repo.head() + repo.publish() + (repo.device / 'web_interface/static/v3/js/plugins/store.js').write_text('var mine;\n') + result = Harness(tmp_path, repo).updater.run() + assert result['core_outcome'] == 'blocked' + assert 'store.js' in result['core_message'] + assert repo.head() == old + + @pytest.fixture + def core_update(self, monkeypatch): + """The real perform_core_update, pointed at a test clone.""" + from web_interface.blueprints import api_v3 as pkg + from web_interface.blueprints.api_v3 import system + + def point_at(device): + monkeypatch.setattr(system, 'PROJECT_ROOT', device) + monkeypatch.setattr(pkg.api_v3, 'plugin_store_manager', None, raising=False) + monkeypatch.setattr(system, '_pip_install_requirements', + lambda *a, **k: pytest.fail('no requirements changed')) + return system.perform_core_update + return point_at + + @pytest.mark.parametrize('change', [ + 'bundled_plugin_edit', + pytest.param('installer_chmod', marks=pytest.mark.skipif( + sys.platform == 'win32', + reason='no exec bit: the reset inside --autostash cannot clear a mode change, ' + 'so git will not reapply it (on Linux it reapplies cleanly)')), + ]) + def test_an_update_that_passed_the_preflight_leaves_no_stash(self, tmp_path, core_update, change): + """Found by audit: a bundled-plugin edit or the installer's chmods + passed the preflight, then perform_core_update stashed them for good. + Now --autostash carries them across the pull and puts them back.""" + repo = _device_with_plugin_and_script(tmp_path) + new = repo.publish() + if change == 'installer_chmod': + _chmod_like_the_installer(repo.device) + else: + (repo.device / 'plugin-repos/web-ui-info/manager.py').write_text('x = 2 # store update\n') + h = Harness(tmp_path, repo, core_update=core_update(repo.device)) + assert h.updater.preflight({})[0] == 'ready' + + result = h.updater.run() + assert result['core_outcome'] == 'verifying', result['core_message'] + assert repo.head() == new + assert git(repo.device, 'stash', 'list') == '' + if change == 'installer_chmod': + assert 'mode change 100755 => 100644 scripts/run.sh' in git(repo.device, 'diff', '--summary') + else: + assert 'store update' in (repo.device / 'plugin-repos/web-ui-info/manager.py').read_text() + + def test_edits_made_after_the_preflight_are_refused_not_stashed(self, tmp_path, core_update): + repo = Repo(tmp_path) + old = repo.head() + repo.publish() + perform = core_update(repo.device) + + def edit_then_update(**kwargs): + (repo.device / 'app.py').write_text('mine\n') + return perform(**kwargs) + result = Harness(tmp_path, repo, core_update=edit_then_update).updater.run() + assert result['core_outcome'] == 'blocked' + assert 'app.py' in result['core_message'] and 'will not stash' in result['core_message'] + assert repo.head() == old + assert git(repo.device, 'stash', 'list') == '' + assert (repo.device / 'app.py').read_text() == 'mine\n' + + def test_update_code_still_stashes_core_edits_but_not_plugin_folders(self, tmp_path, core_update): + repo = _device_with_plugin_and_script(tmp_path) + new = repo.publish() + (repo.device / 'scripts/run.sh').write_text('echo mine\n') + (repo.device / 'plugin-repos/web-ui-info/manager.py').write_text('x = 2 # store update\n') + payload = core_update(repo.device)() + assert payload['status'] == 'success', payload['message'] + assert 'stashed' in payload['message'] + assert repo.head() == new + assert 'LEDMatrix auto-stash before update' in git(repo.device, 'stash', 'list') + stashed = git(repo.device, 'stash', 'show', '--name-only', 'stash@{0}') + assert stashed.split() == ['scripts/run.sh'] + assert 'store update' in (repo.device / 'plugin-repos/web-ui-info/manager.py').read_text() + + # -- the update and its hand-off to the health check --------------------------- @needs_git @@ -395,7 +529,7 @@ class TestUpdateIsVerified: old = repo.head() repo.publish() h = Harness(tmp_path, repo, - core_update=lambda: {'status': 'error', 'message': 'Update failed: network'}) + core_update=lambda **kw: {'status': 'error', 'message': 'Update failed: network'}) result = h.updater.run() assert result['core_outcome'] == 'error' assert repo.head() == old @@ -407,13 +541,43 @@ class TestUpdateIsVerified: repo.publish() pull = real_pull(repo.device) - def half_failed(): + def half_failed(**kwargs): pull() return {'status': 'error', 'message': 'Update failed: interrupted'} result = Harness(tmp_path, repo, core_update=half_failed).updater.run() assert repo.head() == old assert 'rolled back' in result['core_message'] + @pytest.mark.parametrize('how', ['partial_pull', 'check_never_started']) + def test_a_failed_rollback_leaves_plugins_and_the_display_alone(self, tmp_path, how): + """update_core returns rollback_failed itself on two paths. The device + is then in an unknown state, exactly as when the health check reports + rollback_failed, so no plugin updates and no restart may follow.""" + repo = Repo(tmp_path) + repo.publish() + store = FakeStore(tmp_path / 'plugins', {'clock': '1.0.0'}, bump={'clock'}) + pull = real_pull(repo.device) + + def half_failed(**kwargs): + pull() + return {'status': 'error', 'message': 'Update failed: interrupted'} + h = Harness(tmp_path, repo, store=store, pickup=how != 'check_never_started', + core_update=half_failed if how == 'partial_pull' else None) + real_run = h.updater.run_command + + def reset_fails(args, **kwargs): + if args[:3] == ['git', 'reset', '--hard']: + return subprocess.CompletedProcess(args, 1, stdout='', stderr='index.lock exists') + return real_run(args, **kwargs) + h.updater.run_command = reset_fails + + result = h.updater.run() + assert result['core_outcome'] == 'rollback_failed' + assert store.updated_calls == [], "plugins must not pile onto a core in an unknown state" + assert h.restarts == [] + assert h.state['plugins_pending'] is False and result['plugins_deferred'] is False + assert h.state['alert'] + # -- reporting the health check's outcome -------------------------------------- diff --git a/test/test_auto_update_verify.py b/test/test_auto_update_verify.py index 8a6c06c2..c346091e 100644 --- a/test/test_auto_update_verify.py +++ b/test/test_auto_update_verify.py @@ -69,6 +69,8 @@ class FakeHost: self.running_head = None # not restarted yet: still the old, healthy code self.restarts = [] # (unit, commit it was restarted onto) self.pip_installs = [] + self.pip_timeouts = [] + self.pip = None # optional (args, host) -> result, or raises, instead of pip_ok self.now = 0.0 self.nrestarts = 0 @@ -90,6 +92,9 @@ class FakeHost: return done(args, f'{self.nrestarts}\n') if args[0] == 'sudo' and any(a.endswith('safe_pip_install.sh') for a in args): self.pip_installs.append(args[-1]) + self.pip_timeouts.append(kwargs.get('timeout')) + if self.pip: + return self.pip(args, self) return done(args, rc=0 if self.pip_ok else 1) if args[:4] == ['sudo', '-n', 'systemctl', 'restart']: if self.restart_failures: @@ -171,6 +176,77 @@ def test_a_failed_dependency_reinstall_is_reported(tmp_path): assert 'Install Base Requirements' in result['detail'] +def _rollback_with_pip(tmp_path, pip, web_requirements_too=False): + """Roll back an update that changed the requirements, with pip faked by ``pip``.""" + repo, old, new = updated_repo(tmp_path, new_requirements=True) + if web_requirements_too: + (repo / 'web_interface').mkdir() + (repo / 'web_interface' / 'requirements.txt').write_text('flask\n') + git(repo, 'add', '.') + git(repo, 'commit', '-qm', 'web reqs') + (repo / 'web_interface' / 'requirements.txt').write_text('flask\nnew\n') + (repo / 'requirements.txt').write_text('requests\nnewer\n') + git(repo, 'commit', '-qam', 'bump both') + old, new = git(repo, 'rev-parse', 'HEAD~1'), git(repo, 'rev-parse', 'HEAD') + host = FakeHost(repo, new) + host.pip = pip + ok, detail = host.verifier().rollback({'old_head': old, 'new_head': new}) + return ok, detail, host + + +def test_a_failed_pip_is_not_run_again_with_the_other_bash(tmp_path): + """Retrying after pip itself ran only repeats it, and every repeat can + take PIP_TIMEOUT_SECONDS of the unit's time limit.""" + ok, detail, host = _rollback_with_pip( + tmp_path, lambda args, h: subprocess.CompletedProcess(args, 1, '', 'ERROR: No matching distribution')) + assert ok and 'requirements.txt' in detail + assert len(host.pip_installs) == 1 + + +def test_a_pip_timeout_is_not_retried(tmp_path): + def slow(args, h): + h.now += h.pip_timeouts[-1] + raise subprocess.TimeoutExpired(args, h.pip_timeouts[-1]) + ok, detail, host = _rollback_with_pip(tmp_path, slow) + assert ok and 'Install Base Requirements' in detail + assert len(host.pip_installs) == 1 + + +def test_a_sudo_refusal_tries_the_next_bash(tmp_path): + def refused_once(args, h): + if len(h.pip_installs) == 1: + return subprocess.CompletedProcess(args, 1, '', 'sudo: a password is required') + return done(args) + ok, detail, host = _rollback_with_pip(tmp_path, refused_once) + assert ok and detail == '' + assert len(host.pip_installs) == 2 + + +def test_reinstalls_share_one_time_budget(tmp_path): + def hangs(args, h): + h.now += h.pip_timeouts[-1] + raise subprocess.TimeoutExpired(args, h.pip_timeouts[-1]) + ok, detail, host = _rollback_with_pip(tmp_path, hangs, web_requirements_too=True) + assert ok and 'requirements.txt' in detail and 'web_interface/requirements.txt' in detail + assert sum(host.pip_timeouts) <= av.PIP_BUDGET_SECONDS + assert len(host.pip_installs) == 1, "the first file used the whole budget" + + +def test_the_worst_case_fits_the_unit_time_limit(): + """systemd kills the check at TimeoutStartSec, mid-rollback, and the update + then sits in "verifying" until the web interface calls it lost.""" + from web_interface import auto_update as au + service = (ROOT / 'systemd' / 'ledmatrix-update-verify.service').read_text(encoding='utf-8') + minutes = int(re.search(r'^TimeoutStartSec=(\d+)min$', service, re.M).group(1)) + assert av.WORST_CASE_SECONDS < minutes * 60 + assert minutes * 60 < au.VERIFY_LOST_SECONDS, "the web UI must not call a running check lost" + + +def test_sudo_refusal_wording_matches_permission_utils(): + from src.common import permission_utils + assert set(av.SUDO_REFUSAL_PHRASES) == set(permission_utils.SUDO_REFUSAL_PHRASES) + + def test_still_broken_after_rolling_back_is_rollback_failed(tmp_path): code, result, host, head, old, new = check(tmp_path, 'always') assert code == 1 and result['status'] == 'rollback_failed' diff --git a/test/test_safe_pip_install_allowlist.py b/test/test_safe_pip_install_allowlist.py new file mode 100644 index 00000000..c907e14b --- /dev/null +++ b/test/test_safe_pip_install_allowlist.py @@ -0,0 +1,128 @@ +"""scripts/fix_perms/safe_pip_install.sh must accept what the updaters install. + +Update Code and the automatic update's health check install the core's own +requirement files through that root wrapper, and the wrapper refuses any path +it does not list. It used to list only requirements.txt, so an update that +changed web_interface/requirements.txt failed its dependency install -- and +the automatic updater rolls back any update whose dependencies did not +install, on every device, every week. + +The real script runs here with only its final pip command swapped for an +echo, so the path checks under test are the shipped ones. +""" +import importlib.util +import os +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(ROOT)) + +WRAPPER_REL = Path('scripts') / 'fix_perms' / 'safe_pip_install.sh' +PIP_LINE = 'exec "$PYTHON_PATH" -m pip install' + + +def _bash(): + """A bash whose realpath understands --canonicalize-missing, or None.""" + candidates = [shutil.which('bash')] + if os.name == 'nt': + candidates.insert(0, r'C:\Program Files\Git\bin\bash.exe') + for bash in candidates: + if not bash or not os.path.exists(bash): + continue + try: + probe = subprocess.run([bash, '-c', 'realpath --canonicalize-missing /no/such/x'], + capture_output=True, text=True, timeout=30) + except (OSError, subprocess.SubprocessError): + continue + if probe.returncode == 0: + return bash + return None + + +BASH = _bash() +pytestmark = pytest.mark.skipif(BASH is None, reason='no bash with GNU realpath') + + +@pytest.fixture +def project(tmp_path): + """A project tree holding the real wrapper, with pip stubbed out.""" + root = tmp_path / 'LEDMatrix' + wrapper = root / WRAPPER_REL + wrapper.parent.mkdir(parents=True) + text = (ROOT / WRAPPER_REL).read_text(encoding='utf-8').replace('\r\n', '\n') + lines = text.split('\n') + pip = [i for i, line in enumerate(lines) if line.startswith(PIP_LINE)] + assert len(pip) == 1, 'the wrapper no longer ends in the pip install this test stubs' + lines[pip[0]] = 'echo "WOULD INSTALL $RESOLVED_TARGET"; exit 0' + wrapper.write_text('\n'.join(lines), encoding='utf-8', newline='\n') + for rel in ('requirements.txt', 'web_interface/requirements.txt', + 'plugin-repos/clock/requirements.txt', 'src/requirements.txt', + 'web_interface/extra/requirements.txt'): + (root / rel).parent.mkdir(parents=True, exist_ok=True) + (root / rel).write_text('requests\n', encoding='utf-8') + return root + + +def run_wrapper(root, rel): + """Run the wrapper on ``rel``, with the path spelled the way bash sees the tree.""" + return subprocess.run( + [BASH, '-c', 'cd "$1" && bash scripts/fix_perms/safe_pip_install.sh "$PWD/$2"', + '_', str(root), rel], + capture_output=True, text=True, timeout=60) + + +def _core_requirement_files(): + from web_interface.blueprints.api_v3 import system + spec = importlib.util.spec_from_file_location( + 'auto_update_verify_for_allowlist', ROOT / 'scripts' / 'utils' / 'auto_update_verify.py') + verifier = importlib.util.module_from_spec(spec) + spec.loader.exec_module(verifier) + assert set(verifier.REQUIREMENT_FILES) == set(system.CORE_REQUIREMENT_FILES), ( + 'the rollback must reinstall the same files Update Code installs') + return system.CORE_REQUIREMENT_FILES + + +@pytest.mark.parametrize('rel', _core_requirement_files()) +def test_every_core_requirement_file_the_updaters_install_is_allowed(project, rel): + result = run_wrapper(project, rel) + assert result.returncode == 0, result.stderr + assert 'WOULD INSTALL' in result.stdout + + +def test_plugin_requirements_are_still_allowed(project): + assert run_wrapper(project, 'plugin-repos/clock/requirements.txt').returncode == 0 + + +@pytest.mark.parametrize('rel', [ + 'src/requirements.txt', # repo-owned folder, but not listed + 'web_interface/extra/requirements.txt', # below an allowed file's folder + 'web_interface/requirements.txt.bak', # not a requirements.txt +]) +def test_other_paths_are_still_refused(project, rel): + if rel.endswith('.bak'): + (project / rel).write_text('requests\n', encoding='utf-8') + result = run_wrapper(project, rel) + assert result.returncode == 2, result.stdout + result.stderr + assert 'DENIED' in result.stderr + + +@pytest.mark.parametrize('rel', ['requirements.txt', 'web_interface/requirements.txt']) +def test_a_core_requirement_file_symlinked_out_of_the_project_is_refused(project, tmp_path, rel): + """Only the allowed files' folders are resolved, so the link's target is + compared, and refused, rather than allowed as the file itself.""" + outside = tmp_path / 'elsewhere' / 'requirements.txt' + outside.parent.mkdir() + outside.write_text('evil\n', encoding='utf-8') + link = project / rel + link.unlink() + try: + link.symlink_to(outside) + except (OSError, NotImplementedError): + pytest.skip('symlinks unavailable') + result = run_wrapper(project, rel) + assert result.returncode == 2, result.stdout + result.stderr diff --git a/web_interface/auto_update.py b/web_interface/auto_update.py index ce76d2e9..610391a1 100644 --- a/web_interface/auto_update.py +++ b/web_interface/auto_update.py @@ -263,6 +263,56 @@ def start_setup_if_needed(was_enabled, config): return 'Could not restart the display to finish automatic update setup; restart it from the Overview tab.' +#: Top-level folders of separately installed plugins. Edits there are not the +#: core's local changes: the Plugin Store updates plugins in place, including +#: the bundled ones committed under plugin-repos/, and the pull's --autostash +#: carries those edits across and reapplies them. +SEPARATE_INSTALL_DIRS = ('plugins', 'plugin-repos') + + +def local_changes(project_dir, run=None, timeout=30): + """Tracked files edited in the checkout, as both code updates count them. + + The one definition shared by the automatic update's preflight and + ``perform_core_update`` (Update Code), so the preflight's promise that + nothing will be stashed holds for the pull that follows it. + + * Mode-only changes do not count (``core.fileMode=false``): installers + chmod tracked scripts, and --autostash carries modes across the pull. + * Paths under ``SEPARATE_INSTALL_DIRS`` do not count. The match is on + the leading folder, not a substring, so + ``web_interface/static/v3/js/plugins/x.js`` is still a core edit. + + Returns the changed paths, or None when git could not tell. Raises what + ``run`` raises (e.g. ``subprocess.TimeoutExpired``). + """ + run = run or subprocess.run + result = run(['git', '-c', 'core.fileMode=false', 'status', '--porcelain', + '--untracked-files=no', '-z'], + cwd=str(project_dir), capture_output=True, text=True, timeout=timeout) + if result.returncode != 0: + return None + changed = [] + entries = iter((result.stdout or '').split('\0')) + for entry in entries: + if len(entry) < 4: + continue + paths = [entry[3:]] + if entry[0] in 'RC' or entry[1] in 'RC': + paths.append(next(entries, '')) # -z puts a rename's source next + if any(p and p.split('/', 1)[0] not in SEPARATE_INSTALL_DIRS for p in paths): + changed.append(paths[0]) + return changed + + +def describe_local_changes(changed): + """Why an automatic update refused to touch a checkout with local edits.""" + example = f' (for example {changed[0]})' if changed else '' + return (f'{len(changed or ()) or "Some"} tracked file(s) in the LEDMatrix folder were edited ' + f'locally{example}. Automatic updates will not stash your changes; ' + 'commit or revert them, or update manually with Update Code.') + + def _load_verifier(path): spec = importlib.util.spec_from_file_location('ledmatrix_auto_update_verifier', str(path)) module = importlib.util.module_from_spec(spec) @@ -346,11 +396,16 @@ class AutoUpdater: core = {'outcome': 'error', 'message': f'The LEDMatrix update failed unexpectedly: {e}'} deferred = core['outcome'] == 'verifying' - updated, failed = ([], []) if deferred else self._update_plugins() + # A core whose rollback failed is in an unknown state: as when the + # health check reports rollback_failed, plugins are left alone and + # nothing is restarted onto it. + stranded = core['outcome'] == 'rollback_failed' + updated, failed = ([], []) if deferred or stranded else self._update_plugins() self._store_run(state, core, updated, failed) logger.info("Automatic update: core %s (%s); plugins updated=%s failed=%s%s", core['outcome'], core['message'], updated, failed, - '; plugins wait for the health check' if deferred else '') + '; plugins wait for the health check' if deferred + else '; plugins left alone' if stranded else '') # Code restarts belong to the health check; this only covers plugins. if updated: @@ -424,17 +479,11 @@ class AutoUpdater: return 'blocked', ('The current branch has no upstream to update from (or HEAD is detached). ' 'Use Update Code once, or Tools -> Switch branch.'), {} - # Mode-only changes are ignored: older installers chmod tracked - # scripts, and --autostash in the pull carries those across. Plugin - # folders are separate installs, as in perform_core_update. - status = self._git('-c', 'core.fileMode=false', 'status', '--porcelain', '--untracked-files=no') - changed = [line[3:] for line in status.stdout.splitlines() - if line.strip() and 'plugins/' not in line and 'plugin-repos/' not in line] - if status.returncode != 0 or changed: - example = f' (for example {changed[0]})' if changed else '' - return 'blocked', (f'{len(changed) or "Some"} tracked file(s) in the LEDMatrix folder were edited ' - f'locally{example}. Automatic updates will not stash your changes; ' - 'commit or revert them, or update manually with Update Code.'), {} + # The same predicate perform_core_update refuses on when called from + # here, so what passes this check is never stashed by the pull. + changed = local_changes(self.project_root, run=self.run_command) + if changed is None or changed: + return 'blocked', describe_local_changes(changed), {} free = self.disk_free(str(self.project_root)) if free < MIN_FREE_BYTES: @@ -479,11 +528,15 @@ class AutoUpdater: return {'outcome': 'error', 'message': f'Could not prepare the update health check: {e}.'} try: - core = self.core_update() + # Refuse rather than stash: edits made since the preflight are the + # user's, and nothing would ever restore a stash taken here. + core = self.core_update(stash_local_changes=False) except Exception as e: logger.exception("perform_core_update raised") core = {'status': 'error', 'message': f'Update failed: {e}'} new_head = self._git('rev-parse', 'HEAD').stdout.strip() + if core.get('local_changes') is not None and new_head == old_head: + return {'outcome': 'blocked', 'message': describe_local_changes(core['local_changes'])} pending = { 'status': 'pending', 'old_head': old_head, diff --git a/web_interface/blueprints/api_v3/system.py b/web_interface/blueprints/api_v3/system.py index 2c5d712a..1e82b7fd 100644 --- a/web_interface/blueprints/api_v3/system.py +++ b/web_interface/blueprints/api_v3/system.py @@ -228,14 +228,24 @@ def _sudo_hint_for(text): _core_update_lock = threading.Lock() +#: The core's own requirement files, installed after a pull that changes them. +#: scripts/fix_perms/safe_pip_install.sh must accept every one (it refuses +#: anything it does not list), and scripts/utils/auto_update_verify.py +#: reinstalls the same files when it rolls an update back. +CORE_REQUIREMENT_FILES = ('requirements.txt', 'web_interface/requirements.txt') -def perform_core_update(): + +def perform_core_update(stash_local_changes=True): """Pull the latest LEDMatrix code and sync its dependencies. Shared by the Overview "Update Code" button and the weekly automatic updater (web_interface/auto_update.py), so both take exactly the same path. Returns the JSON-able payload the button has always received: ``status``, ``message`` and ``restart_required``. + + Update Code stashes local edits before pulling. The automatic updater + passes ``stash_local_changes=False``: then local edits make this return + an error carrying ``local_changes`` (the edited paths) without pulling. """ # The button and the scheduler can fire together; two pulls racing # over one checkout (and one stash) is how local changes get lost. @@ -243,12 +253,12 @@ def perform_core_update(): return {'status': 'error', 'restart_required': False, 'message': 'An update is already in progress; try again shortly.'} try: - return _perform_core_update_locked() + return _perform_core_update_locked(stash_local_changes) finally: _core_update_lock.release() -def _perform_core_update_locked(): +def _perform_core_update_locked(stash_local_changes=True): project_dir = str(PROJECT_ROOT) # Decide how to pull BEFORE stashing. If this checkout cannot be @@ -259,37 +269,35 @@ def _perform_core_update_locked(): logger.warning("git pull not attempted: %s", pull_error) return {'status': 'error', 'message': pull_error, 'restart_required': False} - # Check if there are local changes that need to be stashed - # Exclude plugins directory - plugins are separate repos and shouldn't be stashed with base project - # Use --untracked-files=no to skip untracked files check (much faster with symlinked plugins) + # Local changes, counted exactly as the automatic update's preflight + # counts them (auto_update.local_changes): mode-only changes and the + # plugin folders don't count, and the pull's --autostash carries those + # across and reapplies them. + from web_interface import auto_update try: - status_result = subprocess.run( - ['git', 'status', '--porcelain', '--untracked-files=no'], - capture_output=True, - text=True, - timeout=30, - cwd=project_dir - ) - # Filter out any changes in plugins directory - plugins are separate repositories - # Git status format: XY filename (where X is status of index, Y is status of work tree) - status_lines = [line for line in status_result.stdout.strip().split('\n') - if line.strip() and 'plugins/' not in line] - has_changes = bool('\n'.join(status_lines).strip()) - except subprocess.TimeoutExpired: - # If status check times out, assume there might be changes and proceed - # This is safer than failing the update - has_changes = True - status_result = type('obj', (object,), {'stdout': '', 'stderr': 'Status check timed out'})() + changed = auto_update.local_changes(project_dir) + except (subprocess.SubprocessError, OSError) as status_err: + logger.warning("git status failed before pull: %s", status_err) + changed = None + # When git cannot say, assume there are changes rather than pull over them. + has_changes = changed is None or bool(changed) + + if has_changes and not stash_local_changes: + # The automatic updater: it promised not to stash, and nothing would + # ever restore a stash taken on its behalf. + return {'status': 'error', 'restart_required': False, 'dependency_failures': [], + 'local_changes': list(changed or []), + 'message': auto_update.describe_local_changes(changed)} stash_info = "" - # Stash local changes if they exist (excluding plugins) - # Plugins are separate repositories and shouldn't be stashed with base project updates + # Stash local changes if they exist. The plugin folders are left out: + # plugins are separate installs, and --autostash carries their edits. if has_changes: try: - # Use pathspec to exclude plugins directory from stash stash_result = subprocess.run( - ['git', 'stash', 'push', '-m', 'LEDMatrix auto-stash before update', '--', ':!plugins'], + ['git', 'stash', 'push', '-m', 'LEDMatrix auto-stash before update', '--', + *(f':!{folder}' for folder in auto_update.SEPARATE_INSTALL_DIRS)], capture_output=True, text=True, timeout=30, @@ -369,7 +377,7 @@ def _perform_core_update_locked(): ['git', 'diff', '--name-only', f'{old_head}..{new_head}'], capture_output=True, text=True, timeout=15, cwd=project_dir) changed = set(diff.stdout.split()) if diff.returncode == 0 else set() - for rel in ('requirements.txt', 'web_interface/requirements.txt'): + for rel in CORE_REQUIREMENT_FILES: req_path = PROJECT_ROOT / rel if rel not in changed or not req_path.exists(): continue