diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/resource/ResourceWebHandler.java b/spring-webflux/src/main/java/org/springframework/web/reactive/resource/ResourceWebHandler.java index 62641b9e301..215a50f6c8d 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/resource/ResourceWebHandler.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/resource/ResourceWebHandler.java @@ -328,7 +328,15 @@ public class ResourceWebHandler implements WebHandler, InitializingBean { if (path.contains("%")) { try { // Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars - if (isInvalidPath(URLDecoder.decode(path, "UTF-8"))) { + String decodedPath = URLDecoder.decode(path, "UTF-8"); + if (isInvalidPath(decodedPath)) { + if (logger.isTraceEnabled()) { + logger.trace("Ignoring invalid resource path with escape sequences [" + path + "]."); + } + return Mono.empty(); + } + decodedPath = processPath(decodedPath); + if (isInvalidPath(decodedPath)) { if (logger.isTraceEnabled()) { logger.trace("Ignoring invalid resource path with escape sequences [" + path + "]."); } @@ -352,12 +360,47 @@ public class ResourceWebHandler implements WebHandler, InitializingBean { } /** - * Process the given resource path to be used. - *

The default implementation replaces any combination of leading '/' and - * control characters (00-1F and 7F) with a single "/" or "". For example - * {@code " // /// //// foo/bar"} becomes {@code "/foo/bar"}. + * Process the given resource path. + *

The default implementation replaces: + *

+ * @since 3.2.12 */ protected String processPath(String path) { + path = StringUtils.replace(path, "\\", "/"); + path = cleanDuplicateSlashes(path); + return cleanLeadingSlash(path); + } + + private String cleanDuplicateSlashes(String path) { + StringBuilder sb = null; + char prev = 0; + for (int i = 0; i < path.length(); i++) { + char curr = path.charAt(i); + try { + if ((curr == '/') && (prev == '/')) { + if (sb == null) { + sb = new StringBuilder(path.substring(0, i)); + } + continue; + } + if (sb != null) { + sb.append(path.charAt(i)); + } + } + finally { + prev = curr; + } + } + return sb != null ? sb.toString() : path; + } + + private String cleanLeadingSlash(String path) { boolean slash = false; for (int i = 0; i < path.length(); i++) { if (path.charAt(i) == '/') { diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/resource/ResourceWebHandlerTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/resource/ResourceWebHandlerTests.java index baf342ef637..1dceef61f4c 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/resource/ResourceWebHandlerTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/resource/ResourceWebHandlerTests.java @@ -54,13 +54,10 @@ import org.springframework.web.server.MethodNotAllowedException; import org.springframework.web.server.ResponseStatusException; import org.springframework.web.server.ServerWebExchange; -import static org.hamcrest.Matchers.instanceOf; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertSame; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.*; /** * Unit tests for {@link ResourceWebHandler}. @@ -240,39 +237,85 @@ public class ResourceWebHandlerTests { } @Test - public void invalidPath() throws Exception { + public void testInvalidPath() throws Exception { + + // Use mock ResourceResolver: i.e. we're only testing upfront validations... + + Resource resource = mock(Resource.class); + when(resource.getFilename()).thenThrow(new AssertionError("Resource should not be resolved")); + when(resource.getInputStream()).thenThrow(new AssertionError("Resource should not be resolved")); + ResourceResolver resolver = mock(ResourceResolver.class); + when(resolver.resolveResource(any(), any(), any(), any())).thenReturn(Mono.just(resource)); + + ResourceWebHandler handler = new ResourceWebHandler(); + handler.setLocations(Collections.singletonList(new ClassPathResource("test/", getClass()))); + handler.setResourceResolvers(Collections.singletonList(resolver)); + handler.afterPropertiesSet(); + + testInvalidPath("../testsecret/secret.txt", handler); + testInvalidPath("test/../../testsecret/secret.txt", handler); + testInvalidPath(":/../../testsecret/secret.txt", handler); + + Resource location = new UrlResource(getClass().getResource("./test/")); + this.handler.setLocations(Collections.singletonList(location)); + Resource secretResource = new UrlResource(getClass().getResource("testsecret/secret.txt")); + String secretPath = secretResource.getURL().getPath(); + + testInvalidPath("file:" + secretPath, handler); + testInvalidPath("/file:" + secretPath, handler); + testInvalidPath("url:" + secretPath, handler); + testInvalidPath("/url:" + secretPath, handler); + testInvalidPath("/../.." + secretPath, handler); + testInvalidPath("/%2E%2E/testsecret/secret.txt", handler); + testInvalidPath("/%2E%2E/testsecret/secret.txt", handler); + testInvalidPath("%2F%2F%2E%2E%2F%2F%2E%2E" + secretPath, handler); + } + + private void testInvalidPath(String requestPath, ResourceWebHandler handler) { + ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("")); + setPathWithinHandlerMapping(exchange, requestPath); + StepVerifier.create(handler.handle(exchange)) + .expectErrorSatisfies(err -> { + assertThat(err, instanceOf(ResponseStatusException.class)); + assertEquals(HttpStatus.NOT_FOUND, ((ResponseStatusException) err).getStatus()); + }).verify(TIMEOUT); + } + + @Test + public void testResolvePathWithTraversal() throws Exception { for (HttpMethod method : HttpMethod.values()) { - testInvalidPath(method); + testResolvePathWithTraversal(method); } } - private void testInvalidPath(HttpMethod httpMethod) throws Exception { + private void testResolvePathWithTraversal(HttpMethod httpMethod) throws Exception { Resource location = new ClassPathResource("test/", getClass()); this.handler.setLocations(Collections.singletonList(location)); - testInvalidPath(httpMethod, "../testsecret/secret.txt", location); - testInvalidPath(httpMethod, "test/../../testsecret/secret.txt", location); - testInvalidPath(httpMethod, ":/../../testsecret/secret.txt", location); + testResolvePathWithTraversal(httpMethod, "../testsecret/secret.txt", location); + testResolvePathWithTraversal(httpMethod, "test/../../testsecret/secret.txt", location); + testResolvePathWithTraversal(httpMethod, ":/../../testsecret/secret.txt", location); location = new UrlResource(getClass().getResource("./test/")); this.handler.setLocations(Collections.singletonList(location)); Resource secretResource = new UrlResource(getClass().getResource("testsecret/secret.txt")); String secretPath = secretResource.getURL().getPath(); - testInvalidPath(httpMethod, "file:" + secretPath, location); - testInvalidPath(httpMethod, "/file:" + secretPath, location); - testInvalidPath(httpMethod, "url:" + secretPath, location); - testInvalidPath(httpMethod, "/url:" + secretPath, location); - testInvalidPath(httpMethod, "////../.." + secretPath, location); - testInvalidPath(httpMethod, "/%2E%2E/testsecret/secret.txt", location); - testInvalidPath(httpMethod, "url:" + secretPath, location); + testResolvePathWithTraversal(httpMethod, "file:" + secretPath, location); + testResolvePathWithTraversal(httpMethod, "/file:" + secretPath, location); + testResolvePathWithTraversal(httpMethod, "url:" + secretPath, location); + testResolvePathWithTraversal(httpMethod, "/url:" + secretPath, location); + testResolvePathWithTraversal(httpMethod, "////../.." + secretPath, location); + testResolvePathWithTraversal(httpMethod, "/%2E%2E/testsecret/secret.txt", location); + testResolvePathWithTraversal(httpMethod, "%2F%2F%2E%2E%2F%2Ftestsecret/secret.txt", location); + testResolvePathWithTraversal(httpMethod, "url:" + secretPath, location); // The following tests fail with a MalformedURLException on Windows - // testInvalidPath(location, "/" + secretPath); - // testInvalidPath(location, "/ " + secretPath); + // testResolvePathWithTraversal(location, "/" + secretPath); + // testResolvePathWithTraversal(location, "/ " + secretPath); } - private void testInvalidPath(HttpMethod httpMethod, String requestPath, Resource location) throws Exception { + private void testResolvePathWithTraversal(HttpMethod httpMethod, String requestPath, Resource location) throws Exception { ServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.method(httpMethod, "")); setPathWithinHandlerMapping(exchange, requestPath); StepVerifier.create(this.handler.handle(exchange)) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java index 43f19c54996..80731c37b65 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java @@ -520,7 +520,15 @@ public class ResourceHttpRequestHandler extends WebContentGenerator if (path.contains("%")) { try { // Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars - if (isInvalidPath(URLDecoder.decode(path, "UTF-8"))) { + String decodedPath = URLDecoder.decode(path, "UTF-8"); + if (isInvalidPath(decodedPath)) { + if (logger.isTraceEnabled()) { + logger.trace("Ignoring invalid resource path with escape sequences [" + path + "]."); + } + return null; + } + decodedPath = processPath(decodedPath); + if (isInvalidPath(decodedPath)) { if (logger.isTraceEnabled()) { logger.trace("Ignoring invalid resource path with escape sequences [" + path + "]."); } @@ -543,13 +551,47 @@ public class ResourceHttpRequestHandler extends WebContentGenerator } /** - * Process the given resource path to be used. - *

The default implementation replaces any combination of leading '/' and - * control characters (00-1F and 7F) with a single "/" or "". For example - * {@code " // /// //// foo/bar"} becomes {@code "/foo/bar"}. + * Process the given resource path. + *

The default implementation replaces: + *

* @since 3.2.12 */ protected String processPath(String path) { + path = StringUtils.replace(path, "\\", "/"); + path = cleanDuplicateSlashes(path); + return cleanLeadingSlash(path); + } + + private String cleanDuplicateSlashes(String path) { + StringBuilder sb = null; + char prev = 0; + for (int i = 0; i < path.length(); i++) { + char curr = path.charAt(i); + try { + if ((curr == '/') && (prev == '/')) { + if (sb == null) { + sb = new StringBuilder(path.substring(0, i)); + } + continue; + } + if (sb != null) { + sb.append(path.charAt(i)); + } + } + finally { + prev = curr; + } + } + return sb != null ? sb.toString() : path; + } + + private String cleanLeadingSlash(String path) { boolean slash = false; for (int i = 0; i < path.length(); i++) { if (path.charAt(i) == '/') { diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java index 0638b987213..1c13e06624f 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java @@ -43,6 +43,7 @@ import org.springframework.web.accept.ContentNegotiationManagerFactoryBean; import org.springframework.web.servlet.HandlerMapping; import static org.junit.Assert.*; +import static org.mockito.Mockito.*; /** * Unit tests for {@link ResourceHttpRequestHandler}. @@ -303,41 +304,84 @@ public class ResourceHttpRequestHandlerTests { } @Test - public void invalidPath() throws Exception { + public void testInvalidPath() throws Exception { + + // Use mock ResourceResolver: i.e. we're only testing upfront validations... + + Resource resource = mock(Resource.class); + when(resource.getFilename()).thenThrow(new AssertionError("Resource should not be resolved")); + when(resource.getInputStream()).thenThrow(new AssertionError("Resource should not be resolved")); + ResourceResolver resolver = mock(ResourceResolver.class); + when(resolver.resolveResource(any(), any(), any(), any())).thenReturn(resource); + + ResourceHttpRequestHandler handler = new ResourceHttpRequestHandler(); + handler.setLocations(Collections.singletonList(new ClassPathResource("test/", getClass()))); + handler.setResourceResolvers(Collections.singletonList(resolver)); + handler.setServletContext(new TestServletContext()); + handler.afterPropertiesSet(); + + testInvalidPath("../testsecret/secret.txt", handler); + testInvalidPath("test/../../testsecret/secret.txt", handler); + testInvalidPath(":/../../testsecret/secret.txt", handler); + + Resource location = new UrlResource(getClass().getResource("./test/")); + this.handler.setLocations(Collections.singletonList(location)); + Resource secretResource = new UrlResource(getClass().getResource("testsecret/secret.txt")); + String secretPath = secretResource.getURL().getPath(); + + testInvalidPath("file:" + secretPath, handler); + testInvalidPath("/file:" + secretPath, handler); + testInvalidPath("url:" + secretPath, handler); + testInvalidPath("/url:" + secretPath, handler); + testInvalidPath("/../.." + secretPath, handler); + testInvalidPath("/%2E%2E/testsecret/secret.txt", handler); + testInvalidPath("/%2E%2E/testsecret/secret.txt", handler); + testInvalidPath("%2F%2F%2E%2E%2F%2F%2E%2E" + secretPath, handler); + } + + private void testInvalidPath(String requestPath, ResourceHttpRequestHandler handler) throws Exception { + this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, requestPath); + this.response = new MockHttpServletResponse(); + handler.handleRequest(this.request, this.response); + assertEquals(HttpStatus.NOT_FOUND.value(), this.response.getStatus()); + } + + @Test + public void resolvePathWithTraversal() throws Exception { for (HttpMethod method : HttpMethod.values()) { this.request = new MockHttpServletRequest("GET", ""); this.response = new MockHttpServletResponse(); - testInvalidPath(method); + testResolvePathWithTraversal(method); } } - private void testInvalidPath(HttpMethod httpMethod) throws Exception { + private void testResolvePathWithTraversal(HttpMethod httpMethod) throws Exception { this.request.setMethod(httpMethod.name()); Resource location = new ClassPathResource("test/", getClass()); this.handler.setLocations(Collections.singletonList(location)); - testInvalidPath(location, "../testsecret/secret.txt"); - testInvalidPath(location, "test/../../testsecret/secret.txt"); - testInvalidPath(location, ":/../../testsecret/secret.txt"); + testResolvePathWithTraversal(location, "../testsecret/secret.txt"); + testResolvePathWithTraversal(location, "test/../../testsecret/secret.txt"); + testResolvePathWithTraversal(location, ":/../../testsecret/secret.txt"); location = new UrlResource(getClass().getResource("./test/")); this.handler.setLocations(Collections.singletonList(location)); Resource secretResource = new UrlResource(getClass().getResource("testsecret/secret.txt")); String secretPath = secretResource.getURL().getPath(); - testInvalidPath(location, "file:" + secretPath); - testInvalidPath(location, "/file:" + secretPath); - testInvalidPath(location, "url:" + secretPath); - testInvalidPath(location, "/url:" + secretPath); - testInvalidPath(location, "/" + secretPath); - testInvalidPath(location, "////../.." + secretPath); - testInvalidPath(location, "/%2E%2E/testsecret/secret.txt"); - testInvalidPath(location, "/ " + secretPath); - testInvalidPath(location, "url:" + secretPath); + testResolvePathWithTraversal(location, "file:" + secretPath); + testResolvePathWithTraversal(location, "/file:" + secretPath); + testResolvePathWithTraversal(location, "url:" + secretPath); + testResolvePathWithTraversal(location, "/url:" + secretPath); + testResolvePathWithTraversal(location, "/" + secretPath); + testResolvePathWithTraversal(location, "////../.." + secretPath); + testResolvePathWithTraversal(location, "/%2E%2E/testsecret/secret.txt"); + testResolvePathWithTraversal(location, "%2F%2F%2E%2E%2F%2Ftestsecret/secret.txt"); + testResolvePathWithTraversal(location, "/ " + secretPath); } - private void testInvalidPath(Resource location, String requestPath) throws Exception { + private void testResolvePathWithTraversal(Resource location, String requestPath) throws Exception { this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, requestPath); this.response = new MockHttpServletResponse(); this.handler.handleRequest(this.request, this.response); @@ -356,7 +400,8 @@ public class ResourceHttpRequestHandlerTests { } @Test - public void processPath() throws Exception { + public void processPath() { + // Unchanged assertSame("/foo/bar", this.handler.processPath("/foo/bar")); assertSame("foo/bar", this.handler.processPath("foo/bar")); @@ -382,10 +427,17 @@ public class ResourceHttpRequestHandlerTests { assertEquals("/", this.handler.processPath("/")); assertEquals("/", this.handler.processPath("///")); assertEquals("/", this.handler.processPath("/ / / ")); + assertEquals("/", this.handler.processPath("\\/ \\/ \\/ ")); + + // duplicate slash or backslash + assertEquals("/foo/ /bar/baz/", this.handler.processPath("//foo/ /bar//baz//")); + assertEquals("/foo/ /bar/baz/", this.handler.processPath("\\\\foo\\ \\bar\\\\baz\\\\")); + assertEquals("foo/bar", this.handler.processPath("foo\\\\/\\////bar")); + } @Test - public void initAllowedLocations() throws Exception { + public void initAllowedLocations() { PathResourceResolver resolver = (PathResourceResolver) this.handler.getResourceResolvers().get(0); Resource[] locations = resolver.getAllowedLocations();