fix(security): close path-injection gap in dependency-satisfaction checks

CodeQL flagged 2 new high-severity "uncontrolled data used in path
expression" alerts at the open() calls inside this PR's new
requirements_has_real_deps()/requirements_are_satisfied() -- both are
reachable from paths that were never run through the basename+trusted-base
sanitiser this codebase already uses elsewhere:

- PluginLoader.install_dependencies() only applied that sanitiser when its
  optional plugins_dir argument was actually passed; the "no plugins_dir"
  branch trusted plugin_dir_real directly. Made plugins_dir required (not
  Optional) so that branch can't exist, and added an explicit guard in
  load_plugin() so install_deps=True without a plugins_dir fails loudly
  instead of silently. Production's only real caller (PluginManager) always
  passes plugins_dir already; the harness/dev-server/render-plugin callers
  all use install_deps=False and are unaffected.

- StoreManager._install_dependencies() never sanitised plugin_path at all,
  and its call sites ultimately derive that path from a plugin's own
  manifest.json "id" field (install_plugin_from_url) -- a malicious plugin
  could otherwise point requirements_file outside plugins_dir. Applied the
  same os.path.basename()-based containment pattern PluginLoader already
  uses (and that CodeQL recognises as a real sanitiser).

Added test_install_dependencies_requires_plugins_dir and
test_install_dependencies_rejects_path_outside_plugins_dir to lock in the
actual security property, not just quiet the scanner. Verified: all 20
tests in test_plugin_loader.py pass, plus the PR's existing test plan
(test_plugin_system.py, test_store_manager_caches.py: 53 passed) and the
full CI plugin-safety suite (test_harness.py, test_visual_rendering.py,
test_plugin_matrix.py: 52 passed, 2 pre-existing skips) all still pass.
This commit is contained in:
ChuckBuilds
2026-07-10 15:10:49 -04:00
parent d86dc5914b
commit d59a66133b
3 changed files with 90 additions and 36 deletions
+34 -7
View File
@@ -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()