fix(workflows): validate requires keys and reject phantom permissions gate by zied-jlassi · Pull Request #3079 · github/spec-kit

zied-jlassi · 2026-06-21T21:35:45Z

Description

A workflow's requires: block was parsed (WorkflowDefinition.requires) but its keys were never validated, so a typo or an unsupported key was silently ignored.

Most importantly, an author could write:

requires:
  permissions:
    shell: true

expecting a runtime capability gate — but no such gate exists. A shell step always runs with the user's privileges, so this declaration gives a false sense of sandboxing. (This came up in #2440, where it was understandably assumed that such a declaration was already enforced.)

This PR makes validate_workflow honest about requires:

Only the recognised keys are accepted: speckit_version, integrations, tools, mcp.
Any unknown key is rejected (a typo surfaces at validation time instead of being silently dropped) — same spirit as the recent "fail loudly on unknown" hardening.
requires.permissions is rejected with an explicit message pointing authors at a gate step for approval, so nobody mistakes it for a security boundary.

It does not add any per-step permission system or runtime prompt — requires stays advisory. The model comment and the docs (docs/reference/workflows.md, workflows/PUBLISHING.md) are updated to say so plainly.

Testing

Ran existing tests with pytest (full suite green, no regressions)
Tested locally with uv run specify --help
Added tests: valid recognised keys, non-mapping requires, unknown key, and requires.permissions

Validation is reached on the user-facing paths (workflow add / info / run), so the new errors actually surface.

AI Disclosure

I did not use AI assistance for this contribution
I did use AI assistance (describe below)

AI assistance (an AI coding agent) was used for the initial code review that surfaced the issue, and to help draft the implementation, tests, and documentation wording. All changes were reviewed and verified by me (red→green tests, full suite, ruff) before submitting.

… gate A workflow's `requires` block was parsed but its keys were never validated, so a typo or an unsupported key was silently ignored. Most importantly, authors could write `requires.permissions.shell: true` expecting a runtime capability gate — but no such gate exists: a `shell` step always runs with the user's privileges. The declaration gave a false sense of sandboxing. `validate_workflow` now accepts only the recognised keys (`speckit_version`, `integrations`, `tools`, `mcp`) and rejects anything else, with an explicit error for `requires.permissions` pointing authors to `gate` steps for approval. Docs and the model comment are updated to state that `requires` is advisory, not a security boundary. - Reject non-mapping `requires`, unknown keys, and `requires.permissions` - Clarify workflows reference + PUBLISHING.md shell-step guidance - Tests for valid keys, non-mapping, unknown key, and permissions Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> Assisted-by: AI

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Tightens workflow validation so requires: only accepts known keys and explicitly rejects requires.permissions to avoid implying a non-existent runtime permission gate.

Changes:

Add validation for requires type and allowed keys; reject requires.permissions with a clear error.
Add tests covering valid/invalid requires cases (unknown keys, non-mapping, permissions).
Update docs to clarify shell runs with user privileges and requires is advisory.

Show a summary per file

File	Description
workflows/PUBLISHING.md	Documents `shell` privilege model and that `requires` is advisory (no permissions gate).
tests/test_workflows.py	Adds tests for `requires` validation behavior (recognized keys, non-mapping, unknown key, permissions).
src/specify_cli/workflows/engine.py	Implements `requires` key/type validation and explicit rejection of `requires.permissions`.
docs/reference/workflows.md	Adds a security note about `shell` steps and (intended) advisory nature of `requires`.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 4/4 changed files
Comments generated: 4

mnriem

Please address Copilot feedback

Follow-up to the review on github#3079: - Guard `requires` validation on `is not None` instead of truthiness so a falsy non-mapping value (e.g. `requires: []` or `requires: ''`) is reported as an error instead of being silently skipped; `requires:` (YAML null) is still treated as an omitted block. Add a regression test. - Reword the workflows security note so `requires.permissions` is shown as rejected/unsupported rather than as a valid example of `requires`. - Standardize on US spelling (`_RECOGNIZED_REQUIRES_KEYS`, "recognized") to match the surrounding code and ease searching. - Tighten the permissions-rejection test to assert on specific message markers (`requires.permissions` and the `gate` guidance) so it fails if the validation path or wording drifts. Assisted-by: AI Signed-off-by: Zied Jlassi (Architect AI) <6190550+zied-jlassi@users.noreply.github.com>

Copilot

Copilot's findings

Files reviewed: 4/4 changed files
Comments generated: 3

mnriem · 2026-06-22T17:12:01Z

Please address Copilot feedback

zied-jlassi · 2026-06-22T17:17:37Z

Thanks @mnriem — addressed the Copilot feedback in the latest commit:

Truthiness vs None (engine.py) — validation now guards on if definition.requires is not None: instead of truthiness, so a falsy-but-non-mapping value (requires: [] / requires: '') is reported instead of silently skipped; a bare requires: (YAML null) is still treated as omitted. Added a regression test (test_requires_empty_sequence_is_rejected_as_non_mapping).
Docs example (workflows.md) — reworded the security note so requires.permissions is presented as rejected/unsupported rather than as a valid example of requires.
Spelling — standardised on US spelling to match the surrounding code (_RECOGNIZED_REQUIRES_KEYS, "recognized" in messages/comments).
Test assertion — the permissions-rejection test now asserts on specific markers (requires.permissions + the gate guidance) instead of the permissive "permissions" + "not".

Validation: tests/test_workflows.py 292 passed (the 5 requires tests included). No new ruff findings on src/. Scope unchanged — still validation + honest docs, no runtime gate.

Copilot

