git.iko.soy/crush
on main help sign in
Merge pull request #138 from charmbracelet/proper-banned-commands
fae5ac8Kujtim Hoxha
internal/llm/tools/bash.go+117−10
compare with main
unifiedsplit
@@ -4,6 +4,7 @@ import (
4 "context"
4 "context"
5 "encoding/json"
5 "encoding/json"
6 "fmt"
6 "fmt"
7+ "log/slog"
7 "runtime"
8 "runtime"
8 "strings"
9 "strings"
9 "time"
10 "time"
@@ -41,9 +42,74 @@ const (
41)
42)
42
43
43var bannedCommands = []string{
44var bannedCommands = []string{
44− "alias", "curl", "curlie", "wget", "axel", "aria2c",
45+ // Network/Download tools
45− "nc", "telnet", "lynx", "w3m", "links", "httpie", "xh",
46+ "alias",
46− "http-prompt", "chrome", "firefox", "safari",
47+ "aria2c",
48+ "axel",
49+ "chrome",
50+ "curl",
51+ "curlie",
52+ "firefox",
53+ "http-prompt",
54+ "httpie",
55+ "links",
56+ "lynx",
57+ "nc",
58+ "safari",
59+ "telnet",
60+ "w3m",
61+ "wget",
62+ "xh",
63+
64+ // System administration
65+ "doas",
66+ "su",
67+ "sudo",
68+
69+ // Package managers
70+ "apk",
71+ "apt",
72+ "apt-cache",
73+ "apt-get",
74+ "dnf",
75+ "dpkg",
76+ "emerge",
77+ "home-manager",
78+ "makepkg",
79+ "opkg",
80+ "pacman",
81+ "paru",
82+ "pkg",
83+ "pkg_add",
84+ "pkg_delete",
85+ "portage",
86+ "rpm",
87+ "yay",
88+ "yum",
89+ "zypper",
90+
91+ // System modification
92+ "at",
93+ "batch",
94+ "chkconfig",
95+ "crontab",
96+ "fdisk",
97+ "mkfs",
98+ "mount",
99+ "parted",
100+ "service",
101+ "systemctl",
102+ "umount",
103+
104+ // Network configuration
105+ "firewall-cmd",
106+ "ifconfig",
107+ "ip",
108+ "iptables",
109+ "netstat",
110+ "pfctl",
111+ "route",
112+ "ufw",
47}
113}
48
114
49// getSafeReadOnlyCommands returns platform-appropriate safe commands
115// getSafeReadOnlyCommands returns platform-appropriate safe commands
@@ -244,7 +310,42 @@ Important:
244- Never update git config`, bannedCommandsStr, MaxOutputLength)
310- Never update git config`, bannedCommandsStr, MaxOutputLength)
245}
311}
246
312
313+func blockFuncs() []shell.BlockFunc {
314+ return []shell.BlockFunc{
315+ shell.CommandsBlocker(bannedCommands),
316+ shell.ArgumentsBlocker([][]string{
317+ // System package managers
318+ {"apk", "add"},
319+ {"apt", "install"},
320+ {"apt-get", "install"},
321+ {"dnf", "install"},
322+ {"emerge"},
323+ {"pacman", "-S"},
324+ {"pkg", "install"},
325+ {"yum", "install"},
326+ {"zypper", "install"},
327+
328+ // Language-specific package managers
329+ {"brew", "install"},
330+ {"cargo", "install"},
331+ {"gem", "install"},
332+ {"go", "install"},
333+ {"npm", "install", "-g"},
334+ {"npm", "install", "--global"},
335+ {"pip", "install", "--user"},
336+ {"pip3", "install", "--user"},
337+ {"pnpm", "add", "-g"},
338+ {"pnpm", "add", "--global"},
339+ {"yarn", "global", "add"},
340+ }),
341+ }
342+}
343+
247func NewBashTool(permission permission.Service, workingDir string) BaseTool {
344func NewBashTool(permission permission.Service, workingDir string) BaseTool {
345+ // Set up command blocking on the persistent shell
346+ persistentShell := shell.GetPersistentShell(workingDir)
347+ persistentShell.SetBlockFuncs(blockFuncs())
348+
248 return &bashTool{
349 return &bashTool{
249 permissions: permission,
350 permissions: permission,
250 workingDir: workingDir,
351 workingDir: workingDir,
@@ -289,13 +390,6 @@ func (b *bashTool) Run(ctx context.Context, call ToolCall) (ToolResponse, error)
289 return NewTextErrorResponse("missing command"), nil
390 return NewTextErrorResponse("missing command"), nil
290 }
391 }
291
392
292− baseCmd := strings.Fields(params.Command)[0]
293− for _, banned := range bannedCommands {
294− if strings.EqualFold(baseCmd, banned) {
295− return NewTextErrorResponse(fmt.Sprintf("command '%s' is not allowed", baseCmd)), nil
296− }
297− }
298−
299 isSafeReadOnly := false
393 isSafeReadOnly := false
300 cmdLower := strings.ToLower(params.Command)
394 cmdLower := strings.ToLower(params.Command)
301
395
@@ -349,7 +443,20 @@ func (b *bashTool) Run(ctx context.Context, call ToolCall) (ToolResponse, error)
349 stdout = truncateOutput(stdout)
443 stdout = truncateOutput(stdout)
350 stderr = truncateOutput(stderr)
444 stderr = truncateOutput(stderr)
351
445
446+ slog.Info("Bash command executed",
447+ "command", params.Command,
448+ "stdout", stdout,
449+ "stderr", stderr,
450+ "exit_code", exitCode,
451+ "interrupted", interrupted,
452+ "err", err,
453+ )
454+
352 errorMessage := stderr
455 errorMessage := stderr
456+ if errorMessage == "" && err != nil {
457+ errorMessage = err.Error()
458+ }
459+
353 if interrupted {
460 if interrupted {
354 if errorMessage != "" {
461 if errorMessage != "" {
355 errorMessage += "\n"
462 errorMessage += "\n"
internal/shell/command_block_test.gonew file+123
@@ -0,0 +1,123 @@
1+package shell
2+
3+import (
4+ "context"
5+ "os"
6+ "strings"
7+ "testing"
8+)
9+
10+func TestCommandBlocking(t *testing.T) {
11+ tests := []struct {
12+ name string
13+ blockFuncs []BlockFunc
14+ command string
15+ shouldBlock bool
16+ }{
17+ {
18+ name: "block simple command",
19+ blockFuncs: []BlockFunc{
20+ func(args []string) bool {
21+ return len(args) > 0 && args[0] == "curl"
22+ },
23+ },
24+ command: "curl https://example.com",
25+ shouldBlock: true,
26+ },
27+ {
28+ name: "allow non-blocked command",
29+ blockFuncs: []BlockFunc{
30+ func(args []string) bool {
31+ return len(args) > 0 && args[0] == "curl"
32+ },
33+ },
34+ command: "echo hello",
35+ shouldBlock: false,
36+ },
37+ {
38+ name: "block subcommand",
39+ blockFuncs: []BlockFunc{
40+ func(args []string) bool {
41+ return len(args) >= 2 && args[0] == "brew" && args[1] == "install"
42+ },
43+ },
44+ command: "brew install wget",
45+ shouldBlock: true,
46+ },
47+ {
48+ name: "allow different subcommand",
49+ blockFuncs: []BlockFunc{
50+ func(args []string) bool {
51+ return len(args) >= 2 && args[0] == "brew" && args[1] == "install"
52+ },
53+ },
54+ command: "brew list",
55+ shouldBlock: false,
56+ },
57+ {
58+ name: "block npm global install with -g",
59+ blockFuncs: []BlockFunc{
60+ ArgumentsBlocker([][]string{
61+ {"npm", "install", "-g"},
62+ {"npm", "install", "--global"},
63+ }),
64+ },
65+ command: "npm install -g typescript",
66+ shouldBlock: true,
67+ },
68+ {
69+ name: "block npm global install with --global",
70+ blockFuncs: []BlockFunc{
71+ ArgumentsBlocker([][]string{
72+ {"npm", "install", "-g"},
73+ {"npm", "install", "--global"},
74+ }),
75+ },
76+ command: "npm install --global typescript",
77+ shouldBlock: true,
78+ },
79+ {
80+ name: "allow npm local install",
81+ blockFuncs: []BlockFunc{
82+ ArgumentsBlocker([][]string{
83+ {"npm", "install", "-g"},
84+ {"npm", "install", "--global"},
85+ }),
86+ },
87+ command: "npm install typescript",
88+ shouldBlock: false,
89+ },
90+ }
91+
92+ for _, tt := range tests {
93+ t.Run(tt.name, func(t *testing.T) {
94+ // Create a temporary directory for each test
95+ tmpDir, err := os.MkdirTemp("", "shell-test-*")
96+ if err != nil {
97+ t.Fatalf("Failed to create temp dir: %v", err)
98+ }
99+ defer os.RemoveAll(tmpDir)
100+
101+ shell := NewShell(&Options{
102+ WorkingDir: tmpDir,
103+ BlockFuncs: tt.blockFuncs,
104+ })
105+
106+ _, _, err = shell.Exec(context.Background(), tt.command)
107+
108+ if tt.shouldBlock {
109+ if err == nil {
110+ t.Errorf("Expected command to be blocked, but it was allowed")
111+ } else if !strings.Contains(err.Error(), "not allowed for security reasons") {
112+ t.Errorf("Expected security error, got: %v", err)
113+ }
114+ } else {
115+ // For non-blocked commands, we might get other errors (like command not found)
116+ // but we shouldn't get the security error
117+ if err != nil && strings.Contains(err.Error(), "not allowed for security reasons") {
118+ t.Errorf("Command was unexpectedly blocked: %v", err)
119+ }
120+ }
121+ })
122+ }
123+}
internal/shell/shell.go+75−7
@@ -44,12 +44,16 @@ type noopLogger struct{}
44
44
45func (noopLogger) InfoPersist(msg string, keysAndValues ...interface{}) {}
45func (noopLogger) InfoPersist(msg string, keysAndValues ...interface{}) {}
46
46
47+// BlockFunc is a function that determines if a command should be blocked
48+type BlockFunc func(args []string) bool
49+
47// Shell provides cross-platform shell execution with optional state persistence
50// Shell provides cross-platform shell execution with optional state persistence
48type Shell struct {
51type Shell struct {
49− env []string
52+ env []string
50− cwd string
53+ cwd string
51− mu sync.Mutex
54+ mu sync.Mutex
52− logger Logger
55+ logger Logger
56+ blockFuncs []BlockFunc
53}
57}
54
58
55// Options for creating a new shell
59// Options for creating a new shell
@@ -57,6 +61,7 @@ type Options struct {
57 WorkingDir string
61 WorkingDir string
58 Env []string
62 Env []string
59 Logger Logger
63 Logger Logger
64+ BlockFuncs []BlockFunc
60}
65}
61
66
62// NewShell creates a new shell instance with the given options
67// NewShell creates a new shell instance with the given options
@@ -81,9 +86,10 @@ func NewShell(opts *Options) *Shell {
81 }
86 }
82
87
83 return &Shell{
88 return &Shell{
84− cwd: cwd,
89+ cwd: cwd,
85− env: env,
90+ env: env,
86− logger: logger,
91+ logger: logger,
92+ blockFuncs: opts.BlockFuncs,
87 }
93 }
88}
94}
89
95
@@ -152,6 +158,13 @@ func (s *Shell) SetEnv(key, value string) {
152 s.env = append(s.env, keyPrefix+value)
158 s.env = append(s.env, keyPrefix+value)
153}
159}
154
160
161+// SetBlockFuncs sets the command block functions for the shell
162+func (s *Shell) SetBlockFuncs(blockFuncs []BlockFunc) {
163+ s.mu.Lock()
164+ defer s.mu.Unlock()
165+ s.blockFuncs = blockFuncs
166+}
167+
155// Windows-specific commands that should use native shell
168// Windows-specific commands that should use native shell
156var windowsNativeCommands = map[string]bool{
169var windowsNativeCommands = map[string]bool{
157 "dir": true,
170 "dir": true,
@@ -203,6 +216,60 @@ func (s *Shell) determineShellType(command string) ShellType {
203 return ShellTypePOSIX
216 return ShellTypePOSIX
204}
217}
205
218
219+// CommandsBlocker creates a BlockFunc that blocks exact command matches
220+func CommandsBlocker(bannedCommands []string) BlockFunc {
221+ bannedSet := make(map[string]bool)
222+ for _, cmd := range bannedCommands {
223+ bannedSet[cmd] = true
224+ }
225+
226+ return func(args []string) bool {
227+ if len(args) == 0 {
228+ return false
229+ }
230+ return bannedSet[args[0]]
231+ }
232+}
233+
234+// ArgumentsBlocker creates a BlockFunc that blocks specific subcommands
235+func ArgumentsBlocker(blockedSubCommands [][]string) BlockFunc {
236+ return func(args []string) bool {
237+ for _, blocked := range blockedSubCommands {
238+ if len(args) >= len(blocked) {
239+ match := true
240+ for i, part := range blocked {
241+ if args[i] != part {
242+ match = false
243+ break
244+ }
245+ }
246+ if match {
247+ return true
248+ }
249+ }
250+ }
251+ return false
252+ }
253+}
254+
255+func (s *Shell) blockHandler() func(next interp.ExecHandlerFunc) interp.ExecHandlerFunc {
256+ return func(next interp.ExecHandlerFunc) interp.ExecHandlerFunc {
257+ return func(ctx context.Context, args []string) error {
258+ if len(args) == 0 {
259+ return next(ctx, args)
260+ }
261+
262+ for _, blockFunc := range s.blockFuncs {
263+ if blockFunc(args) {
264+ return fmt.Errorf("command is not allowed for security reasons: %s", strings.Join(args, " "))
265+ }
266+ }
267+
268+ return next(ctx, args)
269+ }
270+ }
271+}
272+
206// execWindows executes commands using native Windows shells (cmd.exe or PowerShell)
273// execWindows executes commands using native Windows shells (cmd.exe or PowerShell)
207func (s *Shell) execWindows(ctx context.Context, command string, shell string) (string, string, error) {
274func (s *Shell) execWindows(ctx context.Context, command string, shell string) (string, string, error) {
208 var cmd *exec.Cmd
275 var cmd *exec.Cmd
@@ -291,6 +358,7 @@ func (s *Shell) execPOSIX(ctx context.Context, command string) (string, string,
291 interp.Interactive(false),
358 interp.Interactive(false),
292 interp.Env(expand.ListEnviron(s.env...)),
359 interp.Env(expand.ListEnviron(s.env...)),
293 interp.Dir(s.cwd),
360 interp.Dir(s.cwd),
361+ interp.ExecHandlers(s.blockHandler()),
294 )
362 )
295 if err != nil {
363 if err != nil {
296 return "", "", fmt.Errorf("could not run command: %w", err)
364 return "", "", fmt.Errorf("could not run command: %w", err)