From c917d1b1e7a2a90acec79760cb1ff18ffae82409 Mon Sep 17 00:00:00 2001 From: jamesread Date: Sun, 26 Oct 2025 16:40:44 +0000 Subject: [PATCH] bugfix: #639 Exec support, disallow URL and similar arguments with --- service/internal/config/config.go | 1 + service/internal/executor/arguments.go | 65 +++++++++++ service/internal/executor/arguments_test.go | 104 ++++++++++++++++++ service/internal/executor/executor.go | 27 ++++- service/internal/executor/executor_unix.go | 14 ++- service/internal/executor/executor_windows.go | 8 ++ 6 files changed, 216 insertions(+), 3 deletions(-) diff --git a/service/internal/config/config.go b/service/internal/config/config.go index 3c27b14..113d64a 100644 --- a/service/internal/config/config.go +++ b/service/internal/config/config.go @@ -11,6 +11,7 @@ type Action struct { Title string Icon string Shell string + Exec []string ShellAfterCompleted string Timeout int Acls []string diff --git a/service/internal/executor/arguments.go b/service/internal/executor/arguments.go index 41db80f..e563bd9 100644 --- a/service/internal/executor/arguments.go +++ b/service/internal/executor/arguments.go @@ -42,6 +42,44 @@ func parseCommandForReplacements(shellCommand string, values map[string]string, return shellCommand, nil } +func parseActionExec(values map[string]string, action *config.Action, entity *entities.Entity) ([]string, error) { + for _, arg := range action.Arguments { + argName := arg.Name + argValue := values[argName] + + err := typecheckActionArgument(&arg, argValue, action) + + if err != nil { + return nil, err + } + + log.WithFields(log.Fields{ + "name": argName, + "value": argValue, + }).Debugf("Arg assigned") + } + + parsedArgs := make([]string, len(action.Exec)) + for i, arg := range action.Exec { + parsedArg, err := parseCommandForReplacements(arg, values, entity) + if err != nil { + return nil, err + } + + parsedArg = entities.ParseTemplateWithArgs(parsedArg, entity, values) + parsedArgs[i] = parsedArg + } + + redactedArgs := redactExecArgs(parsedArgs, action.Arguments, values) + + log.WithFields(log.Fields{ + "actionTitle": action.Title, + "cmd": redactedArgs, + }).Infof("Action parse args - After (Exec)") + + return parsedArgs, nil +} + func parseActionArguments(values map[string]string, action *config.Action, entity *entities.Entity) (string, error) { log.WithFields(log.Fields{ "actionTitle": action.Title, @@ -103,6 +141,15 @@ func redactShellCommand(shellCommand string, arguments []config.ActionArgument, return shellCommand } +//gocyclo:ignore +func redactExecArgs(execArgs []string, arguments []config.ActionArgument, argumentValues map[string]string) []string { + redacted := make([]string, len(execArgs)) + for i, arg := range execArgs { + redacted[i] = redactShellCommand(arg, arguments, argumentValues) + } + return redacted +} + func typecheckActionArgument(arg *config.ActionArgument, value string, action *config.Action) error { if arg.Type == "confirmation" { return nil @@ -243,6 +290,24 @@ func typeSafetyCheckUrl(value string) error { return err } +func checkShellArgumentSafety(action *config.Action) error { + if action.Shell == "" { + return nil + } + + unsafeTypes := []string{"url", "email", "raw_string_multiline", "very_dangerous_raw_string"} + + for _, arg := range action.Arguments { + for _, unsafeType := range unsafeTypes { + if arg.Type == unsafeType { + 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 +} + func mangleInvalidArgumentValues(req *ExecutionRequest) { for _, arg := range req.Binding.Action.Arguments { if arg.Type == "datetime" { diff --git a/service/internal/executor/arguments_test.go b/service/internal/executor/arguments_test.go index c64258e..3ddf1cd 100644 --- a/service/internal/executor/arguments_test.go +++ b/service/internal/executor/arguments_test.go @@ -92,6 +92,110 @@ func TestArgumentNotProvided(t *testing.T) { assert.Equal(t, err.Error(), "required arg not provided: personName") } +func TestExecArrayParsing(t *testing.T) { + a1 := config.Action{ + Title: "List files", + Exec: []string{"ls", "-alh"}, + Arguments: []config.ActionArgument{}, + } + + values := map[string]string{} + + out, err := parseActionExec(values, &a1, nil) + + assert.Nil(t, err) + assert.Equal(t, []string{"ls", "-alh"}, out) +} + +func TestExecArrayWithTemplateReplacement(t *testing.T) { + a1 := config.Action{ + Title: "List specific path", + Exec: []string{"ls", "-alh", "{{path}}"}, + Arguments: []config.ActionArgument{ + { + Name: "path", + Type: "ascii_identifier", + }, + }, + } + + values := map[string]string{ + "path": "tmp", + } + + out, err := parseActionExec(values, &a1, nil) + + assert.Nil(t, err) + assert.Equal(t, []string{"ls", "-alh", "tmp"}, out) +} + +func TestCheckShellArgumentSafetyWithURL(t *testing.T) { + a1 := config.Action{ + Title: "Download file", + Shell: "curl {{url}}", + Arguments: []config.ActionArgument{ + { + Name: "url", + Type: "url", + }, + }, + } + + err := checkShellArgumentSafety(&a1) + assert.NotNil(t, err) + assert.Contains(t, err.Error(), "unsafe argument type 'url' cannot be used with Shell execution") + assert.Contains(t, err.Error(), "https://docs.olivetin.app/action_execution/shellvsexec.html") +} + +func TestCheckShellArgumentSafetyWithEmail(t *testing.T) { + a1 := config.Action{ + Title: "Send email", + Shell: "sendmail {{email}}", + Arguments: []config.ActionArgument{ + { + Name: "email", + Type: "email", + }, + }, + } + + err := checkShellArgumentSafety(&a1) + assert.NotNil(t, err) + assert.Contains(t, err.Error(), "unsafe argument type 'email' cannot be used with Shell execution") +} + +func TestCheckShellArgumentSafetyWithExec(t *testing.T) { + a1 := config.Action{ + Title: "Download file", + Exec: []string{"curl", "{{url}}"}, + Arguments: []config.ActionArgument{ + { + Name: "url", + Type: "url", + }, + }, + } + + err := checkShellArgumentSafety(&a1) + assert.Nil(t, err) +} + +func TestCheckShellArgumentSafetyWithSafeTypes(t *testing.T) { + a1 := config.Action{ + Title: "List files", + Shell: "ls {{path}}", + Arguments: []config.ActionArgument{ + { + Name: "path", + Type: "ascii_identifier", + }, + }, + } + + err := checkShellArgumentSafety(&a1) + assert.Nil(t, err) +} + 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") diff --git a/service/internal/executor/executor.go b/service/internal/executor/executor.go index ba4939a..d7254c7 100644 --- a/service/internal/executor/executor.go +++ b/service/internal/executor/executor.go @@ -15,6 +15,7 @@ import ( "context" "fmt" "os" + "os/exec" "path" "strings" "sync" @@ -73,6 +74,8 @@ type ExecutionRequest struct { logEntry *InternalLogEntry finalParsedCommand string + execArgs []string + useDirectExec bool executor *Executor } @@ -432,7 +435,21 @@ func stepParseArgs(req *ExecutionRequest) bool { mangleInvalidArgumentValues(req) - req.finalParsedCommand, err = parseActionArguments(req.Arguments, req.Binding.Action, req.Binding.Entity) + if len(req.Binding.Action.Exec) > 0 { + req.useDirectExec = true + req.execArgs, err = parseActionExec(req.Arguments, req.Binding.Action, req.Binding.Entity) + } else { + req.useDirectExec = false + + err = checkShellArgumentSafety(req.Binding.Action) + if err != nil { + req.logEntry.Output = err.Error() + log.Warn(err.Error()) + return false + } + + req.finalParsedCommand, err = parseActionArguments(req.Arguments, req.Binding.Action, req.Binding.Entity) + } if err != nil { req.logEntry.Output = err.Error() @@ -561,7 +578,13 @@ func stepExec(req *ExecutionRequest) bool { streamer := &OutputStreamer{Req: req} - cmd := wrapCommandInShell(ctx, req.finalParsedCommand) + var cmd *exec.Cmd + if req.useDirectExec { + cmd = wrapCommandDirect(ctx, req.execArgs) + } else { + cmd = wrapCommandInShell(ctx, req.finalParsedCommand) + } + cmd.Stdout = streamer cmd.Stderr = streamer cmd.Env = buildEnv(req.Arguments) diff --git a/service/internal/executor/executor_unix.go b/service/internal/executor/executor_unix.go index 54ef686..1a54685 100644 --- a/service/internal/executor/executor_unix.go +++ b/service/internal/executor/executor_unix.go @@ -21,5 +21,17 @@ func wrapCommandInShell(ctx context.Context, finalParsedCommand string) *exec.Cm cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} return cmd - +} + +func wrapCommandDirect(ctx context.Context, execArgs []string) *exec.Cmd { + if len(execArgs) == 0 { + return nil + } + + cmd := exec.CommandContext(ctx, execArgs[0], execArgs[1:]...) + + // This is to ensure that the process group is killed when the parent process is killed. + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + + return cmd } diff --git a/service/internal/executor/executor_windows.go b/service/internal/executor/executor_windows.go index 511a7e4..fcb115b 100644 --- a/service/internal/executor/executor_windows.go +++ b/service/internal/executor/executor_windows.go @@ -22,3 +22,11 @@ func wrapCommandInShell(ctx context.Context, finalParsedCommand string) *exec.Cm return exec.CommandContext(ctx, "cmd", "/u", "/C", finalParsedCommand) } } + +func wrapCommandDirect(ctx context.Context, execArgs []string) *exec.Cmd { + if len(execArgs) == 0 { + return nil + } + + return exec.CommandContext(ctx, execArgs[0], execArgs[1:]...) +}