Merge pull request #96 from 73ai/sandboxing-bash

feat(tools): three-tier tool safety model

Author: priyanshujain

agent/tools/approval_rules.go

@@ -124,6 +124,20 @@ func extractPattern(toolName string, input json.RawMessage) string {
 		return ""
 	}
 	switch toolName {
+	case "bash":
+		if cmd, ok := m["command"]; ok {
+			var s string
+			if json.Unmarshal(cmd, &s) == nil {
+				return firstToken(s)
+			}
+		}
+	case "file_write", "file_edit":
+		if p, ok := m["path"]; ok {
+			var s string
+			if json.Unmarshal(p, &s) == nil {
+				return s
+			}
+		}
 	case "slack_send", "slack_read_channel":
 		if ch, ok := m["channel"]; ok {
 			var s string

agent/tools/approval_rules_test.go

@@ -146,3 +146,59 @@ func TestExtractPattern_GWSMissingCommand(t *testing.T) {
 		t.Errorf("pattern = %q, want gws_execute (fallback)", p)
 	}
 }
+
+func TestExtractPattern_BashCommand(t *testing.T) {
+	input, _ := json.Marshal(map[string]string{"command": "curl example.com"})
+	if p := extractPattern("bash", input); p != "curl" {
+		t.Errorf("pattern = %q, want curl", p)
+	}
+}
+
+func TestExtractPattern_BashSingleWord(t *testing.T) {
+	input, _ := json.Marshal(map[string]string{"command": "ls"})
+	if p := extractPattern("bash", input); p != "ls" {
+		t.Errorf("pattern = %q, want ls", p)
+	}
+}
+
+func TestExtractPattern_FileWrite(t *testing.T) {
+	input, _ := json.Marshal(map[string]string{"path": "/tmp/test.txt", "content": "hello"})
+	if p := extractPattern("file_write", input); p != "/tmp/test.txt" {
+		t.Errorf("pattern = %q, want /tmp/test.txt", p)
+	}
+}
+
+func TestExtractPattern_FileEdit(t *testing.T) {
+	input, _ := json.Marshal(map[string]string{"path": "/tmp/test.txt", "old_string": "a", "new_string": "b"})
+	if p := extractPattern("file_edit", input); p != "/tmp/test.txt" {
+		t.Errorf("pattern = %q, want /tmp/test.txt", p)
+	}
+}
+
+func TestApprovalRuleSet_WildcardPattern(t *testing.T) {
+	s := NewApprovalRuleSet()
+	s.Add(ApprovalRule{ToolName: "bash", Pattern: ""})
+	input, _ := json.Marshal(map[string]string{"command": "anything"})
+	if !s.Matches("bash", input) {
+		t.Error("empty pattern should match any input")
+	}
+}
+
+func TestApprovalRuleSet_DuplicateRulePrevention(t *testing.T) {
+	s := NewApprovalRuleSet()
+	input, _ := json.Marshal(map[string]string{"channel": "#general"})
+	for i := 0; i < autoApproveThreshold*3; i++ {
+		s.RecordApproval("slack_send", input)
+	}
+	s.mu.Lock()
+	count := 0
+	for _, r := range s.rules {
+		if r.ToolName == "slack_send" && r.Pattern == "#general" {
+			count++
+		}
+	}
+	s.mu.Unlock()
+	if count != 1 {
+		t.Errorf("expected 1 rule, got %d (duplicate prevention failed)", count)
+	}
+}

agent/tools/bash.go

@@ -14,9 +14,11 @@ const defaultBashTimeout = 30 * time.Second
 
 // BashTool executes shell commands.
 type BashTool struct {
-	timeout time.Duration
-	filter  *CommandFilter
-	workDir string
+	timeout       time.Duration
+	filter        *CommandFilter
+	workDir       string
+	interactor    Interactor
+	approvalRules *ApprovalRuleSet
 }
 
 // BashOption configures a BashTool.
@@ -32,6 +34,16 @@ func WithWorkDir(dir string) BashOption {
 	return func(b *BashTool) { b.workDir = dir }
 }
 
+// WithInteractor sets the interactor for approval prompts.
+func WithInteractor(i Interactor) BashOption {
+	return func(b *BashTool) { b.interactor = i }
+}
+
+// WithApprovalRuleSet sets the approval rules for session-scoped auto-approve.
+func WithApprovalRuleSet(rules *ApprovalRuleSet) BashOption {
+	return func(b *BashTool) { b.approvalRules = rules }
+}
+
 // NewBashTool creates a new bash tool with the given timeout and options.
 func NewBashTool(timeout time.Duration, opts ...BashOption) *BashTool {
 	if timeout == 0 {
@@ -68,6 +80,7 @@ func (b *BashTool) Execute(ctx context.Context, input json.RawMessage) (string,
 	if err := json.Unmarshal(input, &in); err != nil {
 		return "", fmt.Errorf("parse input: %w", err)
 	}
+	in.Command = strings.TrimSpace(in.Command)
 	if in.Command == "" {
 		return "", fmt.Errorf("command is required")
 	}
@@ -76,14 +89,32 @@ func (b *BashTool) Execute(ctx context.Context, input json.RawMessage) (string,
 		return "", fmt.Errorf("gws commands must use the gws_execute tool, not bash")
 	}
 
-	if err := b.filter.Check(in.Command); err != nil {
-		return "", fmt.Errorf("command blocked: %w", err)
+	filterResult, filterErr := b.filter.CheckWithResult(in.Command)
+	switch filterResult {
+	case FilterDeny:
+		if filterErr != nil {
+			return "", fmt.Errorf("command blocked: %w", filterErr)
+		}
+		return "", fmt.Errorf("command blocked")
+	case FilterPrompt:
+		if b.interactor == nil {
+			return "", fmt.Errorf("command blocked: no interactor for approval")
+		}
+		return GuardedAction(ctx, b.interactor, RiskMedium,
+			"Run: "+in.Command,
+			func() (string, error) { return b.runCommand(ctx, in.Command) },
+			WithApprovalRules(b.approvalRules, "bash", input),
+		)
 	}
 
+	return b.runCommand(ctx, in.Command)
+}
+
+func (b *BashTool) runCommand(ctx context.Context, command string) (string, error) {
 	ctx, cancel := context.WithTimeout(ctx, b.timeout)
 	defer cancel()
 
-	cmd := exec.CommandContext(ctx, "bash", "-c", in.Command)
+	cmd := exec.CommandContext(ctx, "bash", "-c", command)
 	if b.workDir != "" {
 		cmd.Dir = b.workDir
 	}

agent/tools/bash_filter.go

@@ -7,10 +7,20 @@ import (
 	"strings"
 )
 
+// FilterResult indicates the outcome of a command filter check.
+type FilterResult int
+
+const (
+	FilterAllow  FilterResult = iota // on allowlist, run freely
+	FilterDeny                       // hard blocked
+	FilterPrompt                     // not on allowlist, ask user
+)
+
 // CommandFilter validates shell commands against an allowlist or blocklist.
 type CommandFilter struct {
-	allowed []string // if set, only these prefixes pass
-	blocked []string // if set, these prefixes are rejected
+	allowed   []string // if set, only these prefixes pass
+	blocked   []string // if set, these prefixes are rejected
+	softAllow bool     // if true, non-matching returns FilterPrompt instead of FilterDeny
 }
 
 // NewAllowlistFilter creates a filter that only permits commands
@@ -19,34 +29,61 @@ func NewAllowlistFilter(prefixes []string) *CommandFilter {
 	return &CommandFilter{allowed: prefixes}
 }
 
+// NewSoftAllowlistFilter creates a filter that auto-allows commands on the
+// allowlist and returns FilterPrompt (not FilterDeny) for everything else.
+// Use this for interactive mode where unknown commands should be approved by the user.
+func NewSoftAllowlistFilter(prefixes []string) *CommandFilter {
+	return &CommandFilter{allowed: prefixes, softAllow: true}
+}
+
 // NewBlocklistFilter creates a filter that rejects commands
 // whose first token matches any of the given prefixes.
 func NewBlocklistFilter(prefixes []string) *CommandFilter {
 	return &CommandFilter{blocked: prefixes}
 }
 
-// Check validates the given command string. It splits on shell
-// operators (|, &&, ;, ||) and checks each segment. It also
-// detects command substitution via $() and backticks.
-func (f *CommandFilter) Check(command string) error {
+// CheckWithResult validates the given command string and returns a FilterResult
+// indicating whether to allow, deny, or prompt the user.
+func (f *CommandFilter) CheckWithResult(command string) (FilterResult, error) {
 	if f == nil {
-		return nil
+		return FilterAllow, nil
 	}
 
 	segments := splitShellSegments(command)
 	for _, seg := range segments {
-		if err := f.checkSegment(seg); err != nil {
-			return err
+		result, err := f.checkSegmentResult(seg)
+		if err != nil || result != FilterAllow {
+			return result, err
 		}
 	}
 
-	// Check inside $() and backtick substitutions.
 	for _, sub := range extractSubstitutions(command) {
-		if err := f.Check(sub); err != nil {
-			return fmt.Errorf("in command substitution: %w", err)
+		result, err := f.CheckWithResult(sub)
+		if err != nil {
+			return result, fmt.Errorf("in command substitution: %w", err)
+		}
+		if result != FilterAllow {
+			return result, nil
 		}
 	}
 
+	return FilterAllow, nil
+}
+
+// Check validates the given command string. It splits on shell
+// operators (|, &&, ;, ||) and checks each segment. It also
+// detects command substitution via $() and backticks.
+func (f *CommandFilter) Check(command string) error {
+	result, err := f.CheckWithResult(command)
+	if err != nil {
+		return err
+	}
+	if result == FilterDeny {
+		return fmt.Errorf("command not permitted")
+	}
+	if result == FilterPrompt {
+		return fmt.Errorf("command requires approval")
+	}
 	return nil
 }
 
@@ -55,41 +92,43 @@ func basename(token string) string {
 	return filepath.Base(token)
 }
 
-// checkSegment validates a single command segment.
-// Allowlist: only the first token must match.
-// Blocklist: every token is checked to catch wrappers like "env curl".
-func (f *CommandFilter) checkSegment(seg string) error {
+// checkSegmentResult validates a single command segment and returns a FilterResult.
+func (f *CommandFilter) checkSegmentResult(seg string) (FilterResult, error) {
 	fields := strings.Fields(strings.TrimSpace(seg))
 	if len(fields) == 0 {
-		return nil
+		return FilterAllow, nil
 	}
 	if len(f.allowed) > 0 {
-		return f.checkToken(fields[0])
+		return f.checkTokenResult(fields[0])
 	}
 	for _, tok := range fields {
-		if err := f.checkToken(tok); err != nil {
-			return err
+		result, err := f.checkTokenResult(tok)
+		if err != nil || result != FilterAllow {
+			return result, err
 		}
 	}
-	return nil
+	return FilterAllow, nil
 }
 
-func (f *CommandFilter) checkToken(token string) error {
+func (f *CommandFilter) checkTokenResult(token string) (FilterResult, error) {
 	base := basename(token)
 	if len(f.allowed) > 0 {
 		for _, prefix := range f.allowed {
 			if base == prefix {
-				return nil
+				return FilterAllow, nil
 			}
 		}
-		return fmt.Errorf("command %q not in allowlist", token)
+		if f.softAllow {
+			return FilterPrompt, nil
+		}
+		return FilterDeny, fmt.Errorf("command %q not in allowlist", token)
 	}
 	for _, prefix := range f.blocked {
 		if base == prefix {
-			return fmt.Errorf("command %q is blocked", token)
+			return FilterDeny, fmt.Errorf("command %q is blocked", token)
 		}
 	}
-	return nil
+	return FilterAllow, nil
 }
 
 // splitShellSegments splits a command on |, &&, ;, and || operators.
@@ -139,7 +178,18 @@ func extractSubstitutions(cmd string) []string {
 	return subs
 }
 
-// DefaultBlocklist is the default set of blocked commands for interactive mode.
+// InteractiveAllowlist is the set of commands auto-allowed in interactive mode.
+// Commands not on this list require user approval (FilterPrompt).
+var InteractiveAllowlist = []string{
+	"obk", "sqlite3",
+	"ls", "cat", "head", "tail", "wc", "sort", "uniq", "diff",
+	"find", "grep", "rg",
+	"date", "cal", "echo", "printf",
+	"git", "tree", "file", "stat", "jq", "which",
+}
+
+// DefaultBlocklist is the legacy blocklist used when no Interactor is provided
+// (e.g. subagents). Interactive mode now uses InteractiveAllowlist instead.
 var DefaultBlocklist = []string{
 	// Network
 	"curl", "wget", "nc", "ncat", "nmap",