From 4d5fca596dfe193556ad7148d16aae718805a84d Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Thu, 21 May 2015 14:30:12 +0200 Subject: [PATCH] Fix directories I/O in ResourceHttpRequestHandler Prior to this commit, the `ResourceHttpRequestHandler` would not properly handle HTTP requests to **directories contained in JARs**. This would result in HTTP 500 errors, caused by `FileNotFoundException` or `NullPointerException`. This can be tracked to webapp ClassLoader implementations in servlet containers: * in Jetty9x, fetching a directory within a JAR as a `Resource` and getting its InputStream work fine, but attempting to `close()` it results in a NullPointerException as the underlying stream is null. * In Tomcat6x, one cannot fetch an InputStream for the same `Resource` as it throws a FileNotFoundException. This change adds more try/catch clauses and catches more Exception so as to result in HTTP 200 OK responses instead of server errors. While this is inconsistent because the same code path would result in HTTP 404 with existing directories on the file system, there's no other simple way to make those checks for resources contained in JARs. Issue: SPR-12999 --- .../resource/ResourceHttpRequestHandler.java | 20 ++++++--- .../ResourceHttpRequestHandlerTests.java | 43 ++++++++++++++++++- 2 files changed, 55 insertions(+), 8 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 8d6783976e..efac7c9f05 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 @@ -16,6 +16,7 @@ package org.springframework.web.servlet.resource; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -432,18 +433,23 @@ public class ResourceHttpRequestHandler extends WebContentGenerator implements H * @throws IOException in case of errors while writing the content */ protected void writeContent(HttpServletResponse response, Resource resource) throws IOException { - InputStream in = resource.getInputStream(); try { - StreamUtils.copy(in, response.getOutputStream()); - } - finally { + InputStream in = resource.getInputStream(); try { - in.close(); + StreamUtils.copy(in, response.getOutputStream()); } - catch (IOException ex) { - // ignore + finally { + try { + in.close(); + } + catch (Throwable ex) { + // ignore, see SPR-12999 + } } } + catch (FileNotFoundException ex) { + // ignore, see SPR-12999 + } } /** 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 b2bb91be40..ed80ea0251 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 @@ -17,8 +17,12 @@ package org.springframework.web.servlet.resource; import static org.junit.Assert.*; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.*; +import java.io.FileNotFoundException; import java.io.IOException; +import java.io.InputStream; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -60,6 +64,7 @@ public class ResourceHttpRequestHandlerTests { List paths = new ArrayList<>(2); paths.add(new ClassPathResource("test/", getClass())); paths.add(new ClassPathResource("testalternatepath/", getClass())); + paths.add(new ClassPathResource("META-INF/resources/webjars/")); this.handler = new ResourceHttpRequestHandler(); this.handler.setLocations(paths); @@ -247,9 +252,10 @@ public class ResourceHttpRequestHandlerTests { PathResourceResolver resolver = (PathResourceResolver) this.handler.getResourceResolvers().get(0); Resource[] locations = resolver.getAllowedLocations(); - assertEquals(2, locations.length); + assertEquals(3, locations.length); assertEquals("test/", ((ClassPathResource) locations[0]).getPath()); assertEquals("testalternatepath/", ((ClassPathResource) locations[1]).getPath()); + assertEquals("META-INF/resources/webjars/", ((ClassPathResource) locations[2]).getPath()); } @Test @@ -294,6 +300,14 @@ public class ResourceHttpRequestHandlerTests { assertEquals(404, this.response.getStatus()); } + @Test + public void directoryInJarFile() throws Exception { + this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "underscorejs/"); + this.handler.handleRequest(this.request, this.response); + assertEquals(200, this.response.getStatus()); + assertEquals(0, this.response.getContentLength()); + } + @Test public void missingResourcePath() throws Exception { this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, ""); @@ -425,6 +439,33 @@ public class ResourceHttpRequestHandlerTests { assertEquals("t.", ranges[11]); } + // SPR-12999 + @Test + public void writeContentNotGettingInputStream() throws Exception { + Resource resource = mock(Resource.class); + given(resource.getInputStream()).willThrow(FileNotFoundException.class); + + this.handler.writeContent(this.response, resource); + + assertEquals(200, this.response.getStatus()); + assertEquals(0, this.response.getContentLength()); + } + + // SPR-12999 + @Test + public void writeContentNotClosingInputStream() throws Exception { + Resource resource = mock(Resource.class); + InputStream inputStream = mock(InputStream.class); + given(resource.getInputStream()).willReturn(inputStream); + given(inputStream.read(any())).willReturn(-1); + doThrow(new NullPointerException()).when(inputStream).close(); + + this.handler.writeContent(this.response, resource); + + assertEquals(200, this.response.getStatus()); + assertEquals(0, this.response.getContentLength()); + } + private long headerAsLong(String responseHeaderName) { return Long.valueOf(this.response.getHeader(responseHeaderName));