Require and preserve content type for filtered rest requests (#84950)

RestRequest#getXContentType cannot normally be null for a request with a non-empty request body, or the rest controll will reject the request, see eg RestControllerTests#testUnknownContentWithContentStream. This PR fixes sloppy behavior where the RestRequestFilter cleared the xContentType field of the filtered request. Fixes #84784
elastic · Mar 14, 2022 · 56326ef · 56326ef
1 parent ce0edd9
commit 56326ef
Show file tree

Hide file tree

Showing 5 changed files with 80 additions and 17 deletions.
diff --git a/docs/changelog/84914.yaml b/docs/changelog/84914.yaml
@@ -0,0 +1,6 @@
+pr: 84914
+summary: Require and preserve content type for filtered rest requests
+area: Infra/Core
+type: bug
+issues:
+ - 84784
diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java
@@ -127,6 +127,9 @@ protected RestRequest(RestRequest other) {
         assert other.parserConfig.restApiVersion().equals(other.restApiVersion);
         this.parsedAccept = other.parsedAccept;
         this.parsedContentType = other.parsedContentType;
+        if (other.xContentType.get() != null) {
+            this.xContentType.set(other.xContentType.get());
+        }
         this.restApiVersion = other.restApiVersion;
         this.parserConfig = other.parserConfig;
         this.httpRequest = other.httpRequest;

diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequestFilter.java b/server/src/main/java/org/elasticsearch/rest/RestRequestFilter.java
@@ -45,8 +45,11 @@ public boolean hasContent() {
                 @Override
                 public BytesReference content() {
                     if (filteredBytes == null) {
-                        BytesReference content = restRequest.content();
-                        Tuple<XContentType, Map<String, Object>> result = XContentHelper.convertToMap(content, true);
+                        Tuple<XContentType, Map<String, Object>> result = XContentHelper.convertToMap(
+                            restRequest.requiredContent(),
+                            true,
+                            restRequest.getXContentType()
+                        );
                         Map<String, Object> transformedSource = XContentMapValues.filter(
                             result.v2(),
                             null,

diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java
@@ -383,7 +383,10 @@ public void testDispatchRequiresContentTypeForRequestsWithContent() {
     }
 
     public void testDispatchDoesNotRequireContentTypeForRequestsWithoutContent() {
-        FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).build();
+        RestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).build();
+        if (randomBoolean()) {
+            fakeRestRequest = new RestRequest(fakeRestRequest);
+        }
         AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.OK);
 
         assertFalse(channel.getSendResponseCalled());
@@ -393,10 +396,13 @@ public void testDispatchDoesNotRequireContentTypeForRequestsWithoutContent() {
 
     public void testDispatchFailsWithPlainText() {
         String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead()));
-        FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(
-            new BytesArray(content),
-            null
-        ).withPath("/foo").withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("text/plain"))).build();
+        RestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(new BytesArray(content), null)
+            .withPath("/foo")
+            .withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("text/plain")))
+            .build();
+        if (randomBoolean()) {
+            fakeRestRequest = new RestRequest(fakeRestRequest);
+        }
         AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.NOT_ACCEPTABLE);
         restController.registerHandler(
             new Route(GET, "/foo"),
@@ -411,10 +417,13 @@ public void testDispatchFailsWithPlainText() {
     }
 
     public void testDispatchUnsupportedContentType() {
-        FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(new BytesArray("{}"), null)
+        RestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(new BytesArray("{}"), null)
             .withPath("/")
             .withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("application/x-www-form-urlencoded")))
             .build();
+        if (randomBoolean()) {
+            fakeRestRequest = new RestRequest(fakeRestRequest);
+        }
         AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.NOT_ACCEPTABLE);
 
         assertFalse(channel.getSendResponseCalled());
@@ -425,10 +434,13 @@ public void testDispatchUnsupportedContentType() {
     public void testDispatchWorksWithNewlineDelimitedJson() {
         final String mediaType = "application/x-ndjson";
         String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead()));
