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

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

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

Please address Copilot feedback

…dation self.requires holds the raw parsed value, which before validate_workflow() runs may be a non-mapping (None for a bare 'requires:', a list for 'requires: []', etc.). Annotating it dict[str, Any] was misleading for editors/type-checkers; use Any and document that validate_workflow() enforces the mapping shape. Addresses Copilot review feedback on engine.py. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com>

zied-jlassi · 2026-06-23T19:11:22Z

Thanks @mnriem — addressed the latest Copilot feedback:

engine.py annotation — WorkflowDefinition.requires was typed dict[str, Any], but it holds the raw parsed value, so before validate_workflow() runs it can be a non-mapping (None for a bare requires:, a list for requires: [], etc.). Retyped it as Any and added a comment noting that validate_workflow() enforces the mapping shape, so editors/type-checkers aren't misled into assuming it's always a dict. (3f32bed)
PR description — reconciled it with the implementation: the recognised requires keys are now listed as just speckit_version and integrations (dropped the stale tools/mcp, which we already removed from _RECOGNIZED_REQUIRES_KEYS per your earlier note).

Validation: pytest tests/test_workflows.py → 292 passed, ruff check clean. The annotation change is type-only (module uses from __future__ import annotations), no runtime behaviour change.

Copilot

Copilot's findings

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

+    # Guard on ``is not None`` rather than truthiness: a falsy but non-mapping
+    # value (e.g. ``requires: []`` or ``requires: ''``) is still an authoring
+    # error and must surface, whereas ``requires:`` (YAML null) is treated as
+    # an omitted block.
+    if definition.requires is not None:
+        if not isinstance(definition.requires, dict):
+            errors.append(
+                "'requires' must be a mapping (or omitted)."
+            )
+        else:
+            for key in definition.requires:
+                if key == "permissions":
+                    errors.append(
+                        "'requires.permissions' is not a recognized or "
+                        "enforced capability gate — shell steps always run "
+                        "with the user's privileges. Remove it and gate "
+                        "sensitive steps with a 'gate' step instead."
+                    )
+                elif key not in _RECOGNIZED_REQUIRES_KEYS:
+                    errors.append(
+                        f"Unknown 'requires' key {key!r}. Recognized keys: "
+                        f"{', '.join(sorted(_RECOGNIZED_REQUIRES_KEYS))}."
+                    )
+


+    def test_requires_must_be_mapping(self):
+        from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow
+
+        definition = WorkflowDefinition.from_string("""
+workflow:
+  id: "test"
+  name: "Test"
+  version: "1.0.0"
+requires: "claude"
+steps:
+  - id: step-one
+    command: speckit.specify
+""")
+        errors = validate_workflow(definition)
+        assert any("'requires' must be a mapping" in e for e in errors)
+


mnriem · 2026-06-23T19:33:04Z

Please address Copilot feedback

Address Copilot review: validate requires the same way as inputs. A bare requires: parses as YAML null and was previously treated as an omitted block, which is inconsistent with inputs and lets a stray requires: line be silently ignored. Drop the is-not-None guard and check isinstance(..., dict) directly: an omitted block still defaults to {} (valid), but a present-but-non-mapping value -- YAML null, [] or '' -- is now an authoring error that surfaces. Tests: add YAML-null rejection + an omitted-is-still-valid guard test. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com>

zied-jlassi · 2026-06-24T12:06:40Z

Thanks @mnriem — addressed the Copilot feedback in 30191ec.

requires now validates the same way as inputs: I dropped the is not None guard and check isinstance(definition.requires, dict) directly. A bare requires: (YAML null) is no longer treated as an omitted block — it now surfaces as 'requires' must be a mapping (or omitted)., so a stray requires: line fails loudly instead of being silently ignored. A genuinely omitted requires still defaults to {} and stays valid.

Tests: added test_requires_yaml_null_is_rejected_as_non_mapping and a test_requires_omitted_is_valid guard (so the YAML-null rejection doesn't over-correct into flagging the omitted case), and refreshed the now-stale docstring on the empty-list test.

Verified in an isolated container against the repo's own toolchain:

uvx ruff check src/ → All checks passed!
uv run pytest tests/test_workflows.py → 294 passed

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

Comment thread docs/reference/workflows.md

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

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

Comment thread src/specify_cli/workflows/engine.py

mnriem requested a review from Copilot June 23, 2026 19:27

Copilot started reviewing on behalf of mnriem June 23, 2026 19:27 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

Conversation

zied-jlassi commented Jun 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Uh oh!

Uh oh!

mnriem commented Jun 23, 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!

zied-jlassi commented Jun 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zied-jlassi commented Jun 21, 2026 •

edited

Loading