From 5a858915ea343acb1a3e392574f416662c2e7aad Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Wed, 24 Sep 2025 18:47:09 +0100 Subject: [PATCH 1/2] Revise parsed path handling in UrlHandlerFilter Closes gh-35538 --- .../web/filter/UrlHandlerFilter.java | 53 +++++++-------- .../web/util/ServletRequestPathUtils.java | 17 +++-- .../web/filter/UrlHandlerFilterTests.java | 64 +++++++++++++++++++ 3 files changed, 97 insertions(+), 37 deletions(-) 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 3f299b87dec..e8008d3fa4c 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 @@ -74,41 +74,23 @@ 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); } } @@ -349,7 +331,18 @@ 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); + if (previousPath != null) { + ServletRequestPathUtils.parseAndCache(request); + } + try { + chain.doFilter(request, response); + } + finally { + if (previousPath != null) { + ServletRequestPathUtils.setParsedRequestPath(previousPath, request); + } + } } } 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 e191ede839b..05cd8b3661e 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 5bcf0b76e09..0f3701dd63b 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 @@ -19,7 +19,9 @@ package org.springframework.web.filter; import java.io.IOException; import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; import org.junit.jupiter.api.Test; import org.springframework.http.HttpHeaders; @@ -29,6 +31,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 +127,65 @@ public class UrlHandlerFilterTests { assertThat(response.isCommitted()).isFalse(); } + @Test + void shouldNotFilterErrorAndAsyncDispatches() { + UrlHandlerFilter filter = UrlHandlerFilter.trailingSlashHandler("/path/**").wrapRequest().build(); + + assertThat(filter.shouldNotFilterAsyncDispatch()) + .as("Should not filter async dispatch: wrapped request is reused") + .isTrue(); + + assertThat(filter.shouldNotFilterErrorDispatch()) + .as("Should not filter error dispatch: it's a different path") + .isTrue(); + } + + @Test + 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 + void shouldReplaceCachedPath() 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/"); + + PathSavingServlet servlet = new PathSavingServlet(); + MockFilterChain chain = new MockFilterChain(servlet); + filter.doFilterInternal(request, new MockHttpServletResponse(), chain); + + assertThat(servlet.getParsedPath()).isEqualTo("/path/123"); + assertThat(ServletRequestPathUtils.getParsedRequestPath(request).value()).isEqualTo("/path/123/"); + } + + + private static class PathSavingServlet extends HttpServlet { + + private String parsedPath; + + public String getParsedPath() { + return parsedPath; + } + + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + this.parsedPath = ServletRequestPathUtils.getParsedRequestPath(request).value(); + } + } + } From e9fb5eb38aeb7f422b19ed405ff6750574625f8c Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Thu, 25 Sep 2025 12:19:56 +0100 Subject: [PATCH 2/2] Respect forwarded path in UrlHandlerFilter Closes gh-35509 --- .../web/filter/UrlHandlerFilter.java | 29 ++++++----- .../web/filter/UrlHandlerFilterTests.java | 48 ++++++++++++++----- 2 files changed, 54 insertions(+), 23 deletions(-) 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 e8008d3fa4c..cc7d206d6df 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 @@ -332,9 +333,7 @@ public final class UrlHandlerFilter extends OncePerRequestFilter { hasPathInfo ? trimTrailingSlash(pathInfo) : pathInfo); RequestPath previousPath = (RequestPath) request.getAttribute(ServletRequestPathUtils.PATH_ATTRIBUTE); - if (previousPath != null) { - ServletRequestPathUtils.parseAndCache(request); - } + ServletRequestPathUtils.clearParsedRequestPath(request); try { chain.doFilter(request, response); } @@ -372,22 +371,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/test/java/org/springframework/web/filter/UrlHandlerFilterTests.java b/spring-web/src/test/java/org/springframework/web/filter/UrlHandlerFilterTests.java index 0f3701dd63b..efdc1ce0b6e 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,6 +18,7 @@ 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; @@ -127,20 +128,20 @@ public class UrlHandlerFilterTests { assertThat(response.isCommitted()).isFalse(); } - @Test + @Test // gh-35538 void shouldNotFilterErrorAndAsyncDispatches() { UrlHandlerFilter filter = UrlHandlerFilter.trailingSlashHandler("/path/**").wrapRequest().build(); assertThat(filter.shouldNotFilterAsyncDispatch()) - .as("Should not filter async dispatch: wrapped request is reused") + .as("Should not filter ASYNC dispatch as wrapped request is reused") .isTrue(); assertThat(filter.shouldNotFilterErrorDispatch()) - .as("Should not filter error dispatch: it's a different path") + .as("Should not filter ERROR dispatch as it's an internal, fixed path") .isTrue(); } - @Test + @Test // gh-35538 void shouldNotCacheParsedPath() throws Exception { UrlHandlerFilter filter = UrlHandlerFilter.trailingSlashHandler("/path/*").wrapRequest().build(); @@ -155,8 +156,8 @@ public class UrlHandlerFilterTests { .isFalse(); } - @Test - void shouldReplaceCachedPath() throws Exception { + @Test // gh-35538 + void shouldClearPreviouslyCachedPath() throws Exception { UrlHandlerFilter filter = UrlHandlerFilter.trailingSlashHandler("/path/*").wrapRequest().build(); MockHttpServletRequest request = new MockHttpServletRequest("GET", "/path/123/"); @@ -165,16 +166,38 @@ public class UrlHandlerFilterTests { ServletRequestPathUtils.parseAndCache(request); assertThat(ServletRequestPathUtils.getParsedRequestPath(request).value()).isEqualTo("/path/123/"); - PathSavingServlet servlet = new PathSavingServlet(); + PathServlet servlet = new PathServlet(); MockFilterChain chain = new MockFilterChain(servlet); filter.doFilterInternal(request, new MockHttpServletResponse(), chain); - assertThat(servlet.getParsedPath()).isEqualTo("/path/123"); - assertThat(ServletRequestPathUtils.getParsedRequestPath(request).value()).isEqualTo("/path/123/"); + 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); } - private static class PathSavingServlet extends HttpServlet { + @SuppressWarnings("serial") + private static class PathServlet extends HttpServlet { private String parsedPath; @@ -183,8 +206,9 @@ public class UrlHandlerFilterTests { } @Override - protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { - this.parsedPath = ServletRequestPathUtils.getParsedRequestPath(request).value(); + protected void doGet(HttpServletRequest request, HttpServletResponse response) { + this.parsedPath = (ServletRequestPathUtils.hasParsedRequestPath(request) ? + ServletRequestPathUtils.getParsedRequestPath(request).value() : null); } }