From b4d52fedb763ace94842368889c4b2fa5de69cee Mon Sep 17 00:00:00 2001 From: javanna Date: Tue, 20 Oct 2015 21:26:25 +0200 Subject: [PATCH] Restore support for escaped '/' as part of document id With #13691 we introduced some custom logic to make sure that date math expressions like don't get broken up into two where the slash appears in the expression. That said the only correct way to provide such a date math expression as part of the uri would be to properly escape the '/' instead. This fix also introduced a regression, as it would make sure that unescaped '/' are handled only in the case of date math expressions, but it removed support for properly escaped slashes anywhere else. The solution is to keep supporting escaped slashes only and require client libraries to properly escape them. This commit reverts 93ad696 and makes sure that our REST tests runner supports escaping of path parts, which was more involving than expected as each single part of the path needs to be properly escaped. I am not too happy with the current solution but it's the best I could do for now, maybe not that concerning anyway given that it's just test code. I do find uri encoding quite frustrating in java. Relates to #13691 Relates to #13665 Closes #14177 Closes #14216 --- .../elasticsearch/common/path/PathTrie.java | 65 +------------ .../common/path/PathTrieTests.java | 57 +++++------ .../elasticsearch/plugins/SitePluginIT.java | 2 +- .../test/rest/client/RestClient.java | 5 +- .../test/rest/client/RestPath.java | 97 +++++++++++++++++++ .../rest/client/http/HttpRequestBuilder.java | 47 ++++++--- .../elasticsearch/test/rest/spec/RestApi.java | 60 ++---------- 7 files changed, 176 insertions(+), 157 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/test/rest/client/RestPath.java diff --git a/core/src/main/java/org/elasticsearch/common/path/PathTrie.java b/core/src/main/java/org/elasticsearch/common/path/PathTrie.java index 3bf2a9b17ee67..c45f9e359e79f 100644 --- a/core/src/main/java/org/elasticsearch/common/path/PathTrie.java +++ b/core/src/main/java/org/elasticsearch/common/path/PathTrie.java @@ -22,8 +22,6 @@ import com.google.common.collect.ImmutableMap; import org.elasticsearch.common.Strings; -import java.util.ArrayList; -import java.util.List; import java.util.Map; import static org.elasticsearch.common.collect.MapBuilder.newMapBuilder; @@ -33,26 +31,15 @@ */ public class PathTrie { - public static interface Decoder { + public interface Decoder { String decode(String value); } - public static final Decoder NO_DECODER = new Decoder() { - @Override - public String decode(String value) { - return value; - } - }; - private final Decoder decoder; private final TrieNode root; private final char separator; private T rootValue; - public PathTrie() { - this('/', "*", NO_DECODER); - } - public PathTrie(Decoder decoder) { this('/', "*", decoder); } @@ -197,7 +184,7 @@ public T retrieve(String[] path, int index, Map params) { private void put(Map params, TrieNode node, String value) { if (params != null && node.isNamedWildcard()) { - params.put(node.namedWildcard(), value); + params.put(node.namedWildcard(), decoder.decode(value)); } } } @@ -224,7 +211,7 @@ public T retrieve(String path, Map params) { if (path.length() == 0) { return rootValue; } - String[] strings = splitPath(decoder.decode(path)); + String[] strings = Strings.splitStringToArray(path, separator); if (strings.length == 0) { return rootValue; } @@ -235,50 +222,4 @@ public T retrieve(String path, Map params) { } return root.retrieve(strings, index, params); } - - /* - Splits up the url path up by '/' and is aware of - index name expressions that appear between '<' and '>'. - */ - String[] splitPath(final String path) { - if (path == null || path.length() == 0) { - return Strings.EMPTY_ARRAY; - } - int count = 1; - boolean splitAllowed = true; - for (int i = 0; i < path.length(); i++) { - final char currentC = path.charAt(i); - if ('<' == currentC) { - splitAllowed = false; - } else if (currentC == '>') { - splitAllowed = true; - } else if (splitAllowed && currentC == separator) { - count++; - } - } - - final List result = new ArrayList<>(count); - final StringBuilder builder = new StringBuilder(); - - splitAllowed = true; - for (int i = 0; i < path.length(); i++) { - final char currentC = path.charAt(i); - if ('<' == currentC) { - splitAllowed = false; - } else if (currentC == '>') { - splitAllowed = true; - } else if (splitAllowed && currentC == separator) { - if (builder.length() > 0) { - result.add(builder.toString()); - builder.setLength(0); - } - continue; - } - builder.append(currentC); - } - if (builder.length() > 0) { - result.add(builder.toString()); - } - return result.toArray(new String[result.size()]); - } } diff --git a/core/src/test/java/org/elasticsearch/common/path/PathTrieTests.java b/core/src/test/java/org/elasticsearch/common/path/PathTrieTests.java index aec4fb248883d..1859a8ab566aa 100644 --- a/core/src/test/java/org/elasticsearch/common/path/PathTrieTests.java +++ b/core/src/test/java/org/elasticsearch/common/path/PathTrieTests.java @@ -19,13 +19,13 @@ package org.elasticsearch.common.path; +import org.elasticsearch.rest.support.RestUtils; import org.elasticsearch.test.ESTestCase; import org.junit.Test; import java.util.HashMap; import java.util.Map; -import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.nullValue; @@ -34,8 +34,15 @@ */ public class PathTrieTests extends ESTestCase { + public static final PathTrie.Decoder NO_DECODER = new PathTrie.Decoder() { + @Override + public String decode(String value) { + return value; + } + }; + public void testPath() { - PathTrie trie = new PathTrie<>(); + PathTrie trie = new PathTrie<>(NO_DECODER); trie.insert("/a/b/c", "walla"); trie.insert("a/d/g", "kuku"); trie.insert("x/b/c", "lala"); @@ -62,13 +69,13 @@ public void testPath() { } public void testEmptyPath() { - PathTrie trie = new PathTrie<>(); + PathTrie trie = new PathTrie<>(NO_DECODER); trie.insert("/", "walla"); assertThat(trie.retrieve(""), equalTo("walla")); } public void testDifferentNamesOnDifferentPath() { - PathTrie trie = new PathTrie<>(); + PathTrie trie = new PathTrie<>(NO_DECODER); trie.insert("/a/{type}", "test1"); trie.insert("/b/{name}", "test2"); @@ -82,7 +89,7 @@ public void testDifferentNamesOnDifferentPath() { } public void testSameNameOnDifferentPath() { - PathTrie trie = new PathTrie<>(); + PathTrie trie = new PathTrie<>(NO_DECODER); trie.insert("/a/c/{name}", "test1"); trie.insert("/b/{name}", "test2"); @@ -96,7 +103,7 @@ public void testSameNameOnDifferentPath() { } public void testPreferNonWildcardExecution() { - PathTrie trie = new PathTrie<>(); + PathTrie trie = new PathTrie<>(NO_DECODER); trie.insert("{test}", "test1"); trie.insert("b", "test2"); trie.insert("{test}/a", "test3"); @@ -112,7 +119,7 @@ public void testPreferNonWildcardExecution() { } public void testSamePathConcreteResolution() { - PathTrie trie = new PathTrie<>(); + PathTrie trie = new PathTrie<>(NO_DECODER); trie.insert("{x}/{y}/{z}", "test1"); trie.insert("{x}/_y/{k}", "test2"); @@ -128,7 +135,7 @@ public void testSamePathConcreteResolution() { } public void testNamedWildcardAndLookupWithWildcard() { - PathTrie trie = new PathTrie<>(); + PathTrie trie = new PathTrie<>(NO_DECODER); trie.insert("x/{test}", "test1"); trie.insert("{test}/a", "test2"); trie.insert("/{test}", "test3"); @@ -156,24 +163,20 @@ public void testNamedWildcardAndLookupWithWildcard() { assertThat(params.get("test"), equalTo("*")); } - public void testSplitPath() { - PathTrie trie = new PathTrie<>(); - assertThat(trie.splitPath("/a/"), arrayContaining("a")); - assertThat(trie.splitPath("/a/b"),arrayContaining("a", "b")); - assertThat(trie.splitPath("/a/b/c"), arrayContaining("a", "b", "c")); - assertThat(trie.splitPath("/a/b/"), arrayContaining("a", "b", "")); - assertThat(trie.splitPath("/a/b//d"), arrayContaining("a", "b", "", "d")); - - assertThat(trie.splitPath("//_search"), arrayContaining("", "_search")); - assertThat(trie.splitPath("//_search"), arrayContaining("", "_search")); - assertThat(trie.splitPath("//_search"), arrayContaining("", "_search")); - assertThat(trie.splitPath("//_search"), arrayContaining("", "_search")); - assertThat(trie.splitPath("//log/_search"), arrayContaining("", "log", "_search")); - - assertThat(trie.splitPath("/,/_search"), arrayContaining(",", "_search")); - assertThat(trie.splitPath("/,/_search"), arrayContaining(",", "_search")); - assertThat(trie.splitPath("/,/_search"), arrayContaining(",", "_search")); - assertThat(trie.splitPath("/,/_search"), arrayContaining(",", "_search")); + //https://github.com/elastic/elasticsearch/issues/14177 + //https://github.com/elastic/elasticsearch/issues/13665 + public void testEscapedSlashWithinUrl() { + PathTrie pathTrie = new PathTrie<>(RestUtils.REST_DECODER); + pathTrie.insert("/{index}/{type}/{id}", "test"); + HashMap params = new HashMap<>(); + assertThat(pathTrie.retrieve("/index/type/a%2Fe", params), equalTo("test")); + assertThat(params.get("index"), equalTo("index")); + assertThat(params.get("type"), equalTo("type")); + assertThat(params.get("id"), equalTo("a/e")); + params.clear(); + assertThat(pathTrie.retrieve("//type/id", params), equalTo("test")); + assertThat(params.get("index"), equalTo("")); + assertThat(params.get("type"), equalTo("type")); + assertThat(params.get("id"), equalTo("id")); } - } diff --git a/core/src/test/java/org/elasticsearch/plugins/SitePluginIT.java b/core/src/test/java/org/elasticsearch/plugins/SitePluginIT.java index 6e62fd9e675e0..8ecc4ee0e16eb 100644 --- a/core/src/test/java/org/elasticsearch/plugins/SitePluginIT.java +++ b/core/src/test/java/org/elasticsearch/plugins/SitePluginIT.java @@ -100,7 +100,7 @@ public void testThatPathsAreNormalized() throws Exception { notFoundUris.add("/_plugin/dummy/%2e%2e/%2e%2e/%2e%2e/%2e%2e/index.html"); notFoundUris.add("/_plugin/dummy/%2e%2e%2f%2e%2e%2f%2e%2e%2f%2e%2e%2findex.html"); notFoundUris.add("/_plugin/dummy/%2E%2E/%2E%2E/%2E%2E/%2E%2E/index.html"); - notFoundUris.add("/_plugin/dummy/..\\..\\..\\..\\..\\log4j.properties"); + notFoundUris.add("/_plugin/dummy/..%5C..%5C..%5C..%5C..%5Clog4j.properties"); for (String uri : notFoundUris) { HttpResponse response = httpClient().path(uri).execute(); diff --git a/core/src/test/java/org/elasticsearch/test/rest/client/RestClient.java b/core/src/test/java/org/elasticsearch/test/rest/client/RestClient.java index f18f9201d1a9d..5cdf728ef231e 100644 --- a/core/src/test/java/org/elasticsearch/test/rest/client/RestClient.java +++ b/core/src/test/java/org/elasticsearch/test/rest/client/RestClient.java @@ -232,8 +232,9 @@ private HttpRequestBuilder callApiBuilder(String apiName, Map pa httpRequestBuilder.method(RandomizedTest.randomFrom(supportedMethods)); } - //the http method is randomized (out of the available ones with the chosen api) - return httpRequestBuilder.path(RandomizedTest.randomFrom(restApi.getFinalPaths(pathParts))); + //the rest path to use is randomized out of the matching ones (if more than one) + RestPath restPath = RandomizedTest.randomFrom(restApi.getFinalPaths(pathParts)); + return httpRequestBuilder.pathParts(restPath.getPathParts()); } private RestApi restApi(String apiName) { diff --git a/core/src/test/java/org/elasticsearch/test/rest/client/RestPath.java b/core/src/test/java/org/elasticsearch/test/rest/client/RestPath.java new file mode 100644 index 0000000000000..f6e3ddabd5e8f --- /dev/null +++ b/core/src/test/java/org/elasticsearch/test/rest/client/RestPath.java @@ -0,0 +1,97 @@ +/* + * 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. + */ + +package org.elasticsearch.test.rest.client; + +import java.util.*; + +public class RestPath { + private final List parts; + private final List placeholders; + + public RestPath(List parts) { + List pathParts = new ArrayList<>(parts.size()); + for (String part : parts) { + pathParts.add(new PathPart(part, false)); + } + this.parts = pathParts; + this.placeholders = Collections.emptyList(); + } + + public RestPath(String path) { + String[] pathParts = path.split("/"); + List placeholders = new ArrayList<>(); + List parts = new ArrayList<>(); + for (String pathPart : pathParts) { + if (pathPart.length() > 0) { + if (pathPart.startsWith("{")) { + if (pathPart.indexOf('}') != pathPart.length() - 1) { + throw new IllegalArgumentException("more than one parameter found in the same path part: [" + pathPart + "]"); + } + String placeholder = pathPart.substring(1, pathPart.length() - 1); + parts.add(new PathPart(placeholder, true)); + placeholders.add(placeholder); + } else { + parts.add(new PathPart(pathPart, false)); + } + } + } + this.placeholders = placeholders; + this.parts = parts; + } + + public String[] getPathParts() { + String[] parts = new String[this.parts.size()]; + int i = 0; + for (PathPart part : this.parts) { + parts[i++] = part.pathPart; + } + return parts; + } + + public boolean matches(Set params) { + return placeholders.size() == params.size() && placeholders.containsAll(params); + } + + public RestPath replacePlaceholders(Map params) { + List finalPathParts = new ArrayList<>(parts.size()); + for (PathPart pathPart : parts) { + if (pathPart.isPlaceholder) { + String value = params.get(pathPart.pathPart); + if (value == null) { + throw new IllegalArgumentException("parameter [" + pathPart.pathPart + "] missing"); + } + finalPathParts.add(value); + } else { + finalPathParts.add(pathPart.pathPart); + } + } + return new RestPath(finalPathParts); + } + + private static class PathPart { + private final boolean isPlaceholder; + private final String pathPart; + + private PathPart(String pathPart, boolean isPlaceholder) { + this.isPlaceholder = isPlaceholder; + this.pathPart = pathPart; + } + } +} \ No newline at end of file diff --git a/core/src/test/java/org/elasticsearch/test/rest/client/http/HttpRequestBuilder.java b/core/src/test/java/org/elasticsearch/test/rest/client/http/HttpRequestBuilder.java index 7791cda78411c..b56c99d5f5037 100644 --- a/core/src/test/java/org/elasticsearch/test/rest/client/http/HttpRequestBuilder.java +++ b/core/src/test/java/org/elasticsearch/test/rest/client/http/HttpRequestBuilder.java @@ -86,14 +86,42 @@ public HttpRequestBuilder port(int port) { return this; } + /** + * Sets the path to send the request to. Url encoding needs to be applied by the caller. + * Use {@link #pathParts(String...)} instead if the path needs to be encoded, part by part. + */ public HttpRequestBuilder path(String path) { this.path = path; return this; } + /** + * Sets the path by providing the different parts (without slashes), which will be properly encoded. + */ + public HttpRequestBuilder pathParts(String... path) { + //encode rules for path and query string parameters are different. We use URI to encode the path, and URLEncoder for each query string parameter (see addParam). + //We need to encode each path part separately though, as each one might contain slashes that need to be escaped, which needs to be done manually. + if (path.length == 0) { + this.path = "/"; + return this; + } + StringBuilder finalPath = new StringBuilder(); + for (String pathPart : path) { + try { + finalPath.append('/'); + URI uri = new URI(null, null, null, -1, pathPart, null, null); + //manually escape any slash that each part may contain + finalPath.append(uri.getRawPath().replaceAll("/", "%2F")); + } catch(URISyntaxException e) { + throw new RuntimeException("unable to build uri", e); + } + } + this.path = finalPath.toString(); + return this; + } + public HttpRequestBuilder addParam(String name, String value) { try { - //manually url encode params, since URI does it only partially (e.g. '+' stays as is) this.params.put(name, URLEncoder.encode(value, "utf-8")); return this; } catch (UnsupportedEncodingException e) { @@ -181,19 +209,12 @@ private HttpUriRequest buildRequest() { } private URI buildUri() { - try { - //url encode rules for path and query params are different. We use URI to encode the path, but we manually encode each query param through URLEncoder. - URI uri = new URI(protocol, null, host, port, path, null, null); - //String concatenation FTW. If we use the nicer multi argument URI constructor query parameters will get only partially encoded - //(e.g. '+' will stay as is) hence when trying to properly encode params manually they will end up double encoded (+ becomes %252B instead of %2B). - StringBuilder uriBuilder = new StringBuilder(protocol).append("://").append(host).append(":").append(port).append(uri.getRawPath()); - if (params.size() > 0) { - uriBuilder.append("?").append(Joiner.on('&').withKeyValueSeparator("=").join(params)); - } - return URI.create(uriBuilder.toString()); - } catch(URISyntaxException e) { - throw new IllegalArgumentException("unable to build uri", e); + StringBuilder uriBuilder = new StringBuilder(protocol).append("://").append(host).append(":").append(port).append(path); + if (params.size() > 0) { + uriBuilder.append("?").append(Joiner.on('&').withKeyValueSeparator("=").join(params)); } + //using this constructor no url encoding happens, as we did everything upfront in addParam and pathPart methods + return URI.create(uriBuilder.toString()); } private HttpEntityEnclosingRequestBase addOptionalBody(HttpEntityEnclosingRequestBase requestBase) { diff --git a/core/src/test/java/org/elasticsearch/test/rest/spec/RestApi.java b/core/src/test/java/org/elasticsearch/test/rest/spec/RestApi.java index 0996df45b48fe..f22db1be59455 100644 --- a/core/src/test/java/org/elasticsearch/test/rest/spec/RestApi.java +++ b/core/src/test/java/org/elasticsearch/test/rest/spec/RestApi.java @@ -22,12 +22,11 @@ import com.google.common.collect.Maps; import org.apache.http.client.methods.HttpPost; import org.apache.http.client.methods.HttpPut; +import org.elasticsearch.test.rest.client.RestPath; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.regex.Matcher; -import java.util.regex.Pattern; /** * Represents an elasticsearch REST endpoint (api) @@ -41,7 +40,7 @@ public class RestApi { private List params = Lists.newArrayList(); private BODY body = BODY.NOT_SUPPORTED; - public static enum BODY { + public enum BODY { NOT_SUPPORTED, OPTIONAL, REQUIRED } @@ -131,28 +130,18 @@ public boolean isBodyRequired() { * Finds the best matching rest path given the current parameters and replaces * placeholders with their corresponding values received as arguments */ - public String[] getFinalPaths(Map pathParams) { - + public RestPath[] getFinalPaths(Map pathParams) { List matchingRestPaths = findMatchingRestPaths(pathParams.keySet()); if (matchingRestPaths == null || matchingRestPaths.isEmpty()) { throw new IllegalArgumentException("unable to find matching rest path for api [" + name + "] and path params " + pathParams); } - String[] paths = new String[matchingRestPaths.size()]; + RestPath[] restPaths = new RestPath[matchingRestPaths.size()]; for (int i = 0; i < matchingRestPaths.size(); i++) { RestPath restPath = matchingRestPaths.get(i); - String path = restPath.path; - for (Map.Entry paramEntry : restPath.parts.entrySet()) { - // replace path placeholders with actual values - String value = pathParams.get(paramEntry.getValue()); - if (value == null) { - throw new IllegalArgumentException("parameter [" + paramEntry.getValue() + "] missing"); - } - path = path.replace(paramEntry.getKey(), value); - } - paths[i] = path; + restPaths[i] = restPath.replacePlaceholders(pathParams); } - return paths; + return restPaths; } /** @@ -165,15 +154,11 @@ private List findMatchingRestPaths(Set restParams) { List matchingRestPaths = Lists.newArrayList(); RestPath[] restPaths = buildRestPaths(); - for (RestPath restPath : restPaths) { - if (restPath.parts.size() == restParams.size()) { - if (restPath.parts.values().containsAll(restParams)) { - matchingRestPaths.add(restPath); - } + if (restPath.matches(restParams)) { + matchingRestPaths.add(restPath); } } - return matchingRestPaths; } @@ -184,33 +169,4 @@ private RestPath[] buildRestPaths() { } return restPaths; } - - private static class RestPath { - private static final Pattern PLACEHOLDERS_PATTERN = Pattern.compile("(\\{(.*?)})"); - - final String path; - //contains param to replace (e.g. {index}) and param key to use for lookup in the current values map (e.g. index) - final Map parts; - - RestPath(String path) { - this.path = path; - this.parts = extractParts(path); - } - - private static Map extractParts(String input) { - Map parts = Maps.newHashMap(); - Matcher matcher = PLACEHOLDERS_PATTERN.matcher(input); - while (matcher.find()) { - //key is e.g. {index} - String key = input.substring(matcher.start(), matcher.end()); - if (matcher.groupCount() != 2) { - throw new IllegalArgumentException("no lookup key found for param [" + key + "]"); - } - //to be replaced with current value found with key e.g. index - String value = matcher.group(2); - parts.put(key, value); - } - return parts; - } - } }