From 255c4e42fd1ce0575d0b331950d3faeda875725a Mon Sep 17 00:00:00 2001
From: Tim McCormack <tmccormack@brightcove.com>
Date: Tue, 11 Mar 2014 12:38:07 -0400
Subject: [PATCH] Remove pitfall code in URI reconstruction; fix userinfo
 handling.

* newURI was always getting overwritten under normal circumstances
  (any time there was a / or ? or similar) so isURIEncoded is not
  necessary. Use of getQuery is dangerous, but the value was being
  thrown away. This commit helps ensure it will not be used in the
  future.
* getUserInfo is replaced with getRawUserInfo to prevent premature
  decoding of separators such as ":". This should be the only behavioral
  change.
---
 .../netflix/client/LoadBalancerContext.java   | 45 +++++++------------
 .../client/LoadBalancerContextTest.java       | 17 +++++++
 2 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/ribbon-core/src/main/java/com/netflix/client/LoadBalancerContext.java b/ribbon-core/src/main/java/com/netflix/client/LoadBalancerContext.java
index ac77057..200371a 100644
--- a/ribbon-core/src/main/java/com/netflix/client/LoadBalancerContext.java
+++ b/ribbon-core/src/main/java/com/netflix/client/LoadBalancerContext.java
@@ -394,7 +394,6 @@ public abstract class LoadBalancerContext<T extends ClientRequest, S extends IRe
      */
     @SuppressWarnings("unchecked")
     protected T computeFinalUriWithLoadBalancer(T original) throws ClientException{
-        URI newURI;
         URI theUrl = original.getUri();
 
         if (theUrl == null){
@@ -514,42 +513,28 @@ public abstract class LoadBalancerContext<T extends ClientRequest, S extends IRe
         // just verify that at this point we have a full URL
 
         try {
-            String urlPath = "";
-            if (theUrl.getRawPath() != null && theUrl.getRawPath().startsWith("/")) {
-                urlPath = theUrl.getRawPath();
-            } else {
-                urlPath = "/" + theUrl.getRawPath();
+            StringBuilder sb = new StringBuilder();
+            sb.append(scheme).append("://");
+            if (!Strings.isNullOrEmpty(theUrl.getRawUserInfo())) {
+                sb.append(theUrl.getRawUserInfo()).append("@");
             }
-            
-            newURI = new URI(scheme, theUrl.getUserInfo(), host, port, urlPath, theUrl.getQuery(), theUrl.getFragment());
-            if (isURIEncoded(theUrl)) {
-                StringBuilder sb = new StringBuilder();
-                sb.append(newURI.getScheme())
-                  .append("://")
-                  .append(newURI.getRawAuthority())
-                  .append(theUrl.getRawPath());
-                if (!Strings.isNullOrEmpty(theUrl.getRawQuery())) {
-                    sb.append("?").append(theUrl.getRawQuery());
-                }
-                if (!Strings.isNullOrEmpty(theUrl.getRawFragment())) {
-                    sb.append("#").append(theUrl.getRawFragment());
-                }
-                newURI = new URI(sb.toString());
+            sb.append(host);
+            if (port >= 0) {
+                sb.append(":").append(port);
+            }
+            sb.append(theUrl.getRawPath());
+            if (!Strings.isNullOrEmpty(theUrl.getRawQuery())) {
+                sb.append("?").append(theUrl.getRawQuery());
+            }
+            if (!Strings.isNullOrEmpty(theUrl.getRawFragment())) {
+                sb.append("#").append(theUrl.getRawFragment());
             }
+            URI newURI = new URI(sb.toString());
             return (T) original.replaceUri(newURI);            
         } catch (URISyntaxException e) {
             throw new ClientException(ClientException.ErrorType.GENERAL, e.getMessage());
         }
     }
-
-    private boolean isURIEncoded(URI uri) {
-        String original = uri.toString();
-        try {
-            return !URLEncoder.encode(original, "UTF-8").equals(original);
-        } catch (Exception e) {
-            return false;
-        }
-    }
     
     protected boolean isRetriable(T request) {
         if (request.isRetriable()) {
diff --git a/ribbon-core/src/test/java/com/netflix/client/LoadBalancerContextTest.java b/ribbon-core/src/test/java/com/netflix/client/LoadBalancerContextTest.java
index 8d9a70c..3ed6fff 100644
--- a/ribbon-core/src/test/java/com/netflix/client/LoadBalancerContextTest.java
+++ b/ribbon-core/src/test/java/com/netflix/client/LoadBalancerContextTest.java
@@ -61,6 +61,23 @@ public class LoadBalancerContextTest {
         assertEquals(uri, newRequest.getUri().toString());
     }
     
+    @Test
+    public void testPreservesUserInfo() throws ClientException {
+        // %3A == ":" -- ensure user info is not decoded
+        String uri = "http://us%3Aer:pass@localhost:8080?foo=bar";
+        HttpRequest request = HttpRequest.newBuilder().uri(uri).build();
+        HttpRequest newRequest = context.computeFinalUriWithLoadBalancer(request);
+        assertEquals(uri, newRequest.getUri().toString());
+    }
+    
+    @Test
+    public void testQueryWithoutPath() throws ClientException {
+        String uri = "?foo=bar";
+        HttpRequest request = HttpRequest.newBuilder().uri(uri).build();
+        HttpRequest newRequest = context.computeFinalUriWithLoadBalancer(request);
+        assertEquals("http://www.example.com:8080?foo=bar", newRequest.getUri().toString());
+    }
+    
     @Test
     public void testEncodedPathAndHostChange() throws ClientException {
         String uri = "/abc%2Fxyz";
-- 
GitLab