mirror of
https://github.com/github/spec-kit.git
synced 2026-07-03 12:28:06 +08:00
fix(workflows): validate requires keys and reject phantom permissions gate (#3079)
* fix(workflows): validate requires keys and reject phantom permissions 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 * fix(workflows): address review feedback on requires validation Follow-up to the review on #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> * fix(workflows): scope requires validation to workflow keys (drop tools/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> * refactor(workflows): type WorkflowDefinition.requires as Any pre-validation 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> * fix(workflows): reject YAML-null requires: as a non-mapping 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> --------- Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> Signed-off-by: Zied Jlassi (Architect AI) <6190550+zied-jlassi@users.noreply.github.com>
This commit is contained in:
Zied Jlassi
@@ -270,6 +270,8 @@ specify workflow run speckit -i spec="Build a kanban board with drag-and-drop ta
|
||||
| `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.
|
||||
|
||||
## Expressions
|
||||
|
||||
Steps can reference inputs and previous step outputs using `{{ expression }}` syntax:
|
||||
|
||||
@@ -52,9 +52,18 @@ class WorkflowDefinition:
|
||||
if not isinstance(self.default_options, dict):
|
||||
self.default_options = {}
|
||||
|
||||
# Requirements (declared but not yet enforced at runtime;
|
||||
# enforcement is a planned enhancement)
|
||||
self.requires: dict[str, Any] = data.get("requires", {})
|
||||
# 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.
|
||||
#
|
||||
# Holds the raw parsed value, so before ``validate_workflow`` runs it may
|
||||
# be a non-mapping (``None`` for a bare ``requires:``, a list for
|
||||
# ``requires: []``, etc.); typed ``Any`` rather than ``dict[str, Any]``
|
||||
# to avoid implying it is always a mapping at this point.
|
||||
self.requires: Any = data.get("requires", {})
|
||||
|
||||
# Inputs
|
||||
self.inputs: dict[str, Any] = data.get("inputs", {})
|
||||
@@ -87,6 +96,15 @@ class WorkflowDefinition:
|
||||
# ID format: lowercase alphanumeric with hyphens
|
||||
_ID_PATTERN = re.compile(r"^[a-z0-9][a-z0-9-]*[a-z0-9]$|^[a-z0-9]$")
|
||||
|
||||
# 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"})
|
||||
|
||||
# Valid step types (matching STEP_REGISTRY keys)
|
||||
def _get_valid_step_types() -> set[str]:
|
||||
"""Return valid step types from the registry, with a built-in fallback."""
|
||||
@@ -177,6 +195,36 @@ def validate_workflow(definition: WorkflowDefinition) -> list[str]:
|
||||
f"Input {input_name!r} has invalid default: {exc}"
|
||||
)
|
||||
|
||||
# -- Requires ---------------------------------------------------------
|
||||
# ``requires`` declares advisory pre-conditions (the spec-kit version and
|
||||
# integrations a workflow expects). Only a fixed set of keys is recognized;
|
||||
# reject anything else so authoring typos surface here instead of being
|
||||
# silently ignored at runtime. In particular ``requires.permissions`` is
|
||||
# rejected explicitly: it reads like a runtime capability gate, but no such
|
||||
# gate exists — a ``shell`` step always runs with the user's privileges, so
|
||||
# declaring it would give a false sense of sandboxing.
|
||||
#
|
||||
# Mirror ``inputs`` validation: an omitted block defaults to ``{}`` and is
|
||||
# valid, but any present-but-non-mapping value — ``requires:`` (YAML null),
|
||||
# ``requires: []`` or ``requires: ''`` — is an authoring error and must
|
||||
# surface here rather than be silently ignored at runtime.
|
||||
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))}."
|
||||
)
|
||||
|
||||
# -- Steps ------------------------------------------------------------
|
||||
if not isinstance(definition.steps, list):
|
||||
errors.append("'steps' must be a list.")
|
||||
|
||||
@@ -2115,6 +2115,148 @@ steps:
|
||||
errors = validate_workflow(definition)
|
||||
assert any("invalid type" in e.lower() for e in errors)
|
||||
|
||||
def test_requires_with_recognized_keys_is_valid(self):
|
||||
from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow
|
||||
|
||||
definition = WorkflowDefinition.from_string("""
|
||||
workflow:
|
||||
id: "test"
|
||||
name: "Test"
|
||||
version: "1.0.0"
|
||||
requires:
|
||||
speckit_version: ">=0.7.2"
|
||||
integrations:
|
||||
any: ["claude", "gemini"]
|
||||
steps:
|
||||
- id: step-one
|
||||
command: speckit.specify
|
||||
""")
|
||||
errors = validate_workflow(definition)
|
||||
assert errors == []
|
||||
|
||||
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)
|
||||
|
||||
def test_requires_unknown_key_is_rejected(self):
|
||||
from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow
|
||||
|
||||
definition = WorkflowDefinition.from_string("""
|
||||
workflow:
|
||||
id: "test"
|
||||
name: "Test"
|
||||
version: "1.0.0"
|
||||
requires:
|
||||
speckit_version: ">=0.7.2"
|
||||
typo_key: true
|
||||
steps:
|
||||
- id: step-one
|
||||
command: speckit.specify
|
||||
""")
|
||||
errors = validate_workflow(definition)
|
||||
assert any("typo_key" in e and "requires" in e for e in errors)
|
||||
|
||||
def test_requires_permissions_is_rejected_as_not_enforced(self):
|
||||
"""A `requires.permissions` block looks like a runtime capability gate
|
||||
but no such gate exists — shell steps always run with the user's
|
||||
privileges. Reject it explicitly so authors are not misled into
|
||||
believing the declaration sandboxes execution.
|
||||
"""
|
||||
from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow
|
||||
|
||||
definition = WorkflowDefinition.from_string("""
|
||||
workflow:
|
||||
id: "test"
|
||||
name: "Test"
|
||||
version: "1.0.0"
|
||||
requires:
|
||||
permissions:
|
||||
shell: true
|
||||
steps:
|
||||
- id: run
|
||||
type: shell
|
||||
run: "echo hi"
|
||||
""")
|
||||
errors = validate_workflow(definition)
|
||||
# Assert on specific markers from the intended message (the offending
|
||||
# key and the `gate` remediation) so the test fails if the validation
|
||||
# path or wording drifts, rather than passing on any error that merely
|
||||
# happens to contain "permissions" and "not".
|
||||
assert any("requires.permissions" in e and "gate" in e for e in errors)
|
||||
|
||||
def test_requires_empty_sequence_is_rejected_as_non_mapping(self):
|
||||
"""A non-mapping ``requires`` (e.g. an empty list) is an authoring
|
||||
error. Mirroring ``inputs``, validation checks ``isinstance(..., dict)``
|
||||
so ``requires: []`` surfaces instead of silently passing.
|
||||
"""
|
||||
from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow
|
||||
|
||||
definition = WorkflowDefinition.from_string("""
|
||||
workflow:
|
||||
id: "test"
|
||||
name: "Test"
|
||||
version: "1.0.0"
|
||||
requires: []
|
||||
steps:
|
||||
- id: step-one
|
||||
command: speckit.specify
|
||||
""")
|
||||
errors = validate_workflow(definition)
|
||||
assert any("'requires' must be a mapping" in e for e in errors)
|
||||
|
||||
def test_requires_yaml_null_is_rejected_as_non_mapping(self):
|
||||
"""A bare ``requires:`` parses as YAML null. Like ``inputs``, a present
|
||||
block must be a mapping, so YAML null is rejected as an authoring error
|
||||
rather than being silently treated as an omitted block. (A truly
|
||||
omitted ``requires`` defaults to ``{}`` and stays valid.)
|
||||
"""
|
||||
from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow
|
||||
|
||||
definition = WorkflowDefinition.from_string("""
|
||||
workflow:
|
||||
id: "test"
|
||||
name: "Test"
|
||||
version: "1.0.0"
|
||||
requires:
|
||||
steps:
|
||||
- id: step-one
|
||||
command: speckit.specify
|
||||
""")
|
||||
errors = validate_workflow(definition)
|
||||
assert any("'requires' must be a mapping" in e for e in errors)
|
||||
|
||||
def test_requires_omitted_is_valid(self):
|
||||
"""A workflow with no ``requires`` block at all defaults to ``{}`` and
|
||||
must validate cleanly — only a present-but-non-mapping value is an
|
||||
error (guards against over-correcting YAML-null rejection into also
|
||||
flagging the omitted case).
|
||||
"""
|
||||
from specify_cli.workflows.engine import WorkflowDefinition, validate_workflow
|
||||
|
||||
definition = WorkflowDefinition.from_string("""
|
||||
workflow:
|
||||
id: "test"
|
||||
name: "Test"
|
||||
version: "1.0.0"
|
||||
steps:
|
||||
- id: step-one
|
||||
command: speckit.specify
|
||||
""")
|
||||
errors = validate_workflow(definition)
|
||||
assert not any("requires" in e for e in errors)
|
||||
|
||||
|
||||
# ===== Workflow Engine Tests =====
|
||||
|
||||
|
||||
@@ -268,6 +268,7 @@ When releasing a new version:
|
||||
|
||||
### Shell Steps
|
||||
|
||||
- **Shell runs with the user's privileges** — a `shell` step executes a local command directly; there is no capability sandbox. `requires` is an advisory pre-condition block (recognised keys: `speckit_version`, `integrations`), **not** a runtime permission gate — there is no `requires.permissions`. Gate sensitive commands explicitly with a `gate` step.
|
||||
- **Avoid destructive commands** — don't delete files or directories without explicit confirmation via a gate
|
||||
- **Quote variables** — use proper quoting in shell commands to handle spaces
|
||||
- **Check exit codes** — shell step failures stop the workflow; make sure commands are robust
|
||||
|
||||
Reference in New Issue
Block a user