Skip to content

Commit ba7894d

Browse files
mnriemCopilot
andauthored
feat(workflows): add opt-in strict shell execution (#5181)
* feat(workflows): add opt-in strict shell execution Reject workflow expressions in strict shell commands and support separately evaluated environment bindings while preserving legacy interpolation by default. Document the trust boundary and cover validation, execution, and workflow behavior. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(workflows): reject unencodable shell environment bindings Check POSIX bindings with os.fsencode before subprocess execution so encoding failures respect continue_on_error. Clarify mixed-template conversion and cover invalid surrogate values, valid surrogateescape data, and workflow continuation. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * test(workflows): cover non-string strict shell commands Verify that strict shell steps reject null and list-valued run fields during validation and execution without invoking subprocess.run. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(workflows): avoid duplicate strict run diagnostics Keep the strict binding guard as the sole run diagnostic in strict mode and preserve legacy validation behavior. Cover missing, null, and list-valued run fields with exact single-error assertions and pre-launch rejection checks. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs(workflows): include environment bindings in execution audits Clarify that pre-install review includes run, env, referenced code, overlays, called workflows, bundle components, and catalog overrides. Preserve strict mode as an interpolation boundary rather than a sandbox, and demonstrate intentional PATH/PYTHONPATH execution selection with benign real-process tests. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent ec54957 commit ba7894d

7 files changed

Lines changed: 600 additions & 22 deletions

File tree

‎design/workflow-step.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,18 @@ automatically validate a definition passed to `execute()`. Guard invalid
4343
configurations in `execute()` too, returning a failed result rather than a
4444
successful default or an unhandled exception.
4545

46+
The built-in shell step preserves raw `run` interpolation by default.
47+
Per-step `strict: true` rejects expression markers in `run` during validation
48+
and execution. Its optional `env` mapping evaluates string templates separately,
49+
requires NUL-free string results encodable for the process environment on
50+
POSIX, and overrides only the child environment.
51+
`SPECKIT_WORKFLOW_DIR` remains engine-owned. This prevents workflow expression
52+
values from becoming shell source in strict mode; it is not a sandbox or a
53+
guarantee that an authored script handles environment data safely.
54+
Environment bindings and inherited settings can intentionally affect executable
55+
lookup, module loading, and runtime startup. Strict mode does not filter them;
56+
workflow source review must cover both `run` and `env` and referenced code.
57+
4658
New runs persist an execution tree. Resume replays completed occurrences into
4759
their expression contexts without calling their implementations, then continues
4860
at the unfinished occurrence. Selected branches, custom expansions, loop

‎docs/reference/bundles.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,11 @@ non-default source.
335335

336336
> **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.
337337

338+
For workflow components, inspect shell `run` and `env` fields and their
339+
referenced code, including changes introduced by active overlays. This applies
340+
even to `strict: true` steps: environment bindings can change executable lookup
341+
or runtime loading. See [Reviewing shell execution](workflows.md#reviewing-shell-execution).
342+
338343
### List the Catalog Stack
339344

340345
```bash

‎docs/reference/workflows.md‎

Lines changed: 85 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,7 @@ referents, so a pre-existing cache-file symlink can be replaced without changing
453453
its target. If a cache cannot be safely written, a warning is logged and
454454
discovery continues using the fetched catalog.
455455

456-
> **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)).
456+
> **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)).
457457

458458
### List Catalogs
459459

@@ -782,6 +782,32 @@ qualified `step_id`. A step emits completion after its checkpoint; containers
782782
emit before their children, while replayed completed occurrences emit no step
783783
events.
784784

785+
### Shell step
786+
787+
`type: shell` executes `run` through the platform shell, retaining support for
788+
pipes, redirects, and multiple commands. Set `strict: true` to reject `{{`
789+
expression markers anywhere in `run`, including inside quotes. This is checked
790+
both during validation and immediately before execution. `strict` must be a
791+
boolean; omitted or `false` preserves existing raw interpolation behavior.
792+
793+
Use an `env` mapping to bind dynamic values without inserting them into shell
794+
source. For example, on POSIX:
795+
796+
```yaml
797+
- id: display-result
798+
type: shell
799+
strict: true
800+
env:
801+
RESULT: "{{ steps.plan.output.stdout }}"
802+
run: 'printf "%s\n" "$RESULT"'
803+
```
804+
805+
For cross-platform scripts, read the environment in the program itself, such
806+
as `os.environ["RESULT"]` in Python. See
807+
[Shell Step Environment Variables](#shell-step-environment-variables) for the
808+
binding contract and [Interpolation and shell safety](#interpolation-and-shell-safety)
809+
for the remaining trust boundary.
810+
785811
> **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.
786812

787813
### GitHub step
@@ -1237,9 +1263,11 @@ If an interpolated value can contain characters like `;`, `|`, `&`, `$( )`, back
12371263
- **Workflow `inputs.*`** — supplied by whoever runs the workflow.
12381264
- **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.
12391265

1240-
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:
1266+
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.
12411267

1242-
- **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.
1268+
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:
1269+
1270+
- **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.
12431271

12441272
```yaml
12451273
inputs:
@@ -1262,6 +1290,60 @@ Shell steps automatically receive the following environment variables:
12621290
| -------- | ----------- |
12631291
| `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. |
12641292

1293+
An optional per-step `env` mapping is available in both strict and non-strict
1294+
mode. Names must match `[A-Za-z_][A-Za-z0-9_]*`; names are literal, not
1295+
expressions. Values must be strings and may contain workflow expressions.
1296+
Each value is evaluated once and must resolve to a string without NUL
1297+
characters. On POSIX, the string must also be encodable for the process
1298+
environment using the filesystem encoding and its error handler.
1299+
Invalid bindings fail the step before launching a process.
1300+
1301+
Single-expression bindings preserve the expression's result type:
1302+
`COUNT: "{{ inputs.count }}"` rejects a number, boolean, null, list, or mapping
1303+
instead of converting it to text. Mixed templates use standard expression
1304+
interpolation: `COUNT: "n={{ inputs.count }}"` produces `n=42` for integer
1305+
`42`; embedded null becomes empty text, and other values use their string
1306+
representation. To serialize structured data explicitly, use `to_json`.
1307+
1308+
Bindings override inherited environment values for that subprocess only;
1309+
they do not modify the parent process or subsequent steps. An empty string is
1310+
valid. `SPECKIT_WORKFLOW_DIR` is reserved (case-insensitively) and cannot be
1311+
overridden. Keep environment names and executable-loading settings under the
1312+
workflow author's control: environment binding is a data channel, not a sandbox.
1313+
1314+
### Reviewing shell execution
1315+
1316+
Before installing or running an unfamiliar workflow, audit each shell step's
1317+
`run` **and** `env`, including nested branches and fan-out templates. Review
1318+
referenced scripts and resources, called workflows, and active overlay YAML
1319+
that can replace or insert steps. Apply the same review to workflow components
1320+
of bundles, and repeat it when definitions, overlays, or referenced code change.
1321+
Custom step packages require their own source review because loading them
1322+
executes Python code.
1323+
1324+
An environment value can control execution rather than merely supply data.
1325+
For example, `PATH` selects executables, `PYTHONPATH` affects Python imports,
1326+
`BASH_ENV` can select Bash startup code, and `NODE_OPTIONS` can change Node.js
1327+
startup behavior. Native loaders and other programs have their own settings;
1328+
these examples are not an exhaustive list. A fixed `run` can therefore execute
1329+
different code even with `strict: true` and no `eval`. Strict mode neither
1330+
filters execution-controlling bindings nor cleans the inherited environment.
1331+
Keep those settings literal and author-vetted, or constrain dynamic values to
1332+
an approved set of trusted paths/options. Use unconstrained input or agent
1333+
output only for application-data variables whose consumers handle it as data.
1334+
1335+
When a shell step invokes Specify itself, catalog overrides such as
1336+
`SPECKIT_WORKFLOW_CATALOG_URL` can also change where child commands discover
1337+
or install components. Vet those bindings alongside the
1338+
[active catalog sources](catalogs.md#discovery-and-trust).
1339+
1340+
Workflow validation checks configuration, not whether environment settings or
1341+
commands are safe. `specify workflow info` shows metadata and step identities;
1342+
`specify workflow resolve` shows layer and step attribution. Neither displays
1343+
the complete `run`/`env` configuration or replaces reading the contributing
1344+
source files. Catalog install permission, bundle installation, and strict mode
1345+
are not security endorsements.
1346+
12651347
## Input Types
12661348

12671349
| Type | Coercion |

‎src/specify_cli/workflows/step/shell/__init__.py‎

Lines changed: 87 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import locale
88
import math
99
import os
10+
import re
1011
import subprocess
1112
from typing import Any
1213

@@ -23,6 +24,13 @@ class ShellStep(StepBase):
2324
type_key = "shell"
2425

2526
def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
27+
binding_errors = self._binding_errors(config)
28+
if binding_errors:
29+
return StepResult(
30+
status=StepStatus.FAILED,
31+
error="; ".join(binding_errors),
32+
output={"exit_code": -1, "stdout": "", "stderr": "invalid shell bindings"},
33+
)
2634
run_cmd = config.get("run", "")
2735
if isinstance(run_cmd, str) and "{{" in run_cmd:
2836
run_cmd = evaluate_expression(run_cmd, context)
@@ -45,15 +53,33 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
4553
)
4654

4755
env = {**os.environ}
56+
for name, template in config.get("env", {}).items():
57+
try:
58+
value = evaluate_expression(template, context)
59+
except ValueError as exc:
60+
return StepResult(
61+
status=StepStatus.FAILED,
62+
error=f"Shell step {config.get('id', '?')!r}: env {name!r}: {exc}",
63+
output={"exit_code": -1, "stdout": "", "stderr": "invalid environment expression"},
64+
)
65+
value_error = self._environment_value_error(name, value)
66+
if value_error is not None:
67+
return StepResult(
68+
status=StepStatus.FAILED,
69+
error=f"Shell step {config.get('id', '?')!r}: {value_error}",
70+
output={"exit_code": -1, "stdout": "", "stderr": "invalid environment value"},
71+
)
72+
env[name.upper() if os.name == "nt" else name] = value
4873
if context.workflow_dir:
4974
env["SPECKIT_WORKFLOW_DIR"] = context.workflow_dir
5075
else:
5176
env.pop("SPECKIT_WORKFLOW_DIR", None)
5277

5378
# NOTE: shell=True is required to support pipes, redirects, and
5479
# multi-command expressions in workflow YAML. Workflow authors
55-
# control commands; catalog-installed workflows should be reviewed
56-
# before use (see PUBLISHING.md for security guidance).
80+
# control commands. strict=True rejects expression interpolation in
81+
# run; dynamic data can instead be bound through env without becoming
82+
# shell source. Neither mode sandboxes authored shell commands.
5783
try:
5884
proc = subprocess.run( # noqa: S602 -- intentional shell=True (see NOTE above)
5985
run_cmd,
@@ -121,6 +147,51 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
121147
output={"exit_code": -1, "stdout": "", "stderr": str(exc)},
122148
)
123149

150+
@staticmethod
151+
def _environment_value_error(name: str, value: Any) -> str | None:
152+
if not isinstance(value, str):
153+
return f"env {name!r} must be a string, got {type(value).__name__}."
154+
if "\x00" in value:
155+
return f"env {name!r} must not contain NUL characters."
156+
if os.name == "posix":
157+
try:
158+
os.fsencode(value)
159+
except UnicodeEncodeError:
160+
return f"env {name!r} cannot be encoded for the process environment."
161+
return None
162+
163+
@classmethod
164+
def _binding_errors(cls, config: dict[str, Any]) -> list[str]:
165+
errors: list[str] = []
166+
prefix = f"Shell step {config.get('id', '?')!r}: "
167+
strict = config.get("strict", False)
168+
if not isinstance(strict, bool):
169+
errors.append(prefix + "'strict' must be a boolean.")
170+
elif strict:
171+
run = config.get("run")
172+
if not isinstance(run, str):
173+
errors.append(prefix + "'run' must be a string in strict mode.")
174+
elif "{{" in run:
175+
errors.append(
176+
prefix + "strict mode forbids expressions in 'run'; "
177+
"bind dynamic values through 'env' instead."
178+
)
179+
180+
bindings = config.get("env", {})
181+
if not isinstance(bindings, dict):
182+
errors.append(prefix + "'env' must be a mapping.")
183+
return errors
184+
for name, template in bindings.items():
185+
if not isinstance(name, str) or re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*", name) is None:
186+
errors.append(prefix + f"invalid environment variable name {name!r}.")
187+
continue
188+
if name.upper() == "SPECKIT_WORKFLOW_DIR":
189+
errors.append(prefix + f"environment variable {name!r} is reserved.")
190+
value_error = cls._environment_value_error(name, template)
191+
if value_error is not None:
192+
errors.append(prefix + value_error)
193+
return errors
194+
124195
@staticmethod
125196
def _timeout_error(config: dict[str, Any]) -> str | None:
126197
"""Return an error message if ``config['timeout']`` is invalid, else None.
@@ -157,20 +228,20 @@ def _timeout_error(config: dict[str, Any]) -> str | None:
157228

158229
def validate(self, config: dict[str, Any]) -> list[str]:
159230
errors = super().validate(config)
160-
if "run" not in config:
161-
errors.append(
162-
f"Shell step {config.get('id', '?')!r} is missing 'run' field."
163-
)
164-
elif not isinstance(config["run"], str):
165-
# execute() str()-coerces run and invokes it under shell=True, so a
166-
# null or list 'run' would run the Python repr ('None', "['echo']")
167-
# as a command. Reject non-strings at validation, mirroring the
168-
# command-step input/options and gate options type checks. An
169-
# expression like "{{ ... }}" is still a str, so it stays valid.
170-
errors.append(
171-
f"Shell step {config.get('id', '?')!r}: 'run' must be a string, "
172-
f"got {type(config['run']).__name__}."
173-
)
231+
errors.extend(self._binding_errors(config))
232+
# Strict run errors are already reported by _binding_errors().
233+
if config.get("strict") is not True:
234+
if "run" not in config:
235+
errors.append(
236+
f"Shell step {config.get('id', '?')!r} is missing 'run' field."
237+
)
238+
elif not isinstance(config["run"], str):
239+
# Legacy execution coerces run to shell source; validation
240+
# must still reject non-strings without changing that path.
241+
errors.append(
242+
f"Shell step {config.get('id', '?')!r}: 'run' must be a string, "
243+
f"got {type(config['run']).__name__}."
244+
)
174245
output_format = config.get("output_format")
175246
if output_format is not None and output_format != "json":
176247
errors.append(

0 commit comments

Comments
 (0)