Merge pull request 'Prevent issue content from executing shell commands in the release worker' (#308) from timmy/307-prevent-issue-content-from-executing-shell-comma into main

README.md

@@ -262,6 +262,14 @@ claim, reruns the test command, pushes the branch, and opens a PR containing
 `Closes #19` plus test evidence. Replace `19` with the selected issue number;
 do not run without `--issue` when processing a preselected ticket.
 
+Agent and test commands use POSIX argument quoting and run as argument vectors,
+not through a shell. Issue context is available to the agent only through
+`RELEASE_ISSUE_NUMBER`, `RELEASE_ISSUE_TITLE`, `RELEASE_ISSUE_BODY`,
+`RELEASE_REPO`, and `RELEASE_BRANCH`; do not put issue placeholders in command
+text. Shell substitutions, redirects, and pipelines are intentionally not
+interpreted. Put any trusted shell workflow in a reviewed wrapper script and
+configure that script as `RELEASE_AGENT_COMMAND` instead.
+
 Durable state defaults to `.release-engine/state.json`. Full behavior and
 safety gates are documented in
 [`docs/release-engine-spec.md`](docs/release-engine-spec.md).

docs/release-engine-spec.md

@@ -12,8 +12,8 @@ Convert one eligible Gitea issue into a tested, traceable pull request without d
 - Repository key (`owner/repo`).
 - Agent username.
 - Optional explicit issue number; otherwise deterministic queue selection.
-- Coding-agent command template.
-- Test command.
+- Coding-agent command, parsed into an argument vector.
+- Test command, parsed into an argument vector.
 - Local repository path and durable state-file path.
 
 ## State machine
@@ -39,7 +39,10 @@ Terminal failure states are `claim_failed`, `agent_failed`, `tests_failed`, and
 ## Execution
 
 - Branch format: `<agent>/<issue>-<slug>`.
-- Coding command receives issue number, title, body, repo, and branch through template fields and environment variables.
+- Coding and test commands are parsed with POSIX argument quoting and executed directly without a shell.
+- Issue number, title, body, repo, and branch are supplied to the coding agent only through `RELEASE_ISSUE_NUMBER`, `RELEASE_ISSUE_TITLE`, `RELEASE_ISSUE_BODY`, `RELEASE_REPO`, and `RELEASE_BRANCH` environment variables. Gitea-controlled content is never interpolated into executable syntax.
+- Shell operators, substitutions, and pipelines are not interpreted. Operators that are intentionally required must live in a separately reviewed wrapper script configured as the command.
+- Empty or malformed commands abort before the issue claim.
 - Non-zero coding-agent exit blocks tests and PR creation.
 - Tests run using the configured command; stdout/stderr and exit code become evidence.
 

src/release_engine.py

@@ -63,7 +63,7 @@ class Client(Protocol):
 
 
 class Runner(Protocol):
-    def run(self, command: str, cwd: Path, env: dict[str, str] | None = None) -> CommandResult: ...
+    def run(self, command: list[str], cwd: Path, env: dict[str, str] | None = None) -> CommandResult: ...
 
 
 class StateStore:
@@ -113,8 +113,18 @@ def branch_name(agent: str, issue: Issue) -> str:
     return f"{actor}/{issue.number}-{slug or 'issue'}"
 
 
+def parse_command(command: str, name: str) -> list[str]:
+    try:
+        arguments = shlex.split(command)
+    except ValueError as exc:
+        raise ValueError(f"invalid {name}: {exc}") from exc
+    if not arguments:
+        raise ValueError(f"invalid {name}: command is empty")
+    return arguments
+
+
 class ShellRunner:
-    def run(self, command: str, cwd: Path, env: dict[str, str] | None = None) -> CommandResult:
+    def run(self, command: list[str], cwd: Path, env: dict[str, str] | None = None) -> CommandResult:
         merged_env = os.environ.copy()
         if env:
             merged_env.update(env)
@@ -122,7 +132,7 @@ class ShellRunner:
             command,
             cwd=str(cwd),
             env=merged_env,
-            shell=True,
+            shell=False,
             text=True,
             capture_output=True,
         )
