Skip to content

Commit cbaf2b8

Browse files
committed
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.
1 parent e5a03bf commit cbaf2b8

5 files changed

Lines changed: 58 additions & 5 deletions

File tree

pyproject.toml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,3 +74,13 @@ precision = 2
7474
show_missing = true
7575
skip_covered = false
7676

77+
[tool.ruff.lint]
78+
# Lock in subprocess security posture: any reintroduction of shell=True
79+
# (or os.system / popen2) must be acknowledged with an explicit `# noqa`
80+
# pointing at the rule, making the deviation visible in review.
81+
extend-select = [
82+
"S602", # subprocess-popen-with-shell-equals-true
83+
"S604", # call-with-shell-equals-true
84+
"S605", # start-process-with-a-shell
85+
]
86+

src/specify_cli/_utils.py

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,14 +65,31 @@ def dump_frontmatter(data: dict[str, Any]) -> str:
6565
return yaml.safe_dump(data, sort_keys=False, allow_unicode=True).strip()
6666

6767

68-
def run_command(cmd: list[str], check_return: bool = True, capture: bool = False, shell: bool = False) -> str | None:
69-
"""Run a shell command and optionally capture output."""
68+
def run_command(
69+
cmd: list[str],
70+
check_return: bool = True,
71+
capture: bool = False,
72+
shell: bool = False,
73+
) -> str | None:
74+
"""Run a command without invoking a shell and optionally capture output.
75+
76+
The ``shell`` parameter is kept in the signature so existing keyword
77+
callers (and the re-export from ``specify_cli``) don't raise ``TypeError``,
78+
but only the default ``shell=False`` is honoured. ``shell=True`` is
79+
rejected with ``ValueError`` rather than silently ignored, so the
80+
unsupported mode fails loudly instead of running with a different meaning.
81+
"""
82+
if shell:
83+
raise ValueError(
84+
"run_command() does not support shell=True; pass argv as a list"
85+
)
86+
7087
try:
7188
if capture:
72-
result = subprocess.run(cmd, check=check_return, capture_output=True, text=True, shell=shell)
89+
result = subprocess.run(cmd, check=check_return, capture_output=True, text=True)
7390
return result.stdout.strip()
7491
else:
75-
subprocess.run(cmd, check=check_return, shell=shell)
92+
subprocess.run(cmd, check=check_return)
7693
return None
7794
except subprocess.CalledProcessError as e:
7895
if check_return:

src/specify_cli/workflows/steps/shell/__init__.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult:
3131
# control commands; catalog-installed workflows should be reviewed
3232
# before use (see PUBLISHING.md for security guidance).
3333
try:
34-
proc = subprocess.run(
34+
proc = subprocess.run( # noqa: S602 -- intentional shell=True (see NOTE above)
3535
run_cmd,
3636
shell=True,
3737
capture_output=True,

tests/test_utils.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
"""Tests for specify_cli._utils.run_command."""
2+
3+
from __future__ import annotations
4+
5+
import inspect
6+
7+
import pytest
8+
9+
from specify_cli import run_command
10+
11+
12+
def test_run_command_rejects_shell_execution_compatibly():
13+
assert inspect.signature(run_command).parameters["shell"].default is False
14+
with pytest.raises(ValueError, match="does not support shell=True"):
15+
run_command(["echo", "blocked"], shell=True) # noqa: S604

workflows/PUBLISHING.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,17 @@ When releasing a new version:
272272
- **Quote variables** — use proper quoting in shell commands to handle spaces
273273
- **Check exit codes** — shell step failures stop the workflow; make sure commands are robust
274274

275+
#### Security: shell steps execute arbitrary code
276+
277+
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.
278+
279+
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`:
280+
281+
- **Before installing a workflow**, fetch the raw YAML and audit every `shell` step's `run` field directly. `specify workflow info <name>` only shows metadata (name, version, inputs, step IDs/types) — not the shell content that would actually execute.
282+
- **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.
283+
- **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.
284+
- **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.
285+
275286
### Integration Flexibility
276287

277288
- **Set `integration` at workflow level** — use the `workflow.integration` field as the default

0 commit comments

Comments
 (0)