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
This commit is contained in:
javanna 2016-08-02 15:12:19 +02:00 committed by Luca Cavanna
parent 51bbe2c5c4
commit 4805250ecf
5 changed files with 45 additions and 16 deletions

View File

@ -23,6 +23,7 @@ import org.elasticsearch.common.logging.Loggers;
import java.util.Collections; import java.util.Collections;
import java.util.HashSet; import java.util.HashSet;
import java.util.Objects;
import java.util.Set; import java.util.Set;
/** /**
@ -108,6 +109,7 @@ public class ParseField {
* names for this {@link ParseField}. * names for this {@link ParseField}.
*/ */
boolean match(String fieldName, boolean strict) { 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 // if this parse field has not been completely deprecated then try to
// match the preferred name // match the preferred name
if (allReplacedWith == null && fieldName.equals(name)) { if (allReplacedWith == null && fieldName.equals(name)) {

View File

@ -109,13 +109,13 @@ public class QueryParseContext implements ParseFieldMatcherSupplier {
String queryName = parser.currentName(); String queryName = parser.currentName();
// move to the next START_OBJECT // move to the next START_OBJECT
token = parser.nextToken(); token = parser.nextToken();
if (token != XContentParser.Token.START_OBJECT && token != XContentParser.Token.START_ARRAY) { if (token != XContentParser.Token.START_OBJECT) {
throw new ParsingException(parser.getTokenLocation(), "[_na] query malformed, no start_object after query name"); throw new ParsingException(parser.getTokenLocation(), "[" + queryName + "] query malformed, no start_object after query name");
} }
@SuppressWarnings("unchecked") @SuppressWarnings("unchecked")
Optional<QueryBuilder> result = (Optional<QueryBuilder>) indicesQueriesRegistry.lookup(queryName, parseFieldMatcher, Optional<QueryBuilder> result = (Optional<QueryBuilder>) indicesQueriesRegistry.lookup(queryName, parseFieldMatcher,
parser.getTokenLocation()).fromXContent(this); 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... // if we are at END_OBJECT, move to the next one...
parser.nextToken(); parser.nextToken();
} }

View File

@ -24,8 +24,6 @@ import org.apache.lucene.search.Query;
import org.elasticsearch.test.AbstractQueryTestCase; import org.elasticsearch.test.AbstractQueryTestCase;
import java.io.IOException; import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.instanceOf;
@ -36,16 +34,6 @@ public class MatchAllQueryBuilderTests extends AbstractQueryTestCase<MatchAllQue
return new MatchAllQueryBuilder(); return new MatchAllQueryBuilder();
} }
@Override
protected Map<String, MatchAllQueryBuilder> getAlternateVersions() {
Map<String, MatchAllQueryBuilder> alternateVersions = new HashMap<>();
String queryAsString = "{\n" +
" \"match_all\": []\n" +
"}";
alternateVersions.put(queryAsString, new MatchAllQueryBuilder());
return alternateVersions;
}
@Override @Override
protected void doAssertLuceneQuery(MatchAllQueryBuilder queryBuilder, Query query, QueryShardContext context) throws IOException { protected void doAssertLuceneQuery(MatchAllQueryBuilder queryBuilder, Query query, QueryShardContext context) throws IOException {
assertThat(query, instanceOf(MatchAllDocsQuery.class)); assertThat(query, instanceOf(MatchAllDocsQuery.class));

View File

@ -113,7 +113,7 @@ public class QueryParseContextTests extends ESTestCase {
try (XContentParser parser = JsonXContent.jsonXContent.createParser(source)) { try (XContentParser parser = JsonXContent.jsonXContent.createParser(source)) {
QueryParseContext context = new QueryParseContext(indicesQueriesRegistry, parser, ParseFieldMatcher.STRICT); QueryParseContext context = new QueryParseContext(indicesQueriesRegistry, parser, ParseFieldMatcher.STRICT);
ParsingException exception = expectThrows(ParsingException.class, () -> context.parseInnerQueryBuilder()); 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\" : {} }"; source = "{ \"foo\" : {} }";

View File

@ -122,6 +122,7 @@ import static java.util.Collections.emptyList;
import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.either; import static org.hamcrest.Matchers.either;
import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.not;
@ -313,6 +314,44 @@ public abstract class AbstractQueryTestCase<QB extends AbstractQueryBuilder<QB>>
} }
} }
/**
* 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 * 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. * of {@link QueryBuilder#toXContent(XContentBuilder, ToXContent.Params)}. By default there are no alternate versions.