Merge audit16-core/autoupdate into the September 16 core audit fixes

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:

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.

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
+for EXACT in "${ALLOWED_EXACT[@]}"; do
+    if [ "$RESOLVED_TARGET" = "$EXACT" ]; then
         ALLOWED=true
-else
+        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
 

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
+        # --hard: the updater refuses to run with local edits to tracked core
-        # tracked files, so the only thing this discards is the update.
+        # files (web_interface/auto_update.local_changes), so outside the
-        result = self._run(['git', 'reset', '--hard', old], timeout=120)
+        # 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')

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(
+            denied = any(phrase in result.stderr for phrase in SUDO_REFUSAL_PHRASES)
-                phrase in result.stderr
-                for phrase in ("a password is required", "is not allowed to run", "no tty present")
-            )
             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

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 --------------------------------------
 

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'

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

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
+        # The same predicate perform_core_update refuses on when called from
-        # scripts, and --autostash in the pull carries those across. Plugin
+        # here, so what passes this check is never stashed by the pull.
-        # folders are separate installs, as in perform_core_update.
+        changed = local_changes(self.project_root, run=self.run_command)
-        status = self._git('-c', 'core.fileMode=false', 'status', '--porcelain', '--untracked-files=no')
+        if changed is None or changed:
-        changed = [line[3:] for line in status.stdout.splitlines()
+            return 'blocked', describe_local_changes(changed), {}
-                   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.'), {}
 
         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,

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
+    # Local changes, counted exactly as the automatic update's preflight
-    # Exclude plugins directory - plugins are separate repos and shouldn't be stashed with base project
+    # counts them (auto_update.local_changes): mode-only changes and the
-    # Use --untracked-files=no to skip untracked files check (much faster with symlinked plugins)
+    # 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(
+        changed = auto_update.local_changes(project_dir)
-            ['git', 'status', '--porcelain', '--untracked-files=no'],
+    except (subprocess.SubprocessError, OSError) as status_err:
-            capture_output=True,
+        logger.warning("git status failed before pull: %s", status_err)
-            text=True,
+        changed = None
-            timeout=30,
+    # When git cannot say, assume there are changes rather than pull over them.
-            cwd=project_dir
+    has_changes = changed is None or bool(changed)
-        )
+
-        # Filter out any changes in plugins directory - plugins are separate repositories
+    if has_changes and not stash_local_changes:
-        # Git status format: XY filename (where X is status of index, Y is status of work tree)
+        # The automatic updater: it promised not to stash, and nothing would
-        status_lines = [line for line in status_result.stdout.strip().split('\n')
+        # ever restore a stash taken on its behalf.
-                       if line.strip() and 'plugins/' not in line]
+        return {'status': 'error', 'restart_required': False, 'dependency_failures': [],
-        has_changes = bool('\n'.join(status_lines).strip())
+                'local_changes': list(changed or []),
-    except subprocess.TimeoutExpired:
+                'message': auto_update.describe_local_changes(changed)}
-        # 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'})()
 
     stash_info = ""
 
-    # Stash local changes if they exist (excluding plugins)
+    # Stash local changes if they exist. The plugin folders are left out:
-    # Plugins are separate repositories and shouldn't be stashed with base project updates
+    # 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