Introduced new setting search.query.max_query_string_length (#19491) (#19814)

Co-authored-by: Nils Bandener <33570290+nibix@users.noreply.github.com>

Author: cwperks

CHANGELOG.md

@@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ## [Unreleased 2.x]
 ### Added
 - Reject close index requests, while remote store migration is in progress.([#18327](https://github.com/opensearch-project/OpenSearch/pull/18327))
+- New cluster setting search.query.max_query_string_length ([#19491](https://github.com/opensearch-project/OpenSearch/pull/19491))
 
 ### Dependencies
 - Bump `netty` from 4.1.118.Final to 4.1.125.Final ([#18192](https://github.com/opensearch-project/OpenSearch/pull/18192)) ([#19270](https://github.com/opensearch-project/OpenSearch/pull/19270))

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<String> hitIds = new HashSet<>();

server/src/main/java/org/opensearch/common/settings/ClusterSettings.java

@@ -556,6 +556,7 @@ public void apply(Settings value, Settings current, Settings previous) {
                 SearchService.MAX_PIT_KEEPALIVE_SETTING,
                 SearchService.MAX_AGGREGATION_REWRITE_FILTERS,
                 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,

server/src/main/java/org/opensearch/index/search/QueryStringQueryParser.java

@@ -58,6 +58,7 @@
 import org.apache.lucene.util.BytesRef;
 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;
@@ -69,6 +70,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;
@@ -94,6 +96,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<String, Float> fieldsAndWeights;
@@ -860,6 +864,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;
+    }
 }

server/src/main/java/org/opensearch/search/SearchService.java

@@ -89,6 +89,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;
@@ -327,6 +328,15 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv
         Setting.Property.Dynamic
     );
 
+    public static final Setting<Integer> 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<Boolean> CLUSTER_ALLOW_DERIVED_FIELD_SETTING = Setting.boolSetting(
         "search.derived_field.enabled",
         true,
@@ -469,6 +479,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);