fix(policy): deny query matching fails closed, mirror allow-side validation

Addresses PR review findings P1 and P3:

P1: Deny-side query matching now uses fail-closed semantics. If ANY
value for a query key matches the deny matcher, the deny fires. The
previous implementation reused allow-side "all values must match"
logic which allowed ?force=true&force=false to bypass a deny on
force=true.

P3: Deny-side query validation now mirrors the full allow-side checks:
empty any lists, non-string matcher values, glob+any mutual exclusion,
glob type checks, and glob syntax warnings are all validated.

Author: johntmyers

crates/openshell-sandbox/data/sandbox-policy.rego

@@ -239,17 +239,49 @@ request_denied_for_endpoint(request, endpoint) if {
 	command_matches(request.command, deny_rule.command)
 }
 
-# Deny query matching: if no query rules on deny, match any query params.
-# If query rules present, all configured keys must match.
+# Deny query matching: fail-closed semantics.
+# If no query rules on the deny rule, match unconditionally (any query params).
+# If query rules present, trigger the deny if ANY value for a configured key
+# matches the matcher. This is the inverse of allow-side semantics where ALL
+# values must match. For deny logic, a single matching value is enough to block.
 deny_query_params_match(request, deny_rule) if {
 	deny_query_rules := object.get(deny_rule, "query", {})
-	not deny_query_mismatch(request, deny_query_rules)
+	count(deny_query_rules) == 0
 }
 
-deny_query_mismatch(request, query_rules) if {
+deny_query_params_match(request, deny_rule) if {
+	deny_query_rules := object.get(deny_rule, "query", {})
+	count(deny_query_rules) > 0
+	not deny_query_key_missing(request, deny_query_rules)
+	not deny_query_value_mismatch_all(request, deny_query_rules)
+}
+
+# A configured deny query key is missing from the request entirely.
+# Missing key means the deny rule doesn't apply (fail-open on absence).
+deny_query_key_missing(request, query_rules) if {
+	some key
+	query_rules[key]
+	request_query := object.get(request, "query_params", {})
+	values := object.get(request_query, key, null)
+	values == null
+}
+
+# ALL values for a configured key fail to match the matcher.
+# If even one value matches, deny fires. This rule checks the opposite:
+# true when NO value matches (i.e., every value is a mismatch).
+deny_query_value_mismatch_all(request, query_rules) if {
 	some key
 	matcher := query_rules[key]
-	not query_key_matches(request, key, matcher)
+	request_query := object.get(request, "query_params", {})
+	values := object.get(request_query, key, [])
+	count(values) > 0
+	not deny_any_value_matches(values, matcher)
+}
+
+# True if at least one value in the list matches the matcher.
+deny_any_value_matches(values, matcher) if {
+	some i
+	query_value_matches(values[i], matcher)
 }
 
 # --- L7 deny reason ---

crates/openshell-sandbox/src/l7/mod.rs

@@ -377,42 +377,102 @@ pub fn validate_l7_policies(data_json: &serde_json::Value) -> (Vec<String>, Vec<
                             }
                         }
 
