fixed code rabbit suggested issues

Author: AditM99

nodes/src/nodes/tool_shell/IGlobal.py

@@ -70,7 +70,7 @@ def beginGlobal(self) -> None:
         self.timeout = parse_timeout(cfg)
         self.max_output_bytes = parse_max_output(cfg)
         self.env_vars = parse_env_vars(cfg)
-        self.allow_external_env = bool(cfg.get('allowExternalEnv', True))
+        self.allow_external_env = bool(cfg.get('allowExternalEnv', False))
 
         invalid_pattern_errors: list[str] = []
 

nodes/src/nodes/tool_shell/IInstance.py

@@ -120,9 +120,11 @@ def execute(self, args):
         )
 
     def _validate_command(self, command: str) -> None:
-        """Reject commands that don't match any configured allowlist regex."""
+        """Reject commands that don't fully match any configured allowlist regex."""
+        # Use fullmatch (not search) so that an unanchored pattern like
+        # "git status" cannot be smuggled past via "git status; rm -rf /".
         patterns = self.IGlobal.command_patterns or []
-        if patterns and not any(p.search(command) for p in patterns):
+        if patterns and not any(p.fullmatch(command) for p in patterns):
             raise ValueError('Command is not permitted by the configured allowlist.')
 
     def _resolve_cwd(self, override: object) -> str | None:

nodes/src/nodes/tool_shell/README.md

@@ -54,15 +54,15 @@ Common use cases:
 
 ## Configuration
 
-| Field                         | Description                                                                                  |
-| ----------------------------- | -------------------------------------------------------------------------------------------- |
-| Tool Namespace                | Prefix for the tool name (default: `shell`)                                                  |
-| Default working directory     | Working directory used when the agent does not provide one. Defaults to the host process CWD |
-| Execution timeout (seconds)   | Maximum seconds a command may run (default 30, max 1800)                                     |
-| Max output size (bytes)       | Cap on stdout and stderr each (default 1 MiB). Output beyond this is truncated               |
-| Allow agent-supplied env vars | Whether the agent may add env vars per call. Node-defined vars always take precedence        |
-| Environment variables         | Variables injected into every command                                                        |
-| Command allowlist             | Regex patterns. If non-empty, the command must match at least one pattern to run             |
+| Field                         | Description                                                                                                                                  |
+| ----------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------- |
+| Tool Namespace                | Prefix for the tool name (default: `shell`)                                                                                                  |
+| Default working directory     | Working directory used when the agent does not provide one. Defaults to the host process CWD                                                 |
+| Execution timeout (seconds)   | Maximum seconds a command may run (default 30, max 1800)                                                                                     |
+| Max output size (bytes)       | Cap on stdout and stderr each (default 1 MiB). Output beyond this is truncated                                                               |
+| Allow agent-supplied env vars | Whether the agent may add env vars per call (default off). Node-defined vars always take precedence when on                                  |
+| Environment variables         | Variables injected into every command                                                                                                        |
+| Command allowlist             | Regex patterns. If non-empty, the full command must match at least one pattern (re.fullmatch). Use `.*` for substring matches, e.g. `npm .*` |
 
 ## Security
 

nodes/src/nodes/tool_shell/services.json

