From bd6d732fc71e4b4af9b9ba27aa30398bb3ddfbb0 Mon Sep 17 00:00:00 2001 From: Jacques-Etienne Beaudet Date: Mon, 9 Jan 2017 10:53:16 -0500 Subject: [PATCH] Fix potential decoding and reencoding issues in query params when using SimpleHostRoutingFilter (#1464) Some webserver are picky with the encoding (uWSGI for example) and the current code behavior of decoding and reencoding the query string can lead to valid requests rejected by the backend after the modifications in Zuul. This commit adds an optional parameter to force the original encoding of the query string in SimpleHostRoutingFilter. Fixes gh-971 --- .../main/asciidoc/spring-cloud-netflix.adoc | 20 ++ .../netflix/zuul/filters/ZuulProperties.java | 9 + .../route/SimpleHostRoutingFilter.java | 35 ++-- .../zuul/SimpleZuulProxyApplicationTests.java | 171 ++++++++++++++++++ .../route/SimpleHostRoutingFilterTests.java | 3 +- 5 files changed, 224 insertions(+), 14 deletions(-) create mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/SimpleZuulProxyApplicationTests.java diff --git a/docs/src/main/asciidoc/spring-cloud-netflix.adoc b/docs/src/main/asciidoc/spring-cloud-netflix.adoc index 7585fe26..b57103ce 100644 --- a/docs/src/main/asciidoc/spring-cloud-netflix.adoc +++ b/docs/src/main/asciidoc/spring-cloud-netflix.adoc @@ -1638,6 +1638,26 @@ $ curl -v -H "Transfer-Encoding: chunked" \ -F "file=@mylarge.iso" localhost:9999/zuul/simple/file ---- +=== Query String Encoding +When processing the incoming request, query params are decoded so they can be available for possible modifications in +Zuul filters. They are then re-encoded when building the backend request in the route filters. The result +can be different than the original input if it was encoded using Javascript's `encodeURIComponent()` method for example. +While this causes no issues in most cases, some web servers can be picky with the encoding of complex query string. + +To force the original encoding of the query string, it is possible to pass a special flag to `ZuulProperties` so +that the query string is taken as is with the `HttpServletRequest::getQueryString` method : + +.application.yml +[source,yaml] +---- + zuul: + forceOriginalQueryStringEncoding: true +---- + +*Note:* This special flag only works with `SimpleHostRoutingFilter` and you loose the ability to easily override +query parameters with `RequestContext.getCurrentContext().setRequestQueryParams(someOverriddenParameters)` since +the query string is now fetched directly on the original `HttpServletRequest`. + === Plain Embedded Zuul You can also run a Zuul server without the proxying, or switch on parts of the proxying platform selectively, if you diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/ZuulProperties.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/ZuulProperties.java index 59c37dab..54bebeb5 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/ZuulProperties.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/ZuulProperties.java @@ -109,6 +109,15 @@ public class ZuulProperties { * see https://docs.spring.io/spring-security/site/docs/current/reference/html/headers.html#default-security-headers */ private boolean ignoreSecurityHeaders = true; + + /** + * Flag to force the original query string encoding when building the backend URI in + * SimpleHostRoutingFilter. When activated, query string will be built using + * HttpServletRequest getQueryString() method instead of UriTemplate. Note that this + * flag is not used in RibbonRoutingFilter with services found via DiscoveryClient + * (like Eureka). + */ + private boolean forceOriginalQueryStringEncoding = false; /** * Path to install Zuul as a servlet (not part of Spring MVC). The servlet is more diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilter.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilter.java index 9e5a6ec6..72f6dfe5 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilter.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilter.java @@ -96,6 +96,7 @@ public class SimpleHostRoutingFilter extends ZuulFilter { "SimpleHostRoutingFilter.connectionManagerTimer", true); private boolean sslHostnameValidationEnabled; + private boolean forceOriginalQueryStringEncoding; private ProxyRequestHelper helper; private Host hostProperties; @@ -119,6 +120,8 @@ public class SimpleHostRoutingFilter extends ZuulFilter { this.helper = helper; this.hostProperties = properties.getHost(); this.sslHostnameValidationEnabled = properties.isSslHostnameValidationEnabled(); + this.forceOriginalQueryStringEncoding = properties + .isForceOriginalQueryStringEncoding(); } @PostConstruct @@ -275,7 +278,7 @@ public class SimpleHostRoutingFilter extends ZuulFilter { request.getContentType() != null ? ContentType.create(request.getContentType()) : null); - HttpRequest httpRequest = buildHttpRequest(verb, uri, entity, headers, params); + HttpRequest httpRequest = buildHttpRequest(verb, uri, entity, headers, params, request); try { log.debug(httpHost.getHostName() + " " + httpHost.getPort() + " " + httpHost.getSchemeName()); @@ -292,43 +295,49 @@ public class SimpleHostRoutingFilter extends ZuulFilter { } } - protected HttpRequest buildHttpRequest (String verb, String uri, InputStreamEntity entity, - MultiValueMap headers, MultiValueMap params) - { + protected HttpRequest buildHttpRequest(String verb, String uri, + InputStreamEntity entity, MultiValueMap headers, + MultiValueMap params, HttpServletRequest request) { HttpRequest httpRequest; + String uriWithQueryString = uri + (this.forceOriginalQueryStringEncoding + ? getEncodedQueryString(request) : this.helper.getQueryString(params)); switch (verb.toUpperCase()) { case "POST": - HttpPost httpPost = new HttpPost(uri + this.helper.getQueryString(params)); + HttpPost httpPost = new HttpPost(uriWithQueryString); httpRequest = httpPost; httpPost.setEntity(entity); break; case "PUT": - HttpPut httpPut = new HttpPut(uri + this.helper.getQueryString(params)); + HttpPut httpPut = new HttpPut(uriWithQueryString); httpRequest = httpPut; httpPut.setEntity(entity); break; case "PATCH": - HttpPatch httpPatch = new HttpPatch(uri + this.helper.getQueryString(params)); + HttpPatch httpPatch = new HttpPatch(uriWithQueryString); httpRequest = httpPatch; httpPatch.setEntity(entity); break; case "DELETE": - BasicHttpEntityEnclosingRequest entityRequest = new BasicHttpEntityEnclosingRequest(verb, - uri + this.helper.getQueryString(params)); + BasicHttpEntityEnclosingRequest entityRequest = new BasicHttpEntityEnclosingRequest( + verb, uriWithQueryString); httpRequest = entityRequest; entityRequest.setEntity(entity); break; default: - httpRequest = new BasicHttpRequest(verb, - uri + this.helper.getQueryString(params)); - log.debug(uri + this.helper.getQueryString(params)); + httpRequest = new BasicHttpRequest(verb, uriWithQueryString); + log.debug(uriWithQueryString); } httpRequest.setHeaders(convertHeaders(headers)); return httpRequest; } + private String getEncodedQueryString(HttpServletRequest request) { + String query = request.getQueryString(); + return (query != null) ? "?" + query : ""; + } + private MultiValueMap revertHeaders(Header[] headers) { MultiValueMap map = new LinkedMultiValueMap(); for (Header header : headers) { @@ -399,4 +408,4 @@ public class SimpleHostRoutingFilter extends ZuulFilter { boolean isSslHostnameValidationEnabled() { return this.sslHostnameValidationEnabled; } -} +} \ No newline at end of file diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/SimpleZuulProxyApplicationTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/SimpleZuulProxyApplicationTests.java new file mode 100644 index 00000000..c972be6f --- /dev/null +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/SimpleZuulProxyApplicationTests.java @@ -0,0 +1,171 @@ +/* + * Copyright 2013-2015 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.netflix.zuul; + +import static org.junit.Assert.*; + +import java.net.URI; +import java.net.URISyntaxException; + +import javax.servlet.http.HttpServletRequest; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.context.SpringBootTest.WebEnvironment; +import org.springframework.boot.test.web.client.TestRestTemplate; +import org.springframework.cloud.netflix.zuul.filters.discovery.DiscoveryClientRouteLocator; +import org.springframework.context.annotation.Configuration; +import org.springframework.http.HttpEntity; +import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RestController; + +import com.netflix.zuul.context.RequestContext; + +@RunWith(SpringJUnit4ClassRunner.class) +@SpringBootTest(classes = SimpleZuulProxyApplicationTests.SimpleZuulProxyApplication.class, webEnvironment = WebEnvironment.RANDOM_PORT, value = { + "server.port: 0", "zuul.forceOriginalQueryStringEncoding: true" }) +@DirtiesContext +public class SimpleZuulProxyApplicationTests { + + @Value("${local.server.port}") + private int port; + + @Autowired + private DiscoveryClientRouteLocator routes; + + @Autowired + private RoutesEndpoint endpoint; + + @Before + public void setTestRequestcontext() { + RequestContext context = new RequestContext(); + RequestContext.testSetCurrentContext(context); + + this.routes.addRoute("/foo/**", "http://localhost:" + this.port + "/bar"); + this.endpoint.reset(); + } + + @Test + public void getOnSelfViaSimpleHostRoutingFilter() { + ResponseEntity result = executeSimpleRequest(HttpMethod.GET); + + assertResponseCodeAndBody(result, "get bar"); + } + + @Test + public void postOnSelfViaSimpleHostRoutingFilter() { + ResponseEntity result = executeSimpleRequest(HttpMethod.POST); + + assertResponseCodeAndBody(result, "post bar"); + } + + @Test + public void putOnSelfViaSimpleHostRoutingFilter() { + ResponseEntity result = executeSimpleRequest(HttpMethod.PUT); + + assertResponseCodeAndBody(result, "put bar"); + } + + @Test + public void patchOnSelfViaSimpleHostRoutingFilter() { + ResponseEntity result = executeSimpleRequest(HttpMethod.PATCH); + + assertResponseCodeAndBody(result, "patch bar"); + } + + @Test + public void deleteOnSelfViaSimpleHostRoutingFilter() { + ResponseEntity result = executeSimpleRequest(HttpMethod.DELETE); + + assertResponseCodeAndBody(result, "delete bar"); + } + + @Test + public void getOnSelfWithComplexQueryParam() throws URISyntaxException { + String encodedQueryString = "foo=%7B%22project%22%3A%22stream%22%2C%22logger%22%3A%22javascript%22%2C%22platform%22%3A%22javascript%22%2C%22request%22%3A%7B%22url%22%3A%22https%3A%2F%2Ffoo%2Fadmin"; + ResponseEntity result = new TestRestTemplate().exchange( + new URI("http://localhost:" + this.port + "/foo?" + encodedQueryString), + HttpMethod.GET, new HttpEntity<>((Void) null), String.class); + + assertEquals(HttpStatus.OK, result.getStatusCode()); + assertEquals(encodedQueryString, result.getBody()); + } + + private void assertResponseCodeAndBody(ResponseEntity result, + String expectedBody) { + assertEquals(HttpStatus.OK, result.getStatusCode()); + assertEquals(expectedBody, result.getBody()); + } + + private ResponseEntity executeSimpleRequest(HttpMethod httpMethod) { + ResponseEntity result = new TestRestTemplate().exchange( + "http://localhost:" + this.port + "/foo?id=bar", httpMethod, + new HttpEntity<>((Void) null), String.class); + return result; + } + + // Don't use @SpringBootApplication because we don't want to component scan + @Configuration + @EnableAutoConfiguration + @RestController + @EnableZuulProxy + static class SimpleZuulProxyApplication { + + @RequestMapping(value = "/bar", method = RequestMethod.GET) + public String get(@RequestParam String id) { + return "get " + id; + } + + @RequestMapping(value = "/bar", method = RequestMethod.GET, params = { "foo" }) + public String complexGet(@RequestParam String foo, HttpServletRequest request) { + return request.getQueryString(); + } + + @RequestMapping(value = "/bar", method = RequestMethod.POST) + public String post(@RequestParam String id) { + return "post " + id; + } + + @RequestMapping(value = "/bar", method = RequestMethod.PUT) + public String put(@RequestParam String id) { + return "put " + id; + } + + @RequestMapping(value = "/bar", method = RequestMethod.DELETE) + public String delete(@RequestParam String id) { + return "delete " + id; + } + + @RequestMapping(value = "/bar", method = RequestMethod.PATCH) + public String patch(@RequestParam String id) { + return "patch " + id; + } + + } +} \ No newline at end of file diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilterTests.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilterTests.java index faf5871e..183c0132 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilterTests.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/SimpleHostRoutingFilterTests.java @@ -31,6 +31,7 @@ import org.springframework.cloud.netflix.zuul.filters.ZuulProperties; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.util.LinkedMultiValueMap; import static org.junit.Assert.assertEquals; @@ -91,7 +92,7 @@ public class SimpleHostRoutingFilterTests { setupContext(); InputStreamEntity inputStreamEntity = new InputStreamEntity(new ByteArrayInputStream(new byte[]{1})); HttpRequest httpRequest = getFilter().buildHttpRequest("DELETE", "uri", inputStreamEntity, - new LinkedMultiValueMap(), new LinkedMultiValueMap()); + new LinkedMultiValueMap(), new LinkedMultiValueMap(), new MockHttpServletRequest()); assertTrue(httpRequest instanceof HttpEntityEnclosingRequest); HttpEntityEnclosingRequest httpEntityEnclosingRequest = (HttpEntityEnclosingRequest) httpRequest;