Address deep review feedback: error on disabled, strict arg/cmd validation, debug logs, docs

Agent-Logs-Url: https://github.com/mikefarah/yq/sessions/fbfba2db-60ea-4c20-a4c2-0fd396b80c81

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

cmd/root.go

@@ -213,10 +213,6 @@ yq -P -oy sample.json
 	rootCmd.PersistentFlags().BoolVarP(&yqlib.ConfiguredSecurityPreferences.DisableEnvOps, "security-disable-env-ops", "", false, "Disable env related operations.")
 	rootCmd.PersistentFlags().BoolVarP(&yqlib.ConfiguredSecurityPreferences.DisableFileOps, "security-disable-file-ops", "", false, "Disable file related operations (e.g. load)")
 	rootCmd.PersistentFlags().BoolVarP(&yqlib.ConfiguredSecurityPreferences.EnableSystemOps, "security-enable-system-operator", "", false, "Enable system operator to allow execution of external commands.")
-	rootCmd.PersistentFlags().BoolVarP(&yqlib.ConfiguredSecurityPreferences.EnableSystemOps, "enable-system-operator", "", false, "DEPRECATED: use --security-enable-system-operator instead. Enable system operator to allow execution of external commands.")
-	if err = rootCmd.MarkPersistentFlagDeprecated("enable-system-operator", "use --security-enable-system-operator instead."); err != nil {
-		panic(err)
-	}
 
 	rootCmd.AddCommand(
 		createEvaluateSequenceCommand(),

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

@@ -2,22 +2,26 @@
 
 The `system` operator allows you to run an external command and use its output as a value in your expression.
 
-**Security warning**: The system operator is disabled by default. You must explicitly pass `--enable-system-operator` to use it.
+**Security warning**: The system operator is disabled by default. You must explicitly pass `--security-enable-system-operator` to use it.
+
+**Note:** When enabled, the system operator can replicate the functionality of `env` and `load`
+operators via external commands. Enabling it effectively overrides `--security-disable-env-ops`
+and `--security-disable-file-ops`.
 
 ## Usage
 
 ```bash
-yq --enable-system-operator --null-input '.field = system("command"; "arg1")'
+yq --security-enable-system-operator --null-input '.field = system("command"; "arg1")'
 ```
 
 The operator takes:
 - A command string (required)
-- An argument or array of arguments separated by `;` (optional)
+- An argument (or an array of arguments), separated from the command by `;` (optional)
 
 The current matched node's value is serialised and piped to the command via stdin. The command's stdout (with trailing newline stripped) is returned as a string.
 
 ## Disabling the system operator
 
-The system operator is disabled by default. When disabled, a warning is logged and `null` is returned instead of running the command.
+The system operator is disabled by default. When disabled, an error is returned instead of running the command, consistent with `--security-disable-env-ops` and `--security-disable-file-ops`.
 
-Use `--enable-system-operator` flag to enable it.
+Use `--security-enable-system-operator` flag to enable it.

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

@@ -2,28 +2,32 @@
 
 The `system` operator allows you to run an external command and use its output as a value in your expression.
 
-**Security warning**: The system operator is disabled by default. You must explicitly pass `--enable-system-operator` to use it.
+**Security warning**: The system operator is disabled by default. You must explicitly pass `--security-enable-system-operator` to use it.
+
+**Note:** When enabled, the system operator can replicate the functionality of `env` and `load`
+operators via external commands. Enabling it effectively overrides `--security-disable-env-ops`
+and `--security-disable-file-ops`.
 
 ## Usage
 
 ```bash
-yq --enable-system-operator --null-input '.field = system("command"; "arg1")'
+yq --security-enable-system-operator --null-input '.field = system("command"; "arg1")'
 ```
 
 The operator takes:
 - A command string (required)
-- An argument or array of arguments separated by `;` (optional)
+- An argument (or an array of arguments), separated from the command by `;` (optional)
 
 The current matched node's value is serialised and piped to the command via stdin. The command's stdout (with trailing newline stripped) is returned as a string.
 
 ## Disabling the system operator
 
-The system operator is disabled by default. When disabled, a warning is logged and `null` is returned instead of running the command.
+The system operator is disabled by default. When disabled, an error is returned instead of running the command, consistent with `--security-disable-env-ops` and `--security-disable-file-ops`.
 
-Use `--enable-system-operator` flag to enable it.
+Use `--security-enable-system-operator` flag to enable it.
 
-## system operator returns null when disabled
-Use `--enable-system-operator` to enable the system operator.
+## system operator returns error when disabled
+Use `--security-enable-system-operator` to enable the system operator.
 
 Given a sample.yml file of:
 ```yaml
@@ -34,12 +38,12 @@ then
 yq '.country = system("/usr/bin/echo"; "test")' sample.yml
 ```
 will output
-```yaml
-country: null
+```bash
+Error: system operations are disabled, use --security-enable-system-operator to enable
 ```
 
 ## Run a command with an argument
-Use `--enable-system-operator` to enable the system operator.
+Use `--security-enable-system-operator` to enable the system operator.
 
 Given a sample.yml file of:
 ```yaml
@@ -47,7 +51,7 @@ country: Australia
 ```
 then
 ```bash
-yq --enable-system-operator '.country = system("/usr/bin/echo"; "test")' sample.yml
+yq --security-enable-system-operator '.country = system("/usr/bin/echo"; "test")' sample.yml
 ```
 will output
 ```yaml
@@ -63,7 +67,7 @@ a: hello
 ```
 then
 ```bash
-yq --enable-system-operator '.a = system("/usr/bin/echo")' sample.yml
+yq --security-enable-system-operator '.a = system("/usr/bin/echo")' sample.yml
 ```
 will output
 ```yaml

pkg/yqlib/operator_system.go

@@ -8,18 +8,37 @@ import (
 	"strings"
 )
 
-func resolveSystemArgs(argsNode *CandidateNode) []string {
+func resolveSystemArgs(argsNode *CandidateNode) ([]string, error) {
+	if argsNode == nil {
+		return nil, nil
+	}
+
 	if argsNode.Kind == SequenceNode {
 		args := make([]string, 0, len(argsNode.Content))
 		for _, child := range argsNode.Content {
+			// Only non-null scalar children are valid arguments.
+			if child == nil {
+				continue
+			}
+			if child.Kind != ScalarNode || child.Tag == "!!null" {
+				return nil, fmt.Errorf("system operator: argument must be a non-null scalar; got kind=%v tag=%v", child.Kind, child.Tag)
+			}
 			args = append(args, child.Value)
 		}
-		return args
+		if len(args) == 0 {
+			return nil, nil
+		}
+		return args, nil
 	}
-	if argsNode.Tag != "!!null" {
-		return []string{argsNode.Value}
+
+	// Single-argument case: only accept a non-null scalar node.
+	if argsNode.Tag == "!!null" {
+		return nil, nil
 	}
-	return nil
+	if argsNode.Kind != ScalarNode {
+		return nil, fmt.Errorf("system operator: args must be a non-null scalar or sequence of non-null scalars; got kind=%v tag=%v", argsNode.Kind, argsNode.Tag)
+	}
+	return []string{argsNode.Value}, nil
 }
 
 func resolveCommandNode(commandNodes Context) (string, error) {
@@ -30,7 +49,7 @@ func resolveCommandNode(commandNodes Context) (string, error) {
 		log.Debugf("system operator: command expression returned %d results, using first", commandNodes.MatchingNodes.Len())
 	}
 	cmdNode := commandNodes.MatchingNodes.Front().Value.(*CandidateNode)
-	if cmdNode.Kind != ScalarNode || cmdNode.Tag == "!!null" {
+	if cmdNode.Kind != ScalarNode || cmdNode.guessTagFromCustomType() != "!!str" {
 		return "", fmt.Errorf("system operator: command must be a string scalar")
 	}
 	if cmdNode.Value == "" {
@@ -41,13 +60,7 @@ func resolveCommandNode(commandNodes Context) (string, error) {
 
 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")
-		results := list.New()
-		for el := context.MatchingNodes.Front(); el != nil; el = el.Next() {
-			candidate := el.Value.(*CandidateNode)
-			results.PushBack(candidate.CreateReplacement(ScalarNode, "!!null", "null"))
-		}
-		return context.ChildContext(results), nil
+		return Context{}, fmt.Errorf("system operations are disabled, use --security-enable-system-operator to enable")
 	}
 
 	// determine at parse time whether we have (command; args) or just (command)
@@ -77,8 +90,14 @@ func systemOperator(d *dataTreeNavigator, context Context, expressionNode *Expre
 			if err != nil {
 				return Context{}, err
 			}
+			if argsNodes.MatchingNodes.Len() > 1 {
+				log.Debugf("system operator: args expression returned %d results, using first", argsNodes.MatchingNodes.Len())
+			}
 			if argsNodes.MatchingNodes.Front() != nil {
-				args = resolveSystemArgs(argsNodes.MatchingNodes.Front().Value.(*CandidateNode))
+				args, err = resolveSystemArgs(argsNodes.MatchingNodes.Front().Value.(*CandidateNode))
+				if err != nil {
+					return Context{}, err
+				}
 			}
 		} else {
 			commandNodes, err := d.GetMatchingNodes(nodeContext, expressionNode.RHS)

pkg/yqlib/operator_system_test.go

@@ -16,13 +16,11 @@ func findExec(t *testing.T, name string) string {
 
 var systemOperatorDisabledScenarios = []expressionScenario{
 	{
-		description:    "system operator returns null when disabled",
-		subdescription: "Use `--enable-system-operator` to enable the system operator.",
+		description:    "system operator returns error when disabled",
+		subdescription: "Use `--security-enable-system-operator` to enable the system operator.",
 		document:       "country: Australia",
 		expression:     `.country = system("/usr/bin/echo"; "test")`,
-		expected: []string{
-			"D0, P[], (!!map)::country: null\n",
-		},
+		expectedError:  "system operations are disabled, use --security-enable-system-operator to enable",
 	},
 }
 
@@ -54,8 +52,8 @@ func TestSystemOperatorEnabledScenarios(t *testing.T) {
 	scenarios := []expressionScenario{
 		{
 			description:    "Run a command with an argument",
-			subdescription: "Use `--enable-system-operator` to enable the system operator.",
-			yqFlags:        "--enable-system-operator",
+			subdescription: "Use `--security-enable-system-operator` to enable the system operator.",
+			yqFlags:        "--security-enable-system-operator",
 			document:       "country: Australia",
 			expression:     `.country = system("` + echoPath + `"; "test")`,
 			expected: []string{
@@ -65,7 +63,7 @@ func TestSystemOperatorEnabledScenarios(t *testing.T) {
 		{
 			description:    "Run a command without arguments",
 			subdescription: "Omit the semicolon and args to run the command with no extra arguments.",
-			yqFlags:        "--enable-system-operator",
+			yqFlags:        "--security-enable-system-operator",
 			document:       "a: hello",
 			expression:     `.a = system("` + echoPath + `")`,
 			expected: []string{
@@ -105,6 +103,17 @@ func TestSystemOperatorEnabledScenarios(t *testing.T) {
 			expression:    `.a = system(null)`,
 			expectedError: "system operator: command must be a string scalar",
 		},
+		{
+			description: "System operator processes multiple matched nodes",
+			skipDoc:     true,
+			document:    "a: first",
+			document2:   "a: second",
+			expression:  `.a = system("` + echoPath + `"; "replaced")`,
+			expected: []string{
+				"D0, P[], (!!map)::a: replaced\n",
+				"D0, P[], (!!map)::a: replaced\n",
+			},
+		},
 	}
 
 	for _, tt := range scenarios {