Copilot's findings

Files reviewed: 4/4 changed files
Comments generated: 1

 | `fan-out`    | Dispatch a step for each item in a list          |
 | `fan-in`     | Aggregate results from a fan-out step            |

+> **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.


mnriem · 2026-06-22T18:09:47Z

The core fix is solid — validating requires keys and rejecting requires.permissions with a pointer to a gate step is a real improvement, and the tests cover it well.

One change needed before this lands: drop tools and mcp from the recognized set. They're out of scope for this PR, and the justification doesn't hold for workflows:

PUBLISHING.md (L59–62) and the workflow docs only ever document speckit_version and integrations. This PR's own PUBLISHING.md edit is what introduces the "recognised keys: …, tools, mcp" claim.
requires.tools exists only in a different schema — the integration descriptor in integrations/catalog.py (~L709), not workflow requires.
requires.mcp doesn't appear anywhere as a requires key (the only mcp hits are an unrelated cursor-agent --approve-mcps flag). Nothing resolves it for workflows or integrations.

So for workflows, tools/mcp have no docs (pre-PR), no schema, and no consumer. Whitelisting them undercuts the PR's own thesis: an author could write requires.tools:, pass validation, and have nothing read or enforce it — the same false-affordance trap you're closing for permissions.

Please:

Set _RECOGNIZED_REQUIRES_KEYS = {"speckit_version", "integrations"}.
Remove the tools/mcp additions from the PUBLISHING.md line.
Update the code comment so it no longer claims tools/mcp are "resolved by the bundler" for workflows.

Expanding the recognized requires surface is a separate decision and shouldn't ride along with this fix. If there's a future need for tools/mcp in workflow requires, that should go through its own issue/PR with docs and a real consumer.

…s/mcp) tools and mcp belong to the bundle manifest requires schema (bundler/models/manifest.py, resolved in bundler/services/resolver.py), not the workflow requires validated here. Drop them from _RECOGNIZED_REQUIRES_KEYS and revert the PUBLISHING.md claim that this PR had introduced, so workflow requires only recognizes speckit_version and integrations. This keeps the existing docs accurate and resolves the inline doc-consistency review comments. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com>

zied-jlassi · 2026-06-23T05:37:46Z

Thanks @mnriem — agreed, and you're right about the scope. I've dropped tools and mcp from _RECOGNIZED_REQUIRES_KEYS, so workflow requires now only recognizes speckit_version and integrations.

Root cause on my side: I'd sourced those two keys from the bundle manifest requires schema (bundler/models/manifest.py, resolved in bundler/services/resolver.py), which is a different schema from the workflow requires this PR validates. I also reverted the PUBLISHING.md line that introduced the tools/mcp claim — with those gone the existing docs are accurate again, which also resolves the four inline doc-consistency comments.

Validation: pytest -k "requires or Validation" green, ruff clean.

Copilot

Copilot's findings

Files reviewed: 4/4 changed files
Comments generated: 2

+        # Advisory pre-conditions (spec-kit version / integrations a workflow
+        # expects). Validated by ``validate_workflow`` (recognized keys only;
+        # see ``_RECOGNIZED_REQUIRES_KEYS``) but NOT enforced at run time — they
+        # are not a security boundary. In particular there is no
+        # ``requires.permissions`` capability gate: shell steps always run with
+        # the user's privileges.
        self.requires: dict[str, Any] = data.get("requires", {})


+# Keys accepted under a workflow's ``requires`` block: the advisory
+# pre-conditions documented for workflows (``speckit_version`` and
+# ``integrations``). This is the *workflow* schema only — the bundle manifest's
+# ``requires`` (see ``bundler/models/manifest.py``) is a separate schema that
+# also carries ``tools``/``mcp``; those are not workflow ``requires`` keys.
+# Any other key — notably ``permissions`` — is rejected by ``validate_workflow``
+# so it is never mistaken for an enforced runtime control.
+_RECOGNIZED_REQUIRES_KEYS = frozenset({"speckit_version", "integrations"})


mnriem · 2026-06-23T12:51:33Z

Please address Copilot feedback

zied-jlassi requested a review from mnriem as a code owner June 21, 2026 21:35

mnriem requested a review from Copilot June 22, 2026 13:56

Copilot AI reviewed Jun 22, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/engine.py Outdated

Comment thread docs/reference/workflows.md Outdated

Comment thread src/specify_cli/workflows/engine.py Outdated

Comment thread tests/test_workflows.py Outdated

mnriem requested changes Jun 22, 2026

View reviewed changes

mnriem requested a review from Copilot June 22, 2026 17:04

Copilot started reviewing on behalf of mnriem June 22, 2026 17:04 View session

Copilot AI reviewed Jun 22, 2026

View reviewed changes

Comment thread src/specify_cli/workflows/engine.py

Comment thread src/specify_cli/workflows/engine.py

Comment thread docs/reference/workflows.md

mnriem requested a review from Copilot June 22, 2026 17:27

Copilot started reviewing on behalf of mnriem June 22, 2026 17:28 View session

Copilot AI reviewed Jun 22, 2026

View reviewed changes

mnriem requested a review from Copilot June 23, 2026 12:05

Copilot started reviewing on behalf of mnriem June 23, 2026 12:05 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

Conversation

zied-jlassi commented Jun 21, 2026

Description

Testing

AI Disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mnriem left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mnriem commented Jun 22, 2026

Uh oh!

zied-jlassi commented Jun 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

mnriem commented Jun 22, 2026

Uh oh!

zied-jlassi commented Jun 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

mnriem commented Jun 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants