From 10884f7f815075d1c4834308f0e2fbe12b2a9acf Mon Sep 17 00:00:00 2001 From: Huang Rui Date: Mon, 2 Mar 2026 11:51:05 +0800 Subject: [PATCH] feat(tools): add configurable allow patterns and path whitelists - Add custom_allow_patterns to exec config so users can exempt specific commands from deny pattern checks - Add allow_read_paths and allow_write_paths regex lists to tools config for whitelisting specific paths outside the workspace - Introduce whitelistFs that wraps sandboxFs and falls through to hostFs for paths matching whitelist patterns - Use variadic constructor signatures to keep backward compatibility Suggested-by: lxowalle Signed-off-by: Huang Rui --- pkg/agent/instance.go | 30 ++++++++++-- pkg/config/config.go | 17 ++++--- pkg/tools/edit.go | 25 +++++----- pkg/tools/filesystem.go | 88 +++++++++++++++++++++++++++--------- pkg/tools/filesystem_test.go | 34 ++++++++++++++ pkg/tools/shell.go | 25 +++++++++- pkg/tools/shell_test.go | 36 +++++++++++++++ 7 files changed, 206 insertions(+), 49 deletions(-) diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index 796ce2dea..ed438059f 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -1,9 +1,11 @@ package agent import ( + "fmt" "log" "os" "path/filepath" + "regexp" "strings" "github.com/sipeed/picoclaw/pkg/config" @@ -49,18 +51,23 @@ func NewAgentInstance( restrict := defaults.RestrictToWorkspace readRestrict := restrict && !defaults.AllowReadOutsideWorkspace + + // Compile path whitelist patterns from config. + allowReadPaths := compilePatterns(cfg.Tools.AllowReadPaths) + allowWritePaths := compilePatterns(cfg.Tools.AllowWritePaths) + toolsRegistry := tools.NewToolRegistry() - toolsRegistry.Register(tools.NewReadFileTool(workspace, readRestrict)) - toolsRegistry.Register(tools.NewWriteFileTool(workspace, restrict)) - toolsRegistry.Register(tools.NewListDirTool(workspace, readRestrict)) + toolsRegistry.Register(tools.NewReadFileTool(workspace, readRestrict, allowReadPaths)) + toolsRegistry.Register(tools.NewWriteFileTool(workspace, restrict, allowWritePaths)) + toolsRegistry.Register(tools.NewListDirTool(workspace, readRestrict, allowReadPaths)) execTool, err := tools.NewExecToolWithConfig(workspace, restrict, cfg) if err != nil { log.Fatalf("Critical error: unable to initialize exec tool: %v", err) } toolsRegistry.Register(execTool) - toolsRegistry.Register(tools.NewEditFileTool(workspace, restrict)) - toolsRegistry.Register(tools.NewAppendFileTool(workspace, restrict)) + toolsRegistry.Register(tools.NewEditFileTool(workspace, restrict, allowWritePaths)) + toolsRegistry.Register(tools.NewAppendFileTool(workspace, restrict, allowWritePaths)) sessionsDir := filepath.Join(workspace, "sessions") sessionsManager := session.NewSessionManager(sessionsDir) @@ -190,6 +197,19 @@ func resolveAgentFallbacks(agentCfg *config.AgentConfig, defaults *config.AgentD return defaults.ModelFallbacks } +func compilePatterns(patterns []string) []*regexp.Regexp { + compiled := make([]*regexp.Regexp, 0, len(patterns)) + for _, p := range patterns { + re, err := regexp.Compile(p) + if err != nil { + fmt.Printf("Warning: invalid path pattern %q: %v\n", p, err) + continue + } + compiled = append(compiled, re) + } + return compiled +} + func expandHome(path string) string { if path == "" { return path diff --git a/pkg/config/config.go b/pkg/config/config.go index ff37b7ac4..9f4769de4 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -533,8 +533,9 @@ type CronToolsConfig struct { } type ExecConfig struct { - EnableDenyPatterns bool `json:"enable_deny_patterns" env:"PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS"` - CustomDenyPatterns []string `json:"custom_deny_patterns" env:"PICOCLAW_TOOLS_EXEC_CUSTOM_DENY_PATTERNS"` + EnableDenyPatterns bool `json:"enable_deny_patterns" env:"PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS"` + CustomDenyPatterns []string `json:"custom_deny_patterns" env:"PICOCLAW_TOOLS_EXEC_CUSTOM_DENY_PATTERNS"` + CustomAllowPatterns []string `json:"custom_allow_patterns" env:"PICOCLAW_TOOLS_EXEC_CUSTOM_ALLOW_PATTERNS"` } type MediaCleanupConfig struct { @@ -544,11 +545,13 @@ type MediaCleanupConfig struct { } type ToolsConfig struct { - Web WebToolsConfig `json:"web"` - Cron CronToolsConfig `json:"cron"` - Exec ExecConfig `json:"exec"` - Skills SkillsToolsConfig `json:"skills"` - MediaCleanup MediaCleanupConfig `json:"media_cleanup"` + AllowReadPaths []string `json:"allow_read_paths" env:"PICOCLAW_TOOLS_ALLOW_READ_PATHS"` + AllowWritePaths []string `json:"allow_write_paths" env:"PICOCLAW_TOOLS_ALLOW_WRITE_PATHS"` + Web WebToolsConfig `json:"web"` + Cron CronToolsConfig `json:"cron"` + Exec ExecConfig `json:"exec"` + Skills SkillsToolsConfig `json:"skills"` + MediaCleanup MediaCleanupConfig `json:"media_cleanup"` } type SkillsToolsConfig struct { diff --git a/pkg/tools/edit.go b/pkg/tools/edit.go index d3ab267bf..d5bebf4a2 100644 --- a/pkg/tools/edit.go +++ b/pkg/tools/edit.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io/fs" + "regexp" "strings" ) @@ -15,14 +16,12 @@ type EditFileTool struct { } // NewEditFileTool creates a new EditFileTool with optional directory restriction. -func NewEditFileTool(workspace string, restrict bool) *EditFileTool { - var fs fileSystem - if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} +func NewEditFileTool(workspace string, restrict bool, allowPaths ...[]*regexp.Regexp) *EditFileTool { + var patterns []*regexp.Regexp + if len(allowPaths) > 0 { + patterns = allowPaths[0] } - return &EditFileTool{fs: fs} + return &EditFileTool{fs: buildFs(workspace, restrict, patterns)} } func (t *EditFileTool) Name() string { @@ -80,14 +79,12 @@ type AppendFileTool struct { fs fileSystem } -func NewAppendFileTool(workspace string, restrict bool) *AppendFileTool { - var fs fileSystem - if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} +func NewAppendFileTool(workspace string, restrict bool, allowPaths ...[]*regexp.Regexp) *AppendFileTool { + var patterns []*regexp.Regexp + if len(allowPaths) > 0 { + patterns = allowPaths[0] } - return &AppendFileTool{fs: fs} + return &AppendFileTool{fs: buildFs(workspace, restrict, patterns)} } func (t *AppendFileTool) Name() string { diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 03d461dcc..cd8da3195 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -6,6 +6,7 @@ import ( "io/fs" "os" "path/filepath" + "regexp" "strings" "time" @@ -87,14 +88,12 @@ type ReadFileTool struct { fs fileSystem } -func NewReadFileTool(workspace string, restrict bool) *ReadFileTool { - var fs fileSystem - if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} +func NewReadFileTool(workspace string, restrict bool, allowPaths ...[]*regexp.Regexp) *ReadFileTool { + var patterns []*regexp.Regexp + if len(allowPaths) > 0 { + patterns = allowPaths[0] } - return &ReadFileTool{fs: fs} + return &ReadFileTool{fs: buildFs(workspace, restrict, patterns)} } func (t *ReadFileTool) Name() string { @@ -135,14 +134,12 @@ type WriteFileTool struct { fs fileSystem } -func NewWriteFileTool(workspace string, restrict bool) *WriteFileTool { - var fs fileSystem - if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} +func NewWriteFileTool(workspace string, restrict bool, allowPaths ...[]*regexp.Regexp) *WriteFileTool { + var patterns []*regexp.Regexp + if len(allowPaths) > 0 { + patterns = allowPaths[0] } - return &WriteFileTool{fs: fs} + return &WriteFileTool{fs: buildFs(workspace, restrict, patterns)} } func (t *WriteFileTool) Name() string { @@ -192,14 +189,12 @@ type ListDirTool struct { fs fileSystem } -func NewListDirTool(workspace string, restrict bool) *ListDirTool { - var fs fileSystem - if restrict { - fs = &sandboxFs{workspace: workspace} - } else { - fs = &hostFs{} +func NewListDirTool(workspace string, restrict bool, allowPaths ...[]*regexp.Regexp) *ListDirTool { + var patterns []*regexp.Regexp + if len(allowPaths) > 0 { + patterns = allowPaths[0] } - return &ListDirTool{fs: fs} + return &ListDirTool{fs: buildFs(workspace, restrict, patterns)} } func (t *ListDirTool) Name() string { @@ -394,6 +389,57 @@ func (r *sandboxFs) ReadDir(path string) ([]os.DirEntry, error) { return entries, err } +// whitelistFs wraps a sandboxFs and allows access to specific paths outside +// the workspace when they match any of the provided patterns. +type whitelistFs struct { + sandbox *sandboxFs + host hostFs + patterns []*regexp.Regexp +} + +func (w *whitelistFs) matches(path string) bool { + for _, p := range w.patterns { + if p.MatchString(path) { + return true + } + } + return false +} + +func (w *whitelistFs) ReadFile(path string) ([]byte, error) { + if w.matches(path) { + return w.host.ReadFile(path) + } + return w.sandbox.ReadFile(path) +} + +func (w *whitelistFs) WriteFile(path string, data []byte) error { + if w.matches(path) { + return w.host.WriteFile(path, data) + } + return w.sandbox.WriteFile(path, data) +} + +func (w *whitelistFs) ReadDir(path string) ([]os.DirEntry, error) { + if w.matches(path) { + return w.host.ReadDir(path) + } + return w.sandbox.ReadDir(path) +} + +// buildFs returns the appropriate fileSystem implementation based on restriction +// settings and optional path whitelist patterns. +func buildFs(workspace string, restrict bool, patterns []*regexp.Regexp) fileSystem { + if !restrict { + return &hostFs{} + } + sandbox := &sandboxFs{workspace: workspace} + if len(patterns) > 0 { + return &whitelistFs{sandbox: sandbox, patterns: patterns} + } + return sandbox +} + // Helper to get a safe relative path for os.Root usage func getSafeRelPath(workspace, path string) (string, error) { if workspace == "" { diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index 6f896e22d..666004cd4 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -5,6 +5,7 @@ import ( "io" "os" "path/filepath" + "regexp" "strings" "testing" @@ -486,3 +487,36 @@ func TestRootRW_Write(t *testing.T) { assert.NoError(t, err) assert.Equal(t, newData, content) } + +// TestWhitelistFs_AllowsMatchingPaths verifies that whitelistFs allows access to +// paths matching the whitelist patterns while blocking non-matching paths. +func TestWhitelistFs_AllowsMatchingPaths(t *testing.T) { + workspace := t.TempDir() + outsideDir := t.TempDir() + outsideFile := filepath.Join(outsideDir, "allowed.txt") + os.WriteFile(outsideFile, []byte("outside content"), 0o644) + + // Pattern allows access to the outsideDir. + patterns := []*regexp.Regexp{regexp.MustCompile(`^` + regexp.QuoteMeta(outsideDir))} + + tool := NewReadFileTool(workspace, true, patterns) + + // Read from whitelisted path should succeed. + result := tool.Execute(context.Background(), map[string]any{"path": outsideFile}) + if result.IsError { + t.Errorf("expected whitelisted path to be readable, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "outside content") { + t.Errorf("expected file content, got: %s", result.ForLLM) + } + + // Read from non-whitelisted path outside workspace should fail. + otherDir := t.TempDir() + otherFile := filepath.Join(otherDir, "blocked.txt") + os.WriteFile(otherFile, []byte("blocked"), 0o644) + + result = tool.Execute(context.Background(), map[string]any{"path": otherFile}) + if !result.IsError { + t.Errorf("expected non-whitelisted path to be blocked, got: %s", result.ForLLM) + } +} diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 6b1638105..a0c83eb1e 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -21,6 +21,7 @@ type ExecTool struct { timeout time.Duration denyPatterns []*regexp.Regexp allowPatterns []*regexp.Regexp + customAllowPatterns []*regexp.Regexp restrictToWorkspace bool } @@ -98,6 +99,7 @@ func NewExecTool(workingDir string, restrict bool) (*ExecTool, error) { func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Config) (*ExecTool, error) { denyPatterns := make([]*regexp.Regexp, 0) + customAllowPatterns := make([]*regexp.Regexp, 0) if config != nil { execConfig := config.Tools.Exec @@ -118,6 +120,13 @@ func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Conf // 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 { + re, err := regexp.Compile(pattern) + if err != nil { + return nil, fmt.Errorf("invalid custom allow pattern %q: %w", pattern, err) + } + customAllowPatterns = append(customAllowPatterns, re) + } } else { denyPatterns = append(denyPatterns, defaultDenyPatterns...) } @@ -127,6 +136,7 @@ func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Conf timeout: 60 * time.Second, denyPatterns: denyPatterns, allowPatterns: nil, + customAllowPatterns: customAllowPatterns, restrictToWorkspace: restrict, }, nil } @@ -281,9 +291,20 @@ func (t *ExecTool) guardCommand(command, cwd string) string { cmd := strings.TrimSpace(command) lower := strings.ToLower(cmd) - for _, pattern := range t.denyPatterns { + // Custom allow patterns exempt a command from deny checks. + explicitlyAllowed := false + for _, pattern := range t.customAllowPatterns { if pattern.MatchString(lower) { - return "Command blocked by safety guard (dangerous pattern detected)" + explicitlyAllowed = true + break + } + } + + if !explicitlyAllowed { + for _, pattern := range t.denyPatterns { + if pattern.MatchString(lower) { + return "Command blocked by safety guard (dangerous pattern detected)" + } } } diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index 91f93039c..a6abca8ea 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -7,6 +7,8 @@ import ( "strings" "testing" "time" + + "github.com/sipeed/picoclaw/pkg/config" ) // TestShellTool_Success verifies successful command execution @@ -387,3 +389,37 @@ func TestShellTool_SafePathsInWorkspaceRestriction(t *testing.T) { } } } + +// TestShellTool_CustomAllowPatterns verifies that custom allow patterns exempt +// commands from deny pattern checks. +func TestShellTool_CustomAllowPatterns(t *testing.T) { + cfg := &config.Config{ + Tools: config.ToolsConfig{ + Exec: config.ExecConfig{ + EnableDenyPatterns: true, + CustomAllowPatterns: []string{`\bgit\s+push\s+origin\b`}, + }, + }, + } + + tool, err := NewExecToolWithConfig("", false, cfg) + if err != nil { + t.Fatalf("unable to configure exec tool: %s", err) + } + + // "git push origin main" should be allowed by custom allow pattern. + result := tool.Execute(context.Background(), map[string]any{ + "command": "git push origin main", + }) + if result.IsError && strings.Contains(result.ForLLM, "blocked") { + t.Errorf("custom allow pattern should exempt 'git push origin main', got: %s", result.ForLLM) + } + + // "git push upstream main" should still be blocked (does not match allow pattern). + result = tool.Execute(context.Background(), map[string]any{ + "command": "git push upstream main", + }) + if !result.IsError { + t.Errorf("'git push upstream main' should still be blocked by deny pattern") + } +}