From 4805250ecff95d2fc74c5f2fb5a0babed6278154 Mon Sep 17 00:00:00 2001 From: javanna Date: Tue, 2 Aug 2016 15:12:19 +0200 Subject: [PATCH] Throw ParsingException if a query is wrapped in an array Our parsing code accepted up until now queries in the following form (note that the query starts with `[`: ``` { "bool" : [ { "must" : [] } ] } ``` This would lead to a null pointer exception as most parsers assume that the field name ("must" in this example) is the first thing that can be found in a query if its json is valid, hence always non null while parsing. Truth is that the additional array layer doesn't make the json invalid, hence the following code fragment would cause NPE within ParseField, because null gets passed to `parseContext.isDeprecatedSetting`: ``` if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); } else if (parseContext.isDeprecatedSetting(currentFieldName)) { // skip } else if (token == XContentParser.Token.START_OBJECT) { ``` We could add null checks in each of our parsers in lots of places, but we rely on `currentFieldName` being non null in all of our parsers, and we should consider it a bug when these unexpected situations are not caught explicitly. It would be best to find a way to prevent such queries altogether without changing all of our parsers. The reason why such a query goes through is that we've been allowing a query to start with either `[` or `{`. The only reason I found is that we accept `match_all : []`. This seems like an undocumented corner case that we could drop support for. Then we can be stricter and accept only `{` as start token of a query. That way the only next token that the parser can encounter if the json is valid (otherwise the json parser would barf earlier) is actually a field_name, hence the assumption that all our parser makes hold. The downside of this is simply dropping support for `match_all : []` Relates to #12887 --- .../org/elasticsearch/common/ParseField.java | 2 + .../index/query/QueryParseContext.java | 6 +-- .../query/MatchAllQueryBuilderTests.java | 12 ------ .../index/query/QueryParseContextTests.java | 2 +- .../test/AbstractQueryTestCase.java | 39 +++++++++++++++++++ 5 files changed, 45 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/ParseField.java b/core/src/main/java/org/elasticsearch/common/ParseField.java index ed00394640e..7121be7d1d8 100644 --- a/core/src/main/java/org/elasticsearch/common/ParseField.java +++ b/core/src/main/java/org/elasticsearch/common/ParseField.java @@ -23,6 +23,7 @@ import org.elasticsearch.common.logging.Loggers; import java.util.Collections; import java.util.HashSet; +import java.util.Objects; import java.util.Set; /** @@ -108,6 +109,7 @@ public class ParseField { * names for this {@link ParseField}. */ boolean match(String fieldName, boolean strict) { + Objects.requireNonNull(fieldName, "fieldName cannot be null"); // if this parse field has not been completely deprecated then try to // match the preferred name if (allReplacedWith == null && fieldName.equals(name)) { diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryParseContext.java b/core/src/main/java/org/elasticsearch/index/query/QueryParseContext.java index daf0f6838b5..478bcbc51d4 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryParseContext.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryParseContext.java @@ -109,13 +109,13 @@ public class QueryParseContext implements ParseFieldMatcherSupplier { String queryName = parser.currentName(); // move to the next START_OBJECT token = parser.nextToken(); - if (token != XContentParser.Token.START_OBJECT && token != XContentParser.Token.START_ARRAY) { - throw new ParsingException(parser.getTokenLocation(), "[_na] query malformed, no start_object after query name"); + if (token != XContentParser.Token.START_OBJECT) { + throw new ParsingException(parser.getTokenLocation(), "[" + queryName + "] query malformed, no start_object after query name"); } @SuppressWarnings("unchecked") Optional result = (Optional) indicesQueriesRegistry.lookup(queryName, parseFieldMatcher, parser.getTokenLocation()).fromXContent(this); - if (parser.currentToken() == XContentParser.Token.END_OBJECT || parser.currentToken() == XContentParser.Token.END_ARRAY) { + if (parser.currentToken() == XContentParser.Token.END_OBJECT) { // if we are at END_OBJECT, move to the next one... parser.nextToken(); } diff --git a/core/src/test/java/org/elasticsearch/index/query/MatchAllQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/MatchAllQueryBuilderTests.java index ef8a4ffc28d..9195fc83ecc 100644 --- a/core/src/test/java/org/elasticsearch/index/query/MatchAllQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/MatchAllQueryBuilderTests.java @@ -24,8 +24,6 @@ import org.apache.lucene.search.Query; import org.elasticsearch.test.AbstractQueryTestCase; import java.io.IOException; -import java.util.HashMap; -import java.util.Map; import static org.hamcrest.CoreMatchers.instanceOf; @@ -36,16 +34,6 @@ public class MatchAllQueryBuilderTests extends AbstractQueryTestCase getAlternateVersions() { - Map alternateVersions = new HashMap<>(); - String queryAsString = "{\n" + - " \"match_all\": []\n" + - "}"; - alternateVersions.put(queryAsString, new MatchAllQueryBuilder()); - return alternateVersions; - } - @Override protected void doAssertLuceneQuery(MatchAllQueryBuilder queryBuilder, Query query, QueryShardContext context) throws IOException { assertThat(query, instanceOf(MatchAllDocsQuery.class)); diff --git a/core/src/test/java/org/elasticsearch/index/query/QueryParseContextTests.java b/core/src/test/java/org/elasticsearch/index/query/QueryParseContextTests.java index e6b55b0ec59..5568d2fa5a7 100644 --- a/core/src/test/java/org/elasticsearch/index/query/QueryParseContextTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/QueryParseContextTests.java @@ -113,7 +113,7 @@ public class QueryParseContextTests extends ESTestCase { try (XContentParser parser = JsonXContent.jsonXContent.createParser(source)) { QueryParseContext context = new QueryParseContext(indicesQueriesRegistry, parser, ParseFieldMatcher.STRICT); ParsingException exception = expectThrows(ParsingException.class, () -> context.parseInnerQueryBuilder()); - assertEquals("[_na] query malformed, no start_object after query name", exception.getMessage()); + assertEquals("[foo] query malformed, no start_object after query name", exception.getMessage()); } source = "{ \"foo\" : {} }"; diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java index ad2f6974607..215bfe5f18b 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java @@ -122,6 +122,7 @@ import static java.util.Collections.emptyList; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.either; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.not; @@ -313,6 +314,44 @@ public abstract class AbstractQueryTestCase> } } + /** + * Test that wraps the randomly generated query into an array as follows: { "query_name" : [{}]} + * This causes unexpected situations in parser code that may not be handled properly. + */ + public void testQueryWrappedInArray() throws IOException { + QB queryBuilder = createTestQueryBuilder(); + String validQuery = queryBuilder.toString(); + String queryName = queryBuilder.getName(); + int i = validQuery.indexOf("\"" + queryName + "\""); + assertThat(i, greaterThan(0)); + + int insertionPosition; + for (insertionPosition = i; insertionPosition < validQuery.length(); insertionPosition++) { + if (validQuery.charAt(insertionPosition) == ':') { + break; + } + } + insertionPosition++; + + int endArrayPosition; + for (endArrayPosition = validQuery.length() - 1; endArrayPosition >= 0; endArrayPosition--) { + if (validQuery.charAt(endArrayPosition) == '}') { + break; + } + } + + String testQuery = validQuery.substring(0, insertionPosition) + "[" + + validQuery.substring(insertionPosition, endArrayPosition) + "]" + + validQuery.substring(endArrayPosition, validQuery.length()); + + try { + parseQuery(testQuery); + fail("some parsing exception expected for query: " + testQuery); + } catch (ParsingException e) { + assertEquals("[" + queryName + "] query malformed, no start_object after query name", e.getMessage()); + } + } + /** * Returns alternate string representation of the query that need to be tested as they are never used as output * of {@link QueryBuilder#toXContent(XContentBuilder, ToXContent.Params)}. By default there are no alternate versions.