diff --git a/.gitignore b/.gitignore index 99921d3a..8689da04 100644 --- a/.gitignore +++ b/.gitignore @@ -8,6 +8,7 @@ config/config_secrets.json config/config.json config/config.json.backup config/wifi_config.json +config/uninstalled_plugins.json credentials.json token.pickle diff --git a/src/plugin_system/state_reconciliation.py b/src/plugin_system/state_reconciliation.py index 8cc70e4d..afbd2e1d 100644 --- a/src/plugin_system/state_reconciliation.py +++ b/src/plugin_system/state_reconciliation.py @@ -322,10 +322,19 @@ class StateReconciliation: and hasattr(self.store_manager, 'was_recently_uninstalled') and self.store_manager.was_recently_uninstalled(plugin_id) ) + # Also refuse to resurrect a plugin the user has persistently + # uninstalled. Unlike the in-memory race guard above, this record + # survives restarts, so the user's removal sticks across updates. + persistently_uninstalled = ( + self.store_manager is not None + and hasattr(self.store_manager, 'is_plugin_uninstalled') + and self.store_manager.is_plugin_uninstalled(plugin_id) + ) can_repair = ( self.store_manager is not None and not previously_unrecoverable and not recently_uninstalled + and not persistently_uninstalled ) inconsistencies.append(Inconsistency( plugin_id=plugin_id, diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 2f2140cf..e698b453 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -7,6 +7,7 @@ from both the official registry and custom GitHub repositories. import hashlib import os +import re import json import stat import subprocess @@ -19,7 +20,7 @@ import time from concurrent.futures import ThreadPoolExecutor from datetime import datetime from pathlib import Path -from typing import List, Dict, Optional, Any, Tuple +from typing import List, Dict, Optional, Any, Tuple, Set import logging from urllib.parse import urlparse @@ -43,13 +44,24 @@ class PluginStoreManager: """ REGISTRY_URL = "https://raw.githubusercontent.com/ChuckBuilds/ledmatrix-plugins/main/plugins.json" + + # A valid plugin id is a single path component: starts alphanumeric, then + # alphanumerics / dot / dash / underscore. Used to keep the uninstall + # registry from ever turning a corrupt or hand-edited entry (e.g. "", + # "..", "../x") into a filesystem path that purge_uninstalled_plugins + # would delete — an empty id resolves to the plugins root itself. + _PLUGIN_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$") - def __init__(self, plugins_dir: str = "plugins"): + def __init__(self, plugins_dir: str = "plugins", + uninstalled_registry_path: Optional[str] = None): """ Initialize the plugin store manager. Args: plugins_dir: Directory where plugins are installed + uninstalled_registry_path: Path to the JSON file recording plugins + the user has uninstalled. Defaults to + ``config/uninstalled_plugins.json`` under the project root. """ self.plugins_dir = Path(plugins_dir) self.logger = logging.getLogger(__name__) @@ -84,6 +96,25 @@ class PluginStoreManager: self._uninstall_tombstones: Dict[str, float] = {} self._uninstall_tombstone_ttl = 300 # 5 minutes + # Persistent record of plugins the user has uninstalled. Unlike the + # in-memory tombstones above (a short-lived race guard), this survives + # restarts so that a core ``git pull`` update cannot resurrect a + # built-in plugin the user removed. Built-in plugins (e.g. + # ``web-ui-info``, ``starlark-apps``) are committed into the repo under + # ``plugin-repos/``, so a plain ``git pull`` restores their files even + # after the user deleted them. ``purge_uninstalled_plugins`` re-removes + # any such resurrected directory; ``install_plugin`` clears the record + # when the user deliberately reinstalls. The file is gitignored. + if uninstalled_registry_path is not None: + self._uninstalled_registry_path = Path(uninstalled_registry_path) + else: + self._uninstalled_registry_path = ( + Path(__file__).parent.parent.parent / "config" / "uninstalled_plugins.json" + ) + # Serializes read-modify-write of the registry file so concurrent + # install/uninstall requests can't lose updates. + self._uninstalled_registry_lock = threading.Lock() + # Cache for _get_local_git_info: {plugin_path_str: (signature, data)} # where ``signature`` is a tuple of (head_mtime, resolved_ref_mtime, # head_contents) so a fast-forward update to the current branch @@ -143,6 +174,135 @@ class PluginStoreManager: return False return True + def _is_valid_plugin_id(self, plugin_id: Any) -> bool: + """Return True if ``plugin_id`` is a safe single-component plugin id. + + Rejects empty strings, anything with a path separator, and traversal + sequences like ``..`` so a registry entry can never escape (or target + the root of) ``self.plugins_dir`` during a purge. + """ + return isinstance(plugin_id, str) and bool(self._PLUGIN_ID_RE.match(plugin_id)) + + def _read_uninstalled_registry(self) -> Set[str]: + """Read the persistent set of uninstalled plugin IDs. + + Returns an empty set if the file is missing, unreadable, or corrupt — + a broken registry must never block normal plugin operations. Invalid + ids are dropped here so callers never turn them into paths. + """ + try: + if not self._uninstalled_registry_path.exists(): + return set() + with open(self._uninstalled_registry_path, 'r', encoding='utf-8') as f: + data = json.load(f) + if not isinstance(data, list): + self.logger.warning( + "Uninstalled-plugin registry at %s is not a list; ignoring it", + self._uninstalled_registry_path, + ) + return set() + valid: Set[str] = set() + for pid in data: + if self._is_valid_plugin_id(pid): + valid.add(pid) + else: + self.logger.warning( + "Ignoring invalid plugin id in uninstall registry: %r", pid + ) + return valid + except (OSError, ValueError) as e: + self.logger.warning( + "Could not read uninstalled-plugin registry at %s: %s", + self._uninstalled_registry_path, e, + ) + return set() + + def _write_uninstalled_registry(self, plugin_ids: Set[str]) -> None: + """Persist the set of uninstalled plugin IDs (sorted, atomically).""" + path = self._uninstalled_registry_path + try: + path.parent.mkdir(parents=True, exist_ok=True) + tmp_path = path.with_suffix(path.suffix + ".tmp") + with open(tmp_path, 'w', encoding='utf-8') as f: + json.dump(sorted(plugin_ids), f, indent=2) + os.replace(tmp_path, path) + except OSError as e: + self.logger.error( + "Failed to write uninstalled-plugin registry at %s: %s", path, e + ) + + def record_uninstalled_plugin(self, plugin_id: str) -> None: + """Persistently record that the user uninstalled ``plugin_id``. + + Survives restarts so a core update cannot resurrect the plugin. + """ + if not self._is_valid_plugin_id(plugin_id): + self.logger.error("Refusing to record invalid plugin id: %r", plugin_id) + return + with self._uninstalled_registry_lock: + recorded = self._read_uninstalled_registry() + if plugin_id not in recorded: + recorded.add(plugin_id) + self._write_uninstalled_registry(recorded) + self.logger.info("Recorded %s as uninstalled (persistent)", plugin_id) + + def forget_uninstalled_plugin(self, *plugin_ids: str) -> None: + """Drop ``plugin_ids`` from the persistent uninstall registry. + + Called when a plugin is deliberately (re)installed so future updates + keep it. + """ + with self._uninstalled_registry_lock: + recorded = self._read_uninstalled_registry() + to_remove = {pid for pid in plugin_ids if pid in recorded} + if to_remove: + self._write_uninstalled_registry(recorded - to_remove) + self.logger.info( + "Cleared uninstall record for %s", ", ".join(sorted(to_remove)) + ) + + def get_uninstalled_plugins(self) -> Set[str]: + """Return the persistent set of user-uninstalled plugin IDs.""" + return self._read_uninstalled_registry() + + def is_plugin_uninstalled(self, plugin_id: str) -> bool: + """Return True if ``plugin_id`` is in the persistent uninstall registry.""" + return plugin_id in self._read_uninstalled_registry() + + def purge_uninstalled_plugins(self) -> List[str]: + """Remove on-disk directories for plugins the user has uninstalled. + + Built-in plugins committed into the repo are restored on disk by a + core ``git pull``; this re-removes any that the user previously + uninstalled. The registry entries are kept so the purge is idempotent + across every future update (until the user reinstalls). Returns the + list of plugin IDs whose directories were actually removed. + """ + removed: List[str] = [] + plugins_root = self.plugins_dir.resolve() + for plugin_id in sorted(self._read_uninstalled_registry()): + plugin_path = self.plugins_dir / plugin_id + # Defense in depth: ids are already validated on read, but never + # remove anything that isn't a direct child of the plugins root. + resolved = plugin_path.resolve() + if resolved == plugins_root or resolved.parent != plugins_root: + self.logger.error( + "Refusing to purge unsafe plugin path for id %r", plugin_id + ) + continue + if not plugin_path.exists(): + continue + self.logger.info( + "Purging resurrected uninstalled plugin: %s", plugin_id + ) + if self._safe_remove_directory(plugin_path): + removed.append(plugin_id) + else: + self.logger.error( + "Failed to purge resurrected plugin directory: %s", plugin_path + ) + return removed + def _load_github_token(self) -> Optional[str]: """ Load GitHub API token from config_secrets.json if available. @@ -1024,6 +1184,10 @@ class PluginStoreManager: branch_info = f" (branch: {branch})" if branch else " (latest branch head)" self.logger.info(f"Installing plugin: {plugin_id}{branch_info}") + # Remember the originally-requested id so we can clear its uninstall + # record on success even if the manifest renames the directory below. + requested_id = plugin_id + plugin_info = self.get_plugin_info(plugin_id, fetch_latest_from_github=True, force_refresh=True) if not plugin_info: self.logger.error(f"Plugin not found in registry: {plugin_id}") @@ -1162,6 +1326,9 @@ class PluginStoreManager: branch_display = branch_used or plugin_info.get('branch') or plugin_info.get('default_branch', 'unknown') self.logger.info(f"Successfully installed plugin: {plugin_id} (branch {branch_display})") + # User deliberately (re)installed this plugin — clear any persistent + # uninstall record so future core updates keep it. + self.forget_uninstalled_plugin(requested_id, plugin_id) return True except Exception as e: diff --git a/test/test_store_manager_caches.py b/test/test_store_manager_caches.py index 43e81cd9..c3cf190f 100644 --- a/test/test_store_manager_caches.py +++ b/test/test_store_manager_caches.py @@ -43,6 +43,115 @@ class TestUninstallTombstone(unittest.TestCase): self.assertNotIn("foo", self.sm._uninstall_tombstones) +class TestPersistentUninstallRegistry(unittest.TestCase): + """Regression tests for the persistent uninstall registry that stops a + core `git pull` update from resurrecting built-in plugins the user + removed (plugins committed under plugin-repos/).""" + + def setUp(self): + self._tmp = TemporaryDirectory() + self.addCleanup(self._tmp.cleanup) + self.plugins_dir = Path(self._tmp.name) / "plugin-repos" + self.plugins_dir.mkdir() + self.registry_path = Path(self._tmp.name) / "config" / "uninstalled_plugins.json" + self.sm = PluginStoreManager( + plugins_dir=str(self.plugins_dir), + uninstalled_registry_path=str(self.registry_path), + ) + + def _make_plugin_dir(self, plugin_id): + """Simulate a built-in plugin restored on disk (e.g. by git pull).""" + d = self.plugins_dir / plugin_id + d.mkdir(parents=True) + (d / "manifest.json").write_text('{"id": "%s"}' % plugin_id) + return d + + def test_unrecorded_plugin_is_not_uninstalled(self): + self.assertFalse(self.sm.is_plugin_uninstalled("web-ui-info")) + self.assertEqual(self.sm.get_uninstalled_plugins(), set()) + + def test_record_persists_across_instances(self): + self.sm.record_uninstalled_plugin("web-ui-info") + self.assertTrue(self.registry_path.exists()) + # A fresh manager (simulating a service restart after update) still sees it. + fresh = PluginStoreManager( + plugins_dir=str(self.plugins_dir), + uninstalled_registry_path=str(self.registry_path), + ) + self.assertTrue(fresh.is_plugin_uninstalled("web-ui-info")) + + def test_forget_clears_record(self): + self.sm.record_uninstalled_plugin("web-ui-info") + self.sm.forget_uninstalled_plugin("web-ui-info") + self.assertFalse(self.sm.is_plugin_uninstalled("web-ui-info")) + + def test_purge_removes_resurrected_plugin(self): + # The bug: user removed web-ui-info, then a git pull restored its + # committed files. Recorded uninstall + purge must re-remove it. + self._make_plugin_dir("web-ui-info") + self.sm.record_uninstalled_plugin("web-ui-info") + self.assertTrue((self.plugins_dir / "web-ui-info").exists()) + + removed = self.sm.purge_uninstalled_plugins() + + self.assertEqual(removed, ["web-ui-info"]) + self.assertFalse((self.plugins_dir / "web-ui-info").exists()) + # Record is kept so the purge stays idempotent across future updates. + self.assertTrue(self.sm.is_plugin_uninstalled("web-ui-info")) + + def test_purge_leaves_non_uninstalled_plugins_alone(self): + self._make_plugin_dir("baseball-scoreboard") # present, not recorded + self._make_plugin_dir("web-ui-info") + self.sm.record_uninstalled_plugin("web-ui-info") + + self.sm.purge_uninstalled_plugins() + + self.assertTrue((self.plugins_dir / "baseball-scoreboard").exists()) + self.assertFalse((self.plugins_dir / "web-ui-info").exists()) + + def test_purge_noop_when_plugin_absent(self): + # Recorded but never restored on disk — nothing to remove. + self.sm.record_uninstalled_plugin("web-ui-info") + self.assertEqual(self.sm.purge_uninstalled_plugins(), []) + + def test_corrupt_registry_is_ignored(self): + self.registry_path.parent.mkdir(parents=True, exist_ok=True) + self.registry_path.write_text("{ not valid json") + self.assertEqual(self.sm.get_uninstalled_plugins(), set()) + self.assertFalse(self.sm.is_plugin_uninstalled("web-ui-info")) + + def _write_raw_registry(self, value): + self.registry_path.parent.mkdir(parents=True, exist_ok=True) + import json as _json + self.registry_path.write_text(_json.dumps(value)) + + def test_empty_id_does_not_wipe_plugins_root(self): + # An empty id resolves to plugins_dir itself; purge must never delete it. + self._make_plugin_dir("baseball-scoreboard") + self._write_raw_registry([""]) + + removed = self.sm.purge_uninstalled_plugins() + + self.assertEqual(removed, []) + self.assertTrue(self.plugins_dir.exists()) + self.assertTrue((self.plugins_dir / "baseball-scoreboard").exists()) + # Invalid id is filtered out entirely. + self.assertEqual(self.sm.get_uninstalled_plugins(), set()) + + def test_traversal_ids_are_ignored(self): + for bad in ["..", "../evil", "a/b", "."]: + with self.subTest(bad=bad): + self.assertFalse(self.sm._is_valid_plugin_id(bad)) + self._write_raw_registry(["../evil", "..", "web-ui-info"]) + # Only the safe id survives the read. + self.assertEqual(self.sm.get_uninstalled_plugins(), {"web-ui-info"}) + + def test_record_rejects_invalid_id(self): + self.sm.record_uninstalled_plugin("") + self.sm.record_uninstalled_plugin("../escape") + self.assertEqual(self.sm.get_uninstalled_plugins(), set()) + + class TestGitInfoCache(unittest.TestCase): def setUp(self): self._tmp = TemporaryDirectory() diff --git a/web_interface/app.py b/web_interface/app.py index d4d4c20f..b1b3416a 100644 --- a/web_interface/app.py +++ b/web_interface/app.py @@ -79,6 +79,21 @@ plugin_manager = PluginManager( cache_manager=None # Not needed for web interface ) plugin_store_manager = PluginStoreManager(plugins_dir=str(plugins_dir)) +# A core `git pull` update (or any checkout) restores built-in plugins +# committed under plugin-repos/, even ones the user uninstalled. Re-remove any +# the user previously uninstalled at startup so a manual update on the Pi +# doesn't resurrect them. +try: + _purged = plugin_store_manager.purge_uninstalled_plugins() + if _purged: + logging.getLogger(__name__).info( + "Re-removed %d uninstalled plugin(s) restored since last run: %s", + len(_purged), ", ".join(_purged), + ) +except (OSError, RuntimeError) as _purge_err: + logging.getLogger(__name__).warning( + "Startup plugin purge failed: %s", _purge_err + ) saved_repositories_manager = SavedRepositoriesManager() # Initialize schema manager diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 4c954b00..fd656fca 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -1559,6 +1559,20 @@ def execute_system_action(): pull_message = f"Code updated successfully. Local changes were automatically stashed.{stash_info}" if result.stdout and "Already up to date" not in result.stdout: pull_message = f"Code updated successfully.{stash_info}" + # A `git pull` restores built-in plugins (committed under + # plugin-repos/) even if the user uninstalled them. Re-remove + # any the user previously uninstalled so the update doesn't + # resurrect them. + if api_v3.plugin_store_manager: + try: + purged = api_v3.plugin_store_manager.purge_uninstalled_plugins() + if purged: + logger.info( + "Re-removed %d uninstalled plugin(s) restored by update: %s", + len(purged), ", ".join(purged), + ) + except (OSError, RuntimeError) as purge_err: + logger.warning("Post-update plugin purge failed: %s", purge_err) else: logger.warning("git pull failed (returncode=%d): %s", result.returncode, result.stderr) pull_message = "Update failed; check logs for details" @@ -2933,6 +2947,13 @@ def _do_transactional_uninstall(plugin_id, preserve_config): api_v3.schema_manager.invalidate_cache(plugin_id) if api_v3.plugin_state_manager: api_v3.plugin_state_manager.remove_plugin_state(plugin_id) + # Persistently record the uninstall so a later core `git pull` update + # cannot resurrect a built-in plugin (committed under plugin-repos/) that + # the user removed. Best-effort: never fail the uninstall over this. + try: + api_v3.plugin_store_manager.record_uninstalled_plugin(plugin_id) + except Exception as record_err: + logger.warning("Could not record uninstall for %s: %s", plugin_id, record_err) return True, None