From d4d3193c1d44e45f0a5f3f9563bd79bea40b98d4 Mon Sep 17 00:00:00 2001 From: James Read Date: Tue, 29 Jul 2025 22:51:34 +0100 Subject: [PATCH] bugfix: Argument confirmations stopped working (#627) (#632) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- service/internal/executor/arguments.go | 22 +- service/internal/executor/arguments_test.go | 415 ++++++++++++++++++++ 2 files changed, 425 insertions(+), 12 deletions(-) diff --git a/service/internal/executor/arguments.go b/service/internal/executor/arguments.go index 7a8ad33..2485078 100644 --- a/service/internal/executor/arguments.go +++ b/service/internal/executor/arguments.go @@ -52,7 +52,7 @@ func parseActionArguments(values map[string]string, action *config.Action, entit argName := arg.Name argValue := values[argName] - err := typecheckActionArgument(argName, argValue, action) + err := typecheckActionArgument(&arg, argValue, action) if err != nil { return "", err @@ -102,21 +102,19 @@ func redactShellCommand(shellCommand string, arguments []config.ActionArgument, return shellCommand } -func typecheckActionArgument(name string, value string, action *config.Action) error { - if name == "" { +func typecheckActionArgument(arg *config.ActionArgument, value string, action *config.Action) error { + if arg.Type == "confirmation" { + return nil + } + + if arg.Name == "" { return fmt.Errorf("argument name cannot be empty") } - arg := action.FindArg(name) - - if arg == nil { - return fmt.Errorf("action arg not defined: %v", name) - } - - return typecheckActionArgumentFound(name, value, action, arg) + return typecheckActionArgumentFound(value, action, arg) } -func typecheckActionArgumentFound(name string, value string, action *config.Action, arg *config.ActionArgument) error { +func typecheckActionArgumentFound(value string, action *config.Action, arg *config.ActionArgument) error { if value == "" { return typecheckNull(arg) } @@ -125,7 +123,7 @@ func typecheckActionArgumentFound(name string, value string, action *config.Acti return typecheckChoice(value, arg) } - return TypeSafetyCheck(name, value, arg.Type) + return TypeSafetyCheck(arg.Name, value, arg.Type) } // TypeSafetyCheck checks argument values match a specific type. The types are diff --git a/service/internal/executor/arguments_test.go b/service/internal/executor/arguments_test.go index 52ad8f6..e76030b 100644 --- a/service/internal/executor/arguments_test.go +++ b/service/internal/executor/arguments_test.go @@ -1,6 +1,9 @@ package executor import ( + "fmt" + "strings" + config "github.com/OliveTin/OliveTin/internal/config" "github.com/stretchr/testify/assert" @@ -166,3 +169,415 @@ func TestRedactShellCommand(t *testing.T) { res = redactShellCommand(cmd, args, values) assert.Equal(t, cmd, res, "Missing password argument should not change the command") } + +func TestTypeSafetyCheckEmail(t *testing.T) { + tests := []struct { + name string + field string + value string + hasError bool + }{ + {"Valid simple email", "email", "user@example.com", false}, + {"Valid email with subdomain", "email", "user@mail.example.com", false}, + {"Valid email with plus", "email", "user+test@example.com", false}, + {"Valid email with dash", "email", "user-name@example.com", false}, + {"Valid email with numbers", "email", "user123@example123.com", false}, + {"Invalid email no @", "email", "userexample.com", true}, + {"Invalid email no domain", "email", "user@", true}, + {"Invalid email no user", "email", "@example.com", true}, + {"Invalid email spaces", "email", "user name@example.com", true}, + {"Invalid email double @", "email", "user@@example.com", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := TypeSafetyCheck(tt.field, tt.value, "email") + if tt.hasError { + assert.NotNil(t, err, "Expected error for value '%s'", tt.value) + } else { + assert.Nil(t, err, "Expected no error for value '%s', but got: %v", tt.value, err) + } + }) + } +} + +func TestTypeSafetyCheckDatetime(t *testing.T) { + tests := []struct { + name string + field string + value string + hasError bool + }{ + {"Valid datetime", "datetime", "2023-12-25T15:30:45", false}, + {"Valid datetime morning", "datetime", "2023-01-01T00:00:00", false}, + {"Valid datetime evening", "datetime", "2023-12-31T23:59:59", false}, + {"Invalid format missing T", "datetime", "2023-12-25 15:30:45", true}, + {"Invalid format missing seconds", "datetime", "2023-12-25T15:30", true}, + {"Invalid date", "datetime", "2023-13-25T15:30:45", true}, + {"Invalid time", "datetime", "2023-12-25T25:30:45", true}, + {"Random string", "datetime", "not-a-date", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := TypeSafetyCheck(tt.field, tt.value, "datetime") + if tt.hasError { + assert.NotNil(t, err, "Expected error for value '%s'", tt.value) + } else { + assert.Nil(t, err, "Expected no error for value '%s', but got: %v", tt.value, err) + } + }) + } +} + +func TestTypeSafetyCheckRawStringMultiline(t *testing.T) { + tests := []struct { + name string + field string + value string + }{ + {"Simple string", "content", "hello world"}, + {"Multiline string", "content", "line1\nline2\nline3"}, + {"String with special chars", "content", "!@#$%^&*()"}, + {"Unicode string", "content", "héllo wörld 🌍"}, + {"Very long string", "content", strings.Repeat("a", 1000)}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := TypeSafetyCheck(tt.field, tt.value, "raw_string_multiline") + assert.Nil(t, err, "raw_string_multiline should accept any value") + }) + } +} + +func TestTypeSafetyCheckUnicodeIdentifier(t *testing.T) { + tests := []struct { + name string + field string + value string + hasError bool + }{ + {"Valid unicode identifier", "name", "hello_world", false}, + {"Valid with numbers", "name", "test123", false}, + {"Valid with spaces", "name", "hello world", false}, + {"Valid with path separators", "name", "path/to/file", false}, + {"Valid with backslashes", "name", "path\\to\\file", false}, + {"Valid with dots", "name", "file.txt", false}, + {"Valid with underscores", "name", "my_file_name", false}, + {"Invalid with special chars", "name", "hello@world", true}, + {"Invalid with brackets", "name", "hello[world]", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := TypeSafetyCheck(tt.field, tt.value, "unicode_identifier") + if tt.hasError { + assert.NotNil(t, err, "Expected error for value '%s'", tt.value) + } else { + assert.Nil(t, err, "Expected no error for value '%s', but got: %v", tt.value, err) + } + }) + } +} + +func TestTypeSafetyCheckAsciiIdentifier(t *testing.T) { + tests := []struct { + name string + field string + value string + hasError bool + }{ + {"Valid identifier", "name", "hello_world", false}, + {"Valid with numbers", "name", "test123", false}, + {"Valid with dots", "name", "file.txt", false}, + {"Valid with dashes", "name", "my-file", false}, + {"Valid with underscores", "name", "my_file", false}, + {"Invalid with spaces", "name", "hello world", true}, + {"Invalid with special chars", "name", "hello@world", true}, + {"Invalid unicode", "name", "héllo", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := TypeSafetyCheck(tt.field, tt.value, "ascii_identifier") + if tt.hasError { + assert.NotNil(t, err, "Expected error for value '%s'", tt.value) + } else { + assert.Nil(t, err, "Expected no error for value '%s', but got: %v", tt.value, err) + } + }) + } +} + +func TestTypeSafetyCheckAsciiSentence(t *testing.T) { + tests := []struct { + name string + field string + value string + hasError bool + }{ + {"Valid sentence", "text", "Hello world", false}, + {"Valid with numbers", "text", "Test 123", false}, + {"Valid with commas", "text", "Hello, world", false}, + {"Valid with periods", "text", "Hello world.", false}, + {"Valid with multiple spaces", "text", "Hello world", false}, + {"Invalid with special chars", "text", "Hello@world", true}, + {"Invalid with parentheses", "text", "Hello (world)", true}, + {"Invalid unicode", "text", "Héllo world", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := TypeSafetyCheck(tt.field, tt.value, "ascii_sentence") + if tt.hasError { + assert.NotNil(t, err, "Expected error for value '%s'", tt.value) + } else { + assert.Nil(t, err, "Expected no error for value '%s', but got: %v", tt.value, err) + } + }) + } +} + +func TestTypecheckActionArgumentEmptyName(t *testing.T) { + arg := config.ActionArgument{ + Name: "", + Type: "ascii", + } + action := config.Action{Title: "Test"} + + err := typecheckActionArgument(&arg, "test", &action) + assert.NotNil(t, err) + assert.Contains(t, err.Error(), "argument name cannot be empty") +} + +func TestTypecheckActionArgumentConfirmation(t *testing.T) { + arg := config.ActionArgument{ + Name: "confirm", + Type: "confirmation", + } + action := config.Action{Title: "Test"} + + err := typecheckActionArgument(&arg, "any_value", &action) + assert.Nil(t, err, "Confirmation type should always pass validation") +} + +func TestParseCommandForReplacements(t *testing.T) { + tests := []struct { + name string + shellCommand string + values map[string]string + expectedOutput string + expectError bool + errorContains string + }{ + { + name: "Simple replacement", + shellCommand: "echo {{ name }}", + values: map[string]string{"name": "John"}, + expectedOutput: "echo John", + expectError: false, + }, + { + name: "Multiple replacements", + shellCommand: "echo {{ first }} {{ last }}", + values: map[string]string{"first": "John", "last": "Doe"}, + expectedOutput: "echo John Doe", + expectError: false, + }, + { + name: "Replacement with spaces in template", + shellCommand: "echo {{ name }}", + values: map[string]string{"name": "John"}, + expectedOutput: "echo John", + expectError: false, + }, + { + name: "Missing argument", + shellCommand: "echo {{ missing }}", + values: map[string]string{}, + expectedOutput: "", + expectError: true, + errorContains: "required arg not provided: missing", + }, + { + name: "No replacements needed", + shellCommand: "echo hello", + values: map[string]string{}, + expectedOutput: "echo hello", + expectError: false, + }, + { + name: "Multiple same argument", + shellCommand: "echo {{ name }} says hello {{ name }}", + values: map[string]string{"name": "Alice"}, + expectedOutput: "echo Alice says hello Alice", + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + output, err := parseCommandForReplacements(tt.shellCommand, tt.values) + + if tt.expectError { + assert.NotNil(t, err, "Expected error but got none") + if tt.errorContains != "" { + assert.Contains(t, err.Error(), tt.errorContains) + } + } else { + assert.Nil(t, err, "Expected no error but got: %v", err) + assert.Equal(t, tt.expectedOutput, output) + } + }) + } +} + +func TestArgumentChoicesValidation(t *testing.T) { + tests := []struct { + name string + action config.Action + values map[string]string + expectError bool + description string + }{ + { + name: "Valid choice", + action: config.Action{ + Title: "Test choices", + Shell: "echo {{ option }}", + Arguments: []config.ActionArgument{ + { + Name: "option", + Type: "ascii", + Choices: []config.ActionArgumentChoice{ + {Value: "option1", Title: "Option 1"}, + {Value: "option2", Title: "Option 2"}, + }, + }, + }, + }, + values: map[string]string{"option": "option1"}, + expectError: false, + description: "Should accept valid choice", + }, + { + name: "Invalid choice", + action: config.Action{ + Title: "Test choices", + Shell: "echo {{ option }}", + Arguments: []config.ActionArgument{ + { + Name: "option", + Type: "ascii", + Choices: []config.ActionArgumentChoice{ + {Value: "option1", Title: "Option 1"}, + {Value: "option2", Title: "Option 2"}, + }, + }, + }, + }, + values: map[string]string{"option": "invalid_option"}, + expectError: true, + description: "Should reject invalid choice", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := parseActionArguments(tt.values, &tt.action, "") + + if tt.expectError { + assert.NotNil(t, err, tt.description) + assert.Contains(t, err.Error(), "predefined choices") + } else { + assert.Nil(t, err, tt.description) + } + }) + } +} + +func TestTypeSafetyCheckVeryDangerousRawString(t *testing.T) { + // This type should allow anything without validation + tests := []string{ + "normal text", + "_zomg_ c:/ haxxor ' bobby tables && rm -rf /", + "$(rm -rf /)", + "; DROP TABLE users; --", + "../../../../etc/passwd", + "", + "unicode: 你好世界", + "emojis: 🔥💀☠️", + } + + for _, value := range tests { + t.Run(fmt.Sprintf("Value: %s", value), func(t *testing.T) { + err := TypeSafetyCheck("test", value, "very_dangerous_raw_string") + assert.Nil(t, err, "very_dangerous_raw_string should accept any value including: %s", value) + }) + } +} + +func TestParseActionArgumentsWithEntityPrefix(t *testing.T) { + action := config.Action{ + Title: "Test entity prefix", + Shell: "echo 'Processing {{ name }} for entity'", + Arguments: []config.ActionArgument{ + {Name: "name", Type: "ascii"}, + }, + } + + values := map[string]string{ + "name": "testuser", + } + + // Test with entity prefix + output, err := parseActionArguments(values, &action, "entity_123") + assert.Nil(t, err) + assert.Contains(t, output, "testuser") +} + +func TestComplexRegexPatterns(t *testing.T) { + tests := []struct { + name string + pattern string + value string + hasError bool + }{ + { + name: "Phone number pattern", + pattern: "regex:^\\+?[1-9]\\d{1,14}$", + value: "+1234567890", + hasError: false, + }, + { + name: "Invalid phone number", + pattern: "regex:^\\+?[1-9]\\d{1,14}$", + value: "123abc", + hasError: true, + }, + { + name: "Semantic version pattern", + pattern: "regex:^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)$", + value: "1.2.3", + hasError: false, + }, + { + name: "Invalid semantic version", + pattern: "regex:^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)$", + value: "1.2", + hasError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := typeSafetyCheckRegex("test", tt.value, tt.pattern) + if tt.hasError { + assert.NotNil(t, err) + } else { + assert.Nil(t, err) + } + }) + } +}