From 0e45f3b0e3c9e81685bc85b7c4f21187bb0df085 Mon Sep 17 00:00:00 2001 From: jamesread Date: Tue, 7 Jul 2026 13:13:35 +0100 Subject: [PATCH] security: GHSA-xc5w-4v5w-7x65 (HIGH) harden shell argument type safety Block additional unvalidated argument types from shell actions, enforce full-string custom regex matching, and allowlist http/https URL schemes. Update checkbox integration test to use exec, matching the intended shell vs exec split for choiceless checkbox arguments. Co-authored-by: Cursor --- integration-tests/tests/checkbox/config.yaml | 6 +- service/internal/executor/arguments.go | 53 +++++++++++++--- service/internal/executor/arguments_test.go | 66 ++++++++++++++++++++ 3 files changed, 115 insertions(+), 10 deletions(-) diff --git a/integration-tests/tests/checkbox/config.yaml b/integration-tests/tests/checkbox/config.yaml index 2afc362..8c14ebd 100644 --- a/integration-tests/tests/checkbox/config.yaml +++ b/integration-tests/tests/checkbox/config.yaml @@ -7,7 +7,9 @@ defaultPopupOnStart: execution-dialog actions: - title: Test checkbox argument - shell: "echo 'Checkbox value: {{ confirm }}'" + exec: + - echo + - "Checkbox value: {{ confirm }}" icon: ping arguments: - name: confirm @@ -15,5 +17,3 @@ actions: type: checkbox description: "When checked: 1, when unchecked: 0" default: false - - diff --git a/service/internal/executor/arguments.go b/service/internal/executor/arguments.go index 4b8ac4b..8eeae9b 100644 --- a/service/internal/executor/arguments.go +++ b/service/internal/executor/arguments.go @@ -314,11 +314,21 @@ func typeSafetyCheckDatetime(value string) error { return nil } +func anchorCustomRegexPattern(pattern string) string { + if strings.HasPrefix(pattern, "^") && strings.HasSuffix(pattern, "$") { + return pattern + } + + return "^(?:" + pattern + ")$" +} + func typeSafetyCheckRegex(name string, value string, argumentType string) error { pattern := "" + isCustomRegex := strings.HasPrefix(argumentType, "regex:") - if strings.HasPrefix(argumentType, "regex:") { - pattern = strings.Replace(argumentType, "regex:", "", 1) + if isCustomRegex { + pattern = strings.TrimPrefix(argumentType, "regex:") + pattern = anchorCustomRegexPattern(pattern) } else { found := false pattern, found = typecheckRegex[argumentType] @@ -345,21 +355,50 @@ func typeSafetyCheckRegex(name string, value string, argumentType string) error } func typeSafetyCheckUrl(value string) error { - _, err := url.ParseRequestURI(value) + parsed, err := url.ParseRequestURI(value) + if err != nil { + return err + } - return err + scheme := strings.ToLower(parsed.Scheme) + if scheme != "http" && scheme != "https" { + return fmt.Errorf("url scheme %q is not allowed; only http and https are permitted", parsed.Scheme) + } + + return nil +} + +var shellUnsafeArgumentTypes = map[string]struct{}{ + "url": {}, + "email": {}, + "raw_string_multiline": {}, + "very_dangerous_raw_string": {}, + "password": {}, + "html": {}, + "confirmation": {}, +} + +func isUnsafeShellArgumentType(arg *config.ActionArgument) bool { + if strings.HasPrefix(arg.Type, "regex:") { + return true + } + + _, inMap := shellUnsafeArgumentTypes[arg.Type] + return inMap || (arg.Type == "checkbox" && len(arg.Choices) == 0) } func checkShellArgumentSafety(action *config.Action) error { if action.Shell == "" { return nil } - unsafe := map[string]struct{}{"url": {}, "email": {}, "raw_string_multiline": {}, "very_dangerous_raw_string": {}, "password": {}} - for _, arg := range action.Arguments { - if _, bad := unsafe[arg.Type]; bad { + + for i := range action.Arguments { + arg := &action.Arguments[i] + if isUnsafeShellArgumentType(arg) { return fmt.Errorf("unsafe argument type '%s' cannot be used with Shell execution. Use 'exec' instead. See https://docs.olivetin.app/action_execution/shellvsexec.html", arg.Type) } } + return nil } diff --git a/service/internal/executor/arguments_test.go b/service/internal/executor/arguments_test.go index 3c2a917..d820cba 100644 --- a/service/internal/executor/arguments_test.go +++ b/service/internal/executor/arguments_test.go @@ -499,13 +499,72 @@ func TestCheckShellArgumentSafetyWithPasswordAndExec(t *testing.T) { assert.Nil(t, err) } +func TestCheckShellArgumentSafetyWithHTML(t *testing.T) { + a1 := config.Action{ + Title: "HTML shell", + Shell: "echo {{ body }}", + Arguments: []config.ActionArgument{ + {Name: "body", Type: "html"}, + }, + } + + err := checkShellArgumentSafety(&a1) + assert.NotNil(t, err) + assert.Contains(t, err.Error(), "unsafe argument type 'html'") +} + +func TestCheckShellArgumentSafetyWithConfirmation(t *testing.T) { + a1 := config.Action{ + Title: "Confirm shell", + Shell: "echo ok", + Arguments: []config.ActionArgument{ + {Name: "agree", Type: "confirmation"}, + }, + } + + err := checkShellArgumentSafety(&a1) + assert.NotNil(t, err) + assert.Contains(t, err.Error(), "unsafe argument type 'confirmation'") +} + +func TestCheckShellArgumentSafetyWithChoicelessCheckbox(t *testing.T) { + a1 := config.Action{ + Title: "Checkbox shell", + Shell: "echo {{ flag }}", + Arguments: []config.ActionArgument{ + {Name: "flag", Type: "checkbox"}, + }, + } + + err := checkShellArgumentSafety(&a1) + assert.NotNil(t, err) + assert.Contains(t, err.Error(), "unsafe argument type 'checkbox'") +} + +func TestCheckShellArgumentSafetyWithCustomRegex(t *testing.T) { + a1 := config.Action{ + Title: "Regex shell", + Shell: "curl {{ host }}", + Arguments: []config.ActionArgument{ + {Name: "host", Type: "regex:[a-zA-Z0-9.-]+"}, + }, + } + + err := checkShellArgumentSafety(&a1) + assert.NotNil(t, err) + assert.Contains(t, err.Error(), "unsafe argument type 'regex:[a-zA-Z0-9.-]+'") +} + func TestTypeSafetyCheckUrl(t *testing.T) { assert.Nil(t, TypeSafetyCheck("test1", "http://google.com", "url"), "Test URL: google.com") assert.Nil(t, TypeSafetyCheck("test2", "http://technowax.net:80?foo=bar", "url"), "Test URL: technowax.net with query arguments") assert.Nil(t, TypeSafetyCheck("test3", "http://localhost:80?foo=bar", "url"), "Test URL: localhost with query arguments") + assert.Nil(t, TypeSafetyCheck("test7", "https://example.com/path", "url"), "Test URL: https scheme") assert.NotNil(t, TypeSafetyCheck("test4", "http://lo host:80", "url"), "Test a badly formed URL") assert.NotNil(t, TypeSafetyCheck("test5", "12345", "url"), "Test a badly formed URL") assert.NotNil(t, TypeSafetyCheck("test6", "_!23;", "url"), "Test a badly formed URL") + assert.NotNil(t, TypeSafetyCheck("test8", "file:///etc/passwd", "url"), "file:// scheme must be rejected") + assert.NotNil(t, TypeSafetyCheck("test9", "gopher://example.com", "url"), "gopher:// scheme must be rejected") } func TestTypeSafetyCheckRegex(t *testing.T) { @@ -530,6 +589,13 @@ func TestTypeSafetyCheckRegex(t *testing.T) { value: "James1234", hasError: true, }, + { + name: "GHSA-gvxq - reject partial regex match", + field: "host", + pattern: "regex:[a-zA-Z0-9.-]+", + value: "example.com; id", + hasError: true, + }, } for _, tt := range tests {