Harden M7.6/7.7: recursive restriction accumulation, MCP server enforcement, fast-fail authorization

src/cmdforge/providers.py

@@ -5,6 +5,7 @@ import re
 import shlex
 import subprocess
 import shutil
+import sys
 from dataclasses import dataclass, field
 from pathlib import Path
 from typing import Optional, List
@@ -215,6 +216,19 @@ class Provider:
     tools: Optional[List[str]] = None  # Allowed CmdForge tools (None = all)
     mcp_servers: Optional[List[str]] = None  # MCP servers available to this provider
 
+    def __post_init__(self) -> None:
+        for field_name, values in (
+            ("tools", self.tools),
+            ("mcp_servers", self.mcp_servers),
+        ):
+            if values is not None and (
+                not isinstance(values, list)
+                or not all(isinstance(value, str) and value for value in values)
+            ):
+                raise ValueError(
+                    f"Provider {field_name} must be a list of non-empty strings or null"
+                )
+
     def to_dict(self) -> dict:
         d = {
             "name": self.name,
@@ -238,9 +252,9 @@ class Provider:
             d["api_key_env"] = self.api_key_env
         if self.pty_config:
             d["pty_config"] = self.pty_config
-        if self.tools:
+        if self.tools is not None:
             d["tools"] = self.tools
-        if self.mcp_servers:
+        if self.mcp_servers is not None:
             d["mcp_servers"] = self.mcp_servers
         return d
 
@@ -419,8 +433,11 @@ def load_providers() -> List[Provider]:
         if not data or "providers" not in data:
             return DEFAULT_PROVIDERS.copy()
         providers = [Provider.from_dict(p) for p in data["providers"]]
-    except Exception:
-        return DEFAULT_PROVIDERS.copy()
+    except Exception as exc:
+        # A present but malformed configuration must not silently fall back to
+        # unrestricted defaults, particularly when access policies are invalid.
+        print(f"Warning: Failed to load provider configuration: {exc}", file=sys.stderr)
+        return []
 
     config_version = data.get("version", 1)
     if not isinstance(config_version, int):

src/cmdforge/runner.py

@@ -1,9 +1,11 @@
 """Tool execution engine."""
 
 import argparse
+import fnmatch
 import json
 import re
 import sys
+from dataclasses import replace
 from pathlib import Path
 from typing import Optional, List
 
@@ -737,7 +739,10 @@ def execute_tool_step(
     provider_override: Optional[str] = None,
     dry_run: bool = False,
     verbose: bool = False,
-    call_stack: Optional[list] = None
+    call_stack: Optional[list] = None,
+    agent_profile: Optional[str] = None,
+    agent_skills: Optional[List[str]] = None,
+    agent_tool_policies: Optional[List[List[str]]] = None,
 ) -> tuple[str, bool]:
     """
     Execute a tool step by calling another tool.
@@ -761,7 +766,15 @@ def execute_tool_step(
         _print_call_stack(call_stack, f"Maximum tool nesting depth ({MAX_TOOL_DEPTH}) exceeded")
         return "", False
 
-    # Resolve the tool reference
+    # Authorization must happen before resolution because resolution may fetch a
+    # missing tool from the registry.
+    effective_provider = step.provider or provider_override
+    if not _authorize_tool_call(
+        step.tool, effective_provider, agent_tool_policies, call_stack
+    ):
+        return "", False
+
+    # Resolve the tool reference only after authorization succeeds.
     try:
         resolved = resolve_tool(step.tool)
         nested_tool = resolved.tool
@@ -778,26 +791,6 @@ def execute_tool_step(
     for key, value in step.args.items():
         custom_args[key] = substitute_variables(str(value), variables, warn_non_scalar=verbose)
 
-    # Determine effective provider (step override > parent override)
-    effective_provider = step.provider or provider_override
-
-    # Check tool access if the provider restricts which tools it can call
-    if effective_provider:
-        from .providers import get_provider
-        provider_obj = get_provider(effective_provider)
-        if provider_obj and provider_obj.tools is not None:
-            allowed = provider_obj.tools
-            if step.tool not in allowed and not _tool_match_allowlist(step.tool, allowed):
-                _print_call_stack(
-                    call_stack,
-                    f"Provider '{effective_provider}' is not allowed to call tool '{step.tool}'",
-                )
-                print(
-                    f"Provider '{effective_provider}' tools allowlist: {allowed}",
-                    file=sys.stderr,
-                )
-                return "", False
-
     if verbose:
         print(f"[verbose] Tool step: calling {step.tool}", file=sys.stderr)
         print(f"[verbose]   Input length: {len(input_text)} chars", file=sys.stderr)
@@ -817,8 +810,11 @@ def execute_tool_step(
         verbose=verbose,
         _depth=depth + 1,
         _call_stack=call_stack,
-        agent_profile=step.profile,
-        agent_skills=step.skills,
+        agent_profile=step.profile if step.profile is not None else agent_profile,
+        agent_skills=step.skills if step.skills is not None else agent_skills,
+        agent_tool_policies=_extend_agent_tool_policies(
+            agent_tool_policies, step.tools
+        ),
     )
 
     return output, exit_code == 0
@@ -837,6 +833,7 @@ def run_tool(
     _call_stack: Optional[list] = None,
     agent_profile: Optional[str] = None,
     agent_skills: Optional[List[str]] = None,
+    agent_tool_policies: Optional[List[List[str]]] = None,
 ) -> tuple[str, int]:
     """
     Execute a tool.
@@ -1017,7 +1014,10 @@ def run_tool(
                 provider_override=provider_override,
                 dry_run=dry_run,
                 verbose=verbose,
-                call_stack=step_stack
+                call_stack=step_stack,
+                agent_profile=agent_profile,
+                agent_skills=agent_skills,
+                agent_tool_policies=agent_tool_policies,
             )
             if not success:
                 return "", 3
@@ -1033,6 +1033,11 @@ def run_tool(
             if dry_run:
                 variables[step.output_var] = f"[DRY RUN - would call mcp:{step.server}/{step.tool}]"
             else:
+                step_stack = _call_stack + [(tool.name, i + 1)]
+                if not _authorize_mcp_call(
+                    step.server, provider_override, step_stack
+                ):
+                    return "", 4
                 args = _substitute_mcp_args(step.arguments, variables)
                 try:
                     if mcp_manager is None:
@@ -1227,27 +1232,96 @@ def main():
     sys.exit(exit_code)
 
 
-def _tool_match_allowlist(name: str, allowed: list) -> bool:
-    for pattern in allowed:
-        if pattern == name:
-            return True
-        if pattern.endswith("*") and name.startswith(pattern[:-1]):
-            return True
-    return False
+def _matches_allowlist(name: str, allowed: List[str]) -> bool:
+    return any(fnmatch.fnmatchcase(name, pattern) for pattern in allowed)
+
+
+def _authorize_tool_call(
+    name: str,
+    provider_name: Optional[str],
+    agent_tool_policies: Optional[List[List[str]]],
+    call_stack: list,
+) -> bool:
+    """Enforce provider and delegated-agent tool policies."""
+    if provider_name:
+        from .providers import get_provider
+        provider = get_provider(provider_name)
+        if provider is None:
+            _print_call_stack(
+                call_stack,
+                f"Provider '{provider_name}' is not configured; refusing tool call '{name}'",
+            )
+            return False
+        if provider.tools is not None and not _matches_allowlist(name, provider.tools):
+            _print_call_stack(
+                call_stack,
+                f"Provider '{provider_name}' is not allowed to call tool '{name}'",
+            )
+            print(
+                f"Provider '{provider_name}' tools allowlist: {provider.tools}",
+                file=sys.stderr,
+            )
+            return False
+
+    for policy in agent_tool_policies or []:
+        if not _matches_allowlist(name, policy):
+            _print_call_stack(
+                call_stack,
+                f"Delegated agent is not allowed to call tool '{name}'",
+            )
+            print(f"Delegated agent tools allowlist: {policy}", file=sys.stderr)
+            return False
+    return True
+
+
+def _extend_agent_tool_policies(
+    policies: Optional[List[List[str]]],
+    additional: Optional[List[str]],
+) -> Optional[List[List[str]]]:
+    """Add a restriction without allowing descendants to broaden ancestors."""
+    if additional is None:
+        return policies
+    return [*(policies or []), additional]
+
+
+def _authorize_mcp_call(
+    server: str,
+    provider_name: Optional[str],
+    call_stack: list,
+) -> bool:
+    """Enforce the effective provider's MCP-server policy."""
+    if not provider_name:
+        return True
+
+    from .providers import get_provider
+    provider = get_provider(provider_name)
+    if provider is None:
+        _print_call_stack(
+            call_stack,
+            f"Provider '{provider_name}' is not configured; refusing MCP server '{server}'",
+        )
+        return False
+    if provider.mcp_servers is not None and not _matches_allowlist(
+        server, provider.mcp_servers
+    ):
+        _print_call_stack(
+            call_stack,
+            f"Provider '{provider_name}' is not allowed to use MCP server '{server}'",
+        )
+        print(
+            f"Provider '{provider_name}' MCP server allowlist: {provider.mcp_servers}",
+            file=sys.stderr,
+        )
+        return False
+    return True
 
 
 def _step_with_profile(step, profile_name):
-    from copy import copy
-    s = copy(step)
-    s.profile = profile_name
-    return s
+    return replace(step, profile=profile_name)
 
 
 def _step_with_skills(step, skill_names):
-    from copy import copy
-    s = copy(step)
-    s.skills = skill_names
-    return s
+    return replace(step, skills=skill_names)
 
 
 if __name__ == "__main__":

src/cmdforge/tool.py

@@ -17,6 +17,33 @@ TOOLS_DIR = Path.home() / ".cmdforge"
 BIN_DIR = Path.home() / ".local" / "bin"
 
 
+def _validate_skill_selection(skills: Optional[List[str]]) -> None:
+    """Validate a step's optional Agent Skills selection."""
+    if skills is None:
+        return
+    if not isinstance(skills, list):
+        raise ValueError("skills must be a list of skill names")
+    if any(not isinstance(name, str) for name in skills):
+        raise ValueError("skills entries must be strings")
+    if "*" in skills and skills != ["*"]:
+        raise ValueError("'*' must be the only entry when enabling all skills")
+    if len(skills) != len(set(skills)):
+        raise ValueError("skills must not contain duplicate names")
+
+    from .skills import _validate_skill_name
+    for name in skills:
+        if name != "*":
+            _validate_skill_name(name)
+
+
+def _validate_optional_patterns(patterns: Optional[List[str]], field_name: str) -> None:
+    if patterns is not None and (
+        not isinstance(patterns, list)
+        or not all(isinstance(pattern, str) and pattern for pattern in patterns)
+    ):
+        raise ValueError(f"{field_name} must be a list of non-empty strings or null")
+
+
 @dataclass
 class SystemDependency:
     """A system-level package dependency (apt, brew, pacman, etc.)."""
@@ -133,21 +160,7 @@ class PromptStep:
     skills: Optional[List[str]] = None  # Skill names to enable for this step
 
     def __post_init__(self) -> None:
-        if self.skills is None:
-            return
-        if not isinstance(self.skills, list):
-            raise ValueError("skills must be a list of skill names")
-        if any(not isinstance(name, str) for name in self.skills):
-            raise ValueError("skills entries must be strings")
-        if "*" in self.skills and self.skills != ["*"]:
-            raise ValueError("'*' must be the only entry when enabling all skills")
-        if len(self.skills) != len(set(self.skills)):
-            raise ValueError("skills must not contain duplicate names")
-
-        from .skills import _validate_skill_name
-        for name in self.skills:
-            if name != "*":
-                _validate_skill_name(name)
+        _validate_skill_selection(self.skills)
 
     def to_dict(self) -> dict:
         d = {
@@ -244,6 +257,15 @@ class ToolStep:
     profile: Optional[str] = None  # AI persona for the nested tool
     skills: Optional[List[str]] = None  # Skills to enable in the nested tool
     name: Optional[str] = None  # Optional display name for the step
+    tools: Optional[List[str]] = None  # Tools the delegated agent may call
+
+    def __post_init__(self) -> None:
+        _validate_skill_selection(self.skills)
+        _validate_optional_patterns(self.tools, "tools")
+        if self.profile is not None and (
+            not isinstance(self.profile, str) or not self.profile
+        ):
+            raise ValueError("profile must be a non-empty string or null")
 
     def to_dict(self) -> dict:
         d = {
@@ -261,6 +283,8 @@ class ToolStep:
             d["profile"] = self.profile
         if self.skills is not None:
             d["skills"] = self.skills
+        if self.tools is not None:
+            d["tools"] = self.tools
         if self.name:
             d["name"] = self.name
         return d
@@ -275,6 +299,7 @@ class ToolStep:
             provider=data.get("provider"),
             profile=data.get("profile"),
             skills=data.get("skills"),
+            tools=data.get("tools"),
             name=data.get("name")
         )