diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 873d4fcb1..6d54e94cc 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -1135,36 +1135,35 @@ func expandPowerShellEnvVars(cmd string) string { }) } +func (t *ExecTool) commandMatchesAllowPattern(lower string) bool { + for _, pattern := range t.allowPatterns { + if pattern.MatchString(lower) { + return true + } + } + for _, pattern := range t.customAllowPatterns { + if pattern.MatchString(lower) { + return true + } + } + return false +} + func (t *ExecTool) guardCommand(command, cwd string) string { cmd := strings.TrimSpace(command) lower := strings.ToLower(cmd) - // Custom allow patterns exempt a command from deny checks. - explicitlyAllowed := false - for _, pattern := range t.customAllowPatterns { + // Deny patterns always apply, even when a command matches a custom allow rule. + // Custom allow rules can permit a command, but must not disable secret-safety + // deny rules such as jq env access checks (#3079). + for _, pattern := range t.denyPatterns { if pattern.MatchString(lower) { - explicitlyAllowed = true - break - } - } - - if !explicitlyAllowed { - for _, pattern := range t.denyPatterns { - if pattern.MatchString(lower) { - return "Command blocked by safety guard (dangerous pattern detected)" - } + return "Command blocked by safety guard (dangerous pattern detected)" } } if len(t.allowPatterns) > 0 { - allowed := false - for _, pattern := range t.allowPatterns { - if pattern.MatchString(lower) { - allowed = true - break - } - } - if !allowed { + if !t.commandMatchesAllowPattern(lower) { return "Command blocked by safety guard (not in allowlist)" } } diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index cdc78b7fc..a3399ede1 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -1936,3 +1936,54 @@ func TestShellTool_SchemelessURLDetection(t *testing.T) { } } } + +func TestShellTool_CustomAllowDoesNotBypassDenyPatterns(t *testing.T) { + cfg := &config.Config{} + cfg.Tools.Exec.EnableDenyPatterns = true + cfg.Tools.Exec.CustomAllowPatterns = []string{`^jq\b`} + cfg.Tools.Exec.CustomDenyPatterns = []string{`\$env\b`, `(^|[^.$a-z0-9_])env([^a-z0-9_]|$)`} + + tool, err := NewExecToolWithConfig(t.TempDir(), false, cfg) + if err != nil { + t.Fatalf("NewExecToolWithConfig() error: %v", err) + } + + got := tool.guardCommand(`jq -n '$ENV.PICOCLAW_VARIANT_CANARY'`, t.TempDir()) + if !strings.Contains(got, "dangerous pattern detected") { + t.Fatalf("custom allow should not bypass deny patterns, got: %q", got) + } +} + +func TestShellTool_CustomAllowStillPermitsSafeMatch(t *testing.T) { + cfg := &config.Config{} + cfg.Tools.Exec.EnableDenyPatterns = true + cfg.Tools.Exec.CustomAllowPatterns = []string{`^jq\b`} + cfg.Tools.Exec.CustomDenyPatterns = []string{`\$env\b`, `(^|[^.$a-z0-9_])env([^a-z0-9_]|$)`} + + tool, err := NewExecToolWithConfig(t.TempDir(), false, cfg) + if err != nil { + t.Fatalf("NewExecToolWithConfig() error: %v", err) + } + + got := tool.guardCommand(`jq -n '"ok"'`, t.TempDir()) + if got != "" { + t.Fatalf("safe custom-allowed command should pass guard, got: %q", got) + } +} + +func TestShellTool_CustomAllowDoesNotBecomeStrictAllowlist(t *testing.T) { + cfg := &config.Config{} + cfg.Tools.Exec.EnableDenyPatterns = true + cfg.Tools.Exec.CustomAllowPatterns = []string{`^jq\b`} + cfg.Tools.Exec.CustomDenyPatterns = []string{`\$env\b`, `(^|[^.$a-z0-9_])env([^a-z0-9_]|$)`} + + tool, err := NewExecToolWithConfig(t.TempDir(), false, cfg) + if err != nil { + t.Fatalf("NewExecToolWithConfig() error: %v", err) + } + + got := tool.guardCommand("ls", t.TempDir()) + if got != "" { + t.Fatalf("custom allow patterns should not become a strict allowlist, got: %q", got) + } +}