diff --git a/spring-web/src/main/java/org/springframework/web/filter/ForwardedHeaderFilter.java b/spring-web/src/main/java/org/springframework/web/filter/ForwardedHeaderFilter.java index 0984a942d39..2406c36295b 100644 --- a/spring-web/src/main/java/org/springframework/web/filter/ForwardedHeaderFilter.java +++ b/spring-web/src/main/java/org/springframework/web/filter/ForwardedHeaderFilter.java @@ -31,6 +31,7 @@ import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpServletResponseWrapper; import org.springframework.http.HttpRequest; +import org.springframework.http.HttpStatus; import org.springframework.http.server.ServletServerHttpRequest; import org.springframework.lang.Nullable; import org.springframework.util.CollectionUtils; @@ -79,7 +80,7 @@ public class ForwardedHeaderFilter extends OncePerRequestFilter { private boolean removeOnly; - private boolean requestOnly; + private boolean relativeRedirects; public ForwardedHeaderFilter() { @@ -100,21 +101,21 @@ public class ForwardedHeaderFilter extends OncePerRequestFilter { } /** - * Enables mode in which only the HttpServletRequest is modified. This means that - * {@link HttpServletResponse#sendRedirect(String)} will only work when the application is configured to use - * relative redirects. This can be done by placing {@link RelativeRedirectFilter} after this Filter or Servlet - * Container specific setup. For example, using Tomcat's - * useRelativeRedirects - * attribute. - * - * @param requestOnly whether to customize the {@code HttpServletResponse} or not. Default is false (customize the - * {@code HttpServletResponse}) - * @since 4.3.10 + * Use this property to enable relative redirects as explained in and also + * using the same response wrapper as {@link RelativeRedirectFilter} does. + * Or if both filters are used, only one will wrap the response. + *

By default, if this property is set to false, in which case calls to + * {@link HttpServletResponse#sendRedirect(String)} are overridden in order + * to turn relative into absolute URLs since (which Servlet containers are + * also required to do) also taking forwarded headers into consideration. + * @param relativeRedirects whether to use relative redirects + * @since 5.0 */ - public void setRequestOnly(boolean requestOnly) { - this.requestOnly = requestOnly; + public void setRelativeRedirects(boolean relativeRedirects) { + this.relativeRedirects = relativeRedirects; } + @Override protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException { Enumeration names = request.getHeaderNames(); @@ -147,7 +148,8 @@ public class ForwardedHeaderFilter extends OncePerRequestFilter { } else { HttpServletRequest theRequest = new ForwardedHeaderExtractingRequest(request, this.pathHelper); - HttpServletResponse theResponse = this.requestOnly ? response : + HttpServletResponse theResponse = this.relativeRedirects ? + RelativeRedirectResponseWrapper.wrapIfNecessary(response, HttpStatus.SEE_OTHER) : new ForwardedHeaderExtractingResponse(response, theRequest); filterChain.doFilter(theRequest, theResponse); } diff --git a/spring-web/src/main/java/org/springframework/web/filter/RelativeRedirectFilter.java b/spring-web/src/main/java/org/springframework/web/filter/RelativeRedirectFilter.java index c2f893d2b2a..3c7ad8c262f 100644 --- a/spring-web/src/main/java/org/springframework/web/filter/RelativeRedirectFilter.java +++ b/spring-web/src/main/java/org/springframework/web/filter/RelativeRedirectFilter.java @@ -16,7 +16,6 @@ package org.springframework.web.filter; -import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; import org.springframework.util.Assert; @@ -24,63 +23,52 @@ import javax.servlet.FilterChain; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import javax.servlet.http.HttpServletResponseWrapper; import java.io.IOException; /** - * Overrides the {@link HttpServletResponse#sendRedirect(String)} to set the "Location" header and the HTTP Status - * directly to avoid the Servlet Container from creating an absolute URL. This allows redirects that have a relative - * "Location" that ensures support for RFC 7231 Section - * 7.1.2. It should be noted that while relative redirects are more efficient, they may not work with reverse - * proxies under some configurations. + * Overrides {@link HttpServletResponse#sendRedirect(String)} and handles it by + * setting the HTTP status and "Location" headers. This keeps the Servlet + * container from re-writing relative redirect URLs and instead follows the + * recommendation in + * RFC 7231 Section 7.1.2. + * + *

Note: while relative redirects are more efficient they + * may not work with reverse proxies under some configurations. * * @author Rob Winch - * @since 4.3.10 + * @author Rossen Stoyanchev + * @since 5.0 */ public class RelativeRedirectFilter extends OncePerRequestFilter { - private HttpStatus sendRedirectHttpStatus = HttpStatus.FOUND; + + private HttpStatus redirectStatus = HttpStatus.SEE_OTHER; + /** - * Sets the HTTP Status to be used when {@code HttpServletResponse#sendRedirect(String)} is invoked. - * @param sendRedirectHttpStatus the 3xx HTTP Status to be used when - * {@code HttpServletResponse#sendRedirect(String)} is invoked. The default is {@code HttpStatus.FOUND}. + * Set the default HTTP Status to use for redirects. + *

By default this is {@link HttpStatus#SEE_OTHER}. + * @param status the 3xx redirect status to use */ - public void setSendRedirectHttpStatus(HttpStatus sendRedirectHttpStatus) { - Assert.notNull(sendRedirectHttpStatus, "HttpStatus is required"); - if(!sendRedirectHttpStatus.is3xxRedirection()) { - throw new IllegalArgumentException("sendRedirectHttpStatus should be for redirection. Got " + sendRedirectHttpStatus); - } - this.sendRedirectHttpStatus = sendRedirectHttpStatus; - } - - @Override - protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { - filterChain.doFilter(request, new RelativeRedirectResponse(response)); + public void setRedirectStatus(HttpStatus status) { + Assert.notNull(status, "HttpStatus is required"); + Assert.isTrue(status.is3xxRedirection(), "Not a redirect status: " + status); + this.redirectStatus = status; } /** - * Modifies {@link #sendRedirect(String)} to explicitly set the "Location" header and an HTTP status code to avoid - * containers from rewriting the location to be an absolute URL. - * - * @author Rob Winch - * @since 4.3.10 + * Return the configured redirect status. */ - private class RelativeRedirectResponse extends HttpServletResponseWrapper { + public HttpStatus getRedirectStatus() { + return this.redirectStatus; + } - /** - * Constructs a response adaptor wrapping the given response. - * - * @param response - * @throws IllegalArgumentException if the response is null - */ - public RelativeRedirectResponse(HttpServletResponse response) { - super(response); - } - @Override - public void sendRedirect(String location) throws IOException { - setHeader(HttpHeaders.LOCATION, location); - setStatus(sendRedirectHttpStatus.value()); - } + @Override + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, + FilterChain filterChain) throws ServletException, IOException { + + response = RelativeRedirectResponseWrapper.wrapIfNecessary(response, this.redirectStatus); + filterChain.doFilter(request, response); } + } diff --git a/spring-web/src/main/java/org/springframework/web/filter/RelativeRedirectResponseWrapper.java b/spring-web/src/main/java/org/springframework/web/filter/RelativeRedirectResponseWrapper.java new file mode 100644 index 00000000000..744f8d63b1a --- /dev/null +++ b/spring-web/src/main/java/org/springframework/web/filter/RelativeRedirectResponseWrapper.java @@ -0,0 +1,73 @@ +/* + * Copyright 2002-2017 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.web.filter; + +import java.io.IOException; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpServletResponseWrapper; + +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpStatus; +import org.springframework.util.Assert; + +/** + * A response wrapper used for the implementation of + * {@link RelativeRedirectFilter} also shared with {@link ForwardedHeaderFilter}. + * + * @author Rossen Stoyanchev + * @since 5.0 + */ +class RelativeRedirectResponseWrapper extends HttpServletResponseWrapper { + + private final HttpStatus redirectStatus; + + + private RelativeRedirectResponseWrapper(HttpServletResponse response, HttpStatus redirectStatus) { + super(response); + Assert.notNull(redirectStatus, "'redirectStatus' is required."); + this.redirectStatus = redirectStatus; + } + + + @Override + public void sendRedirect(String location) throws IOException { + setStatus(this.redirectStatus.value()); + setHeader(HttpHeaders.LOCATION, location); + } + + + public static HttpServletResponse wrapIfNecessary(HttpServletResponse response, + HttpStatus redirectStatus) { + + return hasWrapper(response) ? response : + new RelativeRedirectResponseWrapper(response, redirectStatus); + } + + private static boolean hasWrapper(ServletResponse response) { + if (response instanceof RelativeRedirectResponseWrapper) { + return true; + } + while (response instanceof HttpServletResponseWrapper) { + response = ((HttpServletResponseWrapper) response).getResponse(); + if (response instanceof RelativeRedirectResponseWrapper) { + return true; + } + } + return false; + } + +} diff --git a/spring-web/src/test/java/org/springframework/web/filter/ForwardedHeaderFilterTests.java b/spring-web/src/test/java/org/springframework/web/filter/ForwardedHeaderFilterTests.java index fc23caa993f..a93fe592245 100644 --- a/spring-web/src/test/java/org/springframework/web/filter/ForwardedHeaderFilterTests.java +++ b/spring-web/src/test/java/org/springframework/web/filter/ForwardedHeaderFilterTests.java @@ -408,7 +408,7 @@ public class ForwardedHeaderFilterTests { this.request.addHeader(X_FORWARDED_PROTO, "https"); this.request.addHeader(X_FORWARDED_HOST, "example.com"); this.request.addHeader(X_FORWARDED_PORT, "443"); - this.filter.setRequestOnly(true); + this.filter.setRelativeRedirects(true); String location = sendRedirect("/a"); @@ -417,7 +417,7 @@ public class ForwardedHeaderFilterTests { @Test public void sendRedirectWhenRequestOnlyAndNoXForwardedThenUsesRelativeRedirects() throws Exception { - this.filter.setRequestOnly(true); + this.filter.setRelativeRedirects(true); String location = sendRedirect("/a"); diff --git a/spring-web/src/test/java/org/springframework/web/filter/RelativeRedirectFilterTests.java b/spring-web/src/test/java/org/springframework/web/filter/RelativeRedirectFilterTests.java index ffcb036033e..5eabda751aa 100644 --- a/spring-web/src/test/java/org/springframework/web/filter/RelativeRedirectFilterTests.java +++ b/spring-web/src/test/java/org/springframework/web/filter/RelativeRedirectFilterTests.java @@ -16,67 +16,96 @@ package org.springframework.web.filter; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpServletResponseWrapper; + import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; + import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; import org.springframework.mock.web.test.MockFilterChain; import org.springframework.mock.web.test.MockHttpServletRequest; +import org.springframework.mock.web.test.MockHttpServletResponse; -import javax.servlet.http.HttpServletResponse; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertSame; /** + * Unit tests for {@link RelativeRedirectFilter}. * @author Rob Winch - * @since 4.3.10 */ @RunWith(MockitoJUnitRunner.class) public class RelativeRedirectFilterTests { + @Mock HttpServletResponse response; RelativeRedirectFilter filter = new RelativeRedirectFilter(); + @Test(expected = IllegalArgumentException.class) public void sendRedirectHttpStatusWhenNullThenIllegalArgumentException() { - this.filter.setSendRedirectHttpStatus(null); + this.filter.setRedirectStatus(null); } @Test(expected = IllegalArgumentException.class) public void sendRedirectHttpStatusWhenNot3xxThenIllegalArgumentException() { - this.filter.setSendRedirectHttpStatus(HttpStatus.OK); + this.filter.setRedirectStatus(HttpStatus.OK); } @Test - public void doFilterSendRedirectWhenDefaultsThenLocationAnd302() throws Exception { + public void doFilterSendRedirectWhenDefaultsThenLocationAnd303() throws Exception { String location = "/foo"; - sendRedirect(location); InOrder inOrder = Mockito.inOrder(this.response); + inOrder.verify(this.response).setStatus(HttpStatus.SEE_OTHER.value()); inOrder.verify(this.response).setHeader(HttpHeaders.LOCATION, location); - inOrder.verify(this.response).setStatus(HttpStatus.FOUND.value()); } @Test public void doFilterSendRedirectWhenCustomSendRedirectHttpStatusThenLocationAnd301() throws Exception { String location = "/foo"; HttpStatus status = HttpStatus.MOVED_PERMANENTLY; - this.filter.setSendRedirectHttpStatus(status); + this.filter.setRedirectStatus(status); sendRedirect(location); InOrder inOrder = Mockito.inOrder(this.response); - inOrder.verify(this.response).setHeader(HttpHeaders.LOCATION, location); inOrder.verify(this.response).setStatus(status.value()); + inOrder.verify(this.response).setHeader(HttpHeaders.LOCATION, location); } - private void sendRedirect(String location) throws Exception { + @Test + public void wrapOnceOnly() throws Exception { + HttpServletResponse original = new MockHttpServletResponse(); + MockFilterChain chain = new MockFilterChain(); + this.filter.doFilterInternal(new MockHttpServletRequest(), original, chain); + + HttpServletResponse wrapped1 = (HttpServletResponse) chain.getResponse(); + assertNotSame(original, wrapped1); - filter.doFilterInternal(new MockHttpServletRequest(), response, chain); + chain.reset(); + this.filter.doFilterInternal(new MockHttpServletRequest(), wrapped1, chain); + HttpServletResponse current = (HttpServletResponse) chain.getResponse(); + assertSame(wrapped1, current); + + chain.reset(); + HttpServletResponse wrapped2 = new HttpServletResponseWrapper(wrapped1); + this.filter.doFilterInternal(new MockHttpServletRequest(), wrapped2, chain); + current = (HttpServletResponse) chain.getResponse(); + assertSame(wrapped2, current); + } + + + private void sendRedirect(String location) throws Exception { + MockFilterChain chain = new MockFilterChain(); + this.filter.doFilterInternal(new MockHttpServletRequest(), this.response, chain); HttpServletResponse wrappedResponse = (HttpServletResponse) chain.getResponse(); wrappedResponse.sendRedirect(location);