From 331064ed4812590eb5fe54022be4f9b73e2660ea Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Mon, 9 Apr 2018 12:41:01 +0200 Subject: [PATCH 1/5] Fail _search request with trailing tokens This change validates that the `_search` request does not have trailing tokens after the main object and fails the request with a parsing exception otherwise. Closes #28995 --- .../migration/migrate_7_0/search.asciidoc | 5 +++++ .../search/builder/SearchSourceBuilder.java | 5 +++++ .../builder/SearchSourceBuilderTests.java | 20 +++++++++++++++++++ 3 files changed, 30 insertions(+) diff --git a/docs/reference/migration/migrate_7_0/search.asciidoc b/docs/reference/migration/migrate_7_0/search.asciidoc index 0d3770993b2ff..92c684d8a125c 100644 --- a/docs/reference/migration/migrate_7_0/search.asciidoc +++ b/docs/reference/migration/migrate_7_0/search.asciidoc @@ -70,3 +70,8 @@ Executing a Regexp Query with a long regex string may degrade search performance To safeguard against this, the maximum length of regex that can be used in a Regexp Query request has been limited to 1000. This default maximum can be changed for a particular index with the index setting `index.max_regex_length`. + +==== Invalid `_search` request body + +Search requests with extra content after the main object will no longer be accepted +by the `_search` endpoint. A parsing exception will be thrown instead. diff --git a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java index 815abf1b7a7c4..37fbe8e2e36c3 100644 --- a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java @@ -1106,6 +1106,11 @@ public void parseXContent(XContentParser parser) throws IOException { parser.getTokenLocation()); } } + // check for extra token at the end + token = parser.nextToken(); + if (token != null) { + throw new ParsingException(parser.getTokenLocation(), "Unexpected token [" + token + "] found after the main object."); + } } @Override diff --git a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index 66d6f68b8a4aa..5e5205dec6cd8 100644 --- a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -19,20 +19,28 @@ package org.elasticsearch.search.builder; +import com.fasterxml.jackson.core.JsonParseException; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.ParsingException; +import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.xcontent.DeprecationHandler; +import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.index.mapper.DocumentMapper; +import org.elasticsearch.index.mapper.MapperParsingException; import org.elasticsearch.index.query.BoolQueryBuilder; import org.elasticsearch.index.query.MatchNoneQueryBuilder; import org.elasticsearch.index.query.QueryBuilders; @@ -67,6 +75,18 @@ public void testFromXContent() throws IOException { assertParseSearchSource(testSearchSourceBuilder, createParser(builder)); } + public void testFromXContentInvalid() throws IOException { + try (XContentParser parser = createParser(JsonXContent.jsonXContent, "{}}")) { + JsonParseException exc = expectThrows(JsonParseException.class, () -> SearchSourceBuilder.fromXContent(parser)); + assertThat(exc.getMessage(), containsString("Unexpected close marker")); + } + + try (XContentParser parser = createParser(JsonXContent.jsonXContent, "{}{}")) { + ParsingException exc = expectThrows(ParsingException.class, () -> SearchSourceBuilder.fromXContent(parser)); + assertThat(exc.getDetailedMessage(), containsString("found after the main object")); + } + } + private static void assertParseSearchSource(SearchSourceBuilder testBuilder, XContentParser parser) throws IOException { if (randomBoolean()) { parser.nextToken(); // sometimes we move it on the START_OBJECT to From 180da97f8134b27c16fff50c914ae4d269ddd00c Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Mon, 9 Apr 2018 13:53:58 +0200 Subject: [PATCH 2/5] do not check for extra tokens if outside of the search action --- .../rest/action/search/RestSearchAction.java | 4 ++-- .../search/builder/SearchSourceBuilder.java | 21 ++++++++++++------- .../builder/SearchSourceBuilderTests.java | 4 ++-- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java b/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java index 011b24309f8f4..184a8d364c6fe 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java @@ -109,7 +109,7 @@ public static void parseSearchRequest(SearchRequest searchRequest, RestRequest r } searchRequest.indices(Strings.splitStringByCommaToArray(request.param("index"))); if (requestContentParser != null) { - searchRequest.source().parseXContent(requestContentParser); + searchRequest.source().parseXContent(requestContentParser, true); } final int batchedReduceSize = request.paramAsInt("batched_reduce_size", searchRequest.getBatchedReduceSize()); @@ -128,7 +128,7 @@ public static void parseSearchRequest(SearchRequest searchRequest, RestRequest r // only set if we have the parameter passed to override the cluster-level default searchRequest.allowPartialSearchResults(request.paramAsBoolean("allow_partial_search_results", null)); } - + // do not allow 'query_and_fetch' or 'dfs_query_and_fetch' search types // from the REST layer. these modes are an internal optimization and should // not be specified explicitly by the user. diff --git a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java index 37fbe8e2e36c3..b2d72c3fcb83b 100644 --- a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java @@ -951,12 +951,18 @@ private SearchSourceBuilder shallowCopy(QueryBuilder queryBuilder, QueryBuilder return rewrittenBuilder; } + public void parseXContent(XContentParser parser) throws IOException { + parseXContent(parser, false); + } + /** * Parse some xContent into this SearchSourceBuilder, overwriting any values specified in the xContent. Use this if you need to set up - * different defaults than a regular SearchSourceBuilder would have and use - * {@link #fromXContent(XContentParser)} if you have normal defaults. + * different defaults than a regular SearchSourceBuilder would have and use {@link #fromXContent(XContentParser)} if you have normal + * defaults. + * @param parser The xContent parser. + * @param checkTrailingTokens If true throws a parsing exception when extra tokens are found the main object. */ - public void parseXContent(XContentParser parser) throws IOException { + public void parseXContent(XContentParser parser, boolean checkTrailingTokens) throws IOException { XContentParser.Token token = parser.currentToken(); String currentFieldName = null; if (token != XContentParser.Token.START_OBJECT && (token = parser.nextToken()) != XContentParser.Token.START_OBJECT) { @@ -1106,10 +1112,11 @@ public void parseXContent(XContentParser parser) throws IOException { parser.getTokenLocation()); } } - // check for extra token at the end - token = parser.nextToken(); - if (token != null) { - throw new ParsingException(parser.getTokenLocation(), "Unexpected token [" + token + "] found after the main object."); + if (checkTrailingTokens) { + token = parser.nextToken(); + if (token != null) { + throw new ParsingException(parser.getTokenLocation(), "Unexpected token [" + token + "] found after the main object."); + } } } diff --git a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index 5e5205dec6cd8..3224e52317ef9 100644 --- a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -77,12 +77,12 @@ public void testFromXContent() throws IOException { public void testFromXContentInvalid() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, "{}}")) { - JsonParseException exc = expectThrows(JsonParseException.class, () -> SearchSourceBuilder.fromXContent(parser)); + JsonParseException exc = expectThrows(JsonParseException.class, () -> SearchSourceBuilder.fromXContent(parser, true)); assertThat(exc.getMessage(), containsString("Unexpected close marker")); } try (XContentParser parser = createParser(JsonXContent.jsonXContent, "{}{}")) { - ParsingException exc = expectThrows(ParsingException.class, () -> SearchSourceBuilder.fromXContent(parser)); + ParsingException exc = expectThrows(ParsingException.class, () -> SearchSourceBuilder.fromXContent(parser, true)); assertThat(exc.getDetailedMessage(), containsString("found after the main object")); } } From b19639b5fae542536332062c07e4e39f608b6baf Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Mon, 9 Apr 2018 13:55:33 +0200 Subject: [PATCH 3/5] fix compil --- .../search/builder/SearchSourceBuilderTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index 3224e52317ef9..5e98412fe80de 100644 --- a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -77,12 +77,12 @@ public void testFromXContent() throws IOException { public void testFromXContentInvalid() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, "{}}")) { - JsonParseException exc = expectThrows(JsonParseException.class, () -> SearchSourceBuilder.fromXContent(parser, true)); + JsonParseException exc = expectThrows(JsonParseException.class, () -> new SearchSourceBuilder().parseXContent(parser, true)); assertThat(exc.getMessage(), containsString("Unexpected close marker")); } try (XContentParser parser = createParser(JsonXContent.jsonXContent, "{}{}")) { - ParsingException exc = expectThrows(ParsingException.class, () -> SearchSourceBuilder.fromXContent(parser, true)); + ParsingException exc = expectThrows(ParsingException.class, () -> new SearchSourceBuilder().parseXContent(parser, true)); assertThat(exc.getDetailedMessage(), containsString("found after the main object")); } } From 267b7188e695ed46205139e41c4ed7a6aa35ae67 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Tue, 10 Apr 2018 08:05:07 +0200 Subject: [PATCH 4/5] remove unused import --- .../search/builder/SearchSourceBuilderTests.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index 5e98412fe80de..07abca3a89efd 100644 --- a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -22,25 +22,18 @@ import com.fasterxml.jackson.core.JsonParseException; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.ParsingException; -import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.unit.TimeValue; -import org.elasticsearch.common.xcontent.DeprecationHandler; -import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; -import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.ToXContent; -import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContent; -import org.elasticsearch.index.mapper.DocumentMapper; -import org.elasticsearch.index.mapper.MapperParsingException; import org.elasticsearch.index.query.BoolQueryBuilder; import org.elasticsearch.index.query.MatchNoneQueryBuilder; import org.elasticsearch.index.query.QueryBuilders; From c6a113b6c3a23d40dc02e773c00a30ed397bb6e1 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Tue, 10 Apr 2018 13:33:43 +0200 Subject: [PATCH 5/5] defaults to checkTrailingTokens for SearchSourceBuilder#fromXContent --- .../org/elasticsearch/client/RequestTests.java | 2 +- .../mustache/TransportSearchTemplateAction.java | 2 +- .../index/rankeval/RatedRequest.java | 2 +- .../index/rankeval/TransportRankEvalAction.java | 2 +- .../index/reindex/RestReindexAction.java | 2 +- .../rest/action/search/RestMultiSearchAction.java | 2 +- .../search/builder/SearchSourceBuilder.java | 15 ++++++++++----- .../action/search/MultiSearchRequestTests.java | 6 +++--- .../search/builder/SearchSourceBuilderTests.java | 4 ++-- 9 files changed, 21 insertions(+), 16 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java index e560540236c2f..4a63088c6d5ad 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java @@ -1155,7 +1155,7 @@ public void testMultiSearch() throws IOException { List requests = new ArrayList<>(); CheckedBiConsumer consumer = (searchRequest, p) -> { - SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(p); + SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(p, false); if (searchSourceBuilder.equals(new SearchSourceBuilder()) == false) { searchRequest.source(searchSourceBuilder); } diff --git a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java index 50f63841231f8..360e332f2c32d 100644 --- a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java +++ b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java @@ -112,7 +112,7 @@ static SearchRequest convert(SearchTemplateRequest searchTemplateRequest, Search try (XContentParser parser = XContentFactory.xContent(XContentType.JSON) .createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, source)) { SearchSourceBuilder builder = SearchSourceBuilder.searchSource(); - builder.parseXContent(parser); + builder.parseXContent(parser, true); builder.explain(searchTemplateRequest.isExplain()); builder.profile(searchTemplateRequest.isProfile()); searchRequest.source(builder); diff --git a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RatedRequest.java b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RatedRequest.java index 770c91e82a6a1..8f17c8203b7e8 100644 --- a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RatedRequest.java +++ b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RatedRequest.java @@ -223,7 +223,7 @@ public void addSummaryFields(List summaryFields) { return RatedDocument.fromXContent(p); }, RATINGS_FIELD); PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> - SearchSourceBuilder.fromXContent(p), REQUEST_FIELD); + SearchSourceBuilder.fromXContent(p, false), REQUEST_FIELD); PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> p.map(), PARAMS_FIELD); PARSER.declareStringArray(RatedRequest::addSummaryFields, FIELDS_FIELD); PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), TEMPLATE_ID_FIELD); diff --git a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/TransportRankEvalAction.java b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/TransportRankEvalAction.java index a076b93fbd30d..019ae274466ab 100644 --- a/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/TransportRankEvalAction.java +++ b/modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/TransportRankEvalAction.java @@ -107,7 +107,7 @@ protected void doExecute(RankEvalRequest request, ActionListener { - searchRequest.source(SearchSourceBuilder.fromXContent(parser)); + searchRequest.source(SearchSourceBuilder.fromXContent(parser, false)); multiRequest.add(searchRequest); }); List requests = multiRequest.requests(); diff --git a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java index b2d72c3fcb83b..b2d1062ba0056 100644 --- a/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java @@ -111,8 +111,12 @@ public final class SearchSourceBuilder implements Writeable, ToXContentObject, R public static final ParseField ALL_FIELDS_FIELDS = new ParseField("all_fields"); public static SearchSourceBuilder fromXContent(XContentParser parser) throws IOException { + return fromXContent(parser, true); + } + + public static SearchSourceBuilder fromXContent(XContentParser parser, boolean checkTrailingTokens) throws IOException { SearchSourceBuilder builder = new SearchSourceBuilder(); - builder.parseXContent(parser); + builder.parseXContent(parser, checkTrailingTokens); return builder; } @@ -952,15 +956,16 @@ private SearchSourceBuilder shallowCopy(QueryBuilder queryBuilder, QueryBuilder } public void parseXContent(XContentParser parser) throws IOException { - parseXContent(parser, false); + parseXContent(parser, true); } /** * Parse some xContent into this SearchSourceBuilder, overwriting any values specified in the xContent. Use this if you need to set up - * different defaults than a regular SearchSourceBuilder would have and use {@link #fromXContent(XContentParser)} if you have normal - * defaults. + * different defaults than a regular SearchSourceBuilder would have and use {@link #fromXContent(XContentParser, boolean)} if you have + * normal defaults. + * * @param parser The xContent parser. - * @param checkTrailingTokens If true throws a parsing exception when extra tokens are found the main object. + * @param checkTrailingTokens If true throws a parsing exception when extra tokens are found after the main object. */ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) throws IOException { XContentParser.Token token = parser.currentToken(); diff --git a/server/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java b/server/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java index d9fea03a56692..a772fa6951c2f 100644 --- a/server/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java +++ b/server/src/test/java/org/elasticsearch/action/search/MultiSearchRequestTests.java @@ -165,7 +165,7 @@ public void testResponseErrorToXContent() throws IOException { new MultiSearchResponse.Item(null, new IllegalStateException("baaaaaazzzz")) }, tookInMillis); - assertEquals("{\"took\":" + assertEquals("{\"took\":" + tookInMillis + ",\"responses\":[" + "{" @@ -225,7 +225,7 @@ public void testMultiLineSerialization() throws IOException { byte[] originalBytes = MultiSearchRequest.writeMultiLineFormat(originalRequest, xContentType.xContent()); MultiSearchRequest parsedRequest = new MultiSearchRequest(); CheckedBiConsumer consumer = (r, p) -> { - SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(p); + SearchSourceBuilder searchSourceBuilder = SearchSourceBuilder.fromXContent(p, false); if (searchSourceBuilder.equals(new SearchSourceBuilder()) == false) { r.source(searchSourceBuilder); } @@ -273,7 +273,7 @@ private static MultiSearchRequest createMultiSearchRequest() throws IOException if (randomBoolean()) { searchRequest.allowPartialSearchResults(true); } - + // scroll is not supported in the current msearch api, so unset it: searchRequest.scroll((Scroll) null); diff --git a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index 07abca3a89efd..cda66cbfc6765 100644 --- a/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -70,12 +70,12 @@ public void testFromXContent() throws IOException { public void testFromXContentInvalid() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, "{}}")) { - JsonParseException exc = expectThrows(JsonParseException.class, () -> new SearchSourceBuilder().parseXContent(parser, true)); + JsonParseException exc = expectThrows(JsonParseException.class, () -> SearchSourceBuilder.fromXContent(parser)); assertThat(exc.getMessage(), containsString("Unexpected close marker")); } try (XContentParser parser = createParser(JsonXContent.jsonXContent, "{}{}")) { - ParsingException exc = expectThrows(ParsingException.class, () -> new SearchSourceBuilder().parseXContent(parser, true)); + ParsingException exc = expectThrows(ParsingException.class, () -> SearchSourceBuilder.fromXContent(parser)); assertThat(exc.getDetailedMessage(), containsString("found after the main object")); } }