From f8fcf0ee2f4f15ee117e90ff2c3dc1f757b12f74 Mon Sep 17 00:00:00 2001 From: Cai_Tang <106404101+Cai-Tang-www@users.noreply.github.com> Date: Sat, 4 Apr 2026 16:31:36 +0800 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20=E4=B8=BAbash=E5=B7=A5=E5=85=B7?= =?UTF-8?q?=E6=8E=A5=E5=85=A5=E5=AE=89=E5=85=A8=E6=89=A7=E8=A1=8C=E5=99=A8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/tools/bash/executor.go | 150 +++++++++++++ internal/tools/bash/executor_test.go | 203 ++++++++++++++++++ internal/tools/bash/helpers_test.go | 6 +- .../tools/bash/security_integration_test.go | 131 +++++++++++ internal/tools/bash/tool.go | 124 +++-------- 5 files changed, 513 insertions(+), 101 deletions(-) create mode 100644 internal/tools/bash/executor.go create mode 100644 internal/tools/bash/executor_test.go create mode 100644 internal/tools/bash/security_integration_test.go diff --git a/internal/tools/bash/executor.go b/internal/tools/bash/executor.go new file mode 100644 index 00000000..5736449c --- /dev/null +++ b/internal/tools/bash/executor.go @@ -0,0 +1,150 @@ +package bash + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "time" + + "neo-code/internal/security" + "neo-code/internal/tools" +) + +// SecurityExecutor is the bash execution boundary that enforces workspace and +// runtime safety checks before running a shell command. +type SecurityExecutor interface { + Execute(ctx context.Context, call tools.ToolCallInput, command string, requestedWorkdir string) (tools.ToolResult, error) +} + +type commandRunner interface { + CombinedOutput(ctx context.Context, binary string, args []string, workdir string) ([]byte, error) +} + +type execCommandRunner struct{} + +func (execCommandRunner) CombinedOutput( + ctx context.Context, + binary string, + args []string, + workdir string, +) ([]byte, error) { + cmd := exec.CommandContext(ctx, binary, args...) + cmd.Dir = workdir + return cmd.CombinedOutput() +} + +type defaultSecurityExecutor struct { + root string + shell string + timeout time.Duration + runner commandRunner +} + +// NewDefaultSecurityExecutor returns the default secure bash executor. +func NewDefaultSecurityExecutor(root string, shell string, timeout time.Duration) SecurityExecutor { + return &defaultSecurityExecutor{ + root: root, + shell: shell, + timeout: timeout, + runner: execCommandRunner{}, + } +} + +func (e *defaultSecurityExecutor) Execute( + ctx context.Context, + call tools.ToolCallInput, + command string, + requestedWorkdir string, +) (tools.ToolResult, error) { + if strings.TrimSpace(command) == "" { + err := errors.New("bash: command is empty") + return tools.NewErrorResult("bash", tools.NormalizeErrorReason("bash", err), "", nil), err + } + + base := strings.TrimSpace(call.Workdir) + if base == "" { + base = e.root + } + _, workdir, err := tools.ResolveWorkspaceTarget( + call, + security.TargetTypeDirectory, + base, + requestedWorkdir, + resolveWorkdir, + ) + if err != nil { + return tools.NewErrorResult("bash", tools.NormalizeErrorReason("bash", err), "", nil), err + } + + runCtx, cancel := context.WithTimeout(ctx, e.timeout) + defer cancel() + + binary, args := shellCommand(e.shell, command) + output, runErr := e.runner.CombinedOutput(runCtx, binary, args, workdir) + content := string(output) + if runErr != nil { + result := tools.NewErrorResult( + "bash", + tools.NormalizeErrorReason("bash", runErr), + content, + map[string]any{"workdir": workdir}, + ) + result = tools.ApplyOutputLimit(result, tools.DefaultOutputLimitBytes) + return result, runErr + } + + result := tools.ToolResult{ + Name: "bash", + Content: content, + Metadata: map[string]any{ + "workdir": workdir, + }, + } + result = tools.ApplyOutputLimit(result, tools.DefaultOutputLimitBytes) + return result, nil +} + +func shellCommand(shell string, command string) (string, []string) { + switch strings.ToLower(strings.TrimSpace(shell)) { + case "powershell", "pwsh": + return "powershell", []string{"-NoProfile", "-Command", command} + case "bash": + return "bash", []string{"-lc", command} + case "sh": + return "sh", []string{"-lc", command} + } + + if runtime.GOOS == "windows" { + return "powershell", []string{"-NoProfile", "-Command", command} + } + return "sh", []string{"-lc", command} +} + +func resolveWorkdir(root string, requested string) (string, error) { + base, err := filepath.Abs(root) + if err != nil { + return "", err + } + target := requested + if strings.TrimSpace(target) == "" { + target = base + } else if !filepath.IsAbs(target) { + target = filepath.Join(base, target) + } + target, err = filepath.Abs(target) + if err != nil { + return "", err + } + rel, err := filepath.Rel(base, target) + if err != nil { + return "", err + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return "", errors.New("bash: workdir escapes workspace root") + } + return target, nil +} diff --git a/internal/tools/bash/executor_test.go b/internal/tools/bash/executor_test.go new file mode 100644 index 00000000..45071e3a --- /dev/null +++ b/internal/tools/bash/executor_test.go @@ -0,0 +1,203 @@ +package bash + +import ( + "context" + "errors" + "path/filepath" + "strings" + "testing" + "time" + + "neo-code/internal/tools" +) + +type stubRunner struct { + run func(ctx context.Context, binary string, args []string, workdir string) ([]byte, error) +} + +func (r stubRunner) CombinedOutput( + ctx context.Context, + binary string, + args []string, + workdir string, +) ([]byte, error) { + if r.run != nil { + return r.run(ctx, binary, args, workdir) + } + return []byte("ok"), nil +} + +func TestDefaultSecurityExecutorExecute(t *testing.T) { + t.Parallel() + + workspace := t.TempDir() + executor := &defaultSecurityExecutor{ + root: workspace, + shell: defaultShell(), + timeout: 20 * time.Millisecond, + runner: stubRunner{}, + } + + tests := []struct { + name string + callWorkdir string + command string + requestedDir string + overrideRun func(ctx context.Context, binary string, args []string, workdir string) ([]byte, error) + expectErr string + expectResult []string + expectMeta string + }{ + { + name: "rejects empty command", + command: "", + callWorkdir: workspace, + expectErr: "command is empty", + }, + { + name: "rejects escaped workdir", + command: "echo hi", + callWorkdir: workspace, + requestedDir: "..", + expectErr: "workdir escapes workspace root", + }, + { + name: "handles timeout from runner context", + command: "slow", + callWorkdir: workspace, + overrideRun: func(ctx context.Context, binary string, args []string, workdir string) ([]byte, error) { + <-ctx.Done() + return nil, ctx.Err() + }, + expectErr: context.DeadlineExceeded.Error(), + expectResult: []string{"tool error", "tool: bash"}, + expectMeta: workspace, + }, + { + name: "applies output truncation on error details", + command: "boom", + callWorkdir: workspace, + overrideRun: func(ctx context.Context, binary string, args []string, workdir string) ([]byte, error) { + return []byte(strings.Repeat("x", tools.DefaultOutputLimitBytes+100)), errors.New("exit status 1") + }, + expectErr: "exit status 1", + expectResult: []string{"...[truncated]"}, + expectMeta: workspace, + }, + { + name: "success returns output and metadata", + command: "ok", + callWorkdir: workspace, + overrideRun: func(ctx context.Context, binary string, args []string, workdir string) ([]byte, error) { + return []byte("hello"), nil + }, + expectResult: []string{"hello"}, + expectMeta: workspace, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + runner := stubRunner{} + if tt.overrideRun != nil { + runner.run = tt.overrideRun + } + executor.runner = runner + + result, err := executor.Execute( + context.Background(), + tools.ToolCallInput{Workdir: tt.callWorkdir}, + tt.command, + tt.requestedDir, + ) + + if tt.expectErr != "" { + if err == nil || !strings.Contains(err.Error(), tt.expectErr) { + t.Fatalf("expected error containing %q, got %v", tt.expectErr, err) + } + } else if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + for _, fragment := range tt.expectResult { + if !strings.Contains(result.Content, fragment) { + t.Fatalf("expected content containing %q, got %q", fragment, result.Content) + } + } + if tt.expectMeta != "" { + got, _ := result.Metadata["workdir"].(string) + if !strings.Contains(got, tt.expectMeta) { + t.Fatalf("expected workdir metadata containing %q, got %q", tt.expectMeta, got) + } + } + }) + } +} + +func TestShellCommand(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + shell string + binary string + }{ + {name: "powershell", shell: "powershell", binary: "powershell"}, + {name: "pwsh", shell: "pwsh", binary: "powershell"}, + {name: "bash", shell: "bash", binary: "bash"}, + {name: "sh", shell: "sh", binary: "sh"}, + {name: "fallback", shell: "unknown", binary: defaultShell()}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + binary, args := shellCommand(tt.shell, "echo hi") + if binary != tt.binary { + t.Fatalf("shellCommand(%q) binary=%q, want %q", tt.shell, binary, tt.binary) + } + if len(args) == 0 { + t.Fatalf("expected non-empty args") + } + }) + } +} + +func TestResolveWorkdir(t *testing.T) { + t.Parallel() + + workspace := t.TempDir() + tests := []struct { + name string + root string + requested string + expectErr string + }{ + {name: "uses root for empty requested", root: workspace, requested: ""}, + {name: "resolves relative path", root: workspace, requested: "sub"}, + {name: "rejects traversal", root: workspace, requested: "..", expectErr: "escapes workspace root"}, + {name: "rejects invalid root", root: string([]byte{0}), requested: "", expectErr: "invalid"}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + target, err := resolveWorkdir(tt.root, tt.requested) + if tt.expectErr != "" { + if err == nil || !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(tt.expectErr)) { + t.Fatalf("expected error containing %q, got %v", tt.expectErr, err) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if tt.requested != "" && !filepath.IsAbs(target) { + t.Fatalf("expected absolute target, got %q", target) + } + }) + } +} diff --git a/internal/tools/bash/helpers_test.go b/internal/tools/bash/helpers_test.go index af6773ed..89b8be75 100644 --- a/internal/tools/bash/helpers_test.go +++ b/internal/tools/bash/helpers_test.go @@ -44,7 +44,8 @@ func TestToolHelpers(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - got := New(t.TempDir(), tt.shell, time.Second).shellArgs("echo hi") + binary, args := shellCommand(tt.shell, "echo hi") + got := append([]string{binary}, args...) if len(got) < len(tt.want) { t.Fatalf("expected shell args prefix %v, got %v", tt.want, got) } @@ -57,7 +58,8 @@ func TestToolHelpers(t *testing.T) { } t.Run("default shell args", func(t *testing.T) { - got := New(t.TempDir(), "unknown", time.Second).shellArgs("echo hi") + binary, args := shellCommand("unknown", "echo hi") + got := append([]string{binary}, args...) if goruntime.GOOS == "windows" { if got[0] != "powershell" { t.Fatalf("expected windows fallback to powershell, got %v", got) diff --git a/internal/tools/bash/security_integration_test.go b/internal/tools/bash/security_integration_test.go new file mode 100644 index 00000000..9b44f080 --- /dev/null +++ b/internal/tools/bash/security_integration_test.go @@ -0,0 +1,131 @@ +package bash + +import ( + "context" + "encoding/json" + "errors" + "strings" + "testing" + "time" + + "neo-code/internal/security" + "neo-code/internal/tools" +) + +type stubSecurityExecutor struct { + calls int + err error +} + +func (e *stubSecurityExecutor) Execute( + ctx context.Context, + call tools.ToolCallInput, + command string, + requestedWorkdir string, +) (tools.ToolResult, error) { + e.calls++ + if e.err != nil { + return tools.NewErrorResult("bash", tools.NormalizeErrorReason("bash", e.err), "", nil), e.err + } + return tools.ToolResult{ + Name: "bash", + Content: "executed", + Metadata: map[string]any{ + "command": command, + }, + }, nil +} + +func TestBashToolManagerPermissionDecisions(t *testing.T) { + t.Parallel() + + workspace := t.TempDir() + args, err := json.Marshal(map[string]string{ + "command": "echo hi", + }) + if err != nil { + t.Fatalf("marshal args: %v", err) + } + + tests := []struct { + name string + rules []security.Rule + executorErr error + expectErr string + expectCalls int + expectBody string + }{ + { + name: "allow runs executor", + expectCalls: 1, + expectBody: "executed", + }, + { + name: "deny blocks before executor", + rules: []security.Rule{ + {ID: "deny-bash", Type: security.ActionTypeBash, Resource: "bash", Decision: security.DecisionDeny, Reason: "bash denied"}, + }, + expectErr: "bash denied", + expectCalls: 0, + expectBody: "reason: bash denied", + }, + { + name: "ask blocks before executor", + rules: []security.Rule{ + {ID: "ask-bash", Type: security.ActionTypeBash, Resource: "bash", Decision: security.DecisionAsk, Reason: "need approval"}, + }, + expectErr: "need approval", + expectCalls: 0, + expectBody: "reason: need approval", + }, + { + name: "executor error is returned", + executorErr: errors.New("bash: failed"), + expectErr: "failed", + expectCalls: 1, + expectBody: "reason: failed", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + executor := &stubSecurityExecutor{err: tt.executorErr} + tool := NewWithExecutor(workspace, defaultShell(), 2*time.Second, executor) + registry := tools.NewRegistry() + registry.Register(tool) + + engine, err := security.NewStaticGateway(security.DecisionAllow, tt.rules) + if err != nil { + t.Fatalf("new static gateway: %v", err) + } + manager, err := tools.NewManager(registry, engine, security.NewWorkspaceSandbox()) + if err != nil { + t.Fatalf("new manager: %v", err) + } + + result, execErr := manager.Execute(context.Background(), tools.ToolCallInput{ + Name: "bash", + Arguments: args, + Workdir: workspace, + }) + + if tt.expectErr != "" { + if execErr == nil || !strings.Contains(execErr.Error(), tt.expectErr) { + t.Fatalf("expected error containing %q, got %v", tt.expectErr, execErr) + } + } else if execErr != nil { + t.Fatalf("unexpected error: %v", execErr) + } + + if executor.calls != tt.expectCalls { + t.Fatalf("expected executor calls %d, got %d", tt.expectCalls, executor.calls) + } + if tt.expectBody != "" && !strings.Contains(result.Content, tt.expectBody) { + t.Fatalf("expected result content containing %q, got %q", tt.expectBody, result.Content) + } + }) + } +} diff --git a/internal/tools/bash/tool.go b/internal/tools/bash/tool.go index 9facbb88..f2559d97 100644 --- a/internal/tools/bash/tool.go +++ b/internal/tools/bash/tool.go @@ -4,21 +4,16 @@ import ( "context" "encoding/json" "errors" - "os" - "os/exec" - "path/filepath" - "runtime" - "strings" "time" - "neo-code/internal/security" "neo-code/internal/tools" ) type Tool struct { - root string - shell string - timeout time.Duration + root string + shell string + timeout time.Duration + executor SecurityExecutor } type input struct { @@ -27,10 +22,25 @@ type input struct { } func New(root string, shell string, timeout time.Duration) *Tool { + executor := NewDefaultSecurityExecutor(root, shell, timeout) return &Tool{ - root: root, - shell: shell, - timeout: timeout, + root: root, + shell: shell, + timeout: timeout, + executor: executor, + } +} + +// NewWithExecutor creates a bash tool using an injected security executor. +func NewWithExecutor(root string, shell string, timeout time.Duration, executor SecurityExecutor) *Tool { + if executor == nil { + executor = NewDefaultSecurityExecutor(root, shell, timeout) + } + return &Tool{ + root: root, + shell: shell, + timeout: timeout, + executor: executor, } } @@ -64,94 +74,10 @@ func (t *Tool) Execute(ctx context.Context, call tools.ToolCallInput) (tools.Too if err := json.Unmarshal(call.Arguments, &in); err != nil { return tools.NewErrorResult(t.Name(), "invalid arguments", err.Error(), nil), err } - if strings.TrimSpace(in.Command) == "" { - err := errors.New("bash: command is empty") + if t.executor == nil { + err := errors.New("bash: security executor is nil") return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } - base := strings.TrimSpace(call.Workdir) - if base == "" { - base = t.root - } - _, workdir, err := tools.ResolveWorkspaceTarget( - call, - security.TargetTypeDirectory, - base, - in.Workdir, - resolveWorkdir, - ) - if err != nil { - return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err - } - - runCtx, cancel := context.WithTimeout(ctx, t.timeout) - defer cancel() - - args := t.shellArgs(in.Command) - cmd := exec.CommandContext(runCtx, args[0], args[1:]...) - cmd.Dir = workdir - output, err := cmd.CombinedOutput() - - content := string(output) - if err != nil { - result := tools.NewErrorResult( - t.Name(), - tools.NormalizeErrorReason(t.Name(), err), - content, - map[string]any{"workdir": workdir}, - ) - result = tools.ApplyOutputLimit(result, tools.DefaultOutputLimitBytes) - return result, err - } - - result := tools.ToolResult{ - Name: t.Name(), - Content: content, - Metadata: map[string]any{ - "workdir": workdir, - }, - } - result = tools.ApplyOutputLimit(result, tools.DefaultOutputLimitBytes) - return result, nil -} - -func (t *Tool) shellArgs(command string) []string { - shell := strings.ToLower(strings.TrimSpace(t.shell)) - switch shell { - case "powershell", "pwsh": - return []string{"powershell", "-NoProfile", "-Command", command} - case "bash": - return []string{"bash", "-lc", command} - case "sh": - return []string{"sh", "-lc", command} - } - if runtime.GOOS == "windows" { - return []string{"powershell", "-NoProfile", "-Command", command} - } - return []string{"sh", "-lc", command} -} - -func resolveWorkdir(root string, requested string) (string, error) { - base, err := filepath.Abs(root) - if err != nil { - return "", err - } - target := requested - if strings.TrimSpace(target) == "" { - target = base - } else if !filepath.IsAbs(target) { - target = filepath.Join(base, target) - } - target, err = filepath.Abs(target) - if err != nil { - return "", err - } - rel, err := filepath.Rel(base, target) - if err != nil { - return "", err - } - if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { - return "", errors.New("bash: workdir escapes workspace root") - } - return target, nil + return t.executor.Execute(ctx, call, in.Command, in.Workdir) } From 287225c2d74c2bd0610b6a71b65c49d3fe821961 Mon Sep 17 00:00:00 2001 From: Cai_Tang <106404101+Cai-Tang-www@users.noreply.github.com> Date: Sun, 5 Apr 2026 10:22:28 +0800 Subject: [PATCH 2/2] fix: reject NUL byte paths in bash workdir resolver --- internal/tools/bash/executor.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/tools/bash/executor.go b/internal/tools/bash/executor.go index 5736449c..e8dc7247 100644 --- a/internal/tools/bash/executor.go +++ b/internal/tools/bash/executor.go @@ -125,6 +125,9 @@ func shellCommand(shell string, command string) (string, []string) { } func resolveWorkdir(root string, requested string) (string, error) { + if strings.ContainsRune(root, '\x00') || strings.ContainsRune(requested, '\x00') { + return "", errors.New("bash: invalid path contains NUL") + } base, err := filepath.Abs(root) if err != nil { return "", err