@@ -220,6 +230,9 @@ class ReleaseEngine:
                 "agent": self.config.agent,
             }
 
+        agent_command = parse_command(self.config.agent_command, "agent command")
+        test_command = parse_command(self.config.test_command, "test command")
+
         self.client.claim_issue(self.config.repo, issue.number, self.config.agent)
         verified = self.client.get_issue(self.config.repo, issue.number)
         if verified.assignee != self.config.agent:
@@ -228,19 +241,11 @@ class ReleaseEngine:
             )
         self.store.save(RunState(issue.number, branch, "claimed"))
 
-        branch_result = self.runner.run(f"git checkout -B {shlex.quote(branch)}", self.config.repo_path)
+        branch_result = self.runner.run(["git", "checkout", "-B", branch], self.config.repo_path)
         if branch_result.returncode != 0:
             raise RuntimeError(f"branch creation failed: {branch_result.stderr.strip()}")
 
-        context = {
-            "issue_number": issue.number,
-            "issue_title": issue.title,
-            "issue_body": issue.body,
-            "repo": self.config.repo,
-            "branch": branch,
-        }
-        command = self.config.agent_command.format(**context)
-        agent_result = self.runner.run(command, self.config.repo_path, {
+        agent_result = self.runner.run(agent_command, self.config.repo_path, {
             "RELEASE_ISSUE_NUMBER": str(issue.number),
             "RELEASE_ISSUE_TITLE": issue.title,
             "RELEASE_ISSUE_BODY": issue.body,
@@ -252,14 +257,14 @@ class ReleaseEngine:
             raise RuntimeError(f"coding agent failed: {agent_result.stderr.strip()}")
         self.store.save(RunState(issue.number, branch, "agent_complete"))
 
-        test_result = self.runner.run(self.config.test_command, self.config.repo_path)
+        test_result = self.runner.run(test_command, self.config.repo_path)
         evidence = (test_result.stdout + "\n" + test_result.stderr).strip()[-4000:]
         if test_result.returncode != 0:
             self.store.save(RunState(issue.number, branch, "tests_failed", evidence))
             raise RuntimeError(f"tests failed: {evidence}")
         self.store.save(RunState(issue.number, branch, "tests_passed", evidence))
 
-        push_result = self.runner.run(f"git push -u origin {shlex.quote(branch)}", self.config.repo_path)
+        push_result = self.runner.run(["git", "push", "-u", "origin", branch], self.config.repo_path)
         if push_result.returncode != 0:
             self.store.save(RunState(issue.number, branch, "push_failed", push_result.stderr[-2000:]))
             raise RuntimeError(f"push failed: {push_result.stderr.strip()}")

tests/test_release_engine.py

@@ -1,4 +1,5 @@
 import json
+import sys
 import threading
 from http.server import BaseHTTPRequestHandler, HTTPServer
 from pathlib import Path
@@ -12,12 +13,34 @@ from src.release_engine import (
     Issue,
     ReleaseEngine,
     RunState,
+    ShellRunner,
     StateStore,
     branch_name,
     select_issue,
 )
 
 
+def test_shell_runner_treats_metacharacters_as_literal_arguments(tmp_path):
+    output = tmp_path / "argument.txt"
+    injected = tmp_path / "injected.txt"
+    payload = f"release title; touch {injected}"
+
+    result = ShellRunner().run(
+        [
+            sys.executable,
+            "-c",
+            "from pathlib import Path; import sys; Path(sys.argv[1]).write_text(sys.argv[2])",
+            str(output),
+            payload,
+        ],
+        tmp_path,
+    )
+
+    assert result.returncode == 0
+    assert output.read_text() == payload
+    assert not injected.exists()
+
+
 def test_stale_token_error_explains_how_to_restore_cron_authentication():
     class UnauthorizedHandler(BaseHTTPRequestHandler):
         def do_GET(self):
@@ -70,9 +93,11 @@ class FakeRunner:
     def __init__(self, results):
         self.results = list(results)
         self.commands = []
+        self.environments = []
 
     def run(self, command, cwd, env=None):
         self.commands.append(command)
+        self.environments.append(env)
         return self.results.pop(0)
 
 
@@ -91,7 +116,7 @@ def config(tmp_path):
         agent="timmy",
         repo_path=tmp_path,
         state_path=tmp_path / "state.json",
-        agent_command="agent --issue {issue_number}",
+        agent_command="agent --issue-from-env",
         test_command="pytest -q",
     )
 
@@ -121,6 +146,24 @@ def test_claim_verification_blocks_execution(tmp_path):
     assert runner.commands == []
 
 
+def test_empty_agent_command_fails_before_claim(tmp_path):
+    issue = Issue(14, "Engine", "body", "open", [], None)
+    client = FakeClient([issue])
+    cfg = EngineConfig(
+        repo="stackchain/stackchain-dashboard",
+        agent="timmy",
+        repo_path=tmp_path,
+        state_path=tmp_path / "state.json",
+        agent_command="  ",
+        test_command="pytest -q",
+    )
+
+    with pytest.raises(ValueError, match="invalid agent command: command is empty"):
+        ReleaseEngine(cfg, client, FakeRunner([])).execute(issue_number=14)
+
+    assert client.claimed == []
+
+
 def test_agent_failure_blocks_tests_and_pr(tmp_path):
     issue = Issue(14, "Engine", "body", "open", [], None)
     client = FakeClient([issue])
@@ -134,12 +177,41 @@ def test_agent_failure_blocks_tests_and_pr(tmp_path):
         engine.execute(issue_number=14)
 
     assert runner.commands == [
-        "git checkout -B timmy/14-engine",
-        "agent --issue 14",
+        ["git", "checkout", "-B", "timmy/14-engine"],
+        ["agent", "--issue-from-env"],
     ]
     assert client.created_prs == []
 
 
+def test_issue_content_reaches_agent_only_through_environment(tmp_path):
+    title = "$(touch /tmp/title-injected); `id`"
+    body = "line one\n&& touch /tmp/body-injected > stolen"
+    issue = Issue(14, title, body, "open", [], None)
+    client = FakeClient([issue])
+    runner = FakeRunner([
+        CommandResult(0, "", ""),
+        CommandResult(1, "", "agent stopped"),
+    ])
+    cfg = EngineConfig(
+        repo="stackchain/stackchain-dashboard",
+        agent="timmy",
+        repo_path=tmp_path,
+        state_path=tmp_path / "state.json",
+        agent_command="agent --work",
+        test_command="pytest -q",
+    )
+
+    with pytest.raises(RuntimeError, match="coding agent failed"):
+        ReleaseEngine(cfg, client, runner).execute(issue_number=14)
+
+    assert runner.commands == [
+        ["git", "checkout", "-B", "timmy/14-touch-tmp-title-injected-id"],
+        ["agent", "--work"],
+    ]
+    assert runner.environments[1]["RELEASE_ISSUE_TITLE"] == title
+    assert runner.environments[1]["RELEASE_ISSUE_BODY"] == body
+
+
 def test_failed_tests_block_push_and_pr(tmp_path):
     issue = Issue(14, "Engine", "body", "open", [], None)
     client = FakeClient([issue])
@@ -153,7 +225,7 @@ def test_failed_tests_block_push_and_pr(tmp_path):
     with pytest.raises(RuntimeError, match="tests failed"):
         engine.execute(issue_number=14)
 
-    assert "git push" not in " ".join(runner.commands)
+    assert not any(command[:2] == ["git", "push"] for command in runner.commands)
     assert client.created_prs == []
 
 
@@ -171,7 +243,7 @@ def test_passing_run_pushes_and_opens_linked_pr(tmp_path):
     result = engine.execute(issue_number=14)
 
     assert result["pr_number"] == 44
-    assert runner.commands[-1].startswith("git push -u origin")
+    assert runner.commands[-1] == ["git", "push", "-u", "origin", "timmy/14-engine-mvp"]
     assert len(client.created_prs) == 1
     body = client.created_prs[0][4]
     assert "Closes #14" in body