Skip to content

Commit 4ad47d2

Browse files
qwerty4030jaymode
qwerty4030
authored and
jaymode
committed
Fixed NPEs caused by requests without content. (#23497)
REST handlers that require a body will throw an an ElasticsearchParseException "request body required". REST handlers that require a body OR source param will throw an ElasticsearchParseException "request body or source param required". Replaced asserts in BulkRequest parsing code with a more descriptive IllegalArgumentException if the line contains an empty object. Updated bulk REST test to verify an empty action line is rejected properly. Updated BulkRequestTests with randomized testing for an empty action line. Used try-with-resouces for XContentParser in AbstractBulkByQueryRestHandler.
1 parent 20189c6 commit 4ad47d2

File tree

30 files changed

+313
-101
lines changed

30 files changed

+313
-101
lines changed

‎client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,8 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
7373
}
7474
bulkRequest.timeout(request.paramAsTime("timeout", BulkShardRequest.DEFAULT_TIMEOUT));
7575
bulkRequest.setRefreshPolicy(request.param("refresh"));
76-
bulkRequest.add(request.content(), defaultIndex, defaultType, defaultRouting, defaultFields, null, defaultPipeline, null, true,
77-
request.getXContentType());
76+
bulkRequest.add(request.requiredContent(), defaultIndex, defaultType, defaultRouting, defaultFields,
77+
null, defaultPipeline, null, true, request.getXContentType());
7878

