From 62d28d54959751eaae752d7550de8989fbdec5e1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Apr 2026 06:44:30 +0000 Subject: [PATCH] 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> --- .../doc/operators/headers/system-operators.md | 8 +++-- pkg/yqlib/doc/operators/system-operators.md | 14 +++++--- pkg/yqlib/operator_system.go | 36 +++++++++---------- pkg/yqlib/operator_system_test.go | 17 ++++++--- 4 files changed, 45 insertions(+), 30 deletions(-) diff --git a/pkg/yqlib/doc/operators/headers/system-operators.md b/pkg/yqlib/doc/operators/headers/system-operators.md index a58fa7f3..220a2fa3 100644 --- a/pkg/yqlib/doc/operators/headers/system-operators.md +++ b/pkg/yqlib/doc/operators/headers/system-operators.md @@ -4,6 +4,10 @@ The `system` operator allows you to run an external command and use its output a **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 @@ -12,12 +16,12 @@ yq --security-enable-system-operator --null-input '.field = system("command"; "a 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 `--security-enable-system-operator` flag to enable it. diff --git a/pkg/yqlib/doc/operators/system-operators.md b/pkg/yqlib/doc/operators/system-operators.md index 65501b25..df1a76cd 100644 --- a/pkg/yqlib/doc/operators/system-operators.md +++ b/pkg/yqlib/doc/operators/system-operators.md @@ -4,6 +4,10 @@ The `system` operator allows you to run an external command and use its output a **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 @@ -12,17 +16,17 @@ yq --security-enable-system-operator --null-input '.field = system("command"; "a 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 `--security-enable-system-operator` flag to enable it. -## system operator returns null when disabled +## system operator returns error when disabled Use `--security-enable-system-operator` to enable the system operator. Given a sample.yml file of: @@ -34,8 +38,8 @@ 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 diff --git a/pkg/yqlib/operator_system.go b/pkg/yqlib/operator_system.go index 5f22508d..c9937957 100644 --- a/pkg/yqlib/operator_system.go +++ b/pkg/yqlib/operator_system.go @@ -8,9 +8,9 @@ import ( "strings" ) -func resolveSystemArgs(argsNode *CandidateNode) []string { +func resolveSystemArgs(argsNode *CandidateNode) ([]string, error) { if argsNode == nil { - return nil + return nil, nil } if argsNode.Kind == SequenceNode { @@ -21,26 +21,24 @@ func resolveSystemArgs(argsNode *CandidateNode) []string { continue } if child.Kind != ScalarNode || child.Tag == "!!null" { - log.Warningf("system operator: argument must be a non-null scalar; got kind=%v tag=%v - ignoring", child.Kind, child.Tag) - continue + 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) } if len(args) == 0 { - return nil + return nil, nil } - return args + return args, nil } // Single-argument case: only accept a non-null scalar node. if argsNode.Tag == "!!null" { - return nil + return nil, nil } if argsNode.Kind != ScalarNode { - log.Warningf("system operator: args must be a non-null scalar or sequence of non-null scalars; got kind=%v tag=%v - ignoring", argsNode.Kind, argsNode.Tag) - return nil + 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} + return []string{argsNode.Value}, nil } func resolveCommandNode(commandNodes Context) (string, error) { @@ -51,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 == "" { @@ -62,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 --security-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) @@ -98,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) diff --git a/pkg/yqlib/operator_system_test.go b/pkg/yqlib/operator_system_test.go index 4f0e4da8..46a0ecd4 100644 --- a/pkg/yqlib/operator_system_test.go +++ b/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", + 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", }, } @@ -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 {