Reject all requests that have an unconsumed body (#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.

Author: jasontedor

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

@@ -99,6 +99,10 @@ public final void handleRequest(RestRequest request, RestChannel channel, NodeCl
             throw new IllegalArgumentException(unrecognized(request, unconsumedParams, candidateParams, "parameter"));
         }
 
+        if (request.hasContent() && request.isContentConsumed() == false) {
+            throw new IllegalArgumentException("request [" + request.method() + " " + request.path() + "] does not support having a body");
+        }
+
         usageCount.increment();
         // execute the action
         action.accept(channel);

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

@@ -66,6 +66,12 @@ public class RestRequest implements ToXContent.Params {
     private final HttpRequest httpRequest;
     private final HttpChannel httpChannel;
 
+    private boolean contentConsumed = false;
+
+    public boolean isContentConsumed() {
+        return contentConsumed;
+    }
+
     protected RestRequest(NamedXContentRegistry xContentRegistry, Map<String, String> params, String path,
                           Map<String, List<String>> headers, HttpRequest httpRequest, HttpChannel httpChannel) {
         final XContentType xContentType;
@@ -173,10 +179,15 @@ public final String path() {
     }
 
     public boolean hasContent() {
-        return content().length() > 0;
+        return content(false).length() > 0;
     }
 
     public BytesReference content() {
+        return content(true);
+    }
+
+    protected BytesReference content(final boolean contentConsumed) {
+        this.contentConsumed = this.contentConsumed | contentConsumed;
         return httpRequest.content();
     }
 

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

@@ -34,7 +34,8 @@
 import static org.elasticsearch.rest.RestRequest.Method.POST;
 
 public class RestForceMergeAction extends BaseRestHandler {
-    public RestForceMergeAction(Settings settings, RestController controller) {
+
+    public RestForceMergeAction(final Settings settings, final RestController controller) {
         super(settings);
         controller.registerHandler(POST, "/_forcemerge", this);
         controller.registerHandler(POST, "/{index}/_forcemerge", this);
@@ -47,14 +48,12 @@ public String getName() {
 
     @Override
     public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
-        if (request.hasContent()) {
-            throw new IllegalArgumentException("forcemerge takes arguments in query parameters, not in the request body");
-        }
-        ForceMergeRequest mergeRequest = new ForceMergeRequest(Strings.splitStringByCommaToArray(request.param("index")));
+        final ForceMergeRequest mergeRequest = new ForceMergeRequest(Strings.splitStringByCommaToArray(request.param("index")));
         mergeRequest.indicesOptions(IndicesOptions.fromRequest(request, mergeRequest.indicesOptions()));
         mergeRequest.maxNumSegments(request.paramAsInt("max_num_segments", mergeRequest.maxNumSegments()));
         mergeRequest.onlyExpungeDeletes(request.paramAsBoolean("only_expunge_deletes", mergeRequest.onlyExpungeDeletes()));
         mergeRequest.flush(request.paramAsBoolean("flush", mergeRequest.flush()));
         return channel -> client.admin().indices().forceMerge(mergeRequest, new RestToXContentListener<>(channel));
     }
+
 }

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

@@ -28,6 +28,7 @@
 import org.elasticsearch.rest.RestController;
 import org.elasticsearch.rest.action.admin.indices.RestForceMergeAction;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.rest.FakeRestChannel;
 import org.elasticsearch.test.rest.FakeRestRequest;
 
 import static org.hamcrest.Matchers.equalTo;
@@ -39,9 +40,12 @@ public void testBodyRejection() throws Exception {
         final RestForceMergeAction handler = new RestForceMergeAction(Settings.EMPTY, mock(RestController.class));
         String json = JsonXContent.contentBuilder().startObject().field("max_num_segments", 1).endObject().toString();
         final FakeRestRequest request = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
-            .withContent(new BytesArray(json), XContentType.JSON).build();
+                .withContent(new BytesArray(json), XContentType.JSON)
+                .withPath("/_forcemerge")
+                .build();
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-            () -> handler.prepareRequest(request, mock(NodeClient.class)));
-        assertThat(e.getMessage(), equalTo("forcemerge takes arguments in query parameters, not in the request body"));
+            () -> handler.handleRequest(request, new FakeRestChannel(request, randomBoolean(), 1), mock(NodeClient.class)));
+        assertThat(e.getMessage(), equalTo("request [GET /_forcemerge] does not support having a body"));
     }
+
 }

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

@@ -21,7 +21,11 @@
 
 import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.common.Table;
+import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentType;
+import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.rest.action.cat.AbstractCatAction;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.rest.FakeRestChannel;
@@ -232,4 +236,78 @@ public String getName() {
         assertTrue(executed.get());
     }
 
+    public void testConsumedBody() throws Exception {
+        final AtomicBoolean executed = new AtomicBoolean();
+        final BaseRestHandler handler = new BaseRestHandler(Settings.EMPTY) {
+            @Override
+            protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
+                request.content();
+                return channel -> executed.set(true);
+            }
+
+            @Override
+            public String getName() {
+                return "test_consumed_body";
+            }
+
+        };
+
+        try (XContentBuilder builder = JsonXContent.contentBuilder().startObject().endObject()) {
+            final RestRequest request = new FakeRestRequest.Builder(xContentRegistry())
+                    .withContent(new BytesArray(builder.toString()), XContentType.JSON)
+                    .build();
+            final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
+            handler.handleRequest(request, channel, mock(NodeClient.class));
+            assertTrue(executed.get());
+        }
+    }
+
+    public void testUnconsumedNoBody() throws Exception {
+        final AtomicBoolean executed = new AtomicBoolean();
+        final BaseRestHandler handler = new BaseRestHandler(Settings.EMPTY) {
+            @Override
+            protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
+                return channel -> executed.set(true);
+            }
+
+            @Override
+            public String getName() {
+                return "test_unconsumed_body";
+            }
+
+        };
+
+        final RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).build();
+        final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
+        handler.handleRequest(request, channel, mock(NodeClient.class));
+        assertTrue(executed.get());
+    }
+
+    public void testUnconsumedBody() throws IOException {
+        final AtomicBoolean executed = new AtomicBoolean();
+        final BaseRestHandler handler = new BaseRestHandler(Settings.EMPTY) {
+            @Override
+            protected RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
+                return channel -> executed.set(true);
+            }
+
+            @Override
+            public String getName() {
+                return "test_unconsumed_body";
+            }
+
+        };
+
+        try (XContentBuilder builder = JsonXContent.contentBuilder().startObject().endObject()) {
+            final RestRequest request = new FakeRestRequest.Builder(xContentRegistry())
+                    .withContent(new BytesArray(builder.toString()), XContentType.JSON)
+                    .build();
+            final RestChannel channel = new FakeRestChannel(request, randomBoolean(), 1);
+            final IllegalArgumentException e =
+                    expectThrows(IllegalArgumentException.class, () -> handler.handleRequest(request, channel, mock(NodeClient.class)));
+            assertThat(e, hasToString(containsString("request [GET /] does not support having a body")));
+            assertFalse(executed.get());
+        }
+    }
+
 }

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