-                        // Validate query matchers (same rules as allow query matchers)
+                        // Validate query matchers — mirrors allow-side validation exactly
                         if let Some(query) = deny_rule.get("query").filter(|v| !v.is_null()) {
-                            if let Some(query_obj) = query.as_object() {
-                                for (param, matcher) in query_obj {
-                                    if let Some(glob_str) = matcher.as_str() {
-                                        if let Some(warning) = check_glob_syntax(glob_str) {
-                                            warnings.push(format!(
-                                                "{deny_loc}.query.{param}: {warning}"
-                                            ));
-                                        }
-                                        continue;
+                            let Some(query_obj) = query.as_object() else {
+                                errors.push(format!(
+                                    "{deny_loc}.query: expected map of query matchers"
+                                ));
+                                continue;
+                            };
+
+                            for (param, matcher) in query_obj {
+                                if let Some(glob_str) = matcher.as_str() {
+                                    if let Some(warning) = check_glob_syntax(glob_str) {
+                                        warnings
+                                            .push(format!("{deny_loc}.query.{param}: {warning}"));
                                     }
-                                    if let Some(matcher_obj) = matcher.as_object() {
-                                        let has_any = matcher_obj.get("any").is_some();
-                                        let has_glob = matcher_obj.get("glob").is_some();
-                                        let has_unknown =
-                                            matcher_obj.keys().any(|k| k != "any" && k != "glob");
-                                        if has_unknown {
-                                            errors.push(format!(
-                                                "{deny_loc}.query.{param}: unknown matcher keys; only `glob` or `any` are supported"
-                                            ));
-                                        } else if has_glob && has_any {
-                                            errors.push(format!(
-                                                "{deny_loc}.query.{param}: matcher cannot specify both `glob` and `any`"
-                                            ));
-                                        } else if !has_glob && !has_any {
+                                    continue;
+                                }
+
+                                let Some(matcher_obj) = matcher.as_object() else {
+                                    errors.push(format!(
+                                        "{deny_loc}.query.{param}: expected string glob or object with `any`"
+                                    ));
+                                    continue;
+                                };
+
+                                let has_any = matcher_obj.get("any").is_some();
+                                let has_glob = matcher_obj.get("glob").is_some();
+                                let has_unknown =
+                                    matcher_obj.keys().any(|k| k != "any" && k != "glob");
+                                if has_unknown {
+                                    errors.push(format!(
+                                        "{deny_loc}.query.{param}: unknown matcher keys; only `glob` or `any` are supported"
+                                    ));
+                                    continue;
+                                }
+
+                                if has_glob && has_any {
+                                    errors.push(format!(
+                                        "{deny_loc}.query.{param}: matcher cannot specify both `glob` and `any`"
+                                    ));
+                                    continue;
+                                }
+
+                                if !has_glob && !has_any {
+                                    errors.push(format!(
+                                        "{deny_loc}.query.{param}: object matcher requires `glob` string or non-empty `any` list"
+                                    ));
+                                    continue;
+                                }
+
+                                if has_glob {
+                                    match matcher_obj.get("glob").and_then(|v| v.as_str()) {
+                                        None => {
                                             errors.push(format!(
-                                                "{deny_loc}.query.{param}: object matcher requires `glob` string or non-empty `any` list"
+                                                "{deny_loc}.query.{param}.glob: expected glob string"
                                             ));
                                         }
+                                        Some(g) => {
+                                            if let Some(warning) = check_glob_syntax(g) {
+                                                warnings.push(format!(
+                                                    "{deny_loc}.query.{param}.glob: {warning}"
+                                                ));
+                                            }
+                                        }
+                                    }
+                                    continue;
+                                }
+
+                                let any = matcher_obj.get("any").and_then(|v| v.as_array());
+                                let Some(any) = any else {
+                                    errors.push(format!(
+                                        "{deny_loc}.query.{param}.any: expected array of glob strings"
+                                    ));
+                                    continue;
+                                };
+
+                                if any.is_empty() {
+                                    errors.push(format!(
+                                        "{deny_loc}.query.{param}.any: list must not be empty"
+                                    ));
+                                    continue;
+                                }
+
+                                if any.iter().any(|v| v.as_str().is_none()) {
+                                    errors.push(format!(
+                                        "{deny_loc}.query.{param}.any: all values must be strings"
+                                    ));
+                                }
+
+                                for item in any.iter().filter_map(|v| v.as_str()) {
+                                    if let Some(warning) = check_glob_syntax(item) {
+                                        warnings.push(format!(
+                                            "{deny_loc}.query.{param}.any: {warning}"
+                                        ));
                                     }
                                 }
-                            } else {
-                                errors.push(format!(
-                                    "{deny_loc}.query: expected map of query matchers"
-                                ));
                             }
                         }
 
@@ -1269,4 +1329,193 @@ mod tests {
             "valid query matcher shapes should not error: {errors:?}"
         );
     }
+
+    // --- Deny rules validation tests ---
+
+    #[test]
+    fn validate_deny_rules_require_protocol() {
+        let data = serde_json::json!({
+            "network_policies": {
+                "test": {
+                    "endpoints": [{
+                        "host": "api.example.com",
+                        "port": 443,
+                        "deny_rules": [{ "method": "POST", "path": "/admin" }]
+                    }],
+                    "binaries": []
+                }
+            }
+        });
+        let (errors, _) = validate_l7_policies(&data);
+        assert!(
+            errors
+                .iter()
+                .any(|e| e.contains("deny_rules require protocol")),
+            "should require protocol for deny_rules: {errors:?}"
+        );
+    }
+
+    #[test]
+    fn validate_deny_rules_require_allow_base() {
+        let data = serde_json::json!({
+            "network_policies": {
+                "test": {
+                    "endpoints": [{
+                        "host": "api.example.com",
+                        "port": 443,
+                        "protocol": "rest",
+                        "deny_rules": [{ "method": "POST", "path": "/admin" }]
+                    }],
+                    "binaries": []
+                }
+            }
+        });
+        let (errors, _) = validate_l7_policies(&data);
+        assert!(
+            errors
+                .iter()
+                .any(|e| e.contains("deny_rules require rules or access")),
+            "should require rules or access for deny_rules: {errors:?}"
+        );
+    }
+
+    #[test]
+    fn validate_deny_rules_empty_list_rejected() {
+        let data = serde_json::json!({
+            "network_policies": {
+                "test": {
+                    "endpoints": [{
+                        "host": "api.example.com",
+                        "port": 443,
+                        "protocol": "rest",
+                        "access": "full",
+                        "deny_rules": []
+                    }],
+                    "binaries": []
+                }
+            }
+        });
+        let (errors, _) = validate_l7_policies(&data);
+        assert!(
+            errors
+                .iter()
+                .any(|e| e.contains("deny_rules list cannot be empty")),
+            "should reject empty deny_rules: {errors:?}"
+        );
+    }
+
+    #[test]
+    fn validate_deny_rules_valid_config_accepted() {
+        let data = serde_json::json!({
+            "network_policies": {
+                "test": {
+                    "endpoints": [{
+                        "host": "api.example.com",
+                        "port": 443,
+                        "protocol": "rest",
+                        "access": "read-write",
+                        "deny_rules": [
+                            { "method": "POST", "path": "/repos/*/pulls/*/reviews" },
+                            { "method": "PUT", "path": "/repos/*/branches/*/protection" }
+                        ]
+                    }],
+                    "binaries": []
+                }
+            }
+        });
+        let (errors, _) = validate_l7_policies(&data);
+        assert!(
+            errors.is_empty(),
+            "valid deny_rules should not error: {errors:?}"
+        );
+    }
+
+    #[test]
+    fn validate_deny_rules_query_empty_any_rejected() {
+        let data = serde_json::json!({
+            "network_policies": {
+                "test": {
+                    "endpoints": [{
+                        "host": "api.example.com",
+                        "port": 443,
+                        "protocol": "rest",
+                        "access": "full",
+                        "deny_rules": [{
+                            "method": "POST",
+                            "path": "/admin",
+                            "query": { "type": { "any": [] } }
+                        }]
+                    }],
+                    "binaries": []
+                }
+            }
+        });
+        let (errors, _) = validate_l7_policies(&data);
+        assert!(
+            errors
+                .iter()
+                .any(|e| e.contains("any: list must not be empty")),
+            "should reject empty any list in deny query: {errors:?}"
+        );
+    }
+
+    #[test]
+    fn validate_deny_rules_query_non_string_rejected() {
+        let data = serde_json::json!({
+            "network_policies": {
+                "test": {
+                    "endpoints": [{
+                        "host": "api.example.com",
+                        "port": 443,
+                        "protocol": "rest",
+                        "access": "full",
+                        "deny_rules": [{
+                            "method": "POST",
+                            "path": "/admin",
+                            "query": { "force": 123 }
+                        }]
+                    }],
+                    "binaries": []
+                }
+            }
+        });
+        let (errors, _) = validate_l7_policies(&data);
+        assert!(
+            errors
+                .iter()
+                .any(|e| e.contains("expected string glob or object")),
+            "should reject non-string/non-object matcher in deny query: {errors:?}"
+        );
+    }
+
+    #[test]
+    fn validate_deny_rules_query_valid_matchers_accepted() {
+        let data = serde_json::json!({
+            "network_policies": {
+                "test": {
+                    "endpoints": [{
+                        "host": "api.example.com",
+                        "port": 443,
+                        "protocol": "rest",
+                        "access": "full",
+                        "deny_rules": [{
+                            "method": "POST",
+                            "path": "/admin/**",
+                            "query": {
+                                "force": "true",
+                                "type": { "any": ["admin-*", "root-*"] },
+                                "scope": { "glob": "org-*" }
+                            }
+                        }]
+                    }],
+                    "binaries": []
+                }
+            }
+        });
+        let (errors, _) = validate_l7_policies(&data);
+        assert!(
+            errors.is_empty(),
+            "valid deny query matchers should not error: {errors:?}"
+        );
+    }
 }