From eee49158d1403a34c4e84023b0438b4f8dc229b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=87=8E=E7=94=9F=E6=B4=BECoder=EF=BD=9E?= Date: Sat, 14 Feb 2026 13:56:45 +0800 Subject: [PATCH] harden root-path deletion guard for rm variants --- pkg/tools/risk.go | 2 +- pkg/tools/shell.go | 23 +++++--- pkg/tools/shell_test.go | 119 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 9 deletions(-) create mode 100644 pkg/tools/shell_test.go diff --git a/pkg/tools/risk.go b/pkg/tools/risk.go index 3f86c67..4ceee87 100644 --- a/pkg/tools/risk.go +++ b/pkg/tools/risk.go @@ -28,11 +28,11 @@ var destructivePatterns = []*regexp.Regexp{ regexp.MustCompile(`\bchown\b.+\s+/`), regexp.MustCompile(`\bclawgo\s+uninstall\b`), regexp.MustCompile(`\bdbt\s+drop\b`), + regexp.MustCompile(`\bgit\s+clean\b`), } var moderatePatterns = []*regexp.Regexp{ regexp.MustCompile(`\bgit\s+reset\s+--hard\b`), - regexp.MustCompile(`\bgit\s+clean\b`), regexp.MustCompile(`\bdocker\s+system\s+prune\b`), regexp.MustCompile(`\bapt(-get)?\s+install\b`), regexp.MustCompile(`\byum\s+install\b`), diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 3e8ebdb..c14632c 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -15,7 +15,7 @@ import ( "clawgo/pkg/logger" ) -var blockedRootWipePattern = regexp.MustCompile(`(?i)(^|[;&|\n])\s*rm\s+-rf\s+/\s*($|[;&|\n])`) +var blockedRootWipePattern = regexp.MustCompile(`(?i)(^|[;&|\n])\s*rm\b[^\n;&|]*\s(?:'/'|"/"|/)(?:\s|$)`) type ExecTool struct { workingDir string @@ -29,15 +29,21 @@ type ExecTool struct { } func NewExecTool(cfg config.ShellConfig, workspace string) *ExecTool { - denyPatterns := make([]*regexp.Regexp, 0) + denyPatterns := make([]*regexp.Regexp, 0, len(cfg.DeniedCmds)) for _, p := range cfg.DeniedCmds { - denyPatterns = append(denyPatterns, regexp.MustCompile(`\b`+regexp.QuoteMeta(p)+`\b`)) + denyPatterns = append(denyPatterns, regexp.MustCompile(`(?i)\b`+regexp.QuoteMeta(p)+`\b`)) + } + + allowPatterns := make([]*regexp.Regexp, 0, len(cfg.AllowedCmds)) + for _, p := range cfg.AllowedCmds { + allowPatterns = append(allowPatterns, regexp.MustCompile(`(?i)\b`+regexp.QuoteMeta(p)+`\b`)) } return &ExecTool{ workingDir: workspace, timeout: cfg.Timeout, denyPatterns: denyPatterns, + allowPatterns: allowPatterns, restrictToWorkspace: cfg.RestrictPath, sandboxEnabled: cfg.Sandbox.Enabled, sandboxImage: cfg.Sandbox.Image, @@ -149,11 +155,11 @@ func (t *ExecTool) guardCommand(command, cwd string) string { lower := strings.ToLower(cmd) if blockedRootWipePattern.MatchString(lower) { - return "Command blocked by safety guard (rm -rf / is forbidden)" + return "Command blocked by safety guard (removing root path / is forbidden)" } for _, pattern := range t.denyPatterns { - if pattern.MatchString(lower) { + if pattern.MatchString(cmd) { return "Command blocked by safety guard (dangerous pattern detected)" } } @@ -161,7 +167,7 @@ func (t *ExecTool) guardCommand(command, cwd string) string { if len(t.allowPatterns) > 0 { allowed := false for _, pattern := range t.allowPatterns { - if pattern.MatchString(lower) { + if pattern.MatchString(cmd) { allowed = true break } @@ -215,7 +221,7 @@ func (t *ExecTool) SetRestrictToWorkspace(restrict bool) { func (t *ExecTool) SetAllowPatterns(patterns []string) error { t.allowPatterns = make([]*regexp.Regexp, 0, len(patterns)) for _, p := range patterns { - re, err := regexp.Compile(p) + re, err := regexp.Compile("(?i)" + p) if err != nil { return fmt.Errorf("invalid allow pattern %q: %w", p, err) } @@ -254,10 +260,11 @@ func (t *ExecTool) applyRiskGate(command string, force bool) (string, string) { return "Error: destructive command is disabled by policy (tools.shell.risk.allow_destructive=false).", "" } - if t.riskCfg.RequireDryRun { + if t.riskCfg.RequireDryRun && !force { if dryRunCmd, ok := buildDryRunCommand(command); ok { return "Risk gate: dry-run required first. Review output, then execute intentionally with force=true.", dryRunCmd } + return "Error: destructive command requires explicit force=true because no dry-run strategy is available.", "" } return "", "" } diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go new file mode 100644 index 0000000..c67dd47 --- /dev/null +++ b/pkg/tools/shell_test.go @@ -0,0 +1,119 @@ +package tools + +import ( + "testing" + + "clawgo/pkg/config" +) + +func TestApplyRiskGate_DryRunCanBeBypassedWithForce(t *testing.T) { + tool := &ExecTool{riskCfg: config.RiskConfig{ + Enabled: true, + AllowDestructive: true, + RequireDryRun: true, + RequireForceFlag: false, + }} + + msg, dryRun := tool.applyRiskGate("git clean -fd", true) + if msg != "" || dryRun != "" { + t.Fatalf("expected force=true to allow execution after dry-run step, got msg=%q dryRun=%q", msg, dryRun) + } +} + +func TestApplyRiskGate_RequiresDryRunWithoutForce(t *testing.T) { + tool := &ExecTool{riskCfg: config.RiskConfig{ + Enabled: true, + AllowDestructive: true, + RequireDryRun: true, + RequireForceFlag: false, + }} + + msg, dryRun := tool.applyRiskGate("git clean -fd", false) + if msg == "" { + t.Fatal("expected dry-run block message") + } + if dryRun == "" { + t.Fatal("expected dry-run command") + } +} + +func TestAssessCommandRisk_GitCleanIsDestructive(t *testing.T) { + assessment := assessCommandRisk("git clean -fd") + if assessment.Level != RiskDestructive { + t.Fatalf("expected git clean to be destructive, got %s", assessment.Level) + } +} + +func TestNewExecTool_LoadsAllowedCmdsIntoAllowPatterns(t *testing.T) { + tool := NewExecTool(config.ShellConfig{AllowedCmds: []string{"echo"}}, ".") + if len(tool.allowPatterns) != 1 { + t.Fatalf("expected one allow pattern, got %d", len(tool.allowPatterns)) + } +} + +func TestGuardCommand_BlocksCommandNotInAllowlist(t *testing.T) { + tool := NewExecTool(config.ShellConfig{AllowedCmds: []string{"echo"}}, ".") + if msg := tool.guardCommand("ls -la", "."); msg == "" { + t.Fatal("expected allowlist to block command not in allowed_cmds") + } + + if msg := tool.guardCommand("echo hi", "."); msg != "" { + t.Fatalf("expected allowed command to pass guard, got %q", msg) + } +} + +func TestGuardCommand_AllowlistIsCaseInsensitive(t *testing.T) { + tool := NewExecTool(config.ShellConfig{AllowedCmds: []string{"ECHO"}}, ".") + if msg := tool.guardCommand("echo hi", "."); msg != "" { + t.Fatalf("expected case-insensitive allowlist match, got %q", msg) + } +} + +func TestGuardCommand_DenylistIsCaseInsensitive(t *testing.T) { + tool := NewExecTool(config.ShellConfig{DeniedCmds: []string{"RM"}}, ".") + if msg := tool.guardCommand("rm -f tmp.txt", "."); msg == "" { + t.Fatal("expected case-insensitive denylist match to block command") + } +} + +func TestApplyRiskGate_RequireDryRunWithoutStrategyStillBlocks(t *testing.T) { + tool := &ExecTool{riskCfg: config.RiskConfig{ + Enabled: true, + AllowDestructive: true, + RequireDryRun: true, + RequireForceFlag: false, + }} + + msg, dryRun := tool.applyRiskGate("rm -rf tmp", false) + if msg == "" { + t.Fatal("expected destructive command without dry-run strategy to be blocked") + } + if dryRun != "" { + t.Fatalf("expected no dry-run command for rm -rf, got %q", dryRun) + } +} + +func TestSetAllowPatterns_IsCaseInsensitive(t *testing.T) { + tool := &ExecTool{} + if err := tool.SetAllowPatterns([]string{`^ECHO\b`}); err != nil { + t.Fatalf("SetAllowPatterns returned error: %v", err) + } + + if msg := tool.guardCommand("echo hi", "."); msg != "" { + t.Fatalf("expected case-insensitive allow pattern to match, got %q", msg) + } +} + +func TestGuardCommand_BlocksRootWipeVariants(t *testing.T) { + tool := &ExecTool{} + cases := []string{ + "rm -rf /", + "rm -fr /", + "rm --no-preserve-root -rf /", + } + for _, c := range cases { + if msg := tool.guardCommand(c, "."); msg == "" { + t.Fatalf("expected root wipe variant to be blocked: %s", c) + } + } +}