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