From 9d13ea290f54fc7d4616418bc9f91db8cf6b664e Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 22 Dec 2023 17:27:06 +0100 Subject: [PATCH] Reject invalid forwarded requests in ForwardedHeaderFilter Prior to this commit, the `ForwardedHeaderFilter` and the forwarded header utils would throw `IllegalArgumentException` and `IllegalStateException` when request headers are invalid and cannot be parsed for Forwarded handling. This commit aligns the behavior with the WebFlux counterpart by rejecting such requests with HTTP 400 responses directly. Fixes gh-31894 --- .../web/filter/ForwardedHeaderFilter.java | 36 ++++++++++++---- .../filter/ForwardedHeaderFilterTests.java | 41 +++++++++++++++---- 2 files changed, 63 insertions(+), 14 deletions(-) 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 abb2b469b00..5c631fcd8dc 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 @@ -30,6 +30,8 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletRequestWrapper; import jakarta.servlet.http.HttpServletResponse; import jakarta.servlet.http.HttpServletResponseWrapper; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.springframework.http.HttpStatus; import org.springframework.http.server.ServerHttpRequest; @@ -64,11 +66,14 @@ import org.springframework.web.util.UrlPathHelper; * @author Rossen Stoyanchev * @author Eddú Meléndez * @author Rob Winch + * @author Brian Clozel * @since 4.3 * @see https://tools.ietf.org/html/rfc7239 */ public class ForwardedHeaderFilter extends OncePerRequestFilter { + private static final Log logger = LogFactory.getLog(ForwardedHeaderFilter.class); + private static final Set FORWARDED_HEADER_NAMES = Collections.newSetFromMap(new LinkedCaseInsensitiveMap<>(10, Locale.ENGLISH)); @@ -143,17 +148,34 @@ public class ForwardedHeaderFilter extends OncePerRequestFilter { filterChain.doFilter(wrappedRequest, response); } else { - HttpServletRequest wrappedRequest = - new ForwardedHeaderExtractingRequest(request); - - HttpServletResponse wrappedResponse = this.relativeRedirects ? - RelativeRedirectResponseWrapper.wrapIfNecessary(response, HttpStatus.SEE_OTHER) : - new ForwardedHeaderExtractingResponse(response, wrappedRequest); - + HttpServletRequest wrappedRequest = null; + HttpServletResponse wrappedResponse = null; + try { + wrappedRequest = new ForwardedHeaderExtractingRequest(request); + wrappedResponse = this.relativeRedirects ? + RelativeRedirectResponseWrapper.wrapIfNecessary(response, HttpStatus.SEE_OTHER) : + new ForwardedHeaderExtractingResponse(response, wrappedRequest); + } + catch (Throwable ex) { + if (logger.isDebugEnabled()) { + logger.debug("Failed to apply forwarded headers to " + formatRequest(request), ex); + } + response.sendError(HttpServletResponse.SC_BAD_REQUEST); + return; + } filterChain.doFilter(wrappedRequest, wrappedResponse); } } + /** + * Format the request for logging purposes including HTTP method and URL. + * @param request the request to format + * @return the String to display, never empty or {@code null} + */ + protected String formatRequest(HttpServletRequest request) { + return "HTTP " + request.getMethod() + " \"" + request.getRequestURI() + "\""; + } + @Override protected void doFilterNestedErrorDispatch(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { 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 bf4a4c722ff..2fae32527b8 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 @@ -32,12 +32,12 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.http.HttpStatus; import org.springframework.web.testfixture.servlet.MockFilterChain; import org.springframework.web.testfixture.servlet.MockHttpServletRequest; import org.springframework.web.testfixture.servlet.MockHttpServletResponse; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.mockito.Mockito.mock; /** @@ -46,6 +46,7 @@ import static org.mockito.Mockito.mock; * @author Rossen Stoyanchev * @author Eddú Meléndez * @author Rob Winch + * @author Brian Clozel */ public class ForwardedHeaderFilterTests { @@ -208,6 +209,38 @@ public class ForwardedHeaderFilterTests { assertThat(actual.getRequestURL().toString()).isEqualTo("https://www.mycompany.example/bar"); } + @Nested // gh-31842 + class InvalidRequests { + + @Test + void shouldRejectInvalidForwardedForIpv4() throws Exception { + request.addHeader(FORWARDED, "for=127.0.0.1:"); + + MockHttpServletResponse response = new MockHttpServletResponse(); + filter.doFilter(request, response, filterChain); + assertThat(response.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value()); + } + + @Test + void shouldRejectInvalidForwardedForIpv6() throws Exception { + request.addHeader(FORWARDED, "for=\"2a02:918:175:ab60:45ee:c12c:dac1:808b\""); + + MockHttpServletResponse response = new MockHttpServletResponse(); + filter.doFilter(request, response, filterChain); + assertThat(response.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value()); + } + + @Test + void shouldRejectInvalidForwardedPort() throws Exception { + request.addHeader(X_FORWARDED_PORT, "invalid"); + + MockHttpServletResponse response = new MockHttpServletResponse(); + filter.doFilter(request, response, filterChain); + assertThat(response.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value()); + } + + } + @Nested class ForwardedPrefix { @@ -474,12 +507,6 @@ public class ForwardedHeaderFilterTests { assertThat(actual.getRemotePort()).isEqualTo(MockHttpServletRequest.DEFAULT_SERVER_PORT); } - @Test // gh-26748 - public void forwardedForInvalidIpV6Address() { - request.addHeader(FORWARDED, "for=\"2a02:918:175:ab60:45ee:c12c:dac1:808b\""); - assertThatIllegalArgumentException().isThrownBy( - ForwardedHeaderFilterTests.this::filterAndGetWrappedRequest); - } } @Nested