From 717a05f60480d05e6272d603d91416040ea4181d Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sun, 12 Apr 2026 03:34:09 +0000 Subject: [PATCH 1/4] fix(security): fix plugins/loader.py load_from_entry_point security #7476 Parse entry point targets before import so allowlist enforcement happens prior to execution and add a Behave regression scenario covering the disallowed-prefix path. ISSUES CLOSED: #7476 --- CHANGELOG.md | 4 ++ .../extensibility/plugin_architecture.feature | 9 ++++ features/steps/plugin_architecture_steps.py | 33 +++++++++++- .../infrastructure/plugins/loader.py | 51 +++++++++++++++++-- 4 files changed, 91 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36ca0cc2c..c463fa2c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -249,6 +249,10 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). e2e test files and removed `tdd_expected_fail` from 4 context assembly e2e tests where bugs were already fixed. +- **PluginLoader entry point prefix validation** (#7476): Parse entry point targets before + import, enforce the module allowlist ahead of loading, and add a Behave regression scenario + to ensure disallowed prefixes never execute untrusted module-level code. + - **`issue-state-updater` Bash Script Errors**: Removed problematic bash script examples that tried to invoke `task forgejo-label-manager` as a bash command (the Task tool cannot be invoked from bash). Replaced with clear step-by-step operational instructions and diff --git a/features/extensibility/plugin_architecture.feature b/features/extensibility/plugin_architecture.feature index ad8feff87..82efed388 100644 --- a/features/extensibility/plugin_architecture.feature +++ b/features/extensibility/plugin_architecture.feature @@ -192,6 +192,15 @@ Feature: Plugin Architecture Framework with module:ClassName resolution Then the discovered plugin list should have 1 entry And the first descriptor name should be "test-ep" + @entry_points @security + Scenario: Entry point with disallowed prefix is skipped without loading module + Given a PluginLoader with default prefixes + And a mocked entry point group "cleveragents.plugins" with raw entry "malicious=maliciouspkg.attack:Exploit" + When I discover plugins from the mocked entry point group + Then the discovered plugin list should be empty + And the mocked entry point load should not be called + And a security warning should be emitted for disallowed entry point "malicious" + # --------------------------------------------------------------------------- # PluginManager — lifecycle # --------------------------------------------------------------------------- diff --git a/features/steps/plugin_architecture_steps.py b/features/steps/plugin_architecture_steps.py index 125428a26..e691d8de8 100644 --- a/features/steps/plugin_architecture_steps.py +++ b/features/steps/plugin_architecture_steps.py @@ -12,7 +12,7 @@ from __future__ import annotations import contextlib import threading from typing import Any -from unittest.mock import MagicMock, patch +from unittest.mock import ANY, MagicMock, patch from behave import given, then, when # type: ignore[import-untyped] from behave.runner import Context # type: ignore[import-untyped] @@ -429,6 +429,37 @@ def step_first_descriptor_name(context: Context, name: str) -> None: assert context.discovered[0].name == name +@given('a mocked entry point group "{group}" with raw entry "{entry_spec}"') +def step_mock_raw_entry(context: Context, group: str, entry_spec: str) -> None: + name, value = entry_spec.split("=", 1) + mock_ep = MagicMock() + mock_ep.name = name + mock_ep.value = value + mock_ep.load = MagicMock(name="load") + + context.mock_group = group + context.mock_eps = [mock_ep] + context.mock_ep_last = mock_ep + context.loader._logger = MagicMock() + + +@then("the mocked entry point load should not be called") +def step_entry_point_not_loaded(context: Context) -> None: + context.mock_ep_last.load.assert_not_called() + + +@then('a security warning should be emitted for disallowed entry point "{name}"') +def step_security_warning_emitted(context: Context, name: str) -> None: + logger_mock = context.loader._logger + logger_mock.warning.assert_any_call( + "plugin.entry_point_disallowed_prefix", + name=name, + value=context.mock_ep_last.value, + group=context.mock_group, + error=ANY, + ) + + # --------------------------------------------------------------------------- # PluginManager — lifecycle # --------------------------------------------------------------------------- diff --git a/src/cleveragents/infrastructure/plugins/loader.py b/src/cleveragents/infrastructure/plugins/loader.py index 3469aa909..a5547e371 100644 --- a/src/cleveragents/infrastructure/plugins/loader.py +++ b/src/cleveragents/infrastructure/plugins/loader.py @@ -182,11 +182,33 @@ class PluginLoader: eps = entry_points.select(group=group) for ep in eps: + raw_value = ep.value + try: + module_path, class_name = self._parse_entry_point_value(raw_value) + except PluginLoadError as exc: + self._logger.warning( + "plugin.entry_point_invalid_value", + name=ep.name, + value=raw_value, + group=group, + error=str(exc), + ) + continue + + try: + self._validate_module_prefix(module_path) + except PluginLoadError as exc: + self._logger.warning( + "plugin.entry_point_disallowed_prefix", + name=ep.name, + value=raw_value, + group=group, + error=str(exc), + ) + continue + try: ep.load() - module_path = ep.value.rsplit(":", 1)[0] if ":" in ep.value else "" - class_name = ep.value.rsplit(":", 1)[1] if ":" in ep.value else ep.value - descriptor = PluginDescriptor( name=ep.name, module_path=module_path, @@ -197,14 +219,14 @@ class PluginLoader: self._logger.info( "plugin.discovered_entry_point", name=ep.name, - value=ep.value, + value=raw_value, group=group, ) except Exception as exc: self._logger.warning( "plugin.entry_point_failed", name=ep.name, - value=ep.value, + value=raw_value, group=group, error=str(exc), ) @@ -270,3 +292,22 @@ class PluginLoader: f"may be dynamically imported." ) raise PluginLoadError(msg) + + @staticmethod + def _parse_entry_point_value(value: str) -> tuple[str, str]: + """Split an entry point value into module and class components.""" + normalized_value = value.strip() + if ":" not in normalized_value: + raise PluginLoadError( + "Entry point value must be in 'module:ClassName' format." + ) + + module_path, class_name = ( + part.strip() for part in normalized_value.split(":", 1) + ) + if not module_path or not class_name: + raise PluginLoadError( + "Entry point value must include both module path and class name." + ) + + return module_path, class_name -- 2.52.0 From cef70ff98a0cfbe815fc93d5d0ea11026cea30f3 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sun, 12 Apr 2026 08:12:47 +0000 Subject: [PATCH 2/4] fix(security): add Robot integration test and CONTRIBUTORS for #7476 Add Robot Framework integration test verifying that load_from_entry_points does not call ep.load() for entry points with disallowed module prefixes (security regression test for issue #7476). Also add HAL 9000 to CONTRIBUTORS.md per CONTRIBUTING.md process rules. ISSUES CLOSED: #7476 --- CHANGELOG.md | 5 ++- .../extensibility/plugin_architecture.feature | 2 +- .../steps/plugins_loader_coverage_steps.py | 2 +- robot/helper_plugin_architecture.py | 43 +++++++++++++++++++ robot/plugin_architecture.robot | 13 ++++++ .../infrastructure/plugins/loader.py | 8 +++- 6 files changed, 68 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c463fa2c9..72970ffa3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -250,8 +250,9 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). bugs were already fixed. - **PluginLoader entry point prefix validation** (#7476): Parse entry point targets before - import, enforce the module allowlist ahead of loading, and add a Behave regression scenario - to ensure disallowed prefixes never execute untrusted module-level code. + import, enforce the module allowlist ahead of loading, and add Behave plus Robot Framework + regression coverage to ensure disallowed prefixes never execute untrusted module-level code + in either unit or integration flows. - **`issue-state-updater` Bash Script Errors**: Removed problematic bash script examples that tried to invoke `task forgejo-label-manager` as a bash command (the Task tool cannot diff --git a/features/extensibility/plugin_architecture.feature b/features/extensibility/plugin_architecture.feature index 82efed388..cabaa7815 100644 --- a/features/extensibility/plugin_architecture.feature +++ b/features/extensibility/plugin_architecture.feature @@ -192,7 +192,7 @@ Feature: Plugin Architecture Framework with module:ClassName resolution Then the discovered plugin list should have 1 entry And the first descriptor name should be "test-ep" - @entry_points @security + @entry_points @security @tdd_issue @tdd_issue_7476 Scenario: Entry point with disallowed prefix is skipped without loading module Given a PluginLoader with default prefixes And a mocked entry point group "cleveragents.plugins" with raw entry "malicious=maliciouspkg.attack:Exploit" diff --git a/features/steps/plugins_loader_coverage_steps.py b/features/steps/plugins_loader_coverage_steps.py index 391066503..89cf3107f 100644 --- a/features/steps/plugins_loader_coverage_steps.py +++ b/features/steps/plugins_loader_coverage_steps.py @@ -39,7 +39,7 @@ def step_mock_failing_entry_point(context): """Create a mock entry point whose load() raises an exception.""" failing_ep = MagicMock() failing_ep.name = "broken_plugin" - failing_ep.value = "some.module:BrokenClass" + failing_ep.value = "cleveragents.domain.models.acms.nonexistent:BrokenClass" failing_ep.load.side_effect = ImportError("Simulated import failure") # Mock the entry_points() call to return a selectable object diff --git a/robot/helper_plugin_architecture.py b/robot/helper_plugin_architecture.py index 2b9790ee8..a28e884bf 100644 --- a/robot/helper_plugin_architecture.py +++ b/robot/helper_plugin_architecture.py @@ -284,6 +284,49 @@ def main() -> int: print(f"plugin-extension-points-fail: {exc}") return 1 + if command == "entry-point-security": + # Security regression test for issue #7476. + # Verifies that load_from_entry_points does NOT call ep.load() for + # entry points whose module prefix is not in the allowlist. + # Uses importlib.metadata.EntryPoint mocking via a real PluginLoader + # instance with a controlled entry_points() override. + try: + import importlib.metadata + import unittest.mock as mock + + loader = PluginLoader() + + # Build a fake entry point that references a disallowed module. + fake_ep = mock.MagicMock() + fake_ep.name = "malicious" + fake_ep.value = "maliciouspkg.attack:Exploit" + fake_ep.group = "cleveragents.plugins" + + # Patch importlib.metadata.entry_points to return our fake entry point. + fake_eps = mock.MagicMock() + fake_eps.select.return_value = [fake_ep] + + with mock.patch.object( + importlib.metadata, + "entry_points", + return_value=fake_eps, + ): + descriptors = loader.load_from_entry_points("cleveragents.plugins") + + # The disallowed entry point must be skipped — no descriptors returned. + assert descriptors == [], ( + f"Expected empty descriptor list, got: {descriptors}" + ) + + # ep.load() must NEVER have been called for the disallowed entry point. + fake_ep.load.assert_not_called() + + print("plugin-entry-point-security-ok") + return 0 + except Exception as exc: + print(f"plugin-entry-point-security-fail: {exc}") + return 1 + print(f"Unknown command: {command}") return 1 diff --git a/robot/plugin_architecture.robot b/robot/plugin_architecture.robot index c5dd94141..b14b67d49 100644 --- a/robot/plugin_architecture.robot +++ b/robot/plugin_architecture.robot @@ -87,3 +87,16 @@ PluginManager Extension Points Log ${result.stderr} Should Be Equal As Integers ${result.rc} 0 Should Contain ${result.stdout} plugin-extension-points-ok + +PluginLoader Entry Point Security - Disallowed Prefix Not Loaded + [Documentation] Security regression test for issue #7476. + ... + ... Verifies that load_from_entry_points does NOT call ep.load() + ... for entry points whose module prefix is not in the allowlist. + ... This ensures arbitrary code execution is prevented even when + ... a malicious package registers an entry point. + ${result}= Run Process ${PYTHON} ${HELPER} entry-point-security cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} plugin-entry-point-security-ok diff --git a/src/cleveragents/infrastructure/plugins/loader.py b/src/cleveragents/infrastructure/plugins/loader.py index a5547e371..83329662d 100644 --- a/src/cleveragents/infrastructure/plugins/loader.py +++ b/src/cleveragents/infrastructure/plugins/loader.py @@ -295,7 +295,13 @@ class PluginLoader: @staticmethod def _parse_entry_point_value(value: str) -> tuple[str, str]: - """Split an entry point value into module and class components.""" + """Split an entry point value into module and class components. + + The returned ``module_path`` is intentionally not validated beyond + basic shape checks; prefix allowlist enforcement and any stricter + validation must be performed by :meth:`_validate_module_prefix` to + preserve the layered defense order (parse → validate prefix → load). + """ normalized_value = value.strip() if ":" not in normalized_value: raise PluginLoadError( -- 2.52.0 From 74ce88caeb66aabb421b1fd55b4c8f206e40ab00 Mon Sep 17 00:00:00 2001 From: HAL 9000 Date: Mon, 13 Apr 2026 17:25:42 +0000 Subject: [PATCH 3/4] docs(contributors): add HAL 9000 to contributors list ISSUES CLOSED: #7476 -- 2.52.0 From 46ed31930ed8afac3c6c11b8d5a610041e4fa2fc Mon Sep 17 00:00:00 2001 From: CleverThis Date: Tue, 14 Apr 2026 15:04:10 +0000 Subject: [PATCH 4/4] docs(contributors): add HAL 9000 plugin security hardening contribution detail Added detail entry for HAL 9000's contribution to the plugin entry point security hardening fix (#7476). ISSUES CLOSED: #7476 --- CONTRIBUTORS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 056737b45..60ee6fb1d 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -18,5 +18,6 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed automated implementation, bug fixes, and feature development as part of the CleverAgents automation pool. * HAL 9000 has contributed concurrency safety improvements, including thread-safe context tier management (issue #7547) for parallel plan execution. * HAL 9000 has contributed the plan concurrency race-condition fix (#7989): wired `LockService` into the plan lifecycle, guarding `execute_plan()` and `apply_plan()` with plan-level advisory locks and unique per-invocation owner identities to prevent silent concurrent state corruption. +* HAL 9000 has contributed the plugin entry point security hardening fix (#7476): enforced entry point allowlist validation before importing plugin modules to prevent malicious plugin loading. * This project was made possible thanks to considerable donation of time, money, and resources by CleverThis, Inc. * HAL 9000 has contributed automated bug fixes, CLI output formatting improvements, and ongoing maintenance as part of the CleverAgents automation system. -- 2.52.0