From de42aa526f58d945e1777cb0c59910cc0af39385 Mon Sep 17 00:00:00 2001 From: Boris Bliznioukov Date: Wed, 4 Mar 2026 21:29:23 +0100 Subject: [PATCH] feat(security): enhance sandboxing and risk classification for shell commands; add Windows-specific tests and deprecate old config fields Signed-off-by: Boris Bliznioukov --- docs/tools_configuration.md | 6 +- pkg/config/config.go | 6 +- .../sources/openclaw/openclaw_config.go | 5 - pkg/tools/shell/env.go | 25 +++- pkg/tools/shell/env_windows_test.go | 52 ++++++++ pkg/tools/shell/risk.go | 69 +++++++--- pkg/tools/shell/runner.go | 19 +-- pkg/tools/shell/runner_test.go | 101 ++++++++++++++ pkg/tools/shell/runner_windows_test.go | 122 +++++++++++++++++ pkg/tools/shell/sandbox.go | 21 ++- pkg/tools/shell/sandbox_test.go | 12 +- pkg/tools/shell_tool.go | 12 ++ pkg/tools/shell_tool_test.go | 125 ++++++++++++++++++ 13 files changed, 528 insertions(+), 47 deletions(-) create mode 100644 pkg/tools/shell/env_windows_test.go create mode 100644 pkg/tools/shell/runner_windows_test.go diff --git a/docs/tools_configuration.md b/docs/tools_configuration.md index f5d53f6a5..7c2aa7436 100644 --- a/docs/tools_configuration.md +++ b/docs/tools_configuration.md @@ -120,7 +120,11 @@ of variables is exposed (e.g., `PATH`, `HOME`, `LANG`, `TERM`). ### File-Access Sandboxing When `restrict_to_workspace` is enabled (the default), the interpreter's -`OpenHandler` blocks reads and writes outside the configured workspace directory. +`OpenHandler` blocks reads and writes outside the configured workspace directory for shell-managed redirections (`>`, `<`, `>>`). + +> NOTE: This is not a general filesystem sandbox. External programs invoked by the +> shell can still perform arbitrary file I/O via their own syscalls; only +> shell-level redirections are constrained by `OpenHandler`. ### Cron Integration diff --git a/pkg/config/config.go b/pkg/config/config.go index 3584f5d3f..42a9d5bf3 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -616,8 +616,10 @@ type ExecConfig struct { EnvSet map[string]string `json:"env_set" env:"PICOCLAW_TOOLS_EXEC_ENV_SET"` // explicit var=value pairs // Deprecated: these fields are ignored. See risk_threshold and risk_overrides. - EnableDenyPatterns bool `json:"enable_deny_patterns,omitempty" env:"PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS"` - CustomDenyPatterns []string `json:"custom_deny_patterns,omitempty" env:"PICOCLAW_TOOLS_EXEC_CUSTOM_DENY_PATTERNS"` + EnableDenyPatterns *bool `json:"enable_deny_patterns,omitempty" env:"PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS"` + // Deprecated: these fields are ignored. See risk_threshold and risk_overrides. + CustomDenyPatterns []string `json:"custom_deny_patterns,omitempty" env:"PICOCLAW_TOOLS_EXEC_CUSTOM_DENY_PATTERNS"` + // Deprecated: these fields are ignored. See risk_threshold and risk_overrides. CustomAllowPatterns []string `json:"custom_allow_patterns,omitempty" env:"PICOCLAW_TOOLS_EXEC_CUSTOM_ALLOW_PATTERNS"` } diff --git a/pkg/migrate/sources/openclaw/openclaw_config.go b/pkg/migrate/sources/openclaw/openclaw_config.go index 39ad48fad..1717304f9 100644 --- a/pkg/migrate/sources/openclaw/openclaw_config.go +++ b/pkg/migrate/sources/openclaw/openclaw_config.go @@ -546,7 +546,6 @@ func (c *OpenClawConfig) ConvertToPicoClaw(sourceHome string) (*PicoClawConfig, "Auth profiles (API keys, OAuth tokens) not migrated for security - set env vars manually", ) } - return cfg, warnings, nil } @@ -1066,9 +1065,5 @@ func (c ToolsConfig) ToStandardTools() config.ToolsConfig { Cron: config.CronToolsConfig{ ExecTimeoutMinutes: c.Cron.ExecTimeoutMinutes, }, - Exec: config.ExecConfig{ - EnableDenyPatterns: c.Exec.EnableDenyPatterns, - CustomDenyPatterns: c.Exec.CustomDenyPatterns, - }, } } diff --git a/pkg/tools/shell/env.go b/pkg/tools/shell/env.go index e3004c8c2..e5c89716b 100644 --- a/pkg/tools/shell/env.go +++ b/pkg/tools/shell/env.go @@ -56,15 +56,15 @@ var windowsEnvAllowlist = map[string]bool{ func BuildSanitizedEnv(extraAllowlist []string, envSet map[string]string) expand.Environ { allowed := make(map[string]bool, len(DefaultEnvAllowlist)+len(extraAllowlist)+len(windowsEnvAllowlist)) for k := range DefaultEnvAllowlist { - allowed[k] = true + allowed[envKey(k)] = true } if runtime.GOOS == "windows" { for k := range windowsEnvAllowlist { - allowed[k] = true + allowed[envKey(k)] = true } } for _, k := range extraAllowlist { - allowed[k] = true + allowed[envKey(k)] = true } vars := make(map[string]string, len(allowed)+len(envSet)) @@ -74,18 +74,29 @@ func BuildSanitizedEnv(extraAllowlist []string, envSet map[string]string) expand if !ok { continue } - if allowed[k] || isAllowedPrefix(k) { - vars[k] = v + norm := envKey(k) + if allowed[norm] || isAllowedPrefix(norm) { + vars[norm] = v } } for k, v := range envSet { - vars[k] = v + vars[envKey(k)] = v } return &sanitizedEnv{vars: vars} } +// envKey normalizes an environment variable name. On Windows, where env +// vars are case-insensitive, it uppercases the key so that "Path" and +// "PATH" map to the same entry. On other platforms it's a no-op. +func envKey(k string) string { + if runtime.GOOS == "windows" { + return strings.ToUpper(k) + } + return k +} + func isAllowedPrefix(name string) bool { for _, prefix := range defaultEnvAllowPrefixes { if strings.HasPrefix(name, prefix) { @@ -101,7 +112,7 @@ type sanitizedEnv struct { } func (e *sanitizedEnv) Get(name string) expand.Variable { - val, ok := e.vars[name] + val, ok := e.vars[envKey(name)] if !ok { return expand.Variable{} } diff --git a/pkg/tools/shell/env_windows_test.go b/pkg/tools/shell/env_windows_test.go new file mode 100644 index 000000000..636eba289 --- /dev/null +++ b/pkg/tools/shell/env_windows_test.go @@ -0,0 +1,52 @@ +package shell + +import ( + "testing" +) + +func TestBuildSanitizedEnv_WindowsCaseInsensitive(t *testing.T) { + // On Windows the OS typically stores "Path" not "PATH". + // Verify that mixed-case inherited vars still pass the allowlist. + t.Setenv("Path", `C:\Windows\system32`) + + env := BuildSanitizedEnv(nil, nil) + + v := env.Get("PATH") + if !v.IsSet() { + t.Fatal("expected PATH to be present when OS provides 'Path'") + } + if v.Str != `C:\Windows\system32` { + t.Errorf("PATH = %q, want %q", v.Str, `C:\Windows\system32`) + } + + // Also verify lookup with original casing works. + v2 := env.Get("Path") + if !v2.IsSet() { + t.Fatal("expected Get('Path') to resolve via case-insensitive lookup") + } +} + +func TestBuildSanitizedEnv_WindowsEnvSetCaseInsensitive(t *testing.T) { + env := BuildSanitizedEnv(nil, map[string]string{ + "path": `C:\custom\bin`, + }) + + v := env.Get("PATH") + if !v.IsSet() { + t.Fatal("expected PATH to be set via lowercase 'path' envSet key") + } + if v.Str != `C:\custom\bin` { + t.Errorf("PATH = %q, want %q", v.Str, `C:\custom\bin`) + } +} + +func TestBuildSanitizedEnv_WindowsExtraAllowlistCaseInsensitive(t *testing.T) { + t.Setenv("my_custom_var", "hello") + + env := BuildSanitizedEnv([]string{"MY_CUSTOM_VAR"}, nil) + + v := env.Get("my_custom_var") + if !v.IsSet() { + t.Fatal("expected my_custom_var to be found via case-insensitive allowlist") + } +} diff --git a/pkg/tools/shell/risk.go b/pkg/tools/shell/risk.go index 6c7c486bc..7b6ab130c 100644 --- a/pkg/tools/shell/risk.go +++ b/pkg/tools/shell/risk.go @@ -446,31 +446,62 @@ func IsAllowed(level, threshold RiskLevel) bool { return level <= threshold } -// BlockedCommandError formats a structured error message for the LLM. -func BlockedCommandError(args []string, level, threshold RiskLevel, reason string) string { - cmd := "" - if len(args) > 0 { - cmd = args[0] - if len(args) > 1 { - end := len(args) - if end > 5 { - end = 5 - } - for _, a := range args[1:end] { - cmd += " " + a - } - if len(args) > 5 { - cmd += " ..." - } - } - } +// BlockedError is returned when a command is blocked by the risk classifier. +type BlockedError struct { + Command string + Level RiskLevel + Threshold RiskLevel + Reason string +} +func (e *BlockedError) Error() string { return fmt.Sprintf( "Command blocked by risk classifier: command=%q risk_level=%s threshold=%s reason=%s", - cmd, level, threshold, reason, + e.Command, e.Level, e.Threshold, e.Reason, ) } +// BlockedCommandError formats a structured error message for the LLM. +// Deprecated: use BlockedError directly. +func BlockedCommandError(args []string, level, threshold RiskLevel, reason string) string { + return (&BlockedError{ + Command: formatCommand(args), + Level: level, + Threshold: threshold, + Reason: reason, + }).Error() +} + +// NewBlockedError constructs a BlockedError from a command's args. +func NewBlockedError(args []string, level, threshold RiskLevel, reason string) *BlockedError { + return &BlockedError{ + Command: formatCommand(args), + Level: level, + Threshold: threshold, + Reason: reason, + } +} + +func formatCommand(args []string) string { + if len(args) == 0 { + return "" + } + cmd := args[0] + if len(args) > 1 { + end := len(args) + if end > 5 { + end = 5 + } + for _, a := range args[1:end] { + cmd += " " + a + } + if len(args) > 5 { + cmd += " ..." + } + } + return cmd +} + // baseCommand extracts the basename from a command path. // On Windows, it additionally lowercases the name and strips known // executable extensions (.exe, .cmd, .bat, .com) so that diff --git a/pkg/tools/shell/runner.go b/pkg/tools/shell/runner.go index f807ac780..85138e73f 100644 --- a/pkg/tools/shell/runner.go +++ b/pkg/tools/shell/runner.go @@ -3,6 +3,7 @@ package shell import ( "bytes" "context" + "errors" "fmt" "os" "path/filepath" @@ -97,14 +98,17 @@ func Run(ctx context.Context, cfg RunConfig) RunResult { } if err != nil { - if runCtx.Err() == context.DeadlineExceeded { - msg := fmt.Sprintf("Command timed out after %v", cfg.Timeout) - return RunResult{Output: msg, IsError: true} + if runCtx.Err() != nil { + if runCtx.Err() == context.DeadlineExceeded { + msg := fmt.Sprintf("Command timed out after %v", cfg.Timeout) + return RunResult{Output: msg, IsError: true} + } + return RunResult{Output: "Command canceled", IsError: true} } - errStr := err.Error() - if strings.Contains(errStr, "Command blocked by risk classifier") { - return RunResult{Output: errStr, IsError: true} + var blocked *BlockedError + if errors.As(err, &blocked) { + return RunResult{Output: blocked.Error(), IsError: true} } output += fmt.Sprintf("\nExit code: %v", err) @@ -140,8 +144,7 @@ func riskExecHandler( level := ClassifyCommand(args, overrides, extraMods) if !IsAllowed(level, threshold) { - reason := "command risk exceeds configured threshold" - return fmt.Errorf("%s", BlockedCommandError(args, level, threshold, reason)) + return NewBlockedError(args, level, threshold, "command risk exceeds configured threshold") } return next(ctx, args) diff --git a/pkg/tools/shell/runner_test.go b/pkg/tools/shell/runner_test.go index 151dd3590..d008df8c4 100644 --- a/pkg/tools/shell/runner_test.go +++ b/pkg/tools/shell/runner_test.go @@ -2,10 +2,12 @@ package shell import ( "context" + "fmt" "os" "path/filepath" "runtime" "strings" + "syscall" "testing" "time" @@ -91,6 +93,10 @@ func TestRun_BlocksCommandSubstitution(t *testing.T) { } func TestRun_Timeout(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("requires Unix sleep command") + } + result := Run(context.Background(), RunConfig{ Command: "sleep 60", Dir: t.TempDir(), @@ -106,7 +112,78 @@ func TestRun_Timeout(t *testing.T) { } } +func TestRun_TimeoutKillsBackgroundChild(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Unix-only: relies on kill -0 for process liveness check") + } + + tmpDir := t.TempDir() + pidFile := filepath.Join(tmpDir, "child.pid") + + // Spawn a background sh that writes its real OS PID, then execs sleep. + // The foreground also blocks on sleep so the interpreter stays alive + // until the timeout fires. sh is normally risk=critical so we + // downgrade it via an override for this test. + cmd := fmt.Sprintf( + `/bin/sh -c 'echo $$ > %s; exec sleep 300' & sleep 300`, + pidFile, + ) + + result := Run(context.Background(), RunConfig{ + Command: cmd, + Dir: tmpDir, + Timeout: 2 * time.Second, + RiskThreshold: RiskMedium, + RiskOverrides: map[string]string{"sh": "low", "/bin/sh": "low"}, + }) + + if !result.IsError { + t.Fatal("expected timeout error") + } + if !strings.Contains(result.Output, "timed out") { + t.Errorf("expected 'timed out' in output: %s", result.Output) + } + + // Read the PID that was written by the background sh process. + raw, err := os.ReadFile(pidFile) + if err != nil { + // PID file might not have been flushed before timeout — that's fine, + // it just means the child never started and there's nothing to check. + t.Skipf("PID file not written (child may not have started): %v", err) + } + pidStr := strings.TrimSpace(string(raw)) + if pidStr == "" { + t.Skip("PID file empty — child may not have started") + } + + var pid int + if _, err := fmt.Sscanf(pidStr, "%d", &pid); err != nil { + t.Fatalf("failed to parse PID %q: %v", pidStr, err) + } + + // Give the OS a moment to reap the child. + time.Sleep(200 * time.Millisecond) + + proc, err := os.FindProcess(pid) + if err != nil { + // Process already gone — exactly what we want. + return + } + + // Signal 0 checks liveness without actually killing. + if err := proc.Signal(syscall.Signal(0)); err == nil { + t.Errorf("background child (PID %d) still alive after timeout — process leak", pid) + // Best-effort cleanup so we don't leave a zombie. + _ = proc.Kill() + } + // err != nil means the process is gone — success. +} + func TestRun_WorkingDir(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("requires Unix cat command") + } + tmpDir := t.TempDir() testFile := filepath.Join(tmpDir, "test.txt") os.WriteFile(testFile, []byte("test content"), 0o644) @@ -143,6 +220,10 @@ func TestRun_ParseError(t *testing.T) { } func TestRun_StderrCapture(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("requires Unix shell redirections") + } + result := Run(context.Background(), RunConfig{ Command: "echo stdout_msg; echo stderr_msg >&2", Dir: t.TempDir(), @@ -159,6 +240,10 @@ func TestRun_StderrCapture(t *testing.T) { } func TestRun_HighThresholdAllowsRm(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("requires Unix rm command") + } + tmpDir := t.TempDir() testFile := filepath.Join(tmpDir, "delete_me.txt") os.WriteFile(testFile, []byte("bye"), 0o644) @@ -179,6 +264,10 @@ func TestRun_HighThresholdAllowsRm(t *testing.T) { } func TestRun_EnvSanitization(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("requires Unix env command") + } + t.Setenv("OPENAI_API_KEY", "sk-secret-test") t.Setenv("PATH", os.Getenv("PATH")) @@ -201,6 +290,10 @@ func TestRun_EnvSanitization(t *testing.T) { } func TestRun_PipelineCommand(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("requires Unix wc command") + } + result := Run(context.Background(), RunConfig{ Command: "echo 'line1\nline2\nline3' | wc -l", Dir: t.TempDir(), @@ -214,6 +307,10 @@ func TestRun_PipelineCommand(t *testing.T) { } func TestRun_DevNullRedirection(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("requires /dev/null") + } + result := Run(context.Background(), RunConfig{ Command: "echo hello 2>/dev/null", Dir: t.TempDir(), @@ -229,6 +326,10 @@ func TestRun_DevNullRedirection(t *testing.T) { } func TestRun_RiskOverrides(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("requires Unix rm command and /dev/null") + } + // Override rm to low so it passes with threshold=medium. result := Run(context.Background(), RunConfig{ Command: "rm nonexistent_file_xyz 2>/dev/null; echo done", diff --git a/pkg/tools/shell/runner_windows_test.go b/pkg/tools/shell/runner_windows_test.go new file mode 100644 index 000000000..12c464277 --- /dev/null +++ b/pkg/tools/shell/runner_windows_test.go @@ -0,0 +1,122 @@ +package shell + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestRun_Timeout_Windows(t *testing.T) { + // ping with a high count effectively blocks like sleep on Unix. + result := Run(context.Background(), RunConfig{ + Command: "ping -n 60 127.0.0.1", + Dir: t.TempDir(), + Timeout: 500 * time.Millisecond, + RiskThreshold: RiskMedium, + RiskOverrides: map[string]string{"ping": "low"}, + }) + + if !result.IsError { + t.Fatal("expected timeout error") + } + if !strings.Contains(result.Output, "timed out") { + t.Errorf("expected 'timed out' in output: %s", result.Output) + } +} + +func TestRun_WorkingDir_Windows(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "test.txt") + os.WriteFile(testFile, []byte("test content"), 0o644) + + result := Run(context.Background(), RunConfig{ + Command: "cmd.exe /c type test.txt", + Dir: tmpDir, + Timeout: 5 * time.Second, + RiskThreshold: RiskHigh, // cmd.exe is risk=critical + RiskOverrides: map[string]string{"cmd.exe": "low"}, + }) + + if result.IsError { + t.Fatalf("expected success: %s", result.Output) + } + if !strings.Contains(result.Output, "test content") { + t.Errorf("expected 'test content' in output: %s", result.Output) + } +} + +func TestRun_HighThresholdAllowsDel_Windows(t *testing.T) { + tmpDir := t.TempDir() + testFile := filepath.Join(tmpDir, "delete_me.txt") + os.WriteFile(testFile, []byte("bye"), 0o644) + + result := Run(context.Background(), RunConfig{ + Command: "cmd.exe /c del delete_me.txt", + Dir: tmpDir, + Timeout: 5 * time.Second, + RiskThreshold: RiskHigh, + RiskOverrides: map[string]string{"cmd.exe": "low"}, + }) + + if result.IsError { + t.Fatalf("with threshold=high, del should be allowed: %s", result.Output) + } + if _, err := os.Stat(testFile); err == nil { + t.Error("file should have been deleted") + } +} + +func TestRun_EnvSanitization_Windows(t *testing.T) { + t.Setenv("OPENAI_API_KEY", "sk-secret-test") + t.Setenv("PATH", os.Getenv("PATH")) + + result := Run(context.Background(), RunConfig{ + Command: "cmd.exe /c set", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + RiskThreshold: RiskHigh, + RiskOverrides: map[string]string{"cmd.exe": "low"}, + }) + + if result.IsError { + t.Fatalf("expected set command to succeed: %s", result.Output) + } + if strings.Contains(result.Output, "OPENAI_API_KEY") { + t.Error("OPENAI_API_KEY should not be in child environment") + } + if !strings.Contains(result.Output, "PATH=") { + t.Error("PATH should be in child environment") + } +} + +func TestRun_NulRedirection_Windows(t *testing.T) { + result := Run(context.Background(), RunConfig{ + Command: "echo hello 2>NUL", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + Restrict: true, + WorkspaceDir: t.TempDir(), + RiskThreshold: RiskMedium, + }) + + if result.IsError && strings.Contains(result.Output, "sandbox") { + t.Errorf("NUL should not be blocked: %s", result.Output) + } +} + +func TestRun_RiskOverrides_Windows(t *testing.T) { + result := Run(context.Background(), RunConfig{ + Command: "cmd.exe /c del nonexistent_file_xyz 2>NUL & echo done", + Dir: t.TempDir(), + Timeout: 5 * time.Second, + RiskThreshold: RiskMedium, + RiskOverrides: map[string]string{"cmd.exe": "low"}, + }) + + if result.IsError && strings.Contains(result.Output, "blocked") { + t.Errorf("cmd.exe should be allowed with override: %s", result.Output) + } +} diff --git a/pkg/tools/shell/sandbox.go b/pkg/tools/shell/sandbox.go index 8de7d0c69..a26c8170c 100644 --- a/pkg/tools/shell/sandbox.go +++ b/pkg/tools/shell/sandbox.go @@ -6,13 +6,15 @@ import ( "io" "os" "path/filepath" + "runtime" + "strings" "mvdan.cc/sh/v3/interp" ) -// SafePaths are kernel pseudo-devices that are always safe to open, +// safePaths are kernel pseudo-devices that are always safe to open, // regardless of workspace restriction. -var SafePaths = map[string]bool{ +var safePaths = map[string]bool{ "/dev/null": true, "/dev/zero": true, "/dev/random": true, @@ -22,6 +24,19 @@ var SafePaths = map[string]bool{ "/dev/stderr": true, } +// isSafePath reports whether path is a platform-appropriate pseudo-device +// that should always be accessible regardless of sandbox restrictions. +func isSafePath(path string) bool { + if safePaths[path] { + return true + } + // On Windows, NUL (case-insensitive) is the equivalent of /dev/null. + if runtime.GOOS == "windows" && strings.EqualFold(path, "NUL") { + return true + } + return false +} + // SandboxedOpenHandler returns an interp.OpenHandlerFunc that restricts // shell redirections (>, <, >>) to files within the workspace directory. // @@ -41,7 +56,7 @@ func SandboxedOpenHandler(workspaceDir string) interp.OpenHandlerFunc { } return func(ctx context.Context, path string, flag int, perm os.FileMode) (io.ReadWriteCloser, error) { - if SafePaths[path] { + if isSafePath(path) { return os.OpenFile(path, flag, perm) } diff --git a/pkg/tools/shell/sandbox_test.go b/pkg/tools/shell/sandbox_test.go index 3240c15c7..9b76d49d6 100644 --- a/pkg/tools/shell/sandbox_test.go +++ b/pkg/tools/shell/sandbox_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "runtime" "strings" "testing" "time" @@ -40,9 +41,16 @@ func TestSandboxedOpenHandler_AllowsSafePaths(t *testing.T) { workspace := t.TempDir() handler := SandboxedOpenHandler(workspace) - f, err := handler(context.Background(), "/dev/null", os.O_WRONLY, 0) + // Pick a platform-appropriate safe path that exists in the + // sandbox's safe-path list and is openable on the current OS. + safePath := "/dev/null" + if runtime.GOOS == "windows" { + safePath = "NUL" + } + + f, err := handler(context.Background(), safePath, os.O_WRONLY, 0) if err != nil { - t.Fatalf("expected /dev/null to be allowed: %v", err) + t.Fatalf("expected %s to be allowed: %v", safePath, err) } f.Close() } diff --git a/pkg/tools/shell_tool.go b/pkg/tools/shell_tool.go index dd5b0d7ae..6de22e293 100644 --- a/pkg/tools/shell_tool.go +++ b/pkg/tools/shell_tool.go @@ -71,6 +71,18 @@ func NewExecToolWithConfig(workingDir string, restrict bool, cfg *config.Config) } func warnDeprecatedExecConfig(cfg config.ExecConfig) { + if cfg.EnableDenyPatterns != nil { + if !*cfg.EnableDenyPatterns { + fmt.Println("Warning: 'enable_deny_patterns: false' is deprecated and ignored. " + + "Previously this disabled all command filtering. The new risk-based system " + + "is now always active (default threshold=medium). " + + "To allow all commands, set 'risk_threshold: critical'.") + } else { + fmt.Println("Warning: 'enable_deny_patterns' is deprecated and ignored. " + + "Command filtering is now always active via the risk-based classifier. " + + "Remove this field from your config.") + } + } if len(cfg.CustomDenyPatterns) > 0 { fmt.Println("Warning: 'custom_deny_patterns' is deprecated and ignored. " + "Use 'risk_overrides' to adjust per-command risk levels.") diff --git a/pkg/tools/shell_tool_test.go b/pkg/tools/shell_tool_test.go index 863e19868..26d23ee83 100644 --- a/pkg/tools/shell_tool_test.go +++ b/pkg/tools/shell_tool_test.go @@ -1,10 +1,16 @@ package tools import ( + "bytes" "context" + "fmt" + "os" + "strings" "sync" "testing" "time" + + "github.com/sipeed/picoclaw/pkg/config" ) func TestExecTool_SyncExecution(t *testing.T) { @@ -143,3 +149,122 @@ func TestExecTool_ImplementsAsyncTool(t *testing.T) { var _ AsyncTool = tool // compile-time check } + +// captureStdout runs fn and returns whatever it wrote to os.Stdout. +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + + old := os.Stdout + os.Stdout = w + + fn() + + w.Close() + os.Stdout = old + + var buf bytes.Buffer + buf.ReadFrom(r) + return buf.String() +} + +func boolPtr(b bool) *bool { return &b } + +func TestWarnDeprecatedExecConfig_EnableDenyPatternsFalse(t *testing.T) { + out := captureStdout(t, func() { + warnDeprecatedExecConfig(config.ExecConfig{ + EnableDenyPatterns: boolPtr(false), + }) + }) + + if !strings.Contains(out, "enable_deny_patterns: false") { + t.Errorf("expected warning about 'enable_deny_patterns: false', got: %s", out) + } + if !strings.Contains(out, "risk_threshold: critical") { + t.Errorf("expected migration hint to 'risk_threshold: critical', got: %s", out) + } +} + +func TestWarnDeprecatedExecConfig_EnableDenyPatternsTrue(t *testing.T) { + out := captureStdout(t, func() { + warnDeprecatedExecConfig(config.ExecConfig{ + EnableDenyPatterns: boolPtr(true), + }) + }) + + if !strings.Contains(out, "enable_deny_patterns") { + t.Errorf("expected deprecation warning, got: %s", out) + } + if !strings.Contains(out, "Remove this field") { + t.Errorf("expected removal hint, got: %s", out) + } +} + +func TestWarnDeprecatedExecConfig_NilNoWarning(t *testing.T) { + out := captureStdout(t, func() { + warnDeprecatedExecConfig(config.ExecConfig{}) + }) + + if strings.Contains(out, "enable_deny_patterns") { + t.Errorf("expected no warning when field is absent, got: %s", out) + } +} + +func TestWarnDeprecatedExecConfig_CustomPatterns(t *testing.T) { + out := captureStdout(t, func() { + warnDeprecatedExecConfig(config.ExecConfig{ + CustomDenyPatterns: []string{"rm"}, + CustomAllowPatterns: []string{"ls"}, + }) + }) + + if !strings.Contains(out, "custom_deny_patterns") { + t.Errorf("expected custom_deny_patterns warning, got: %s", out) + } + if !strings.Contains(out, "custom_allow_patterns") { + t.Errorf("expected custom_allow_patterns warning, got: %s", out) + } +} + +func TestWarnDeprecatedExecConfig_AllDeprecatedFields(t *testing.T) { + out := captureStdout(t, func() { + warnDeprecatedExecConfig(config.ExecConfig{ + EnableDenyPatterns: boolPtr(false), + CustomDenyPatterns: []string{"rm"}, + CustomAllowPatterns: []string{"ls"}, + }) + }) + + // All three warnings should fire. + for _, want := range []string{ + "enable_deny_patterns: false", + "custom_deny_patterns", + "custom_allow_patterns", + } { + if !strings.Contains(out, want) { + t.Errorf("expected warning containing %q, got: %s", want, out) + } + } +} + +func TestNewExecToolWithConfig_EnableDenyPatternsFalseWarning(t *testing.T) { + out := captureStdout(t, func() { + cfg := &config.Config{} + cfg.Tools.Exec.EnableDenyPatterns = boolPtr(false) + _, err := NewExecToolWithConfig(t.TempDir(), false, cfg) + if err != nil { + t.Fatal(err) + } + }) + + if !strings.Contains(out, "enable_deny_patterns: false") { + t.Errorf("expected warning in NewExecToolWithConfig output: %s", out) + } +} + +// Suppress unused import lint for fmt (used by captureStdout indirectly). +var _ = fmt.Sprintf