Polish ForwardedHeaderFilter and related code

Issue: SPR-16506

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

@@ -101,13 +101,13 @@ public class ForwardedHeaderFilter extends OncePerRequestFilter {
 	}
 
 	/**
-	 * 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.
+	 * Use this property to enable relative redirects as explained in
+	 * {@link RelativeRedirectFilter}, and also using the same response wrapper
+	 * as that filter does, or if both are configured, only one will wrap.
 	 * <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.
+	 * to turn relative into absolute URLs, also taking into account forwarded
+	 * headers.
 	 * @param relativeRedirects whether to use relative redirects
 	 * @since 4.3.10
 	 */
@@ -307,54 +307,41 @@ public class ForwardedHeaderFilter extends OncePerRequestFilter {
 			this.request = request;
 		}
 
-		@Override
-		public void sendRedirect(String location) throws IOException {
-			UriComponentsBuilder builder = UriComponentsBuilder.fromUriString(location);
-
-			// Absolute location
-			if (builder.build().getScheme() != null) {
-				super.sendRedirect(location);
-				return;
-			}
+@Override
+public void sendRedirect(String location) throws IOException {
 
-			// Network-path reference
-			if (location.startsWith("//")) {
-				String scheme = this.request.getScheme();
-				super.sendRedirect(builder.scheme(scheme).toUriString());
-				return;
-			}
+	UriComponentsBuilder builder = UriComponentsBuilder.fromUriString(location);
+	UriComponents uriComponents = builder.build();
 
-			String fragment = null;
-			int fragmentIndex = location.indexOf('#');
-			if (fragmentIndex != -1) {
-				if (location.length() > fragmentIndex) {
-					fragment = location.substring(fragmentIndex + 1);
-				}
-				location = location.substring(0, fragmentIndex);
-			}
+	// Absolute location
+	if (uriComponents.getScheme() != null) {
+		super.sendRedirect(location);
+		return;
+	}
 
-			String query = null;
-			int queryIndex = location.indexOf('?');
-			if (queryIndex != -1) {
-				if (location.length() > queryIndex) {
-					query = location.substring(queryIndex + 1);
-				}
-				location = location.substring(0, queryIndex);
-			}
+	// Network-path reference
+	if (location.startsWith("//")) {
+		String scheme = this.request.getScheme();
+		super.sendRedirect(builder.scheme(scheme).toUriString());
+		return;
+	}
 
-			// Relative to Servlet container root or to current request
-			String path = (location.startsWith(FOLDER_SEPARATOR) ? location :
-					StringUtils.applyRelativePath(this.request.getRequestURI(), location));
+	String path = uriComponents.getPath();
+	if (path != null) {
+		// Relative to Servlet container root or to current request
+		path = (path.startsWith(FOLDER_SEPARATOR) ? path :
+				StringUtils.applyRelativePath(this.request.getRequestURI(), path));
+	}
 
-			String result = UriComponentsBuilder
-					.fromHttpRequest(new ServletServerHttpRequest(this.request))
-					.replacePath(path)
-					.replaceQuery(query)
-					.fragment(fragment)
-					.build().normalize().toUriString();
+	String result = UriComponentsBuilder
+			.fromHttpRequest(new ServletServerHttpRequest(this.request))
+			.replacePath(path)
+			.replaceQuery(uriComponents.getQuery())
+			.fragment(uriComponents.getFragment())
+			.build().normalize().toUriString();
 
-			super.sendRedirect(result);
-		}
+	super.sendRedirect(result);
+}
 	}
 
 }

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2018 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.
@@ -27,13 +27,14 @@ import org.springframework.util.Assert;
 
 /**
  * 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>.
+ * setting the HTTP status and "Location" headers, which keeps the Servlet
+ * container from re-writing relative redirect URLs into absolute ones.
+ * Servlet containers are required to do that but against the recommendation of
+ * <a href="https://tools.ietf.org/html/rfc7231#section-7.1.2"> RFC 7231 Section 7.1.2</a>,
+ * and furthermore not necessarily taking into account "X-Forwarded" headers.
  *
- * <p><strong>Note:</strong> While relative redirects are more efficient they
- * may not work with reverse proxies under some configurations.
+ * <p><strong>Note:</strong> While relative redirects are recommended in the
+ * RFC, under some configurations with reverse proxies they may not work.
  *
  * @author Rob Winch
  * @author Rossen Stoyanchev

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

@@ -15,14 +15,13 @@
  */
 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;
+import org.springframework.web.util.WebUtils;
 
 /**
  * A response wrapper used for the implementation of
@@ -44,7 +43,7 @@ class RelativeRedirectResponseWrapper extends HttpServletResponseWrapper {
 
 
 	@Override
-	public void sendRedirect(String location) throws IOException {
+	public void sendRedirect(String location) {
 		setStatus(this.redirectStatus.value());
 		setHeader(HttpHeaders.LOCATION, location);
 	}
@@ -53,20 +52,11 @@ class RelativeRedirectResponseWrapper extends HttpServletResponseWrapper {
 	public static HttpServletResponse wrapIfNecessary(HttpServletResponse response,
 			HttpStatus redirectStatus) {
 
-		return (hasWrapper(response) ? response : new RelativeRedirectResponseWrapper(response, redirectStatus));
-	}
+		RelativeRedirectResponseWrapper wrapper =
+				WebUtils.getNativeResponse(response, RelativeRedirectResponseWrapper.class);
 
-	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;
+		return (wrapper != null ? response :
+				new RelativeRedirectResponseWrapper(response, redirectStatus));
 	}
 
 }

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

@@ -433,18 +433,22 @@ public class ForwardedHeaderFilterTests {
 	}
 
 	private String sendRedirect(final String location) throws ServletException, IOException {
-		MockHttpServletResponse response = doWithFiltersAndGetResponse(this.filter, new OncePerRequestFilter() {
+		Filter filter = new OncePerRequestFilter() {
 			@Override
-			protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
-					throws ServletException, IOException {
-				response.sendRedirect(location);
+			protected void doFilterInternal(HttpServletRequest req, HttpServletResponse res,
+					FilterChain chain) throws IOException {
+
+				res.sendRedirect(location);
 			}
-		});
+		};
+		MockHttpServletResponse response = doWithFiltersAndGetResponse(this.filter, filter);
 		return response.getRedirectedUrl();
 	}
 
 	@SuppressWarnings("serial")
-	private MockHttpServletResponse doWithFiltersAndGetResponse(Filter... filters) throws ServletException, IOException {
+	private MockHttpServletResponse doWithFiltersAndGetResponse(Filter... filters)
+			throws ServletException, IOException {
+
 		MockHttpServletResponse response = new MockHttpServletResponse();
 		FilterChain filterChain = new MockFilterChain(new HttpServlet() {}, filters);
 		filterChain.doFilter(request, response);