From b9fa03b11892d8b3b4b5684238ecb4eb2dc52c8a Mon Sep 17 00:00:00 2001 From: "Zied Jlassi (Architect AI)" <6190550+zied-jlassi@users.noreply.github.com> Date: Sun, 21 Jun 2026 22:58:22 +0200 Subject: [PATCH 1/3] fix(workflows): validate requires keys and reject phantom permissions gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/reference/workflows.md | 2 + src/specify_cli/workflows/engine.py | 45 ++++++++++++++++- tests/test_workflows.py | 77 +++++++++++++++++++++++++++++ workflows/PUBLISHING.md | 1 + 4 files changed, 123 insertions(+), 2 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 5f6e90d924..b90a139847 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` (e.g. `requires.permissions`) is an advisory pre-condition block, not a runtime gate, so it does **not** restrict what a step can do. 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..6250f16a8e 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`` (recognised keys only; + # see ``_RECOGNISED_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. These mirror the +# pre-conditions documented in workflows/PUBLISHING.md and resolved by the +# bundler (``speckit_version``, ``integrations``, ``tools``, ``mcp``). Any other +# key — notably ``permissions`` — is rejected by ``validate_workflow`` so it is +# never mistaken for an enforced runtime control. +_RECOGNISED_REQUIRES_KEYS = frozenset( + {"speckit_version", "integrations", "tools", "mcp"} +) + # 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,34 @@ 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 recognised; + # 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. + if definition.requires: + 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 recognised 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 _RECOGNISED_REQUIRES_KEYS: + errors.append( + f"Unknown 'requires' key {key!r}. Recognised keys: " + f"{', '.join(sorted(_RECOGNISED_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..f9ea02ec83 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2048,6 +2048,83 @@ 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 any("permissions" in e and "not" in e.lower() for e in errors) + # ===== Workflow Engine Tests ===== diff --git a/workflows/PUBLISHING.md b/workflows/PUBLISHING.md index ce0d251826..7ba0202b97 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`, `tools`, `mcp`), **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 From 0f8dbdbe04331fa8e229d93f666d361eee176bc5 Mon Sep 17 00:00:00 2001 From: "Zied Jlassi (Architect AI)" <6190550+zied-jlassi@users.noreply.github.com> Date: Mon, 22 Jun 2026 18:55:37 +0200 Subject: [PATCH 2/3] 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> --- docs/reference/workflows.md | 2 +- src/specify_cli/workflows/engine.py | 23 ++++++++++++++--------- tests/test_workflows.py | 26 +++++++++++++++++++++++++- 3 files changed, 40 insertions(+), 11 deletions(-) diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index b90a139847..ffa25301e1 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -270,7 +270,7 @@ 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` (e.g. `requires.permissions`) is an advisory pre-condition block, not a runtime gate, so it does **not** restrict what a step can do. Review any catalog or downloaded workflow before running it, and use a `gate` step to require explicit approval before sensitive or destructive shell commands. +> **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 diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index 6250f16a8e..c4d0129825 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -53,8 +53,8 @@ def __init__(self, data: dict[str, Any], source_path: Path | None = None) -> Non self.default_options = {} # Advisory pre-conditions (spec-kit version / integrations a workflow - # expects). Validated by ``validate_workflow`` (recognised keys only; - # see ``_RECOGNISED_REQUIRES_KEYS``) but NOT enforced at run time — they + # 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. @@ -96,7 +96,7 @@ def from_string(cls, content: str) -> WorkflowDefinition: # bundler (``speckit_version``, ``integrations``, ``tools``, ``mcp``). Any other # key — notably ``permissions`` — is rejected by ``validate_workflow`` so it is # never mistaken for an enforced runtime control. -_RECOGNISED_REQUIRES_KEYS = frozenset( +_RECOGNIZED_REQUIRES_KEYS = frozenset( {"speckit_version", "integrations", "tools", "mcp"} ) @@ -192,13 +192,18 @@ def validate_workflow(definition: WorkflowDefinition) -> list[str]: # -- Requires --------------------------------------------------------- # ``requires`` declares advisory pre-conditions (the spec-kit version and - # integrations a workflow expects). Only a fixed set of keys is recognised; + # 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. - if definition.requires: + # + # 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)." @@ -207,15 +212,15 @@ def validate_workflow(definition: WorkflowDefinition) -> list[str]: for key in definition.requires: if key == "permissions": errors.append( - "'requires.permissions' is not a recognised or " + "'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 _RECOGNISED_REQUIRES_KEYS: + elif key not in _RECOGNIZED_REQUIRES_KEYS: errors.append( - f"Unknown 'requires' key {key!r}. Recognised keys: " - f"{', '.join(sorted(_RECOGNISED_REQUIRES_KEYS))}." + f"Unknown 'requires' key {key!r}. Recognized keys: " + f"{', '.join(sorted(_RECOGNIZED_REQUIRES_KEYS))}." ) # -- Steps ------------------------------------------------------------ diff --git a/tests/test_workflows.py b/tests/test_workflows.py index f9ea02ec83..eeec44f65b 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -2123,7 +2123,31 @@ def test_requires_permissions_is_rejected_as_not_enforced(self): run: "echo hi" """) errors = validate_workflow(definition) - assert any("permissions" in e and "not" in e.lower() for e in errors) + # 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 ===== From b0bccbdf8d069eba90405b12c9a1ef43cb0ce8b4 Mon Sep 17 00:00:00 2001 From: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> Date: Tue, 23 Jun 2026 04:51:24 +0000 Subject: [PATCH 3/3] 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> --- src/specify_cli/workflows/engine.py | 16 ++++++++-------- workflows/PUBLISHING.md | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/specify_cli/workflows/engine.py b/src/specify_cli/workflows/engine.py index c4d0129825..abb78196e8 100644 --- a/src/specify_cli/workflows/engine.py +++ b/src/specify_cli/workflows/engine.py @@ -91,14 +91,14 @@ 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. These mirror the -# pre-conditions documented in workflows/PUBLISHING.md and resolved by the -# bundler (``speckit_version``, ``integrations``, ``tools``, ``mcp``). 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", "tools", "mcp"} -) +# 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]: diff --git a/workflows/PUBLISHING.md b/workflows/PUBLISHING.md index 7ba0202b97..ae3fc91308 100644 --- a/workflows/PUBLISHING.md +++ b/workflows/PUBLISHING.md @@ -268,7 +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`, `tools`, `mcp`), **not** a runtime permission gate — there is no `requires.permissions`. Gate sensitive commands explicitly with a `gate` step. +- **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