feat: AST-based command unfurling for secure mode

README.md

@@ -148,7 +148,7 @@ make release            # binary + docker image
 ## Security
 
 - **Default**: Secure mode, restricted to a narrow allowlist of read-only utilities. No interpreters.
-- **Secure mode** (`use_shell_execution: false`): Executable allowlist, no shell parsing. Blocks injection. Never allowlist interpreters (bash/sh/python/git) — doing so defeats the allowlist.
+- **Secure mode** (`use_shell_execution: false`): the command is parsed into a shell AST and only a single, fully-literal simple command is accepted (no pipes, lists, substitution, redirection or globs); its executable must be on the allowlist. Interpreters (bash/sh/python) are hard-denied even if allowlisted, and per-tool policies strip escape hatches (`git -c`, `find -exec`, `tar --checkpoint-action`). This is an early-reject layer, not a sandbox.
 - **Unrestricted**: Only via `MCP_SHELL_ALLOW_UNSAFE=true`. Full access; fine for local dev, dangerous otherwise.
 - **Docker**: Runs as non-root, Alpine-based. Use it in production. Best paired with an OS sandbox (read-only FS, dropped caps) as defense-in-depth.
 

argpolicy.go

@@ -0,0 +1,111 @@
+package main
+
+import (
+	"fmt"
+	"path/filepath"
+	"strings"
+)
+
+// argPolicy decides whether a resolved argv for one executable is permitted.
+// argv[0] is the executable. Implementations are pure and stateless and exist
+// to strip per-tool escape hatches (GTFOBins) that structural validation alone
+// cannot see: an allowlisted binary may itself run arbitrary commands.
+type argPolicy interface {
+	name() string
+	check(argv []string) error
+}
+
+// policySet maps an executable basename to its governing argPolicy. Absence of
+// an entry means no extra per-argument restriction beyond structural + allowlist.
+type policySet struct {
+	byName map[string]argPolicy
+}
+
+func newPolicySet(policies ...argPolicy) *policySet {
+	byName := make(map[string]argPolicy, len(policies))
+	for _, p := range policies {
+		byName[p.name()] = p
+	}
+	return &policySet{byName: byName}
+}
+
+func newDefaultPolicySet() *policySet {
+	return newPolicySet(newGitArgPolicy(), newFindArgPolicy(), newTarArgPolicy())
+}
+
+// governs reports whether a policy exists for the executable's basename.
+func (s *policySet) governs(executable string) bool {
+	_, ok := s.byName[filepath.Base(executable)]
+	return ok
+}
+
+func (s *policySet) check(argv []string) error {
+	if len(argv) == 0 {
+		return nil
+	}
+	if pol, ok := s.byName[filepath.Base(argv[0])]; ok {
+		return pol.check(argv)
+	}
+	return nil
+}
+
+// gitArgPolicy blocks git's config-injection and alias-as-shell escape hatches.
+type gitArgPolicy struct{}
+
+func newGitArgPolicy() *gitArgPolicy { return &gitArgPolicy{} }
+
+func (*gitArgPolicy) name() string { return "git" }
+
+func (*gitArgPolicy) check(argv []string) error {
+	for _, a := range argv[1:] {
+		if a == "-c" || a == "--config-env" {
+			return fmt.Errorf("git: -c/--config-env config injection is not allowed in secure mode")
+		}
+		if strings.HasPrefix(a, "--exec-path") ||
+			strings.HasPrefix(a, "--upload-pack") ||
+			strings.HasPrefix(a, "--receive-pack") {
+			return fmt.Errorf("git: %q can execute arbitrary programs and is not allowed in secure mode", a)
+		}
+	}
+	return nil
+}
+
+// findArgPolicy blocks find's action primaries that spawn processes or mutate.
+type findArgPolicy struct{}
+
+func newFindArgPolicy() *findArgPolicy { return &findArgPolicy{} }
+
+func (*findArgPolicy) name() string { return "find" }
+
+func (*findArgPolicy) check(argv []string) error {
+	blocked := map[string]struct{}{
+		"-exec": {}, "-execdir": {}, "-ok": {}, "-okdir": {},
+		"-fprintf": {}, "-fprint": {}, "-fprint0": {}, "-delete": {},
+	}
+	for _, a := range argv[1:] {
+		if _, bad := blocked[a]; bad {
+			return fmt.Errorf("find: %q is not allowed in secure mode", a)
+		}
+	}
+	return nil
+}
+
+// tarArgPolicy blocks tar options that run an external command.
+type tarArgPolicy struct{}
+
+func newTarArgPolicy() *tarArgPolicy { return &tarArgPolicy{} }
+
+func (*tarArgPolicy) name() string { return "tar" }
+
+func (*tarArgPolicy) check(argv []string) error {
+	for _, a := range argv[1:] {
+		if a == "-I" ||
+			strings.HasPrefix(a, "--to-command") ||
+			strings.HasPrefix(a, "--checkpoint-action") ||
+			strings.HasPrefix(a, "--use-compress-program") ||
+			strings.HasPrefix(a, "--rsh-command") {
+			return fmt.Errorf("tar: %q can execute arbitrary programs and is not allowed in secure mode", a)
+		}
+	}
+	return nil
+}

