From f95bf96ec903c1b227ba452c0d5e7a2d754afe4a Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Mon, 23 May 2022 11:03:42 +0100 Subject: [PATCH] Correct path encoding/decoding in PathResourceResolver Decoding is required for non-UrlResource when the HandlerMapping is not expected to decode the path. Encoding is the opposite. This commit ensures correct determination of whether the HandlerMapping is expected to have decoded the path or not that in turn depends on whether PathPattern or PathMatcher is in use. Closes gh-27791 --- .../resource/PathResourceResolver.java | 59 ++++++++++------- ...rceHttpRequestHandlerIntegrationTests.java | 63 +++++++++++++++---- 2 files changed, 86 insertions(+), 36 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/PathResourceResolver.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/PathResourceResolver.java index cc042115d8..b6ca8cfb23 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/PathResourceResolver.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/PathResourceResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -260,36 +260,47 @@ public class PathResourceResolver extends AbstractResourceResolver { } private String encodeOrDecodeIfNecessary(String path, @Nullable HttpServletRequest request, Resource location) { - if (shouldDecodeRelativePath(location, request)) { - return UriUtils.decode(path, StandardCharsets.UTF_8); - } - else if (shouldEncodeRelativePath(location) && request != null) { - Charset charset = this.locationCharsets.getOrDefault(location, StandardCharsets.UTF_8); - StringBuilder sb = new StringBuilder(); - StringTokenizer tokenizer = new StringTokenizer(path, "/"); - while (tokenizer.hasMoreTokens()) { - String value = UriUtils.encode(tokenizer.nextToken(), charset); - sb.append(value); - sb.append('/'); + if (request != null) { + boolean usesPathPattern = ( + ServletRequestPathUtils.hasCachedPath(request) && + ServletRequestPathUtils.getCachedPath(request) instanceof PathContainer); + + if (shouldDecodeRelativePath(location, usesPathPattern)) { + return UriUtils.decode(path, StandardCharsets.UTF_8); } - if (!path.endsWith("/")) { - sb.setLength(sb.length() - 1); + else if (shouldEncodeRelativePath(location, usesPathPattern)) { + Charset charset = this.locationCharsets.getOrDefault(location, StandardCharsets.UTF_8); + StringBuilder sb = new StringBuilder(); + StringTokenizer tokenizer = new StringTokenizer(path, "/"); + while (tokenizer.hasMoreTokens()) { + String value = UriUtils.encode(tokenizer.nextToken(), charset); + sb.append(value); + sb.append('/'); + } + if (!path.endsWith("/")) { + sb.setLength(sb.length() - 1); + } + return sb.toString(); } - return sb.toString(); - } - else { - return path; } + return path; } - private boolean shouldDecodeRelativePath(Resource location, @Nullable HttpServletRequest request) { - return (!(location instanceof UrlResource) && request != null && - ServletRequestPathUtils.hasCachedPath(request) && - ServletRequestPathUtils.getCachedPath(request) instanceof PathContainer); + /** + * When the {@code HandlerMapping} is set to not decode the URL path, the + * path needs to be decoded for non-{@code UrlResource} locations. + */ + private boolean shouldDecodeRelativePath(Resource location, boolean usesPathPattern) { + return (!(location instanceof UrlResource) && + (usesPathPattern || (this.urlPathHelper != null && !this.urlPathHelper.isUrlDecode()))); } - private boolean shouldEncodeRelativePath(Resource location) { - return (location instanceof UrlResource && + /** + * When the {@code HandlerMapping} is set to decode the URL path, the path + * needs to be encoded for {@code UrlResource} locations. + */ + private boolean shouldEncodeRelativePath(Resource location, boolean usesPathPattern) { + return (location instanceof UrlResource && !usesPathPattern && this.urlPathHelper != null && this.urlPathHelper.isUrlDecode()); } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerIntegrationTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerIntegrationTests.java index ef3e1f6104..b03b45f691 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerIntegrationTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerIntegrationTests.java @@ -41,6 +41,7 @@ import org.springframework.web.testfixture.servlet.MockHttpServletResponse; import org.springframework.web.testfixture.servlet.MockServletConfig; import org.springframework.web.testfixture.servlet.MockServletContext; import org.springframework.web.util.UriUtils; +import org.springframework.web.util.UrlPathHelper; import org.springframework.web.util.pattern.PathPatternParser; import static org.assertj.core.api.Assertions.assertThat; @@ -60,23 +61,35 @@ public class ResourceHttpRequestHandlerIntegrationTests { public static Stream argumentSource() { return Stream.of( - arguments(true, "/cp"), - arguments(true, "/fs"), - arguments(true, "/url"), - arguments(false, "/cp"), - arguments(false, "/fs"), - arguments(false, "/url") + // PathPattern + arguments(true, true, "/cp"), + arguments(true, true, "/fs"), + arguments(true, true, "/url"), + + arguments(true, false, "/cp"), + arguments(true, false, "/fs"), + arguments(true, false, "/url"), + + // PathMatcher + arguments(false, true, "/cp"), + arguments(false, true, "/fs"), + arguments(false, true, "/url"), + + arguments(false, false, "/cp"), + arguments(false, false, "/fs"), + arguments(false, false, "/url") ); } @ParameterizedTest @MethodSource("argumentSource") - void cssFile(boolean usePathPatterns, String pathPrefix) throws Exception { + void cssFile(boolean usePathPatterns, boolean decodingUrlPathHelper, String pathPrefix) throws Exception { + MockHttpServletRequest request = initRequest(pathPrefix + "/test/foo.css"); MockHttpServletResponse response = new MockHttpServletResponse(); - DispatcherServlet servlet = initDispatcherServlet(usePathPatterns, WebConfig.class); + DispatcherServlet servlet = initDispatcherServlet(usePathPatterns, decodingUrlPathHelper, WebConfig.class); servlet.service(request, response); String description = "usePathPattern=" + usePathPatterns + ", prefix=" + pathPrefix; @@ -87,11 +100,13 @@ public class ResourceHttpRequestHandlerIntegrationTests { @ParameterizedTest @MethodSource("argumentSource") // gh-26775 - void classpathLocationWithEncodedPath(boolean usePathPatterns, String pathPrefix) throws Exception { + void classpathLocationWithEncodedPath( + boolean usePathPatterns, boolean decodingUrlPathHelper, String pathPrefix) throws Exception { + MockHttpServletRequest request = initRequest(pathPrefix + "/test/foo with spaces.css"); MockHttpServletResponse response = new MockHttpServletResponse(); - DispatcherServlet servlet = initDispatcherServlet(usePathPatterns, WebConfig.class); + DispatcherServlet servlet = initDispatcherServlet(usePathPatterns, decodingUrlPathHelper, WebConfig.class); servlet.service(request, response); String description = "usePathPattern=" + usePathPatterns + ", prefix=" + pathPrefix; @@ -100,14 +115,16 @@ public class ResourceHttpRequestHandlerIntegrationTests { assertThat(response.getContentAsString()).as(description).isEqualTo("h1 { color:red; }"); } - private DispatcherServlet initDispatcherServlet(boolean usePathPatterns, Class... configClasses) - throws ServletException { + private DispatcherServlet initDispatcherServlet( + boolean usePathPatterns, boolean decodingUrlPathHelper, Class... configClasses) throws ServletException { AnnotationConfigWebApplicationContext context = new AnnotationConfigWebApplicationContext(); context.register(configClasses); if (usePathPatterns) { context.register(PathPatternParserConfig.class); } + context.register(decodingUrlPathHelper ? + DecodingUrlPathHelperConfig.class : NonDecodingUrlPathHelperConfig.class); context.setServletConfig(this.servletConfig); context.refresh(); @@ -171,4 +188,26 @@ public class ResourceHttpRequestHandlerIntegrationTests { } } + + static class DecodingUrlPathHelperConfig implements WebMvcConfigurer { + + @Override + public void configurePathMatch(PathMatchConfigurer configurer) { + UrlPathHelper helper = new UrlPathHelper(); + helper.setUrlDecode(true); + configurer.setUrlPathHelper(helper); + } + } + + + static class NonDecodingUrlPathHelperConfig implements WebMvcConfigurer { + + @Override + public void configurePathMatch(PathMatchConfigurer configurer) { + UrlPathHelper helper = new UrlPathHelper(); + helper.setUrlDecode(false); + configurer.setUrlPathHelper(helper); + } + } + }