Merge pull request #3161 from danmobot/fix/exec-custom-allow-deny

fix(exec): keep deny patterns active for custom allow rules

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)"
 		}
 	}

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)
+	}
+}