Watcher: Do not encode URLs in HttpClient

When using a path like `"/<logstash-{now%2Fd}>/_search"` in the
http webhook. The already escaped slash (%2F) got escaped twice
and thus did not work any more.

The escaping happened when the code created an URI and was done
as part of that constructor. This is now switched to an URL (which
is used at the end anyway) which does not do the escaping, even though
this was required for the query string, which is now done when constructing.

Closes elastic/elasticsearch#1364

Original commit: elastic/x-pack-elasticsearch@861b6d2378

elasticsearch/x-pack/watcher/src/main/java/org/elasticsearch/watcher/support/http/HttpClient.java

@@ -7,7 +7,6 @@ package org.elasticsearch.watcher.support.http;
 
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.ElasticsearchTimeoutException;
-import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.SpecialPermission;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.component.AbstractLifecycleComponent;
@@ -31,9 +30,8 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.net.HttpURLConnection;
 import java.net.SocketTimeoutException;
-import java.net.URI;
-import java.net.URISyntaxException;
 import java.net.URL;
+import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -137,20 +135,18 @@ public class HttpClient extends AbstractLifecycleComponent<HttpClient> {
                 if (builder.length() != 0) {
                     builder.append('&');
                 }
-                builder.append(entry.getKey())
+                builder.append(URLEncoder.encode(entry.getKey(), "UTF-8"))
                         .append('=')
-                        .append(entry.getValue());
+                        .append(URLEncoder.encode(entry.getValue(), "UTF-8"));
             }
             queryString = builder.toString();
         }
 
-        URI uri;
-        try {
-            uri = new URI(request.scheme().scheme(), null, request.host(), request.port(), request.path(), queryString, null);
-        } catch (URISyntaxException e) {
-            throw ExceptionsHelper.convertToElastic(e);
+        String path = request.path;
+        if (Strings.hasLength(queryString)) {
+            path += "?" + queryString;
         }
-        URL url = uri.toURL();
+        URL url = new URL(request.scheme.scheme(), request.host, request.port, path);
 
         logger.debug("making [{}] request to [{}]", request.method().method(), url);
         logger.trace("sending [{}] as body of request", request.body());

elasticsearch/x-pack/watcher/src/test/java/org/elasticsearch/watcher/support/http/HttpClientTests.java

@@ -35,6 +35,7 @@ import java.security.UnrecoverableKeyException;
 import static java.util.Collections.singletonMap;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.core.Is.is;
@@ -116,7 +117,7 @@ public class HttpClientTests extends ESTestCase {
         assertThat(recordedRequest.getBody().readUtf8Line(), nullValue());
     }
 
-    public void testUrlEncoding() throws Exception{
+    public void testUrlEncodingWithQueryStrings() throws Exception{
         webServer.enqueue(new MockResponse().setResponseCode(200).setBody("body"));
         HttpRequest.Builder requestBuilder = HttpRequest.builder("localhost", webPort)
                 .method(HttpMethod.GET)
@@ -128,7 +129,7 @@ public class HttpClientTests extends ESTestCase {
         assertThat(response.body().toUtf8(), equalTo("body"));
 
         RecordedRequest recordedRequest = webServer.takeRequest();
-        assertThat(recordedRequest.getPath(), equalTo("/test?key=value%20123:123"));
+        assertThat(recordedRequest.getPath(), equalTo("/test?key=value+123%3A123"));
         assertThat(recordedRequest.getBody().readUtf8Line(), nullValue());
     }
 
@@ -358,6 +359,21 @@ public class HttpClientTests extends ESTestCase {
         }
     }
 
+    public void testThatUrlPathIsNotEncoded() throws Exception {
+        // %2F is a slash that needs to be encoded to not be misinterpreted as a path
+        String path = "/<logstash-{now%2Fd}>/_search";
+        webServer.enqueue(new MockResponse().setResponseCode(200).setBody("foo"));
+        HttpRequest request = HttpRequest.builder("localhost", webServer.getPort()).path(path).build();
+        httpClient.execute(request);
+
+        assertThat(webServer.getRequestCount(), is(1));
+
+        RecordedRequest recordedRequest = webServer.takeRequest();
+        // under no circumstances have a double encode of %2F => %25 (percent sign)
+        assertThat(recordedRequest.getPath(), not(containsString("%25")));
+        assertThat(recordedRequest.getPath(), equalTo(path));
+    }
+
     private MockWebServer startWebServer(int startPort, int endPort) throws IOException {
         for (int port = startPort; port < endPort; port++) {
             try {