From 9dff5e2af7555886c4f8309cdc724b871db24120 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 13 Feb 2017 09:34:52 -0500 Subject: [PATCH] Properly encode location header Today when trying to encode the location header to ASCII, we rely on the Java URI API. This API requires a proper URI which blows up whenever the URI contains, for example, a space (which can happen if the type, ID, or routing contain a space). This commit addresses this issue by properly encoding the URI. Additionally, we remove the need to create a URI simplifying the code flow. Relates #23133 --- .../action/DocWriteResponse.java | 55 ++++++++++------ .../action/RestStatusToXContentListener.java | 2 +- .../rest/action/document/RestIndexAction.java | 10 +-- .../action/document/RestUpdateAction.java | 11 +--- .../action/DocWriteResponseTests.java | 66 +++++++++---------- 5 files changed, 69 insertions(+), 75 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/DocWriteResponse.java b/core/src/main/java/org/elasticsearch/action/DocWriteResponse.java index 920ef1b5438..2f7b3fac44d 100644 --- a/core/src/main/java/org/elasticsearch/action/DocWriteResponse.java +++ b/core/src/main/java/org/elasticsearch/action/DocWriteResponse.java @@ -38,8 +38,11 @@ import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.rest.RestStatus; import java.io.IOException; +import java.io.UnsupportedEncodingException; import java.net.URI; import java.net.URISyntaxException; +import java.net.URLEncoder; +import java.nio.charset.Charset; import java.util.Locale; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; @@ -201,31 +204,43 @@ public abstract class DocWriteResponse extends ReplicationResponse implements Wr } /** - * Gets the location of the written document as a string suitable for a {@code Location} header. - * @param routing any routing used in the request. If null the location doesn't include routing information. + * Return the relative URI for the location of the document suitable for use in the {@code Location} header. The use of relative URIs is + * permitted as of HTTP/1.1 (cf. https://tools.ietf.org/html/rfc7231#section-7.1.2). * + * @param routing custom routing or {@code null} if custom routing is not used + * @return the relative URI for the location of the document */ - public String getLocation(@Nullable String routing) throws URISyntaxException { - // Absolute path for the location of the document. This should be allowed as of HTTP/1.1: - // https://tools.ietf.org/html/rfc7231#section-7.1.2 - String index = getIndex(); - String type = getType(); - String id = getId(); - String routingStart = "?routing="; - int bufferSize = 3 + index.length() + type.length() + id.length(); - if (routing != null) { - bufferSize += routingStart.length() + routing.length(); + public String getLocation(@Nullable String routing) { + final String encodedIndex; + final String encodedType; + final String encodedId; + final String encodedRouting; + try { + // encode the path components separately otherwise the path separators will be encoded + encodedIndex = URLEncoder.encode(getIndex(), "UTF-8"); + encodedType = URLEncoder.encode(getType(), "UTF-8"); + encodedId = URLEncoder.encode(getId(), "UTF-8"); + encodedRouting = routing == null ? null : URLEncoder.encode(routing, "UTF-8"); + } catch (final UnsupportedEncodingException e) { + throw new AssertionError(e); } - StringBuilder location = new StringBuilder(bufferSize); - location.append('/').append(index); - location.append('/').append(type); - location.append('/').append(id); - if (routing != null) { - location.append(routingStart).append(routing); + final String routingStart = "?routing="; + final int bufferSizeExcludingRouting = 3 + encodedIndex.length() + encodedType.length() + encodedId.length(); + final int bufferSize; + if (encodedRouting == null) { + bufferSize = bufferSizeExcludingRouting; + } else { + bufferSize = bufferSizeExcludingRouting + routingStart.length() + encodedRouting.length(); + } + final StringBuilder location = new StringBuilder(bufferSize); + location.append('/').append(encodedIndex); + location.append('/').append(encodedType); + location.append('/').append(encodedId); + if (encodedRouting != null) { + location.append(routingStart).append(encodedRouting); } - URI uri = new URI(location.toString()); - return uri.toASCIIString(); + return location.toString(); } @Override diff --git a/core/src/main/java/org/elasticsearch/rest/action/RestStatusToXContentListener.java b/core/src/main/java/org/elasticsearch/rest/action/RestStatusToXContentListener.java index f17399743b4..6abe61ea5ed 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/RestStatusToXContentListener.java +++ b/core/src/main/java/org/elasticsearch/rest/action/RestStatusToXContentListener.java @@ -57,7 +57,7 @@ public class RestStatusToXContentListener - client.index(indexRequest, new RestStatusToXContentListener<>(channel, r -> { - try { - return r.getLocation(indexRequest.routing()); - } catch (URISyntaxException ex) { - logger.warn("Location string is not a valid URI.", ex); - return null; - } - })); + client.index(indexRequest, new RestStatusToXContentListener<>(channel, r -> r.getLocation(indexRequest.routing()))); } } diff --git a/core/src/main/java/org/elasticsearch/rest/action/document/RestUpdateAction.java b/core/src/main/java/org/elasticsearch/rest/action/document/RestUpdateAction.java index f9593072daa..1b21f6c710c 100644 --- a/core/src/main/java/org/elasticsearch/rest/action/document/RestUpdateAction.java +++ b/core/src/main/java/org/elasticsearch/rest/action/document/RestUpdateAction.java @@ -36,7 +36,6 @@ import org.elasticsearch.rest.action.RestStatusToXContentListener; import org.elasticsearch.search.fetch.subphase.FetchSourceContext; import java.io.IOException; -import java.net.URISyntaxException; import static org.elasticsearch.rest.RestRequest.Method.POST; @@ -98,13 +97,7 @@ public class RestUpdateAction extends BaseRestHandler { }); return channel -> - client.update(updateRequest, new RestStatusToXContentListener<>(channel, r -> { - try { - return r.getLocation(updateRequest.routing()); - } catch (URISyntaxException ex) { - logger.warn("Location string is not a valid URI.", ex); - return null; - } - })); + client.update(updateRequest, new RestStatusToXContentListener<>(channel, r -> r.getLocation(updateRequest.routing()))); } + } diff --git a/core/src/test/java/org/elasticsearch/action/DocWriteResponseTests.java b/core/src/test/java/org/elasticsearch/action/DocWriteResponseTests.java index 356d74acb13..52eb8a82743 100644 --- a/core/src/test/java/org/elasticsearch/action/DocWriteResponseTests.java +++ b/core/src/test/java/org/elasticsearch/action/DocWriteResponseTests.java @@ -30,55 +30,49 @@ import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.test.ESTestCase; import java.io.IOException; -import java.net.URISyntaxException; import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.hasKey; import static org.hamcrest.Matchers.not; public class DocWriteResponseTests extends ESTestCase { - public void testGetLocation() throws URISyntaxException { - DocWriteResponse response = - new DocWriteResponse( - new ShardId("index", "uuid", 0), - "type", - "id", - SequenceNumbersService.UNASSIGNED_SEQ_NO, - 0, - Result.CREATED) { - // DocWriteResponse is abstract so we have to sneak a subclass in here to test it. - }; + public void testGetLocation() { + final DocWriteResponse response = + new DocWriteResponse( + new ShardId("index", "uuid", 0), + "type", + "id", + SequenceNumbersService.UNASSIGNED_SEQ_NO, + 0, + Result.CREATED) {}; assertEquals("/index/type/id", response.getLocation(null)); assertEquals("/index/type/id?routing=test_routing", response.getLocation("test_routing")); } - public void testGetLocationNonAscii() throws URISyntaxException { - DocWriteResponse response = - new DocWriteResponse( - new ShardId("index", "uuid", 0), - "type", - "❤", - SequenceNumbersService.UNASSIGNED_SEQ_NO, - 0, - Result.CREATED) { - }; + public void testGetLocationNonAscii() { + final DocWriteResponse response = + new DocWriteResponse( + new ShardId("index", "uuid", 0), + "type", + "❤", + SequenceNumbersService.UNASSIGNED_SEQ_NO, + 0, + Result.CREATED) {}; assertEquals("/index/type/%E2%9D%A4", response.getLocation(null)); - assertEquals("/index/type/%E2%9D%A4?routing=%C3%A4", response.getLocation("%C3%A4")); + assertEquals("/index/type/%E2%9D%A4?routing=%C3%A4", response.getLocation("ä")); } - public void testInvalidGetLocation() { - String invalidPath = "!^*$(@!^!#@"; - DocWriteResponse invalid = - new DocWriteResponse( - new ShardId("index", "uuid", 0), - "type", - invalidPath, - SequenceNumbersService.UNASSIGNED_SEQ_NO, - 0, - Result.CREATED) { - }; - Throwable exception = expectThrows(URISyntaxException.class, () -> invalid.getLocation(null)); - assertTrue(exception.getMessage().contains(invalidPath)); + public void testGetLocationWithSpaces() { + final DocWriteResponse response = + new DocWriteResponse( + new ShardId("index", "uuid", 0), + "type", + "a b", + SequenceNumbersService.UNASSIGNED_SEQ_NO, + 0, + Result.CREATED) {}; + assertEquals("/index/type/a+b", response.getLocation(null)); + assertEquals("/index/type/a+b?routing=c+d", response.getLocation("c d")); } /**