diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index eb3ba659..951eb63d 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -207,7 +207,7 @@ class PluginLoader: self, plugin_dir: Path, plugin_id: str, - plugins_dir: Optional[Path] = None, + plugins_dir: Path, timeout: int = 300 ) -> bool: """ @@ -216,7 +216,12 @@ class PluginLoader: Args: plugin_dir: Plugin directory path plugin_id: Plugin identifier - plugins_dir: Trusted base plugins directory for path containment check + plugins_dir: Trusted base plugins directory for path containment check. + Required (not optional) so every caller reconstructs the plugin + path through the sanitiser below rather than trusting plugin_dir + directly -- CodeQL's path-injection query (and a malicious + manifest/plugin_id in practice) can't tell a legitimate + plugin_dir from one crafted to traverse outside plugins_dir. timeout: Installation timeout in seconds Returns: @@ -229,29 +234,23 @@ class PluginLoader: # 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 + # 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 requirements_file = os.path.join(safe_plugin_dir, "requirements.txt") @@ -698,6 +697,14 @@ class PluginLoader: """ # Install dependencies if needed if install_deps: + if plugins_dir is None: + raise PluginError( + f"plugins_dir is required to install dependencies for plugin {plugin_id} " + "(needed for path containment; pass install_deps=False if the caller " + "doesn't have a trusted plugins directory to supply)", + plugin_id=plugin_id, + context={'plugin_dir': str(plugin_dir)}, + ) 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}", diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 2f76151d..863afa76 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -1900,15 +1900,35 @@ class PluginStoreManager: def _install_dependencies(self, plugin_path: Path) -> bool: """ Install Python dependencies from requirements.txt. - + Args: plugin_path: Path to plugin directory - + Returns: True if successful or no requirements file """ - requirements_file = plugin_path / "requirements.txt" - + # Reconstruct the plugin path from the trusted self.plugins_dir base + + # a sanitised directory name rather than trusting plugin_path directly + # -- callers ultimately derive it from a plugin-supplied manifest "id" + # field (see install_plugin_from_url), so without this a malicious + # manifest could point requirements_file outside plugins_dir. + # os.path.basename() is CodeQL's recognised py/path-injection + # sanitiser: it strips all directory components so the result cannot + # contain traversal sequences, matching the pattern already used in + # PluginLoader.install_dependencies(). + plugin_dir_real = os.path.realpath(str(plugin_path)) + plugins_dir_real = os.path.realpath(str(self.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 dependency install") + return False + safe_plugin_path = Path(os.path.join(plugins_dir_real, safe_dir_name)) + if not safe_plugin_path.is_dir(): + self.logger.error("Plugin directory not found inside plugins dir: %s", safe_plugin_path) + return False + + requirements_file = safe_plugin_path / "requirements.txt" + if not requirements_file.exists(): self.logger.debug(f"No requirements.txt found in {plugin_path.name}") return True diff --git a/test/test_plugin_loader.py b/test/test_plugin_loader.py index 6e1edad6..efc42833 100644 --- a/test/test_plugin_loader.py +++ b/test/test_plugin_loader.py @@ -193,7 +193,7 @@ class TestPluginLoader: mock_subprocess.return_value = MagicMock(returncode=0) - result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") + result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir) assert result is True mock_subprocess.assert_called_once() @@ -204,7 +204,7 @@ class TestPluginLoader: plugin_dir = tmp_plugins_dir / "test_plugin" plugin_dir.mkdir() - result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") + result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir) assert result is True mock_subprocess.assert_not_called() @@ -219,7 +219,7 @@ class TestPluginLoader: mock_subprocess.return_value = MagicMock(returncode=1) - result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") + result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir) assert result is False @@ -245,7 +245,7 @@ class TestPluginLoader: retry_attempt = MagicMock(returncode=0, stderr="") mock_subprocess.side_effect = [first_attempt, retry_attempt] - result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") + result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir) assert result is True assert mock_subprocess.call_count == 2 @@ -271,7 +271,7 @@ class TestPluginLoader: retry_attempt = MagicMock(returncode=1, stderr="some other pip error") mock_subprocess.side_effect = [first_attempt, retry_attempt] - result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") + result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir) assert result is True assert mock_subprocess.call_count == 2 @@ -298,7 +298,7 @@ class TestPluginLoader: subprocess.TimeoutExpired(cmd="pip", timeout=300), ] - result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") + result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir) assert result is True assert mock_subprocess.call_count == 2 @@ -311,7 +311,34 @@ class TestPluginLoader: requirements_file = plugin_dir / "requirements.txt" requirements_file.write_text("pytest>=1.0\n") - result = plugin_loader.install_dependencies(plugin_dir, "test_plugin") + result = plugin_loader.install_dependencies(plugin_dir, "test_plugin", plugins_dir=tmp_plugins_dir) assert result is True mock_subprocess.assert_not_called() + + def test_install_dependencies_requires_plugins_dir(self, plugin_loader, tmp_plugins_dir): + """plugins_dir is a required argument, not an optional trust-me flag -- + calling without it must fail loudly (TypeError) rather than silently + falling back to trusting plugin_dir unchecked.""" + plugin_dir = tmp_plugins_dir / "test_plugin" + plugin_dir.mkdir() + + with pytest.raises(TypeError): + plugin_loader.install_dependencies(plugin_dir, "test_plugin") + + @patch('subprocess.run') + def test_install_dependencies_rejects_path_outside_plugins_dir( + self, mock_subprocess, plugin_loader, tmp_path, tmp_plugins_dir + ): + """A plugin_dir that doesn't actually live inside plugins_dir (e.g. a + manifest-derived id crafted to traverse elsewhere) must be rejected + rather than read from -- this is the path-injection containment + check CodeQL flagged as missing.""" + outside_dir = tmp_path / "outside" + outside_dir.mkdir() + (outside_dir / "requirements.txt").write_text("requests>=2.0\n") + + result = plugin_loader.install_dependencies(outside_dir, "evil_plugin", plugins_dir=tmp_plugins_dir) + + assert result is False + mock_subprocess.assert_not_called()