diff --git a/pkg/providers/openai_compat/provider.go b/pkg/providers/openai_compat/provider.go index c12a8348..6a1b73d8 100644 --- a/pkg/providers/openai_compat/provider.go +++ b/pkg/providers/openai_compat/provider.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "io" + "log" "maps" "net/http" "net/url" diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 85bdcb8c..3d5a97fe 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 + return "Command blocked by safety guard (dangerous pattern detected)" } } - if !explicitlyAllowed { - for _, pattern := range t.denyPatterns { - if pattern.MatchString(lower) { - 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 len(t.allowPatterns) > 0 || len(t.customAllowPatterns) > 0 { + 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 cdc78b7f..5312283b 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -1936,3 +1936,37 @@ 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) + } +}