From cac9644b6dfa12fef19753ff3d4ecf1fcf18f5fc Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 30 May 2026 14:56:20 -0400 Subject: [PATCH] fix(plugin-loader): auto-detect new dependencies via requirements.txt hash (#355) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(plugin-loader): detect new deps via requirements.txt hash instead of empty marker The .dependencies_installed marker was an empty file, so adding a new package to requirements.txt (e.g. astral in ledmatrix-weather v2.3.0) never triggered a pip re-install on existing installs — the file existed so the check returned early. The marker now stores a SHA-256 hash of requirements.txt. On every plugin load, the loader compares the current hash to the stored one; a mismatch (or missing marker) triggers pip install and writes the new hash. store_manager._install_dependencies() also writes the hash marker after a store install/update so the loader skips a redundant pip run on next boot. Co-Authored-By: Claude Sonnet 4.6 * fix(plugin-loader): address CodeQL path expression and I/O error handling - Add explicit relative_to() containment check after path resolution so CodeQL recognizes the plugin directory boundary (fixes 4 CodeQL alerts: Uncontrolled data used in path expression, lines 168/172/189/205) - Wrap requirements_file.read_bytes() in try/except OSError — on Raspberry Pi with flaky SD card storage this can fail; returns False with a clear log - Wrap marker_path.read_text() in try/except OSError — a corrupted marker falls through to a clean reinstall instead of crashing - Wrap both marker_path.write_text() calls in try/except OSError — pip already succeeded at this point so a marker write failure should not return False or propagate through the generic exception handler Co-Authored-By: Claude Sonnet 4.6 * fix(plugin-loader): use realpath+startswith containment check for CodeQL path-injection Replace relative_to() (not recognised by CodeQL as a path sanitiser) with the os.path.realpath() + startswith() pattern that CodeQL explicitly models as sanitising py/path-injection. - Add plugins_dir optional param to install_dependencies() and load_plugin() - PluginManager.load_plugin() passes self.plugins_dir as the trusted anchor; install_dependencies() validates that the resolved plugin_dir starts with the resolved plugins_dir before any file I/O - Replace all Path.read_bytes/read_text/write_text/exists with open() and os.path.isfile() so the sanitised string paths flow directly to file ops without re-introducing taint through Path object conversion Co-Authored-By: Claude Sonnet 4.6 * fix(plugin-loader): fail-fast when install_dependencies returns False Previously the boolean result was silently discarded, so a failed pip install would log a warning but continue attempting to import the plugin module — resulting in a confusing ModuleNotFoundError instead of a clear dependency failure message. Now raises PluginError with plugin_id and plugin_dir if dependency installation fails, stopping the load before the import is attempted. Co-Authored-By: Claude Sonnet 4.6 * fix(plugin-loader): use basename+reconstruct to satisfy CodeQL py/path-injection startswith() is a validation check in CodeQL's model, not a sanitiser — taint still flows through plugin_dir_real to the file operations. os.path.basename() IS in CodeQL's recognised sanitiser list: it strips all directory components so the result cannot contain traversal sequences. Reconstructing the plugin path from the trusted plugins_dir base joined with the basename-sanitised directory name produces a path CodeQL considers untainted, breaking the taint chain from the plugin_dir parameter. Co-Authored-By: Claude Sonnet 4.6 * fix(plugin-loader): guard against empty basename when plugin_dir resolves to fs root If plugin_dir somehow resolves to '/' or a bare drive root, os.path.basename() returns '', causing safe_plugin_dir to equal plugins_dir_real and the isdir() check to pass incorrectly. Reject early with a clear error in that case. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Chuck Co-authored-by: Claude Sonnet 4.6 --- src/plugin_system/plugin_loader.py | 117 +++++++++++++++++++++------- src/plugin_system/plugin_manager.py | 3 +- src/plugin_system/store_manager.py | 7 ++ 3 files changed, 96 insertions(+), 31 deletions(-) diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 75628bb0..49d2e404 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -5,6 +5,7 @@ Handles plugin module imports, dependency installation, and class instantiation. Extracted from PluginManager to improve separation of concerns. """ +import hashlib import json import importlib import importlib.util @@ -138,53 +139,98 @@ class PluginLoader: self, plugin_dir: Path, plugin_id: str, + plugins_dir: Optional[Path] = None, timeout: int = 300 ) -> bool: """ Install plugin dependencies from requirements.txt. - + Args: plugin_dir: Plugin directory path plugin_id: Plugin identifier + plugins_dir: Trusted base plugins directory for path containment check timeout: Installation timeout in seconds - + Returns: True if dependencies installed or not needed, False on error """ plugin_id = os.path.basename(plugin_id or '') if not plugin_id: return False - # Resolve and validate plugin_dir before constructing any derived paths - try: - plugin_dir_resolved = plugin_dir.resolve(strict=True) - except OSError: - self.logger.error("Plugin directory does not exist: %s", plugin_dir) - return False - requirements_file = plugin_dir_resolved / "requirements.txt" - if not requirements_file.exists(): - return True # No dependencies needed - marker_path = plugin_dir_resolved / ".dependencies_installed" - # Check if already installed - if marker_path.exists(): - self.logger.debug("Dependencies already installed for %s", plugin_id) - return True - + # Resolve to a canonical absolute path (normalises .. and symlinks) + plugin_dir_real = os.path.realpath(str(plugin_dir)) + + if plugins_dir is not None: + # Reconstruct the plugin path from a trusted base + a sanitised + # directory name. os.path.basename() is CodeQL's recognised + # py/path-injection sanitiser: it strips all directory components + # so the result cannot contain traversal sequences. Joining it + # with the resolved, trusted plugins_dir produces a path that + # CodeQL considers untainted. + plugins_dir_real = os.path.realpath(str(plugins_dir)) + safe_dir_name = os.path.basename(plugin_dir_real) + if not safe_dir_name: + self.logger.error("Could not determine plugin directory name for %s", plugin_id) + return False + safe_plugin_dir = os.path.join(plugins_dir_real, safe_dir_name) + if not os.path.isdir(safe_plugin_dir): + self.logger.error( + "Plugin directory for %s not found inside plugins dir", plugin_id + ) + return False + else: + safe_plugin_dir = plugin_dir_real + if not os.path.isdir(safe_plugin_dir): + self.logger.error("Plugin directory does not exist: %s", plugin_dir) + return False + + requirements_file = os.path.join(safe_plugin_dir, "requirements.txt") + marker_file = os.path.join(safe_plugin_dir, ".dependencies_installed") + + if not os.path.isfile(requirements_file): + return True # No dependencies needed + + try: + with open(requirements_file, 'rb') as fh: + current_hash = hashlib.sha256(fh.read()).hexdigest() + except OSError as e: + self.logger.error("Failed to read requirements.txt for %s: %s", plugin_id, e) + return False + + # Skip if requirements.txt hasn't changed since last install + if os.path.isfile(marker_file): + try: + with open(marker_file, 'r', encoding='utf-8') as fh: + stored_hash = fh.read().strip() + except OSError as e: + self.logger.warning( + "Could not read dependency marker for %s (%s), will reinstall dependencies", + plugin_id, e + ) + else: + if stored_hash == current_hash: + self.logger.debug("Dependencies already installed for %s (requirements unchanged)", plugin_id) + return True + self.logger.info("Requirements changed for %s, reinstalling dependencies", plugin_id) + try: self.logger.info("Installing dependencies for plugin %s...", plugin_id) result = subprocess.run( - [sys.executable, "-m", "pip", "install", "--break-system-packages", "-r", str(requirements_file)], + [sys.executable, "-m", "pip", "install", "--break-system-packages", "-r", requirements_file], capture_output=True, text=True, timeout=timeout, check=False ) - + if result.returncode == 0: - # Mark as installed - marker_path.touch() - # Set proper file permissions after creating marker - ensure_file_permissions(marker_path, get_plugin_file_mode()) + try: + with open(marker_file, 'w', encoding='utf-8') as fh: + fh.write(current_hash) + ensure_file_permissions(Path(marker_file), get_plugin_file_mode()) + except OSError as marker_err: + self.logger.debug("Could not write dependency marker for %s: %s", plugin_id, marker_err) self.logger.info("Dependencies installed successfully for %s", plugin_id) return True else: @@ -199,8 +245,12 @@ class PluginLoader: "Assuming they are satisfied: %s", plugin_id, stderr.strip() ) - marker_path.touch() - ensure_file_permissions(marker_path, get_plugin_file_mode()) + try: + with open(marker_file, 'w', encoding='utf-8') as fh: + fh.write(current_hash) + ensure_file_permissions(Path(marker_file), get_plugin_file_mode()) + except OSError as marker_err: + self.logger.debug("Could not write dependency marker for %s: %s", plugin_id, marker_err) return True self.logger.warning( "Dependency installation returned non-zero exit code for %s: %s", @@ -543,11 +593,12 @@ class PluginLoader: display_manager: Any, cache_manager: Any, plugin_manager: Any, - install_deps: bool = True + install_deps: bool = True, + plugins_dir: Optional[Path] = None, ) -> Tuple[Any, Any]: """ Complete plugin loading process. - + Args: plugin_id: Plugin identifier manifest: Plugin manifest @@ -557,16 +608,22 @@ class PluginLoader: cache_manager: Cache manager instance plugin_manager: Plugin manager instance install_deps: Whether to install dependencies - + plugins_dir: Trusted base plugins directory forwarded to install_dependencies + Returns: Tuple of (plugin_instance, module) - + Raises: PluginError: If loading fails """ # Install dependencies if needed if install_deps: - self.install_dependencies(plugin_dir, plugin_id) + if not self.install_dependencies(plugin_dir, plugin_id, plugins_dir=plugins_dir): + raise PluginError( + f"Dependency installation failed for plugin {plugin_id} in {plugin_dir}", + plugin_id=plugin_id, + context={'plugin_dir': str(plugin_dir)}, + ) # Load module entry_point = manifest.get('entry_point', 'manager.py') diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 18ed6b08..e9eb4761 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -350,7 +350,8 @@ class PluginManager: display_manager=self.display_manager, cache_manager=self.cache_manager, plugin_manager=self, - install_deps=True + install_deps=True, + plugins_dir=self.plugins_dir, ) # Store module diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 5a774056..2f2140cf 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -5,6 +5,7 @@ Handles plugin discovery, installation, updates, and uninstallation from both the official registry and custom GitHub repositories. """ +import hashlib import os import json import stat @@ -1755,6 +1756,12 @@ class PluginStoreManager: timeout=300 ) self.logger.info(f"Dependencies installed successfully for {plugin_path.name}") + # Write hash marker so plugin_loader skips redundant pip run on next startup + try: + current_hash = hashlib.sha256(requirements_file.read_bytes()).hexdigest() + (plugin_path / ".dependencies_installed").write_text(current_hash, encoding='utf-8') + except OSError as marker_err: + self.logger.debug("Could not write dependency marker for %s: %s", plugin_path.name, marker_err) return True except subprocess.CalledProcessError as e: