Allow httpclient to follow requests to absolute urls

SeleniumHQ · Feb 13, 2018 · deb865f · deb865f
1 parent 47118bd
commit deb865f
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 11 deletions.
diff --git a/java/client/src/org/openqa/selenium/remote/internal/ApacheHttpClient.java b/java/client/src/org/openqa/selenium/remote/internal/ApacheHttpClient.java
@@ -44,24 +44,30 @@
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URL;
+import java.util.AbstractMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
 import java.util.StringTokenizer;
 
 public class ApacheHttpClient implements org.openqa.selenium.remote.http.HttpClient {
 
   private static final int MAX_REDIRECTS = 10;
+  private static final int MAX_CACHED_HOSTS = 50;
 
   private final URL url;
-  private final HttpHost targetHost;
   private final HttpClient client;
+  private final Map<Map.Entry<String, Integer>, HttpHost> cachedHosts;
 
   public ApacheHttpClient(HttpClient client, URL url) {
     this.client = checkNotNull(client, "null HttpClient");
     this.url = checkNotNull(url, "null URL");
 
-    // Some machines claim "localhost.localdomain" is the same as "localhost".
-    // This assumption is not always true.
-    String host = url.getHost().replace(".localdomain", "");
-    this.targetHost = new HttpHost(host, url.getPort(), url.getProtocol());
+    this.cachedHosts = new LinkedHashMap<Map.Entry<String, Integer>, HttpHost>(200) {
+      @Override
+      protected boolean removeEldestEntry(Map.Entry eldest) {
+        return size() > MAX_CACHED_HOSTS;
+      }
+    };
   }
 
   @Override
@@ -148,7 +154,7 @@ private static HttpUriRequest createHttpUriRequest(HttpMethod method, URL url)
   private org.apache.http.HttpResponse fallBackExecute(
       HttpContext context, HttpUriRequest httpMethod) throws IOException {
     try {
-      return client.execute(targetHost, httpMethod, context);
+      return client.execute(getHost(httpMethod), httpMethod, context);
     } catch (BindException | NoHttpResponseException e) {
       // If we get this, there's a chance we've used all the local ephemeral sockets
       // Sleep for a bit to let the OS reclaim them, then try the request again.
@@ -158,7 +164,7 @@ private org.apache.http.HttpResponse fallBackExecute(
         throw new RuntimeException(ie);
       }
     }
-    return client.execute(targetHost, httpMethod, context);
+    return client.execute(getHost(httpMethod), httpMethod, context);
   }
 
   private org.apache.http.HttpResponse followRedirects(
@@ -190,13 +196,45 @@ private org.apache.http.HttpResponse followRedirects(
 
       HttpGet get = new HttpGet(uri);
       get.setHeader("Accept", "application/json; charset=utf-8");
-      org.apache.http.HttpResponse newResponse = client.execute(targetHost, get, context);
+      org.apache.http.HttpResponse newResponse = client.execute(getHost(get), get, context);
       return followRedirects(client, context, newResponse, redirectCount + 1);
     } catch (URISyntaxException | IOException e) {
       throw new WebDriverException(e);
     }
   }
 
+  private HttpHost getHost(HttpUriRequest method) {
+    // Some machines claim "localhost.localdomain" is the same as "localhost".
+    // This assumption is not always true.
+    String host = method.getURI().getHost().replace(".localdomain", "");
+    int port = method.getURI().getPort();
+    if (port == -1) {
+      switch (method.getURI().getScheme()) {
+        case "http":
+          port = 80;
+          break;
+
+        case "https":
+          port = 443;
+          break;
+
+        default:
+          // Do nothing
+          break;
+      }
+    }
+
+    synchronized (cachedHosts) {
+      Map.Entry<String, Integer> entry =
+          new AbstractMap.SimpleImmutableEntry<>(host, port);
+      HttpHost
+          httpHost =
+          cachedHosts.computeIfAbsent(entry, e -> new HttpHost(e.getKey(), e.getValue()));
+      System.out.println("cachedHosts = " + cachedHosts);
+      return httpHost;
+    }
+  }
+
   private URI buildUri(HttpContext context, String location) throws URISyntaxException {
     URI uri;
     uri = new URI(location);

diff --git a/java/client/src/org/openqa/selenium/remote/internal/HttpUrlBuilder.java b/java/client/src/org/openqa/selenium/remote/internal/HttpUrlBuilder.java
@@ -63,7 +63,13 @@ static URL toUrl(URL base, HttpRequest request) throws MalformedURLException {
         .collect(Collectors.toList());
     parameters.appendTo(queryString, allParams);
 
-    String baseUrl = base.toExternalForm().replaceAll("/$", "") + request.getUri();
+    String baseUrl;
+    if (request.getUri().startsWith("http://") || request.getUri().startsWith("https://")) {
+      baseUrl = request.getUri();
+    } else {
+      baseUrl = base.toExternalForm().replaceAll("/$", "") + request.getUri();
+    }
+
     if (!queryString.toString().isEmpty()) {
       baseUrl += "?" + queryString.toString();
     }

diff --git a/java/client/src/org/openqa/selenium/remote/internal/OkHttpClient.java b/java/client/src/org/openqa/selenium/remote/internal/OkHttpClient.java
@@ -64,7 +64,13 @@ public HttpResponse execute(HttpRequest request, boolean followRedirects) throws
 
     HttpUrl.Builder url;
     try {
-      String rawUrl = baseUrl.toExternalForm().replaceAll("/$", "") + request.getUri();
+      String rawUrl;
+      if (request.getUri().startsWith("http:") || request.getUri().startsWith("https:")) {
+        rawUrl = request.getUri();
+      } else {
+        rawUrl = baseUrl.toExternalForm().replaceAll("/$", "") + request.getUri();
+      }
+
       url = HttpUrl.parse(rawUrl).newBuilder();
     } catch (NullPointerException e) {
       throw new IOException("Unable to parse URL: " + baseUrl.toString() + request.getUri());

diff --git a/java/client/test/org/openqa/selenium/remote/internal/HttpClientTestBase.java b/java/client/test/org/openqa/selenium/remote/internal/HttpClientTestBase.java
@@ -39,7 +39,10 @@
 import org.seleniumhq.jetty9.servlet.ServletHolder;
 
 import java.io.IOException;
+import java.io.PrintWriter;
 import java.io.Writer;
+import java.net.URI;
+import java.net.URL;
 import java.util.Map;
 import java.util.stream.Stream;
 
@@ -83,7 +86,7 @@ public void responseShouldKeepMultipleHeadersSeparate() throws Exception {
   }
 
   @Test
-  public void shouldAddUrlParameters() throws Exception {
+  public void shouldAddUrlParameters() {
     HttpRequest request = new HttpRequest(GET, "/query");
     String value = request.getQueryParameter("cheese");
     assertNull(value);
@@ -129,6 +132,42 @@ public void canAddMoreThanOneQueryParameter() throws Exception {
     assertEquals(ImmutableList.of("peas"), values.get("vegetable"));
   }
 
+  @Test
+  public void shouldAllowUrlsWithSchemesToBeUsed() throws Exception {
+    Server server = new Server(PortProber.findFreePort());
+    ServletContextHandler handler = new ServletContextHandler();
+    handler.setContextPath("");
+
+    class Canned extends HttpServlet {
+      @Override
+      protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
+        try (PrintWriter writer = resp.getWriter()) {
+          writer.append("Hello, World!");
+        }
+      }
+    }
+    ServletHolder holder = new ServletHolder(new Canned());
+    handler.addServlet(holder, "/*");
+    server.setHandler(handler);
+
+    server.start();
+    try {
+      // This is a terrible choice of URL
+      HttpClient client = createFactory().createClient(new URL("http://example.com"));
+
+      URI uri = server.getURI();
+      HttpRequest request = new HttpRequest(
+          GET,
+          String.format("http://%s:%s/hello", uri.getHost(), uri.getPort()));
+
+      HttpResponse response = client.execute(request);
+
+      assertEquals("Hello, World!", response.getContentString());
+    } finally {
+      server.stop();
+    }
+  }
+
   private HttpResponse getResponseWithHeaders(final Multimap<String, String> headers)
       throws Exception {
     Server server = new Server(PortProber.findFreePort());