Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,6 @@ public final class BashValidator {
"program",
"command",
"command_name",
// `export VAR=...`, `readonly`, `declare`, `local`, `typeset`
"declaration_command",
"pipeline",
"list",
"redirected_statement",
Expand All @@ -76,7 +74,6 @@ public final class BashValidator {
"number",
"simple_expansion", // $VAR
"expansion", // ${VAR}
"arithmetic_expansion", // $((...))
"binary_expression",
"unary_expression",
"parenthesized_expression",
Expand Down Expand Up @@ -143,6 +140,12 @@ private static Optional<String> 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<String> err =
validateCommand(node, command, allowedCommands, allowedScriptDirs, cwd);
Expand All @@ -168,8 +171,9 @@ private static Optional<String> 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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,35 @@ void variableExpansionAllowed() {
}

@Test
void arithmeticExpansionAllowed() {
void arithmeticExpansionRejected() {
Optional<String> 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<String> 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<String> 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
Expand Down
13 changes: 7 additions & 6 deletions python/flink_agents/plan/tools/bash/bash_validator.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@
"program",
"command",
"command_name",
# `export VAR=...`, `readonly`, `declare`, `local`, `typeset`
"declaration_command",
"pipeline",
"list",
"redirected_statement",
Expand All @@ -67,7 +65,6 @@
"number",
"simple_expansion", # $VAR
"expansion", # ${VAR}
"arithmetic_expansion", # $((...))
"binary_expression",
"unary_expression",
"parenthesized_expression",
Expand Down Expand Up @@ -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:
Expand All @@ -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
Expand Down
41 changes: 41 additions & 0 deletions python/flink_agents/plan/tools/bash/tests/test_bash_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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 <<EOF\n$(rm /)\nEOF", ["cat"], [])
assert error is not None
Expand Down Expand Up @@ -237,6 +252,32 @@ def test_reject_command_substitution(self, tool: BashTool) -> 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
Expand Down
Loading