security: remove shell=True from ReAct agent bash tool

skillopt/envs/spreadsheetbench/react_agent.py

@@ -9,6 +9,7 @@
 
 import json
 import os
+import shlex
 import subprocess
 
 from skillopt.model import chat_target_messages
@@ -248,13 +249,28 @@ def _auto_verify(work_dir: str) -> str:
         return f"\n\n[AUTO-VERIFY] Could not inspect output: {e}"
 
 
+# Commands that the ReAct agent is allowed to run (benchmark only needs Python).
+_CMD_ALLOW = {"python", "python3"}
+
+
 # ── Bash execution ────────────────────────────────────────────────────────────
 
 def _run_bash(cmd: str, work_dir: str, timeout: int = 60) -> str:
     try:
+        parts = shlex.split(cmd)
+        if not parts:
+            return "[error: empty command]"
+        exe_name = os.path.basename(parts[0]).lower()
+        # Strip .exe suffix on Windows (python.exe → python)
+        exe_stem = exe_name.split(".")[0]
+        if exe_stem not in _CMD_ALLOW:
+            return (
+                f"[blocked: '{parts[0]}' not in allow-list {sorted(_CMD_ALLOW)}; "
+                "use Python to manipulate spreadsheets]"
+            )
         proc = subprocess.run(
-            cmd,
-            shell=True,
+            parts,
+            shell=False,
             capture_output=True,
             text=True,
             timeout=timeout,

tests/test_react_agent_no_shell.py

@@ -0,0 +1,32 @@
+"""Tests for shell-injection hardening in the ReAct agent's bash tool.
+
+``_run_bash`` previously ran commands with ``shell=True``, allowing arbitrary
+shell metacharacter injection. It now uses ``shlex.split`` + ``shell=False``
+and restricts the executable to a small allow-list (python/python3). These
+tests assert both the allow-list gate and that shell metacharacters are no
+longer interpreted.
+"""
+from __future__ import annotations
+
+from skillopt.envs.spreadsheetbench.react_agent import _run_bash
+
+
+def test_disallowed_command_is_blocked(tmp_path) -> None:
+    out = _run_bash("curl http://example.com/evil", str(tmp_path))
+    assert "blocked" in out.lower()
+
+
+def test_allowed_python_runs(tmp_path) -> None:
+    # Bare 'python' resolves via PATH; a full Windows path would be mangled by
+    # shlex.split (posix mode), which is expected agent-input behaviour here.
+    out = _run_bash('python -c "print(42)"', str(tmp_path))
+    assert "42" in out
+
+
+def test_shell_metacharacters_not_interpreted(tmp_path) -> None:
+    # With shell=False the ';' and following tokens become arguments to python,
+    # not a second shell command, so the marker file must NOT be created.
+    marker = tmp_path / "pwned.txt"
+    cmd = "python -c \"print(1)\" ; python -c \"open('pwned.txt','w')\""
+    _run_bash(cmd, str(tmp_path))
+    assert not marker.exists()