Deprecate requests that have an unconsumed body

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

@@ -145,7 +145,7 @@ public boolean hasContent() {
     }
 
     @Override
-    public BytesReference content() {
+    public BytesReference innerContent() {
         return content;
     }
 

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

@@ -25,6 +25,7 @@
 import org.elasticsearch.common.CheckedConsumer;
 import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.component.AbstractComponent;
+import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Setting.Property;
 import org.elasticsearch.common.settings.Settings;
@@ -53,6 +54,8 @@
  */
 public abstract class BaseRestHandler extends AbstractComponent implements RestHandler {
 
+    private final DeprecationLogger deprecationLogger;
+
     public static final Setting<Boolean> MULTI_ALLOW_EXPLICIT_INDEX =
         Setting.boolSetting("rest.action.multi.allow_explicit_index", true, Property.NodeScope);
 
@@ -67,6 +70,7 @@ public abstract class BaseRestHandler extends AbstractComponent implements RestH
 
     protected BaseRestHandler(Settings settings) {
         // TODO drop settings from ctor
+        this.deprecationLogger = new DeprecationLogger(logger);
     }
 
     public final long getUsageCount() {
@@ -99,6 +103,13 @@ public final void handleRequest(RestRequest request, RestChannel channel, NodeCl
             throw new IllegalArgumentException(unrecognized(request, unconsumedParams, candidateParams, "parameter"));
         }
 
+        if (request.hasContent() && request.isContentConsumed() == false) {
+            deprecationLogger.deprecated(
+                    "request [{} {}] does not support having a body; Elasticsearch 7.x+ will reject such requests",
+                    request.method(),
+                    request.path());
+        }
+
         usageCount.increment();
         // execute the action
         action.accept(channel);

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

@@ -63,6 +63,12 @@ public abstract class RestRequest implements ToXContent.Params {
     private final Set<String> consumedParams = new HashSet<>();
     private final SetOnce<XContentType> xContentType = new SetOnce<>();
 
+    private boolean contentConsumed = false;
+
+    public boolean isContentConsumed() {
+        return contentConsumed;
+    }
+
     /**
      * Creates a new REST request.
      *
@@ -156,7 +162,12 @@ public final String path() {
 
     public abstract boolean hasContent();
 
-    public abstract BytesReference content();
+    public final BytesReference content() {
+        contentConsumed = true;
+        return innerContent();
+    }
+
+    protected abstract BytesReference innerContent();
 
     /**
      * @return content of the request body or throw an exception if the body or content type is missing

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,77 @@ 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 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";
+            }
+
+        };
+
+        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));
+            assertWarnings("request [GET /] does not support having a body; Elasticsearch 7.x+ will reject such requests");
+            assertTrue(executed.get());
+        }
+    }
+
 }

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

@@ -187,7 +187,7 @@ public boolean hasContent() {
                     }
 
                     @Override
-                    public BytesReference content() {
+                    public BytesReference innerContent() {
                         return null;
                     }
                 };

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

@@ -569,7 +569,7 @@ public boolean hasContent() {
         }
 
         @Override
-        public BytesReference content() {
+        public BytesReference innerContent() {
             return content;
         }
 

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

@@ -20,11 +20,13 @@
 package org.elasticsearch.rest;
 
 import org.elasticsearch.ElasticsearchParseException;
+import org.elasticsearch.common.CheckedConsumer;
 import org.elasticsearch.common.Strings;
 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.test.ESTestCase;
 
@@ -40,8 +42,79 @@
 import static java.util.Collections.singletonMap;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.mockito.Mockito.mock;
 
 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 RestRequest request = new RestRequest(mock(NamedXContentRegistry.class), "", Collections.emptyMap()) {
+            @Override
+            public Method method() {
+                return Method.GET;
+            }
+
+            @Override
+            public String uri() {
+                return "";
+            }
+
+            @Override
+            public boolean hasContent() {
+                return true;
+            }
+
+            private final BytesReference content = new BytesArray(new byte[1]);
+
+            @Override
+            public BytesReference innerContent() {
+                return content;
+            }
+        };
+        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, () ->
             new ContentRestRequest("", emptyMap()).contentParser());
@@ -194,7 +267,7 @@ public boolean hasContent() {
         }
 
         @Override
-        public BytesReference content() {
+        public BytesReference innerContent() {
             return content;
         }
 
@@ -208,4 +281,5 @@ public Method method() {
             throw new UnsupportedOperationException("Not used by this test");
         }
     }
+
 }

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

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.test.rest;
 
+import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
 import org.elasticsearch.common.xcontent.XContentType;
@@ -29,6 +30,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 
 public class FakeRestRequest extends RestRequest {
 
@@ -37,13 +39,13 @@ public class FakeRestRequest extends RestRequest {
     private final SocketAddress remoteAddress;
 
     public FakeRestRequest() {
-        this(NamedXContentRegistry.EMPTY, new HashMap<>(), new HashMap<>(), null, Method.GET, "/", null);
+        this(NamedXContentRegistry.EMPTY, new HashMap<>(), new HashMap<>(), BytesArray.EMPTY, Method.GET, "/", null);
     }
 
     private FakeRestRequest(NamedXContentRegistry xContentRegistry, Map<String, List<String>> headers,
                             Map<String, String> params, BytesReference content, Method method, String path, SocketAddress remoteAddress) {
         super(xContentRegistry, params, path, headers);
-        this.content = content;
+        this.content = Objects.requireNonNull(content);
         this.method = method;
         this.remoteAddress = remoteAddress;
     }
@@ -60,11 +62,11 @@ public String uri() {
 
     @Override
     public boolean hasContent() {
-        return content != null;
+        return content.length() > 0;
     }
 
     @Override
-    public BytesReference content() {
+    public BytesReference innerContent() {
         return content;
     }
 
@@ -80,7 +82,7 @@ public static class Builder {
 
         private Map<String, String> params = new HashMap<>();
 
-        private BytesReference content;
+        private BytesReference content = BytesArray.EMPTY;
 
         private String path = "/";
 
@@ -103,7 +105,7 @@ public Builder withParams(Map<String, String> params) {
         }
 
         public Builder withContent(BytesReference content, XContentType xContentType) {
-            this.content = content;
+            this.content = Objects.requireNonNull(content);
             if (xContentType != null) {
                 headers.put("Content-Type", Collections.singletonList(xContentType.mediaType()));
             }

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/rest/RestRequestFilter.java

@@ -65,7 +65,7 @@ public SocketAddress getLocalAddress() {
                 }
 
                 @Override
-                public BytesReference content() {
+                public BytesReference innerContent() {
                     if (filteredBytes == null) {
                         BytesReference content = restRequest.content();
                         Tuple<XContentType, Map<String, Object>> result = XContentHelper.convertToMap(content, true);