diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 5f6e90d924..ffa25301e1 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -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: diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 0d56f7df70..abb78196e8 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -52,8 +52,12 @@ def __init__(self, data: dict[str, Any], source_path: Path | None = None) -> Non if not isinstance(self.default_options, dict): self.default_options = {} - # Requirements (declared but not yet enforced at runtime; - # enforcement is a planned enhancement) + # 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", {}) # Inputs @@ -87,6 +91,15 @@ def from_string(cls, content: str) -> 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 +190,39 @@ 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. + # + # 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))}." + ) + # -- Steps ------------------------------------------------------------ if not isinstance(definition.steps, list): errors.append("'steps' must be a list.") diff --git a/tests/test_workflows.py b/tests/test_workflows.py index a87c09cf05..eeec44f65b 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2048,6 +2048,107 @@ def test_invalid_input_type(self): 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 falsy-but-non-mapping ``requires`` (e.g. an empty list) is still an + authoring error: validation must guard on ``is not None`` rather than + truthiness, otherwise ``requires: []`` would silently pass. + """ + 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) + # ===== Workflow Engine Tests ===== diff --git a/workflows/PUBLISHING.md b/workflows/PUBLISHING.md index ce0d251826..ae3fc91308 100644 --- a/workflows/PUBLISHING.md +++ b/workflows/PUBLISHING.md @@ -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