Prevent Query Rule Creation with Invalid Numeric Match Criteria (#122823)

* SEARCH-802 - bug fixed - Query rules allows for creation of rules with invalid match criteria

* [CI] Auto commit changes from spotless

* Worked on the comments given in the PR

* [CI] Auto commit changes from spotless

* Fixed Integration tests

* [CI] Auto commit changes from spotless

* Made changes from the PR

* Update docs/changelog/122823.yaml

* [CI] Auto commit changes from spotless

* Fixed the duplicate code issue in queryRuleTests

* Refactored code to clean it up based on PR comments

* [CI] Auto commit changes from spotless

* Logger statements were removed

* Cleaned up the QueryRule tests

* [CI] Auto commit changes from spotless

* Update x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java

Co-authored-by: Mike Pellegrini <mike.pellegrini@elastic.co>

* [CI] Auto commit changes from spotless

---------

Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>
Co-authored-by: Kathleen DeRusso <kathleen.derusso@elastic.co>
Co-authored-by: Mike Pellegrini <mike.pellegrini@elastic.co>

docs/changelog/122823.yaml

@@ -0,0 +1,5 @@
+pr: 122823
+summary: Prevent Query Rule Creation with Invalid Numeric Match Criteria
+area: Relevance
+type: bug
+issues: []

x-pack/plugin/ent-search/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/10_query_ruleset_put.yml

@@ -15,6 +15,16 @@ teardown:
         ruleset_id: test-query-ruleset-recreating
         ignore: 404
 
+  - do:
+      query_rules.delete_ruleset:
+        ruleset_id: invalid_numeric_test
+        ignore: 404
+
+  - do:
+      query_rules.delete_ruleset:
+        ruleset_id: multiple_values_test
+        ignore: 404
+
 ---
 'Create Query Ruleset':
   - do:
@@ -160,3 +170,51 @@ teardown:
                 - 'id2'
 
   - match: { error.type: 'security_exception' }
+
+---
+"Test numeric comparison rule validation":
+  - skip:
+      features: headers
+
+  - requires:
+      cluster_features: "query_rules.numeric_validation"
+      reason: "Fixed bug to enforce numeric rule validation starting with 8.19"
+
+  - do:
+      catch: /Input \[not_a_number\] is not valid for CriteriaType \[lte\]/
+      query_rules.put_ruleset:
+        ruleset_id: invalid_numeric_test
+        body:
+          rules:
+            - rule_id: invalid_numeric_rule
+              type: pinned
+              criteria:
+                - type: lte
+                  metadata: price
+                  values: ["not_a_number"]
+              actions:
+                ids: ["1"]
+
+---
+"Test numeric comparison with multiple values":
+  - skip:
+      features: headers
+
+  - requires:
+      cluster_features: "query_rules.numeric_validation"
+      reason: "Fixed bug to enforce numeric rule validation starting with 8.19"
+
+  - do:
+      catch: /Input \[not_a_number\] is not valid for CriteriaType \[lte\]/
+      query_rules.put_ruleset:
+        ruleset_id: multiple_values_test
+        body:
+          rules:
+            - rule_id: multiple_values_rule
+              type: pinned
+              criteria:
+                - type: lte
+                  metadata: price
+                  values: ["100", "not_a_number"]
+              actions:
+                ids: ["1"]

x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearchFeatures.java

@@ -9,6 +9,7 @@ package org.elasticsearch.xpack.application;
 
 import org.elasticsearch.features.FeatureSpecification;
 import org.elasticsearch.features.NodeFeature;
+import org.elasticsearch.xpack.application.rules.QueryRule;
 
 import java.util.Set;
 
@@ -18,4 +19,9 @@ public class EnterpriseSearchFeatures implements FeatureSpecification {
     public Set<NodeFeature> getFeatures() {
         return Set.of();
     }
+
+    @Override
+    public Set<NodeFeature> getTestFeatures() {
+        return Set.of(QueryRule.NUMERIC_VALIDATION);
+    }
 }

x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java

@@ -16,6 +16,7 @@ import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
 import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.core.Nullable;
+import org.elasticsearch.features.NodeFeature;
 import org.elasticsearch.xcontent.ConstructingObjectParser;
 import org.elasticsearch.xcontent.ParseField;
 import org.elasticsearch.xcontent.ToXContentObject;
@@ -81,6 +82,8 @@ public class QueryRule implements Writeable, ToXContentObject {
         }
     }
 
