Skip to content

Commit f6538e8

Browse files
mridula-s109elasticsearchmachinekderussoMikep86
authored
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 <[email protected]> * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]> Co-authored-by: Kathleen DeRusso <[email protected]> Co-authored-by: Mike Pellegrini <[email protected]>
1 parent 79e776a commit f6538e8

File tree

6 files changed

+283
-26
lines changed

6 files changed

+283
-26
lines changed

‎docs/changelog/122823.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 122823
2+
summary: Prevent Query Rule Creation with Invalid Numeric Match Criteria
3+
area: Relevance
4+
type: bug
5+
issues: []

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

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,16 @@ teardown:
1515
ruleset_id: test-query-ruleset-recreating
1616
ignore: 404
1717

18+
- do:
19+
query_rules.delete_ruleset:
20+
ruleset_id: invalid_numeric_test
21+
ignore: 404
22+
23+
- do:
24+
query_rules.delete_ruleset:
25+
ruleset_id: multiple_values_test
26+
ignore: 404
27+
1828
---
1929
'Create Query Ruleset':
2030
- do:
@@ -160,3 +170,51 @@ teardown:
160170
- 'id2'
161171

162172
- match: { error.type: 'security_exception' }
173+
174+
---
175+
"Test numeric comparison rule validation":
176+
- skip:
177+
features: headers
178+
179+
- requires:
180+
cluster_features: "query_rules.numeric_validation"
181+
reason: "Fixed bug to enforce numeric rule validation starting with 8.19"
182+
183+
- do:
184+
catch: /Input \[not_a_number\] is not valid for CriteriaType \[lte\]/
185+
query_rules.put_ruleset:
186+
ruleset_id: invalid_numeric_test
187+
body:
188+
rules:
189+
- rule_id: invalid_numeric_rule
190+
type: pinned
191+
criteria:
192+
- type: lte
193+
metadata: price
194+
values: ["not_a_number"]
195+
actions:
196+
ids: ["1"]
197+
198+
---
199+
"Test numeric comparison with multiple values":
200+
- skip:
201+
features: headers
202+
203+
- requires:
204+
cluster_features: "query_rules.numeric_validation"
205+
reason: "Fixed bug to enforce numeric rule validation starting with 8.19"
206+
207+
- do:
208+
catch: /Input \[not_a_number\] is not valid for CriteriaType \[lte\]/
209+
query_rules.put_ruleset:
210+
ruleset_id: multiple_values_test
211+
body:
212+
rules:
213+
- rule_id: multiple_values_rule
214+
type: pinned
215+
criteria:
216+
- type: lte
217+
metadata: price
218+
values: ["100", "not_a_number"]
219+
actions:
220+
ids: ["1"]

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import org.elasticsearch.features.FeatureSpecification;
1111
import org.elasticsearch.features.NodeFeature;
12+
import org.elasticsearch.xpack.application.rules.QueryRule;
1213

1314
import java.util.Set;
1415

@@ -18,4 +19,9 @@ public class EnterpriseSearchFeatures implements FeatureSpecification {
1819
public Set<NodeFeature> getFeatures() {
1920
return Set.of();
2021
}
22+
23+
@Override
24+
public Set<NodeFeature> getTestFeatures() {
25+
return Set.of(QueryRule.NUMERIC_VALIDATION);
26+
}
2127
}

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import org.elasticsearch.common.io.stream.Writeable;
1717
import org.elasticsearch.common.xcontent.XContentHelper;
1818
import org.elasticsearch.core.Nullable;
19+
import org.elasticsearch.features.NodeFeature;
1920
import org.elasticsearch.xcontent.ConstructingObjectParser;
2021
import org.elasticsearch.xcontent.ParseField;
2122
import org.elasticsearch.xcontent.ToXContentObject;
@@ -81,6 +82,8 @@ public String toString() {
8182
}
8283
}
8384

