Refactor relative redirect filter support

Issue: SPR-15717

Author: rstoyanchev

spring-web/src/main/java/org/springframework/web/filter/ForwardedHeaderFilter.java

@@ -31,6 +31,7 @@
 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 void setRemoveOnly(boolean removeOnly) {
 	}
 
 	/**
-	 * 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
-	 * <a href="https://tomcat.apache.org/tomcat-8.0-doc/config/context.html#Common_Attributes">useRelativeRedirects</a>
-	 * 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.
+	 * <p>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<String> names = request.getHeaderNames();
@@ -147,7 +148,8 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
 		}
 		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);
 		}

spring-web/src/main/java/org/springframework/web/filter/RelativeRedirectFilter.java

@@ -16,71 +16,59 @@
 
 package org.springframework.web.filter;
 
-import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpStatus;
 import org.springframework.util.Assert;
 
 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 <a href="https://tools.ietf.org/html/rfc7231#section-7.1.2">RFC 7231 Section
- * 7.1.2</a>. 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 <a href="https://tools.ietf.org/html/rfc7231#section-7.1.2">
+ * RFC 7231 Section 7.1.2</a>.
+ *
+ * <p><strong>Note:</strong> 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.
+	 * <p>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);
 	}
+
 }

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;
+	}
+
+}

spring-web/src/test/java/org/springframework/web/filter/ForwardedHeaderFilterTests.java

@@ -408,7 +408,7 @@ public void sendRedirectWhenRequestOnlyAndXForwardedThenUsesRelativeRedirects()
 		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 void sendRedirectWhenRequestOnlyAndXForwardedThenUsesRelativeRedirects()
 
 	@Test
 	public void sendRedirectWhenRequestOnlyAndNoXForwardedThenUsesRelativeRedirects() throws Exception {
-		this.filter.setRequestOnly(true);
+		this.filter.setRelativeRedirects(true);
 
 		String location = sendRedirect("/a");
 

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);