From 57b466fdfc28a6a3baaf0ab4538e7ecc239ee340 Mon Sep 17 00:00:00 2001 From: matthew-pearson Date: Sun, 14 Feb 2016 09:19:04 +0000 Subject: [PATCH 1/2] 404 rather than 405 or 200 Check that the path is valid and resolvable before checking that the http method is supported. For invalid or unresolvable paths, always respond with a 404. --- .../resource/ResourceHttpRequestHandler.java | 16 ++-- .../ResourceHttpRequestHandlerTests.java | 90 ++++++++++++++++++- 2 files changed, 94 insertions(+), 12 deletions(-) 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 8b7d4c3471f..1362972a2be 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 @@ -226,6 +226,14 @@ public class ResourceHttpRequestHandler extends WebContentGenerator public void handleRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + // First, check whether a matching resource exists + Resource resource = getResource(request); + if (resource == null) { + logger.trace("No matching resource found - returning 404"); + response.sendError(HttpServletResponse.SC_NOT_FOUND); + return; + } + if (HttpMethod.OPTIONS.matches(request.getMethod())) { response.setHeader("Allow", getAllowHeader()); return; @@ -234,14 +242,6 @@ public class ResourceHttpRequestHandler extends WebContentGenerator // Supported methods and required session checkRequest(request); - // Check whether a matching resource exists - Resource resource = getResource(request); - if (resource == null) { - logger.trace("No matching resource found - returning 404"); - response.sendError(HttpServletResponse.SC_NOT_FOUND); - return; - } - // Header phase if (new ServletWebRequest(request, response).checkNotModified(resource.lastModified())) { logger.trace("Resource not modified - returning 304"); 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 818bb5cfcce..5cae355039a 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 @@ -39,6 +39,8 @@ import org.junit.Test; import org.springframework.core.io.ClassPathResource; import org.springframework.core.io.Resource; import org.springframework.core.io.UrlResource; +import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatus; import org.springframework.mock.web.test.MockHttpServletRequest; import org.springframework.mock.web.test.MockHttpServletResponse; import org.springframework.mock.web.test.MockServletContext; @@ -223,7 +225,47 @@ public class ResourceHttpRequestHandlerTests { } @Test - public void invalidPath() throws Exception { + public void invalidPathForDelete() throws Exception { + invalidPath(HttpMethod.DELETE); + } + + @Test + public void invalidPathForGet() throws Exception { + invalidPath(HttpMethod.GET); + } + + @Test + public void invalidPathForHead() throws Exception { + invalidPath(HttpMethod.HEAD); + } + + @Test + public void invalidPathForOptions() throws Exception { + invalidPath(HttpMethod.OPTIONS); + } + + @Test + public void invalidPathForPost() throws Exception { + invalidPath(HttpMethod.POST); + } + + @Test + public void invalidPathForPut() throws Exception { + invalidPath(HttpMethod.PUT); + } + + @Test + public void invalidPathForPatch() throws Exception { + invalidPath(HttpMethod.PATCH); + } + + @Test + public void invalidPathForTrace() throws Exception { + invalidPath(HttpMethod.TRACE); + } + + private void invalidPath(HttpMethod httpMethod) throws Exception { + this.request.setMethod(httpMethod.name()); Resource location = new ClassPathResource("test/", getClass()); this.handler.setLocations(Arrays.asList(location)); @@ -255,7 +297,7 @@ public class ResourceHttpRequestHandlerTests { if (!location.createRelative(requestPath).exists() && !requestPath.contains(":")) { fail(requestPath + " doesn't actually exist as a relative path"); } - assertEquals(404, this.response.getStatus()); + assertEquals(HttpStatus.NOT_FOUND.value(), this.response.getStatus()); } @Test @@ -376,10 +418,50 @@ public class ResourceHttpRequestHandlerTests { } @Test - public void resourceNotFound() throws Exception { + public void resourceNotFoundForDelete() throws Exception { + resourceNotFound(HttpMethod.DELETE); + } + + @Test + public void resourceNotFoundForGet() throws Exception { + resourceNotFound(HttpMethod.GET); + } + + @Test + public void resourceNotFoundForHead() throws Exception { + resourceNotFound(HttpMethod.HEAD); + } + + @Test + public void resourceNotFoundForOptions() throws Exception { + resourceNotFound(HttpMethod.OPTIONS); + } + + @Test + public void resourceNotFoundForPost() throws Exception { + resourceNotFound(HttpMethod.POST); + } + + @Test + public void resourceNotFoundForPut() throws Exception { + resourceNotFound(HttpMethod.PUT); + } + + @Test + public void resourceNotFoundForPatch() throws Exception { + resourceNotFound(HttpMethod.PATCH); + } + + @Test + public void resourceNotFoundForTrace() throws Exception { + resourceNotFound(HttpMethod.TRACE); + } + + private void resourceNotFound(HttpMethod httpMethod) throws Exception { + this.request.setMethod(httpMethod.name()); this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "not-there.css"); this.handler.handleRequest(this.request, this.response); - assertEquals(404, this.response.getStatus()); + assertEquals(HttpStatus.NOT_FOUND.value(), this.response.getStatus()); } @Test From 024bd6e60419d221ad04b769a41603a59dc704e9 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 17 Feb 2016 08:22:42 -0500 Subject: [PATCH 2/2] Polish --- .../resource/ResourceHttpRequestHandler.java | 2 +- .../ResourceHttpRequestHandlerTests.java | 99 ++++--------------- 2 files changed, 20 insertions(+), 81 deletions(-) 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 1362972a2be..7b56747158f 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 @@ -226,7 +226,7 @@ public class ResourceHttpRequestHandler extends WebContentGenerator public void handleRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { - // First, check whether a matching resource exists + // For very general mappings (e.g. "/") we need to check 404 first Resource resource = getResource(request); if (resource == null) { logger.trace("No matching resource found - returning 404"); 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 5cae355039a..d826972fe0a 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 @@ -26,6 +26,7 @@ import java.io.InputStream; import java.text.SimpleDateFormat; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.TimeZone; @@ -225,57 +226,26 @@ public class ResourceHttpRequestHandlerTests { } @Test - public void invalidPathForDelete() throws Exception { - invalidPath(HttpMethod.DELETE); + public void invalidPath() throws Exception { + for (HttpMethod method : HttpMethod.values()) { + this.request = new MockHttpServletRequest("GET", ""); + this.response = new MockHttpServletResponse(); + testInvalidPath(method); + } } - @Test - public void invalidPathForGet() throws Exception { - invalidPath(HttpMethod.GET); - } - - @Test - public void invalidPathForHead() throws Exception { - invalidPath(HttpMethod.HEAD); - } - - @Test - public void invalidPathForOptions() throws Exception { - invalidPath(HttpMethod.OPTIONS); - } - - @Test - public void invalidPathForPost() throws Exception { - invalidPath(HttpMethod.POST); - } - - @Test - public void invalidPathForPut() throws Exception { - invalidPath(HttpMethod.PUT); - } - - @Test - public void invalidPathForPatch() throws Exception { - invalidPath(HttpMethod.PATCH); - } - - @Test - public void invalidPathForTrace() throws Exception { - invalidPath(HttpMethod.TRACE); - } - - private void invalidPath(HttpMethod httpMethod) throws Exception { + private void testInvalidPath(HttpMethod httpMethod) throws Exception { this.request.setMethod(httpMethod.name()); Resource location = new ClassPathResource("test/", getClass()); - this.handler.setLocations(Arrays.asList(location)); + this.handler.setLocations(Collections.singletonList(location)); testInvalidPath(location, "../testsecret/secret.txt"); testInvalidPath(location, "test/../../testsecret/secret.txt"); testInvalidPath(location, ":/../../testsecret/secret.txt"); location = new UrlResource(getClass().getResource("./test/")); - this.handler.setLocations(Arrays.asList(location)); + this.handler.setLocations(Collections.singletonList(location)); Resource secretResource = new UrlResource(getClass().getResource("testsecret/secret.txt")); String secretPath = secretResource.getURL().getPath(); @@ -357,7 +327,7 @@ public class ResourceHttpRequestHandlerTests { pathResolver.setAllowedLocations(location1); ResourceHttpRequestHandler handler = new ResourceHttpRequestHandler(); - handler.setResourceResolvers(Arrays.asList(pathResolver)); + handler.setResourceResolvers(Collections.singletonList(pathResolver)); handler.setLocations(Arrays.asList(location1, location2)); handler.afterPropertiesSet(); @@ -418,43 +388,12 @@ public class ResourceHttpRequestHandlerTests { } @Test - public void resourceNotFoundForDelete() throws Exception { - resourceNotFound(HttpMethod.DELETE); - } - - @Test - public void resourceNotFoundForGet() throws Exception { - resourceNotFound(HttpMethod.GET); - } - - @Test - public void resourceNotFoundForHead() throws Exception { - resourceNotFound(HttpMethod.HEAD); - } - - @Test - public void resourceNotFoundForOptions() throws Exception { - resourceNotFound(HttpMethod.OPTIONS); - } - - @Test - public void resourceNotFoundForPost() throws Exception { - resourceNotFound(HttpMethod.POST); - } - - @Test - public void resourceNotFoundForPut() throws Exception { - resourceNotFound(HttpMethod.PUT); - } - - @Test - public void resourceNotFoundForPatch() throws Exception { - resourceNotFound(HttpMethod.PATCH); - } - - @Test - public void resourceNotFoundForTrace() throws Exception { - resourceNotFound(HttpMethod.TRACE); + public void resourceNotFound() throws Exception { + for (HttpMethod method : HttpMethod.values()) { + this.request = new MockHttpServletRequest("GET", ""); + this.response = new MockHttpServletResponse(); + resourceNotFound(method); + } } private void resourceNotFound(HttpMethod httpMethod) throws Exception { @@ -570,7 +509,7 @@ public class ResourceHttpRequestHandlerTests { } // SPR-12999 - @Test + @Test @SuppressWarnings("unchecked") public void writeContentNotGettingInputStream() throws Exception { Resource resource = mock(Resource.class); given(resource.getInputStream()).willThrow(FileNotFoundException.class); @@ -597,7 +536,7 @@ public class ResourceHttpRequestHandlerTests { } // SPR-13620 - @Test + @Test @SuppressWarnings("unchecked") public void writeContentInputStreamThrowingNullPointerException() throws Exception { Resource resource = mock(Resource.class); InputStream in = mock(InputStream.class);