diff --git a/CHANGELOG.md b/CHANGELOG.md index f1a661ea15222..b800a4af4061a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Implement GRPC Nested query ([#19453](https://github.com/opensearch-project/OpenSearch/pull/19453)) - Add sub aggregation support for histogram aggregation using skiplist ([19438](https://github.com/opensearch-project/OpenSearch/pull/19438)) - Optimization in String Terms Aggregation query for Large Bucket Counts([#18732](https://github.com/opensearch-project/OpenSearch/pull/18732)) +- New cluster setting search.query.max_query_string_length ([#19491](https://github.com/opensearch-project/OpenSearch/pull/19491)) ### Changed - Refactor `if-else` chains to use `Java 17 pattern matching switch expressions`(([#18965](https://github.com/opensearch-project/OpenSearch/pull/18965)) diff --git a/server/src/internalClusterTest/java/org/opensearch/search/query/SimpleQueryStringIT.java b/server/src/internalClusterTest/java/org/opensearch/search/query/SimpleQueryStringIT.java index ca32c854897c0..c30497828fc82 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/query/SimpleQueryStringIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/query/SimpleQueryStringIT.java @@ -80,6 +80,7 @@ import static org.opensearch.index.query.QueryBuilders.termQuery; import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING; import static org.opensearch.search.SearchService.INDICES_MAX_CLAUSE_COUNT_SETTING; +import static org.opensearch.search.SearchService.SEARCH_MAX_QUERY_STRING_LENGTH; import static org.opensearch.test.StreamsUtils.copyToStringFromClasspath; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertFailures; @@ -767,6 +768,34 @@ public void testDynamicClauseCountUpdate() throws Exception { ); } + public void testMaxQueryStringLength() throws Exception { + try { + String indexBody = copyToStringFromClasspath("/org/opensearch/search/query/all-query-index.json"); + assertAcked(prepareCreate("test").setSource(indexBody, MediaTypeRegistry.JSON)); + ensureGreen("test"); + + assertAcked( + client().admin() + .cluster() + .prepareUpdateSettings() + .setTransientSettings(Settings.builder().put(SEARCH_MAX_QUERY_STRING_LENGTH.getKey(), 10)) + ); + + SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, () -> { + client().prepareSearch("test").setQuery(queryStringQuery("foo OR foo OR foo OR foo")).get(); + }); + + assertThat(e.getDetailedMessage(), containsString("Query string length exceeds max allowed length 10")); + } finally { + assertAcked( + client().admin() + .cluster() + .prepareUpdateSettings() + .setTransientSettings(Settings.builder().putNull(SEARCH_MAX_QUERY_STRING_LENGTH.getKey())) + ); + } + } + private void assertHits(SearchHits hits, String... ids) { assertThat(hits.getTotalHits().value(), equalTo((long) ids.length)); Set hitIds = new HashSet<>(); diff --git a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java index 9140401c70b7b..3cbf6007cd563 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -570,6 +570,7 @@ public void apply(Settings value, Settings current, Settings previous) { SearchService.MAX_AGGREGATION_REWRITE_FILTERS, SearchService.AGGREGATION_REWRITE_FILTER_SEGMENT_THRESHOLD, SearchService.INDICES_MAX_CLAUSE_COUNT_SETTING, + SearchService.SEARCH_MAX_QUERY_STRING_LENGTH, SearchService.CARDINALITY_AGGREGATION_PRUNING_THRESHOLD, SearchService.KEYWORD_INDEX_OR_DOC_VALUES_ENABLED, CreatePitController.PIT_INIT_KEEP_ALIVE, diff --git a/server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java b/server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java index afa3acc28fc54..34d3a7d9c90ba 100644 --- a/server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java +++ b/server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java @@ -60,6 +60,7 @@ import org.apache.lucene.util.automaton.TooComplexToDeterminizeException; import org.opensearch.common.lucene.search.Queries; import org.opensearch.common.regex.Regex; +import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.Fuzziness; import org.opensearch.common.util.io.IOUtils; import org.opensearch.index.IndexSettings; @@ -71,6 +72,7 @@ import org.opensearch.index.query.ExistsQueryBuilder; import org.opensearch.index.query.MultiMatchQueryBuilder; import org.opensearch.index.query.QueryShardContext; +import org.opensearch.search.SearchService; import java.io.IOException; import java.time.ZoneId; @@ -96,6 +98,8 @@ */ public class QueryStringQueryParser extends XQueryParser { private static final String EXISTS_FIELD = "_exists_"; + @SuppressWarnings("NonFinalStaticField") + private static int maxQueryStringLength = SearchService.SEARCH_MAX_QUERY_STRING_LENGTH.get(Settings.EMPTY); private final QueryShardContext context; private final Map fieldsAndWeights; @@ -867,6 +871,23 @@ public Query parse(String query) throws ParseException { if (query.trim().isEmpty()) { return Queries.newMatchNoDocsQuery("Matching no documents because no terms present"); } + if (query.length() > maxQueryStringLength) { + throw new ParseException( + "Query string length exceeds max allowed length " + + maxQueryStringLength + + " (" + + SearchService.SEARCH_MAX_QUERY_STRING_LENGTH.getKey() + + "); actual length: " + + query.length() + ); + } return super.parse(query); } + + /** + * Sets the maximum allowed length for query strings. This should be only called from SearchService on settings updates. + */ + public static void setMaxQueryStringLength(int maxQueryStringLength) { + QueryStringQueryParser.maxQueryStringLength = maxQueryStringLength; + } } diff --git a/server/src/main/java/org/opensearch/search/SearchService.java b/server/src/main/java/org/opensearch/search/SearchService.java index eeb4978d4c1f8..66d00e7dffe07 100644 --- a/server/src/main/java/org/opensearch/search/SearchService.java +++ b/server/src/main/java/org/opensearch/search/SearchService.java @@ -93,6 +93,7 @@ import org.opensearch.index.query.QueryRewriteContext; import org.opensearch.index.query.QueryShardContext; import org.opensearch.index.query.Rewriteable; +import org.opensearch.index.search.QueryStringQueryParser; import org.opensearch.index.shard.IndexEventListener; import org.opensearch.index.shard.IndexShard; import org.opensearch.index.shard.SearchOperationListener; @@ -369,6 +370,15 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv Setting.Property.Dynamic ); + public static final Setting SEARCH_MAX_QUERY_STRING_LENGTH = Setting.intSetting( + "search.query.max_query_string_length", + 32_000, + 1, + Integer.MAX_VALUE, + Setting.Property.NodeScope, + Setting.Property.Dynamic + ); + public static final Setting CLUSTER_ALLOW_DERIVED_FIELD_SETTING = Setting.boolSetting( "search.derived_field.enabled", true, @@ -524,6 +534,10 @@ public SearchService( IndexSearcher.setMaxClauseCount(INDICES_MAX_CLAUSE_COUNT_SETTING.get(settings)); clusterService.getClusterSettings().addSettingsUpdateConsumer(INDICES_MAX_CLAUSE_COUNT_SETTING, IndexSearcher::setMaxClauseCount); + QueryStringQueryParser.setMaxQueryStringLength(SEARCH_MAX_QUERY_STRING_LENGTH.get(settings)); + clusterService.getClusterSettings() + .addSettingsUpdateConsumer(SEARCH_MAX_QUERY_STRING_LENGTH, QueryStringQueryParser::setMaxQueryStringLength); + allowDerivedField = CLUSTER_ALLOW_DERIVED_FIELD_SETTING.get(settings); clusterService.getClusterSettings().addSettingsUpdateConsumer(CLUSTER_ALLOW_DERIVED_FIELD_SETTING, this::setAllowDerivedField);