From 6d5b4a78fefdf66e0a3fa7aa32681ca6b7cf99fe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christoph=20B=C3=BCscher?= <christoph@elastic.co>
Date: Thu, 23 Jun 2016 17:04:56 +0200
Subject: [PATCH] Make parsing of bool queries stricter

Currently we don't throw an error when there is more than one query clause
specified in a must/must_not/should/filter object of the bool query without
using array notation, e.g.:
 { "bool" : { "must" : { "match" : { ... }, "match": { ... }}}}

In these cases, only the first query will be parsed and further behaviour is
unspecified, possibly leading to silently ignoring the rest of the query.
Instead we should throw a ParsingException if we don't encounter an END_OBJECT
token after having parsed the query clause.
---
 .../index/query/BoolQueryBuilder.java         |  4 ++++
 .../index/query/BoolQueryBuilderTests.java    | 23 +++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java
index 052ddd85663..c37da43d399 100644
--- a/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java
+++ b/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java
@@ -340,6 +340,10 @@ public class BoolQueryBuilder extends AbstractQueryBuilder<BoolQueryBuilder> {
                 default:
                     throw new ParsingException(parser.getTokenLocation(), "[bool] query does not support [" + currentFieldName + "]");
                 }
+                if (parser.currentToken() != XContentParser.Token.END_OBJECT) {
+                    throw new ParsingException(parser.getTokenLocation(),
+                            "expected [END_OBJECT] but got [{}], possibly too many query clauses", parser.currentToken());
+                }
             } else if (token == XContentParser.Token.START_ARRAY) {
                 while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
                     switch (currentFieldName) {
diff --git a/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java
index 89da227df87..3839254bf1d 100644
--- a/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java
+++ b/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java
@@ -25,6 +25,7 @@ import org.apache.lucene.search.ConstantScoreQuery;
 import org.apache.lucene.search.MatchAllDocsQuery;
 import org.apache.lucene.search.Query;
 import org.elasticsearch.common.ParseFieldMatcher;
+import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
@@ -370,6 +371,28 @@ public class BoolQueryBuilderTests extends AbstractQueryTestCase<BoolQueryBuilde
         assertThat(ex.getMessage(), startsWith("query malformed, empty clause found at"));
     }
 
+    /**
+     * test that unknown query names in the clauses throw an error
+     */
+    public void testUnknownQueryName() throws IOException {
+        String query = "{\"bool\" : {\"must\" : { \"unknown_query\" : { } } } }";
+
+        ParsingException ex = expectThrows(ParsingException.class, () -> parseQuery(query, ParseFieldMatcher.EMPTY));
+        assertEquals("no [query] registered for [unknown_query]", ex.getMessage());
+    }
+
+    /**
+     * test that two queries in object throws error
+     */
+    public void testTooManyQueriesInObject() throws IOException {
+        String clauseType = randomFrom(new String[] {"must", "should", "must_not", "filter"});
+        // should also throw error if invalid query is preceded by a valid one
+        String query = "{\"bool\" : {\"" + clauseType
+                + "\" : { \"match\" : { \"foo\" : \"bar\" } , \"match\" : { \"baz\" : \"buzz\" } } } }";
+        ParsingException ex = expectThrows(ParsingException.class, () -> parseQuery(query, ParseFieldMatcher.EMPTY));
+        assertEquals("expected [END_OBJECT] but got [FIELD_NAME], possibly too many query clauses", ex.getMessage());
+    }
+
     public void testRewrite() throws IOException {
         BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder();
         boolean mustRewrite = false;