Evaluate system command/args per matched node using SingleReadonlyChildContext

Agent-Logs-Url: https://github.com/mikefarah/yq/sessions/dca841eb-3f63-4f23-adeb-556431560420

Co-authored-by: mikefarah <1151925+mikefarah@users.noreply.github.com>

pkg/yqlib/doc/operators/system-operators.md

@@ -63,7 +63,7 @@ a: hello
 ```
 then
 ```bash
-yq '.a = system("/bin/echo")' sample.yml
+yq '.a = system("/usr/bin/echo")' sample.yml
 ```
 will output
 ```yaml

pkg/yqlib/operator_system.go

@@ -8,6 +8,20 @@ import (
 	"strings"
 )
 
+func resolveSystemArgs(argsNode *CandidateNode) []string {
+	if argsNode.Kind == SequenceNode {
+		args := make([]string, 0, len(argsNode.Content))
+		for _, child := range argsNode.Content {
+			args = append(args, child.Value)
+		}
+		return args
+	}
+	if argsNode.Tag != "!!null" {
+		return []string{argsNode.Value}
+	}
+	return nil
+}
+
 func systemOperator(d *dataTreeNavigator, context Context, expressionNode *ExpressionNode) (Context, error) {
 	if !ConfiguredSecurityPreferences.EnableSystemOps {
 		log.Warning("system operator is disabled, use --enable-system-operator flag to enable")
@@ -19,55 +33,46 @@ func systemOperator(d *dataTreeNavigator, context Context, expressionNode *Expre
 		return context.ChildContext(results), nil
 	}
 
-	var command string
+	// determine at parse time whether we have (command; args) or just (command)
-	var argsExpression *ExpressionNode
+	hasArgs := expressionNode.RHS.Operation.OperationType == blockOpType
-
-	// check if it's a block operator (command; args) or just (command)
-	if expressionNode.RHS.Operation.OperationType == blockOpType {
-		block := expressionNode.RHS
-		commandNodes, err := d.GetMatchingNodes(context.ReadOnlyClone(), block.LHS)
-		if err != nil {
-			return Context{}, err
-		}
-		if commandNodes.MatchingNodes.Front() == nil {
-			return Context{}, fmt.Errorf("system operator: command expression returned no results")
-		}
-		command = commandNodes.MatchingNodes.Front().Value.(*CandidateNode).Value
-		argsExpression = block.RHS
-	} else {
-		commandNodes, err := d.GetMatchingNodes(context.ReadOnlyClone(), expressionNode.RHS)
-		if err != nil {
-			return Context{}, err
-		}
-		if commandNodes.MatchingNodes.Front() == nil {
-			return Context{}, fmt.Errorf("system operator: command expression returned no results")
-		}
-		command = commandNodes.MatchingNodes.Front().Value.(*CandidateNode).Value
-	}
-
-	// evaluate args if present
-	var args []string
-	if argsExpression != nil {
-		argsNodes, err := d.GetMatchingNodes(context.ReadOnlyClone(), argsExpression)
-		if err != nil {
-			return Context{}, err
-		}
-		if argsNodes.MatchingNodes.Front() != nil {
-			argsNode := argsNodes.MatchingNodes.Front().Value.(*CandidateNode)
-			if argsNode.Kind == SequenceNode {
-				for _, child := range argsNode.Content {
-					args = append(args, child.Value)
-				}
-			} else if argsNode.Tag != "!!null" {
-				args = []string{argsNode.Value}
-			}
-		}
-	}
 
 	var results = list.New()
 
 	for el := context.MatchingNodes.Front(); el != nil; el = el.Next() {
 		candidate := el.Value.(*CandidateNode)
+		nodeContext := context.SingleReadonlyChildContext(candidate)
+
+		var command string
+		var args []string
+
+		if hasArgs {
+			block := expressionNode.RHS
+			commandNodes, err := d.GetMatchingNodes(nodeContext, block.LHS)
+			if err != nil {
+				return Context{}, err
+			}
+			if commandNodes.MatchingNodes.Front() == nil {
+				return Context{}, fmt.Errorf("system operator: command expression returned no results")
+			}
+			command = commandNodes.MatchingNodes.Front().Value.(*CandidateNode).Value
+
+			argsNodes, err := d.GetMatchingNodes(nodeContext, block.RHS)
+			if err != nil {
+				return Context{}, err
+			}
+			if argsNodes.MatchingNodes.Front() != nil {
+				args = resolveSystemArgs(argsNodes.MatchingNodes.Front().Value.(*CandidateNode))
+			}
+		} else {
+			commandNodes, err := d.GetMatchingNodes(nodeContext, expressionNode.RHS)
+			if err != nil {
+				return Context{}, err
+			}
+			if commandNodes.MatchingNodes.Front() == nil {
+				return Context{}, fmt.Errorf("system operator: command expression returned no results")
+			}
+			command = commandNodes.MatchingNodes.Front().Value.(*CandidateNode).Value
+		}
 
 		var stdin bytes.Buffer
 		if candidate.Tag != "!!null" {

pkg/yqlib/operator_system_test.go

@@ -1,9 +1,19 @@
 package yqlib
 
 import (
+	"os/exec"
 	"testing"
 )
 
+func findExec(t *testing.T, name string) string {
+	t.Helper()
+	path, err := exec.LookPath(name)
+	if err != nil {
+		t.Skipf("skipping: %v not found: %v", name, err)
+	}
+	return path
+}
+
 var systemOperatorDisabledScenarios = []expressionScenario{
 	{
 		description:    "system operator returns null when disabled",
@@ -16,46 +26,7 @@ var systemOperatorDisabledScenarios = []expressionScenario{
 	},
 }
 
-var systemOperatorEnabledScenarios = []expressionScenario{
-	{
-		description:    "Run a command with an argument",
-		subdescription: "Use `--enable-system-operator` to enable the system operator.",
-		document:       "country: Australia",
-		expression:     `.country = system("/usr/bin/echo"; "test")`,
-		expected: []string{
-			"D0, P[], (!!map)::country: test\n",
-		},
-	},
-	{
-		description:    "Run a command without arguments",
-		subdescription: "Omit the semicolon and args to run the command with no extra arguments.",
-		document:       "a: hello",
-		expression:     `.a = system("/bin/echo")`,
-		expected: []string{
-			"D0, P[], (!!map)::a: \"\"\n",
-		},
-	},
-	{
-		description:    "Run a command with multiple arguments",
-		subdescription: "Pass an array of arguments.",
-		skipDoc:        true,
-		document:       "a: hello",
-		expression:     `.a = system("/bin/echo"; ["foo", "bar"])`,
-		expected: []string{
-			"D0, P[], (!!map)::a: foo bar\n",
-		},
-	},
-	{
-		description:   "Command failure returns error",
-		skipDoc:       true,
-		document:      "a: hello",
-		expression:    `.a = system("/bin/false")`,
-		expectedError: "system command '/bin/false' failed: exit status 1",
-	},
-}
-
 func TestSystemOperatorDisabledScenarios(t *testing.T) {
-	// ensure system operator is disabled
 	originalEnableSystemOps := ConfiguredSecurityPreferences.EnableSystemOps
 	defer func() {
 		ConfiguredSecurityPreferences.EnableSystemOps = originalEnableSystemOps
@@ -70,6 +41,9 @@ func TestSystemOperatorDisabledScenarios(t *testing.T) {
 }
 
 func TestSystemOperatorEnabledScenarios(t *testing.T) {
+	echoPath := findExec(t, "echo")
+	falsePath := findExec(t, "false")
+
 	originalEnableSystemOps := ConfiguredSecurityPreferences.EnableSystemOps
 	defer func() {
 		ConfiguredSecurityPreferences.EnableSystemOps = originalEnableSystemOps
@@ -77,8 +51,55 @@ func TestSystemOperatorEnabledScenarios(t *testing.T) {
 
 	ConfiguredSecurityPreferences.EnableSystemOps = true
 
-	for _, tt := range systemOperatorEnabledScenarios {
+	scenarios := []expressionScenario{
+		{
+			description:    "Run a command with an argument",
+			subdescription: "Use `--enable-system-operator` to enable the system operator.",
+			document:       "country: Australia",
+			expression:     `.country = system("` + echoPath + `"; "test")`,
+			expected: []string{
+				"D0, P[], (!!map)::country: test\n",
+			},
+		},
+		{
+			description:    "Run a command without arguments",
+			subdescription: "Omit the semicolon and args to run the command with no extra arguments.",
+			document:       "a: hello",
+			expression:     `.a = system("` + echoPath + `")`,
+			expected: []string{
+				"D0, P[], (!!map)::a: \"\"\n",
+			},
+		},
+		{
+			description:    "Run a command with multiple arguments",
+			subdescription: "Pass an array of arguments.",
+			skipDoc:        true,
+			document:       "a: hello",
+			expression:     `.a = system("` + echoPath + `"; ["foo", "bar"])`,
+			expected: []string{
+				"D0, P[], (!!map)::a: foo bar\n",
+			},
+		},
+		{
+			description: "Command and args are evaluated per matched node",
+			skipDoc:     true,
+			document:    "cmd: " + echoPath + "\narg: hello",
+			expression:  `.result = system(.cmd; .arg)`,
+			expected: []string{
+				"D0, P[], (!!map)::cmd: " + echoPath + "\narg: hello\nresult: hello\n",
+			},
+		},
+		{
+			description:   "Command failure returns error",
+			skipDoc:       true,
+			document:      "a: hello",
+			expression:    `.a = system("` + falsePath + `")`,
+			expectedError: "system command '" + falsePath + "' failed: exit status 1",
+		},
+	}
+
+	for _, tt := range scenarios {
 		testScenario(t, &tt)
 	}
-	appendOperatorDocumentScenario(t, "system-operators", systemOperatorEnabledScenarios)
+	appendOperatorDocumentScenario(t, "system-operators", scenarios)
 }