Support '+' in encoded query string when rebuilding the URI to https

RibbonUtils now replace the encoded space character '+' in the query string in favor of the equivalent '%20' when rewriting the URI in https. This is due to UriComponentsBuilder verifying the allowed characters in the encoded URI when building to the URI java.net class. The '+' is considered illegal but is widely used in the field. (#1367)

spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java

@@ -1,18 +1,19 @@
 package org.springframework.cloud.netflix.ribbon;
 
+import java.net.URI;
+
+import org.springframework.web.util.UriComponentsBuilder;
+
 import com.netflix.client.config.CommonClientConfigKey;
 import com.netflix.client.config.IClientConfig;
-import com.netflix.client.config.IClientConfigKey;
 import com.netflix.config.ConfigurationManager;
 import com.netflix.config.DynamicPropertyFactory;
 import com.netflix.config.DynamicStringProperty;
 import com.netflix.loadbalancer.Server;
-import org.springframework.web.util.UriComponentsBuilder;
-
-import java.net.URI;
 
 /**
  * @author Spencer Gibb
+ * @author Jacques-Etienne Beaudet
  */
 public class RibbonUtils {
 
@@ -67,13 +68,19 @@ public class RibbonUtils {
 	 * @param server
 	 * @return
 	 */
-	public static URI updateToHttpsIfNeeded(URI uri, IClientConfig config, ServerIntrospector serverIntrospector, Server server) {
+	public static URI updateToHttpsIfNeeded(URI uri, IClientConfig config, ServerIntrospector serverIntrospector,
+			Server server) {
 		String scheme = uri.getScheme();
 		if (!"https".equals(scheme) && isSecure(config, serverIntrospector, server)) {
-			return UriComponentsBuilder.fromUri(uri).scheme("https").build(true)
-					.toUri();
+			UriComponentsBuilder uriComponentsBuilder = UriComponentsBuilder.fromUri(uri).scheme("https");
+			if (uri.getRawQuery() != null) {
+				// When building the URI, UriComponentsBuilder verify the allowed characters and does not 
+				// support the '+' so we replace it for its equivalent '%20'.
+				// See issue https://jira.spring.io/browse/SPR-10172
+				uriComponentsBuilder.replaceQuery(uri.getRawQuery().replace("+", "%20"));
+			}
+			return uriComponentsBuilder.build(true).toUri();
 		}
 		return uri;
 	}
-
 }

spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfigurationTests.java

@@ -127,6 +127,17 @@ public class RibbonClientConfigurationTests {
 		}
 	}
 
+	@Test
+	public void testPlusInQueryStringGetsRewrittenWhenServerIsSecure() throws Exception {
+		Server server = new Server("foo", 7777);
+		when(this.inspector.isSecure(server)).thenReturn(true);
+
+		for (AbstractLoadBalancerAwareClient client : clients()) {
+			URI uri = client.reconstructURIWithServer(server, new URI("http://foo/%20bar?hello=1+2"));
+			assertThat(uri, is(new URI("https://foo:7777/%20bar?hello=1%202")));
+		}
+	}
+
 	private List<AbstractLoadBalancerAwareClient> clients() {
 		ArrayList<AbstractLoadBalancerAwareClient> clients = new ArrayList<>();
 		clients.add(new OverrideRestClient(this.config, this.inspector));

spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java

@@ -17,20 +17,23 @@
 
 package org.springframework.cloud.netflix.ribbon;
 
-import com.netflix.client.config.CommonClientConfigKey;
+import static org.hamcrest.Matchers.is;
+import static org.springframework.cloud.netflix.ribbon.RibbonUtils.*;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.Map;
+
 import org.junit.Assert;
 import org.junit.Test;
 
+import com.netflix.client.config.CommonClientConfigKey;
 import com.netflix.client.config.DefaultClientConfigImpl;
 import com.netflix.loadbalancer.Server;
 
-import java.util.Map;
-
-import static org.hamcrest.Matchers.is;
-import static org.springframework.cloud.netflix.ribbon.RibbonUtils.isSecure;
-
 /**
  * @author Spencer Gibb
+ * @author Jacques-Etienne Beaudet
  */
 public class RibbonUtilsTests {
 
@@ -77,6 +80,35 @@ public class RibbonUtilsTests {
 		Assert.assertThat("isSecure was wrong", secure, is(false));
 	}
 
+	@Test
+	public void uriIsNotChangedWhenServerIsNotSecured() throws URISyntaxException {
+		URI original = new URI("http://foo");
+		URI updated = updateToHttpsIfNeeded(original, NON_SECURE_CONFIG, NON_SECURE_INTROSPECTOR, SERVER);
+		Assert.assertThat("URI should not have been updated since server is not secured.", original, is(updated));
+	}
+
+	@Test
+	public void uriIsNotChangedWhenServerIsSecuredAndUriAlreadyInHttps() throws URISyntaxException {
+		URI original = new URI("https://foo");
+		URI updated = updateToHttpsIfNeeded(original, SECURE_CONFIG, SECURE_INTROSPECTOR, SERVER);
+		Assert.assertThat("URI should not have been updated since uri is already in https.", original, is(updated));
+	}
+
+	@Test
+	public void shouldUpgradeUriToHttpsWhenServerIsSecureAndUriNotInHttps() throws URISyntaxException {
+		URI original = new URI("http://foo");
+		URI updated = updateToHttpsIfNeeded(original, SECURE_CONFIG, SECURE_INTROSPECTOR, SERVER);
+		Assert.assertThat("URI should have been updated to https.", updated, is(new URI("https://foo")));
+	}
+
+	@Test
+	public void shouldSubstitutePlusInQueryParam() throws URISyntaxException {
+		URI original = new URI("http://foo/%20bar?hello=1+2");
+		URI updated = updateToHttpsIfNeeded(original, SECURE_CONFIG, SECURE_INTROSPECTOR, SERVER);
+		Assert.assertThat("URI should have had its plus sign replaced in query string.", updated, is(new URI(
+				"https://foo/%20bar?hello=1%202")));
+	}
+
 	static DefaultClientConfigImpl getConfig(boolean value) {
 		DefaultClientConfigImpl config = new DefaultClientConfigImpl();
 		config.setProperty(CommonClientConfigKey.IsSecure, value);