From 381270c323aa614457eaba7275303a2c1bbbc26c Mon Sep 17 00:00:00 2001 From: afjcjsbx Date: Wed, 25 Feb 2026 18:20:40 +0100 Subject: [PATCH 1/2] fix(exec) fail close on invalid deny pattern --- cmd/picoclaw/internal/gateway/helpers.go | 6 ++- pkg/agent/instance.go | 8 +++- pkg/tools/cron.go | 10 ++-- pkg/tools/shell.go | 9 ++-- pkg/tools/shell_test.go | 59 +++++++++++++++++++----- pkg/tools/shell_timeout_unix_test.go | 2 +- 6 files changed, 72 insertions(+), 22 deletions(-) diff --git a/cmd/picoclaw/internal/gateway/helpers.go b/cmd/picoclaw/internal/gateway/helpers.go index a06625dc9..52adc6e85 100644 --- a/cmd/picoclaw/internal/gateway/helpers.go +++ b/cmd/picoclaw/internal/gateway/helpers.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "log" "net/http" "os" "os/signal" @@ -232,7 +233,10 @@ func setupCronTool( cronService := cron.NewCronService(cronStorePath, nil) // Create and register CronTool - cronTool := tools.NewCronTool(cronService, agentLoop, msgBus, workspace, restrict, execTimeout, cfg) + cronTool, err := tools.NewCronTool(cronService, agentLoop, msgBus, workspace, restrict, execTimeout, cfg) + if err != nil { + log.Fatalf("Critical error during CronTool initialization: %v", err) + } agentLoop.RegisterTool(cronTool) // Set the onJob handler diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index a6fd365c7..302d21f8e 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -1,6 +1,7 @@ package agent import ( + "log" "os" "path/filepath" "strings" @@ -51,7 +52,12 @@ func NewAgentInstance( toolsRegistry.Register(tools.NewReadFileTool(workspace, restrict)) toolsRegistry.Register(tools.NewWriteFileTool(workspace, restrict)) toolsRegistry.Register(tools.NewListDirTool(workspace, restrict)) - toolsRegistry.Register(tools.NewExecToolWithConfig(workspace, restrict, cfg)) + 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)) diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index 562fffc84..01007af95 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -33,15 +33,19 @@ type CronTool struct { func NewCronTool( cronService *cron.CronService, executor JobExecutor, msgBus *bus.MessageBus, workspace string, restrict bool, execTimeout time.Duration, config *config.Config, -) *CronTool { - execTool := NewExecToolWithConfig(workspace, restrict, config) +) (*CronTool, error) { + execTool, err := NewExecToolWithConfig(workspace, restrict, config) + if err != nil { + return nil, fmt.Errorf("unable to configure exec tool: %w", err) + } + execTool.SetTimeout(execTimeout) return &CronTool{ cronService: cronService, executor: executor, msgBus: msgBus, execTool: execTool, - } + }, nil } // Name returns the tool name diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index ad1664b5b..b52433b6f 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -69,11 +69,11 @@ var defaultDenyPatterns = []*regexp.Regexp{ regexp.MustCompile(`\bsource\s+.*\.sh\b`), } -func NewExecTool(workingDir string, restrict bool) *ExecTool { +func NewExecTool(workingDir string, restrict bool) (*ExecTool, error) { return NewExecToolWithConfig(workingDir, restrict, nil) } -func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Config) *ExecTool { +func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Config) (*ExecTool, error) { denyPatterns := make([]*regexp.Regexp, 0) if config != nil { @@ -86,8 +86,7 @@ func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Conf for _, pattern := range execConfig.CustomDenyPatterns { re, err := regexp.Compile(pattern) if err != nil { - fmt.Printf("Invalid custom deny pattern %q: %v\n", pattern, err) - continue + return nil, fmt.Errorf("invalid custom deny pattern %q: %w", pattern, err) } denyPatterns = append(denyPatterns, re) } @@ -106,7 +105,7 @@ func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Conf denyPatterns: denyPatterns, allowPatterns: nil, restrictToWorkspace: restrict, - } + }, nil } func (t *ExecTool) Name() string { diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index 6d35815e8..1a179547a 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -11,7 +11,10 @@ import ( // TestShellTool_Success verifies successful command execution func TestShellTool_Success(t *testing.T) { - tool := NewExecTool("", false) + tool, err := NewExecTool("", false) + if err != nil { + t.Errorf("unable to configure exec tool: %s", err) + } ctx := context.Background() args := map[string]any{ @@ -38,7 +41,10 @@ func TestShellTool_Success(t *testing.T) { // TestShellTool_Failure verifies failed command execution func TestShellTool_Failure(t *testing.T) { - tool := NewExecTool("", false) + tool, err := NewExecTool("", false) + if err != nil { + t.Errorf("unable to configure exec tool: %s", err) + } ctx := context.Background() args := map[string]any{ @@ -65,7 +71,11 @@ func TestShellTool_Failure(t *testing.T) { // TestShellTool_Timeout verifies command timeout handling func TestShellTool_Timeout(t *testing.T) { - tool := NewExecTool("", false) + tool, err := NewExecTool("", false) + if err != nil { + t.Errorf("unable to configure exec tool: %s", err) + } + tool.SetTimeout(100 * time.Millisecond) ctx := context.Background() @@ -93,7 +103,10 @@ func TestShellTool_WorkingDir(t *testing.T) { testFile := filepath.Join(tmpDir, "test.txt") os.WriteFile(testFile, []byte("test content"), 0o644) - tool := NewExecTool("", false) + tool, err := NewExecTool("", false) + if err != nil { + t.Errorf("unable to configure exec tool: %s", err) + } ctx := context.Background() args := map[string]any{ @@ -114,7 +127,10 @@ func TestShellTool_WorkingDir(t *testing.T) { // TestShellTool_DangerousCommand verifies safety guard blocks dangerous commands func TestShellTool_DangerousCommand(t *testing.T) { - tool := NewExecTool("", false) + tool, err := NewExecTool("", false) + if err != nil { + t.Errorf("unable to configure exec tool: %s", err) + } ctx := context.Background() args := map[string]any{ @@ -135,7 +151,10 @@ func TestShellTool_DangerousCommand(t *testing.T) { // TestShellTool_MissingCommand verifies error handling for missing command func TestShellTool_MissingCommand(t *testing.T) { - tool := NewExecTool("", false) + tool, err := NewExecTool("", false) + if err != nil { + t.Errorf("unable to configure exec tool: %s", err) + } ctx := context.Background() args := map[string]any{} @@ -150,7 +169,10 @@ func TestShellTool_MissingCommand(t *testing.T) { // TestShellTool_StderrCapture verifies stderr is captured and included func TestShellTool_StderrCapture(t *testing.T) { - tool := NewExecTool("", false) + tool, err := NewExecTool("", false) + if err != nil { + t.Errorf("unable to configure exec tool: %s", err) + } ctx := context.Background() args := map[string]any{ @@ -170,7 +192,10 @@ func TestShellTool_StderrCapture(t *testing.T) { // TestShellTool_OutputTruncation verifies long output is truncated func TestShellTool_OutputTruncation(t *testing.T) { - tool := NewExecTool("", false) + tool, err := NewExecTool("", false) + if err != nil { + t.Errorf("unable to configure exec tool: %s", err) + } ctx := context.Background() // Generate long output (>10000 chars) @@ -198,7 +223,11 @@ func TestShellTool_WorkingDir_OutsideWorkspace(t *testing.T) { t.Fatalf("failed to create outside dir: %v", err) } - tool := NewExecTool(workspace, true) + tool, err := NewExecTool(workspace, true) + if err != nil { + t.Errorf("unable to configure exec tool: %s", err) + } + result := tool.Execute(context.Background(), map[string]any{ "command": "pwd", "working_dir": outsideDir, @@ -232,7 +261,11 @@ func TestShellTool_WorkingDir_SymlinkEscape(t *testing.T) { t.Skipf("symlinks not supported in this environment: %v", err) } - tool := NewExecTool(workspace, true) + tool, err := NewExecTool(workspace, true) + if err != nil { + t.Errorf("unable to configure exec tool: %s", err) + } + result := tool.Execute(context.Background(), map[string]any{ "command": "cat secret.txt", "working_dir": link, @@ -249,7 +282,11 @@ func TestShellTool_WorkingDir_SymlinkEscape(t *testing.T) { // TestShellTool_RestrictToWorkspace verifies workspace restriction func TestShellTool_RestrictToWorkspace(t *testing.T) { tmpDir := t.TempDir() - tool := NewExecTool(tmpDir, false) + tool, err := NewExecTool(tmpDir, false) + if err != nil { + t.Errorf("unable to configure exec tool: %s", err) + } + tool.SetRestrictToWorkspace(true) ctx := context.Background() diff --git a/pkg/tools/shell_timeout_unix_test.go b/pkg/tools/shell_timeout_unix_test.go index 04ef8e441..d11fe34b7 100644 --- a/pkg/tools/shell_timeout_unix_test.go +++ b/pkg/tools/shell_timeout_unix_test.go @@ -22,7 +22,7 @@ func processExists(pid int) bool { } func TestShellTool_TimeoutKillsChildProcess(t *testing.T) { - tool := NewExecTool(t.TempDir(), false) + tool, err := NewExecTool(t.TempDir(), false) tool.SetTimeout(500 * time.Millisecond) args := map[string]any{ From 36320767fb7a615ce36d9e9118cb197e82903d66 Mon Sep 17 00:00:00 2001 From: afjcjsbx Date: Thu, 26 Feb 2026 11:17:59 +0100 Subject: [PATCH 2/2] fix: error check --- pkg/tools/shell_timeout_unix_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/tools/shell_timeout_unix_test.go b/pkg/tools/shell_timeout_unix_test.go index d11fe34b7..357e1276e 100644 --- a/pkg/tools/shell_timeout_unix_test.go +++ b/pkg/tools/shell_timeout_unix_test.go @@ -23,6 +23,10 @@ func processExists(pid int) bool { func TestShellTool_TimeoutKillsChildProcess(t *testing.T) { tool, err := NewExecTool(t.TempDir(), false) + if err != nil { + t.Errorf("unable to configure exec tool: %s", err) + } + tool.SetTimeout(500 * time.Millisecond) args := map[string]any{