+    public static final NodeFeature NUMERIC_VALIDATION = new NodeFeature("query_rules.numeric_validation", true);
+
     /**
      * Public constructor.
      *
@@ -140,7 +143,6 @@ public class QueryRule implements Writeable, ToXContentObject {
     }
 
     private void validate() {
-
         if (priority != null && (priority < MIN_PRIORITY || priority > MAX_PRIORITY)) {
             throw new IllegalArgumentException("Priority was " + priority + ", must be between " + MIN_PRIORITY + " and " + MAX_PRIORITY);
         }
@@ -155,6 +157,13 @@ public class QueryRule implements Writeable, ToXContentObject {
                 throw new ElasticsearchParseException(type.toString() + " query rule actions must contain only one of either ids or docs");
             }
         }
+
+        criteria.forEach(criterion -> {
+            List<Object> values = criterion.criteriaValues();
+            if (values != null) {
+                values.forEach(criterion.criteriaType()::validateInput);
+            }
+        });
     }
 
     private void validateIdOrDocAction(Object action) {

x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/EnterpriseSearchModuleTestUtils.java

@@ -20,25 +20,26 @@ import org.elasticsearch.xpack.application.search.SearchApplicationTemplate;
 import org.elasticsearch.xpack.application.search.TemplateParamValidator;
 import org.elasticsearch.xpack.core.action.util.PageParams;
 
+import java.math.BigDecimal;
+import java.math.RoundingMode;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Set;
 
 import static org.elasticsearch.test.ESTestCase.generateRandomStringArray;
 import static org.elasticsearch.test.ESTestCase.randomAlphaOfLengthBetween;
 import static org.elasticsearch.test.ESTestCase.randomBoolean;
+import static org.elasticsearch.test.ESTestCase.randomDoubleBetween;
 import static org.elasticsearch.test.ESTestCase.randomFrom;
 import static org.elasticsearch.test.ESTestCase.randomIdentifier;
 import static org.elasticsearch.test.ESTestCase.randomIntBetween;
-import static org.elasticsearch.test.ESTestCase.randomList;
 import static org.elasticsearch.test.ESTestCase.randomLongBetween;
 import static org.elasticsearch.test.ESTestCase.randomMap;
 import static org.elasticsearch.xpack.application.rules.QueryRule.MAX_PRIORITY;
 import static org.elasticsearch.xpack.application.rules.QueryRule.MIN_PRIORITY;
-import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.ALWAYS;
 
 public final class EnterpriseSearchModuleTestUtils {
 
@@ -86,19 +87,48 @@ public final class EnterpriseSearchModuleTestUtils {
         return randomMap(0, 10, () -> Tuple.tuple(randomIdentifier(), randomAlphaOfLengthBetween(0, 10)));
     }
 
-    public static QueryRuleCriteria randomQueryRuleCriteria() {
-        // We intentionally don't allow ALWAYS criteria in this method, since we want to test parsing metadata and values
-        QueryRuleCriteriaType type = randomFrom(Arrays.stream(QueryRuleCriteriaType.values()).filter(t -> t != ALWAYS).toList());
-        return new QueryRuleCriteria(type, randomAlphaOfLengthBetween(1, 10), randomList(1, 5, () -> randomAlphaOfLengthBetween(1, 10)));
-    }
-
     public static QueryRule randomQueryRule() {
-        String id = randomIdentifier();
+        String ruleId = randomIdentifier();
         QueryRule.QueryRuleType type = randomFrom(QueryRule.QueryRuleType.values());
-        List<QueryRuleCriteria> criteria = List.of(randomQueryRuleCriteria());
-        Map<String, Object> actions = Map.of(randomFrom("ids", "docs"), List.of(randomAlphaOfLengthBetween(2, 10)));
-        Integer priority = randomQueryRulePriority();
-        return new QueryRule(id, type, criteria, actions, priority);
+        List<QueryRuleCriteria> criteria = new ArrayList<>();
+        int numCriteria = randomIntBetween(1, 3);
+
+        for (int i = 0; i < numCriteria; i++) {
+            criteria.add(randomQueryRuleCriteria());
+        }
+
+        return new QueryRule(ruleId, type, criteria, randomQueryRuleActions(), randomQueryRulePriority());
+    }
+
+    public static QueryRuleCriteria randomQueryRuleCriteria() {
+        Set<QueryRuleCriteriaType> numericValueCriteriaType = Set.of(
+            QueryRuleCriteriaType.GT,
+            QueryRuleCriteriaType.GTE,
+            QueryRuleCriteriaType.LT,
+            QueryRuleCriteriaType.LTE
+        );
+        QueryRuleCriteriaType criteriaType = randomFrom(QueryRuleCriteriaType.values());
+
+        String metadata;
+        List<Object> values;
+        if (criteriaType == QueryRuleCriteriaType.ALWAYS) {
+            metadata = null;
+            values = null;
+        } else {
+            metadata = randomAlphaOfLengthBetween(3, 10);
+            values = new ArrayList<>();
+
+            int numValues = randomIntBetween(1, 3);
+            for (int i = 0; i < numValues; i++) {
+                values.add(
+                    numericValueCriteriaType.contains(criteriaType)
+                        ? BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)).setScale(2, RoundingMode.HALF_UP).toString()
+                        : randomAlphaOfLengthBetween(3, 10)
+                );
+            }
+        }
+
+        return new QueryRuleCriteria(criteriaType, metadata, values);
     }
 
     public static Integer randomQueryRulePriority() {
@@ -115,8 +145,24 @@ public final class EnterpriseSearchModuleTestUtils {
         return new QueryRuleset(id, rules);
     }
 
+    public static Map<String, Object> randomQueryRuleActions() {
+        if (randomBoolean()) {
+            return Map.of(QueryRule.IDS_FIELD.getPreferredName(), List.of(randomAlphaOfLengthBetween(3, 10)));
+        }
+        return Map.of(
+            QueryRule.DOCS_FIELD.getPreferredName(),
+            List.of(
+                Map.of(
+                    QueryRule.INDEX_FIELD.getPreferredName(),
+                    randomAlphaOfLengthBetween(3, 10),
+                    QueryRule.ID_FIELD.getPreferredName(),
+                    randomAlphaOfLengthBetween(3, 10)
+                )
+            )
+        );
+    }
+
     public static Map<String, Object> randomMatchCriteria() {
         return randomMap(1, 3, () -> Tuple.tuple(randomIdentifier(), randomAlphaOfLengthBetween(0, 10)));
     }
-
 }

x-pack/plugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java

@@ -15,6 +15,8 @@ import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.search.SearchModule;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.xcontent.ToXContent;
+import org.elasticsearch.xcontent.XContentBuilder;
+import org.elasticsearch.xcontent.XContentFactory;
 import org.elasticsearch.xcontent.XContentParser;
 import org.elasticsearch.xcontent.XContentType;
 import org.elasticsearch.xpack.application.EnterpriseSearchModuleTestUtils;
@@ -30,8 +32,10 @@ import static java.util.Collections.emptyList;
 import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;
 import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.EXACT;
+import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.LTE;
 import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.PREFIX;
 import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.SUFFIX;
+import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.equalTo;
 
 public class QueryRuleTests extends ESTestCase {
@@ -53,6 +57,79 @@ public class QueryRuleTests extends ESTestCase {
         }
     }
 
+    public void testNumericValidationWithValidValues() throws IOException {
+        String content = XContentHelper.stripWhitespace("""
+            {
+              "rule_id": "numeric_rule",
+              "type": "pinned",
+              "criteria": [
+                { "type": "lte", "metadata": "price", "values": ["100.50", "200"] }
+              ],
+              "actions": {
+                "ids": ["id1"]
+              }
+            }""");
+        testToXContentRules(content);
+    }
+
+    public void testNumericValidationWithInvalidValues() throws IOException {
+        String content = XContentHelper.stripWhitespace("""
+            {
+              "rule_id": "numeric_rule",
+              "type": "pinned",
+              "criteria": [
+                { "type": "lte", "metadata": "price", "values": ["abc"] }
+              ],
+              "actions": {
+                "ids": ["id1"]
+              }
+            }""");
+        IllegalArgumentException e = expectThrows(
+            IllegalArgumentException.class,
+            () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)
+        );
+        assertThat(e.getMessage(), containsString("Failed to build [query_rule]"));
+    }
+
+    public void testNumericValidationWithMixedValues() throws IOException {
+        String content = XContentHelper.stripWhitespace("""
+            {
+              "rule_id": "numeric_rule",
+              "type": "pinned",
+              "criteria": [
+                { "type": "lte", "metadata": "price", "values": ["100", "abc", "200"] }
+              ],
+              "actions": {
+                "ids": ["id1"]
+              }
+            }""");
+        IllegalArgumentException e = expectThrows(
+            IllegalArgumentException.class,
+            () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)
+        );
+        assertThat(e.getMessage(), containsString("Failed to build [query_rule]"));
+    }
+
+    public void testNumericValidationWithEmptyValues() throws IOException {
+        String content = XContentHelper.stripWhitespace("""
+            {
+              "rule_id": "numeric_rule",
+              "type": "pinned",
+              "criteria": [
+                { "type": "lte", "metadata": "price", "values": [] }
+              ],
+              "actions": {
+                "ids": ["id1"]
+              }
+            }""");
+        IllegalArgumentException e = expectThrows(
+            IllegalArgumentException.class,
+            () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)
+        );
+        logger.info("Actual error message: " + e.getMessage());
+        assertTrue(e.getMessage().contains("failed to parse field [criteria]"));
+    }
+
     public void testToXContent() throws IOException {
         String content = XContentHelper.stripWhitespace("""
             {
@@ -66,15 +143,7 @@ public class QueryRuleTests extends ESTestCase {
               },
               "priority": 5
             }""");
-
-        QueryRule queryRule = QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON);
-        boolean humanReadable = true;
-        BytesReference originalBytes = toShuffledXContent(queryRule, XContentType.JSON, ToXContent.EMPTY_PARAMS, humanReadable);
-        QueryRule parsed;
-        try (XContentParser parser = createParser(XContentType.JSON.xContent(), originalBytes)) {
-            parsed = QueryRule.fromXContent(parser);
-        }
-        assertToXContentEquivalent(originalBytes, toXContent(parsed, XContentType.JSON, humanReadable), XContentType.JSON);
+        testToXContentRules(content);
     }
 
     public void testToXContentEmptyCriteria() throws IOException {
@@ -85,7 +154,15 @@ public class QueryRuleTests extends ESTestCase {
               "criteria": [],
               "actions": {}
             }""");
-        expectThrows(IllegalArgumentException.class, () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON));
+        IllegalArgumentException e = expectThrows(
+            IllegalArgumentException.class,
+            () -> QueryRule.fromXContentBytes(new BytesArray(content), XContentType.JSON)
+        );
+        logger.info("Actual error message for empty criteria: " + e.getMessage());
+        assertTrue(
+            "Error message [" + e.getMessage() + "] should contain 'Failed to build [query_rule]'",
+            e.getMessage().contains("Failed to build [query_rule]")
+        );
     }
 
     public void testToXContentValidPinnedRulesWithIds() throws IOException {
@@ -333,6 +410,62 @@ public class QueryRuleTests extends ESTestCase {
         assertEquals(Collections.emptyList(), appliedQueryRules.excludedDocs());
     }
 
+    public void testValidateNumericComparisonRule() {
+        IllegalArgumentException e = expectThrows(
+            IllegalArgumentException.class,
+            () -> new QueryRule(
+                "test_rule",
+                QueryRule.QueryRuleType.PINNED,
+                List.of(new QueryRuleCriteria(QueryRuleCriteriaType.LTE, "price", List.of("not_a_number"))),
+                Map.of("ids", List.of("1")),
+                null
+            )
+        );
+        assertEquals("Input [not_a_number] is not valid for CriteriaType [lte]", e.getMessage());
+    }
+
+    public void testParseNumericComparisonRule() throws IOException {
+        XContentBuilder builder = XContentFactory.jsonBuilder();
+        builder.startObject();
+        {
+            builder.field("rule_id", "test_rule");
+            builder.field("type", "pinned");
+            builder.startArray("criteria");
+            {
+                builder.startObject();
+                builder.field("type", "lte");
+                builder.field("metadata", "price");
+                builder.startArray("values").value("100").endArray();
+                builder.endObject();
+            }
+            builder.endArray();
+            builder.startObject("actions");
+            {
+                builder.startArray("ids").value("1").endArray();
+            }
+            builder.endObject();
+        }
+        builder.endObject();
+
+        BytesReference bytesRef = BytesReference.bytes(builder);
+        QueryRule rule = QueryRule.fromXContentBytes(bytesRef, builder.contentType());
+        assertNotNull(rule);
+        assertEquals("test_rule", rule.id());
+        assertEquals(QueryRule.QueryRuleType.PINNED, rule.type());
+        assertEquals(1, rule.criteria().size());
+        assertEquals(LTE, rule.criteria().get(0).criteriaType());
+        assertEquals("100", rule.criteria().get(0).criteriaValues().get(0));
+    }
+
+    public void testIsRuleMatchWithNumericComparison() {
+        QueryRuleCriteria criteria = new QueryRuleCriteria(LTE, "price", List.of("100"));
+        QueryRule rule = new QueryRule("test_rule", QueryRule.QueryRuleType.PINNED, List.of(criteria), Map.of("ids", List.of("1")), null);
+
+        assertTrue(rule.isRuleMatch(Map.of("price", "50")));
+        assertFalse(rule.isRuleMatch(Map.of("price", "150")));
+        assertFalse(rule.isRuleMatch(Map.of("price", "not_a_number")));
+    }
+
     private void assertXContent(QueryRule queryRule, boolean humanReadable) throws IOException {
         BytesReference originalBytes = toShuffledXContent(queryRule, XContentType.JSON, ToXContent.EMPTY_PARAMS, humanReadable);
         QueryRule parsed;