@@ -56,8 +56,8 @@
 		"tool_shell.allowExternalEnv": {
 			"type": "boolean",
 			"title": "Allow agent-supplied env vars",
-			"description": "If enabled, the agent may inject additional environment variables per call. Variables defined in this node's config always take precedence.",
-			"default": true,
+			"description": "If enabled, the agent may inject additional environment variables per call. Variables defined in this node's config always take precedence. Defaults off because env vars like LD_PRELOAD, PATH, or NODE_OPTIONS can redirect command execution.",
+			"default": false,
 			"enum": [
 				[true, "Yes"],
 				[false, "No"]
@@ -91,7 +91,7 @@
 		},
 		"tool_shell.commandAllowlist": {
 			"title": "Command allowlist",
-			"description": "Regex patterns for allowed commands. A command must match at least one pattern to run. If empty, any command is allowed.",
+			"description": "Regex patterns for allowed commands. The full command string must match at least one pattern (re.fullmatch) to run; use .* for substring matching (e.g. 'npm .*'). If empty, any command is allowed.",
 			"type": "array",
 			"optional": true,
 			"minItems": 0,

nodes/src/nodes/tool_shell/shell_executor.py

@@ -30,12 +30,43 @@
 from __future__ import annotations
 
 import os
+import signal
 import subprocess
+import sys
 import threading
 
 
 _TRUNCATED_MARKER = '\n...[truncated]'
 _READ_CHUNK_SIZE = 4096
+_IS_WINDOWS = sys.platform == 'win32'
+
+
+def _kill_process_tree(proc: subprocess.Popen) -> None:
+    """Force-kill *proc* and every descendant it spawned through the shell."""
+    if _IS_WINDOWS:
+        # taskkill /T walks the process tree; /F forces termination.
+        # Falls back to proc.kill() if taskkill itself can't run.
+        try:
+            subprocess.run(
+                ['taskkill', '/T', '/F', '/PID', str(proc.pid)],
+                stdout=subprocess.DEVNULL,
+                stderr=subprocess.DEVNULL,
+                timeout=5,
+                check=False,
+            )
+        except (OSError, subprocess.SubprocessError):
+            try:
+                proc.kill()
+            except OSError:
+                pass
+    else:
+        try:
+            os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
+        except (ProcessLookupError, PermissionError, OSError):
+            try:
+                proc.kill()
+            except OSError:
+                pass
 
 
 def execute_command(
@@ -54,15 +85,23 @@ def execute_command(
     are drained and discarded so the child can finish writing without
     blocking on a full OS pipe (preserving its natural exit code).
     """
+    # Spawn the child in its own process group/session so we can later kill
+    # the entire tree on timeout — otherwise shell-spawned grandchildren
+    # outlive proc.kill() and keep our reader threads blocked on their pipes.
+    popen_kwargs: dict = {
+        'shell': True,
+        'cwd': cwd,
+        'env': env,
+        'stdout': subprocess.PIPE,
+        'stderr': subprocess.PIPE,
+    }
+    if _IS_WINDOWS:
+        popen_kwargs['creationflags'] = subprocess.CREATE_NEW_PROCESS_GROUP
+    else:
+        popen_kwargs['start_new_session'] = True
+
     try:
-        proc = subprocess.Popen(
-            command,
-            shell=True,
-            cwd=cwd,
-            env=env,
-            stdout=subprocess.PIPE,
-            stderr=subprocess.PIPE,
-        )
+        proc = subprocess.Popen(command, **popen_kwargs)
     except FileNotFoundError as exc:
         return {
             'stdout': '',
@@ -105,7 +144,7 @@ def _drain(stream, buf: bytearray, key: str) -> None:
         exit_code = proc.wait(timeout=timeout)
     except subprocess.TimeoutExpired:
         timed_out = True
-        proc.kill()
+        _kill_process_tree(proc)
         proc.wait()
         exit_code = -1
 

nodes/test/tool_shell/test_shell_executor.py

@@ -71,15 +71,25 @@ class TestExecuteCommandTimeout:
 
     def test_kills_long_running_command(self):
         """Long-running commands are killed and reported as timed_out."""
+        import time
+
+        # Track wall time so we can verify the tree-kill returns promptly.
+        start = time.monotonic()
         result = execute_command(
             f'{PY} -c "import time; time.sleep(5)"',
             cwd=None,
             env=dict(os.environ),
             timeout=1,
             max_output_bytes=4096,
         )
+        elapsed = time.monotonic() - start
         assert result['timed_out'] is True
         assert result['exit_code'] == -1
+        # Without tree-kill the reader threads stay blocked on the orphan's
+        # pipes until the child's own sleep finishes (~5s). Tree-kill should
+        # bring everything down well under that. Generous bound to absorb
+        # CI jitter while still failing if the regression returns.
+        assert elapsed < 4.0, f'expected tree-kill to return fast; took {elapsed:.2f}s'
 
 
 class TestExecuteCommandWorkingDir: