From 8d61c87d2b9373394bd1b758579d7486cf46369e Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 15 Jan 2019 16:52:57 -0500 Subject: [PATCH 1/7] Reject all requests that have an unconsumed body 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. --- .../elasticsearch/rest/BaseRestHandler.java | 4 + .../org/elasticsearch/rest/RestRequest.java | 14 +++- .../admin/indices/RestForceMergeAction.java | 9 +-- .../forcemerge/RestForceMergeActionTests.java | 11 ++- .../rest/BaseRestHandlerTests.java | 78 +++++++++++++++++++ .../test/rest/FakeRestRequest.java | 2 +- 6 files changed, 108 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java b/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java index 98611e4c9d8ab..36189381c5b22 100644 --- a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java +++ b/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); diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index 813d6feb55167..7ac0cff0bd914 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -19,6 +19,7 @@ package org.elasticsearch.rest; +import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.SetOnce; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.Booleans; @@ -66,6 +67,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 params, String path, Map> headers, HttpRequest httpRequest, HttpChannel httpChannel) { final XContentType xContentType; @@ -173,10 +180,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(); } diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestForceMergeAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestForceMergeAction.java index 6ec4cec77193e..2d9d691d2c71a 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestForceMergeAction.java +++ b/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)); } + } diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java index aeb5beb09e2fc..558533444780d 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java @@ -25,9 +25,11 @@ import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.rest.RestChannel; 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 +41,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")); } + } diff --git a/server/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java b/server/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java index 835dd7cd9fab0..68e3c8416b9c2 100644 --- a/server/src/test/java/org/elasticsearch/rest/BaseRestHandlerTests.java +++ b/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()); + } + } + } diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java index 2aec495390b6c..699aeed327abd 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java @@ -50,7 +50,7 @@ private FakeRestRequest(NamedXContentRegistry xContentRegistry, HttpRequest http @Override public boolean hasContent() { - return content() != null; + return content(false) != null; } private static class FakeHttpRequest implements HttpRequest { From c7c3047fd646594c69cae5cb04f019ac6f3b5e54 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 15 Jan 2019 17:43:08 -0500 Subject: [PATCH 2/7] Update server/src/main/java/org/elasticsearch/rest/RestRequest.java --- server/src/main/java/org/elasticsearch/rest/RestRequest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index 7ac0cff0bd914..fe976ee4ddce6 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -19,7 +19,6 @@ package org.elasticsearch.rest; -import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.SetOnce; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.Booleans; From 92c822ab797a2fcb2a1fd2bedaec4a6302983e04 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 15 Jan 2019 22:00:49 -0500 Subject: [PATCH 3/7] More tests --- .../elasticsearch/rest/RestRequestTests.java | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java index 3ad9c61de3c8e..396d38756c514 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java @@ -20,12 +20,16 @@ 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.http.HttpChannel; +import org.elasticsearch.http.HttpRequest; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.rest.FakeRestRequest; @@ -36,13 +40,68 @@ import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; import static java.util.Collections.emptyMap; 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 consumer = mock(CheckedConsumer.class); + runConsumesContentTest(request -> request.withContentOrSourceParamParserOrNull(consumer), true); + } + + public void testApplyContentParserConsumesContent() { + @SuppressWarnings("unchecked") CheckedConsumer consumer = mock(CheckedConsumer.class); + runConsumesContentTest(request -> request.applyContentParser(consumer), true); + } + + public void testHasContentDoesNotConsumesContent() { + runConsumesContentTest(RestRequest::hasContent, false); + } + + private void runConsumesContentTest( + final CheckedConsumer 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()); From d5bfbcb381175aa632278bc782b8a1b5cae26f17 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 15 Jan 2019 22:10:48 -0500 Subject: [PATCH 4/7] Burn in hell checkstyle --- .../admin/indices/forcemerge/RestForceMergeActionTests.java | 1 - .../src/test/java/org/elasticsearch/rest/RestRequestTests.java | 1 - 2 files changed, 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java index 558533444780d..2d4093d8525d9 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java @@ -25,7 +25,6 @@ import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContent; -import org.elasticsearch.rest.RestChannel; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.action.admin.indices.RestForceMergeAction; import org.elasticsearch.test.ESTestCase; diff --git a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java index 396d38756c514..7cf09de6305d0 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java @@ -40,7 +40,6 @@ import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicReference; -import java.util.function.Consumer; import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; From 60ce8d0bd37755b2627280e934f6dbf795c19b2f Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 15 Jan 2019 23:27:39 -0500 Subject: [PATCH 5/7] Fix broken test --- .../main/java/org/elasticsearch/test/rest/FakeRestRequest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java index 699aeed327abd..9e3535047000c 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java @@ -50,7 +50,8 @@ private FakeRestRequest(NamedXContentRegistry xContentRegistry, HttpRequest http @Override public boolean hasContent() { - return content(false) != null; + final BytesReference content = content(false); + return content != null && content.length() > 0; } private static class FakeHttpRequest implements HttpRequest { From 0231a1d757513e358cab2a3e1f0b5f5f46003d24 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 15 Jan 2019 23:40:30 -0500 Subject: [PATCH 6/7] Remove unnecessary override --- .../java/org/elasticsearch/rest/RestRequestTests.java | 5 ----- .../java/org/elasticsearch/test/rest/FakeRestRequest.java | 8 +------- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java index 7cf09de6305d0..38b054bec6773 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java @@ -269,11 +269,6 @@ public String uri() { return restRequest.uri(); } - @Override - public boolean hasContent() { - return Strings.hasLength(content()); - } - @Override public BytesReference content() { return restRequest.content(); diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java index 9e3535047000c..a659d6af5c6aa 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java @@ -48,12 +48,6 @@ private FakeRestRequest(NamedXContentRegistry xContentRegistry, HttpRequest http super(xContentRegistry, params, httpRequest.uri(), httpRequest.getHeaders(), httpRequest, httpChannel); } - @Override - public boolean hasContent() { - final BytesReference content = content(false); - return content != null && content.length() > 0; - } - private static class FakeHttpRequest implements HttpRequest { private final Method method; @@ -167,7 +161,7 @@ public static class Builder { private Map params = new HashMap<>(); - private BytesReference content; + private BytesReference content = BytesArray.EMPTY; private String path = "/"; From 58cb7c058ebd8146c92e84908cdab0f430135076 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 15 Jan 2019 23:56:01 -0500 Subject: [PATCH 7/7] Fix imports --- .../src/test/java/org/elasticsearch/rest/RestRequestTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java index 38b054bec6773..8a2994a69816b 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java @@ -21,7 +21,6 @@ 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; @@ -274,4 +273,5 @@ public BytesReference content() { return restRequest.content(); } } + }