diff --git a/examples/supported/05_reported_failure.yaml b/examples/supported/05_reported_failure.yaml new file mode 100644 index 00000000..f706ac7c --- /dev/null +++ b/examples/supported/05_reported_failure.yaml @@ -0,0 +1,56 @@ +# Supported example 05: a step that fails without raising. +# +# There are two ways a step fails, and they are not the same shape. +# +# 06 covers the first: the step raises, the task ends FAILED, and the error is +# an exception. This example covers the second, which is easier to miss. A +# tool can decline its work and return `{"success": false, "error": ...}` +# without raising anything. The task then ends **completed** -- it did run to +# the end -- while the step did **not** succeed. +# +# Reading `status` alone reports this run as fine. Only the step's `success` +# flag says otherwise, and the pipeline must take the failure from there: this +# run exits 1 and reports success=false even though no task is FAILED and +# nothing raised. +# +# `read_missing` asks for a file that is not there. The filesystem tool +# reports that as a failed result rather than an exception. +id: reported_failure +name: Reported Failure +description: A tool that reports failure without raising, and a run that honours it +version: "1.0.0" + +parameters: + out_dir: + type: string + default: "./output" + missing_path: + type: string + default: "./output/does-not-exist.txt" + +steps: + - id: before + tool: filesystem + action: write + parameters: + path: "{{ out_dir }}/before.txt" + content: "this step succeeds" + + # Completes without raising, and reports that it did not succeed. + - id: read_missing + tool: filesystem + action: read + on_failure: continue + parameters: + path: "{{ missing_path }}" + dependencies: + - before + + - id: after + tool: filesystem + action: write + parameters: + path: "{{ out_dir }}/after.txt" + content: "the run continued" + dependencies: + - read_missing diff --git a/src/orchestrator/_api.py b/src/orchestrator/_api.py index 47cff749..e00f4a09 100644 --- a/src/orchestrator/_api.py +++ b/src/orchestrator/_api.py @@ -417,7 +417,7 @@ async def _resolve_outputs(self, inputs): outputs_def = self._extract_outputs() if outputs_def: - from jinja2 import Template + from .core.template_sandbox import sandboxed_template for name, value in outputs_def.items(): if isinstance(value, str): @@ -438,7 +438,7 @@ async def _resolve_outputs(self, inputs): else: # Regular template - render with current context try: - template = Template(value) + template = sandboxed_template(value) outputs[name] = template.render( inputs=inputs, outputs=outputs ) @@ -475,12 +475,12 @@ async def _resolve_runtime_templates( async def _resolve_task_templates(self, obj, context): """Recursively resolve templates in task parameters.""" - from jinja2 import Template + from .core.template_sandbox import sandboxed_template if isinstance(obj, str): if "{{" in obj and "}}" in obj: try: - template = Template(obj) + template = sandboxed_template(obj) return template.render(**context) except Exception: # If template resolution fails, return original diff --git a/src/orchestrator/compiler/yaml_compiler.py b/src/orchestrator/compiler/yaml_compiler.py index 203a4866..01cc9050 100644 --- a/src/orchestrator/compiler/yaml_compiler.py +++ b/src/orchestrator/compiler/yaml_compiler.py @@ -8,7 +8,9 @@ from typing import Any, Dict, List, Optional import yaml -from jinja2 import Environment, StrictUndefined +from jinja2 import StrictUndefined + +from ..core.template_sandbox import create_sandboxed_environment from ..core.actions import canonical_action from ..core.pipeline import Pipeline @@ -147,7 +149,7 @@ def __init__( self.ambiguity_resolver = None logger.info("No model registry provided - AUTO tags will be preserved") - self.template_engine = Environment(undefined=StrictUndefined) + self.template_engine = create_sandboxed_environment() # Add custom filters to Jinja2 environment self._register_custom_filters() diff --git a/src/orchestrator/core/output_tracker.py b/src/orchestrator/core/output_tracker.py index 41b19bf2..df342c9a 100644 --- a/src/orchestrator/core/output_tracker.py +++ b/src/orchestrator/core/output_tracker.py @@ -271,9 +271,9 @@ def get_template_variables(self) -> Dict[str, Any]: def resolve_template_string(self, template: str) -> str: """Resolve template string with tracked output values.""" - from jinja2 import Environment, StrictUndefined + from .template_sandbox import create_sandboxed_environment - env = Environment(undefined=StrictUndefined) + env = create_sandboxed_environment() template_obj = env.from_string(template) try: diff --git a/src/orchestrator/core/template_manager.py b/src/orchestrator/core/template_manager.py index affb112c..a3ca52bb 100644 --- a/src/orchestrator/core/template_manager.py +++ b/src/orchestrator/core/template_manager.py @@ -3,7 +3,11 @@ import asyncio import logging from typing import Any, Dict, List, Union, Optional -from jinja2 import Environment, StrictUndefined, Template, TemplateSyntaxError, UndefinedError +from jinja2 import StrictUndefined, Template, TemplateSyntaxError, UndefinedError + +from jinja2.sandbox import SecurityError + +from .template_sandbox import create_sandboxed_environment from jinja2.filters import FILTERS from datetime import datetime import json @@ -100,7 +104,7 @@ def __init__(self, debug_mode: bool = False, file_inclusion_processor: Optional[ self.loop_context_manager = loop_context_manager or GlobalLoopContextManager() # Set up Jinja2 environment with custom filters and undefined handling - self.env = Environment( + self.env = create_sandboxed_environment( undefined=StrictUndefined, trim_blocks=True, lstrip_blocks=True, @@ -538,6 +542,12 @@ def render(self, template_string: str, additional_context: Optional[Dict[str, An logger.warning(f"Variable '{undefined_var}' not found in context") return template_string + except SecurityError: + # A sandbox violation is never recoverable. Every other render + # failure below falls back to returning the template unrendered, + # which for this one would hand the caller back the payload it was + # refused -- and leave the refusal visible only in a log line. + raise except Exception as e: logger.error(f"Error rendering template: {e}") logger.error(f"Error type: {type(e).__name__}") diff --git a/src/orchestrator/core/template_sandbox.py b/src/orchestrator/core/template_sandbox.py new file mode 100644 index 00000000..8b96efc7 --- /dev/null +++ b/src/orchestrator/core/template_sandbox.py @@ -0,0 +1,45 @@ +"""One place that builds Jinja environments, and it builds sandboxed ones. + +A pipeline's `{{ }}` expressions are authored with the pipeline. The values +substituted into them are not: they arrive from `-i name=value`, from an +inputs file, or from an upstream step's output. Those values are themselves +rendered, which is a deliberate feature -- `-i out_dir='{{ base }}/reports'` +is useful -- but it means a parameter value is executed as a template. + +Jinja's stock `Environment` executes it with Python's object graph in reach. +`orchestrator run 01_hello_filesystem.yaml -i greeting='{{ "".__class__ }}'` +was enough to walk from a string literal to `__subclasses__()`, which is the +first hop of the standard Jinja sandbox escape. `SandboxedEnvironment` refuses +that traversal while leaving ordinary expressions -- arithmetic, filters, +`{{ step.result.content }}` -- working exactly as before. + +Every environment on the execution path is built here so a plain +`Environment(...)` cannot quietly reappear in one of them; `test_template_sandbox.py` +asserts that none does. +""" + +from __future__ import annotations + +from typing import Any + +from jinja2 import StrictUndefined, Template +from jinja2.sandbox import SandboxedEnvironment + + +def create_sandboxed_environment(**kwargs: Any) -> SandboxedEnvironment: + """A Jinja environment that will not hand out Python internals. + + `undefined=StrictUndefined` unless the caller overrides it: an unresolved + name must fail the render rather than silently become an empty string. + """ + kwargs.setdefault("undefined", StrictUndefined) + return SandboxedEnvironment(**kwargs) + + +def sandboxed_template(source: str, **kwargs: Any) -> Template: + """A single template, compiled under the sandbox. + + For the call sites that want one template rather than a whole environment. + `Template(source)` there would build its own unsandboxed environment. + """ + return create_sandboxed_environment(**kwargs).from_string(source) diff --git a/src/orchestrator/orchestrator.py b/src/orchestrator/orchestrator.py index 4aa3ee55..91ac71f8 100644 --- a/src/orchestrator/orchestrator.py +++ b/src/orchestrator/orchestrator.py @@ -2339,13 +2339,15 @@ def _extract_outputs( output_defs = pipeline.metadata.get("outputs", {}) # Use Jinja2 for template rendering to support filters - from jinja2 import Template, TemplateError + from jinja2 import TemplateError + + from .core.template_sandbox import sandboxed_template for output_name, output_expr in output_defs.items(): try: if isinstance(output_expr, str) and "{{" in output_expr: # Render template with results context - template = Template(output_expr) + template = sandboxed_template(output_expr) # Create a context that includes all step results # Also create objects with .result attribute for backward compatibility context = {} diff --git a/src/orchestrator/runtime/dependency_resolver.py b/src/orchestrator/runtime/dependency_resolver.py index 2723ee28..33a79ade 100644 --- a/src/orchestrator/runtime/dependency_resolver.py +++ b/src/orchestrator/runtime/dependency_resolver.py @@ -11,7 +11,9 @@ import re from typing import Any, Dict, List, Optional, Set, Tuple, Union from dataclasses import dataclass -from jinja2 import Environment, StrictUndefined, TemplateSyntaxError, UndefinedError, meta +from jinja2 import StrictUndefined, TemplateSyntaxError, UndefinedError, meta + +from ..core.template_sandbox import create_sandboxed_environment import ast from .execution_state import PipelineExecutionState, UnresolvedItem, ItemStatus @@ -59,7 +61,7 @@ def __init__(self, execution_state: PipelineExecutionState, max_iterations: int self.max_iterations = max_iterations # Initialize Jinja2 environment for template parsing - self.jinja_env = Environment(undefined=StrictUndefined) + self.jinja_env = create_sandboxed_environment() # Regex patterns for different types of references self.patterns = { diff --git a/src/orchestrator/validation/data_flow_validator.py b/src/orchestrator/validation/data_flow_validator.py index 3bd1c010..6e8fc768 100644 --- a/src/orchestrator/validation/data_flow_validator.py +++ b/src/orchestrator/validation/data_flow_validator.py @@ -14,7 +14,9 @@ import re from typing import Any, Dict, List, Optional, Set, Tuple, Union from dataclasses import dataclass, field -from jinja2 import Environment, TemplateSyntaxError, meta +from jinja2 import TemplateSyntaxError, Undefined, meta + +from ..core.template_sandbox import create_sandboxed_environment logger = logging.getLogger(__name__) @@ -128,7 +130,7 @@ def __init__(self, self.tool_validator = tool_validator # Jinja2 environment for template analysis - self.jinja_env = Environment() + self.jinja_env = create_sandboxed_environment(undefined=Undefined) # Pattern for extracting template variables self.template_var_pattern = re.compile(r'\{\{\s*([^}]+)\s*\}\}') diff --git a/tests/test_supported_examples.py b/tests/test_supported_examples.py index b92b3a38..c951603d 100644 --- a/tests/test_supported_examples.py +++ b/tests/test_supported_examples.py @@ -85,9 +85,16 @@ class Case: artifacts: Dict[str, str] #: The complete declared `outputs` document, compared exactly. outputs: Dict[str, Any] + #: Steps that ran to the end AND succeeded. completed: Tuple[str, ...] skipped: Tuple[str, ...] = () + #: Steps whose task ended FAILED, i.e. something raised. failed: Tuple[str, ...] = () + #: Steps that ran to the end and reported `success: false` without raising. + #: Their status is "completed" like `completed` above, so status alone + #: cannot tell the two apart -- which is the entire point of listing them + #: separately. + reported_failure: Tuple[str, ...] = () #: Pipeline parameters, passed as `-i k=v` to the CLI and as `context=` to #: the API -- the two surfaces must reach the same branch the same way. inputs: Dict[str, Any] = field(default_factory=dict) @@ -178,6 +185,31 @@ def id(self) -> str: ("summarise",), ), ), + # The sibling of 06. There, the step raises and the task ends FAILED; here + # the tool declines without raising and the task ends *completed*, so the + # run is failing while no task is. Nothing else in this suite covers it: + # deleting the `reported_failure` branch of StepResult.from_task left the + # whole suite green before this case existed. + Case( + example="05_reported_failure.yaml", + variant="default", + exit_code=1, + artifacts={ + "output/before.txt": "this step succeeds", + "output/after.txt": "the run continued", + }, + outputs={}, + completed=("before", "after"), + reported_failure=("read_missing",), + levels=(("before",), ("read_missing",), ("after",)), + step_fields={ + "read_missing": { + "status": "completed", + "success": False, + "error": "File not found: ./output/does-not-exist.txt", + }, + }, + ), Case( example="06_failure_policy.yaml", variant="default", @@ -286,9 +318,12 @@ def _assert_case_holds(case: Case, payload: Dict[str, Any], run_dir: Path): for step_id, step in steps.items(): by_status.setdefault(step["status"], []).append(step_id) - assert sorted(by_status.get("completed", [])) == sorted(case.completed), ( + # Both `completed` and `reported_failure` steps carry status "completed"; + # only the success flag separates them. + ran_to_the_end = sorted(case.completed) + sorted(case.reported_failure) + assert sorted(by_status.get("completed", [])) == sorted(ran_to_the_end), ( f"{case.id}: completed steps were {sorted(by_status.get('completed', []))}, " - f"expected {sorted(case.completed)}" + f"expected {sorted(ran_to_the_end)}" ) assert sorted(by_status.get("skipped", [])) == sorted(case.skipped), ( f"{case.id}: skipped steps were {sorted(by_status.get('skipped', []))}, " @@ -310,6 +345,17 @@ def _assert_case_holds(case: Case, payload: Dict[str, Any], run_dir: Path): assert steps[step_id]["success"] is False, ( f"{case.id}: step {step_id} was expected to fail" ) + # The combination that status alone cannot express, and that nothing else + # in this suite exercises: ran to the end, and did not succeed. + for step_id in case.reported_failure: + assert steps[step_id]["status"] == "completed", ( + f"{case.id}: step {step_id} was expected to report failure without " + f"raising, but its status was {steps[step_id]['status']!r}" + ) + assert steps[step_id]["success"] is False, ( + f"{case.id}: step {step_id} completed and was reported successful, " + f"but it returned a failing result" + ) assert payload["success"] is (case.exit_code == 0), ( f"{case.id}: success={payload['success']} disagrees with the expected " diff --git a/tests/test_template_sandbox.py b/tests/test_template_sandbox.py new file mode 100644 index 00000000..4b90b966 --- /dev/null +++ b/tests/test_template_sandbox.py @@ -0,0 +1,120 @@ +"""Template rendering is sandboxed: parameter values are data, not code. + +A pipeline's `{{ }}` expressions are written by the pipeline author, but the +*values* substituted into them are not: they arrive from `-i name=value`, from +an inputs file, or from an earlier step's output. Those values are rendered +too, so a value that itself contains `{{ }}` gets evaluated. + +That much is a feature -- `-i out_dir='{{ base }}/reports'` is useful. What is +not a feature is the evaluation being unrestricted. Jinja's default +`Environment` exposes Python's object graph, so a parameter value could reach +`''.__class__.__mro__[1].__subclasses__()` and walk from a string literal to +arbitrary types. That is the standard Jinja SSTI-to-RCE escalation, and it was +reachable from a plain `orchestrator run ... -i greeting=`: + + -i greeting='{{ "".__class__.__mro__[1].__subclasses__() | length }}' + -> output/greeting.txt == "1183, world" + +These tests pin the boundary. Ordinary expressions still evaluate; anything +that reaches for Python internals is refused rather than rendered. +""" + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +from orchestrator.core.template_manager import TemplateManager + +pytestmark = [pytest.mark.contract] + +REPO = Path(__file__).parent.parent +EXAMPLE = REPO / "examples" / "supported" / "01_hello_filesystem.yaml" + +#: Payloads that must never evaluate. Each one is the first hop of a documented +#: Jinja sandbox escape -- reaching any of them means the object graph is open. +ESCAPES = [ + '{{ "".__class__ }}', + '{{ "".__class__.__mro__[1].__subclasses__() | length }}', + '{{ [].__class__.__base__.__subclasses__() | length }}', + '{{ "".__class__.__init__.__globals__ }}', + '{{ self.__init__.__globals__ }}', + '{{ cycler.__init__.__globals__.os.popen("id").read() }}', +] + + +def _run_cli(args, cwd): + env = dict(os.environ) + env["PYTHONPATH"] = str(REPO / "src") + os.pathsep + env.get("PYTHONPATH", "") + env.pop("ANTHROPIC_API_KEY", None) + env["ORCHESTRATOR_AUTO_INSTALL"] = "0" + return subprocess.run( + [sys.executable, "-m", "orchestrator.cli", *args], + cwd=str(cwd), + env=env, + capture_output=True, + text=True, + timeout=300, + ) + + +@pytest.mark.parametrize("payload", ESCAPES, ids=lambda p: p[:34]) +def test_a_parameter_value_cannot_reach_python_internals(payload): + """The unit-level boundary: rendering refuses, it does not compute.""" + manager = TemplateManager() + + with pytest.raises(Exception) as excinfo: + manager.render(payload, {}) + + # A SecurityError, not an incidental AttributeError that happens to abort. + assert "SecurityError" in type(excinfo.value).__name__ or "unsafe" in str( + excinfo.value + ).lower(), ( + f"{payload!r} failed, but not because the sandbox refused it: " + f"{type(excinfo.value).__name__}: {excinfo.value}" + ) + + +@pytest.mark.e2e +def test_an_escape_payload_never_reaches_an_artifact(tmp_path): + """End to end, through the CLI, exactly as the vulnerability was found. + + Before the sandbox this wrote `1183, world` and exited 0. + """ + payload = '{{ "".__class__.__mro__[1].__subclasses__() | length }}' + + result = _run_cli(["run", str(EXAMPLE), "-i", f"greeting={payload}"], cwd=tmp_path) + + artifact = tmp_path / "output" / "greeting.txt" + if artifact.exists(): + content = artifact.read_text() + assert "__" not in content and not content.split(",")[0].strip().isdigit(), ( + f"a template-injection payload was evaluated into an artifact: " + f"{content!r}" + ) + assert result.returncode != 0, ( + "an injection payload was accepted and the run reported success" + ) + + +def test_ordinary_expressions_still_evaluate(): + """The sandbox must not cost us the feature it protects. + + Parameter values holding templates are legitimate; only the object graph + is off limits. + """ + manager = TemplateManager() + + assert manager.render("{{ 7 * 7 }}", {}) == "49" + assert manager.render("{{ name | upper }}", {"name": "ada"}) == "ADA" + assert manager.render("{{ items | length }}", {"items": [1, 2, 3]}) == "3" + + +def test_step_result_references_still_resolve(): + """The shape every supported example depends on: `{{ id.result.field }}`.""" + manager = TemplateManager() + context = {"read_header": {"result": {"content": "# Title\n"}}} + + assert manager.render("{{ read_header.result.content }}", context) == "# Title\n"