From ad355f3e0bd0f9e141e9c7707f02651a377f74e9 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Wed, 6 Sep 2017 09:58:46 +0200 Subject: [PATCH] Forbid direct usage of ContentType.create() methods (#26457) It's easy to create a wrong Content-Type header when converting a XContentType to a Apache HTTP ContentType instance. This commit the direct usages of ContentType.create() methods in favor of a Request.createContentType(XContentType) method that does the right thing. Closes #26438 --- client/rest-high-level/build.gradle | 1 + .../org/elasticsearch/client/Request.java | 21 +++++++++++++++---- .../forbidden/rest-high-level-signatures.txt | 21 +++++++++++++++++++ .../elasticsearch/client/RequestTests.java | 21 ++++++++++++------- 4 files changed, 52 insertions(+), 12 deletions(-) create mode 100644 client/rest-high-level/src/main/resources/forbidden/rest-high-level-signatures.txt diff --git a/client/rest-high-level/build.gradle b/client/rest-high-level/build.gradle index 600bf602080..ba97605dba8 100644 --- a/client/rest-high-level/build.gradle +++ b/client/rest-high-level/build.gradle @@ -59,4 +59,5 @@ forbiddenApisMain { // core does not depend on the httpclient for compile so we add the signatures here. We don't add them for test as they are already // specified signaturesURLs += [PrecommitTasks.getResource('/forbidden/http-signatures.txt')] + signaturesURLs += [file('src/main/resources/forbidden/rest-high-level-signatures.txt').toURI().toURL()] } \ No newline at end of file diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java index 9e881cf7b9a..77e501551cd 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java @@ -42,6 +42,7 @@ import org.elasticsearch.action.support.WriteRequest; import org.elasticsearch.action.update.UpdateRequest; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.lucene.uid.Versions; import org.elasticsearch.common.unit.TimeValue; @@ -57,6 +58,7 @@ import org.elasticsearch.search.fetch.subphase.FetchSourceContext; import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.nio.charset.Charset; import java.util.Collections; import java.util.HashMap; import java.util.Locale; @@ -139,8 +141,8 @@ final class Request { bulkContentType = XContentType.JSON; } - byte separator = bulkContentType.xContent().streamSeparator(); - ContentType requestContentType = ContentType.create(bulkContentType.mediaType()); + final byte separator = bulkContentType.xContent().streamSeparator(); + final ContentType requestContentType = createContentType(bulkContentType); ByteArrayOutputStream content = new ByteArrayOutputStream(); for (DocWriteRequest request : bulkRequest.requests()) { @@ -268,7 +270,7 @@ final class Request { parameters.withWaitForActiveShards(indexRequest.waitForActiveShards()); BytesRef source = indexRequest.source().toBytesRef(); - ContentType contentType = ContentType.create(indexRequest.getContentType().mediaType()); + ContentType contentType = createContentType(indexRequest.getContentType()); HttpEntity entity = new ByteArrayEntity(source.bytes, source.offset, source.length, contentType); return new Request(method, endpoint, parameters.getParams(), entity); @@ -352,7 +354,7 @@ final class Request { private static HttpEntity createEntity(ToXContent toXContent, XContentType xContentType) throws IOException { BytesRef source = XContentHelper.toXContent(toXContent, xContentType, false).toBytesRef(); - return new ByteArrayEntity(source.bytes, source.offset, source.length, ContentType.create(xContentType.mediaType())); + return new ByteArrayEntity(source.bytes, source.offset, source.length, createContentType(xContentType)); } static String endpoint(String[] indices, String[] types, String endpoint) { @@ -372,6 +374,17 @@ final class Request { return joiner.toString(); } + /** + * Returns a {@link ContentType} from a given {@link XContentType}. + * + * @param xContentType the {@link XContentType} + * @return the {@link ContentType} + */ + @SuppressForbidden(reason = "Only allowed place to convert a XContentType to a ContentType") + static ContentType createContentType(final XContentType xContentType) { + return ContentType.create(xContentType.mediaTypeWithoutParameters(), (Charset) null); + } + /** * Utility class to build request's parameters map and centralize all parameter names. */ diff --git a/client/rest-high-level/src/main/resources/forbidden/rest-high-level-signatures.txt b/client/rest-high-level/src/main/resources/forbidden/rest-high-level-signatures.txt new file mode 100644 index 00000000000..fb2330f3f08 --- /dev/null +++ b/client/rest-high-level/src/main/resources/forbidden/rest-high-level-signatures.txt @@ -0,0 +1,21 @@ +# Licensed to Elasticsearch under one or more contributor +# license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright +# ownership. Elasticsearch licenses this file to you under +# the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on +# an 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +# either express or implied. See the License for the specific +# language governing permissions and limitations under the License. + +@defaultMessage Use Request#createContentType(XContentType) to be sure to pass the right MIME type +org.apache.http.entity.ContentType#create(java.lang.String) +org.apache.http.entity.ContentType#create(java.lang.String,java.lang.String) +org.apache.http.entity.ContentType#create(java.lang.String,java.nio.charset.Charset) +org.apache.http.entity.ContentType#create(java.lang.String,org.apache.http.NameValuePair[]) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java index f18e348adce..f7996bec924 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java @@ -278,7 +278,7 @@ public class RequestTests extends ESTestCase { HttpEntity entity = request.entity; assertTrue(entity instanceof ByteArrayEntity); - assertEquals(indexRequest.getContentType().mediaType(), entity.getContentType().getValue()); + assertEquals(indexRequest.getContentType().mediaTypeWithoutParameters(), entity.getContentType().getValue()); try (XContentParser parser = createParser(xContentType.xContent(), entity.getContent())) { assertEquals(nbFields, parser.map().size()); } @@ -488,7 +488,7 @@ public class RequestTests extends ESTestCase { assertEquals("/_bulk", request.endpoint); assertEquals(expectedParams, request.params); assertEquals("POST", request.method); - assertEquals(xContentType.mediaType(), request.entity.getContentType().getValue()); + assertEquals(xContentType.mediaTypeWithoutParameters(), request.entity.getContentType().getValue()); byte[] content = new byte[(int) request.entity.getContentLength()]; try (InputStream inputStream = request.entity.getContent()) { Streams.readFully(inputStream, content); @@ -541,7 +541,7 @@ public class RequestTests extends ESTestCase { bulkRequest.add(new DeleteRequest("index", "type", "2")); Request request = Request.bulk(bulkRequest); - assertEquals(XContentType.JSON.mediaType(), request.entity.getContentType().getValue()); + assertEquals(XContentType.JSON.mediaTypeWithoutParameters(), request.entity.getContentType().getValue()); } { XContentType xContentType = randomFrom(XContentType.JSON, XContentType.SMILE); @@ -551,7 +551,7 @@ public class RequestTests extends ESTestCase { bulkRequest.add(new DeleteRequest("index", "type", "2")); Request request = Request.bulk(bulkRequest); - assertEquals(xContentType.mediaType(), request.entity.getContentType().getValue()); + assertEquals(xContentType.mediaTypeWithoutParameters(), request.entity.getContentType().getValue()); } { XContentType xContentType = randomFrom(XContentType.JSON, XContentType.SMILE); @@ -563,7 +563,7 @@ public class RequestTests extends ESTestCase { } Request request = Request.bulk(new BulkRequest().add(updateRequest)); - assertEquals(xContentType.mediaType(), request.entity.getContentType().getValue()); + assertEquals(xContentType.mediaTypeWithoutParameters(), request.entity.getContentType().getValue()); } { BulkRequest bulkRequest = new BulkRequest(); @@ -732,7 +732,7 @@ public class RequestTests extends ESTestCase { assertEquals("/_search/scroll", request.endpoint); assertEquals(0, request.params.size()); assertToXContentBody(searchScrollRequest, request.entity); - assertEquals(Request.REQUEST_BODY_CONTENT_TYPE.mediaType(), request.entity.getContentType().getValue()); + assertEquals(Request.REQUEST_BODY_CONTENT_TYPE.mediaTypeWithoutParameters(), request.entity.getContentType().getValue()); } public void testClearScroll() throws IOException { @@ -746,12 +746,12 @@ public class RequestTests extends ESTestCase { assertEquals("/_search/scroll", request.endpoint); assertEquals(0, request.params.size()); assertToXContentBody(clearScrollRequest, request.entity); - assertEquals(Request.REQUEST_BODY_CONTENT_TYPE.mediaType(), request.entity.getContentType().getValue()); + assertEquals(Request.REQUEST_BODY_CONTENT_TYPE.mediaTypeWithoutParameters(), request.entity.getContentType().getValue()); } private static void assertToXContentBody(ToXContent expectedBody, HttpEntity actualEntity) throws IOException { BytesReference expectedBytes = XContentHelper.toXContent(expectedBody, Request.REQUEST_BODY_CONTENT_TYPE, false); - assertEquals(XContentType.JSON.mediaType(), actualEntity.getContentType().getValue()); + assertEquals(XContentType.JSON.mediaTypeWithoutParameters(), actualEntity.getContentType().getValue()); assertEquals(expectedBytes, new BytesArray(EntityUtils.toByteArray(actualEntity))); } @@ -793,6 +793,11 @@ public class RequestTests extends ESTestCase { assertEquals("/a/_create", Request.endpoint("a", null, null, "_create")); } + public void testCreateContentType() { + final XContentType xContentType = randomFrom(XContentType.values()); + assertEquals(xContentType.mediaTypeWithoutParameters(), Request.createContentType(xContentType).getMimeType()); + } + public void testEnforceSameContentType() { XContentType xContentType = randomFrom(XContentType.JSON, XContentType.SMILE); IndexRequest indexRequest = new IndexRequest().source(singletonMap("field", "value"), xContentType);