argpolicy_test.go

@@ -0,0 +1,120 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestPolicySet_check(t *testing.T) {
+	tests := []struct {
+		name          string
+		argv          []string
+		expectError   bool
+		errorContains string
+	}{
+		{
+			name:          "git -c config injection blocked",
+			argv:          []string{"git", "-c", "alias.pwn=!id"},
+			expectError:   true,
+			errorContains: "config injection",
+		},
+		{
+			name:          "git -c core.pager blocked",
+			argv:          []string{"git", "-c", "core.pager=sh"},
+			expectError:   true,
+			errorContains: "config injection",
+		},
+		{
+			name:          "git --exec-path blocked",
+			argv:          []string{"git", "--exec-path=/tmp"},
+			expectError:   true,
+			errorContains: "exec-path",
+		},
+		{
+			name:        "git status allowed",
+			argv:        []string{"git", "status"},
+			expectError: false,
+		},
+		{
+			name:        "git log allowed",
+			argv:        []string{"git", "log", "--oneline"},
+			expectError: false,
+		},
+		{
+			name:          "find -exec blocked",
+			argv:          []string{"find", ".", "-exec", "rm", "{}", "+"},
+			expectError:   true,
+			errorContains: "-exec",
+		},
+		{
+			name:          "find -delete blocked",
+			argv:          []string{"find", ".", "-delete"},
+			expectError:   true,
+			errorContains: "-delete",
+		},
+		{
+			name:        "find name search allowed",
+			argv:        []string{"find", ".", "-name", "x", "-type", "f"},
+			expectError: false,
+		},
+		{
+			name:          "tar checkpoint-action blocked",
+			argv:          []string{"tar", "-cf", "/dev/null", "x", "--checkpoint-action=exec=sh"},
+			expectError:   true,
+			errorContains: "checkpoint-action",
+		},
+		{
+			name:          "tar to-command blocked",
+			argv:          []string{"tar", "--to-command=sh", "-xf", "a.tar"},
+			expectError:   true,
+			errorContains: "to-command",
+		},
+		{
+			name:        "tar list allowed",
+			argv:        []string{"tar", "-tf", "a.tar"},
+			expectError: false,
+		},
+		{
+			name:        "ungoverned executable passes",
+			argv:        []string{"ls", "-la"},
+			expectError: false,
+		},
+	}
+
+	policies := newDefaultPolicySet()
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			err := policies.check(tt.argv)
+
+			if tt.expectError {
+				require.Error(t, err)
+				if tt.errorContains != "" {
+					assert.Contains(t, err.Error(), tt.errorContains)
+				}
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func TestPolicySet_governs(t *testing.T) {
+	policies := newDefaultPolicySet()
+
+	t.Run("governed by basename", func(t *testing.T) {
+		assert.True(t, policies.governs("git"))
+		assert.True(t, policies.governs("/usr/bin/git"))
+		assert.True(t, policies.governs("find"))
+		assert.True(t, policies.governs("tar"))
+	})
+
+	t.Run("ungoverned", func(t *testing.T) {
+		assert.False(t, policies.governs("ls"))
+		assert.False(t, policies.governs("/bin/bash"))
+	})
+}

executor.go

@@ -34,14 +34,16 @@ type SecurityInfo struct {
 }
 
 type CommandExecutor struct {
-	config SecurityConfig
-	logger zerolog.Logger
+	config   SecurityConfig
+	logger   zerolog.Logger
+	unfurler *commandUnfurler
 }
 
 func newCommandExecutor(cfg SecurityConfig, logger zerolog.Logger) *CommandExecutor {
 	return &CommandExecutor{
-		config: cfg,
-		logger: logger.With().Str("component", "executor").Logger(),
+		config:   cfg,
+		logger:   logger.With().Str("component", "executor").Logger(),
+		unfurler: newCommandUnfurler(),
 	}
 }
 
@@ -97,40 +99,6 @@ func (e *CommandExecutor) execute(
 	return result, nil
 }
 
-// parseCommand securely parses a command string into executable and arguments
-// without using shell interpretation. This prevents command injection through
-// shell metacharacters and substitution.
-func (e *CommandExecutor) parseCommand(command string) (string, []string, error) {
-	command = strings.TrimSpace(command)
-	if command == "" {
-		return "", nil, fmt.Errorf("empty command")
-	}
-
-	// Simple whitespace-based splitting - no shell interpretation
-	parts := strings.Fields(command)
-	if len(parts) == 0 {
-		return "", nil, fmt.Errorf("no command found")
-	}
-
-	executable := parts[0]
-	args := parts[1:]
-
-	// Validate that the executable doesn't contain shell metacharacters
-	if containsShellMetacharacters(executable) {
-		return "", nil, fmt.Errorf("executable contains shell metacharacters: %s", executable)
-	}
-
-	// Validate arguments don't contain dangerous shell constructs
-	// In secure mode, this should be an error, not just a warning
-	for _, arg := range args {
-		if containsDangerousShellConstructs(arg) {
-			return "", nil, fmt.Errorf("argument contains dangerous shell constructs: %s", arg)
-		}
-	}
-
-	return executable, args, nil
-}
-
 func (e *CommandExecutor) executeSecureCommand(
 	ctx context.Context,
 	command string,
@@ -145,22 +113,23 @@ func (e *CommandExecutor) executeSecureCommand(
 			Msg("Using legacy shell execution mode - vulnerable to injection attacks")
 		cmd = exec.CommandContext(ctx, "bash", "-c", command)
 	} else {
-		// Secure execution: parse command and execute directly
-		executable, args, err := e.parseCommand(command)
-		if err != nil {
+		// Secure execution: parse the command into a literal argv and execute it
+		// directly, using the same unfurler the validator used.
+		res := e.unfurler.unfurl(command)
+		if !res.Allowed {
 			e.logger.Error().
-				Err(err).
 				Str("command", command).
+				Str("reason", res.Reason).
 				Msg("Failed to parse command securely")
-			return nil, fmt.Errorf("command parsing failed: %w", err)
+			return nil, fmt.Errorf("command parsing failed: %s", res.Reason)
 		}
 
 		e.logger.Debug().
-			Str("executable", executable).
-			Strs("args", args).
+			Str("executable", res.Argv[0]).
+			Strs("args", res.Argv[1:]).
 			Msg("Executing command with direct execution")
 
-		cmd = exec.CommandContext(ctx, executable, args...)
+		cmd = exec.CommandContext(ctx, res.Argv[0], res.Argv[1:]...)
 	}
 
 	if e.config.WorkingDirectory != "" {