Merge commit from fork

security: GHSA-prj9-97mp-mwh2 (HIGH) Treat all ot_ system arguments as reserved, preventing RCE

docs/modules/ROOT/pages/args/types.adoc

@@ -10,6 +10,7 @@ A full list of argument types are below;
 | (default)                   | xref:args/input.adoc[Textbox]           | If a `type:` is not set, and `choices:` is empty, then ascii will be used, and a warning will be logged. It is recommended that you set the type explicitly, rather than relying on defaults.
 | ascii                       | xref:args/input.adoc[Textbox]           | a-z (case insensitive), 0-9, but no spaces or punctuation
 | ascii_identifier            | xref:args/input.adoc[Textbox]           | Like a DNS name, a-Z (case insensitive), 0-9, `-`, `.`, and `_`. 
+| shell_safe_identifier       | xref:args/input.adoc[Textbox]           | Like an ascii identifier, but also allows `@` and `+`. Useful for shell-safe usernames and email-style identifiers.
 | ascii_sentence              | xref:args/input.adoc[Textbox]           | a-z (case insensitive), 0-9, with spaces, `.` and `,`. 
 | unicode_identifier          | xref:args/input.adoc[Textbox]           | Like an ascii identifier, but allows unicode characters. This is useful for languages that use non-ascii characters, such as Chinese, Japanese, etc.
 | email                       | xref:args/input.adoc[Textbox]           | An email address.

frontend/resources/vue/views/ArgumentForm.vue

@@ -174,7 +174,7 @@ function getInputType(arg) {
     return 'checkbox'
   }
 
