diff --git a/skillopt/envs/spreadsheetbench/react_agent.py b/skillopt/envs/spreadsheetbench/react_agent.py index f9646ac..2ddbc30 100644 --- a/skillopt/envs/spreadsheetbench/react_agent.py +++ b/skillopt/envs/spreadsheetbench/react_agent.py @@ -11,6 +11,7 @@ import json import os import shlex import subprocess +import sys from skillopt.model import chat_target_messages from skillopt.prompts import load_prompt @@ -249,25 +250,35 @@ 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"} +# Command aliases that the ReAct agent may request. Every accepted alias is +# resolved to this process's interpreter before execution, so behavior does not +# depend on PATH and similarly named executables cannot bypass the allow-list. +_PYTHON_ALIASES = {"python", "python3", "python.exe", "python3.exe"} # ── Bash execution ──────────────────────────────────────────────────────────── def _run_bash(cmd: str, work_dir: str, timeout: int = 60) -> str: try: - parts = shlex.split(cmd) + parts = shlex.split(cmd, posix=os.name != "nt") + if os.name == "nt": + # shlex in non-POSIX mode retains surrounding quotes. + parts = [ + part[1:-1] + if len(part) >= 2 and part[0] == part[-1] and part[0] in {'"', "'"} + else part + for part in parts + ] 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: + exe_name = parts[0].replace("\\", "/").rsplit("/", 1)[-1].lower() + if exe_name not in _PYTHON_ALIASES: return ( - f"[blocked: '{parts[0]}' not in allow-list {sorted(_CMD_ALLOW)}; " + f"[blocked: '{parts[0]}' not in allow-list " + f"{sorted(_PYTHON_ALIASES)}; " "use Python to manipulate spreadsheets]" ) + parts[0] = sys.executable proc = subprocess.run( parts, shell=False, diff --git a/tests/test_react_agent_no_shell.py b/tests/test_react_agent_no_shell.py index 0bb181d..267bed4 100644 --- a/tests/test_react_agent_no_shell.py +++ b/tests/test_react_agent_no_shell.py @@ -16,13 +16,19 @@ def test_disallowed_command_is_blocked(tmp_path) -> None: 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. +def test_allowed_python_runs_without_path_lookup(tmp_path, monkeypatch) -> None: + # Accepted aliases are mapped to the running interpreter, so an absent PATH + # must not make the benchmark depend on a system-level Python command. + monkeypatch.setenv("PATH", "") out = _run_bash('python -c "print(42)"', str(tmp_path)) assert "42" in out +def test_similarly_named_executable_is_blocked(tmp_path) -> None: + out = _run_bash('python.evil -c "print(42)"', str(tmp_path)) + assert "blocked" in out.lower() + + 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.