fix(shell): validate every command position against allowlist, not just argv[0] (#61)

An operator argv runs via `sh -c`, so ["ls","&&","rm","-rf","/"] passed an
argv[0]-only allowlist check with {ls} then executed rm. Now every command
position (argv[0] + the token after each &&/||/|/;/& separator) must be
allowlisted; shell builtins are implicit, redirect targets are treated as
filenames. Tests for the bypass, an all-allowed chain, and a redirect target.

infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/shell/ShellTool.kt

@@ -78,8 +78,15 @@ class ShellTool(
                 // is not enough — an auto-approve loop waves these straight through.
                 deniedReason(parsed.argv) != null ->
                     ValidationResult.Invalid(deniedReason(parsed.argv)!!)
-                allowedExecutables.isNotEmpty() && parsed.argv[0] !in allowedExecutables ->
+                // Check EVERY command position, not just argv[0]. An operator form runs via `sh -c`
-                    ValidationResult.Invalid("Executable '${parsed.argv[0]}' is not in the allowed list.")
+                // (below), so `["ls", "&&", "rm", "-rf", "/"]` would slip past an argv[0]-only check
+                // with allowlist {ls} and then execute `rm`. Shell builtins (cd, export…) are allowed
+                // implicitly — they navigate, they don't exec an external program.
+                allowedExecutables.isNotEmpty() ->
+                    commandPositions(parsed.argv)
+                        .firstOrNull { it !in SHELL_BUILTINS && it !in allowedExecutables }
+                        ?.let { ValidationResult.Invalid("Executable '$it' is not in the allowed list.") }
+                        ?: ValidationResult.Valid
                 else -> ValidationResult.Valid
             }
         }
@@ -140,6 +147,19 @@ class ShellTool(
     private fun looksLikeShellCommand(argv: List<String>): Boolean =
         argv[0] in SHELL_BUILTINS || argv.any { it in SHELL_OPERATORS }
 
+    // The tokens that land in a command position once the argv is joined and run via `sh -c`:
+    // argv[0], plus the token right after each command-separator (&&, ||, |, ;, &). Redirect
+    // operators (>, >>, <, 2>…) are followed by a filename, not a command, so they open no new
+    // command position. For a plain (non-operator) argv this is just [argv[0]].
+    private fun commandPositions(argv: List<String>): List<String> = buildList {
+        var expectCommand = true
+        for (tok in argv) when {
+            tok in COMMAND_SEPARATORS -> expectCommand = true
+            tok in SHELL_OPERATORS -> expectCommand = false // redirect target follows, not a command
+            expectCommand -> { add(tok); expectCommand = false }
+        }
+    }
+
     // argv[0] is the program name; embedded quotes/commas/whitespace mean the array collapsed into
     // one string element and there is no real executable to run.
     private fun checkExecutable(argv: List<String>): ArgvParse =
@@ -204,6 +224,9 @@ class ShellTool(
         const val EMPTY_MSG = "Missing or empty 'argv' parameter. Expected a JSON array of strings."
         val SHELL_BUILTINS = setOf("cd", "export", "source", ".", "pushd", "popd", "umask", "ulimit")
         val SHELL_OPERATORS = setOf("&&", "||", "|", ">", ">>", "<", ";", "&", "2>", "2>&1")
+        // Operators that chain a new command (its following token is a command position); the rest
+        // of SHELL_OPERATORS are redirects whose following token is a filename.
+        val COMMAND_SEPARATORS = setOf("&&", "||", "|", ";", "&")
         // Runners that download and execute an arbitrary remote package in one shot.
         val REMOTE_EXEC_RUNNERS = setOf("npx", "bunx", "pnpx")
     }

infrastructure/tools/src/test/kotlin/com/correx/infrastructure/tools/shell/ShellToolTest.kt

@@ -46,6 +46,31 @@ class ShellToolTest {
         assertEquals("Executable 'ls' is not in the allowed list.", (result as ValidationResult.Invalid).reason)
     }
 
+    @Test
+    fun `allowlist rejects a disallowed command hidden after a shell operator`(): Unit = runBlocking {
+        // Bypass (#61): ["echo","&&","rm"] joins to `sh -c "echo && rm"`; an argv[0]-only allowlist
+        // check would wave it through with {echo} and then run rm. Every command position is checked.
+        val tool = ShellTool(allowedExecutables = setOf("echo"))
+        val result = tool.validateRequest(createRequest(listOf("echo", "hi", "&&", "rm", "-rf", "x")))
+        assertTrue(result is ValidationResult.Invalid)
+        assertEquals("Executable 'rm' is not in the allowed list.", (result as ValidationResult.Invalid).reason)
+    }
+
+    @Test
+    fun `allowlist allows a chain where every command is allowed, builtins implicit`(): Unit = runBlocking {
+        // `cd dir && echo x` — cd is a builtin (implicitly allowed), echo is allowlisted, `dir`/`x`
+        // are arguments (not command positions), so the chain is valid.
+        val tool = ShellTool(allowedExecutables = setOf("echo"))
+        assertEquals(ValidationResult.Valid, tool.validateRequest(createRequest(listOf("cd", "dir", "&&", "echo", "x"))))
+    }
+
+    @Test
+    fun `allowlist treats a redirect target as a filename, not a command`(): Unit = runBlocking {
+        // `echo x > rm` — the token after `>` is a redirect FILE named "rm", not a command to run.
+        val tool = ShellTool(allowedExecutables = setOf("echo"))
+        assertEquals(ValidationResult.Valid, tool.validateRequest(createRequest(listOf("echo", "x", ">", "rm"))))
+    }
+
     @Test
     fun `validateRequest returns Invalid for empty argv`(): Unit = runBlocking {
         val tool = ShellTool(allowedExecutables = setOf("echo"))