mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-01 16:58:06 +00:00
* fix(plugins): replace dependency marker files with a real satisfaction check The .dependencies_installed hash-marker system only tracked "was this exact requirements.txt hashed before" — not whether the packages it names are actually present. That made it fragile (a wiped venv, a manually removed package, or a lost/corrupted marker forces a needless full pip reinstall or, worse, a false skip) and produced dead weight for the ~10 plugins whose requirements.txt is comment-only (they still paid a pip subprocess on first boot before a marker existed). Replace it with requirements_are_satisfied() in plugin_loader.py, which checks each real requirement line against importlib.metadata directly, so install_dependencies() only shells out to pip when something is actually missing or version-mismatched. Drops the marker file entirely: removed all marker read/write sites in plugin_loader.py and store_manager.py, the now-pointless marker-cleanup step in the git-update path, the unused legacy marker implementation in plugin_manager.py, and the already-stale clear_dependency_markers.sh script. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * 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. * fix(security): replace basename-only sanitiser with a trusted-enumeration check The previous commit's os.path.basename() + os.path.join() pattern (which a pre-existing code comment claimed CodeQL recognises as a sanitiser) did not actually clear the alert -- the next CodeQL run still flagged the same 2 sink lines, plus a new one at the os.path.join() call itself. Taking a substring of tainted data apparently isn't treated as a barrier by this query, whatever the comment assumed. Replaced it with find_trusted_subdir(): enumerate the trusted plugins_dir via os.scandir() and only use a name that scandir itself produced, matched by equality against the caller's requested name. The path is then built from that enumerated entry, not from the caller's string -- a value sourced from iterating a trusted, non-tainted directory carries no taint regardless of what it happens to equal, which is a stronger and more conventional allowlist-style barrier than string-stripping. Applied identically in both PluginLoader.install_dependencies() and StoreManager._install_dependencies(), sharing one implementation. Re-verified: all 65 tests across test_plugin_loader.py (20, including the 2 new security regression tests), test_store_manager_caches.py (35), test_plugin_system.py (10) pass, plus the full CI plugin-safety suite (test_harness.py/test_visual_rendering.py/test_plugin_matrix.py: 52 passed, 2 pre-existing skips). * fix(security): redact URL credentials from pip subprocess output before logging CodeQL flagged 3 clear-text-logging-of-secrets alerts in install_requirements_file() (src/common/permission_utils.py:353,360,371). Pre-existing on main, unrelated to this PR's own diff, but now visible since the path-injection alerts that previously took priority in the annotation list are fixed. The underlying risk is real: pip can echo a private index URL's embedded basic-auth credentials (from a requirements.txt --index-url line or PIP_INDEX_URL) back verbatim in its own stderr/stdout on failure, and this function both logs that output directly and returns it to callers -- store_manager.py's _install_dependencies() logs result.stderr from this same function too. Added _redact_url_credentials(), applied immediately after each of the two subprocess.run() calls (mutating result.stderr/stdout in place) rather than patching each log call site individually. This closes the leak at the source: every downstream use -- the three flagged log lines, the "note" string embedded in the returned stdout, and store_manager.py's own logging of the returned result -- gets the redacted text for free. Verified the fixed-phrase "denied" check (`"a password is required" in result.stderr`) is unaffected, since URL syntax and those phrases don't overlap -- covered explicitly by test_does_not_touch_denied_check_phrases. Added test/test_permission_utils.py (6 tests) covering the redaction helper directly and both subprocess.run() call sites (the sudo-wrapper branch, which this repo's scripts/fix_perms/safe_pip_install.sh makes live, and the no-wrapper fallback branch). All pass. * fix(security): stop interpolating req_file/pip-output into log calls The previous commit's redaction (mutating result.stderr/stdout right after each subprocess.run()) didn't clear CodeQL's clear-text-logging alerts -- same lesson as the path-injection fix earlier in this PR: a static analyzer can't tell "this value was already sanitised two lines up" from "this is still the raw tainted value" just by looking at a single log call in isolation, so it conservatively keeps flagging it regardless of what the redaction function actually does. Removed all dynamic interpolation (req_file, result.stderr) from the 3 flagged logger.warning() calls entirely, replacing them with fixed messages plus (for the one that had it) result.returncode, which is a plain int with no possible taint. The full redacted detail is still available where it actually matters -- in the returned CompletedProcess.stderr/stdout and the "note" text -- just not duplicated into a log line a scanner has to reason about in isolation. Re-verified: all 6 test_permission_utils.py tests still pass (they assert on the returned result, not log call arguments), plus the full test_plugin_loader.py/test_store_manager_caches.py/test_plugin_system.py suite (71 passed, 1 pre-existing deselect, 4 subtests). --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
247 lines
11 KiB
Python
247 lines
11 KiB
Python
import time
|
|
from unittest.mock import MagicMock, patch
|
|
from pathlib import Path
|
|
from src.plugin_system.plugin_manager import PluginManager
|
|
from src.plugin_system.plugin_state import PluginState
|
|
from src.plugin_system.resource_monitor import PluginResourceMonitor
|
|
|
|
class TestPluginManager:
|
|
"""Test PluginManager functionality."""
|
|
|
|
def test_init(self, mock_config_manager, mock_display_manager, mock_cache_manager):
|
|
"""Test PluginManager initialization."""
|
|
with patch('src.plugin_system.plugin_manager.ensure_directory_permissions'):
|
|
pm = PluginManager(
|
|
plugins_dir="plugins",
|
|
config_manager=mock_config_manager,
|
|
display_manager=mock_display_manager,
|
|
cache_manager=mock_cache_manager
|
|
)
|
|
assert pm.plugins_dir == Path("plugins")
|
|
assert pm.config_manager == mock_config_manager
|
|
assert pm.display_manager == mock_display_manager
|
|
assert pm.cache_manager == mock_cache_manager
|
|
assert pm.plugins == {}
|
|
|
|
def test_discover_plugins(self, test_plugin_manager):
|
|
"""Test plugin discovery."""
|
|
pm = test_plugin_manager
|
|
# Mock _scan_directory_for_plugins since we can't easily create real files in fixture
|
|
pm._scan_directory_for_plugins = MagicMock(return_value=["plugin1", "plugin2"])
|
|
|
|
# We need to call the real discover_plugins method, not the mock from the fixture
|
|
# But the fixture mocks the whole class instance.
|
|
# Let's create a real instance with mocked dependencies for this test
|
|
pass # Handled by separate test below
|
|
|
|
def test_load_plugin_success(self, mock_config_manager, mock_display_manager, mock_cache_manager):
|
|
"""Test successful plugin loading."""
|
|
with patch('src.plugin_system.plugin_manager.ensure_directory_permissions'), \
|
|
patch('src.plugin_system.plugin_manager.PluginManager._scan_directory_for_plugins'), \
|
|
patch('src.plugin_system.plugin_manager.PluginLoader') as MockLoader, \
|
|
patch('src.plugin_system.plugin_manager.SchemaManager'):
|
|
|
|
pm = PluginManager(
|
|
plugins_dir="plugins",
|
|
config_manager=mock_config_manager,
|
|
display_manager=mock_display_manager,
|
|
cache_manager=mock_cache_manager
|
|
)
|
|
|
|
# Setup mocks
|
|
pm.plugin_manifests = {"test_plugin": {"id": "test_plugin", "name": "Test Plugin"}}
|
|
|
|
mock_loader = MockLoader.return_value
|
|
mock_loader.find_plugin_directory.return_value = Path("plugins/test_plugin")
|
|
mock_loader.load_plugin.return_value = (MagicMock(), MagicMock())
|
|
|
|
# Test loading
|
|
result = pm.load_plugin("test_plugin")
|
|
|
|
assert result is True
|
|
assert "test_plugin" in pm.plugin_modules
|
|
# PluginManager sets state to ENABLED after successful load
|
|
assert pm.state_manager.get_state("test_plugin") == PluginState.ENABLED
|
|
|
|
def test_load_plugin_missing_manifest(self, mock_config_manager, mock_display_manager, mock_cache_manager):
|
|
"""Test loading plugin with missing manifest."""
|
|
with patch('src.plugin_system.plugin_manager.ensure_directory_permissions'):
|
|
pm = PluginManager(
|
|
plugins_dir="plugins",
|
|
config_manager=mock_config_manager,
|
|
display_manager=mock_display_manager,
|
|
cache_manager=mock_cache_manager
|
|
)
|
|
|
|
# No manifest in pm.plugin_manifests
|
|
result = pm.load_plugin("non_existent_plugin")
|
|
|
|
assert result is False
|
|
assert pm.state_manager.get_state("non_existent_plugin") == PluginState.ERROR
|
|
|
|
def test_run_scheduled_updates_calls_update_with_resource_monitor(
|
|
self, mock_config_manager, mock_display_manager, mock_cache_manager
|
|
):
|
|
"""Regression test: run_scheduled_updates() must actually call a
|
|
plugin's update() when self.resource_monitor is set (as it is in
|
|
every real deployment -- display_controller.py and web_interface/
|
|
app.py both assign a real PluginResourceMonitor after construction).
|
|
|
|
Previously, the resource_monitor branch wrapped the call in a
|
|
function stored as a *class* attribute on a dynamically-built type
|
|
(`type('obj', (object,), {'update': monitored_update})()`), which
|
|
the descriptor protocol turns into a bound method on access --
|
|
silently passing the synthetic instance as an implicit first
|
|
argument to monitored_update(), which takes none. Every plugin's
|
|
scheduled update failed with "monitored_update() takes 0 positional
|
|
arguments but 1 was given" and was silently swallowed into a
|
|
circuit-breaker retry loop that never succeeded, so plugin data
|
|
(scores, odds, etc.) never refreshed.
|
|
"""
|
|
with patch('src.plugin_system.plugin_manager.ensure_directory_permissions'):
|
|
pm = PluginManager(
|
|
plugins_dir="plugins",
|
|
config_manager=mock_config_manager,
|
|
display_manager=mock_display_manager,
|
|
cache_manager=mock_cache_manager
|
|
)
|
|
|
|
plugin_instance = MagicMock()
|
|
plugin_instance.enabled = True
|
|
plugin_instance.update = MagicMock()
|
|
|
|
pm.plugins["test_plugin"] = plugin_instance
|
|
pm.plugin_manifests["test_plugin"] = {"update_interval": 10}
|
|
pm.state_manager.set_state("test_plugin", PluginState.ENABLED)
|
|
# Plain MagicMock, not the mock_cache_manager fixture: this test
|
|
# is about run_scheduled_updates() actually invoking update()
|
|
# through the resource-monitor wrapper, not about
|
|
# PluginResourceMonitor's own cache-backed metrics persistence
|
|
# (which calls cache_manager.get(..., memory_ttl=...) --
|
|
# a kwarg the fixture's mock_get() doesn't accept).
|
|
pm.resource_monitor = PluginResourceMonitor(MagicMock())
|
|
|
|
pm.run_scheduled_updates(current_time=time.time())
|
|
|
|
plugin_instance.update.assert_called_once()
|
|
assert "test_plugin" in pm.plugin_last_update
|
|
assert pm.state_manager.get_state("test_plugin") == PluginState.ENABLED
|
|
|
|
|
|
class TestPluginLoader:
|
|
"""Test PluginLoader functionality."""
|
|
|
|
def test_dependency_check(self):
|
|
"""Test dependency checking logic."""
|
|
# Covered by test_plugin_loader.py's install_dependencies tests,
|
|
# which exercise requirements_has_real_deps/requirements_are_satisfied
|
|
# and the pip subprocess fallback.
|
|
|
|
|
|
class TestPluginExecutor:
|
|
"""Test PluginExecutor functionality."""
|
|
|
|
def test_execute_display_success(self):
|
|
"""Test successful display execution."""
|
|
from src.plugin_system.plugin_executor import PluginExecutor
|
|
executor = PluginExecutor()
|
|
|
|
mock_plugin = MagicMock()
|
|
mock_plugin.display.return_value = True
|
|
|
|
result = executor.execute_display(mock_plugin, "test_plugin")
|
|
|
|
assert result is True
|
|
mock_plugin.display.assert_called_once()
|
|
|
|
def test_execute_display_exception(self):
|
|
"""Test display execution with exception."""
|
|
from src.plugin_system.plugin_executor import PluginExecutor
|
|
executor = PluginExecutor()
|
|
|
|
mock_plugin = MagicMock()
|
|
mock_plugin.display.side_effect = Exception("Test error")
|
|
|
|
result = executor.execute_display(mock_plugin, "test_plugin")
|
|
|
|
assert result is False
|
|
|
|
def test_execute_update_timeout(self):
|
|
"""Test update execution timeout."""
|
|
# Using a very short timeout for testing
|
|
from src.plugin_system.plugin_executor import PluginExecutor
|
|
executor = PluginExecutor(default_timeout=0.01)
|
|
|
|
mock_plugin = MagicMock()
|
|
def slow_update():
|
|
time.sleep(0.05)
|
|
mock_plugin.update.side_effect = slow_update
|
|
|
|
result = executor.execute_update(mock_plugin, "test_plugin")
|
|
|
|
assert result is False
|
|
|
|
|
|
class TestPluginHealth:
|
|
"""Test plugin health monitoring."""
|
|
|
|
def test_circuit_breaker(self, mock_cache_manager):
|
|
"""Test circuit breaker activation."""
|
|
from src.plugin_system.plugin_health import PluginHealthTracker
|
|
tracker = PluginHealthTracker(cache_manager=mock_cache_manager, failure_threshold=3, cooldown_period=60)
|
|
|
|
plugin_id = "test_plugin"
|
|
|
|
# Initial state
|
|
assert tracker.should_skip_plugin(plugin_id) is False
|
|
|
|
# Failures
|
|
tracker.record_failure(plugin_id, Exception("Error 1"))
|
|
assert tracker.should_skip_plugin(plugin_id) is False
|
|
|
|
tracker.record_failure(plugin_id, Exception("Error 2"))
|
|
assert tracker.should_skip_plugin(plugin_id) is False
|
|
|
|
tracker.record_failure(plugin_id, Exception("Error 3"))
|
|
# Should trip now
|
|
assert tracker.should_skip_plugin(plugin_id) is True
|
|
|
|
# Recovery (simulate timeout - need to update health state correctly)
|
|
if plugin_id in tracker._health_state:
|
|
tracker._health_state[plugin_id]["last_failure"] = time.time() - 61
|
|
tracker._health_state[plugin_id]["circuit_state"] = "closed"
|
|
assert tracker.should_skip_plugin(plugin_id) is False
|
|
|
|
|
|
class TestBasePlugin:
|
|
"""Test BasePlugin functionality."""
|
|
|
|
def test_dynamic_duration_defaults(self, mock_display_manager, mock_cache_manager):
|
|
"""Test default dynamic duration behavior."""
|
|
from src.plugin_system.base_plugin import BasePlugin
|
|
|
|
# Concrete implementation for testing
|
|
class ConcretePlugin(BasePlugin):
|
|
def update(self): pass
|
|
def display(self, force_clear=False): pass
|
|
|
|
config = {"enabled": True}
|
|
plugin = ConcretePlugin("test", config, mock_display_manager, mock_cache_manager, None)
|
|
|
|
assert plugin.supports_dynamic_duration() is False
|
|
assert plugin.get_dynamic_duration_cap() is None
|
|
assert plugin.is_cycle_complete() is True
|
|
|
|
def test_live_priority_config(self, mock_display_manager, mock_cache_manager):
|
|
"""Test live priority configuration."""
|
|
from src.plugin_system.base_plugin import BasePlugin
|
|
|
|
class ConcretePlugin(BasePlugin):
|
|
def update(self): pass
|
|
def display(self, force_clear=False): pass
|
|
|
|
config = {"enabled": True, "live_priority": True}
|
|
plugin = ConcretePlugin("test", config, mock_display_manager, mock_cache_manager, None)
|
|
|
|
assert plugin.has_live_priority() is True
|