85+
public static final NodeFeature NUMERIC_VALIDATION = new NodeFeature("query_rules.numeric_validation", true);
86+
8487
/**
8588
* Public constructor.
8689
*
@@ -140,7 +143,6 @@ public QueryRule(StreamInput in) throws IOException {
140143
}
141144

142145
private void validate() {
143-
144146
if (priority != null && (priority < MIN_PRIORITY || priority > MAX_PRIORITY)) {
145147
throw new IllegalArgumentException("Priority was " + priority + ", must be between " + MIN_PRIORITY + " and " + MAX_PRIORITY);
146148
}
@@ -155,6 +157,13 @@ private void validate() {
155157
throw new ElasticsearchParseException(type.toString() + " query rule actions must contain only one of either ids or docs");
156158
}
157159
}
160+
161+
criteria.forEach(criterion -> {
162+
List<Object> values = criterion.criteriaValues();
163+
if (values != null) {
164+
values.forEach(criterion.criteriaType()::validateInput);
165+
}
166+
});
158167
}
159168

160169
private void validateIdOrDocAction(Object action) {

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

Lines changed: 61 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,25 +20,26 @@
2020
import org.elasticsearch.xpack.application.search.TemplateParamValidator;
2121
import org.elasticsearch.xpack.core.action.util.PageParams;
2222

23+
import java.math.BigDecimal;
24+
import java.math.RoundingMode;
2325
import java.util.ArrayList;
24-
import java.util.Arrays;
2526
import java.util.Collections;
2627
import java.util.List;
2728
import java.util.Locale;
2829
import java.util.Map;
30+
import java.util.Set;
2931

3032
import static org.elasticsearch.test.ESTestCase.generateRandomStringArray;
3133
import static org.elasticsearch.test.ESTestCase.randomAlphaOfLengthBetween;
3234
import static org.elasticsearch.test.ESTestCase.randomBoolean;
35+
import static org.elasticsearch.test.ESTestCase.randomDoubleBetween;
3336
import static org.elasticsearch.test.ESTestCase.randomFrom;
3437
import static org.elasticsearch.test.ESTestCase.randomIdentifier;
3538
import static org.elasticsearch.test.ESTestCase.randomIntBetween;
36-
import static org.elasticsearch.test.ESTestCase.randomList;
3739
import static org.elasticsearch.test.ESTestCase.randomLongBetween;
3840
import static org.elasticsearch.test.ESTestCase.randomMap;
3941
import static org.elasticsearch.xpack.application.rules.QueryRule.MAX_PRIORITY;
4042
import static org.elasticsearch.xpack.application.rules.QueryRule.MIN_PRIORITY;
41-
import static org.elasticsearch.xpack.application.rules.QueryRuleCriteriaType.ALWAYS;
4243

4344
public final class EnterpriseSearchModuleTestUtils {
4445

@@ -86,19 +87,48 @@ public static Map<String, Object> randomSearchApplicationQueryParams() {
8687
return randomMap(0, 10, () -> Tuple.tuple(randomIdentifier(), randomAlphaOfLengthBetween(0, 10)));
8788
}
8889

89-
public static QueryRuleCriteria randomQueryRuleCriteria() {
90-
// We intentionally don't allow ALWAYS criteria in this method, since we want to test parsing metadata and values
91-
QueryRuleCriteriaType type = randomFrom(Arrays.stream(QueryRuleCriteriaType.values()).filter(t -> t != ALWAYS).toList());
92-
return new QueryRuleCriteria(type, randomAlphaOfLengthBetween(1, 10), randomList(1, 5, () -> randomAlphaOfLengthBetween(1, 10)));
93-
}
94-
9590
public static QueryRule randomQueryRule() {
96-
String id = randomIdentifier();
91+
String ruleId = randomIdentifier();
9792
QueryRule.QueryRuleType type = randomFrom(QueryRule.QueryRuleType.values());
98-
List<QueryRuleCriteria> criteria = List.of(randomQueryRuleCriteria());
99-
Map<String, Object> actions = Map.of(randomFrom("ids", "docs"), List.of(randomAlphaOfLengthBetween(2, 10)));
100-
Integer priority = randomQueryRulePriority();
101-
return new QueryRule(id, type, criteria, actions, priority);
93+
List<QueryRuleCriteria> criteria = new ArrayList<>();
94+
int numCriteria = randomIntBetween(1, 3);
95+
96+
for (int i = 0; i < numCriteria; i++) {
97+
criteria.add(randomQueryRuleCriteria());
98+
}
99+
100+
return new QueryRule(ruleId, type, criteria, randomQueryRuleActions(), randomQueryRulePriority());
101+
}
102+
103+
public static QueryRuleCriteria randomQueryRuleCriteria() {
104+
Set<QueryRuleCriteriaType> numericValueCriteriaType = Set.of(
105+
QueryRuleCriteriaType.GT,
106+
QueryRuleCriteriaType.GTE,
107+
QueryRuleCriteriaType.LT,
108+
QueryRuleCriteriaType.LTE
109+
);
110+
QueryRuleCriteriaType criteriaType = randomFrom(QueryRuleCriteriaType.values());
111+
112+
String metadata;
113+
List<Object> values;
114+
if (criteriaType == QueryRuleCriteriaType.ALWAYS) {
115+
metadata = null;
116+
values = null;
117+
} else {
118+
metadata = randomAlphaOfLengthBetween(3, 10);
119+
values = new ArrayList<>();
120+
121+
int numValues = randomIntBetween(1, 3);
122+
for (int i = 0; i < numValues; i++) {
123+
values.add(
124+
numericValueCriteriaType.contains(criteriaType)
125+
? BigDecimal.valueOf(randomDoubleBetween(0, 1000, true)).setScale(2, RoundingMode.HALF_UP).toString()
126+
: randomAlphaOfLengthBetween(3, 10)
127+
);
128+
}
129+
}
130+
131+
return new QueryRuleCriteria(criteriaType, metadata, values);
102132
}
103133

104134
public static Integer randomQueryRulePriority() {
@@ -115,8 +145,24 @@ public static QueryRuleset randomQueryRuleset() {
115145
return new QueryRuleset(id, rules);
116146
}
117147

148+
public static Map<String, Object> randomQueryRuleActions() {
149+
if (randomBoolean()) {
150+
return Map.of(QueryRule.IDS_FIELD.getPreferredName(), List.of(randomAlphaOfLengthBetween(3, 10)));
151+
}
152+
return Map.of(
153+
QueryRule.DOCS_FIELD.getPreferredName(),
154+
List.of(
155+
Map.of(
156+
QueryRule.INDEX_FIELD.getPreferredName(),
157+
randomAlphaOfLengthBetween(3, 10),
158+
QueryRule.ID_FIELD.getPreferredName(),
159+
randomAlphaOfLengthBetween(3, 10)
160+
)
161+
)
162+
);
163+
}
164+
118165
public static Map<String, Object> randomMatchCriteria() {
119166
return randomMap(1, 3, () -> Tuple.tuple(randomIdentifier(), randomAlphaOfLengthBetween(0, 10)));
120167
}
121-
122168
}

0 commit comments

Comments
 (0)