Restore support for escaped '/' as part of document id

With #13691 we introduced some custom logic to make sure that date math expressions like <logstash-{now/D}> 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

core/src/main/java/org/elasticsearch/common/path/PathTrie.java

@@ -21,9 +21,7 @@ package org.elasticsearch.common.path;
 
 import org.elasticsearch.common.Strings;
 
-import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
 
 import static java.util.Collections.emptyMap;
@@ -34,26 +32,15 @@ import static java.util.Collections.unmodifiableMap;
  */
 public class PathTrie<T> {
 
-    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);
     }
@@ -198,7 +185,7 @@ public class PathTrie<T> {
 
         private void put(Map<String, String> params, TrieNode node, String value) {
             if (params != null && node.isNamedWildcard()) {
-                params.put(node.namedWildcard(), value);
+                params.put(node.namedWildcard(), decoder.decode(value));
             }
         }
 
@@ -230,7 +217,7 @@ public class PathTrie<T> {
         if (path.length() == 0) {
             return rootValue;
         }
-        String[] strings = splitPath(decoder.decode(path));
+        String[] strings = Strings.splitStringToArray(path, separator);
         if (strings.length == 0) {
             return rootValue;
         }
@@ -241,50 +228,4 @@ public class PathTrie<T> {
         }
         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<String> 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()]);
-    }
 }

core/src/test/java/org/elasticsearch/common/path/PathTrieTests.java

@@ -19,12 +19,12 @@
 
 package org.elasticsearch.common.path;
 
+import org.elasticsearch.rest.support.RestUtils;
 import org.elasticsearch.test.ESTestCase;
 
 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;
 
@@ -33,8 +33,15 @@ import static org.hamcrest.Matchers.nullValue;
  */
 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<String> trie = new PathTrie<>();
+        PathTrie<String> trie = new PathTrie<>(NO_DECODER);
         trie.insert("/a/b/c", "walla");
         trie.insert("a/d/g", "kuku");
         trie.insert("x/b/c", "lala");
