security: GHSA-vc6p-m6vx-6cwq (HIGH) harden shellAfterCompleted execution

Route output and exitCode through OUTPUT/EXITCODE environment variables
instead of shell interpolation, and block shellAfterCompleted for webhook
actions.

Co-authored-by: Cursor <cursoragent@cursor.com>

docs/modules/ROOT/pages/action_execution/aftercompletion.adoc

@@ -13,10 +13,10 @@ actions:
     shellAfterCompleted: "apprise -c /config/apprise.yml -t 'Notification: Backup script completed' -b 'The backup script completed with code {{ exitCode}}. The log is: \n {{ output }} '"
 ----
 
-When running shellAfterCompleted, you *cannot* use argument values - they are not passed to the command. However the following special arguments are defined; 
+When running shellAfterCompleted, you *cannot* use argument values - they are not passed to the command. However the following special arguments are defined;
 
-* `{{ exitCode }}` - The exit code of the previous shell command
-* `{{ output }}` - The standard output of the previous shell command
+* `{{ exitCode }}` - The exit code of the previous shell command. OliveTin substitutes this with the `EXITCODE` environment variable when running `shellAfterCompleted`, so shell metacharacters in the value cannot break quoting.
+* `{{ output }}` - The standard output of the previous shell command. OliveTin substitutes this with the `OUTPUT` environment variable when running `shellAfterCompleted`, so shell metacharacters in command output cannot be executed. You can also reference `$OUTPUT` directly in your `shellAfterCompleted` command.
 * `{{ .Arguments.ot_executionTrackingId }}` - The unique execution tracking id for this execution (version 3k; in 2k use `{{ ot_executionTrackingId }}`)
 * `{{ .Arguments.ot_username }}` - The username of the user who started the execution (version 3k; in 2k use `{{ ot_username }}`). May be `guest` or `cron` for unauthenticated or automated runs.
 

service/internal/executor/executor.go

@@ -1251,24 +1251,56 @@ func shellAfterCompletedAction(req *ExecutionRequest) (*config.Action, bool) {
 	return req.Binding.Action, true
 }
 
+func substituteShellAfterCompletedEnvRefs(command string) string {
+	replacements := []struct{ old, new string }{
+		{"{{ output }}", `"$OUTPUT"`},
+		{"{{output}}", `"$OUTPUT"`},
+		{"{{ exitCode }}", `"$EXITCODE"`},
+		{"{{exitCode}}", `"$EXITCODE"`},
+		{"{{ exitCode}}", `"$EXITCODE"`},
+		{"{{exitCode }}", `"$EXITCODE"`},
+	}
+
+	for _, replacement := range replacements {
+		command = strings.ReplaceAll(command, replacement.old, replacement.new)
+	}
+
+	return command
+}
+
+func parseShellAfterCompletedCommand(req *ExecutionRequest, commandTemplate string, args map[string]string) (string, error) {
+	finalParsedCommand, err := tpl.ParseTemplateWithActionContext(commandTemplate, req.Binding.Entity, args)
+	if err != nil {
+		msg := "Could not prepare shellAfterCompleted command: " + err.Error() + "\n"
+		req.mutateLogEntry(func(entry *InternalLogEntry) {
+			entry.Output += msg
+		})
+		log.Warn(msg)
+		return "", err
+	}
+
+	return finalParsedCommand, nil
+}
+
+//gocyclo:ignore
 func buildShellAfterCommand(ctx context.Context, req *ExecutionRequest, stdout, stderr *bytes.Buffer) (*exec.Cmd, map[string]string, error) {
 	action, ok := shellAfterCompletedAction(req)
 	if !ok {
 		return nil, nil, nil
 	}
 
+	if hasWebhookTag(req) {
+		return nil, nil, fmt.Errorf("webhooks cannot use shellAfterCompleted; use exec without after-completion shell instead. See https://docs.olivetin.app/action_execution/shellvsexec.html")
+	}
+
 	args, err := buildShellAfterArgs(req)
 	if err != nil {
 		return nil, nil, err
 	}
 
-	finalParsedCommand, err := tpl.ParseTemplateWithActionContext(action.ShellAfterCompleted, req.Binding.Entity, args)
+	commandTemplate := substituteShellAfterCompletedEnvRefs(action.ShellAfterCompleted)
+	finalParsedCommand, err := parseShellAfterCompletedCommand(req, commandTemplate, args)
 	if err != nil {
-		msg := "Could not prepare shellAfterCompleted command: " + err.Error() + "\n"
-		req.mutateLogEntry(func(entry *InternalLogEntry) {
-			entry.Output += msg
-		})
-		log.Warn(msg)
 		return nil, nil, err
 	}
 

service/internal/executor/executor_test.go

@@ -1,6 +1,8 @@
 package executor
 
 import (
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 	"time"
@@ -385,6 +387,64 @@ func TestWebhookAllowsExecExecution(t *testing.T) {
 	assert.Contains(t, req.logEntry.Output, "hello")
 }
 
+func TestWebhookRejectsShellAfterCompleted(t *testing.T) {
+	cfg := config.DefaultConfig()
+	e := DefaultExecutor(cfg)
+	a1 := &config.Action{
+		Title:               "Webhook After Shell Reject",
+		Exec:                []string{"echo", "{{ msg }}"},
+		ShellAfterCompleted: "echo after",
+		Arguments: []config.ActionArgument{
+			{Name: "msg", Type: "ascii"},
+		},
+	}
+	cfg.Actions = append(cfg.Actions, a1)
+	cfg.Sanitize()
+	e.RebuildActionMap()
+
+	req := ExecutionRequest{
+		Tags:              []string{"webhook"},
+		AuthenticatedUser: auth.UserFromSystem(cfg, "webhook"),
+		Cfg:               cfg,
+		Arguments:         map[string]string{"msg": "hello"},
+		Binding:           e.FindBindingWithNoEntity(a1),
+	}
+
+	wg, _ := e.ExecRequest(&req)
+	wg.Wait()
+
+	assert.NotNil(t, req.logEntry)
+	assert.Contains(t, req.logEntry.Output, "webhooks cannot use shellAfterCompleted")
+}
+
+func TestShellAfterCompletedUsesOutputEnvSafely(t *testing.T) {
+	cfg := config.DefaultConfig()
+	e := DefaultExecutor(cfg)
+	injectedPath := filepath.Join(t.TempDir(), "olivetin-injected")
+	a1 := &config.Action{
+		Title:               "After completion escape",
+		Shell:               "printf %s \"'; touch " + injectedPath + "; echo '\"",
+		ShellAfterCompleted: "printf %s '{{ output }}'",
+	}
+	cfg.Actions = append(cfg.Actions, a1)
+	cfg.Sanitize()
+	e.RebuildActionMap()
+
+	req := ExecutionRequest{
+		AuthenticatedUser: auth.UserFromSystem(cfg, "cron"),
+		Cfg:               cfg,
+		Binding:           e.FindBindingWithNoEntity(a1),
+	}
+
+	wg, _ := e.ExecRequest(&req)
+	wg.Wait()
+
+	assert.NotNil(t, req.logEntry)
+	assert.Equal(t, int32(0), req.logEntry.ExitCode)
+	_, err := os.Stat(injectedPath)
+	assert.True(t, os.IsNotExist(err), "shellAfterCompleted must not execute injected commands from output")
+}
+
 func TestFilterToDefinedArgumentsOnly(t *testing.T) {
 	req := newExecRequest()
 	req.Binding.Action = &config.Action{