-  if (arg.type === 'ascii_identifier' || arg.type === 'ascii' || arg.type === 'ascii_sentence') {
+  if (arg.type === 'ascii_identifier' || arg.type === 'shell_safe_identifier' || arg.type === 'ascii' || arg.type === 'ascii_sentence') {
     return 'text'
   }
 

service/internal/config/config.go

@@ -4,6 +4,9 @@ import (
 	"fmt"
 )
 
+// ReservedArgumentNamePrefix is reserved for OliveTin-injected system arguments.
+const ReservedArgumentNamePrefix = "ot_"
+
 // Action represents the core functionality of OliveTin - commands that show up
 // as buttons in the UI.
 type Action struct {

service/internal/config/sanitize.go

@@ -26,6 +26,34 @@ func (cfg *Config) Sanitize() {
 	}
 
 	cfg.sanitizeDashboardsForInlineActions()
+
+	if err := cfg.validateReservedActionArgumentNames(); err != nil {
+		log.Fatalf("%v", err)
+	}
+}
+
+func (cfg *Config) validateReservedActionArgumentNames() error {
+	for _, action := range cfg.Actions {
+		if err := action.validateReservedArgumentNames(); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func (action *Action) validateReservedArgumentNames() error {
+	if action == nil {
+		return nil
+	}
+
+	for _, arg := range action.Arguments {
+		if strings.HasPrefix(arg.Name, ReservedArgumentNamePrefix) {
+			return fmt.Errorf("action %q argument %q uses reserved prefix %q", action.Title, arg.Name, ReservedArgumentNamePrefix)
+		}
+	}
+
+	return nil
 }
 
 func (cfg *Config) sanitizeDashboardsForInlineActions() {

service/internal/config/sanitize_test.go

@@ -92,6 +92,59 @@ func TestSanitizeConfigInlineDashboardActions(t *testing.T) {
 	}
 }
 
+func TestValidateReservedActionArgumentNames(t *testing.T) {
+	c := DefaultConfig()
+	c.Actions = append(c.Actions, &Action{
+		Title: "Reserved arg",
+		Arguments: []ActionArgument{
+			{Name: "ot_custom", Type: "ascii"},
+		},
+	})
+
+	err := c.validateReservedActionArgumentNames()
+
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), `action "Reserved arg" argument "ot_custom" uses reserved prefix "ot_"`)
+}
+
+func TestValidateReservedActionArgumentNamesAllowsNonReserved(t *testing.T) {
+	c := DefaultConfig()
+	c.Actions = append(c.Actions, &Action{
+		Title: "Allowed arg",
+		Arguments: []ActionArgument{
+			{Name: "target", Type: "ascii"},
+		},
+	})
+
+	require.NoError(t, c.validateReservedActionArgumentNames())
+}
+
+func TestValidateReservedActionArgumentNamesChecksInlineActions(t *testing.T) {
+	c := DefaultConfig()
+	c.Dashboards = []*DashboardComponent{
+		{
+			Title: "Dashboard",
+			Contents: []*DashboardComponent{
+				{
+					Title: "Inline reserved arg",
+					InlineAction: &Action{
+						Shell: "echo test",
+						Arguments: []ActionArgument{
+							{Name: "ot_custom", Type: "ascii"},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	c.sanitizeDashboardsForInlineActions()
+	err := c.validateReservedActionArgumentNames()
+
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), `action "Inline reserved arg" argument "ot_custom" uses reserved prefix "ot_"`)
+}
+
 func TestValidateUniqueLocalUserAPIKeys(t *testing.T) {
 	t.Parallel()
 

service/internal/executor/arguments.go

@@ -21,6 +21,7 @@ var (
 		"unicode_identifier":        `^[\w\-\.\_\d]+$`,
 		"ascii":                     `^[a-zA-Z0-9]+$`,
 		"ascii_identifier":          `^[a-zA-Z0-9\-\._]+$`,
+		"shell_safe_identifier":     `^[a-zA-Z0-9@\.\_\+\-]+$`,
 		"ascii_sentence":            `^[a-zA-Z0-9\-\._, ]+$`,
 	}
 )

service/internal/executor/arguments_test.go

@@ -576,6 +576,38 @@ func TestTypeSafetyCheckAsciiIdentifier(t *testing.T) {
 	}
 }
 
+func TestTypeSafetyCheckShellSafeIdentifier(t *testing.T) {
+	tests := []struct {
+		name     string
+		value    string
+		hasError bool
+	}{
+		{"Simple username", "alice123", false},
+		{"Email username", "alice@example.com", false},
+		{"Plus addressing", "alice+test@example.com", false},
+		{"Hyphen underscore dot", "alice-test_user.example", false},
+		{"Invalid space", "alice example", true},
+		{"Invalid shell substitution", "$(whoami)", true},
+		{"Invalid backtick", "`whoami`", true},
+		{"Invalid semicolon", "alice;id", true},
+		{"Invalid ampersand", "alice&id", true},
+		{"Invalid pipe", "alice|id", true},
+		{"Invalid quote", "alice'example", true},
+		{"Invalid slash", "alice/example", true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := TypeSafetyCheck("username", tt.value, "shell_safe_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

service/internal/executor/executor.go

@@ -666,13 +666,15 @@ func stepACLCheck(req *ExecutionRequest) bool {
 
 func stepParseArgs(req *ExecutionRequest) bool {
 	ensureArgumentMap(req)
-	injectSystemArgs(req)
 
 	if !hasBindingAndAction(req) {
 		return fail(req, fmt.Errorf("cannot parse arguments: Binding or Action is nil"))
 	}
 
 	filterToDefinedArgumentsOnly(req)
+	if err := injectSystemArgs(req); err != nil {
+		return fail(req, err)
+	}
 	mangleInvalidArgumentValues(req)
 
 	if hasExec(req) {
@@ -735,7 +737,7 @@ func filterToDefinedArgumentsOnly(req *ExecutionRequest) {
 
 func keepArgument(name string, definedNames map[string]struct{}) bool {
 	_, ok := definedNames[name]
-	return ok || strings.HasPrefix(name, "ot_")
+	return ok
 }
 
 func hasWebhookTag(req *ExecutionRequest) bool {
@@ -747,9 +749,38 @@ func hasWebhookTag(req *ExecutionRequest) bool {
 	return false
 }
 
-func injectSystemArgs(req *ExecutionRequest) {
-	req.Arguments["ot_executionTrackingId"] = req.TrackingID
-	req.Arguments["ot_username"] = req.AuthenticatedUser.Username
+var systemArgumentDefinitions = []config.ActionArgument{
+	{Name: "ot_executionTrackingId", Type: "ascii_identifier", RejectNull: true},
+	{Name: "ot_username", Type: "shell_safe_identifier", RejectNull: true},
+}
+
+func injectSystemArgs(req *ExecutionRequest) error {
+	args, err := validatedSystemArgs(req)
+	if err != nil {
+		return err
+	}
+
+	for name, value := range args {
+		req.Arguments[name] = value
+	}
+
+	return nil
+}
+
+func validatedSystemArgs(req *ExecutionRequest) (map[string]string, error) {
+	values := map[string]string{
+		"ot_executionTrackingId": req.TrackingID,
+		"ot_username":            req.AuthenticatedUser.Username,
+	}
+
+	for i := range systemArgumentDefinitions {
+		arg := &systemArgumentDefinitions[i]
+		if err := ValidateArgument(arg, values[arg.Name], req.Binding.Action); err != nil {
+			return nil, fmt.Errorf("system argument %q failed validation: %w", arg.Name, err)
+		}
+	}
+
+	return values, nil
 }
 
 func hasBindingAndAction(req *ExecutionRequest) bool {
@@ -939,36 +970,20 @@ func prepareCommand(cmd *exec.Cmd, streamer *OutputStreamer, req *ExecutionReque
 }
 
 func stepExecAfter(req *ExecutionRequest) bool {
-	if req.Binding.Action.ShellAfterCompleted == "" {
-		return true
-	}
-
 	ctx, cancel := newTimeoutContext(context.Background(), time.Duration(req.Binding.Action.Timeout)*time.Second, req.executor)
 	defer cancel()
 
 	var stdout bytes.Buffer
 	var stderr bytes.Buffer
 
-	args := map[string]string{
-		"output":                 req.logEntry.Output,
-		"exitCode":               fmt.Sprintf("%v", req.logEntry.ExitCode),
-		"ot_executionTrackingId": req.TrackingID,
-		"ot_username":            req.AuthenticatedUser.Username,
-	}
-
-	finalParsedCommand, err := tpl.ParseTemplateWithActionContext(req.Binding.Action.ShellAfterCompleted, req.Binding.Entity, args)
-
+	cmd, args, err := buildShellAfterCommand(ctx, req, &stdout, &stderr)
 	if err != nil {
-		msg := "Could not prepare shellAfterCompleted command: " + err.Error() + "\n"
-		req.logEntry.Output += msg
-		log.Warn(msg)
+		return fail(req, err)
+	}
+	if cmd == nil {
 		return true
 	}
 
-	cmd := wrapCommandInShell(ctx, finalParsedCommand)
-	cmd.Stdout = &stdout
-	cmd.Stderr = &stderr
-
 	cmd.Env = buildEnv(args)
 
 	runerr := cmd.Start()
@@ -998,6 +1013,43 @@ func stepExecAfter(req *ExecutionRequest) bool {
 	return true
 }
 
+func buildShellAfterCommand(ctx context.Context, req *ExecutionRequest, stdout, stderr *bytes.Buffer) (*exec.Cmd, map[string]string, error) {
+	if req.Binding.Action.ShellAfterCompleted == "" {
+		return nil, nil, nil
+	}
+
+	args, err := buildShellAfterArgs(req)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	finalParsedCommand, err := tpl.ParseTemplateWithActionContext(req.Binding.Action.ShellAfterCompleted, req.Binding.Entity, args)
+	if err != nil {
+		msg := "Could not prepare shellAfterCompleted command: " + err.Error() + "\n"
+		req.logEntry.Output += msg
+		log.Warn(msg)
+		return nil, nil, nil
+	}
+
+	cmd := wrapCommandInShell(ctx, finalParsedCommand)
+	cmd.Stdout = stdout
+	cmd.Stderr = stderr
+
+	return cmd, args, nil
+}
+
+func buildShellAfterArgs(req *ExecutionRequest) (map[string]string, error) {
+	args, err := validatedSystemArgs(req)
+	if err != nil {
+		return nil, err
+	}
+
+	args["output"] = req.logEntry.Output
+	args["exitCode"] = fmt.Sprintf("%v", req.logEntry.ExitCode)
+
+	return args, nil
+}
+
 //gocyclo:ignore
 func stepTrigger(req *ExecutionRequest) bool {
 	if req.Binding.Action.Triggers == nil {

service/internal/executor/executor_test.go

@@ -1,6 +1,7 @@
 package executor
 
 import (
+	"strings"
 	"testing"
 	"time"
 
@@ -37,7 +38,7 @@ func TestCreateExecutorAndExec(t *testing.T) {
 	e, cfg := testingExecutor()
 
 	req := ExecutionRequest{
-		AuthenticatedUser: &authpublic.AuthenticatedUser{Username: "Mr Tickle"},
+		AuthenticatedUser: &authpublic.AuthenticatedUser{Username: "MrTickle"},
 		Cfg:               cfg,
 		Arguments: map[string]string{
 			"person": "yourself",
@@ -379,7 +380,7 @@ func TestFilterToDefinedArgumentsOnly(t *testing.T) {
 	assert.Empty(t, req.Arguments["extra_undefined"])
 }
 
-func TestFilterToDefinedArgumentsPreservesSystemArgs(t *testing.T) {
+func TestFilterToDefinedArgumentsDropsReservedPrefixArgs(t *testing.T) {
 	req := newExecRequest()
 	req.Binding.Action = &config.Action{
 		Title:     "Filter test",
@@ -393,8 +394,209 @@ func TestFilterToDefinedArgumentsPreservesSystemArgs(t *testing.T) {
 
 	filterToDefinedArgumentsOnly(req)
 
-	assert.Equal(t, "track-123", req.Arguments["ot_executionTrackingId"])
-	assert.Equal(t, "webhook", req.Arguments["ot_username"])
+	assert.Empty(t, req.Arguments["ot_executionTrackingId"])
+	assert.Empty(t, req.Arguments["ot_username"])
+}
+
+func TestStepParseArgsInjectsSystemArgsAfterFiltering(t *testing.T) {
+	req := newExecRequest()
+	req.TrackingID = "server-track-456"
+	req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice"}
+	req.Binding.Action = &config.Action{
+		Title: "Filter then inject",
+		Shell: "echo test",
+		Arguments: []config.ActionArgument{
+			{Name: "name", Type: "ascii"},
+		},
+	}
+	req.Arguments = map[string]string{
+		"name":                   "Alice",
+		"ot_executionTrackingId": "attacker-track",
+		"ot_username":            "mallory",
+		"ot_custom":              "polluted",
+	}
+
+	assert.True(t, stepParseArgs(req))
+	assert.Equal(t, "Alice", req.Arguments["name"])
+	assert.Equal(t, "server-track-456", req.Arguments["ot_executionTrackingId"])
+	assert.Equal(t, "alice", req.Arguments["ot_username"])
+	assert.Empty(t, req.Arguments["ot_custom"])
+}
+
+func TestStepParseArgsDropsReservedPrefixArgsFromEnvironment(t *testing.T) {
+	req := newExecRequest()
+	req.TrackingID = "server-track-456"
+	req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice@example.com"}
+	req.Binding.Action = &config.Action{
+		Title:     "No reserved prefix pollution",
+		Shell:     "echo test",
+		Arguments: []config.ActionArgument{},
+	}
+	req.Arguments = map[string]string{
+		"ot_custom": "polluted",
+	}
+
+	assert.True(t, stepParseArgs(req))
+	env := buildEnv(req.Arguments)
+
+	assert.False(t, containsEnvPrefix(env, "OT_CUSTOM="))
+	assert.True(t, containsEnvPrefix(env, "OT_USERNAME=alice@example.com"))
+	assert.True(t, containsEnvPrefix(env, "OT_EXECUTIONTRACKINGID=server-track-456"))
+}
+
+func TestSystemArgumentDefinitionsAreReservedAndShellSafe(t *testing.T) {
+	unsafeTypes := map[string]struct{}{
+		"email":                     {},
+		"password":                  {},
+		"raw_string_multiline":      {},
+		"url":                       {},
+		"very_dangerous_raw_string": {},
+	}
+	seen := map[string]struct{}{}
+
+	for _, arg := range systemArgumentDefinitions {
+		assert.True(t, strings.HasPrefix(arg.Name, config.ReservedArgumentNamePrefix))
+		assert.NotEmpty(t, arg.Type)
+		assert.True(t, arg.RejectNull)
+
+		_, duplicate := seen[arg.Name]
+		assert.False(t, duplicate, "duplicate system argument definition %q", arg.Name)
+		seen[arg.Name] = struct{}{}
+
+		_, unsafe := unsafeTypes[arg.Type]
+		assert.False(t, unsafe, "system argument %q uses unsafe type %q", arg.Name, arg.Type)
+	}
+}
+
+func TestValidatedSystemArgsMatchesSystemArgumentDefinitions(t *testing.T) {
+	req := newExecRequest()
+	req.TrackingID = "server-track-456"
+	req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice@example.com"}
+
+	args, err := validatedSystemArgs(req)
+
+	assert.Nil(t, err)
+	assert.Len(t, args, len(systemArgumentDefinitions))
+	for _, arg := range systemArgumentDefinitions {
+		assert.Contains(t, args, arg.Name)
+	}
+}
+
+func TestBuildShellAfterArgsOnlyAddsExpectedNonSystemArgs(t *testing.T) {
+	req := newExecRequest()
+	req.logEntry = &InternalLogEntry{
+		Output:   "hello",
+		ExitCode: 7,
+	}
+	req.TrackingID = "server-track-456"
+	req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice@example.com"}
+	req.Binding.Action = &config.Action{ShellAfterCompleted: "echo test"}
+
+	args, err := buildShellAfterArgs(req)
+
+	assert.Nil(t, err)
+	assert.Len(t, args, len(systemArgumentDefinitions)+2)
+	assert.Contains(t, args, "output")
+	assert.Contains(t, args, "exitCode")
+	for _, arg := range systemArgumentDefinitions {
+		assert.Contains(t, args, arg.Name)
+	}
+}
+
+func TestStepParseArgsAllowsEmailUsernameSystemArg(t *testing.T) {
+	req := newExecRequest()
+	req.logEntry = &InternalLogEntry{}
+	req.TrackingID = "server-track-456"
+	req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice@example.com"}
+	req.Binding.Action = &config.Action{
+		Title:     "Email username",
+		Shell:     "echo test",
+		Arguments: []config.ActionArgument{},
+	}
+
+	assert.True(t, stepParseArgs(req))
+	assert.Equal(t, "alice@example.com", req.Arguments["ot_username"])
+}
+
+func TestStepParseArgsFailsWhenUsernameSystemArgIsInvalid(t *testing.T) {
+	req := newExecRequest()
+	req.logEntry = &InternalLogEntry{}
+	req.TrackingID = "server-track-456"
+	req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice;id"}
+	req.Binding.Action = &config.Action{
+		Title:     "Invalid system arg",
+		Shell:     "echo test",
+		Arguments: []config.ActionArgument{},
+	}
+
+	assert.False(t, stepParseArgs(req))
+	assert.Contains(t, req.logEntry.Output, `system argument "ot_username" failed validation`)
+	assert.Empty(t, req.Arguments["ot_username"])
+}
+
+func TestStepParseArgsFailsWhenTrackingIDSystemArgIsInvalid(t *testing.T) {
+	req := newExecRequest()
+	req.logEntry = &InternalLogEntry{}
+	req.TrackingID = "track/../../bad"
+	req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice"}
+	req.Binding.Action = &config.Action{
+		Title:     "Invalid tracking ID",
+		Shell:     "echo test",
+		Arguments: []config.ActionArgument{},
+	}
+
+	assert.False(t, stepParseArgs(req))
+	assert.Contains(t, req.logEntry.Output, `system argument "ot_executionTrackingId" failed validation`)
+	assert.Empty(t, req.Arguments["ot_executionTrackingId"])
+}
+
+func TestBuildShellAfterArgsUsesValidatedSystemArgs(t *testing.T) {
+	req := newExecRequest()
+	req.logEntry = &InternalLogEntry{
+		Output:   "hello",
+		ExitCode: 7,
+	}
+	req.TrackingID = "server-track-456"
+	req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice@example.com"}
+	req.Binding.Action = &config.Action{
+		Title:               "Shell after",
+		ShellAfterCompleted: "echo test",
+	}
+
+	args, err := buildShellAfterArgs(req)
+
+	assert.Nil(t, err)
+	assert.Equal(t, "alice@example.com", args["ot_username"])
+	assert.Equal(t, "server-track-456", args["ot_executionTrackingId"])
+	assert.Equal(t, "hello", args["output"])
+	assert.Equal(t, "7", args["exitCode"])
+}
+
+func TestBuildShellAfterArgsFailsWhenSystemArgIsInvalid(t *testing.T) {
+	req := newExecRequest()
+	req.logEntry = &InternalLogEntry{}
+	req.TrackingID = "server-track-456"
+	req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice;id"}
+	req.Binding.Action = &config.Action{
+		Title:               "Shell after invalid username",
+		ShellAfterCompleted: "echo test",
+	}
+
+	args, err := buildShellAfterArgs(req)
+
+	assert.Nil(t, args)
+	assert.NotNil(t, err)
+	assert.Contains(t, err.Error(), `system argument "ot_username" failed validation`)
+}
+
+func containsEnvPrefix(env []string, prefix string) bool {
+	for _, item := range env {
+		if strings.HasPrefix(item, prefix) {
+			return true
+		}
+	}
+
+	return false
 }
 
 func TestTriggerExecutesTriggeredAction(t *testing.T) {