#713 - Centralize all Forward header handling

Spring 5.1 is centralizing all Forward header handling. This moves critical bits into one location, making it easy to completely remove once we rebaseline against this version.

Also adds a test profile to ensure Spring 5.1 doesn't break anything.

Author: gregturn

.travis.yml

@@ -5,6 +5,7 @@ env:
   matrix:
     - PROFILE=non-existant
     - PROFILE=spring5-next
+    - PROFILE=spring51-next
 addons:
   apt:
     packages:

pom.xml

@@ -97,6 +97,20 @@
 			</repositories>
 		</profile>
 
+		<profile>
+			<id>spring51-next</id>
+			<properties>
+				<spring.version>5.1.0.BUILD-SNAPSHOT</spring.version>
+				<jackson.version>2.9.2</jackson.version>
+			</properties>
+			<repositories>
+				<repository>
+					<id>spring-libs-snapshot</id>
+					<url>http://repo.spring.io/libs-snapshot</url>
+				</repository>
+			</repositories>
+		</profile>
+
 		<profile>
 
 			<!-- Profile to be run on the CI server, JARs JavaDocs -->

src/main/java/org/springframework/hateoas/mvc/ControllerLinkBuilder.java

@@ -15,7 +15,7 @@
  */
 package org.springframework.hateoas.mvc;
 
-import static org.springframework.util.StringUtils.*;
+import static org.springframework.hateoas.mvc.ForwardedHeader.handleXForwardedSslHeader;
 
 import lombok.RequiredArgsConstructor;
 import lombok.experimental.Delegate;
@@ -28,6 +28,7 @@
 
 import javax.servlet.http.HttpServletRequest;
 
+import org.springframework.context.ApplicationContext;
 import org.springframework.core.io.support.SpringFactoriesLoader;
 import org.springframework.hateoas.Affordance;
 import org.springframework.hateoas.Link;