@@ -20,12 +20,15 @@
 package org.elasticsearch.rest;
 
 import org.elasticsearch.ElasticsearchParseException;
-import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.CheckedConsumer;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.collect.MapBuilder;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
+import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentType;
+import org.elasticsearch.http.HttpChannel;
+import org.elasticsearch.http.HttpRequest;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.rest.FakeRestRequest;
 
@@ -41,8 +44,62 @@
 import static java.util.Collections.singletonMap;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 public class RestRequestTests extends ESTestCase {
+
+    public void testContentConsumesContent() {
+        runConsumesContentTest(RestRequest::content, true);
+    }
+
+    public void testRequiredContentConsumesContent() {
+        runConsumesContentTest(RestRequest::requiredContent, true);
+    }
+
+    public void testContentParserConsumesContent() {
+        runConsumesContentTest(RestRequest::contentParser, true);
+    }
+
+    public void testContentOrSourceParamConsumesContent() {
+        runConsumesContentTest(RestRequest::contentOrSourceParam, true);
+    }
+
+    public void testContentOrSourceParamsParserConsumesContent() {
+        runConsumesContentTest(RestRequest::contentOrSourceParamParser, true);
+    }
+
+    public void testWithContentOrSourceParamParserOrNullConsumesContent() {
+        @SuppressWarnings("unchecked") CheckedConsumer<XContentParser, IOException> consumer = mock(CheckedConsumer.class);
+        runConsumesContentTest(request -> request.withContentOrSourceParamParserOrNull(consumer), true);
+    }
+
+    public void testApplyContentParserConsumesContent() {
+        @SuppressWarnings("unchecked") CheckedConsumer<XContentParser, IOException> consumer = mock(CheckedConsumer.class);
+        runConsumesContentTest(request -> request.applyContentParser(consumer), true);
+    }
+
+    public void testHasContentDoesNotConsumesContent() {
+        runConsumesContentTest(RestRequest::hasContent, false);
+    }
+
+    private <T extends Exception> void runConsumesContentTest(
+            final CheckedConsumer<RestRequest, T> consumer, final boolean expected) {
+        final HttpRequest httpRequest = mock(HttpRequest.class);
+        when (httpRequest.uri()).thenReturn("");
+        when (httpRequest.content()).thenReturn(new BytesArray(new byte[1]));
+        final RestRequest request =
+                RestRequest.request(mock(NamedXContentRegistry.class), httpRequest, mock(HttpChannel.class));
+        request.setXContentType(XContentType.JSON);
+        assertFalse(request.isContentConsumed());
+        try {
+            consumer.accept(request);
+        } catch (final Exception e) {
+            throw new RuntimeException(e);
+        }
+        assertThat(request.isContentConsumed(), equalTo(expected));
+    }
+
     public void testContentParser() throws IOException {
         Exception e = expectThrows(ElasticsearchParseException.class, () ->
             contentRestRequest("", emptyMap()).contentParser());
@@ -211,14 +268,10 @@ public String uri() {
             return restRequest.uri();
         }
 
-        @Override
-        public boolean hasContent() {
-            return Strings.hasLength(content());
-        }
-
         @Override
         public BytesReference content() {
             return restRequest.content();
         }
     }
+
 }

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

@@ -48,11 +48,6 @@ private FakeRestRequest(NamedXContentRegistry xContentRegistry, HttpRequest http
         super(xContentRegistry, params, httpRequest.uri(), httpRequest.getHeaders(), httpRequest, httpChannel);
     }
 
-    @Override
-    public boolean hasContent() {
-        return content() != null;
-    }
-
     private static class FakeHttpRequest implements HttpRequest {
 
         private final Method method;
@@ -166,7 +161,7 @@ public static class Builder {
 
         private Map<String, String> params = new HashMap<>();
 
-        private BytesReference content;
+        private BytesReference content = BytesArray.EMPTY;
 
         private String path = "/";