From f259a04aa9a909bc8e6a023027ffa92146a13de3 Mon Sep 17 00:00:00 2001 From: stevef Date: Fri, 17 Apr 2026 20:32:09 +0200 Subject: [PATCH] feat: block exec commands from writing to protected dirs via denyWritePaths --- pkg/agent/instance.go | 6 +++-- pkg/tools/shell.go | 47 +++++++++++++++++++++++++++++++------ pkg/tools/shell_test.go | 52 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 9 deletions(-) diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index 8a9463a46..dc2ea163b 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -96,11 +96,13 @@ func NewAgentInstance( toolsRegistry.Register(tools.NewListDirTool(workspace, readRestrict, allowReadPaths, denyReadPaths)) } if cfg.Tools.IsToolEnabled("exec") { - execTool, err := tools.NewExecToolWithConfig(workspace, restrict, cfg, allowReadPaths) + execTool, err := tools.NewExecToolWithDenyPaths(workspace, restrict, [][]*regexp.Regexp{allowReadPaths}, denyWritePaths, cfg) if err != nil { logger.ErrorCF("agent", "Failed to initialize exec tool; continuing without exec", map[string]any{"error": err.Error()}) - } else { + execTool = nil + } + if execTool != nil { toolsRegistry.Register(execTool) } } diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 96200b9ff..1d789ede0 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -40,6 +40,7 @@ type ExecTool struct { allowPatterns []*regexp.Regexp customAllowPatterns []*regexp.Regexp allowedPathPatterns []*regexp.Regexp + denyWritePaths []*regexp.Regexp restrictToWorkspace bool allowRemote bool sessionManager *SessionManager @@ -114,14 +115,24 @@ var ( ) func NewExecTool(workingDir string, restrict bool, allowPaths ...[]*regexp.Regexp) (*ExecTool, error) { - return NewExecToolWithConfig(workingDir, restrict, nil, allowPaths...) + return NewExecToolWithDenyPaths(workingDir, restrict, allowPaths, nil, nil) } func NewExecToolWithConfig( workingDir string, restrict bool, - config *config.Config, + cfg *config.Config, allowPaths ...[]*regexp.Regexp, +) (*ExecTool, error) { + return NewExecToolWithDenyPaths(workingDir, restrict, allowPaths, nil, cfg) +} + +func NewExecToolWithDenyPaths( + workingDir string, + restrict bool, + allowPaths [][]*regexp.Regexp, + denyWritePaths []*regexp.Regexp, + cfg *config.Config, ) (*ExecTool, error) { denyPatterns := make([]*regexp.Regexp, 0) customAllowPatterns := make([]*regexp.Regexp, 0) @@ -131,8 +142,8 @@ func NewExecToolWithConfig( allowedPathPatterns = allowPaths[0] } - if config != nil { - execConfig := config.Tools.Exec + if cfg != nil { + execConfig := cfg.Tools.Exec enableDenyPatterns := execConfig.EnableDenyPatterns allowRemote = execConfig.AllowRemote if enableDenyPatterns { @@ -148,7 +159,6 @@ func NewExecToolWithConfig( } } } else { - // If deny patterns are disabled, we won't add any patterns, allowing all commands. fmt.Println("Warning: deny patterns are disabled. All commands will be allowed.") } for _, pattern := range execConfig.CustomAllowPatterns { @@ -163,8 +173,8 @@ func NewExecToolWithConfig( } var timeout time.Duration - if config != nil && config.Tools.Exec.TimeoutSeconds > 0 { - timeout = time.Duration(config.Tools.Exec.TimeoutSeconds) * time.Second + if cfg != nil && cfg.Tools.Exec.TimeoutSeconds > 0 { + timeout = time.Duration(cfg.Tools.Exec.TimeoutSeconds) * time.Second } return &ExecTool{ @@ -174,6 +184,7 @@ func NewExecToolWithConfig( allowPatterns: nil, customAllowPatterns: customAllowPatterns, allowedPathPatterns: allowedPathPatterns, + denyWritePaths: denyWritePaths, restrictToWorkspace: restrict, allowRemote: allowRemote, sessionManager: getSessionManager(), @@ -1033,6 +1044,28 @@ func (t *ExecTool) guardCommand(command, cwd string) string { return "Command blocked by safety guard (dangerous pattern detected)" } } + + // Check deny write paths - block commands that write to protected directories + if len(t.denyWritePaths) > 0 { + words := strings.Fields(cmd) + for i, word := range words { + for _, pattern := range t.denyWritePaths { + if pattern.MatchString(word) { + return fmt.Sprintf("Command blocked: cannot write to %s (access denied)", word) + } + // Also check path components like "skills" in "mkdir -p skills/my_skill" + if i >= 0 && (word == "-p" || word == "-rf" || word == "-r") { + continue + } + pathParts := strings.Split(word, "/") + for _, part := range pathParts { + if pattern.MatchString(part) { + return fmt.Sprintf("Command blocked: cannot write to %s (access denied)", part) + } + } + } + } + } } if len(t.allowPatterns) > 0 { diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index a8de2f4c9..f5284139e 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "os" "path/filepath" + "regexp" "runtime" "strings" "testing" @@ -1613,3 +1614,54 @@ func TestEncodeKeyTokenWithPtyKeyMode(t *testing.T) { }) } } + +func TestShellTool_DenyWritePaths(t *testing.T) { + tests := []struct { + name string + command string + denyPaths []*regexp.Regexp + expectBlock bool + }{ + { + name: "mkdir blocked", + command: "mkdir -p skills", + denyPaths: []*regexp.Regexp{regexp.MustCompile(`^skills(/.*)?$`)}, + expectBlock: true, + }, + { + name: "mkdir -p blocked", + command: "mkdir -p skills/my_skill", + denyPaths: []*regexp.Regexp{regexp.MustCompile(`^skills(/.*)?$`)}, + expectBlock: true, + }, + { + name: "mkdir allowed", + command: "mkdir -p workspace/data", + denyPaths: []*regexp.Regexp{regexp.MustCompile(`^skills(/.*)?$`)}, + expectBlock: false, + }, + { + name: "touch skills file blocked", + command: "touch skills/test.txt", + denyPaths: []*regexp.Regexp{regexp.MustCompile(`^skills(/.*)?$`)}, + expectBlock: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tool, err := NewExecToolWithDenyPaths("", false, nil, tt.denyPaths, nil) + require.NoError(t, err) + result := tool.Execute(context.Background(), map[string]any{ + "action": "run", + "command": tt.command, + }) + if tt.expectBlock { + require.True(t, result.IsError, "expected block for command: %s", tt.command) + require.Contains(t, result.ForLLM, "access denied") + } else { + require.False(t, result.IsError, "expected allow for command: %s, got: %s", tt.command, result.ForLLM) + } + }) + } +}