ForwardedHeaderFilter handles query+fragment correctly

Issue: SPR-16506

Author: rstoyanchev

spring-web/src/main/java/org/springframework/web/filter/ForwardedHeaderFilter.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.
@@ -100,13 +100,13 @@ public void setRemoveOnly(boolean removeOnly) {
 	}
 
 	/**
-	 * 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
 	 */
@@ -304,10 +304,12 @@ public ForwardedHeaderExtractingResponse(HttpServletResponse response, HttpServl
 
 		@Override
 		public void sendRedirect(String location) throws IOException {
+
 			UriComponentsBuilder builder = UriComponentsBuilder.fromUriString(location);
+			UriComponents uriComponents = builder.build();
 
 			// Absolute location
-			if (builder.build().getScheme() != null) {
+			if (uriComponents.getScheme() != null) {
 				super.sendRedirect(location);
 				return;
 			}
@@ -319,13 +321,18 @@ public void sendRedirect(String location) throws IOException {
 				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(uriComponents.getQuery())
+					.fragment(uriComponents.getFragment())
 					.build().normalize().toUriString();
 
 			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 @@
 
 /**
  * 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

@@ -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.
@@ -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 @@ private RelativeRedirectResponseWrapper(HttpServletResponse response, HttpStatus
 
 
 	@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 @@ public void sendRedirect(String location) throws IOException {
 	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

@@ -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.
@@ -301,6 +301,17 @@ public void sendRedirectWithAbsolutePath() throws Exception {
 		assertEquals("https://example.com/foo/bar", redirectedUrl);
 	}
 
+	@Test // SPR-16506
+	public void sendRedirectWithAbsolutePathQueryParamAndFragment() throws Exception {
+		this.request.addHeader(X_FORWARDED_PROTO, "https");
+		this.request.addHeader(X_FORWARDED_HOST, "example.com");
+		this.request.addHeader(X_FORWARDED_PORT, "443");
+		this.request.setQueryString("oldqp=1");
+
+		String redirectedUrl = sendRedirect("/foo/bar?newqp=2#fragment");
+		assertEquals("https://example.com/foo/bar?newqp=2#fragment", redirectedUrl);
+	}
+
 	@Test
 	public void sendRedirectWithContextPath() throws Exception {
 		this.request.addHeader(X_FORWARDED_PROTO, "https");