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: