From d21a7ef8e203ec8310a58493d37d3f3baf712789 Mon Sep 17 00:00:00 2001 From: Jacques-Etienne Beaudet Date: Wed, 3 Aug 2016 13:51:38 -0400 Subject: [PATCH] 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