diff --git a/spring-web/src/main/java/org/springframework/web/filter/UrlHandlerFilter.java b/spring-web/src/main/java/org/springframework/web/filter/UrlHandlerFilter.java index ccc465b0c7d..4112fee6099 100644 --- a/spring-web/src/main/java/org/springframework/web/filter/UrlHandlerFilter.java +++ b/spring-web/src/main/java/org/springframework/web/filter/UrlHandlerFilter.java @@ -22,6 +22,7 @@ import java.util.List; import java.util.Map; import java.util.function.Consumer; +import jakarta.servlet.DispatcherType; import jakarta.servlet.FilterChain; import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; @@ -43,8 +44,8 @@ import org.springframework.web.util.pattern.PathPattern; import org.springframework.web.util.pattern.PathPatternParser; /** - * {@link jakarta.servlet.Filter} that modifies the URL, and then redirects or - * wraps the request to apply the change. + * {@link jakarta.servlet.Filter} that modifies the URL, and then either + * redirects or wraps the request to effect the change. * *

To create an instance, you can use the following: * @@ -55,8 +56,8 @@ import org.springframework.web.util.pattern.PathPatternParser; * .build(); * * - *

This {@code Filter} should be ordered after {@link ForwardedHeaderFilter} - * and before any security filters. + *

This {@code Filter} should be ordered after {@link ForwardedHeaderFilter}, + * before {@link ServletRequestPathFilter}, and before security filters. * * @author Rossen Stoyanchev * @since 6.2 @@ -74,43 +75,25 @@ public final class UrlHandlerFilter extends OncePerRequestFilter { } - @Override - protected boolean shouldNotFilterAsyncDispatch() { - return false; - } - - @Override - protected boolean shouldNotFilterErrorDispatch() { - return false; - } - @Override protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws ServletException, IOException { - RequestPath previousPath = (RequestPath) request.getAttribute(ServletRequestPathUtils.PATH_ATTRIBUTE); - RequestPath path = previousPath; - try { - if (path == null) { - path = ServletRequestPathUtils.parseAndCache(request); + RequestPath path = (ServletRequestPathUtils.hasParsedRequestPath(request) ? + ServletRequestPathUtils.getParsedRequestPath(request) : + ServletRequestPathUtils.parse(request)); + + for (Map.Entry> entry : this.handlers.entrySet()) { + if (!entry.getKey().supports(request, path)) { + continue; } - for (Map.Entry> entry : this.handlers.entrySet()) { - if (!entry.getKey().supports(request, path)) { - continue; - } - for (PathPattern pattern : entry.getValue()) { - if (pattern.matches(path)) { - entry.getKey().handle(request, response, chain); - return; - } + for (PathPattern pattern : entry.getValue()) { + if (pattern.matches(path)) { + entry.getKey().handle(request, response, chain); + return; } } } - finally { - if (previousPath != null) { - ServletRequestPathUtils.setParsedRequestPath(previousPath, request); - } - } chain.doFilter(request, response); } @@ -348,7 +331,16 @@ public final class UrlHandlerFilter extends OncePerRequestFilter { hasPathInfo ? servletPath : trimTrailingSlash(servletPath), hasPathInfo ? trimTrailingSlash(pathInfo) : pathInfo); - chain.doFilter(request, response); + RequestPath previousPath = (RequestPath) request.getAttribute(ServletRequestPathUtils.PATH_ATTRIBUTE); + ServletRequestPathUtils.clearParsedRequestPath(request); + try { + chain.doFilter(request, response); + } + finally { + if (previousPath != null) { + ServletRequestPathUtils.setParsedRequestPath(previousPath, request); + } + } } } @@ -378,22 +370,30 @@ public final class UrlHandlerFilter extends OncePerRequestFilter { @Override public String getRequestURI() { - return this.requestURI; + return (isForward() ? getDelegate().getRequestURI() : this.requestURI); } @Override public StringBuffer getRequestURL() { - return this.requestURL; + return (isForward() ? getDelegate().getRequestURL() : this.requestURL); } @Override public String getServletPath() { - return this.servletPath; + return (isForward() ? getDelegate().getServletPath() : this.servletPath); } @Override public String getPathInfo() { - return this.pathInfo; + return (isForward() ? getDelegate().getPathInfo() : this.pathInfo); + } + + private boolean isForward() { + return (getDispatcherType() == DispatcherType.FORWARD); + } + + private HttpServletRequest getDelegate() { + return (HttpServletRequest) getRequest(); } } diff --git a/spring-web/src/main/java/org/springframework/web/util/ServletRequestPathUtils.java b/spring-web/src/main/java/org/springframework/web/util/ServletRequestPathUtils.java index 95ff9627d6c..80cc2e772e9 100644 --- a/spring-web/src/main/java/org/springframework/web/util/ServletRequestPathUtils.java +++ b/spring-web/src/main/java/org/springframework/web/util/ServletRequestPathUtils.java @@ -52,16 +52,19 @@ public abstract class ServletRequestPathUtils { /** * Parse the {@link HttpServletRequest#getRequestURI() requestURI} to a - * {@link RequestPath} and save it in the request attribute - * {@link #PATH_ATTRIBUTE} for subsequent use with - * {@link org.springframework.web.util.pattern.PathPattern parsed patterns}. + * {@link RequestPath}. *

The returned {@code RequestPath} will have both the contextPath and any * servletPath prefix omitted from the {@link RequestPath#pathWithinApplication() * pathWithinApplication} it exposes. - *

This method is typically called by the {@code DispatcherServlet} to determine - * if any {@code HandlerMapping} indicates that it uses parsed patterns. - * After that the pre-parsed and cached {@code RequestPath} can be accessed - * through {@link #getParsedRequestPath(ServletRequest)}. + * @since 6.2.12 + */ + public static RequestPath parse(HttpServletRequest request) { + return ServletRequestPath.parse(request); + } + + /** + * Variant of {@link #parse(HttpServletRequest)} that also saves the parsed + * path in the request attribute {@link #PATH_ATTRIBUTE}. */ public static RequestPath parseAndCache(HttpServletRequest request) { RequestPath requestPath = ServletRequestPath.parse(request); diff --git a/spring-web/src/test/java/org/springframework/web/filter/UrlHandlerFilterTests.java b/spring-web/src/test/java/org/springframework/web/filter/UrlHandlerFilterTests.java index 2816620ce6d..df01c8f09bd 100644 --- a/spring-web/src/test/java/org/springframework/web/filter/UrlHandlerFilterTests.java +++ b/spring-web/src/test/java/org/springframework/web/filter/UrlHandlerFilterTests.java @@ -18,8 +18,11 @@ package org.springframework.web.filter; import java.io.IOException; +import jakarta.servlet.DispatcherType; import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; @@ -29,6 +32,7 @@ import org.springframework.util.StringUtils; import org.springframework.web.testfixture.servlet.MockFilterChain; import org.springframework.web.testfixture.servlet.MockHttpServletRequest; import org.springframework.web.testfixture.servlet.MockHttpServletResponse; +import org.springframework.web.util.ServletRequestPathUtils; import static org.assertj.core.api.Assertions.assertThat; @@ -124,4 +128,88 @@ public class UrlHandlerFilterTests { assertThat(response.isCommitted()).isFalse(); } + @Test // gh-35538 + void shouldNotFilterErrorAndAsyncDispatches() { + UrlHandlerFilter filter = UrlHandlerFilter.trailingSlashHandler("/path/**").wrapRequest().build(); + + assertThat(filter.shouldNotFilterAsyncDispatch()) + .as("Should not filter ASYNC dispatch as wrapped request is reused") + .isTrue(); + + assertThat(filter.shouldNotFilterErrorDispatch()) + .as("Should not filter ERROR dispatch as it's an internal, fixed path") + .isTrue(); + } + + @Test // gh-35538 + void shouldNotCacheParsedPath() throws Exception { + UrlHandlerFilter filter = UrlHandlerFilter.trailingSlashHandler("/path/*").wrapRequest().build(); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/path/123/"); + request.setServletPath("/path/123/"); + + MockFilterChain chain = new MockFilterChain(); + filter.doFilterInternal(request, new MockHttpServletResponse(), chain); + + assertThat(ServletRequestPathUtils.hasParsedRequestPath(request)) + .as("Path with trailing slash should not be cached") + .isFalse(); + } + + @Test // gh-35538 + void shouldClearPreviouslyCachedPath() throws Exception { + UrlHandlerFilter filter = UrlHandlerFilter.trailingSlashHandler("/path/*").wrapRequest().build(); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/path/123/"); + request.setServletPath("/path/123/"); + + ServletRequestPathUtils.parseAndCache(request); + assertThat(ServletRequestPathUtils.getParsedRequestPath(request).value()).isEqualTo("/path/123/"); + + PathServlet servlet = new PathServlet(); + MockFilterChain chain = new MockFilterChain(servlet); + filter.doFilterInternal(request, new MockHttpServletResponse(), chain); + + assertThat(servlet.getParsedPath()).isNull(); + } + + @Test // gh-35509 + void shouldRespectForwardedPath() throws Exception { + UrlHandlerFilter filter = UrlHandlerFilter.trailingSlashHandler("/requestURI/*").wrapRequest().build(); + + String requestURI = "/requestURI/123/"; + MockHttpServletRequest originalRequest = new MockHttpServletRequest("GET", requestURI); + originalRequest.setServletPath(requestURI); + + MockFilterChain chain = new MockFilterChain(); + filter.doFilterInternal(originalRequest, new MockHttpServletResponse(), chain); + + HttpServletRequest wrapped = (HttpServletRequest) chain.getRequest(); + assertThat(wrapped).isNotNull().isNotSameAs(originalRequest); + assertThat(wrapped.getRequestURI()).isEqualTo("/requestURI/123"); + + // Change dispatcher type of underlying requests + originalRequest.setDispatcherType(DispatcherType.FORWARD); + assertThat(wrapped.getRequestURI()) + .as("Should delegate to underlying request for the requestURI on FORWARD") + .isEqualTo(requestURI); + } + + + @SuppressWarnings("serial") + private static class PathServlet extends HttpServlet { + + private String parsedPath; + + public String getParsedPath() { + return parsedPath; + } + + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) { + this.parsedPath = (ServletRequestPathUtils.hasParsedRequestPath(request) ? + ServletRequestPathUtils.getParsedRequestPath(request).value() : null); + } + } + }