diff --git a/design/workflow-step.md b/design/workflow-step.md index e571940109..7ab49998cd 100644 --- a/design/workflow-step.md +++ b/design/workflow-step.md @@ -43,6 +43,18 @@ automatically validate a definition passed to `execute()`. Guard invalid configurations in `execute()` too, returning a failed result rather than a successful default or an unhandled exception. +The built-in shell step preserves raw `run` interpolation by default. +Per-step `strict: true` rejects expression markers in `run` during validation +and execution. Its optional `env` mapping evaluates string templates separately, +requires NUL-free string results encodable for the process environment on +POSIX, and overrides only the child environment. +`SPECKIT_WORKFLOW_DIR` remains engine-owned. This prevents workflow expression +values from becoming shell source in strict mode; it is not a sandbox or a +guarantee that an authored script handles environment data safely. +Environment bindings and inherited settings can intentionally affect executable +lookup, module loading, and runtime startup. Strict mode does not filter them; +workflow source review must cover both `run` and `env` and referenced code. + New runs persist an execution tree. Resume replays completed occurrences into their expression contexts without calling their implementations, then continues at the unfinished occurrence. Selected branches, custom expansions, loop diff --git a/docs/reference/bundles.md b/docs/reference/bundles.md index 7c0f407524..fbfae537fe 100644 --- a/docs/reference/bundles.md +++ b/docs/reference/bundles.md @@ -335,6 +335,11 @@ non-default source. > **Vet both the bundle source and its component catalogs.** A project can supply its own bundle catalog and `install-allowed` component catalogs (extension, preset, workflow, and step) under `.specify/`; that project configuration existing is not evidence anything in it was reviewed. Before installing a bundle from an unfamiliar project, run `specify bundle catalog list` — and the equivalent `extension`/`preset`/`workflow catalog list` and `specify workflow step catalog list` commands for the components it pulls in — and treat any source you didn't add yourself as unvetted until you've reviewed it. +For workflow components, inspect shell `run` and `env` fields and their +referenced code, including changes introduced by active overlays. This applies +even to `strict: true` steps: environment bindings can change executable lookup +or runtime loading. See [Reviewing shell execution](workflows.md#reviewing-shell-execution). + ### List the Catalog Stack ```bash diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index cb4bc1fa96..073f6484fd 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -453,7 +453,7 @@ referents, so a pre-existing cache-file symlink can be replaced without changing its target. If a cache cannot be safely written, a warning is logged and discovery continues using the fetched catalog. -> **A project's `.specify/workflow-catalogs.yml` can point `add` and `search` at a catalog you didn't choose.** Before running a workflow from an unfamiliar project, run `specify workflow catalog list` (and `specify workflow step catalog list` for the step catalogs its steps can pull in) — a project supplying that config is not evidence its workflows or steps were vetted. Maintainers do not audit `run` fields; read a workflow's shell steps yourself before running it (see [Who maintains workflows?](#who-maintains-workflows)). +> **A project's `.specify/workflow-catalogs.yml` can point `add` and `search` at a catalog you didn't choose.** Before running a workflow from an unfamiliar project, run `specify workflow catalog list` (and `specify workflow step catalog list` for the step catalogs its steps can pull in) — a project supplying that config is not evidence its workflows or steps were vetted. Maintainers do not audit workflow code; review both `run` and `env` in its shell steps yourself, including strict steps (see [Reviewing shell execution](#reviewing-shell-execution) and [Who maintains workflows?](#who-maintains-workflows)). ### List Catalogs @@ -782,6 +782,32 @@ qualified `step_id`. A step emits completion after its checkpoint; containers emit before their children, while replayed completed occurrences emit no step events. +### Shell step + +`type: shell` executes `run` through the platform shell, retaining support for +pipes, redirects, and multiple commands. Set `strict: true` to reject `{{` +expression markers anywhere in `run`, including inside quotes. This is checked +both during validation and immediately before execution. `strict` must be a +boolean; omitted or `false` preserves existing raw interpolation behavior. + +Use an `env` mapping to bind dynamic values without inserting them into shell +source. For example, on POSIX: + +```yaml +- id: display-result + type: shell + strict: true + env: + RESULT: "{{ steps.plan.output.stdout }}" + run: 'printf "%s\n" "$RESULT"' +``` + +For cross-platform scripts, read the environment in the program itself, such +as `os.environ["RESULT"]` in Python. See +[Shell Step Environment Variables](#shell-step-environment-variables) for the +binding contract and [Interpolation and shell safety](#interpolation-and-shell-safety) +for the remaining trust boundary. + > **Security note:** a `shell` step runs a local command with **your** privileges. There is no capability sandbox — `requires` is an advisory pre-condition block (spec-kit version, integrations), not a runtime gate, so it does **not** restrict what a step can do. In particular there is no `requires.permissions` capability gate: it is rejected by validation precisely because it would imply a sandbox that does not exist. Review any catalog or downloaded workflow before running it, and use a `gate` step to require explicit approval before sensitive or destructive shell commands. ### GitHub step @@ -1237,9 +1263,11 @@ If an interpolated value can contain characters like `;`, `|`, `&`, `$( )`, back - **Workflow `inputs.*`** — supplied by whoever runs the workflow. - **A prior step's output**, e.g. `{{ steps.plan.output.stdout }}` — for a `command` or `prompt` step with [`capture: true`](#capturing-command-and-prompt-output) this is **text produced by the AI agent**, which can in turn be influenced by files, tickets, or web content the agent read. Treat agent output as untrusted when it flows into a `shell` step. -There is **no shell-escaping filter** in the expression language and **no sandbox** around a `shell` step, so none of the practices below can be treated as a guarantee that a hostile value is neutralised. The only reliable control is to constrain what an interpolated value *can* be, and to keep values you cannot constrain out of `run` fields entirely. Scrutinise every `run` field that interpolates a value you do not control, and at minimum: +There is **no shell-escaping filter** in the expression language and **no sandbox** around a `shell` step. Prefer [`strict: true`](#shell-step) with `env` bindings to keep dynamic values out of shell source. Strict mode does not sanitise data, inspect commands, or prevent an authored script from interpreting data as code. Do not use `eval`, build another `sh -c` command from a variable, or pass untrusted values through shell startup variables such as `BASH_ENV`. On Windows, `cmd.exe` expansion of `%VALUE%` can introduce shell syntax; read untrusted values inside a program instead. On POSIX, quote variable expansions and use the target program's argument validation or `--` where appropriate to prevent option injection. -- **Constrain the value at the source with `enum`/an allowlist.** When `inputs.*` feeds a `run` field, restrict it to a fixed set of known-safe values so a caller cannot supply arbitrary shell text at all. This is the strongest control the engine offers — prefer it over any downstream mitigation. +For compatibility, raw interpolation remains enabled when `strict` is omitted or `false`. Its injection risk remains unchanged. Keep values you cannot constrain out of `run` fields entirely. Scrutinise every non-strict `run` field that interpolates a value you do not control, and at minimum: + +- **Constrain the value at the source with `enum`/an allowlist.** When `inputs.*` feeds a non-strict `run` field, restrict it to a fixed set of known-safe values so a caller cannot supply arbitrary shell text at all. ```yaml inputs: @@ -1262,6 +1290,60 @@ Shell steps automatically receive the following environment variables: | -------- | ----------- | | `SPECKIT_WORKFLOW_DIR` | Resolved absolute path to the workflow source directory (same value as `{{ context.workflow_dir }}`). Not set when the workflow has no source path. | +An optional per-step `env` mapping is available in both strict and non-strict +mode. Names must match `[A-Za-z_][A-Za-z0-9_]*`; names are literal, not +expressions. Values must be strings and may contain workflow expressions. +Each value is evaluated once and must resolve to a string without NUL +characters. On POSIX, the string must also be encodable for the process +environment using the filesystem encoding and its error handler. +Invalid bindings fail the step before launching a process. + +Single-expression bindings preserve the expression's result type: +`COUNT: "{{ inputs.count }}"` rejects a number, boolean, null, list, or mapping +instead of converting it to text. Mixed templates use standard expression +interpolation: `COUNT: "n={{ inputs.count }}"` produces `n=42` for integer +`42`; embedded null becomes empty text, and other values use their string +representation. To serialize structured data explicitly, use `to_json`. + +Bindings override inherited environment values for that subprocess only; +they do not modify the parent process or subsequent steps. An empty string is +valid. `SPECKIT_WORKFLOW_DIR` is reserved (case-insensitively) and cannot be +overridden. Keep environment names and executable-loading settings under the +workflow author's control: environment binding is a data channel, not a sandbox. + +### Reviewing shell execution + +Before installing or running an unfamiliar workflow, audit each shell step's +`run` **and** `env`, including nested branches and fan-out templates. Review +referenced scripts and resources, called workflows, and active overlay YAML +that can replace or insert steps. Apply the same review to workflow components +of bundles, and repeat it when definitions, overlays, or referenced code change. +Custom step packages require their own source review because loading them +executes Python code. + +An environment value can control execution rather than merely supply data. +For example, `PATH` selects executables, `PYTHONPATH` affects Python imports, +`BASH_ENV` can select Bash startup code, and `NODE_OPTIONS` can change Node.js +startup behavior. Native loaders and other programs have their own settings; +these examples are not an exhaustive list. A fixed `run` can therefore execute +different code even with `strict: true` and no `eval`. Strict mode neither +filters execution-controlling bindings nor cleans the inherited environment. +Keep those settings literal and author-vetted, or constrain dynamic values to +an approved set of trusted paths/options. Use unconstrained input or agent +output only for application-data variables whose consumers handle it as data. + +When a shell step invokes Specify itself, catalog overrides such as +`SPECKIT_WORKFLOW_CATALOG_URL` can also change where child commands discover +or install components. Vet those bindings alongside the +[active catalog sources](catalogs.md#discovery-and-trust). + +Workflow validation checks configuration, not whether environment settings or +commands are safe. `specify workflow info` shows metadata and step identities; +`specify workflow resolve` shows layer and step attribution. Neither displays +the complete `run`/`env` configuration or replaces reading the contributing +source files. Catalog install permission, bundle installation, and strict mode +are not security endorsements. + ## Input Types | Type | Coercion | diff --git a/src/specify_cli/workflows/step/shell/__init__.py b/src/specify_cli/workflows/step/shell/__init__.py index 42684b83e1..38b50dfa84 100644 --- a/src/specify_cli/workflows/step/shell/__init__.py +++ b/src/specify_cli/workflows/step/shell/__init__.py @@ -7,6 +7,7 @@ import locale import math import os +import re import subprocess from typing import Any @@ -23,6 +24,13 @@ class ShellStep(StepBase): type_key = "shell" def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: + binding_errors = self._binding_errors(config) + if binding_errors: + return StepResult( + status=StepStatus.FAILED, + error="; ".join(binding_errors), + output={"exit_code": -1, "stdout": "", "stderr": "invalid shell bindings"}, + ) run_cmd = config.get("run", "") if isinstance(run_cmd, str) and "{{" in run_cmd: run_cmd = evaluate_expression(run_cmd, context) @@ -45,6 +53,23 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: ) env = {**os.environ} + for name, template in config.get("env", {}).items(): + try: + value = evaluate_expression(template, context) + except ValueError as exc: + return StepResult( + status=StepStatus.FAILED, + error=f"Shell step {config.get('id', '?')!r}: env {name!r}: {exc}", + output={"exit_code": -1, "stdout": "", "stderr": "invalid environment expression"}, + ) + value_error = self._environment_value_error(name, value) + if value_error is not None: + return StepResult( + status=StepStatus.FAILED, + error=f"Shell step {config.get('id', '?')!r}: {value_error}", + output={"exit_code": -1, "stdout": "", "stderr": "invalid environment value"}, + ) + env[name.upper() if os.name == "nt" else name] = value if context.workflow_dir: env["SPECKIT_WORKFLOW_DIR"] = context.workflow_dir else: @@ -52,8 +77,9 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: # NOTE: shell=True is required to support pipes, redirects, and # multi-command expressions in workflow YAML. Workflow authors - # control commands; catalog-installed workflows should be reviewed - # before use (see PUBLISHING.md for security guidance). + # control commands. strict=True rejects expression interpolation in + # run; dynamic data can instead be bound through env without becoming + # shell source. Neither mode sandboxes authored shell commands. try: proc = subprocess.run( # noqa: S602 -- intentional shell=True (see NOTE above) run_cmd, @@ -121,6 +147,51 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: output={"exit_code": -1, "stdout": "", "stderr": str(exc)}, ) + @staticmethod + def _environment_value_error(name: str, value: Any) -> str | None: + if not isinstance(value, str): + return f"env {name!r} must be a string, got {type(value).__name__}." + if "\x00" in value: + return f"env {name!r} must not contain NUL characters." + if os.name == "posix": + try: + os.fsencode(value) + except UnicodeEncodeError: + return f"env {name!r} cannot be encoded for the process environment." + return None + + @classmethod + def _binding_errors(cls, config: dict[str, Any]) -> list[str]: + errors: list[str] = [] + prefix = f"Shell step {config.get('id', '?')!r}: " + strict = config.get("strict", False) + if not isinstance(strict, bool): + errors.append(prefix + "'strict' must be a boolean.") + elif strict: + run = config.get("run") + if not isinstance(run, str): + errors.append(prefix + "'run' must be a string in strict mode.") + elif "{{" in run: + errors.append( + prefix + "strict mode forbids expressions in 'run'; " + "bind dynamic values through 'env' instead." + ) + + bindings = config.get("env", {}) + if not isinstance(bindings, dict): + errors.append(prefix + "'env' must be a mapping.") + return errors + for name, template in bindings.items(): + if not isinstance(name, str) or re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*", name) is None: + errors.append(prefix + f"invalid environment variable name {name!r}.") + continue + if name.upper() == "SPECKIT_WORKFLOW_DIR": + errors.append(prefix + f"environment variable {name!r} is reserved.") + value_error = cls._environment_value_error(name, template) + if value_error is not None: + errors.append(prefix + value_error) + return errors + @staticmethod def _timeout_error(config: dict[str, Any]) -> str | None: """Return an error message if ``config['timeout']`` is invalid, else None. @@ -157,20 +228,20 @@ def _timeout_error(config: dict[str, Any]) -> str | None: def validate(self, config: dict[str, Any]) -> list[str]: errors = super().validate(config) - if "run" not in config: - errors.append( - f"Shell step {config.get('id', '?')!r} is missing 'run' field." - ) - elif not isinstance(config["run"], str): - # execute() str()-coerces run and invokes it under shell=True, so a - # null or list 'run' would run the Python repr ('None', "['echo']") - # as a command. Reject non-strings at validation, mirroring the - # command-step input/options and gate options type checks. An - # expression like "{{ ... }}" is still a str, so it stays valid. - errors.append( - f"Shell step {config.get('id', '?')!r}: 'run' must be a string, " - f"got {type(config['run']).__name__}." - ) + errors.extend(self._binding_errors(config)) + # Strict run errors are already reported by _binding_errors(). + if config.get("strict") is not True: + if "run" not in config: + errors.append( + f"Shell step {config.get('id', '?')!r} is missing 'run' field." + ) + elif not isinstance(config["run"], str): + # Legacy execution coerces run to shell source; validation + # must still reject non-strings without changing that path. + errors.append( + f"Shell step {config.get('id', '?')!r}: 'run' must be a string, " + f"got {type(config['run']).__name__}." + ) output_format = config.get("output_format") if output_format is not None and output_format != "json": errors.append( diff --git a/tests/workflows/test_shell_step.py b/tests/workflows/test_shell_step.py index 5acdc5efd4..54b8241ada 100644 --- a/tests/workflows/test_shell_step.py +++ b/tests/workflows/test_shell_step.py @@ -1,8 +1,9 @@ -"""Real-process regression tests for shell step output decoding.""" +"""Regression tests for shell step execution and output decoding.""" from __future__ import annotations import locale +import os import sys from pathlib import Path @@ -26,6 +27,396 @@ def _python_run(project_dir: Path, body: str) -> str: return f'"{sys.executable}" "{script}"' +@pytest.mark.parametrize("run", [ + "echo {{ inputs.value }}", + 'echo "{{ steps.source.output.stdout }}"', + "echo '{{ item }}'", + "{{ inputs.command }}", + "echo {{", +]) +def test_strict_rejects_run_expressions_before_execution(monkeypatch, run): + def unexpected_run(*args, **kwargs): + pytest.fail("A strict run expression must never reach the shell") + + monkeypatch.setattr("subprocess.run", unexpected_run) + config = {"id": "strict", "run": run, "strict": True} + step = ShellStep() + + assert any("strict" in error and "'run'" in error for error in step.validate(config)) + result = step.execute(config, StepContext(inputs={"value": "$(echo injected)"})) + assert result.status == StepStatus.FAILED + assert "strict" in result.error + assert result.output["exit_code"] == -1 + + +@pytest.mark.parametrize("options, message", [ + ({"strict": "true"}, "'strict' must be a boolean"), + ({"strict": 1}, "'strict' must be a boolean"), + ({"strict": None}, "'strict' must be a boolean"), + ({"strict": True, "run": None}, "'run' must be a string in strict mode"), + ({"strict": True, "run": ["echo", "hello"]}, "'run' must be a string in strict mode"), + ({"env": None}, "'env' must be a mapping"), + ({"env": []}, "'env' must be a mapping"), + ({"env": "NAME=value"}, "'env' must be a mapping"), + ({"env": {"": "value"}}, "environment variable name"), + ({"env": {"BAD=NAME": "value"}}, "environment variable name"), + ({"env": {"BAD-NAME": "value"}}, "environment variable name"), + ({"env": {1: "value"}}, "environment variable name"), + ({"env": {"SPECKIT_WORKFLOW_DIR": "value"}}, "reserved"), + ({"env": {"speckit_workflow_dir": "value"}}, "reserved"), + ({"env": {"VALUE": None}}, "must be a string"), + ({"env": {"VALUE": 1}}, "must be a string"), + ({"env": {"VALUE": False}}, "must be a string"), + ({"env": {"VALUE": []}}, "must be a string"), + ({"env": {"VALUE": "nul\x00value"}}, "NUL"), +]) +def test_invalid_shell_bindings_fail_validation_and_execution(monkeypatch, options, message): + def unexpected_run(*args, **kwargs): + pytest.fail("Invalid configuration must never reach the shell") + + monkeypatch.setattr("subprocess.run", unexpected_run) + config = {"id": "invalid", "run": "echo hello", **options} + step = ShellStep() + + assert any(message in error for error in step.validate(config)) + result = step.execute(config, StepContext()) + assert result.status == StepStatus.FAILED + assert message in result.error + + +@pytest.mark.parametrize("run_config", [ + {}, + {"run": None}, + {"run": ["echo", "hello"]}, +], ids=["missing", "null", "list"]) +def test_strict_invalid_run_has_one_diagnostic_and_never_launches(monkeypatch, run_config): + def unexpected_run(*args, **kwargs): + pytest.fail("An invalid strict run must never reach the shell") + + monkeypatch.setattr("subprocess.run", unexpected_run) + config = {"id": "invalid-run", "strict": True, **run_config} + expected_error = "Shell step 'invalid-run': 'run' must be a string in strict mode." + step = ShellStep() + + assert step.validate(config) == [expected_error] + result = step.execute(config, StepContext()) + assert result.status == StepStatus.FAILED + assert result.error == expected_error + assert result.output["exit_code"] == -1 + + +@pytest.mark.parametrize("value", [None, 42, False, [], {}, "nul\x00value"]) +def test_invalid_resolved_environment_fails_before_execution(monkeypatch, value): + def unexpected_run(*args, **kwargs): + pytest.fail("Invalid environment values must never reach the shell") + + monkeypatch.setattr("subprocess.run", unexpected_run) + config = { + "id": "invalid-env", "strict": True, "run": "echo hello", + "env": {"VALUE": "{{ inputs.value }}"}, + } + result = ShellStep().execute(config, StepContext(inputs={"value": value})) + assert result.status == StepStatus.FAILED + assert "VALUE" in result.error + assert "string" in result.error or "NUL" in result.error + + +def test_environment_expression_error_fails_before_execution(monkeypatch): + def unexpected_run(*args, **kwargs): + pytest.fail("An invalid environment expression must never reach the shell") + + monkeypatch.setattr("subprocess.run", unexpected_run) + result = ShellStep().execute({ + "id": "invalid-expression", "run": "echo hello", + "env": {"VALUE": "{{ inputs.value | upper }}"}, + }, StepContext(inputs={"value": []})) + assert result.status == StepStatus.FAILED + assert "VALUE" in result.error + assert "upper" in result.error + + +@pytest.mark.skipif(os.name != "posix", reason="POSIX process environment encoding") +@pytest.mark.parametrize("value", ["\ud800", "\udc00"]) +@pytest.mark.parametrize("template", ["literal", "{{ inputs.value }}", "value={{ inputs.value }}"]) +@pytest.mark.parametrize("strict", [False, True]) +def test_unencodable_environment_fails_before_execution(monkeypatch, value, template, strict): + def unexpected_run(*args, **kwargs): + pytest.fail("Unencodable environment values must never reach subprocess.run") + + monkeypatch.setattr("subprocess.run", unexpected_run) + config = { + "id": "unencodable-env", "strict": strict, "run": "echo hello", + "env": {"VALUE": value if template == "literal" else template}, + } + step = ShellStep() + errors = step.validate(config) + if template == "literal": + assert any("env 'VALUE'" in error and "encoded" in error for error in errors) + else: + assert errors == [] + + result = step.execute(config, StepContext(inputs={"value": value})) + assert result.status == StepStatus.FAILED + assert "env 'VALUE'" in result.error + assert "encoded" in result.error + assert result.output["exit_code"] == -1 + + +@pytest.mark.skipif(os.name != "posix", reason="POSIX process environment encoding") +@pytest.mark.parametrize("value", ["caf\u00e9", "byte-\udcff"]) +def test_encodable_environment_preserves_unicode_and_surrogateescape(project_dir, value): + run = _python_run( + project_dir, "import os\nprint(os.fsencode(os.environ['VALUE']).hex())\n", + ) + config = { + "id": "encodable-env", "strict": True, "run": run, + "env": {"VALUE": "{{ inputs.value }}"}, + } + assert ShellStep().validate(config) == [] + result = ShellStep().execute(config, StepContext( + project_root=str(project_dir), inputs={"value": value}, + )) + assert result.status == StepStatus.COMPLETED + assert result.output["stdout"].strip() == os.fsencode(value).hex() + + +@pytest.mark.parametrize(("value", "expected"), [ + (42, "n=42"), + (None, "n="), + (False, "n=False"), + ([1, 2], "n=[1, 2]"), + ({"key": "value"}, "n={'key': 'value'}"), +]) +def test_mixed_environment_template_converts_embedded_values(project_dir, value, expected): + run = _python_run(project_dir, "import os\nprint(os.environ['VALUE'])\n") + config = { + "id": "mixed-env", "strict": True, "run": run, + "env": {"VALUE": "n={{ inputs.value }}"}, + } + assert ShellStep().validate(config) == [] + result = ShellStep().execute(config, StepContext( + project_root=str(project_dir), inputs={"value": value}, + )) + assert result.status == StepStatus.COMPLETED + assert result.output["stdout"] == expected + "\n" + + +@pytest.mark.skipif(os.name != "posix", reason="POSIX process environment encoding") +@pytest.mark.parametrize("continue_on_error", [False, True]) +def test_unencodable_environment_respects_continue_on_error(project_dir, continue_on_error): + definition = WorkflowDefinition.from_string(yaml.safe_dump({ + "schema_version": "1.0", + "workflow": {"id": "env-encoding", "name": "Env Encoding", "version": "1.0.0"}, + "inputs": {"encoded": {"type": "string", "default": r'"\ud800"'}}, + "steps": [ + { + "id": "invalid", "type": "shell", "strict": True, "run": "echo hello", + "env": {"VALUE": "{{ inputs.encoded | from_json }}"}, + "continue_on_error": continue_on_error, + }, + {"id": "after", "type": "shell", "strict": True, "run": "echo after"}, + ], + })) + engine = WorkflowEngine(project_dir) + assert engine.validate(definition) == [] + state = engine.execute(definition) + assert state.step_results["invalid"]["status"] == "failed" + assert "encoded" in state.step_results["invalid"]["error"] + if continue_on_error: + assert state.status == RunStatus.COMPLETED + assert state.step_results["after"]["status"] == "completed" + else: + assert state.status == RunStatus.FAILED + assert "after" not in state.step_results + + +@pytest.mark.parametrize("strict", [False, True]) +@pytest.mark.parametrize("expression", [ + "{{ inputs.value }}", + "{{ steps.source.output.stdout }}", + "{{ item }}", +]) +def test_environment_preserves_dynamic_values_as_data( + project_dir, monkeypatch, strict, expression, +): + import json + + payload = """'"; echo injected; $(echo substituted) `echo substituted` | & %PATH% +{{ inputs.other }}""" + monkeypatch.setenv("SHELL_INHERITED", "inherited") + monkeypatch.setenv("VALUE", "old") + monkeypatch.delenv("STATIC", raising=False) + run = _python_run(project_dir, ( + "import json, os\n" + "print(json.dumps({key: os.environ[key] for key in " + "['VALUE', 'STATIC', 'SHELL_INHERITED', 'SPECKIT_WORKFLOW_DIR']}))\n" + )) + config = { + "id": "data", "run": run, "strict": strict, + "env": {"VALUE": expression, "STATIC": ""}, + "output_format": "json", + } + context = StepContext( + project_root=str(project_dir), + workflow_dir=str(project_dir / "workflow source"), + inputs={"value": payload}, + steps={"source": {"output": {"stdout": payload}}}, + item=payload, + ) + assert ShellStep().validate(config) == [] + result = ShellStep().execute(config, context) + + assert result.status == StepStatus.COMPLETED + assert json.loads(result.output["stdout"]) == { + "VALUE": payload, "STATIC": "", "SHELL_INHERITED": "inherited", + "SPECKIT_WORKFLOW_DIR": context.workflow_dir, + } + assert os.environ["VALUE"] == "old" + assert "STATIC" not in os.environ + + +@pytest.mark.skipif(os.name != "posix", reason="POSIX executable fixtures") +@pytest.mark.parametrize("variable", ["PATH", "PYTHONPATH"]) +@pytest.mark.parametrize("selected", ["first", "second"]) +def test_strict_environment_can_select_executables_and_modules(project_dir, variable, selected): + parent_value = os.environ.get(variable) + for name in ("first", "second"): + directory = project_dir / name + directory.mkdir() + if variable == "PATH": + script = directory / "strict-env-probe" + script.write_text(f"#!/bin/sh\nprintf '%s\\n' '{name}'\n", encoding="utf-8") + script.chmod(0o755) + else: + (directory / "workflow_env_probe.py").write_text( + f"selected = {name!r}\n", encoding="utf-8", + ) + run = "strict-env-probe" if variable == "PATH" else _python_run( + project_dir, "import workflow_env_probe\nprint(workflow_env_probe.selected)\n", + ) + config = { + "id": "execution-env", "strict": True, "run": run, + "env": {variable: "{{ inputs.directory }}"}, + } + assert ShellStep().validate(config) == [] + result = ShellStep().execute(config, StepContext( + project_root=str(project_dir), + inputs={"directory": str(project_dir / selected)}, + )) + assert result.status == StepStatus.COMPLETED + assert result.output["stdout"] == selected + "\n" + assert os.environ.get(variable) == parent_value + + +@pytest.mark.parametrize("strict_options", [{}, {"strict": False}]) +def test_legacy_run_interpolation_is_preserved(project_dir, strict_options): + config = { + "id": "legacy", "run": "echo {{ inputs.value }}", **strict_options, + } + assert ShellStep().validate(config) == [] + result = ShellStep().execute( + config, StepContext(project_root=str(project_dir), inputs={"value": "hello"}), + ) + assert result.status == StepStatus.COMPLETED + assert result.output["stdout"].strip() == "hello" + + +def test_strict_fixed_shell_retains_pipes_and_redirection(project_dir): + copy_stdin = _python_run( + project_dir, "import sys\nsys.stdout.write(sys.stdin.read())\n", + ) + config = { + "id": "pipeline", "strict": True, + "run": f"echo hello | {copy_stdin} > result.txt && echo done", + } + result = ShellStep().execute(config, StepContext(project_root=str(project_dir))) + assert result.status == StepStatus.COMPLETED + assert result.output["stdout"].strip() == "done" + assert (project_dir / "result.txt").read_text().strip() == "hello" + + +def test_windows_environment_override_is_case_insensitive(monkeypatch): + import subprocess + from types import SimpleNamespace + + import specify_cli.workflows.step.shell as shell_module + + parent_env = {"PATH": "inherited", "SPECKIT_WORKFLOW_DIR": "stale"} + monkeypatch.setattr(shell_module, "os", SimpleNamespace(name="nt", environ=parent_env)) + + def capture_run(command, **kwargs): + assert kwargs["env"] == {"PATH": "configured"} + return subprocess.CompletedProcess(command, 0, stdout=b"", stderr=b"") + + monkeypatch.setattr("subprocess.run", capture_run) + result = ShellStep().execute({ + "id": "windows-env", "strict": True, "run": "echo hello", + "env": {"Path": "configured"}, + }, StepContext()) + assert result.status == StepStatus.COMPLETED + assert parent_env == {"PATH": "inherited", "SPECKIT_WORKFLOW_DIR": "stale"} + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX shell variable syntax") +def test_strict_quoted_environment_expansion_does_not_execute_data(project_dir): + payload = """'"; echo injected; $(echo substituted) `echo substituted` +second line""" + result = ShellStep().execute({ + "id": "literal", "strict": True, + "run": """printf '%s' "$VALUE" """, + "env": {"VALUE": "{{ inputs.value }}"}, + }, StepContext(project_root=str(project_dir), inputs={"value": payload})) + assert result.status == StepStatus.COMPLETED + assert result.output["stdout"] == payload + + +@pytest.mark.parametrize("continue_on_error", [False, True]) +def test_strict_failure_stops_or_continues_workflow(project_dir, continue_on_error): + definition = WorkflowDefinition.from_string(yaml.safe_dump({ + "schema_version": "1.0", + "workflow": {"id": "strict-failure", "name": "Strict Failure", "version": "1.0.0"}, + "steps": [ + { + "id": "blocked", "type": "shell", "strict": True, + "run": "echo {{ inputs.value }}", + "continue_on_error": continue_on_error, + }, + {"id": "after", "type": "shell", "strict": True, "run": "echo after"}, + ], + })) + engine = WorkflowEngine(project_dir) + assert any("strict mode" in error for error in engine.validate(definition)) + state = engine.execute(definition) + assert state.step_results["blocked"]["status"] == "failed" + assert "strict mode" in state.step_results["blocked"]["error"] + if continue_on_error: + assert state.status == RunStatus.COMPLETED + assert state.step_results["after"]["status"] == "completed" + else: + assert state.status == RunStatus.FAILED + assert "after" not in state.step_results + + +def test_engine_binds_prior_output_to_strict_shell_environment(project_dir): + run = _python_run(project_dir, "import os\nprint(os.environ['VALUE'])\n") + definition = WorkflowDefinition.from_string(yaml.safe_dump({ + "schema_version": "1.0", + "workflow": {"id": "strict-env", "name": "Strict Env", "version": "1.0.0"}, + "steps": [ + {"id": "source", "type": "shell", "strict": True, "run": "echo source-data"}, + { + "id": "consumer", "type": "shell", "strict": True, "run": run, + "env": {"VALUE": "{{ steps.source.output.stdout }}"}, + }, + ], + })) + engine = WorkflowEngine(project_dir) + assert engine.validate(definition) == [] + state = engine.execute(definition) + assert state.status == RunStatus.COMPLETED + assert state.step_results["consumer"]["output"]["stdout"] == "source-data\n\n" + + @pytest.mark.parametrize("stream", [1, 2, "both"], ids=["stdout", "stderr", "both"]) @pytest.mark.parametrize("exit_code", [0, 3], ids=["zero-exit", "nonzero-exit"]) @pytest.mark.parametrize("output_format", [None, "json"], ids=["text", "json"]) diff --git a/workflows/PUBLISHING.md b/workflows/PUBLISHING.md index 9a62773716..12d478588b 100644 --- a/workflows/PUBLISHING.md +++ b/workflows/PUBLISHING.md @@ -235,7 +235,7 @@ Maintainers check that: 3. **Input design** — clear prompts, sensible defaults and enums 4. **Documentation** — clear README explaining what the workflow does and when to use it -This is a check of the submission's **form and completeness**, not a security review — maintainers may reject a submission if its `shell` step content looks obviously dangerous during this pass, but they do not systematically audit, endorse, or support the workflow's code. Treat every workflow, including catalog-listed ones, as untrusted until you've read its `run` fields yourself (see [Security: shell steps execute arbitrary code](#security-shell-steps-execute-arbitrary-code)). +This is a check of the submission's **form and completeness**, not a security review — maintainers may reject a submission if its `shell` step content looks obviously dangerous during this pass, but they do not systematically audit, endorse, or support the workflow's code. Treat every workflow, including catalog-listed ones, as untrusted until you've reviewed its source, including shell `run` and `env` fields (see [Security: shell steps execute arbitrary code](#security-shell-steps-execute-arbitrary-code)). Once these checks pass, the workflow appears in `specify workflow search`. @@ -280,7 +280,10 @@ Workflow `shell` steps execute their `run` field through `/bin/sh` (POSIX) or th Catalog-listed workflows are checked for form and completeness at submission time (see [Verification Process](#verification-process)); maintainers may reject an obviously dangerous submission they happen to notice, but this is not a security audit — you should treat every install as code-execution from an untrusted source until you have read the `workflow.yml`: -- **Before installing a workflow**, fetch the raw YAML and audit every `shell` step's `run` field directly. `specify workflow info ` only shows metadata (name, version, inputs, step IDs/types) — not the shell content that would actually execute. +- **Before installing a workflow**, fetch the raw YAML and audit every `shell` step's `run` and `env` fields, including nested branches and fan-out templates, and any scripts or resources they reference. `specify workflow info ` only shows metadata (name, version, inputs, step IDs/types); `specify workflow resolve ` shows layer/step attribution. Neither displays the complete commands and environment bindings that would execute. +- **Audit execution-controlling environment values, even in strict steps.** `PATH` can select different executables; `PYTHONPATH`, `BASH_ENV`, `NODE_OPTIONS`, and platform-specific loader settings can select code or change startup behavior without interpolating anything into `run`. If the step invokes Specify, catalog overrides such as `SPECKIT_WORKFLOW_CATALOG_URL` can redirect component discovery or installation. These are examples, not a complete list. Keep such values author-vetted or constrained to approved trusted paths/options, rather than accepting unconstrained input or agent output. Strict mode does not filter these settings or clean the inherited environment. +- **Review contributing sources too** — include called workflows, custom step packages, and active overlay YAML before running. Bundle installation does not replace vetting its workflow components. Repeat the review after updates or changes to overlays and referenced code. +- **Prefer `strict: true` with `env` bindings** for shell steps that consume dynamic data. Strict mode rejects workflow expressions in `run`; `env` values are evaluated separately and must resolve to strings. Read them inside a program, or use quoted POSIX variable expansions in a fixed script. Do not feed them into `eval`, another shell command string, or shell startup variables. Windows `cmd.exe` `%VAR%` expansion can also introduce shell syntax; read untrusted values inside a program instead. Strict mode is opt-in and is not a sandbox; existing non-strict interpolation remains unchanged. - **Constrain interpolated values, don't just quote them** in `run` blocks: expressions are spliced in as raw text with no automatic escaping, and there is no shell-escaping filter, so quoting is not a security boundary. Restrict `{{ inputs.something }}` substitutions to a fixed set with `enum`/an allowlist so a malicious input can't inject shell syntax; treat quoting only as correctness handling for already-constrained values. - **Treat prior-step output as untrusted too** — `{{ steps.*.output.* }}` from a `prompt` step is AI-generated text that upstream content can influence. Don't interpolate agent output into a `run` field at all when you can't constrain it; branch on it with `if`/`switch` or act on it in a non-shell step instead. - **Limit privilege**: shell steps inherit the user's environment. Workflows that need elevated access (sudo, secrets, GitHub tokens) should call them out explicitly in the README so reviewers can spot the requirement. diff --git a/workflows/README.md b/workflows/README.md index c71010892a..eda0bcd329 100644 --- a/workflows/README.md +++ b/workflows/README.md @@ -184,10 +184,24 @@ Run a shell command and capture output: ```yaml - id: run-tests type: shell + strict: true # Optional: reject workflow expressions in run run: "npm test" # runs from the project root; no interpolation needed timeout: 1800 # Optional: max seconds before the command is killed (default 300) ``` +For dynamic values, prefer `strict: true` and a per-step `env` mapping such as +`env: {RESULT: "{{ steps.plan.output.stdout }}"}`. Values must resolve to strings +and are passed as environment data, not inserted into `run`. Read them inside +the called program; on POSIX, quoted variable expansion in a fixed script is +also suitable. Strict mode is not a sandbox: do not reinterpret values with +`eval` or a nested shell. Omitting `strict`, or setting it to `false`, preserves +existing raw interpolation and its risks. + +Before installing or running an unfamiliar workflow, review both `run` and +`env`, including overlay changes and referenced code. Even with `strict: true`, +settings such as `PATH` and `PYTHONPATH` can select different executables or modules. +String validation and catalog or bundle installation do not establish trust. + > ⚠️ **Constrain interpolated values in `run` fields.** A `run` field is executed > by the system shell, and `{{ ... }}` expressions are substituted as raw text > with no automatic quoting or escaping. An `inputs.*` value or prior-step output