Skip to content

Commit 4c2e350

Browse files
jasontedorjasontedor
committed
Reject all requests that have an unconsumed body (elastic#37504)
This commit removes some leniency from REST handling where we move to reject all requests that have a body where the body is not used during the course of handling the request. For example, DELETE /index { "query" : { "term" : { "field" : "value" } } } is now rejected.
1 parent 282ce36 commit 4c2e350

File tree

11 files changed

+191
-19
lines changed

11 files changed

+191
-19
lines changed

‎modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpRequest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ public boolean hasContent() {
145145
}
146146

147147
@Override
148-
public BytesReference content() {
148+
public BytesReference innerContent() {
149149
return content;
150150
}
151151

‎server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,10 @@ public final void handleRequest(RestRequest request, RestChannel channel, NodeCl
9999
throw new IllegalArgumentException(unrecognized(request, unconsumedParams, candidateParams, "parameter"));
100100
}
101101

102+
if (request.hasContent() && request.isContentConsumed() == false) {
103+
throw new IllegalArgumentException("request [" + request.method() + " " + request.path() + "] does not support having a body");
104+
}
105+
102106
usageCount.increment();
103107
// execute the action
104108
action.accept(channel);

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,12 @@ public abstract class RestRequest implements ToXContent.Params {
6363
private final Set<String> consumedParams = new HashSet<>();
6464
private final SetOnce<XContentType> xContentType = new SetOnce<>();
6565

66+
private boolean contentConsumed = false;
67+
68+
public boolean isContentConsumed() {
69+
return contentConsumed;
70+
}
71+
6672
/**
6773
* Creates a new REST request.
6874
*
@@ -156,7 +162,12 @@ public final String path() {
156162

157163
public abstract boolean hasContent();
158164

159-
public abstract BytesReference content();
165+
public final BytesReference content() {
166+
contentConsumed = true;
167+
return innerContent();
168+
}
169+
170+
protected abstract BytesReference innerContent();
160171

161172
/**
162173
* @return content of the request body or throw an exception if the body or content type is missing

‎server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestForceMergeAction.java‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@
3434
import static org.elasticsearch.rest.RestRequest.Method.POST;
3535

3636
public class RestForceMergeAction extends BaseRestHandler {
37-
public RestForceMergeAction(Settings settings, RestController controller) {
37+
38+
public RestForceMergeAction(final Settings settings, final RestController controller) {
3839
super(settings);
3940
controller.registerHandler(POST, "/_forcemerge", this);
4041
controller.registerHandler(POST, "/{index}/_forcemerge", this);
@@ -47,14 +48,12 @@ public String getName() {
4748

4849
@Override
4950
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
50-
if (request.hasContent()) {
51-
throw new IllegalArgumentException("forcemerge takes arguments in query parameters, not in the request body");
52-
}
53-
ForceMergeRequest mergeRequest = new ForceMergeRequest(Strings.splitStringByCommaToArray(request.param("index")));
51+
final ForceMergeRequest mergeRequest = new ForceMergeRequest(Strings.splitStringByCommaToArray(request.param("index")));
5452
mergeRequest.indicesOptions(IndicesOptions.fromRequest(request, mergeRequest.indicesOptions()));
5553
mergeRequest.maxNumSegments(request.paramAsInt("max_num_segments", mergeRequest.maxNumSegments()));
5654
mergeRequest.onlyExpungeDeletes(request.paramAsBoolean("only_expunge_deletes", mergeRequest.onlyExpungeDeletes()));
5755
mergeRequest.flush(request.paramAsBoolean("flush", mergeRequest.flush()));
5856
return channel -> client.admin().indices().forceMerge(mergeRequest, new RestToXContentListener<>(channel));
5957
}
58+
6059
}

‎server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.elasticsearch.rest.RestController;
2929
import org.elasticsearch.rest.action.admin.indices.RestForceMergeAction;
3030
import org.elasticsearch.test.ESTestCase;
31+
import org.elasticsearch.test.rest.FakeRestChannel;
3132
import org.elasticsearch.test.rest.FakeRestRequest;
3233

3334
import static org.hamcrest.Matchers.equalTo;
@@ -39,9 +40,12 @@ public void testBodyRejection() throws Exception {
3940
final RestForceMergeAction handler = new RestForceMergeAction(Settings.EMPTY, mock(RestController.class));
4041
String json = JsonXContent.contentBuilder().startObject().field("max_num_segments", 1).endObject().toString();
4142
final FakeRestRequest request = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
42-
.withContent(new BytesArray(json), XContentType.JSON).build();
43+
.withContent(new BytesArray(json), XContentType.JSON)
44+
.withPath("/_forcemerge")
45+
.build();
4346
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
44-
() -> handler.prepareRequest(request, mock(NodeClient.class)));
45-
assertThat(e.getMessage(), equalTo("forcemerge takes arguments in query parameters, not in the request body"));
47+
() -> handler.handleRequest(request, new FakeRestChannel(request, randomBoolean(), 1), mock(NodeClient.class)));
48+
assertThat(e.getMessage(), equalTo("request [GET /_forcemerge] does not support having a body"));
4649
}
50+
4751
}

‎server/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,11 @@
2121

2222
import org.elasticsearch.client.node.NodeClient;
2323
import org.elasticsearch.common.Table;
24+
import org.elasticsearch.common.bytes.BytesArray;
2425
import org.elasticsearch.common.settings.Settings;
26+
import org.elasticsearch.common.xcontent.XContentBuilder;
27+
import org.elasticsearch.common.xcontent.XContentType;
28+
import org.elasticsearch.common.xcontent.json.JsonXContent;
2529
import org.elasticsearch.rest.action.cat.AbstractCatAction;
2630
import org.elasticsearch.test.ESTestCase;
2731
import org.elasticsearch.test.rest.FakeRestChannel;
@@ -232,4 +236,78 @@ public String getName() {
232236
assertTrue(executed.get());
233237
}
234238

239+
public void testConsumedBody() throws Exception {
240+
final AtomicBoolean executed = new AtomicBoolean();
241+
final BaseRestHandler handler = new BaseRestHandler(Settings.EMPTY) {
242+
@Override
243+
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
244+
request.content();
245+
return channel -> executed.set(true);
246+
}
247+
248+
@Override
249+
public String getName() {
250+
return "test_consumed_body";
251+
}
252+
253+
};
254+
255+
try (XContentBuilder builder = JsonXContent.contentBuilder().startObject().endObject()) {
256+
final RestRequest request = new FakeRestRequest.Builder(xContentRegistry())
257+
.withContent(new BytesArray(builder.toString()), XContentType.JSON)
258+
.build();
259+
final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
260+
handler.handleRequest(request, channel, mock(NodeClient.class));
261+
assertTrue(executed.get());
262+
}
263+
}
264+
265+
public void testUnconsumedNoBody() throws Exception {
266+
final AtomicBoolean executed = new AtomicBoolean();
267+
final BaseRestHandler handler = new BaseRestHandler(Settings.EMPTY) {
268+
@Override
269+
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
270+
return channel -> executed.set(true);
271+
}
272+
273+
@Override
274+
public String getName() {
275+
return "test_unconsumed_body";
276+
}
277+
278+
};
279+
280+
final RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).build();
281+
final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
282+
handler.handleRequest(request, channel, mock(NodeClient.class));
283+
assertTrue(executed.get());
284+
}
285+
286+
public void testUnconsumedBody() throws IOException {
287+
final AtomicBoolean executed = new AtomicBoolean();
288+
final BaseRestHandler handler = new BaseRestHandler(Settings.EMPTY) {
289+
@Override
290+
protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
291+
return channel -> executed.set(true);
292+
}
293+
294+
@Override
295+
public String getName() {
296+
return "test_unconsumed_body";
297+
}
298+
299+
};
300+
301+
try (XContentBuilder builder = JsonXContent.contentBuilder().startObject().endObject()) {
302+
final RestRequest request = new FakeRestRequest.Builder(xContentRegistry())
303+
.withContent(new BytesArray(builder.toString()), XContentType.JSON)
304+
.build();
305+
final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
306+
final IllegalArgumentException e =
307+
expectThrows(IllegalArgumentException.class, () -> handler.handleRequest(request, channel, mock(NodeClient.class)));
308+
assertThat(e, hasToString(containsString("request [GET /] does not support having a body")));
309+
assertFalse(executed.get());
310+
}
311+
}
312+
235313
}

‎server/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ public boolean hasContent() {
187187
}
188188

189189
@Override
190-
public BytesReference content() {
190+
public BytesReference innerContent() {
191191
return null;
192192
}
193193
};

‎server/src/test/java/org/elasticsearch/rest/RestControllerTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -569,7 +569,7 @@ public boolean hasContent() {
569569
}
570570

571571
@Override
572-
public BytesReference content() {
572+
public BytesReference innerContent() {
573573
return content;
574574
}
575575

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

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,13 @@
2020
package org.elasticsearch.rest;
2121

2222
import org.elasticsearch.ElasticsearchParseException;
23+
import org.elasticsearch.common.CheckedConsumer;
2324
import org.elasticsearch.common.Strings;
2425
import org.elasticsearch.common.bytes.BytesArray;
2526
import org.elasticsearch.common.bytes.BytesReference;
2627
import org.elasticsearch.common.collect.MapBuilder;
2728
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
29+
import org.elasticsearch.common.xcontent.XContentParser;
2830
import org.elasticsearch.common.xcontent.XContentType;
2931
import org.elasticsearch.test.ESTestCase;
3032

@@ -40,8 +42,79 @@
4042
import static java.util.Collections.singletonMap;
4143
import static org.hamcrest.Matchers.equalTo;
4244
import static org.hamcrest.Matchers.instanceOf;
45+
import static org.mockito.Mockito.mock;
4346

4447
public class RestRequestTests extends ESTestCase {
48+
49+
public void testContentConsumesContent() {
50+
runConsumesContentTest(RestRequest::content, true);
51+
}
52+
53+
public void testRequiredContentConsumesContent() {
54+
runConsumesContentTest(RestRequest::requiredContent, true);
55+
}
56+
57+
public void testContentParserConsumesContent() {
58+
runConsumesContentTest(RestRequest::contentParser, true);
59+
}
60+
61+
public void testContentOrSourceParamConsumesContent() {
62+
runConsumesContentTest(RestRequest::contentOrSourceParam, true);
63+
}
64+
65+
public void testContentOrSourceParamsParserConsumesContent() {
66+
runConsumesContentTest(RestRequest::contentOrSourceParamParser, true);
67+
}
68+
69+
public void testWithContentOrSourceParamParserOrNullConsumesContent() {
70+
@SuppressWarnings("unchecked") CheckedConsumer<XContentParser, IOException> consumer = mock(CheckedConsumer.class);
71+
runConsumesContentTest(request -> request.withContentOrSourceParamParserOrNull(consumer), true);
72+
}
73+
74+
public void testApplyContentParserConsumesContent() {
75+
@SuppressWarnings("unchecked") CheckedConsumer<XContentParser, IOException> consumer = mock(CheckedConsumer.class);
76+
runConsumesContentTest(request -> request.applyContentParser(consumer), true);
77+
}
78+
79+
public void testHasContentDoesNotConsumesContent() {
80+
runConsumesContentTest(RestRequest::hasContent, false);
81+
}
82+
83+
private <T extends Exception> void runConsumesContentTest(
84+
final CheckedConsumer<RestRequest, T> consumer, final boolean expected) {
85+
final RestRequest request = new RestRequest(mock(NamedXContentRegistry.class), "", Collections.emptyMap()) {
86+
@Override
87+
public Method method() {
88+
return Method.GET;
89+
}
90+
91+
@Override
92+
public String uri() {
93+
return "";
94+
}
95+
96+
@Override
97+
public boolean hasContent() {
98+
return true;
99+
}
100+
101+
private final BytesReference content = new BytesArray(new byte[1]);
102+
103+
@Override
104+
public BytesReference innerContent() {
105+
return content;
106+
}
107+
};
108+
request.setXContentType(XContentType.JSON);
109+
assertFalse(request.isContentConsumed());
110+
try {
111+
consumer.accept(request);
112+
} catch (final Exception e) {
113+
throw new RuntimeException(e);
114+
}
115+
assertThat(request.isContentConsumed(), equalTo(expected));
116+
}
117+
45118
public void testContentParser() throws IOException {
46119
Exception e = expectThrows(ElasticsearchParseException.class, () ->
47120
new ContentRestRequest("", emptyMap()).contentParser());
@@ -194,7 +267,7 @@ public boolean hasContent() {
194267
}
195268

196269
@Override
197-
public BytesReference content() {
270+
public BytesReference innerContent() {
198271
return content;
199272
}
200273

@@ -208,4 +281,5 @@ public Method method() {
208281
throw new UnsupportedOperationException("Not used by this test");
209282
}
210283
}
284+
211285
}

‎test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
package org.elasticsearch.test.rest;
2121

22+
import org.elasticsearch.common.bytes.BytesArray;
2223
import org.elasticsearch.common.bytes.BytesReference;
2324
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
2425
import org.elasticsearch.common.xcontent.XContentType;
@@ -29,6 +30,7 @@
2930
import java.util.HashMap;
3031
import java.util.List;
3132
import java.util.Map;
33+
import java.util.Objects;
3234

3335
public class FakeRestRequest extends RestRequest {
3436

@@ -43,7 +45,7 @@ public FakeRestRequest() {
4345
private FakeRestRequest(NamedXContentRegistry xContentRegistry, Map<String, List<String>> headers,
4446
Map<String, String> params, BytesReference content, Method method, String path, SocketAddress remoteAddress) {
4547
super(xContentRegistry, params, path, headers);
46-
this.content = content;
48+
this.content = Objects.requireNonNull(content);
4749
this.method = method;
4850
this.remoteAddress = remoteAddress;
4951
}
@@ -60,11 +62,11 @@ public String uri() {
6062

6163
@Override
6264
public boolean hasContent() {
63-
return content != null;
65+
return content.length() > 0;
6466
}
6567

6668
@Override
67-
public BytesReference content() {
69+
public BytesReference innerContent() {
6870
return content;
6971
}
7072

@@ -80,7 +82,7 @@ public static class Builder {
8082

8183
private Map<String, String> params = new HashMap<>();
8284

83-
private BytesReference content;
85+
private BytesReference content = BytesArray.EMPTY;
8486

8587
private String path = "/";
8688

@@ -103,7 +105,7 @@ public Builder withParams(Map<String, String> params) {
103105
}
104106

105107
public Builder withContent(BytesReference content, XContentType xContentType) {
106-
this.content = content;
108+
this.content = Objects.requireNonNull(content);
107109
if (xContentType != null) {
108110
headers.put("Content-Type", Collections.singletonList(xContentType.mediaType()));
109111
}

0 commit comments

Comments
 (0)