From cbaf2b86f17b285dab8629447eece83ed2816fdc Mon Sep 17 00:00:00 2001 From: Pascal Date: Tue, 23 Jun 2026 22:56:36 +0200 Subject: [PATCH] harden: reject shell=True in run_command run_command() forwarded shell= straight to subprocess.run, so a caller passing shell=True would invoke a shell. Reject shell=True with ValueError (keeping the parameter for signature compatibility) and drop shell= from both subprocess.run calls. Enable ruff S602/S604/S605 to flag any future shell=True reintroduction, annotate the one intentional workflow shell sink with # noqa: S602, and document the shell-step execution risk in workflows/PUBLISHING.md. --- pyproject.toml | 10 ++++++++ src/specify_cli/_utils.py | 25 ++++++++++++++++--- .../workflows/steps/shell/__init__.py | 2 +- tests/test_utils.py | 15 +++++++++++ workflows/PUBLISHING.md | 11 ++++++++ 5 files changed, 58 insertions(+), 5 deletions(-) create mode 100644 tests/test_utils.py diff --git a/pyproject.toml b/pyproject.toml index 6a5099a9d1..d3af4690db 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -74,3 +74,13 @@ precision = 2 show_missing = true skip_covered = false +[tool.ruff.lint] +# Lock in subprocess security posture: any reintroduction of shell=True +# (or os.system / popen2) must be acknowledged with an explicit `# noqa` +# pointing at the rule, making the deviation visible in review. +extend-select = [ + "S602", # subprocess-popen-with-shell-equals-true + "S604", # call-with-shell-equals-true + "S605", # start-process-with-a-shell +] + diff --git a/src/specify_cli/_utils.py b/src/specify_cli/_utils.py index d921e591d9..df0b8ddec1 100644 --- a/src/specify_cli/_utils.py +++ b/src/specify_cli/_utils.py @@ -65,14 +65,31 @@ def dump_frontmatter(data: dict[str, Any]) -> str: return yaml.safe_dump(data, sort_keys=False, allow_unicode=True).strip() -def run_command(cmd: list[str], check_return: bool = True, capture: bool = False, shell: bool = False) -> str | None: - """Run a shell command and optionally capture output.""" +def run_command( + cmd: list[str], + check_return: bool = True, + capture: bool = False, + shell: bool = False, +) -> str | None: + """Run a command without invoking a shell and optionally capture output. + + The ``shell`` parameter is kept in the signature so existing keyword + callers (and the re-export from ``specify_cli``) don't raise ``TypeError``, + but only the default ``shell=False`` is honoured. ``shell=True`` is + rejected with ``ValueError`` rather than silently ignored, so the + unsupported mode fails loudly instead of running with a different meaning. + """ + if shell: + raise ValueError( + "run_command() does not support shell=True; pass argv as a list" + ) + try: if capture: - result = subprocess.run(cmd, check=check_return, capture_output=True, text=True, shell=shell) + result = subprocess.run(cmd, check=check_return, capture_output=True, text=True) return result.stdout.strip() else: - subprocess.run(cmd, check=check_return, shell=shell) + subprocess.run(cmd, check=check_return) return None except subprocess.CalledProcessError as e: if check_return: diff --git a/src/specify_cli/workflows/steps/shell/__init__.py b/src/specify_cli/workflows/steps/shell/__init__.py index 8c62e4cfa8..2a65fca444 100644 --- a/src/specify_cli/workflows/steps/shell/__init__.py +++ b/src/specify_cli/workflows/steps/shell/__init__.py @@ -31,7 +31,7 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: # control commands; catalog-installed workflows should be reviewed # before use (see PUBLISHING.md for security guidance). try: - proc = subprocess.run( + proc = subprocess.run( # noqa: S602 -- intentional shell=True (see NOTE above) run_cmd, shell=True, capture_output=True, diff --git a/tests/test_utils.py b/tests/test_utils.py new file mode 100644 index 0000000000..869c9ff9cc --- /dev/null +++ b/tests/test_utils.py @@ -0,0 +1,15 @@ +"""Tests for specify_cli._utils.run_command.""" + +from __future__ import annotations + +import inspect + +import pytest + +from specify_cli import run_command + + +def test_run_command_rejects_shell_execution_compatibly(): + assert inspect.signature(run_command).parameters["shell"].default is False + with pytest.raises(ValueError, match="does not support shell=True"): + run_command(["echo", "blocked"], shell=True) # noqa: S604 diff --git a/workflows/PUBLISHING.md b/workflows/PUBLISHING.md index ce0d251826..0370ed09f9 100644 --- a/workflows/PUBLISHING.md +++ b/workflows/PUBLISHING.md @@ -272,6 +272,17 @@ When releasing a new version: - **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 +#### Security: shell steps execute arbitrary code + +Workflow `shell` steps execute their `run` field through `/bin/sh` (POSIX) or the platform shell. There is no sandbox between the step and the user's machine: a malicious or buggy `run` block can read environment variables, modify files outside the project, exfiltrate data, or escalate privileges. + +Catalog-listed workflows are reviewed at submission time (see [Verification Process](#verification-process)), but you should still treat every install as code-execution from an untrusted source until you have read the `workflow.yml`: + +- **Before installing a workflow**, fetch the raw YAML and audit every `shell` step's `run` field directly. `specify workflow info ` only shows metadata (name, version, inputs, step IDs/types) — not the shell content that would actually execute. +- **Prefer explicit commands over interpolation** in `run` blocks: `{{ inputs.something }}` substitutions should be quoted and constrained via `enum` so a malicious input can't inject shell syntax. +- **Limit privilege**: shell steps inherit the user's environment. Workflows that need elevated access (sudo, secrets, GitHub tokens) should call them out explicitly in the README so reviewers can spot the requirement. +- **Authors**: if your workflow has shell steps that look risky out of context (deletions, network calls, credential reads), document the rationale in your README. Maintainers will reject submissions whose shell steps can't be justified at review time. + ### Integration Flexibility - **Set `integration` at workflow level** — use the `workflow.integration` field as the default