From d21a7ef8e203ec8310a58493d37d3f3baf712789 Mon Sep 17 00:00:00 2001 From: Jacques-Etienne Beaudet Date: Wed, 3 Aug 2016 13:51:38 -0400 Subject: [PATCH 1/5] Fix an issue where the FormBodyWrapperFilter would encode the form parameters differently than on the original request. The FormBodyWrapperFilter handles the application/x-www-form-urlencoded. In the case of requests received by curl or javascript, the FormHttpMessageConverter will reencode the parameters differently (for example, '(' will be encoded while it's not with the javascript encodeURIComponent method). While this doesn't create any problem, the content length was not properly set in AbstractRibbonCommand. This causes the form params being stripped or the backend server would wait a long time for additional bytes depending on if the content length header was bigger/smaller than the actual data. --- .../filters/pre/FormBodyWrapperFilter.java | 16 +++++ .../route/support/AbstractRibbonCommand.java | 6 +- ...stClientRibbonCommandIntegrationTests.java | 25 +++++++ .../NoEncodingFormHttpMessageConverter.java | 65 +++++++++++++++++++ .../route/support/ZuulProxyTestBase.java | 45 ++++++++++++- 5 files changed, 153 insertions(+), 4 deletions(-) create mode 100644 spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/NoEncodingFormHttpMessageConverter.java diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/FormBodyWrapperFilter.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/FormBodyWrapperFilter.java index 35a3a45a..3c252b4b 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/FormBodyWrapperFilter.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/FormBodyWrapperFilter.java @@ -26,6 +26,8 @@ import java.util.Map.Entry; import java.util.Set; import javax.servlet.ServletInputStream; +import javax.servlet.ServletRequest; +import javax.servlet.ServletRequestWrapper; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.Part; @@ -57,13 +59,19 @@ import com.netflix.zuul.http.ServletInputStreamWrapper; public class FormBodyWrapperFilter extends ZuulFilter { private Field requestField; + private Field servletRequestField; public FormBodyWrapperFilter() { this.requestField = ReflectionUtils.findField(HttpServletRequestWrapper.class, "req", HttpServletRequest.class); + this.servletRequestField = ReflectionUtils.findField(ServletRequestWrapper.class, + "request", ServletRequest.class); Assert.notNull(this.requestField, "HttpServletRequestWrapper.req field not found"); + Assert.notNull(this.servletRequestField, + "ServletRequestWrapper.request field not found"); this.requestField.setAccessible(true); + this.servletRequestField.setAccessible(true); } @Override @@ -113,6 +121,9 @@ public class FormBodyWrapperFilter extends ZuulFilter { .getField(this.requestField, request); wrapper = new FormBodyRequestWrapper(wrapped); ReflectionUtils.setField(this.requestField, request, wrapper); + if(request instanceof ServletRequestWrapper) { + ReflectionUtils.setField(this.servletRequestField, request, wrapper); + } } else { wrapper = new FormBodyRequestWrapper(request); @@ -159,6 +170,11 @@ public class FormBodyWrapperFilter extends ZuulFilter { } return this.contentLength; } + + @Override + public long getContentLengthLong() { + return getContentLength(); + } @Override public ServletInputStream getInputStream() throws IOException { diff --git a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommand.java b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommand.java index 04261c79..eb15b4ba 100644 --- a/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommand.java +++ b/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/route/support/AbstractRibbonCommand.java @@ -74,9 +74,9 @@ public abstract class AbstractRibbonCommand> converters = new ArrayList<>(); + converters.addAll(Arrays.asList(new StringHttpMessageConverter(), + new NoEncodingFormHttpMessageConverter())); + testRestTemplate.setMessageConverters(converters); + + MultiValueMap map = new LinkedMultiValueMap<>(); + map.add("foo", "(bar)"); + ResponseEntity result = testRestTemplate.postForEntity( + "http://localhost:" + this.port + "/simple/local", map, String.class); + assertEquals(HttpStatus.OK, result.getStatusCode()); + assertEquals("Posted [(bar)] and Content-Length was: -1!", result.getBody()); + } @Test public void routeLocatorOverridden() { diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/NoEncodingFormHttpMessageConverter.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/NoEncodingFormHttpMessageConverter.java new file mode 100644 index 00000000..3f54c909 --- /dev/null +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/NoEncodingFormHttpMessageConverter.java @@ -0,0 +1,65 @@ +/* + * Copyright 2013-2016 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.filters.route.support; + +import java.io.IOException; +import java.util.Iterator; + +import org.springframework.http.HttpOutputMessage; +import org.springframework.http.MediaType; +import org.springframework.http.converter.FormHttpMessageConverter; +import org.springframework.http.converter.HttpMessageNotWritableException; +import org.springframework.util.MultiValueMap; +import org.springframework.util.StreamUtils; + +/** + * @author Jacques-Etienne Beaudet + */ +public class NoEncodingFormHttpMessageConverter extends FormHttpMessageConverter { + + @SuppressWarnings("unchecked") + @Override + public void write(MultiValueMap map, MediaType contentType, HttpOutputMessage outputMessage) + throws IOException, HttpMessageNotWritableException { + + MultiValueMap form = (MultiValueMap) map; + StringBuilder builder = new StringBuilder(); + for (Iterator nameIterator = form.keySet().iterator(); nameIterator.hasNext();) { + String name = nameIterator.next(); + for (Iterator valueIterator = form.get(name).iterator(); valueIterator.hasNext();) { + String value = valueIterator.next(); + builder.append(name); + if (value != null) { + builder.append('='); + builder.append(value); + if (valueIterator.hasNext()) { + builder.append('&'); + } + } + } + if (nameIterator.hasNext()) { + builder.append('&'); + } + } + final byte[] bytes = builder.toString().getBytes(FormHttpMessageConverter.DEFAULT_CHARSET); + outputMessage.getHeaders().setContentLength(bytes.length); + outputMessage.getHeaders().setContentType(MediaType.APPLICATION_FORM_URLENCODED); + + StreamUtils.copy(bytes, outputMessage.getBody()); + } +} diff --git a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/ZuulProxyTestBase.java b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/ZuulProxyTestBase.java index ffdb3771..21e993c4 100644 --- a/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/ZuulProxyTestBase.java +++ b/spring-cloud-netflix-core/src/test/java/org/springframework/cloud/netflix/zuul/filters/route/support/ZuulProxyTestBase.java @@ -17,6 +17,8 @@ package org.springframework.cloud.netflix.zuul.filters.route.support; +import java.nio.charset.Charset; +import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.List; @@ -25,7 +27,6 @@ import java.util.concurrent.atomic.AtomicBoolean; import javax.servlet.http.HttpServletRequest; -import org.junit.Assume; import org.junit.Before; import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -45,7 +46,12 @@ 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.MediaType; import org.springframework.http.ResponseEntity; +import org.springframework.http.converter.FormHttpMessageConverter; +import org.springframework.http.converter.HttpMessageConverter; +import org.springframework.http.converter.StringHttpMessageConverter; +import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.RequestBody; @@ -53,6 +59,7 @@ 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.servlet.config.annotation.DelegatingWebMvcConfiguration; +import org.springframework.web.servlet.config.annotation.WebMvcConfigurerAdapter; import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping; import com.netflix.loadbalancer.Server; @@ -247,6 +254,23 @@ public abstract class ZuulProxyTestBase { assertEquals(HttpStatus.OK, result.getStatusCode()); assertEquals("Patched 1!", result.getBody()); } + + @SuppressWarnings("deprecation") + @Test + public void javascriptEncodedFormParams() { + TestRestTemplate testRestTemplate = new TestRestTemplate(); + ArrayList> converters = new ArrayList<>(); + converters.addAll(Arrays.asList(new StringHttpMessageConverter(), + new NoEncodingFormHttpMessageConverter())); + testRestTemplate.setMessageConverters(converters); + + MultiValueMap map = new LinkedMultiValueMap<>(); + map.add("foo", "(bar)"); + ResponseEntity result = testRestTemplate.postForEntity( + "http://localhost:" + this.port + "/simple/local", map, String.class); + assertEquals(HttpStatus.OK, result.getStatusCode()); + assertEquals("Posted [(bar)] and Content-Length was: 13!", result.getBody()); + } protected abstract boolean supportsPatch(); @@ -268,6 +292,12 @@ public abstract class ZuulProxyTestBase { public String local() { return "Hello local"; } + + @RequestMapping(value = "/local", method = RequestMethod.POST) + public String postWithFormParam(HttpServletRequest request, + @RequestBody MultiValueMap body) { + return "Posted " + body.get("foo") + " and Content-Length was: " + request.getContentLength() + "!"; + } @RequestMapping(value = "/local/{id}", method = RequestMethod.DELETE) public String delete(@PathVariable String id) { @@ -341,6 +371,19 @@ public abstract class ZuulProxyTestBase { return mapping; } } + + @Configuration + public class FormEncodedMessageConverterConfiguration extends WebMvcConfigurerAdapter { + + @Override + public void configureMessageConverters(List> converters) { + FormHttpMessageConverter converter = new FormHttpMessageConverter(); + MediaType mediaType = new MediaType("application", "x-www-form-urlencoded", Charset.forName("UTF-8")); + converter.setSupportedMediaTypes(Arrays.asList(mediaType)); + converters.add(converter); + super.configureMessageConverters(converters); + } + } // Load balancer with fixed server list for "simple" pointing to localhost @Configuration From eed72600c5b812146328c71d3fa8a5040aad4a00 Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Mon, 8 Aug 2016 11:47:29 -0400 Subject: [PATCH 2/5] initial commit for circleci build --- circle.yml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 circle.yml diff --git a/circle.yml b/circle.yml new file mode 100644 index 00000000..526f14ff --- /dev/null +++ b/circle.yml @@ -0,0 +1,25 @@ +general: + branches: + ignore: + - gh-pages # list of branches to ignore +machine: + java: + version: oraclejdk8 + environment: + _JAVA_OPTIONS: "-Xms1024m -Xmx2048m" +dependencies: + override: + - ./mvnw -s .settings.xml -U --fail-never dependency:go-offline || true +test: + override: + - ./mvnw -s .settings.xml clean install -U -P sonar -nsu --batch-mode -Dmaven.test.redirectTestOutputToFile=true -Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn + post: + - mkdir -p $CIRCLE_TEST_REPORTS/junit/ + - find . -type f -regex ".*/target/.*-reports/.*" -exec cp {} $CIRCLE_TEST_REPORTS/junit/ \; +# - bash <(curl -s https://codecov.io/bash) + +notify: + webhooks: + # A list of hook hashes, containing the url field + # gitter hook +# - url: https://webhooks.gitter.im/e/fac69f6fc000ef14f7ec From 5676c8f7533996889db13bda863cf8d73dedb162 Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Mon, 8 Aug 2016 13:20:12 -0400 Subject: [PATCH 3/5] added artifacts directory to circle config --- circle.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/circle.yml b/circle.yml index 526f14ff..39c52677 100644 --- a/circle.yml +++ b/circle.yml @@ -2,6 +2,8 @@ general: branches: ignore: - gh-pages # list of branches to ignore + artifacts: + - "target/" machine: java: version: oraclejdk8 From 4f93897b71b93830fd021af05b3995f3fd94df44 Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Mon, 8 Aug 2016 13:57:23 -0400 Subject: [PATCH 4/5] more circle ci configuration --- circle.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/circle.yml b/circle.yml index 39c52677..20b3eeeb 100644 --- a/circle.yml +++ b/circle.yml @@ -3,7 +3,7 @@ general: ignore: - gh-pages # list of branches to ignore artifacts: - - "target/" + - ".*/target/" machine: java: version: oraclejdk8 @@ -18,10 +18,10 @@ test: post: - mkdir -p $CIRCLE_TEST_REPORTS/junit/ - find . -type f -regex ".*/target/.*-reports/.*" -exec cp {} $CIRCLE_TEST_REPORTS/junit/ \; -# - bash <(curl -s https://codecov.io/bash) + - bash <(curl -s https://codecov.io/bash) notify: webhooks: # A list of hook hashes, containing the url field # gitter hook -# - url: https://webhooks.gitter.im/e/fac69f6fc000ef14f7ec + - url: https://webhooks.gitter.im/e/22e6bb4eb945dd61ba54 From 52c956831e4daf6f2bd09a4b8b960c28030912fd Mon Sep 17 00:00:00 2001 From: Ryan Baxter Date: Mon, 8 Aug 2016 15:03:00 -0400 Subject: [PATCH 5/5] circleci updates --- circle.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/circle.yml b/circle.yml index 20b3eeeb..466be43d 100644 --- a/circle.yml +++ b/circle.yml @@ -2,8 +2,6 @@ general: branches: ignore: - gh-pages # list of branches to ignore - artifacts: - - ".*/target/" machine: java: version: oraclejdk8 @@ -16,6 +14,7 @@ test: override: - ./mvnw -s .settings.xml clean install -U -P sonar -nsu --batch-mode -Dmaven.test.redirectTestOutputToFile=true -Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn post: + - find . -type f -regex ".*/spring-cloud-*.*/target/*.*" | cpio -pdm $CIRCLE_ARTIFACTS - mkdir -p $CIRCLE_TEST_REPORTS/junit/ - find . -type f -regex ".*/target/.*-reports/.*" -exec cp {} $CIRCLE_TEST_REPORTS/junit/ \; - bash <(curl -s https://codecov.io/bash)