diff --git a/plan/src/main/java/org/apache/flink/agents/plan/tools/bash/BashValidator.java b/plan/src/main/java/org/apache/flink/agents/plan/tools/bash/BashValidator.java index 2d938ecfb..4ca6401bc 100644 --- a/plan/src/main/java/org/apache/flink/agents/plan/tools/bash/BashValidator.java +++ b/plan/src/main/java/org/apache/flink/agents/plan/tools/bash/BashValidator.java @@ -56,8 +56,6 @@ public final class BashValidator { "program", "command", "command_name", - // `export VAR=...`, `readonly`, `declare`, `local`, `typeset` - "declaration_command", "pipeline", "list", "redirected_statement", @@ -76,7 +74,6 @@ public final class BashValidator { "number", "simple_expansion", // $VAR "expansion", // ${VAR} - "arithmetic_expansion", // $((...)) "binary_expression", "unary_expression", "parenthesized_expression", @@ -143,6 +140,12 @@ private static Optional walk( return Optional.of( "Disallowed shell construct '" + node.getType() + "' in: '" + snippet + "'"); } + TSNode parent = node.getParent(); + if ("variable_assignment".equals(node.getType()) + && (parent == null || parent.isNull() || !"command".equals(parent.getType()))) { + return Optional.of( + "Standalone variable assignment without an executable is not allowed."); + } if ("command".equals(node.getType())) { Optional err = validateCommand(node, command, allowedCommands, allowedScriptDirs, cwd); @@ -168,8 +171,9 @@ private static Optional validateCommand( @Nullable String cwd) { TSNode nameNode = commandNode.getChildByFieldName("name"); if (nameNode == null || nameNode.isNull()) { - // Bare variable-assignment parsed as command — nothing to validate. - return Optional.empty(); + // Fail closed for constructs such as bare variable assignments. Bash can later + // reinterpret their values in arithmetic contexts. + return Optional.of("Command without an executable is not allowed."); } String executable = nodeText(nameNode, command); if (allowedCommands.contains(executable)) { diff --git a/plan/src/test/java/org/apache/flink/agents/plan/tools/bash/BashToolTest.java b/plan/src/test/java/org/apache/flink/agents/plan/tools/bash/BashToolTest.java index 3a134923e..76698e6ec 100644 --- a/plan/src/test/java/org/apache/flink/agents/plan/tools/bash/BashToolTest.java +++ b/plan/src/test/java/org/apache/flink/agents/plan/tools/bash/BashToolTest.java @@ -23,12 +23,15 @@ import org.apache.flink.agents.api.tools.ToolParameters; import org.apache.flink.agents.api.tools.ToolResponse; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import java.nio.file.Path; import java.util.HashMap; import java.util.List; import java.util.Map; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; class BashToolTest { @@ -70,6 +73,34 @@ void controlFlowRejected() { assertTrue(out.startsWith("Command rejected:")); } + @Test + void integerDeclarationCannotReevaluateCommandSubstitution(@TempDir Path tempDir) { + Path marker = tempDir.resolve("integer-declaration-marker"); + ToolResponse r = + tool().call( + args( + "declare -i VALUE='$(touch " + marker + ")'", + List.of(), + List.of())); + String out = (String) r.getResult(); + assertTrue(out.startsWith("Command rejected:")); + assertFalse(marker.toFile().exists()); + } + + @Test + void arithmeticExpansionCannotReevaluateAssignedValue(@TempDir Path tempDir) { + Path marker = tempDir.resolve("arithmetic-expansion-marker"); + ToolResponse r = + tool().call( + args( + "VALUE='$(touch " + marker + ")'; echo $((VALUE))", + List.of("echo"), + List.of())); + String out = (String) r.getResult(); + assertTrue(out.startsWith("Command rejected:")); + assertFalse(marker.toFile().exists()); + } + @Test void successfulCommandWithEmptyOutput() { ToolResponse r = tool().call(args("true", List.of("true"), List.of())); diff --git a/plan/src/test/java/org/apache/flink/agents/plan/tools/bash/BashValidatorTest.java b/plan/src/test/java/org/apache/flink/agents/plan/tools/bash/BashValidatorTest.java index 7ff28f071..6dc967df0 100644 --- a/plan/src/test/java/org/apache/flink/agents/plan/tools/bash/BashValidatorTest.java +++ b/plan/src/test/java/org/apache/flink/agents/plan/tools/bash/BashValidatorTest.java @@ -76,10 +76,35 @@ void variableExpansionAllowed() { } @Test - void arithmeticExpansionAllowed() { + void arithmeticExpansionRejected() { + Optional r = + BashValidator.validate("echo $((1+2))", List.of("echo"), List.of(), null); + assertTrue(r.isPresent()); + assertTrue(r.get().contains("arithmetic_expansion")); + } + + @Test + void declarationCommandRejected() { + Optional r = + BashValidator.validate( + "declare -i VALUE='1 + 2'", List.of("echo"), List.of(), null); + assertTrue(r.isPresent()); + assertTrue(r.get().contains("declaration_command")); + } + + @Test + void standaloneVariableAssignmentRejected() { + Optional r = + BashValidator.validate("VALUE='1 + 2'", List.of("echo"), List.of(), null); + assertTrue(r.isPresent()); + assertTrue(r.get().contains("executable")); + } + + @Test + void variableAssignmentPrefixWithAllowedCommandPasses() { assertEquals( Optional.empty(), - BashValidator.validate("echo $((1+2))", List.of("echo"), List.of(), null)); + BashValidator.validate("VALUE=abc echo hi", List.of("echo"), List.of(), null)); } @Test diff --git a/python/flink_agents/plan/tools/bash/bash_validator.py b/python/flink_agents/plan/tools/bash/bash_validator.py index d5b9c20f8..c87dea757 100644 --- a/python/flink_agents/plan/tools/bash/bash_validator.py +++ b/python/flink_agents/plan/tools/bash/bash_validator.py @@ -47,8 +47,6 @@ "program", "command", "command_name", - # `export VAR=...`, `readonly`, `declare`, `local`, `typeset` - "declaration_command", "pipeline", "list", "redirected_statement", @@ -67,7 +65,6 @@ "number", "simple_expansion", # $VAR "expansion", # ${VAR} - "arithmetic_expansion", # $((...)) "binary_expression", "unary_expression", "parenthesized_expression", @@ -121,6 +118,10 @@ def _walk( if node.is_named and node.type not in _ALLOWED_NAMED: snippet = node.text.decode("utf-8", errors="replace")[:80] return f"Disallowed shell construct '{node.type}' in: {snippet!r}" + if node.type == "variable_assignment" and ( + node.parent is None or node.parent.type != "command" + ): + return "Standalone variable assignment without an executable is not allowed." if node.type == "command": err = _validate_command_node(node, allowed_commands, allowed_script_dirs, cwd) if err is not None: @@ -140,9 +141,9 @@ def _validate_command_node( ) -> str | None: name_node = node.child_by_field_name("name") if name_node is None: - # Commands without a resolvable name (edge case, e.g. bare - # variable-assignment parsed as `command`) — nothing to validate. - return None + # Fail closed for constructs such as bare variable assignments. Bash + # can later reinterpret their values in arithmetic contexts. + return "Command without an executable is not allowed." executable = name_node.text.decode("utf-8", errors="replace") if executable in allowed_commands: return None diff --git a/python/flink_agents/plan/tools/bash/tests/test_bash_tool.py b/python/flink_agents/plan/tools/bash/tests/test_bash_tool.py index e05936a0d..3f6048fec 100644 --- a/python/flink_agents/plan/tools/bash/tests/test_bash_tool.py +++ b/python/flink_agents/plan/tools/bash/tests/test_bash_tool.py @@ -116,6 +116,11 @@ def test_allow_env_prefix(self) -> None: def test_reject_env_prefix_command_not_whitelisted(self) -> None: assert validate_command("FOO=bar rm -rf /", ["echo"], []) is not None + def test_reject_standalone_variable_assignment(self) -> None: + error = validate_command("VALUE='1 + 2'", ["echo"], []) + assert error is not None + assert "executable" in error + # -- injection vectors: MUST be rejected ------------------------------ def test_reject_dollar_paren_substitution(self) -> None: @@ -165,6 +170,16 @@ def test_reject_function_definition(self) -> None: error = validate_command("f() { echo hi; }", ["echo"], []) assert error is not None + def test_reject_declaration_command(self) -> None: + error = validate_command("declare -i VALUE='1 + 2'", ["echo"], []) + assert error is not None + assert "declaration_command" in error + + def test_reject_arithmetic_expansion(self) -> None: + error = validate_command("echo $((1 + 2))", ["echo"], []) + assert error is not None + assert "arithmetic_expansion" in error + def test_reject_heredoc(self) -> None: error = validate_command("cat < None: ) assert "Command rejected" in result + def test_reject_substitution_re_evaluated_by_integer_declaration( + self, tool: BashTool, tmp_path: Path + ) -> None: + marker = tmp_path / "integer-declaration-marker" + result = tool.call( + command=f"declare -i VALUE='$(touch {marker})'", + timeout=10, + allowed_commands=[], + allowed_script_dirs=[], + ) + assert "Command rejected" in result + assert not marker.exists() + + def test_reject_substitution_re_evaluated_by_arithmetic_expansion( + self, tool: BashTool, tmp_path: Path + ) -> None: + marker = tmp_path / "arithmetic-expansion-marker" + result = tool.call( + command=f"VALUE='$(touch {marker})'; echo $((VALUE))", + timeout=10, + allowed_commands=["echo"], + allowed_script_dirs=[], + ) + assert "Command rejected" in result + assert not marker.exists() + def test_no_allowed_commands_rejects_everything(self, tool: BashTool) -> None: result = tool.call(command="echo hello", timeout=10) assert "Command rejected" in result