diff --git a/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/shell/ShellTool.kt b/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/shell/ShellTool.kt index b01d0d1d..9fb40f08 100644 --- a/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/shell/ShellTool.kt +++ b/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 -> - ValidationResult.Invalid("Executable '${parsed.argv[0]}' is not in the allowed list.") + // Check EVERY command position, not just argv[0]. An operator form runs via `sh -c` + // (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): 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): List = 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): 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") } diff --git a/infrastructure/tools/src/test/kotlin/com/correx/infrastructure/tools/shell/ShellToolTest.kt b/infrastructure/tools/src/test/kotlin/com/correx/infrastructure/tools/shell/ShellToolTest.kt index 14dd79ed..a34d658f 100644 --- a/infrastructure/tools/src/test/kotlin/com/correx/infrastructure/tools/shell/ShellToolTest.kt +++ b/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"))