@@ -306,27 +307,38 @@ public String toString() {
 	 * 
 	 * @return
 	 */
-	public static UriComponentsBuilder getBuilder() {
+	public static UriComponentsBuilder getBuilder() {
 
 		if (RequestContextHolder.getRequestAttributes() == null) {
 			return UriComponentsBuilder.fromPath("/");
 		}
 
 		HttpServletRequest request = getCurrentRequest();
-		UriComponentsBuilder builder = ServletUriComponentsBuilder.fromServletMapping(request);
-
-		// special case handling for X-Forwarded-Ssl:
-		// apply it, but only if X-Forwarded-Proto is unset.
+		ServletUriComponentsBuilder builder = ServletUriComponentsBuilder.fromServletMapping(request);
+
+		// As of SPR-16863, Spring 5.1 can handle X-Forwarded-Ssl headers...
+		if (isSpringAtLeast5_1()) {
+			//return builder;
+			return handleXForwardedSslHeader(request, builder);
+		} else {
+			return handleXForwardedSslHeader(request, builder);
+		}
+	}
 
-		String forwardedSsl = request.getHeader("X-Forwarded-Ssl");
-		ForwardedHeader forwarded = ForwardedHeader.of(request.getHeader(ForwardedHeader.NAME));
-		String proto = hasText(forwarded.getProto()) ? forwarded.getProto() : request.getHeader("X-Forwarded-Proto");
+	/**
+	 * Check if the current version of Spring Framework is 5.1 or higher.
+	 * 
+	 * @return
+	 */
+	private static boolean isSpringAtLeast5_1() {
+		
+		String versionOfSpringFramework = ApplicationContext.class.getPackage().getImplementationVersion();
 
-		if (!hasText(proto) && hasText(forwardedSsl) && forwardedSsl.equalsIgnoreCase("on")) {
-			builder.scheme("https");
-		}
+		String[] parts = versionOfSpringFramework.split("\\.");
+		int majorVersion = Integer.parseInt(parts[0]);
+		int minorVersion = Integer.parseInt(parts[1]);
 
-		return builder;
+		return (majorVersion >= 5 && minorVersion >= 1) || (majorVersion > 5);
 	}
 
 	/**
@@ -361,13 +373,13 @@ private static Collection<Affordance> findAffordances(MethodInvocation invocatio
 	private static class CachingAnnotationMappingDiscoverer implements MappingDiscoverer {
 
 		private final @Delegate AnnotationMappingDiscoverer delegate;
-		private final Map<String, UriTemplate> templates = new ConcurrentReferenceHashMap<>();
+		private final Map<String, UriTemplate> templates = new ConcurrentReferenceHashMap<>();
 
 		public UriTemplate getMappingAsUriTemplate(Class<?> type, Method method) {
 
 			String mapping = delegate.getMapping(type, method);
-			
-			return templates.computeIfAbsent(mapping, UriTemplate::new);
+			
+			return templates.computeIfAbsent(mapping, UriTemplate::new);
 		}
 	}
 

src/main/java/org/springframework/hateoas/mvc/ForwardedHeader.java

@@ -15,38 +15,70 @@
  */
 package org.springframework.hateoas.mvc;
 
+import static org.springframework.util.StringUtils.hasText;
+
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Map;
 import java.util.stream.Collectors;
 
+import javax.servlet.http.HttpServletRequest;
+
 import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
+import org.springframework.web.util.UriComponentsBuilder;
 
 /**
  * Value object to partially implement the {@literal Forwarded} header defined in RFC 7239.
  *
  * @author Oliver Gierke
  * @see http://tools.ietf.org/html/rfc7239
+ * @deprecated In Spring 5.1, all Forwarded headers will by handled by Spring MVC.
  */
+@Deprecated
 class ForwardedHeader {
 
-	public static String NAME = "Forwarded";
 	private static final ForwardedHeader NO_HEADER = new ForwardedHeader(Collections.emptyMap());
-
 	private final Map<String, String> elements;
 
 	private ForwardedHeader(Map<String, String> elements) {
 		this.elements = elements;
 	}
 
+	/**
+	 * Utility method to pull handling of {@literal X-Forwarded-Ssl} into a class that will be removed when
+	 * rebaselined against Spring 5.1
+	 *
+	 * @param request
+	 * @param builder
+	 * @return
+	 * @deprecated No longer needed with Spring 5.1
+	 */
+	@Deprecated
+	public static UriComponentsBuilder handleXForwardedSslHeader(HttpServletRequest request, UriComponentsBuilder builder) {
+
+		// special case handling for X-Forwarded-Ssl:
+		// apply it, but only if X-Forwarded-Proto is unset.
+
+		String forwardedSsl = request.getHeader("X-Forwarded-Ssl");
+		ForwardedHeader forwarded = ForwardedHeader.of(request.getHeader("Forwarded"));
+		String proto = hasText(forwarded.getProto()) ? forwarded.getProto() : request.getHeader("X-Forwarded-Proto");
+
+		if (!hasText(proto) && hasText(forwardedSsl) && forwardedSsl.equalsIgnoreCase("on")) {
+			builder.scheme("https");
+		}
+
+		return builder;
+
+	}
+
 	/**
 	 * Creates a new {@link ForwardedHeader} from the given source.
 	 * 
 	 * @param source can be {@literal null}.
 	 * @return
 	 */
-	public static ForwardedHeader of(String source) {
+	static ForwardedHeader of(String source) {
 
 		if (!StringUtils.hasText(source)) {
 			return NO_HEADER;
@@ -67,7 +99,7 @@ public static ForwardedHeader of(String source) {
 	 * 
 	 * @return
 	 */
-	public String getProto() {
+	String getProto() {
 		return elements.get("proto");
 	}
 
@@ -76,7 +108,7 @@ public String getProto() {
 	 * 
 	 * @return
 	 */
-	public String getHost() {
+	String getHost() {
 		return elements.get("host");
 	}
 }

src/test/java/org/springframework/hateoas/TestUtils.java

@@ -17,10 +17,18 @@
 
 import static org.assertj.core.api.Assertions.*;
 
+import java.io.IOException;
+
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+
 import org.junit.Before;
+import org.springframework.mock.web.MockFilterChain;
 import org.springframework.mock.web.MockHttpServletRequest;
+import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.web.context.request.RequestContextHolder;
 import org.springframework.web.context.request.ServletRequestAttributes;
+import org.springframework.web.filter.ForwardedHeaderFilter;
 
 /**
  * Utility class to ease testing.
@@ -43,6 +51,24 @@ protected void assertPointsToMockServer(Link link) {
 		assertThat(link.getHref()).startsWith("http://localhost");
 	}
 
+	/**
+	 * Provide a mechanism to simulate inserting a {@link ForwardedHeaderFilter} into the servlet
+	 * filter chain, so {@literal Forwarded} headers are properly inserted into the test web request.
+	 *
+	 * @see https://jira.spring.io/browse/SPR-16668
+	 */
+	protected void adaptRequestFromForwardedHeaders() {
+		
+		MockFilterChain chain = new MockFilterChain();
+		try {
+			new ForwardedHeaderFilter().doFilter(this.request, new MockHttpServletResponse(), chain);
+		} catch (ServletException | IOException e) {
+			throw new RuntimeException(e);
+		}
+		HttpServletRequest adaptedRequest = (HttpServletRequest) chain.getRequest();
+		RequestContextHolder.setRequestAttributes(new ServletRequestAttributes(adaptedRequest));
+	}
+
 	public static void assertEqualAndSameHashCode(Object left, Object right) {
 
 		assertThat(left).isEqualTo(right);

src/test/java/org/springframework/hateoas/mvc/ControllerLinkBuilderUnitTest.java

@@ -20,11 +20,14 @@
 import static org.hamcrest.Matchers.*;
 import static org.springframework.hateoas.mvc.ControllerLinkBuilder.*;
 
+import java.io.IOException;
 import java.lang.reflect.Method;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
 
+import javax.servlet.ServletException;
+
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -162,6 +165,8 @@ public void usesForwardedHostAsHostIfHeaderIsSet() {
 
 		request.addHeader("X-Forwarded-Host", "somethingDifferent");
 
+		adaptRequestFromForwardedHeaders();
+
 		Link link = linkTo(PersonControllerImpl.class).withSelfRel();
 		assertThat(link.getHref(), startsWith("http://somethingDifferent"));
 	}
@@ -199,6 +204,8 @@ public void usesForwardedSslAndHostIfHeaderIsSet() {
 		request.addHeader("X-Forwarded-Host", "somethingDifferent");
 		request.addHeader("X-Forwarded-Ssl", "on");
 
+		adaptRequestFromForwardedHeaders();
+
 		Link link = linkTo(PersonControllerImpl.class).withSelfRel();
 		assertThat(link.getHref(), startsWith("https://somethingDifferent"));
 	}
@@ -270,6 +277,8 @@ public void usesForwardedHostAndPortFromHeader() {
 
 		request.addHeader("X-Forwarded-Host", "foobar:8088");
 
+		adaptRequestFromForwardedHeaders();
+
 		Link link = linkTo(PersonControllerImpl.class).withSelfRel();
 		assertThat(link.getHref(), startsWith("http://foobar:8088"));
 	}
@@ -282,6 +291,8 @@ public void usesFirstHostOfXForwardedHost() {
 
 		request.addHeader("X-Forwarded-Host", "barfoo:8888, localhost:8088");
 
+		adaptRequestFromForwardedHeaders();
+
 		Link link = linkTo(PersonControllerImpl.class).withSelfRel();
 		assertThat(link.getHref(), startsWith("http://barfoo:8888"));
 	}
@@ -335,6 +346,8 @@ public void usesForwardedPortFromHeader() {
 		request.addHeader("X-Forwarded-Port", "9090");
 		request.setServerPort(8080);
 
+		adaptRequestFromForwardedHeaders();
+
 		Link link = linkTo(PersonControllerImpl.class).withSelfRel();
 
 		assertThat(link.getHref(), startsWith("http://foobarhost:9090/"));
@@ -349,6 +362,8 @@ public void usesForwardedHostFromHeaderWithDefaultPort() {
 		request.addHeader("X-Forwarded-Host", "foobarhost");
 		request.setServerPort(8080);
 
+		adaptRequestFromForwardedHeaders();
+
 		Link link = linkTo(PersonControllerImpl.class).withSelfRel();
 		assertThat(link.getHref(), startsWith("http://foobarhost/"));
 	}
@@ -410,12 +425,14 @@ public void usesRootMappingOfTargetClassForMethodsOfParent() throws Exception {
 	 * @see #257, #107
 	 */
 	@Test
-	public void usesXForwardedProtoHeaderAsLinkSchema() {
+	public void usesXForwardedProtoHeaderAsLinkSchema() throws ServletException, IOException {
 
 		for (String proto : Arrays.asList("http", "https")) {
 
 			setUp();
 			request.addHeader("X-Forwarded-Proto", proto);
+			
+			adaptRequestFromForwardedHeaders();
 
 			Link link = linkTo(PersonControllerImpl.class).withSelfRel();
 			assertThat(link.getHref(), startsWith(proto + "://"));
@@ -433,6 +450,8 @@ public void usesProtoValueFromForwardedHeaderAsLinkSchema() {
 			setUp();
 			request.addHeader("Forwarded", new String[] { "proto=" + proto });
 
+			adaptRequestFromForwardedHeaders();
+
 			Link link = linkTo(PersonControllerImpl.class).withSelfRel();
 			assertThat(link.getHref(), startsWith(proto.concat("://")));
 		}
@@ -445,7 +464,9 @@ public void usesProtoValueFromForwardedHeaderAsLinkSchema() {
 	public void favorsStandardForwardHeaderOverXForwardedProto() {
 
 		request.addHeader("X-Forwarded-Proto", "foo");
-		request.addHeader(ForwardedHeader.NAME, "proto=bar");
+		request.addHeader("Forwarded", "proto=bar");
+		
+		adaptRequestFromForwardedHeaders();
 
 		Link link = linkTo(PersonControllerImpl.class).withSelfRel();
 		assertThat(link.getHref(), startsWith("bar://"));
@@ -527,6 +548,8 @@ public void supportsTwoProxiesAddingXForwardedPort() {
 		request.addHeader("X-Forwarded-Port", "1443,8443");
 		request.addHeader("X-Forwarded-Host", "proxy1,proxy2");
 
+		adaptRequestFromForwardedHeaders();
+
 		assertThat(linkTo(PersonControllerImpl.class).withSelfRel().getHref(), startsWith("http://proxy1:1443"));
 	}