bugfix: #639 Exec support, disallow URL and similar arguments with (#671)

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

service/internal/executor/arguments.go

@@ -42,6 +42,48 @@ 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) {
+	if action == nil {
+		return nil, fmt.Errorf("action is nil")
+	}
+
+	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 +145,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 +294,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" {

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")

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,28 @@ func stepParseArgs(req *ExecutionRequest) bool {
 
 	mangleInvalidArgumentValues(req)
 
-	req.finalParsedCommand, err = parseActionArguments(req.Arguments, req.Binding.Action, req.Binding.Entity)
+	if req.Binding == nil || req.Binding.Action == nil {
+		err = fmt.Errorf("cannot parse arguments: Binding or Action is nil")
+		req.logEntry.Output = err.Error()
+		log.Warn(err.Error())
+		return false
+	}
+
+	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 +585,19 @@ 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)
+	}
+
+	if cmd == nil {
+		req.logEntry.Output = "Cannot execute: no command arguments provided"
+		log.Warn("Cannot execute: no command arguments provided")
+		return false
+	}
+
 	cmd.Stdout = streamer
 	cmd.Stderr = streamer
 	cmd.Env = buildEnv(req.Arguments)

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
 }

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:]...)
+}