From 085e005d92182cca39bcfb7b3136578f01547ed8 Mon Sep 17 00:00:00 2001
From: Murali Chillakuru <murali.chillakuru@microsoft.com>
Date: Sat, 25 Jul 2026 18:41:23 -0400
Subject: [PATCH] security: remove shell=True from ReAct agent bash tool

Parse the command with shlex.split and run with shell=False, and restrict the executable to a python/python3 allow-list. This removes shell metacharacter injection (the benchmark only needs Python to manipulate spreadsheets). Adds tests for the allow-list gate and non-interpretation of shell metacharacters.
---
 skillopt/envs/spreadsheetbench/react_agent.py | 20 ++++++++++--
 tests/test_react_agent_no_shell.py            | 32 +++++++++++++++++++
 2 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100644 tests/test_react_agent_no_shell.py

diff --git a/skillopt/envs/spreadsheetbench/react_agent.py b/skillopt/envs/spreadsheetbench/react_agent.py
index 2e729534..f9646ac5 100644
--- a/skillopt/envs/spreadsheetbench/react_agent.py
+++ b/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,
diff --git a/tests/test_react_agent_no_shell.py b/tests/test_react_agent_no_shell.py
new file mode 100644
index 00000000..0bb181d1
--- /dev/null
+++ b/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()