HLRC: Fix '+' Not Correctly Encoded in GET Req. (#33164) (#44324)

* HLRC: Fix '+' Not Correctly Encoded in GET Req.

* Encode `+` correctly as `%2B` in URL paths
* Keep encoding `+` as space in URL parameters
* Closes #33077

client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java

@@ -1020,6 +1020,24 @@ public class CrudIT extends ESRestHighLevelClientTestCase {
         }
     }
 
+    public void testGetIdWithPlusSign() throws Exception {
+        String id = "id+id";
+        {
+            IndexRequest indexRequest = new IndexRequest("index").id(id);
+            indexRequest.source("field", "value");
+            IndexResponse indexResponse = highLevelClient().index(indexRequest, RequestOptions.DEFAULT);
+            assertEquals("index", indexResponse.getIndex());
+            assertEquals(id, indexResponse.getId());
+        }
+        {
+            GetRequest getRequest = new GetRequest("index").id(id);
+            GetResponse getResponse = highLevelClient().get(getRequest, RequestOptions.DEFAULT);
+            assertTrue(getResponse.isExists());
+            assertEquals("index", getResponse.getIndex());
+            assertEquals(id, getResponse.getId());
+        }
+    }
+
     // Not entirely sure if _termvectors belongs to CRUD, and in the absence of a better place, will have it here
     public void testTermvectors() throws IOException {
         final String sourceIndex = "index1";

docs/reference/migration/migrate_7_4.asciidoc

@@ -56,3 +56,15 @@ breaking change was made necessary by
 https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story/[AWS's
 announcement] that the path-style access pattern is deprecated and will be
 unsupported on buckets created after September 30th 2020.
+
+[float]
+[[breaking_80_http_changes]]
+=== HTTP changes
+
+[float]
+==== Changes to Encoding Plus Signs in URLs
+
+Starting in version 7.4, a `+` in a URL will be encoded as `%2B` by all REST API functionality. Prior versions handled a `+` as a single space.
+If your application requires handling `+` as a single space you can return to the old behaviour by setting the system property
+`es.rest.url_plus_as_space` to `true`. Note that this behaviour is deprecated and setting this system property to `true` will cease
+to be supported in version 8.

server/src/main/java/org/elasticsearch/rest/RestUtils.java

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.rest;
 
+import org.elasticsearch.common.Booleans;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.path.PathTrie;
 
@@ -30,6 +31,11 @@ import java.util.regex.Pattern;
 
 public class RestUtils {
 
+    /**
+     * Sets whether we decode a '+' in an url as a space or not.
+     */
+    private static final boolean DECODE_PLUS_AS_SPACE = Booleans.parseBoolean(System.getProperty("es.rest.url_plus_as_space", "false"));
+
     public static final PathTrie.Decoder REST_DECODER = new PathTrie.Decoder() {
         @Override
         public String decode(String value) {
@@ -55,7 +61,7 @@ public class RestUtils {
             c = s.charAt(i);
             if (c == '=' && name == null) {
                 if (pos != i) {
-                    name = decodeComponent(s.substring(pos, i));
+                    name = decodeQueryStringParam(s.substring(pos, i));
                 }
                 pos = i + 1;
             } else if (c == '&' || c == ';') {
@@ -63,9 +69,9 @@ public class RestUtils {
                     // We haven't seen an `=' so far but moved forward.
                     // Must be a param of the form '&a&' so add it with
                     // an empty value.
-                    addParam(params, decodeComponent(s.substring(pos, i)), "");
+                    addParam(params, decodeQueryStringParam(s.substring(pos, i)), "");
                 } else if (name != null) {
-                    addParam(params, name, decodeComponent(s.substring(pos, i)));
+                    addParam(params, name, decodeQueryStringParam(s.substring(pos, i)));
                     name = null;
                 }
                 pos = i + 1;
@@ -74,15 +80,19 @@ public class RestUtils {
 
         if (pos != i) {  // Are there characters we haven't dealt with?
             if (name == null) {     // Yes and we haven't seen any `='.
-                addParam(params, decodeComponent(s.substring(pos, i)), "");
+                addParam(params, decodeQueryStringParam(s.substring(pos, i)), "");
             } else {                // Yes and this must be the last value.
-                addParam(params, name, decodeComponent(s.substring(pos, i)));
+                addParam(params, name, decodeQueryStringParam(s.substring(pos, i)));
             }
         } else if (name != null) {  // Have we seen a name without value?
             addParam(params, name, "");
         }
     }
 
+    private static String decodeQueryStringParam(final String s) {
+        return decodeComponent(s, StandardCharsets.UTF_8, true);
+    }
+
     private static void addParam(Map<String, String> params, String name, String value) {
         params.put(name, value);
     }
@@ -90,7 +100,7 @@ public class RestUtils {
     /**
      * Decodes a bit of an URL encoded by a browser.
      * <p>
-     * This is equivalent to calling {@link #decodeComponent(String, Charset)}
+     * This is equivalent to calling {@link #decodeComponent(String, Charset, boolean)}
      * with the UTF-8 charset (recommended to comply with RFC 3986, Section 2).
      *
      * @param s The string to decode (can be empty).
@@ -100,7 +110,7 @@ public class RestUtils {
      *                                  escape sequence.
      */
     public static String decodeComponent(final String s) {
-        return decodeComponent(s, StandardCharsets.UTF_8);
+        return decodeComponent(s, StandardCharsets.UTF_8, DECODE_PLUS_AS_SPACE);
     }
 
     /**
@@ -122,49 +132,47 @@ public class RestUtils {
      * @param s           The string to decode (can be empty).
      * @param charset     The charset to use to decode the string (should really
      *                    be {@link StandardCharsets#UTF_8}.
+     * @param plusAsSpace Whether to decode a {@code '+'} to a single space {@code ' '}
      * @return The decoded string, or {@code s} if there's nothing to decode.
      *         If the string to decode is {@code null}, returns an empty string.
      * @throws IllegalArgumentException if the string contains a malformed
      *                                  escape sequence.
      */
-    public static String decodeComponent(final String s, final Charset charset) {
+    private static String decodeComponent(final String s, final Charset charset, boolean plusAsSpace) {
         if (s == null) {
             return "";
         }
         final int size = s.length();
-        if (!decodingNeeded(s, size)) {
+        if (!decodingNeeded(s, size, plusAsSpace)) {
             return s;
         }
         final byte[] buf = new byte[size];
-        int pos = decode(s, size, buf);
+        int pos = decode(s, size, buf, plusAsSpace);
         return new String(buf, 0, pos, charset);
     }
 
-    @SuppressWarnings("fallthrough")
+    private static boolean decodingNeeded(String s, int size, boolean plusAsSpace) {
-    private static boolean decodingNeeded(String s, int size) {
         boolean decodingNeeded = false;
         for (int i = 0; i < size; i++) {
             final char c = s.charAt(i);
-            switch (c) {
+            if (c == '%') {
-                case '%':
                 i++;  // We can skip at least one char, e.g. `%%'.
-                    // Fall through.
-                case '+':
                 decodingNeeded = true;
-                    break;
+            } else if (plusAsSpace && c == '+') {
+                decodingNeeded = true;
             }
         }
         return decodingNeeded;
     }
 
     @SuppressWarnings("fallthrough")
-    private static int decode(String s, int size, byte[] buf) {
+    private static int decode(String s, int size, byte[] buf, boolean plusAsSpace) {
         int pos = 0;  // position in `buf'.
         for (int i = 0; i < size; i++) {
             char c = s.charAt(i);
             switch (c) {
                 case '+':
-                    buf[pos++] = ' ';  // "+" -> " "
+                    buf[pos++] = (byte) (plusAsSpace ? ' ' : '+');  // "+" -> " "
                     break;
                 case '%':
                     if (i == size - 1) {

x-pack/plugin/ml/qa/basic-multi-node/src/test/java/org/elasticsearch/xpack/ml/integration/MlBasicMultiNodeIT.java

@@ -14,9 +14,9 @@ import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.test.rest.ESRestTestCase;
 import org.elasticsearch.xpack.ml.MachineLearning;
+import org.yaml.snakeyaml.util.UriEncoder;
 
 import java.io.IOException;
-import java.net.URLEncoder;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -303,7 +303,7 @@ public class MlBasicMultiNodeIT extends ESRestTestCase {
         }
         xContentBuilder.endObject();
 
-        Request request = new Request("PUT", MachineLearning.BASE_PATH + "anomaly_detectors/" + URLEncoder.encode(jobId, "UTF-8"));
+        Request request = new Request("PUT", MachineLearning.BASE_PATH + "anomaly_detectors/" + UriEncoder.encode(jobId));
         request.setJsonEntity(Strings.toString(xContentBuilder));
         return client().performRequest(request);
     }