Skip to content

feat(workflows): add opt-in strict shell execution - #5181

Open
mnriem wants to merge 4 commits into
github:mainfrom
mnriem:mnriem-workflow-shell-injection
Open

mnriem wants to merge 4 commits into
github:mainfrom
mnriem:mnriem-workflow-shell-injection

Conversation

@mnriem

@mnriem mnriem commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

Description

Add opt-in strict: true for workflow shell steps. Strict mode rejects workflow expression markers in run during validation and execution, while retaining fixed shell scripts, pipes, and redirects.

Add per-step env bindings that evaluate expressions separately and pass string values through the child process environment rather than inserting them into shell source. Invalid bindings fail before process launch, and SPECKIT_WORKFLOW_DIR remains engine-owned.

Existing raw interpolation remains unchanged when strict is omitted or false. Update the workflow reference, authoring guidance, and step design to explain adoption and remaining risks: strict mode is not a sandbox and does not protect scripts that reinterpret environment data as code.

Testing

  • Tested locally with uv run specify --help
  • Ran existing tests with uv sync && uv run pytest
  • Tested with a sample project (if applicable)

Used the documented worktree-local interpreter instead of the legacy test command in the checklist:

  • LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests -q -n 4 — passed: 13,704 passed, 130 skipped.
  • uv run specify --help — passed.
  • Sample-project CLI execution with strict: true and an expression-bound environment value — completed; shell metacharacters remained literal output.

Regression coverage includes strict-mode rejection before subprocess launch, invalid configuration and resolved values, input/prior-output/fan-out-item bindings, environment isolation, legacy interpolation, fixed shell pipelines, and engine failure/continuation behavior.

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (fill in the disclosure below)

AI disclosure: GitHub Copilot, powered by GPT-6.1 Sol (gpt-6.1-sol), authored the implementation, tests, and documentation and executed validation, Git, and PR commands on behalf of @mnriem. Work was performed in an interactive session with autonomous authoring and execution under user-directed scope; reasoning effort was not exposed. The user selected the opt-in design; no human line-by-line review is claimed. A security-review subagent assisted the initial assessment; its model and settings were not reported.

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>
Copilot AI auto review requested due to automatic review settings October 29, 2026 14:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

On POSIX, lone-surrogate shell bindings can pass validation and trigger an uncaught UnicodeEncodeError, aborting workflows even when continue_on_error is enabled.

2 open issues
What changed in this PR

Adds opt-in strict execution for Spec Kit workflow shell steps, separating dynamic environment data from shell source while preserving legacy interpolation.

Changes:

  • Reject workflow expression markers in strict run fields during validation and execution.
  • Add per-step env bindings with value validation and child-process-only overrides.
  • Expand regression coverage and document adoption, compatibility, and remaining security risks.
File Description
workflows/​README.md Introduces strict shell usage and environment bindings.
workflows/​PUBLISHING.md Adds security guidance for workflow authors.
tests/​workflows/​test_shell_step.py Covers strict rejection, environment handling, compatibility, and workflow continuation.
src/​specify_cli/​workflows/​step/​shell/​__init__.py Implements strict validation and environment binding execution.
docs/​reference/​workflows.md Documents configuration, binding rules, and trust boundaries.
design/​workflow-step.md Defines strict execution and environment ownership in the step contract.

Review effort: Max (auto)

Comment thread src/specify_cli/workflows/step/shell/__init__.py
Comment thread docs/reference/workflows.md Outdated
@mnriem mnriem added the triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review label Oct 29, 2026
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>
Copilot AI auto review requested due to automatic review settings October 29, 2026 15:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Negative coverage for None and list run overrides is missing, leaving unverified that validation and execution both reject them before calling subprocess.run.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Low severity Add negative tests for non-string strict run values

tests/​workflows/​test_shell_step.py:70

The strict-mode guard for non-string run values has no negative coverage. Add None and list overrides here to verify validation and execution both reject them before subprocess.run is called. This covers the prevented behavior required by CONTRIBUTING.md:229–235.

2 resolved since last review

Review effort: Max (auto)

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>
Copilot AI auto review requested due to automatic review settings October 29, 2026 16:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human review is needed to determine whether duplicate diagnostics for missing or non-string run values in strict mode warrant follow-up.

No open issues 🎉

Review effort: Max (auto)

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>
Copilot AI auto review requested due to automatic review settings October 29, 2026 16:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Human review is needed to judge whether the pre-install audit adequately covers environment bindings that can alter shell-step execution, even for strict steps.

No open issues 🎉

Review effort: Max (auto)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants