diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java index 711b665b..fb172027 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/ribbon/RibbonUtils.java +++ b/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; } - } diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfigurationTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfigurationTests.java index e6d05f7c..be0bbe45 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonClientConfigurationTests.java +++ b/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 clients() { ArrayList clients = new ArrayList<>(); clients.add(new OverrideRestClient(this.config, this.inspector)); diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java index f8e8204f..53b1bd8d 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/ribbon/RibbonUtilsTests.java +++ b/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);