-        FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(
-            new BytesArray(content),
-            null
-        ).withPath("/foo").withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList(mediaType))).build();
+        RestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(new BytesArray(content), null)
+            .withPath("/foo")
+            .withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList(mediaType)))
+            .build();
+        if (randomBoolean()) {
+            fakeRestRequest = new RestRequest(fakeRestRequest);
+        }
         AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.OK);
         restController.registerHandler(new Route(GET, "/foo"), new RestHandler() {
             @Override
@@ -453,10 +465,13 @@ public void testDispatchWithContentStream() {
         final String mediaType = randomFrom("application/json", "application/smile");
         String content = randomAlphaOfLength((int) Math.round(BREAKER_LIMIT.getBytes() / inFlightRequestsBreaker.getOverhead()));
         final List<String> contentTypeHeader = Collections.singletonList(mediaType);
-        FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(
+        RestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(
             new BytesArray(content),
             RestRequest.parseContentType(contentTypeHeader)
         ).withPath("/foo").withHeaders(Collections.singletonMap("Content-Type", contentTypeHeader)).build();
+        if (randomBoolean()) {
+            fakeRestRequest = new RestRequest(fakeRestRequest);
+        }
         AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.OK);
         restController.registerHandler(new Route(GET, "/foo"), new RestHandler() {
             @Override
@@ -478,10 +493,13 @@ public boolean supportsContentStream() {
     }
 
     public void testDispatchWithContentStreamNoContentType() {
-        FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(new BytesArray("{}"), null)
+        RestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(new BytesArray("{}"), null)
             .withPath("/foo")
             .build();
         AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.NOT_ACCEPTABLE);
+        if (randomBoolean()) {
+            fakeRestRequest = new RestRequest(fakeRestRequest);
+        }
         restController.registerHandler(new Route(GET, "/foo"), new RestHandler() {
             @Override
             public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
@@ -500,10 +518,13 @@ public boolean supportsContentStream() {
     }
 
     public void testNonStreamingXContentCausesErrorResponse() throws IOException {
-        FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(
+        RestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(
             BytesReference.bytes(YamlXContent.contentBuilder().startObject().endObject()),
             XContentType.YAML
         ).withPath("/foo").build();
+        if (randomBoolean()) {
+            fakeRestRequest = new RestRequest(fakeRestRequest);
+        }
         AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.NOT_ACCEPTABLE);
         restController.registerHandler(new Route(GET, "/foo"), new RestHandler() {
             @Override
@@ -522,10 +543,13 @@ public boolean supportsContentStream() {
     }
 
     public void testUnknownContentWithContentStream() {
-        FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(
+        RestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(
             new BytesArray("aaaabbbbb"),
             null
         ).withPath("/foo").withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("foo/bar"))).build();
+        if (randomBoolean()) {
+            fakeRestRequest = new RestRequest(fakeRestRequest);
+        }
         AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.NOT_ACCEPTABLE);
         restController.registerHandler(new Route(GET, "/foo"), new RestHandler() {
             @Override
@@ -898,6 +922,10 @@ private static RestRequest testRestRequest(String path, String content, XContent
         FakeRestRequest.Builder builder = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY);
         builder.withPath(path);
         builder.withContent(new BytesArray(content), xContentType);
-        return builder.build();
+        if (randomBoolean()) {
+            return builder.build();
+        } else {
+            return new RestRequest(builder.build());
+        }
     }
 }
diff --git a/.../security/src/test/java/org/elasticsearch/xpack/security/rest/RestRequestFilterTests.java b/.../security/src/test/java/org/elasticsearch/xpack/security/rest/RestRequestFilterTests.java
@@ -22,6 +22,8 @@
 import java.util.Collections;
 import java.util.Map;
 
+import static org.hamcrest.Matchers.containsString;
+
 public class RestRequestFilterTests extends ESTestCase {
 
     public void testFilteringItemsInSubLevels() throws IOException {
@@ -101,4 +103,25 @@ public void testRemoteAddressWorks() throws IOException {
         RestRequest filtered = filter.getFilteredRequest(restRequest);
         assertEquals(address, filtered.getHttpChannel().getRemoteAddress());
     }
+
+    public void testFilterUnknownContentTypeThrows() throws IOException {
+        RestRequest restRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withContent(new BytesArray("""
+            {"simple": "test"}"""), null)
+            .withPath("/whatever")
+            .withHeaders(Collections.singletonMap("Content-Type", Collections.singletonList("foo/bar")))
+            .build();
+        if (randomBoolean()) {
+            restRequest = new TestRestRequest(restRequest);
+        }
+        RestRequestFilter filter = () -> Collections.singleton("root.second.third");
+        RestRequest filtered = filter.getFilteredRequest(restRequest);
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> filtered.content());
+        assertThat(e.getMessage(), containsString("unknown content type"));
+    }
+
+    private static class TestRestRequest extends RestRequest {
+        TestRestRequest(RestRequest other) {
+            super(other);
+        }
+    }
 }