@@ -61,13 +68,13 @@ public class PathTrieTests extends ESTestCase {
     }
 
     public void testEmptyPath() {
-        PathTrie<String> trie = new PathTrie<>();
+        PathTrie<String> trie = new PathTrie<>(NO_DECODER);
         trie.insert("/", "walla");
         assertThat(trie.retrieve(""), equalTo("walla"));
     }
 
     public void testDifferentNamesOnDifferentPath() {
-        PathTrie<String> trie = new PathTrie<>();
+        PathTrie<String> trie = new PathTrie<>(NO_DECODER);
         trie.insert("/a/{type}", "test1");
         trie.insert("/b/{name}", "test2");
 
@@ -81,7 +88,7 @@ public class PathTrieTests extends ESTestCase {
     }
 
     public void testSameNameOnDifferentPath() {
-        PathTrie<String> trie = new PathTrie<>();
+        PathTrie<String> trie = new PathTrie<>(NO_DECODER);
         trie.insert("/a/c/{name}", "test1");
         trie.insert("/b/{name}", "test2");
 
@@ -95,7 +102,7 @@ public class PathTrieTests extends ESTestCase {
     }
 
     public void testPreferNonWildcardExecution() {
-        PathTrie<String> trie = new PathTrie<>();
+        PathTrie<String> trie = new PathTrie<>(NO_DECODER);
         trie.insert("{test}", "test1");
         trie.insert("b", "test2");
         trie.insert("{test}/a", "test3");
@@ -111,7 +118,7 @@ public class PathTrieTests extends ESTestCase {
     }
 
     public void testSamePathConcreteResolution() {
-        PathTrie<String> trie = new PathTrie<>();
+        PathTrie<String> trie = new PathTrie<>(NO_DECODER);
         trie.insert("{x}/{y}/{z}", "test1");
         trie.insert("{x}/_y/{k}", "test2");
 
@@ -127,7 +134,7 @@ public class PathTrieTests extends ESTestCase {
     }
 
     public void testNamedWildcardAndLookupWithWildcard() {
-        PathTrie<String> trie = new PathTrie<>();
+        PathTrie<String> trie = new PathTrie<>(NO_DECODER);
         trie.insert("x/{test}", "test1");
         trie.insert("{test}/a", "test2");
         trie.insert("/{test}", "test3");
@@ -155,24 +162,20 @@ public class PathTrieTests extends ESTestCase {
         assertThat(params.get("test"), equalTo("*"));
     }
 
-    public void testSplitPath() {
-        PathTrie<String> 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/<c/d>"), arrayContaining("a", "b", "<c/d>"));
-        assertThat(trie.splitPath("/a/b/<c/d>/d"), arrayContaining("a", "b", "<c/d>", "d"));
-
-        assertThat(trie.splitPath("/<logstash-{now}>/_search"), arrayContaining("<logstash-{now}>", "_search"));
-        assertThat(trie.splitPath("/<logstash-{now/d}>/_search"), arrayContaining("<logstash-{now/d}>", "_search"));
-        assertThat(trie.splitPath("/<logstash-{now/M{YYYY.MM}}>/_search"), arrayContaining("<logstash-{now/M{YYYY.MM}}>", "_search"));
-        assertThat(trie.splitPath("/<logstash-{now/M{YYYY.MM}}>/_search"), arrayContaining("<logstash-{now/M{YYYY.MM}}>", "_search"));
-        assertThat(trie.splitPath("/<logstash-{now/M{YYYY.MM|UTC}}>/log/_search"), arrayContaining("<logstash-{now/M{YYYY.MM|UTC}}>", "log", "_search"));
-
-        assertThat(trie.splitPath("/<logstash-{now/M}>,<logstash-{now/M-1M}>/_search"), arrayContaining("<logstash-{now/M}>,<logstash-{now/M-1M}>", "_search"));
-        assertThat(trie.splitPath("/<logstash-{now/M}>,<logstash-{now/M-1M}>/_search"), arrayContaining("<logstash-{now/M}>,<logstash-{now/M-1M}>", "_search"));
-        assertThat(trie.splitPath("/<logstash-{now/M{YYYY.MM}}>,<logstash-{now/M-1M{YYYY.MM}}>/_search"), arrayContaining("<logstash-{now/M{YYYY.MM}}>,<logstash-{now/M-1M{YYYY.MM}}>", "_search"));
-        assertThat(trie.splitPath("/<logstash-{now/M{YYYY.MM|UTC}}>,<logstash-{now/M-1M{YYYY.MM|UTC}}>/_search"), arrayContaining("<logstash-{now/M{YYYY.MM|UTC}}>,<logstash-{now/M-1M{YYYY.MM|UTC}}>", "_search"));
+    //https://github.com/elastic/elasticsearch/issues/14177
+    //https://github.com/elastic/elasticsearch/issues/13665
+    public void testEscapedSlashWithinUrl() {
+        PathTrie<String> pathTrie = new PathTrie<>(RestUtils.REST_DECODER);
+        pathTrie.insert("/{index}/{type}/{id}", "test");
+        HashMap<String, String> 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("/<logstash-{now%2Fd}>/type/id", params), equalTo("test"));
+        assertThat(params.get("index"), equalTo("<logstash-{now/d}>"));
+        assertThat(params.get("type"), equalTo("type"));
+        assertThat(params.get("id"), equalTo("id"));
     }
-
 }

core/src/test/java/org/elasticsearch/plugins/SitePluginIT.java

@@ -98,7 +98,7 @@ public class SitePluginIT extends ESIntegTestCase {
         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();

core/src/test/java/org/elasticsearch/test/rest/client/RestClient.java

@@ -230,8 +230,9 @@ public class RestClient implements Closeable {
             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) {

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<PathPart> parts;
+    private final List<String> placeholders;
+
+    public RestPath(List<String> parts) {
+        List<PathPart> 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<String> placeholders = new ArrayList<>();
+        List<PathPart> 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<String> params) {
+        return placeholders.size() == params.size() && placeholders.containsAll(params);
+    }
+
+    public RestPath replacePlaceholders(Map<String,String> params) {
+        List<String> 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;
+        }
+    }
+}

core/src/test/java/org/elasticsearch/test/rest/client/http/HttpRequestBuilder.java

@@ -86,14 +86,42 @@ public class HttpRequestBuilder {
         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 @@ public class HttpRequestBuilder {
     }
 
     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(params.entrySet().stream().map(e -> e.getKey() + "=" + e.getValue()).collect(Collectors.joining("&")));
-            }
-            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(params.entrySet().stream().map(e -> e.getKey() + "=" + e.getValue()).collect(Collectors.joining("&")));
         }
+        //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) {

core/src/test/java/org/elasticsearch/test/rest/spec/RestApi.java

@@ -20,14 +20,12 @@ package org.elasticsearch.test.rest.spec;
 
 import org.apache.http.client.methods.HttpPost;
 import org.apache.http.client.methods.HttpPut;
+import org.elasticsearch.test.rest.client.RestPath;
 
 import java.util.ArrayList;
-import java.util.HashMap;
 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 +39,7 @@ public class RestApi {
     private List<String> params = new ArrayList<>();
     private BODY body = BODY.NOT_SUPPORTED;
 
-    public static enum BODY {
+    public enum BODY {
         NOT_SUPPORTED, OPTIONAL, REQUIRED
     }
 
@@ -131,28 +129,18 @@ public class RestApi {
      * Finds the best matching rest path given the current parameters and replaces
      * placeholders with their corresponding values received as arguments
      */
-    public String[] getFinalPaths(Map<String, String> pathParams) {
-
+    public RestPath[] getFinalPaths(Map<String, String> pathParams) {
         List<RestPath> 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<String, String> 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 +153,11 @@ public class RestApi {
 
         List<RestPath> matchingRestPaths = new ArrayList<>();
         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 +168,4 @@ public class RestApi {
         }
         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<String, String> parts;
-
-        RestPath(String path) {
-            this.path = path;
-            this.parts = extractParts(path);
-        }
-
-        private static Map<String,String> extractParts(String input) {
-            Map<String, String> parts = new HashMap<>();
-            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;
-        }
-    }
 }