From b5a0e020fc14f8604e71876e48a3a2e4e4ecb48a Mon Sep 17 00:00:00 2001 From: Michael Lieberman Date: Tue, 6 Oct 2026 20:12:18 -0400 Subject: [PATCH 1/2] docs(spec): one module path resolution policy for configured imports Section 6.5 described a prefix whitelist "discovered from entry points" that the code never implemented, and did not cover the MCP tool loader, which imported any module named in [mcp.tools]. State the contract the fix implements: one resolver for every module:attribute import, a top-level package of darnit or an installed implementation's entry point module, refusal before import, and refused tools reported at server start. Refs #490 Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Michael Lieberman --- docs/architecture/example-plugin.md | 6 +++--- docs/architecture/framework-design.md | 30 +++++++++++++++++++++------ 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/docs/architecture/example-plugin.md b/docs/architecture/example-plugin.md index 13d3e963..06c5f6d7 100644 --- a/docs/architecture/example-plugin.md +++ b/docs/architecture/example-plugin.md @@ -97,11 +97,11 @@ The implementation SHALL provide a `register_handlers()` method that registers a - **THEN** the returned path has filename `example-hygiene.toml` and `path.exists()` is `True` ### Requirement: Framework integration with minimal changes -The package SHALL integrate into the darnit workspace with only two framework-side changes: adding `"darnit_example."` to `ALLOWED_MODULE_PREFIXES` in handlers.py, and adding `darnit-example` to the root `pyproject.toml` workspace sources and ruff config. +The package SHALL integrate into the darnit workspace with only one framework-side change: adding `darnit-example` to the root `pyproject.toml` workspace sources and ruff config. Its modules are importable by handler path because `darnit_example` is the module of its `darnit.implementations` entry point (framework-design.md 6.5); no list in the framework names it. -#### Scenario: Module prefix allowlisted +#### Scenario: Module path permitted by the entry point - **WHEN** handler resolution attempts to load a `darnit_example.*` module -- **THEN** the security allowlist permits the import +- **THEN** the module resolution policy permits the import ### Requirement: Documentation cross-references The package README SHALL map each section of `docs/IMPLEMENTATION_GUIDE.md` to its corresponding example file. The implementation guide SHALL reference `packages/darnit-example/` as a working companion example. diff --git a/docs/architecture/framework-design.md b/docs/architecture/framework-design.md index 019a9fc6..abe743d0 100644 --- a/docs/architecture/framework-design.md +++ b/docs/architecture/framework-design.md @@ -1436,16 +1436,34 @@ handler = "my_audit" # Short name instead of "my_plugin.tools:my_audit" ### 6.5 Function Reference Security -TOML can reference Python functions via `module:function` syntax: +A TOML handler reference may name a Python attribute as `package.module:attribute` instead of a registered short name (Section 6.4): ```toml -api_check = "darnit_baseline.checks:check_branch_protection" +[mcp.tools.remediate_community_spec] +handler = "darnit_csl.mcp_tools:remediate_community_spec" ``` -**Security Rules**: -- Only whitelisted module prefixes are allowed -- Base whitelist: `darnit.`, `darnit_baseline.`, `darnit_plugins.` -- Additional prefixes discovered from registered entry points +Every place that turns such a string into an import SHALL resolve it through one function, `darnit.core.handlers.resolve_module_path`. That covers MCP tool handlers (`ToolRegistry.load_handler`), handler-registry lookups (`HandlerRegistry.get_handler`), and Python adapter configuration (`PluginRegistry` and `AdapterRegistry`). No other code in the framework SHALL call `importlib.import_module` on a configured string. + +**Resolution policy**: +- The path SHALL have the form `a.b.c:attr`: exactly one `:`, every dotted module part and the attribute a Python identifier. A relative path (leading `.`), an empty part, a dotted attribute, or any other form is refused. +- The module's top-level package SHALL be `darnit`, or the top-level package of an implementation that discovery loaded from the `darnit.implementations` entry point group (Section 6.2). The set is read from the entry points' module names (`darnit_csl:register` gives `darnit_csl`), not from a list in code, so an installed third-party implementation is covered and an uninstalled one is not. It is computed once per process with the implementation cache and recomputed when that cache is cleared. +- The policy is checked before any import. A refused module is never imported. + +**Failure behavior**: +- A refused path SHALL raise `HandlerImportRefused` (a `ValueError`) whose message names the path and the allowed top-level packages, logged at WARNING. A refusal is an error, never "not found": `HandlerRegistry.get_handler` raises it rather than returning `None`. +- An allowed path whose module or attribute does not exist raises `ImportError` or `AttributeError`. `HandlerRegistry.get_handler` reports that as not found (`None`, with a warning). +- At MCP server start a refused tool handler SHALL NOT be registered, and the server SHALL log an ERROR naming the tool and the refused path. The remaining tools still load. + +This policy limits which installed code a configured string can reach. It is not a sandbox: the allowed packages are code the operator installed, and `[mcp.tools]` comes only from an installed framework TOML or an operator-supplied `darnit serve `, never from the audited repository (Section 14). + +#### Scenario: Shipped plugin tool by module path +- **WHEN** the community-spec server starts and its TOML names `darnit_csl.mcp_tools:remediate_community_spec` +- **THEN** the tool SHALL load, because `darnit_csl` is the module of the `community-spec` entry point + +#### Scenario: Tool handler outside the policy +- **WHEN** an `[mcp.tools]` entry names `os:system`, or a module of a package that is not an installed implementation +- **THEN** the module SHALL NOT be imported, the tool SHALL NOT be registered, and server start SHALL report the refusal at ERROR ### 6.6 Plugin Verification with Sigstore From 3522b28cf01f9ceaad99c81ad18fbb2702e49577 Mon Sep 17 00:00:00 2001 From: Michael Lieberman Date: Tue, 6 Oct 2026 20:12:18 -0400 Subject: [PATCH 2/2] fix(core): resolve every configured module path under one policy ToolRegistry.load_handler, the path that loads [mcp.tools] handlers at server start, imported any module:function string with no check, while HandlerRegistry and the two adapter registries each kept a hardcoded prefix tuple that missed darnit_csl, darnit_gittuf, darnit_reproducibility and darnit_hello. The shipped community-spec tool loaded only through the unchecked path. All four now call darnit.core.handlers.resolve_module_path. It refuses a malformed path or a top-level package other than darnit or the module of a discovered darnit.implementations entry point, before importing. The allowed set is recorded by discovery and reset with its cache. A refusal raises HandlerImportRefused (a ValueError) naming the path and allowed packages; HandlerRegistry.get_handler raises it rather than returning None, and the server factory logs the refused tool at ERROR and does not register it. Fixes #490 Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Michael Lieberman --- CHANGELOG.md | 13 ++ THREAT_MODEL.md | 173 +++--------------- docs/IMPLEMENTATION_GUIDE.md | 38 ++-- docs/SECURITY_GUIDE.md | 81 ++------ packages/darnit/src/darnit/core/adapters.py | 25 +-- packages/darnit/src/darnit/core/discovery.py | 21 ++- packages/darnit/src/darnit/core/handlers.py | 107 +++++++---- packages/darnit/src/darnit/core/registry.py | 21 +-- packages/darnit/src/darnit/server/factory.py | 7 + packages/darnit/src/darnit/server/registry.py | 16 +- tests/darnit/core/test_handlers.py | 53 +++--- tests/darnit/core/test_module_path_policy.py | 153 ++++++++++++++++ tests/darnit/server/test_factory.py | 88 +++++++-- tests/darnit/server/test_registry.py | 37 +++- 14 files changed, 469 insertions(+), 364 deletions(-) create mode 100644 tests/darnit/core/test_module_path_policy.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 356dc084..64d79092 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ceiling) is refused, logged at WARNING naming both registrants, and listed by `darnit list` and in the warnings of an audit of that plugin's framework (or a framework composing it); the existing step type is unchanged. +- One policy governs every import of a `module:attribute` path from + configuration (MCP tool handlers, handler references, Python adapters): + the module's top-level package must be `darnit` or the package of an + installed implementation, read from the `darnit.implementations` entry + points. Before, the MCP tool loader imported any module named in + `[mcp.tools]`, and the other loaders checked hardcoded prefix lists that + missed most shipped implementations. A refused path raises + `HandlerImportRefused` naming the path and the allowed packages; at server + start the tool is not registered and the refusal is logged at ERROR. + `HandlerRegistry.get_handler` raises it instead of returning `None` (#490). ### Removed @@ -49,6 +59,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `darnit.config.context_resolve.resolve_context` (read) and `darnit.config.context_writes` (write); `detect_ci_provider` is the one CI detector. +- `ALLOWED_MODULE_PREFIXES` on `HandlerRegistry`, `PluginRegistry`, and + `AdapterRegistry`. The allowed packages come from installed + implementations; there is no list to extend (#490). ### Added diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md index 85100314..cc8ecbca 100644 --- a/THREAT_MODEL.md +++ b/THREAT_MODEL.md @@ -13,7 +13,7 @@ |------------|-------| | 🔴 Critical | 0 | | 🟠 High | 4 | -| 🟡 Medium | 20 | +| 🟡 Medium | 17 | | đŸŸĸ Low | 93 | | ℹī¸ Info | 0 | @@ -782,139 +782,28 @@ The `local_path` MCP parameter is the primary trust boundary — the user (MCP c #### TM-E-001: Dynamic import via importlib.import_module(module_path) **Risk:** MEDIUM (severity × confidence = 3.50) -**Location:** `packages/darnit/src/darnit/core/registry.py:821` +**Location:** `packages/darnit/src/darnit/core/handlers.py:422` **Source:** `tree_sitter_structural` — query `python.eop.dynamic_import_attr` Dynamic imports allow loading arbitrary modules at runtime. If the module name originates from untrusted input, an attacker can achieve arbitrary code execution. -> **Mitigation (verified):** Module path is validated against `ALLOWED_MODULE_PREFIXES` allowlist before import (visible in the code snippet at lines 812-818). Only modules from trusted package prefixes can be loaded. An attacker would need to modify the allowlist or the TOML config, both of which require write access to the installation. - -``` - 811 | - 812 | # Security: Validate module path against allowlist to prevent arbitrary code loading - 813 | if not any(module_path.startswith(prefix) for prefix in self.ALLOWED_MODULE_PREFIXES): - 814 | logger.error( - 815 | f"Adapter {name}: module '{module_path}' not in allowed prefixes. " - 816 | f"Allowed: {self.ALLOWED_MODULE_PREFIXES}" - 817 | ) - 818 | return None - 819 | - 820 | try: ->>> 821 | module = importlib.import_module(module_path) - 822 | adapter_class = getattr(module, class_name) - 823 | return adapter_class() - 824 | - 825 | except ImportError as e: - 826 | logger.error(f"Failed to import adapter {name}: {e}") - 827 | return None - 828 | except AttributeError as e: - 829 | logger.error(f"Adapter {name}: class {class_name} not found: {e}") - 830 | return None - 831 | -``` - -#### TM-E-002: Dynamic import via importlib.import_module(module_path) - -**Risk:** MEDIUM (severity × confidence = 3.50) -**Location:** `packages/darnit/src/darnit/core/handlers.py:237` -**Source:** `tree_sitter_structural` — query `python.eop.dynamic_import_attr` - -Dynamic imports allow loading arbitrary modules at runtime. If the module name originates from untrusted input, an attacker can achieve arbitrary code execution. - -> **Mitigation (verified):** Module path is validated against `ALLOWED_MODULE_PREFIXES` allowlist before import (visible in the code snippet at lines 812-818). Only modules from trusted package prefixes can be loaded. An attacker would need to modify the allowlist or the TOML config, both of which require write access to the installation. +> **Mitigation (verified, #490):** This is the only place darnit-core imports a module named by configuration. `resolve_module_path` serves every caller that turns a `module:attribute` string into an import: MCP tool handlers (`ToolRegistry.load_handler`, `server/registry.py`), handler-registry lookups (`HandlerRegistry.get_handler`), and Python adapter configuration (`PluginRegistry` in `core/registry.py`, `AdapterRegistry` in `core/adapters.py`). Before importing, it requires the form `a.b.c:attr` (identifiers only, no relative or empty parts) and a top-level package that is `darnit` or the package of an implementation discovery loaded from the `darnit.implementations` entry points. The allowed set is derived from installed entry point metadata, not a list in code, so it covers third-party implementations and excludes packages that are not installed implementations. A refused path raises `HandlerImportRefused` naming the path and the allowed packages; at MCP server start the tool is not registered and the refusal is logged at ERROR. Residual risk: an allowed package is code the operator installed, so the policy narrows what a configured string can reach but does not sandbox it. `[mcp.tools]` comes from an installed framework TOML or an operator-supplied `darnit serve `, never from the audited repository (framework-design.md 6.5, 14). ``` - 227 | module_path, func_name = path.rsplit(":", 1) - 228 | - 229 | # Validate module path against allowlist to prevent arbitrary imports - 230 | if not any(module_path.startswith(prefix) for prefix in self.ALLOWED_MODULE_PREFIXES): - 231 | logger.warning( - 232 | f"Module path '{module_path}' not in allowed prefixes: " - 233 | f"{self.ALLOWED_MODULE_PREFIXES}" - 234 | ) - 235 | return None - 236 | ->>> 237 | module = importlib.import_module(module_path) - 238 | return getattr(module, func_name, None) - 239 | except (ValueError, ImportError, AttributeError) as e: - 240 | logger.warning(f"Failed to load handler from path '{path}': {e}") - 241 | return None - 242 | - 243 | # ========================================================================= - 244 | # Pass Registration - 245 | # ========================================================================= - 246 | - 247 | def register_pass( -``` - -#### TM-E-003: Dynamic import via importlib.import_module(module_path) - -**Risk:** MEDIUM (severity × confidence = 3.50) -**Location:** `packages/darnit/src/darnit/core/adapters.py:666` -**Source:** `tree_sitter_structural` — query `python.eop.dynamic_import_attr` - -Dynamic imports allow loading arbitrary modules at runtime. If the module name originates from untrusted input, an attacker can achieve arbitrary code execution. - -> **Mitigation (verified):** Module path is validated against `ALLOWED_MODULE_PREFIXES` allowlist before import (visible in the code snippet at lines 812-818). Only modules from trusted package prefixes can be loaded. An attacker would need to modify the allowlist or the TOML config, both of which require write access to the installation. - -``` - 656 | - 657 | # Security: Validate module path against allowlist to prevent arbitrary code loading - 658 | if not any(module_path.startswith(prefix) for prefix in self.ALLOWED_MODULE_PREFIXES): - 659 | logger.error( - 660 | f"Adapter {name}: module '{module_path}' not in allowed prefixes. " - 661 | f"Allowed: {self.ALLOWED_MODULE_PREFIXES}" - 662 | ) - 663 | return None - 664 | - 665 | try: ->>> 666 | module = importlib.import_module(module_path) - 667 | adapter_class = getattr(module, class_name) - 668 | - 669 | if not issubclass(adapter_class, expected_type): - 670 | logger.error( - 671 | f"Adapter {name}: {class_name} is not a {expected_type.__name__}" - 672 | ) - 673 | return None - 674 | - 675 | return adapter_class() - 676 | -``` - -#### TM-E-004: Dynamic import via importlib.import_module(module_path) - -**Risk:** MEDIUM (severity × confidence = 3.50) -**Location:** `packages/darnit/src/darnit/server/registry.py:151` -**Source:** `tree_sitter_structural` — query `python.eop.dynamic_import_attr` - -Dynamic imports allow loading arbitrary modules at runtime. If the module name originates from untrusted input, an attacker can achieve arbitrary code execution. - -> **Mitigation (partial):** Unlike the other dynamic imports, this path does NOT validate against an allowlist — it directly imports `spec.handler` after splitting on `:`. The handler name comes from TOML tool configuration, which is a trusted source. However, adding an `ALLOWED_MODULE_PREFIXES` check here (as exists in registry.py, handlers.py, and adapters.py) would improve defense-in-depth. **Recommendation:** Add allowlist validation to `ToolRegistry.load_handler()`. - -``` - 141 | return handler - 142 | - 143 | raise ValueError( - 144 | f"Handler '{spec.handler}' not found in registry. " - 145 | "Either register it via register_handlers() or use " - 146 | "full module path 'module.path:function_name'" - 147 | ) - 148 | - 149 | # Full module path format - 150 | module_path, func_name = spec.handler.rsplit(":", 1) ->>> 151 | module = importlib.import_module(module_path) - 152 | return getattr(module, func_name) - 153 | - 154 | def _load_builtin( - 155 | self, spec: ToolSpec, framework_name: str | None - 156 | ) -> Callable[..., Any]: - 157 | """Load a built-in tool and bind it to a framework. - 158 | - 159 | Built-in tools receive the framework name as a bound parameter - 160 | so they know which TOML config to load. - 161 | + 414 | module_path, sep, attr = path.rpartition(":") + 415 | parts = module_path.split(".") + 416 | if not sep or ":" in module_path or not attr.isidentifier() or not all(part.isidentifier() for part in parts): + 417 | raise _refuse(path, "not of the form 'package.module:attribute'") + 418 | + 419 | if parts[0] != CORE_PACKAGE and parts[0] not in allowed_module_packages(): + 420 | raise _refuse(path, f"'{parts[0]}' is neither darnit nor an installed darnit implementation") + 421 | +>>> 422 | module = importlib.import_module(module_path) + 423 | return getattr(module, attr) ``` +This entry replaces the four earlier dynamic-import entries (TM-E-001 `core/registry.py`, TM-E-002 `HandlerRegistry._load_handler_from_path`, TM-E-003 `core/adapters.py`, TM-E-004 `server/registry.py`). Those sites no longer import; they call `resolve_module_path`. Before #490 three of them kept their own hardcoded prefix tuple, none of which named every shipped implementation, and the MCP tool loader, the path that loads `[mcp.tools]` handlers, checked nothing. + ## Attack Chains No compound attack paths identified. @@ -932,27 +821,23 @@ No compound attack paths identified. 1. **Unauthenticated mcp tool (mcp): (dynamic — registered from registry.tools)** — `packages/darnit/src/darnit/server/factory.py:149` (mitigated: MCP stdio transport auth) 2. **Unauthenticated mcp tool (mcp): (dynamic — registered from registry.tools)** — `packages/darnit/src/darnit/server/factory.py:195` (mitigated: MCP stdio transport auth) -3. **Dynamic import via importlib.import_module(module_path)** — `packages/darnit/src/darnit/core/registry.py:821` (mitigated: ALLOWED_MODULE_PREFIXES allowlist) -4. **Dynamic import via importlib.import_module(module_path)** — `packages/darnit/src/darnit/core/handlers.py:237` (mitigated: ALLOWED_MODULE_PREFIXES allowlist) -5. **Dynamic import via importlib.import_module(module_path)** — `packages/darnit/src/darnit/core/adapters.py:666` (mitigated: ALLOWED_MODULE_PREFIXES allowlist) -6. **Dynamic import via importlib.import_module(module_path)** — `packages/darnit/src/darnit/server/registry.py:151` (NO allowlist — recommend adding one) -7. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/tools/audit_org.py:62` (mitigated: list-form subprocess, gh validates) -8. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/tools/audit_org.py:113` (mitigated: list-form subprocess, gh validates) -9. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/core/utils.py:27` (mitigated: list-form subprocess, gh validates) -10. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/core/adapters.py:354` (mitigated: trusted TOML config) -11. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/test_repository.py:141` (test tool only) -12. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/git_operations.py:45` (mitigated: list-form subprocess) -13. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/git_operations.py:53` (mitigated: list-form subprocess) -14. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/git_operations.py:68` (mitigated: list-form subprocess) -15. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/git_operations.py:98` (mitigated: list-form subprocess) -16. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/git_operations.py:209` (mitigated: list-form subprocess) -17. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/git_operations.py:295` (mitigated: list-form subprocess) -18. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/remediation/github.py:268` (mitigated: stdin input, not shell args) +3. **Dynamic import via importlib.import_module(module_path)** — `packages/darnit/src/darnit/core/handlers.py:422` (mitigated: the one module path resolver; top-level package must be darnit or an installed implementation's) +4. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/tools/audit_org.py:62` (mitigated: list-form subprocess, gh validates) +5. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/tools/audit_org.py:113` (mitigated: list-form subprocess, gh validates) +6. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/core/utils.py:27` (mitigated: list-form subprocess, gh validates) +7. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/core/adapters.py:354` (mitigated: trusted TOML config) +8. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/test_repository.py:141` (test tool only) +9. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/git_operations.py:45` (mitigated: list-form subprocess) +10. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/git_operations.py:53` (mitigated: list-form subprocess) +11. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/git_operations.py:68` (mitigated: list-form subprocess) +12. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/git_operations.py:98` (mitigated: list-form subprocess) +13. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/git_operations.py:209` (mitigated: list-form subprocess) +14. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/server/tools/git_operations.py:295` (mitigated: list-form subprocess) +15. **Potential command injection via subprocess.run** — `packages/darnit/src/darnit/remediation/github.py:268` (mitigated: stdin input, not shell args) ### Recommended Code Improvements -1. **Add allowlist to `ToolRegistry.load_handler()`** (server/registry.py:151) — The only dynamic import path without `ALLOWED_MODULE_PREFIXES` validation. Low urgency since handler names come from trusted TOML. -2. **Add timeouts to git_operations.py subprocess calls** — 42 of the 43 DoS findings are missing `timeout=` on subprocess calls in this file. +1. **Add timeouts to git_operations.py subprocess calls** — 42 of the 43 DoS findings are missing `timeout=` on subprocess calls in this file. ## Verification Prompts diff --git a/docs/IMPLEMENTATION_GUIDE.md b/docs/IMPLEMENTATION_GUIDE.md index c4932d71..4cbea7a7 100644 --- a/docs/IMPLEMENTATION_GUIDE.md +++ b/docs/IMPLEMENTATION_GUIDE.md @@ -1560,23 +1560,16 @@ Or by full module path: handler = "darnit_mystandard.tools:audit_mystandard" ``` -### Module allowlist security +### Module path policy -When handlers are referenced by `module:function` path in TOML, the registry only -allows imports from approved module prefixes. The default allowlist in -`packages/darnit/src/darnit/core/handlers.py`: - -```python -ALLOWED_MODULE_PREFIXES = ( - "darnit.", - "darnit_baseline.", - "darnit_testchecks.", -) -``` - -If your implementation uses `module:function` references, you'll need to add your -module prefix to this allowlist. Using short names (via `register_handler()`) avoids -this restriction entirely. +When a handler is referenced by `module:function` path in TOML, darnit imports it +only if the module's top-level package is `darnit` or the package of an installed +implementation, read from the `darnit.implementations` entry points. If your entry +point is `mystandard = "darnit_mystandard:register"`, any `darnit_mystandard.*` +module may be named; nothing needs to be added to darnit. Any other path is refused +with `HandlerImportRefused`, and an MCP tool that names one does not load +(framework-design.md 6.5). Short names (via `register_handler()`) need no import at +all and remain the recommended form. > **Reference**: See `packages/darnit-baseline/src/darnit_baseline/implementation.py:92` > for the OpenSSF Baseline's `register_handlers()` method and @@ -1755,15 +1748,12 @@ def my_handler(config: dict, context: HandlerContext) -> HandlerResult: ) ``` -### Module allowlist for dynamic loading - -If you reference handlers by `module:function` path in TOML, the handler registry -enforces a module allowlist. Your module must start with an approved prefix -(`darnit.`, `darnit_baseline.`, `darnit_testchecks.`). For new implementations, -either: +### Module path policy for dynamic loading -1. Use short names via `register_handler()` (recommended), or -2. Add your module prefix to `HandlerRegistry.ALLOWED_MODULE_PREFIXES` +A `module:function` handler path must name a module in `darnit` or in your own +implementation's package (the module of your `darnit.implementations` entry point). +A path into any other package is refused at load time. Prefer short names via +`register_handler()`. ### TOML path resolution diff --git a/docs/SECURITY_GUIDE.md b/docs/SECURITY_GUIDE.md index 12a44bed..58b79fed 100644 --- a/docs/SECURITY_GUIDE.md +++ b/docs/SECURITY_GUIDE.md @@ -18,67 +18,25 @@ This document describes security considerations, best practices, and configurati ## Dynamic Module Loading Security -Darnit uses dynamic module loading to instantiate adapters defined in configuration files. To prevent arbitrary code execution, **module paths are validated against a allowlist** before loading. +Darnit can import a Python attribute named in configuration as `package.module:attribute`: MCP tool handlers in `[mcp.tools]`, handler references, and `type = "python"` adapters. Every such import goes through one function, `darnit.core.handlers.resolve_module_path` (framework-design.md 6.5). -### Allowed Module Prefixes +### Resolution Policy -By default, only modules from these prefixes can be dynamically loaded: +- The path must be `a.b.c:attr`, with every part a Python identifier. Relative paths, empty parts, and dotted attributes are refused. +- The module's top-level package must be `darnit`, or the package of an implementation that discovery loaded from the `darnit.implementations` entry points (`darnit_csl:register` allows `darnit_csl.*`). +- The allowed set is derived from installed entry point metadata. There is no list to extend: installing an implementation package allows its modules, and nothing else does. +- The check runs before the import, so a refused module is never imported. -```python -ALLOWED_MODULE_PREFIXES = ( - "darnit.", - "darnit_baseline.", - "darnit_plugins.", - "darnit_testchecks.", -) -``` - -### Security Implications - -- **Configuration-defined adapters** must reference modules within the allowed prefixes -- **Malicious configurations** cannot load arbitrary Python code -- **Custom adapters** must be installed as proper Python packages with `darnit_` prefix - -### Extending the Whitelist +### Failure Behavior -If you need to use custom adapters from your own packages, you have two options: +- A refused path raises `HandlerImportRefused` (a `ValueError`) naming the path and the allowed packages, logged at WARNING. +- At MCP server start, a tool whose handler is refused is not registered, and the server logs an ERROR naming the tool. -#### Option 1: Use the `darnit_` Prefix Convention (Recommended) - -Name your custom adapter package with the `darnit_` prefix: - -``` -darnit_mycompany/ -├── adapters/ -│ └── custom.py -└── __init__.py -``` - -This automatically allows your module to be loaded: - -```toml -# Framework TOML shipped in your plugin package -[adapters.mycompany] -type = "python" -module = "darnit_mycompany.adapters.custom" -class = "MyCustomAdapter" -``` - -#### Option 2: Modify the Whitelist (Advanced) - -For enterprise deployments, you can subclass `AdapterRegistry` or `PluginRegistry` to extend the allowlist: - -```python -from darnit.core.registry import PluginRegistry - -class EnterprisePluginRegistry(PluginRegistry): - ALLOWED_MODULE_PREFIXES = PluginRegistry.ALLOWED_MODULE_PREFIXES + ( - "mycompany.", - "mycompany_compliance.", - ) -``` +### Security Implications -> **Warning**: Extending the allowlist increases your attack surface. Only add trusted module prefixes. +- A configuration string cannot reach `os`, `subprocess`, or any other package that is not darnit or an installed implementation. +- The policy is not a sandbox. Allowed packages are code the operator installed, and `[mcp.tools]` is read only from an installed framework TOML or an operator-supplied `darnit serve `, never from the audited repository. +- To use your own adapter or tool module, ship it in a package registered under `darnit.implementations`. --- @@ -437,18 +395,7 @@ dev_verifier = PluginVerifier(dev_config) ### Handler Registration Security -Plugins register handlers using the `@register_handler` decorator. Only modules matching the allowlist can register handlers. - -#### Allowlist - -```python -ALLOWED_MODULE_PREFIXES = ( - "darnit.", # Core framework - "darnit_baseline.", # OpenSSF Baseline implementation - "darnit_plugins.", # Official plugins - "darnit_testchecks.",# Test utilities -) -``` +Plugins register handlers using the `@register_handler` decorator or `register_handlers()`. Registration needs no import by name. A handler referenced by `module:function` path instead is resolved under the policy in [Dynamic Module Loading Security](#dynamic-module-loading-security). #### Registering Handlers diff --git a/packages/darnit/src/darnit/core/adapters.py b/packages/darnit/src/darnit/core/adapters.py index a9e8d26c..e5b0f40c 100644 --- a/packages/darnit/src/darnit/core/adapters.py +++ b/packages/darnit/src/darnit/core/adapters.py @@ -7,7 +7,6 @@ - Resolution functions for loading adapters from configuration """ -import importlib import json import logging import subprocess @@ -15,6 +14,7 @@ from dataclasses import dataclass, field from typing import Any +from darnit.core.handlers import resolve_module_path from darnit.core.models import ( AdapterCapability, CheckResult, @@ -453,14 +453,6 @@ class AdapterRegistry: # Config-based adapter definitions _adapter_configs: dict[str, dict[str, Any]] = field(default_factory=dict) - # Allowed module prefixes for dynamic imports (security allowlist) - ALLOWED_MODULE_PREFIXES: tuple = ( - "darnit.", - "darnit_baseline.", - "darnit_plugins.", - "darnit_testchecks.", - ) - def register_check_adapter( self, name: str, @@ -646,6 +638,10 @@ def _load_python_adapter( Returns: Adapter instance or None + + Raises: + HandlerImportRefused: If the module is outside the module + resolution policy """ module_path = config.get("module") class_name = config.get("class", "Adapter") @@ -654,17 +650,8 @@ def _load_python_adapter( logger.error(f"Adapter {name} missing 'module' in config") return None - # Security: Validate module path against allowlist to prevent arbitrary code loading - if not any(module_path.startswith(prefix) for prefix in self.ALLOWED_MODULE_PREFIXES): - logger.error( - f"Adapter {name}: module '{module_path}' not in allowed prefixes. " - f"Allowed: {self.ALLOWED_MODULE_PREFIXES}" - ) - return None - try: - module = importlib.import_module(module_path) - adapter_class = getattr(module, class_name) + adapter_class = resolve_module_path(f"{module_path}:{class_name}") if not issubclass(adapter_class, expected_type): logger.error( diff --git a/packages/darnit/src/darnit/core/discovery.py b/packages/darnit/src/darnit/core/discovery.py index 43558fb3..d506fb24 100644 --- a/packages/darnit/src/darnit/core/discovery.py +++ b/packages/darnit/src/darnit/core/discovery.py @@ -14,16 +14,18 @@ # Cache for discovered implementations _implementations: dict[str, ComplianceImplementation] | None = None +_implementation_packages: frozenset[str] = frozenset() def discover_implementations() -> dict[str, ComplianceImplementation]: """Discover compliance implementations from entry points.""" - global _implementations + global _implementations, _implementation_packages if _implementations is not None: return _implementations _implementations = {} + packages: set[str] = set() # Use importlib.metadata for Python 3.9+ from importlib.metadata import entry_points @@ -66,6 +68,7 @@ def discover_implementations() -> dict[str, ComplianceImplementation]: if isinstance(impl, ComplianceImplementation): _implementations[impl.name] = impl + packages.add(ep.module.partition(".")[0]) logger.info(f"Discovered implementation: {impl.name} v{impl.version}") else: logger.warning( @@ -80,10 +83,22 @@ def discover_implementations() -> dict[str, ComplianceImplementation]: logger.error(f"Error occurred while verifying or loading plugin '{ep.name}': {e}") continue + _implementation_packages = frozenset(packages) logger.info(f"Discovered {len(_implementations)} implementation(s)") return _implementations +def implementation_packages() -> frozenset[str]: + """Top-level packages of the discovered implementations' entry points. + + Read from entry point metadata (``darnit_csl:register`` gives + ``darnit_csl``), so the framework names no implementation package + itself. Only implementations that discovery accepted are included. + """ + discover_implementations() + return _implementation_packages + + def get_implementation(name: str) -> ComplianceImplementation | None: """Get a specific implementation by name. @@ -165,13 +180,15 @@ def clear_cache() -> None: Useful for testing or when implementations may have changed. """ - global _implementations + global _implementations, _implementation_packages _implementations = None + _implementation_packages = frozenset() __all__ = [ "clear_cache", "discover_implementations", "get_implementation", + "implementation_packages", "register_implementation_handlers", ] diff --git a/packages/darnit/src/darnit/core/handlers.py b/packages/darnit/src/darnit/core/handlers.py index 8833ae61..1817ad85 100644 --- a/packages/darnit/src/darnit/core/handlers.py +++ b/packages/darnit/src/darnit/core/handlers.py @@ -30,13 +30,10 @@ def check_branch_protection(owner: str, repo: str, local_path: Path, config: dic import importlib import logging -from collections.abc import Callable +from collections.abc import Callable, Iterable from dataclasses import dataclass from pathlib import Path -from typing import TYPE_CHECKING, Any, TypeVar - -if TYPE_CHECKING: - from collections.abc import Iterable +from typing import Any, TypeVar logger = logging.getLogger(__name__) @@ -173,6 +170,10 @@ def get_handler(self, name: str) -> Callable[..., Any] | None: Returns: Handler function or None if not found + + Raises: + HandlerImportRefused: If ``name`` is a module path the resolution + policy refuses (see :func:`resolve_module_path`) """ # First check the registry if name in self._handlers: @@ -202,41 +203,17 @@ def list_handlers(self, plugin: str | None = None) -> list[HandlerInfo]: handlers = [h for h in handlers if h.plugin == plugin] return handlers - # Allowed module prefixes for handler imports (security allowlist) - # Only modules starting with these prefixes can be dynamically loaded - ALLOWED_MODULE_PREFIXES = ( - "darnit.", - "darnit_baseline.", - "darnit_example.", - "darnit_testchecks.", - ) - def _load_handler_from_path(self, path: str) -> Callable[..., Any] | None: - """Load handler from module:function path. + """Load a handler from a module:function path under the resolution policy. - Security: Only modules matching ALLOWED_MODULE_PREFIXES can be loaded - to prevent arbitrary code execution via malicious module paths. + Returns None when an allowed module or its attribute does not exist. - Args: - path: String in format "module.path:function_name" - - Returns: - Handler function or None if loading fails or module not allowed + Raises: + HandlerImportRefused: If the policy refuses the path """ try: - module_path, func_name = path.rsplit(":", 1) - - # Validate module path against allowlist to prevent arbitrary imports - if not any(module_path.startswith(prefix) for prefix in self.ALLOWED_MODULE_PREFIXES): - logger.warning( - f"Module path '{module_path}' not in allowed prefixes: " - f"{self.ALLOWED_MODULE_PREFIXES}" - ) - return None - - module = importlib.import_module(module_path) - return getattr(module, func_name, None) - except (ValueError, ImportError, AttributeError) as e: + return resolve_module_path(path) + except (ImportError, AttributeError) as e: logger.warning(f"Failed to load handler from path '{path}': {e}") return None @@ -389,6 +366,63 @@ def clear(self) -> None: self._current_plugin = None +# ============================================================================= +# Module Path Resolution +# ============================================================================= + +CORE_PACKAGE = "darnit" + + +class HandlerImportRefused(ValueError): + """A ``module:attribute`` path the module resolution policy refuses.""" + + def __init__(self, path: str, reason: str, allowed: Iterable[str]) -> None: + self.path = path + self.allowed = tuple(sorted(allowed)) + super().__init__( + f"Refused to import '{path}': {reason}. " + f"Allowed top-level packages: {', '.join(self.allowed)}" + ) + + +def allowed_module_packages() -> frozenset[str]: + """Top-level packages a configured module path may name: core plus installed implementations.""" + from darnit.core.discovery import implementation_packages + + return frozenset({CORE_PACKAGE}) | implementation_packages() + + +def _refuse(path: str, reason: str) -> HandlerImportRefused: + error = HandlerImportRefused(path, reason, allowed_module_packages()) + logger.warning(str(error)) + return error + + +def resolve_module_path(path: str) -> Any: + """Import the attribute named by ``package.module:attribute``. + + This is the only place the framework imports a module named by + configuration. The module's top-level package must be ``darnit`` or the + package of an implementation discovered from the ``darnit.implementations`` + entry points; anything else is refused before it is imported. + + Raises: + HandlerImportRefused: If the path is malformed or outside the policy + ImportError: If an allowed module cannot be imported + AttributeError: If the module has no such attribute + """ + module_path, sep, attr = path.rpartition(":") + parts = module_path.split(".") + if not sep or ":" in module_path or not attr.isidentifier() or not all(part.isidentifier() for part in parts): + raise _refuse(path, "not of the form 'package.module:attribute'") + + if parts[0] != CORE_PACKAGE and parts[0] not in allowed_module_packages(): + raise _refuse(path, f"'{parts[0]}' is neither darnit nor an installed darnit implementation") + + module = importlib.import_module(module_path) + return getattr(module, attr) + + # ============================================================================= # Global Registry Instance # ============================================================================= @@ -492,6 +526,7 @@ def get_template(name: str) -> TemplateInfo | None: __all__ = [ # Classes + "HandlerImportRefused", "HandlerRegistry", "HandlerInfo", "PassInfo", @@ -504,4 +539,6 @@ def get_template(name: str) -> TemplateInfo | None: "get_handler", "list_handlers", "get_template", + "allowed_module_packages", + "resolve_module_path", ] diff --git a/packages/darnit/src/darnit/core/registry.py b/packages/darnit/src/darnit/core/registry.py index a944b29f..5954a8d6 100644 --- a/packages/darnit/src/darnit/core/registry.py +++ b/packages/darnit/src/darnit/core/registry.py @@ -42,7 +42,6 @@ from __future__ import annotations -import importlib import logging from collections.abc import Callable from dataclasses import dataclass, field @@ -52,6 +51,7 @@ ) from .adapters import CheckAdapter, RemediationAdapter +from .handlers import resolve_module_path from .models import AdapterCapability logger = logging.getLogger(__name__) @@ -219,14 +219,6 @@ class PluginRegistry: # Discovery state _discovered: set[str] = field(default_factory=set) - # Allowed module prefixes for dynamic imports (security allowlist) - ALLOWED_MODULE_PREFIXES: tuple = ( - "darnit.", - "darnit_baseline.", - "darnit_plugins.", - "darnit_testchecks.", - ) - # ========================================================================= # Discovery Methods # ========================================================================= @@ -809,17 +801,8 @@ def _load_python_adapter( logger.error(f"Adapter {name} missing 'module' in config") return None - # Security: Validate module path against allowlist to prevent arbitrary code loading - if not any(module_path.startswith(prefix) for prefix in self.ALLOWED_MODULE_PREFIXES): - logger.error( - f"Adapter {name}: module '{module_path}' not in allowed prefixes. " - f"Allowed: {self.ALLOWED_MODULE_PREFIXES}" - ) - return None - try: - module = importlib.import_module(module_path) - adapter_class = getattr(module, class_name) + adapter_class = resolve_module_path(f"{module_path}:{class_name}") return adapter_class() except ImportError as e: diff --git a/packages/darnit/src/darnit/server/factory.py b/packages/darnit/src/darnit/server/factory.py index 6fc2e1d4..fb19baaa 100644 --- a/packages/darnit/src/darnit/server/factory.py +++ b/packages/darnit/src/darnit/server/factory.py @@ -14,6 +14,8 @@ if TYPE_CHECKING: from mcp.server.fastmcp import FastMCP +from darnit.core.handlers import HandlerImportRefused + from .registry import ToolRegistry logger = logging.getLogger(__name__) @@ -193,6 +195,9 @@ def create_server( server.add_tool(handler, name=name, description=spec.description) registered_count += 1 logger.debug(f"Registered tool: {name}") + except HandlerImportRefused as e: + logger.error(f"Refused to load tool '{name}': {e}") + continue except (ImportError, AttributeError, ValueError) as e: logger.warning(f"Failed to load tool '{name}': {e}") continue @@ -258,6 +263,8 @@ def create_server_from_dict(config: dict) -> FastMCP: if spec.parameters: handler = _bind_tool_config(handler, spec.parameters) server.add_tool(handler, name=name, description=spec.description) + except HandlerImportRefused as e: + logger.error(f"Refused to load tool '{name}': {e}") except (ImportError, AttributeError, ValueError) as e: logger.warning(f"Failed to load tool '{name}': {e}") diff --git a/packages/darnit/src/darnit/server/registry.py b/packages/darnit/src/darnit/server/registry.py index 55917ed9..c5a2839b 100644 --- a/packages/darnit/src/darnit/server/registry.py +++ b/packages/darnit/src/darnit/server/registry.py @@ -6,12 +6,12 @@ Supports three handler resolution modes: 1. builtin = "audit" — uses framework-provided generic tool 2. handler = "short_name" — looks up in handler registry -3. handler = "module.path:function_name" — imports directly +3. handler = "module.path:function_name" — imported under the module + resolution policy (darnit or an installed implementation's package) """ from __future__ import annotations -import importlib from collections.abc import Callable from dataclasses import dataclass, field from typing import Any @@ -113,7 +113,8 @@ def load_handler( Supports three formats: 1. Built-in: builtin = "audit" - uses framework-provided generic tool 2. Short name: "audit_openssf_baseline" - looks up in handler registry - 3. Module path: "module.path:function_name" - imports directly + 3. Module path: "module.path:function_name" - imported under the + module resolution policy Args: spec: Tool specification containing the handler name or import path @@ -123,6 +124,8 @@ def load_handler( The imported function Raises: + HandlerImportRefused: If the module path is outside the module + resolution policy (``darnit.core.handlers.resolve_module_path``) ValueError: If handler cannot be resolved ImportError: If module cannot be imported AttributeError: If function doesn't exist in module @@ -146,10 +149,9 @@ def load_handler( "full module path 'module.path:function_name'" ) - # Full module path format - module_path, func_name = spec.handler.rsplit(":", 1) - module = importlib.import_module(module_path) - return getattr(module, func_name) + from darnit.core.handlers import resolve_module_path + + return resolve_module_path(spec.handler) def _load_builtin( self, spec: ToolSpec, framework_name: str | None diff --git a/tests/darnit/core/test_handlers.py b/tests/darnit/core/test_handlers.py index b56d57c9..4dc94858 100644 --- a/tests/darnit/core/test_handlers.py +++ b/tests/darnit/core/test_handlers.py @@ -1,8 +1,12 @@ """Tests for handler registration system.""" +import re from pathlib import Path +import pytest + from darnit.core.handlers import ( + HandlerImportRefused, HandlerRegistry, TemplateInfo, get_handler, @@ -98,22 +102,16 @@ def test_load_handler_from_module_path(self) -> None: assert handler is not None assert callable(handler) - def test_load_handler_blocked_module_path(self) -> None: - """Test that non-allowlisted module paths are blocked.""" + def test_load_handler_refused_module_path(self) -> None: + """A module outside the resolution policy is refused, not "not found".""" registry = HandlerRegistry() - # os.path is not in ALLOWED_MODULE_PREFIXES, should be blocked - handler = registry._load_handler_from_path("os.path:exists") - assert handler is None + for path in ("os.path:exists", "subprocess:run", "invalid"): + with pytest.raises(HandlerImportRefused, match=re.escape(path)): + registry._load_handler_from_path(path) - # subprocess is not allowed either - handler = registry._load_handler_from_path("subprocess:run") - assert handler is None - - def test_load_handler_invalid_path(self) -> None: - """Test loading handler from invalid path returns None.""" + def test_load_handler_missing_module_returns_none(self) -> None: registry = HandlerRegistry() - assert registry._load_handler_from_path("invalid") is None assert registry._load_handler_from_path("darnit.nonexistent.module:func") is None @@ -297,26 +295,25 @@ def test_resolve_baseline_handler_by_module_path(self) -> None: assert handler is not None assert callable(handler) - def test_resolve_blocked_module_path(self) -> None: - """Test that non-allowlisted modules are blocked.""" + def test_resolve_refused_module_path(self) -> None: + """Arbitrary modules are refused with an error naming the path.""" registry = HandlerRegistry() - # Should block arbitrary modules - handler = registry.get_handler("os:system") - assert handler is None + for path in ("os:system", "subprocess:run"): + with pytest.raises(HandlerImportRefused, match=re.escape(path)): + registry.get_handler(path) - handler = registry.get_handler("subprocess:run") - assert handler is None - - def test_allowlist_includes_darnit_packages(self) -> None: - """Test that allowlist includes all darnit package prefixes.""" - from darnit.core.handlers import HandlerRegistry - - allowed = HandlerRegistry.ALLOWED_MODULE_PREFIXES + def test_resolve_other_installed_implementation_modules(self) -> None: + """Every installed implementation's package resolves, not only a fixed few.""" + registry = HandlerRegistry() - assert "darnit." in allowed - assert "darnit_baseline." in allowed - assert "darnit_testchecks." in allowed + for path in ( + "darnit_gittuf.implementation:GittufImplementation", + "darnit_reproducibility.implementation:ReproducibilityImplementation", + "darnit_hello.implementation:HelloImplementation", + "darnit_csl.mcp_tools:remediate_community_spec", + ): + assert callable(registry.get_handler(path)) def test_get_handler_with_colon_tries_module_resolution(self) -> None: """Test that handler names with ':' trigger module resolution.""" diff --git a/tests/darnit/core/test_module_path_policy.py b/tests/darnit/core/test_module_path_policy.py new file mode 100644 index 00000000..27032656 --- /dev/null +++ b/tests/darnit/core/test_module_path_policy.py @@ -0,0 +1,153 @@ +"""One module-path resolution policy for every ``module:attribute`` import (#490).""" + +from __future__ import annotations + +import logging +import re +from pathlib import Path + +import pytest + +from darnit.core import discovery +from darnit.core.handlers import HandlerImportRefused, HandlerRegistry, resolve_module_path + +REPO_ROOT = Path(__file__).resolve().parents[3] +CORE_SRC = REPO_ROOT / "packages" / "darnit" / "src" / "darnit" + + +@pytest.fixture(autouse=True) +def fresh_discovery(): + discovery.clear_cache() + yield + discovery.clear_cache() + + +class TestAccepted: + @pytest.mark.parametrize( + "path", + [ + "darnit.core.logging:get_logger", + "darnit_baseline.tools:audit_openssf_baseline", + "darnit_csl.mcp_tools:remediate_community_spec", + "darnit_gittuf.implementation:GittufImplementation", + "darnit_reproducibility.implementation:ReproducibilityImplementation", + "darnit_hello.implementation:HelloImplementation", + ], + ) + def test_core_and_installed_implementation_modules_resolve(self, path: str) -> None: + assert callable(resolve_module_path(path)) + + def test_allowed_set_comes_from_implementation_entry_points(self) -> None: + packages = discovery.implementation_packages() + + assert {"darnit_baseline", "darnit_csl", "darnit_gittuf", "darnit_reproducibility", "darnit_hello"} <= packages + assert "darnit_testchecks" not in packages + + def test_allowed_set_is_recomputed_after_discovery_reset(self, monkeypatch: pytest.MonkeyPatch) -> None: + import importlib.metadata + + assert callable(resolve_module_path("darnit_csl.mcp_tools:remediate_community_spec")) + + discovery.clear_cache() + monkeypatch.setattr(importlib.metadata, "entry_points", lambda **_: []) + with pytest.raises(HandlerImportRefused): + resolve_module_path("darnit_csl.mcp_tools:remediate_community_spec") + assert callable(resolve_module_path("darnit.core.logging:get_logger")) + + monkeypatch.undo() + discovery.clear_cache() + assert callable(resolve_module_path("darnit_csl.mcp_tools:remediate_community_spec")) + + +class TestRefused: + @pytest.mark.parametrize( + "path", + [ + "os:system", + "subprocess:run", + "builtins:eval", + "os.path:exists", + "darnit_not_installed_xyz.tools:run", + "darnitx.core:thing", + "darnit_testchecks.adapters.builtin:TestCheckAdapter", + ], + ) + def test_module_outside_policy_is_refused(self, path: str) -> None: + with pytest.raises(HandlerImportRefused) as exc: + resolve_module_path(path) + assert isinstance(exc.value, ValueError) + assert path in str(exc.value) + assert "darnit_csl" in str(exc.value) + + @pytest.mark.parametrize( + "path", + [ + "invalid", + ":get_logger", + "darnit.core.logging:", + ".darnit.core.logging:get_logger", + "darnit..core.logging:get_logger", + "darnit.core.logging.:get_logger", + "darnit.core:logging:get_logger", + "darnit.core.logging:get_logger.__globals__", + "darnit/core/logging:get_logger", + ], + ) + def test_malformed_path_is_refused(self, path: str) -> None: + with pytest.raises(HandlerImportRefused, match=re.escape(path)): + resolve_module_path(path) + + def test_refusal_is_logged_at_warning(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING, logger="darnit.core.handlers"): + with pytest.raises(HandlerImportRefused): + resolve_module_path("os:system") + assert any(r.levelno == logging.WARNING and "os:system" in r.getMessage() for r in caplog.records) + + def test_refused_module_is_never_imported(self, monkeypatch: pytest.MonkeyPatch) -> None: + import importlib + + imported: list[str] = [] + real = importlib.import_module + monkeypatch.setattr(importlib, "import_module", lambda name, *a: imported.append(name) or real(name, *a)) + + with pytest.raises(HandlerImportRefused): + resolve_module_path("darnit_not_installed_xyz.tools:run") + assert imported == [] + + +class TestNotFound: + def test_allowed_module_that_does_not_exist_raises_import_error(self) -> None: + with pytest.raises(ImportError): + resolve_module_path("darnit.nonexistent_module_xyz:func") + + def test_allowed_module_without_the_attribute_raises_attribute_error(self) -> None: + with pytest.raises(AttributeError): + resolve_module_path("darnit.core.logging:nonexistent_function_xyz") + + +class TestOnePolicy: + def test_no_prefix_allowlist_remains(self) -> None: + from darnit.core.adapters import AdapterRegistry + from darnit.core.registry import PluginRegistry + + for owner in (HandlerRegistry, PluginRegistry, AdapterRegistry): + assert not hasattr(owner, "ALLOWED_MODULE_PREFIXES") + offenders = [p for p in CORE_SRC.rglob("*.py") if "ALLOWED_MODULE_PREFIXES" in p.read_text(encoding="utf-8")] + assert offenders == [] + + def test_core_imports_by_name_in_one_place(self) -> None: + sites = [ + p.relative_to(CORE_SRC).as_posix() + for p in CORE_SRC.rglob("*.py") + if "import_module(" in p.read_text(encoding="utf-8") + ] + assert sites == ["core/handlers.py"] + + def test_adapter_loaders_use_the_policy(self) -> None: + from darnit.core.adapters import AdapterRegistry, CheckAdapter + from darnit.core.registry import PluginRegistry + + with pytest.raises(HandlerImportRefused): + AdapterRegistry()._load_python_adapter("x", {"module": "os", "class": "system"}, CheckAdapter) + with pytest.raises(HandlerImportRefused): + PluginRegistry()._load_python_adapter("x", {"module": "os", "class": "system"}) diff --git a/tests/darnit/server/test_factory.py b/tests/darnit/server/test_factory.py index 5553388d..ef2910f6 100644 --- a/tests/darnit/server/test_factory.py +++ b/tests/darnit/server/test_factory.py @@ -1,5 +1,8 @@ """Tests for darnit.server.factory module.""" +import asyncio +import logging +import tomllib from pathlib import Path import pytest @@ -41,7 +44,7 @@ def test_registers_tools(self): "name": "test-server", "tools": { "my_tool": { - "handler": "json:dumps", + "handler": "darnit.core.logging:get_logger", "description": "Serialize to JSON", } }, @@ -69,9 +72,9 @@ def test_loads_from_toml_file(self, tmp_path): [mcp] name = "from-file-server" -[mcp.tools.json_dump] -handler = "json:dumps" -description = "JSON serializer" +[mcp.tools.get_logger] +handler = "darnit.core.logging:get_logger" +description = "Logger" ''') server = create_server(str(config_path)) assert server.name == "from-file-server" @@ -87,25 +90,57 @@ def test_loads_path_object(self, tmp_path): assert server.name == "path-server" def test_handles_invalid_handler(self, tmp_path, caplog): - """Test that invalid handlers are skipped with warning.""" + """A missing handler is skipped with a warning; the other tools still load.""" config_path = tmp_path / "test.toml" config_path.write_text(''' [mcp] name = "test-server" [mcp.tools.valid_tool] -handler = "json:dumps" +handler = "darnit.core.logging:get_logger" description = "Valid tool" [mcp.tools.invalid_tool] -handler = "nonexistent_module:func" +handler = "darnit.nonexistent_module:func" description = "Invalid tool" ''') - server = create_server(str(config_path)) - # Server should still be created - assert server.name == "test-server" - # Should log warning about invalid tool - assert "Failed to load tool" in caplog.text or True # May not have logging configured + with caplog.at_level(logging.WARNING): + server = create_server(str(config_path)) + tools = {tool.name for tool in asyncio.run(server.list_tools())} + assert "valid_tool" in tools + assert "invalid_tool" not in tools + assert "Failed to load tool 'invalid_tool'" in caplog.text + + @pytest.mark.parametrize("handler", ["os:system", "subprocess:run", "darnit_not_installed_xyz.tools:run"]) + def test_refused_handler_is_reported_at_startup(self, tmp_path, caplog, handler): + """A tool whose module path the policy refuses does not load, and startup says so.""" + config_path = tmp_path / "test.toml" + config_path.write_text(f''' +[mcp] +name = "test-server" + +[mcp.tools.valid_tool] +handler = "darnit.core.logging:get_logger" +description = "Valid tool" + +[mcp.tools.refused_tool] +handler = "{handler}" +description = "Refused tool" +''') + with caplog.at_level(logging.WARNING): + server = create_server(str(config_path)) + tools = {tool.name for tool in asyncio.run(server.list_tools())} + assert "valid_tool" in tools + assert "refused_tool" not in tools + refusals = [r for r in caplog.records if r.levelno == logging.ERROR and "refused_tool" in r.getMessage()] + assert refusals and handler in refusals[0].getMessage() + + def test_refused_handler_is_reported_from_dict(self, caplog): + config = {"mcp": {"tools": {"refused_tool": {"handler": "os:system", "description": "x"}}}} + with caplog.at_level(logging.WARNING): + server = create_server_from_dict(config) + assert "refused_tool" not in {tool.name for tool in asyncio.run(server.list_tools())} + assert any(r.levelno == logging.ERROR and "refused_tool" in r.getMessage() for r in caplog.records) def test_openssf_baseline_toml(self): """Test loading the actual openssf-baseline.toml file.""" @@ -127,3 +162,32 @@ def test_openssf_baseline_toml(self): pytest.skip("darnit_baseline not installed") else: pytest.skip("openssf-baseline.toml not found") + + +def _shipped_framework_configs() -> list[tuple[str, Path]]: + from darnit.core.discovery import discover_implementations + + configs = [] + for name, impl in sorted(discover_implementations().items()): + path = impl.get_framework_config_path() + if path is not None and Path(path).is_file(): + configs.append((name, Path(path))) + return configs + + +SHIPPED_FRAMEWORKS = _shipped_framework_configs() + + +class TestShippedFrameworkTools: + """Every [mcp.tools] entry of every installed framework loads (#490).""" + + def test_the_community_spec_module_path_tool_is_among_them(self): + assert "community-spec" in dict(SHIPPED_FRAMEWORKS) + + @pytest.mark.parametrize(("name", "config_path"), SHIPPED_FRAMEWORKS, ids=[name for name, _ in SHIPPED_FRAMEWORKS]) + def test_all_declared_tools_register(self, name, config_path, caplog): + declared = set(tomllib.loads(config_path.read_text(encoding="utf-8")).get("mcp", {}).get("tools", {})) + with caplog.at_level(logging.WARNING, logger="darnit"): + server = create_server(config_path) + registered = {tool.name for tool in asyncio.run(server.list_tools())} + assert declared <= registered, f"{name}: {sorted(declared - registered)} did not load\n{caplog.text}" diff --git a/tests/darnit/server/test_registry.py b/tests/darnit/server/test_registry.py index 01ff7922..693e71f4 100644 --- a/tests/darnit/server/test_registry.py +++ b/tests/darnit/server/test_registry.py @@ -1,7 +1,10 @@ """Tests for darnit.server.registry module.""" +import re + import pytest +from darnit.core.handlers import HandlerImportRefused from darnit.server.registry import ToolRegistry, ToolSpec @@ -148,16 +151,36 @@ def test_list_tools(self): def test_load_handler_success(self): """Test successfully loading a handler function.""" + from darnit.core.logging import get_logger + spec = ToolSpec( name="test", - handler="json:dumps", # json.dumps is a real function + handler="darnit.core.logging:get_logger", description="Test", ) registry = ToolRegistry() - handler = registry.load_handler(spec) - assert callable(handler) - # Verify it's actually json.dumps - assert handler([1, 2, 3]) == "[1, 2, 3]" + assert registry.load_handler(spec) is get_logger + + def test_load_handler_from_installed_implementation(self): + """The shipped community-spec tool names a module of its own package.""" + from darnit_csl.mcp_tools import remediate_community_spec + + spec = ToolSpec( + name="remediate_community_spec", + handler="darnit_csl.mcp_tools:remediate_community_spec", + description="Test", + ) + assert ToolRegistry().load_handler(spec) is remediate_community_spec + + @pytest.mark.parametrize( + "handler", + ["os:system", "subprocess:run", "json:dumps", "darnit_not_installed_xyz.tools:run"], + ) + def test_load_handler_refuses_module_outside_policy(self, handler): + """A module path outside darnit and the installed implementations is never imported.""" + spec = ToolSpec(name="test", handler=handler, description="Test") + with pytest.raises(HandlerImportRefused, match=re.escape(handler)): + ToolRegistry().load_handler(spec) def test_load_handler_invalid_format(self): """Test loading handler with invalid format raises ValueError.""" @@ -174,7 +197,7 @@ def test_load_handler_module_not_found(self): """Test loading handler from non-existent module raises ImportError.""" spec = ToolSpec( name="test", - handler="nonexistent_module_xyz:func", + handler="darnit.nonexistent_module_xyz:func", description="Test", ) registry = ToolRegistry() @@ -185,7 +208,7 @@ def test_load_handler_function_not_found(self): """Test loading non-existent function raises AttributeError.""" spec = ToolSpec( name="test", - handler="json:nonexistent_function_xyz", + handler="darnit.core.logging:nonexistent_function_xyz", description="Test", ) registry = ToolRegistry()