7979
// short circuit the call to the transport layer
8080
return channel -> {

‎core/src/main/java/org/elasticsearch/action/bulk/BulkRequest.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -338,10 +338,16 @@ public BulkRequest add(BytesReference data, @Nullable String defaultIndex, @Null
338338
if (token == null) {
339339
continue;
340340
}
341-
assert token == XContentParser.Token.START_OBJECT;
341+
if (token != XContentParser.Token.START_OBJECT) {
342+
throw new IllegalArgumentException("Malformed action/metadata line [" + line + "], expected "
343+
+ XContentParser.Token.START_OBJECT + " but found [" + token + "]");
344+
}
342345
// Move to FIELD_NAME, that's the action
343346
token = parser.nextToken();
344-
assert token == XContentParser.Token.FIELD_NAME;
347+
if (token != XContentParser.Token.FIELD_NAME) {
348+
throw new IllegalArgumentException("Malformed action/metadata line [" + line + "], expected "
349+
+ XContentParser.Token.FIELD_NAME + " but found [" + token + "]");
350+
}
345351
String action = parser.currentName();
346352

347353
String index = defaultIndex;

‎core/src/main/java/org/elasticsearch/rest/RestRequest.java

Lines changed: 39 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,18 @@ public final String path() {
140140

141141
public abstract BytesReference content();
142142

143+
/**
144+
* @return content of the request body or throw an exception if the body or content type is missing
145+
*/
146+
public final BytesReference requiredContent() {
147+
if (hasContent() == false) {
148+
throw new ElasticsearchParseException("request body is required");
149+
} else if (xContentType.get() == null) {
150+
throw new IllegalStateException("unknown content type");
151+
}
152+
return content();
153+
}
154+
143155
/**
144156
* Get the value of the header or {@code null} if not found. This method only retrieves the first header value if multiple values are
145157
* sent. Use of {@link #getAllHeaderValues(String)} should be preferred
@@ -336,12 +348,7 @@ public NamedXContentRegistry getXContentRegistry() {
336348
* {@link #contentOrSourceParamParser()} for requests that support specifying the request body in the {@code source} param.
337349
*/
338350
public final XContentParser contentParser() throws IOException {
339-
BytesReference content = content();
340-
if (content.length() == 0) {
341-
throw new ElasticsearchParseException("Body required");
342-
} else if (xContentType.get() == null) {
343-
throw new IllegalStateException("unknown content type");
344-
}
351+
BytesReference content = requiredContent(); // will throw exception if body or content type missing
345352
return xContentType.get().xContent().createParser(xContentRegistry, content);
346353
}
347354

@@ -371,11 +378,7 @@ public final boolean hasContentOrSourceParam() {
371378
*/
372379
public final XContentParser contentOrSourceParamParser() throws IOException {
373380
Tuple<XContentType, BytesReference> tuple = contentOrSourceParam();
374-
BytesReference content = tuple.v2();
375-
if (content.length() == 0) {
376-
throw new ElasticsearchParseException("Body required");
377-
}
378-
return tuple.v1().xContent().createParser(xContentRegistry, content);
381+
return tuple.v1().xContent().createParser(xContentRegistry, tuple.v2());
379382
}
380383

381384
/**
@@ -384,10 +387,10 @@ public final XContentParser contentOrSourceParamParser() throws IOException {
384387
* back to the user when there isn't request content.
385388
*/
386389
public final void withContentOrSourceParamParserOrNull(CheckedConsumer<XContentParser, IOException> withParser) throws IOException {
387-
Tuple<XContentType, BytesReference> tuple = contentOrSourceParam();
388-
BytesReference content = tuple.v2();
389-
XContentType xContentType = tuple.v1();
390-
if (content.length() > 0) {
390+
if (hasContentOrSourceParam()) {
391+
Tuple<XContentType, BytesReference> tuple = contentOrSourceParam();
392+
BytesReference content = tuple.v2();
393+
XContentType xContentType = tuple.v1();
391394
try (XContentParser parser = xContentType.xContent().createParser(xContentRegistry, content)) {
392395
withParser.accept(parser);
393396
}
@@ -397,36 +400,31 @@ public final void withContentOrSourceParamParserOrNull(CheckedConsumer<XContentP
397400
}
398401

399402
/**
400-
* Get the content of the request or the contents of the {@code source} param. Prefer {@link #contentOrSourceParamParser()} or
401-
* {@link #withContentOrSourceParamParserOrNull(CheckedConsumer)} if you need a parser.
403+
* Get the content of the request or the contents of the {@code source} param or throw an exception if both are missing.
404+
* Prefer {@link #contentOrSourceParamParser()} or {@link #withContentOrSourceParamParserOrNull(CheckedConsumer)} if you need a parser.
402405
*/
403406
public final Tuple<XContentType, BytesReference> contentOrSourceParam() {
404-
if (hasContent()) {
405-
if (xContentType.get() == null) {
406-
throw new IllegalStateException("unknown content type");
407-
}
408-
return new Tuple<>(xContentType.get(), content());
407+
if (hasContentOrSourceParam() == false) {
408+
throw new ElasticsearchParseException("request body or source parameter is required");
409+
} else if (hasContent()) {
410+
return new Tuple<>(xContentType.get(), requiredContent());
409411
}
410-
411412
String source = param("source");
412413
String typeParam = param("source_content_type");
413-
if (source != null) {
414-
BytesArray bytes = new BytesArray(source);
415-
final XContentType xContentType;
416-
if (typeParam != null) {
417-
xContentType = parseContentType(Collections.singletonList(typeParam));
418-
} else {
419-
DEPRECATION_LOGGER.deprecated("Deprecated use of the [source] parameter without the [source_content_type] parameter. Use " +
420-
"the [source_content_type] parameter to specify the content type of the source such as [application/json]");
421-
xContentType = XContentFactory.xContentType(bytes);
422-
}
414+
BytesArray bytes = new BytesArray(source);
415+
final XContentType xContentType;
416+
if (typeParam != null) {
417+
xContentType = parseContentType(Collections.singletonList(typeParam));
418+
} else {
419+
DEPRECATION_LOGGER.deprecated("Deprecated use of the [source] parameter without the [source_content_type] parameter. Use " +
420+
"the [source_content_type] parameter to specify the content type of the source such as [application/json]");
421+
xContentType = XContentFactory.xContentType(bytes);
422+
}
423423

424-
if (xContentType == null) {
425-
throw new IllegalStateException("could not determine source content type");
426-
}
427-
return new Tuple<>(xContentType, bytes);
424+
if (xContentType == null) {
425+
throw new IllegalStateException("could not determine source content type");
428426
}
429-
return new Tuple<>(XContentType.JSON, BytesArray.EMPTY);
427+
return new Tuple<>(xContentType, bytes);
430428
}
431429

432430
/**
@@ -437,7 +435,9 @@ public final Tuple<XContentType, BytesReference> contentOrSourceParam() {
437435
@Deprecated
438436
public final void withContentOrSourceParamParserOrNullLenient(CheckedConsumer<XContentParser, IOException> withParser)
439437
throws IOException {
440-
if (hasContent() && xContentType.get() == null) {
438+
if (hasContentOrSourceParam() == false) {
439+
withParser.accept(null);
440+
} else if (hasContent() && xContentType.get() == null) {
441441
withParser.accept(null);
442442
} else {
443443
Tuple<XContentType, BytesReference> tuple = contentOrSourceParam();

‎core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestPutStoredScriptAction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client
5858
lang = null;
5959
}
6060

61-
BytesReference content = request.content();
61+
BytesReference content = request.requiredContent();
6262

6363
if (lang != null) {
6464
deprecationLogger.deprecated(

‎core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestPutIndexTemplateAction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
4444
putRequest.masterNodeTimeout(request.paramAsTime("master_timeout", putRequest.masterNodeTimeout()));
4545
putRequest.create(request.paramAsBoolean("create", false));
4646
putRequest.cause(request.param("cause", ""));
47-
putRequest.source(request.content(), request.getXContentType());
47+
putRequest.source(request.requiredContent(), request.getXContentType());
4848
return channel -> client.admin().indices().putTemplate(putRequest, new AcknowledgedRestListener<>(channel));
4949
}
5050

‎core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestPutMappingAction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ public RestPutMappingAction(Settings settings, RestController controller) {
6767
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
6868
PutMappingRequest putMappingRequest = putMappingRequest(Strings.splitStringByCommaToArray(request.param("index")));
6969
putMappingRequest.type(request.param("type"));
70-
putMappingRequest.source(request.content(), request.getXContentType());
70+
putMappingRequest.source(request.requiredContent(), request.getXContentType());
7171
putMappingRequest.updateAllTypes(request.paramAsBoolean("update_all_types", false));
7272
putMappingRequest.timeout(request.paramAsTime("timeout", putMappingRequest.timeout()));
7373
putMappingRequest.masterNodeTimeout(request.paramAsTime("master_timeout", putMappingRequest.masterNodeTimeout()));

‎core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestUpdateSettingsAction.java

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -57,18 +57,16 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
5757
updateSettingsRequest.indicesOptions(IndicesOptions.fromRequest(request, updateSettingsRequest.indicesOptions()));
5858

5959
Map<String, Object> settings = new HashMap<>();
60-
if (request.hasContent()) {
61-
try (XContentParser parser = request.contentParser()) {
62-
Map<String, Object> bodySettings = parser.map();
63-
Object innerBodySettings = bodySettings.get("settings");
64-
// clean up in case the body is wrapped with "settings" : { ... }
65-
if (innerBodySettings instanceof Map) {
66-
@SuppressWarnings("unchecked")
67-
Map<String, Object> innerBodySettingsMap = (Map<String, Object>) innerBodySettings;
68-
settings.putAll(innerBodySettingsMap);
69-
} else {
70-
settings.putAll(bodySettings);
71-
}
60+
try (XContentParser parser = request.contentParser()) {
61+
Map<String, Object> bodySettings = parser.map();
62+
Object innerBodySettings = bodySettings.get("settings");
63+
// clean up in case the body is wrapped with "settings" : { ... }
64+
if (innerBodySettings instanceof Map) {
65+
@SuppressWarnings("unchecked")
66+
Map<String, Object> innerBodySettingsMap = (Map<String, Object>) innerBodySettings;
67+
settings.putAll(innerBodySettingsMap);
68+
} else {
69+
settings.putAll(bodySettings);
7270
}
7371
}
7472
updateSettingsRequest.settings(settings);

‎core/src/main/java/org/elasticsearch/rest/action/document/RestBulkAction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
8686
}
8787
bulkRequest.timeout(request.paramAsTime("timeout", BulkShardRequest.DEFAULT_TIMEOUT));
8888
bulkRequest.setRefreshPolicy(request.param("refresh"));
89-
bulkRequest.add(request.content(), defaultIndex, defaultType, defaultRouting, defaultFields,
89+
bulkRequest.add(request.requiredContent(), defaultIndex, defaultType, defaultRouting, defaultFields,
9090
defaultFetchSourceContext, defaultPipeline, null, allowExplicitIndex, request.getXContentType());
9191

9292
return channel -> client.bulk(bulkRequest, new RestStatusToXContentListener<>(channel));

‎core/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
7272
indexRequest.ttl(request.param("ttl"));
7373
}
7474
indexRequest.setPipeline(request.param("pipeline"));
75-
indexRequest.source(request.content(), request.getXContentType());
75+
indexRequest.source(request.requiredContent(), request.getXContentType());
7676
indexRequest.timeout(request.paramAsTime("timeout", IndexRequest.DEFAULT_TIMEOUT));
7777
indexRequest.setRefreshPolicy(request.param("refresh"));
7878
indexRequest.version(RestActions.parseVersion(request));

‎core/src/test/java/org/elasticsearch/action/bulk/BulkRequestTests.java

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,31 @@ public void testSimpleBulk10() throws Exception {
176176
assertThat(bulkRequest.numberOfActions(), equalTo(9));
177177
}
178178

179+
public void testBulkEmptyObject() throws Exception {
180+
String bulkIndexAction = "{ \"index\":{\"_index\":\"test\",\"_type\":\"type1\",\"_id\":\"1\"} }\r\n";
181+
String bulkIndexSource = "{ \"field1\" : \"value1\" }\r\n";
182+
String emptyObject = "{}\r\n";
183+
StringBuilder bulk = new StringBuilder();
184+
int emptyLine;
185+
if (randomBoolean()) {
186+
bulk.append(emptyObject);
187+
emptyLine = 1;
188+
} else {
189+
int actions = randomIntBetween(1, 10);
190+
int emptyAction = randomIntBetween(1, actions);
191+
emptyLine = emptyAction * 2 - 1;
192+
for (int i = 1; i <= actions; i++) {
193+
bulk.append(i == emptyAction ? emptyObject : bulkIndexAction);
194+
bulk.append(randomBoolean() ? emptyObject : bulkIndexSource);
195+
}
196+
}
197+
String bulkAction = bulk.toString();
198+
BulkRequest bulkRequest = new BulkRequest();
199+
IllegalArgumentException exc = expectThrows(IllegalArgumentException.class,
200+
() -> bulkRequest.add(bulkAction.getBytes(StandardCharsets.UTF_8), 0, bulkAction.length(), null, null, XContentType.JSON));
201+
assertThat(exc.getMessage(), containsString("Malformed action/metadata line [" + emptyLine + "], expected FIELD_NAME but found [END_OBJECT]"));
202+
}
203+
179204
// issue 7361
180205
public void testBulkRequestWithRefresh() throws Exception {
181206
BulkRequest bulkRequest = new BulkRequest();

‎core/src/test/java/org/elasticsearch/rest/RestRequestTests.java

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import org.elasticsearch.common.Strings;
2424
import org.elasticsearch.common.bytes.BytesArray;
2525
import org.elasticsearch.common.bytes.BytesReference;
26+
import org.elasticsearch.common.collect.MapBuilder;
2627
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
2728
import org.elasticsearch.common.xcontent.XContentType;
2829
import org.elasticsearch.test.ESTestCase;
@@ -42,11 +43,14 @@ public class RestRequestTests extends ESTestCase {
4243
public void testContentParser() throws IOException {
4344
Exception e = expectThrows(ElasticsearchParseException.class, () ->
4445
new ContentRestRequest("", emptyMap()).contentParser());
45-
assertEquals("Body required", e.getMessage());
46+
assertEquals("request body is required", e.getMessage());
4647
e = expectThrows(ElasticsearchParseException.class, () ->
4748
new ContentRestRequest("", singletonMap("source", "{}")).contentParser());
48-
assertEquals("Body required", e.getMessage());
49+
assertEquals("request body is required", e.getMessage());
4950
assertEquals(emptyMap(), new ContentRestRequest("{}", emptyMap()).contentParser().map());
51+
e = expectThrows(ElasticsearchParseException.class, () ->
52+
new ContentRestRequest("", emptyMap(), emptyMap()).contentParser());
53+
assertEquals("request body is required", e.getMessage());
5054
}
5155

5256
public void testApplyContentParser() throws IOException {
@@ -58,7 +62,9 @@ public void testApplyContentParser() throws IOException {
5862
}
5963

6064
public void testContentOrSourceParam() throws IOException {
61-
assertEquals(BytesArray.EMPTY, new ContentRestRequest("", emptyMap()).contentOrSourceParam().v2());
65+
Exception e = expectThrows(ElasticsearchParseException.class, () ->
66+
new ContentRestRequest("", emptyMap()).contentOrSourceParam());
67+
assertEquals("request body or source parameter is required", e.getMessage());
6268
assertEquals(new BytesArray("stuff"), new ContentRestRequest("stuff", emptyMap()).contentOrSourceParam().v2());
6369
assertEquals(new BytesArray("stuff"),
6470
new ContentRestRequest("stuff", singletonMap("source", "stuff2")).contentOrSourceParam().v2());
@@ -78,7 +84,7 @@ public void testHasContentOrSourceParam() throws IOException {
7884
public void testContentOrSourceParamParser() throws IOException {
7985
Exception e = expectThrows(ElasticsearchParseException.class, () ->
8086
new ContentRestRequest("", emptyMap()).contentOrSourceParamParser());
81-
assertEquals("Body required", e.getMessage());
87+
assertEquals("request body or source parameter is required", e.getMessage());
8288
assertEquals(emptyMap(), new ContentRestRequest("{}", emptyMap()).contentOrSourceParamParser().map());
8389
assertEquals(emptyMap(), new ContentRestRequest("{}", singletonMap("source", "stuff2")).contentOrSourceParamParser().map());
8490
assertEquals(emptyMap(), new ContentRestRequest("", singletonMap("source", "{}")).contentOrSourceParamParser().map());
@@ -137,6 +143,24 @@ public void testMultipleContentTypeHeaders() {
137143
assertEquals("only one Content-Type header should be provided", e.getMessage());
138144
}
139145

146+
public void testRequiredContent() {
147+
Exception e = expectThrows(ElasticsearchParseException.class, () ->
148+
new ContentRestRequest("", emptyMap()).requiredContent());
149+
assertEquals("request body is required", e.getMessage());
150+
assertEquals(new BytesArray("stuff"), new ContentRestRequest("stuff", emptyMap()).requiredContent());
151+
assertEquals(new BytesArray("stuff"),
152+
new ContentRestRequest("stuff", MapBuilder.<String, String>newMapBuilder()
153+
.put("source", "stuff2").put("source_content_type", "application/json").immutableMap()).requiredContent());
154+
e = expectThrows(ElasticsearchParseException.class, () ->
155+
new ContentRestRequest("", MapBuilder.<String, String>newMapBuilder()
156+
.put("source", "{\"foo\": \"stuff\"}").put("source_content_type", "application/json").immutableMap())
157+
.requiredContent());
158+
assertEquals("request body is required", e.getMessage());
159+
e = expectThrows(IllegalStateException.class, () ->
160+
new ContentRestRequest("test", null, Collections.emptyMap()).requiredContent());
161+
assertEquals("unknown content type", e.getMessage());
162+
}
163+
140164
private static final class ContentRestRequest extends RestRequest {
141165
private final BytesArray content;
142166

‎modules/ingest-common/src/test/resources/rest-api-spec/test/ingest/20_crud.yaml

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,3 +203,16 @@ teardown:
203203
catch: missing
204204
ingest.get_pipeline:
205205
id: "my_pipeline"
206+
207+
---
208+
"missing body":
209+
210+
- skip:
211+
version: " - 5.4.99"
212+
reason: NPE caused by missing body fixed in 5.5.0
213+
214+
- do:
215+
catch: /request body or source parameter is required/
216+
raw:
217+
method: PUT
218+
path: _ingest/pipeline/my_pipeline

‎modules/ingest-common/src/test/resources/rest-api-spec/test/ingest/90_simulate.yaml

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -605,3 +605,16 @@ teardown:
605605
- length: { docs.0.processor_results.1: 2 }
606606
- match: { docs.0.processor_results.1.tag: "rename-1" }
607607
- match: { docs.0.processor_results.1.doc._source.new_status: 200 }
608+
609+
---
610+
"missing body":
611+
612+
- skip:
613+
version: " - 5.4.99"
614+
reason: NPE caused by missing body fixed in 5.5.0
615+
616+
- do:
617+
catch: /request body or source parameter is required/
618+
raw:
619+
method: POST
620+
path: _ingest/pipeline/my_pipeline/_simulate

0 commit comments

Comments
 (0)