Merge pull request #2561 from eclipse/jetty-9.4.x-issue-2560-pathresource-exceptions

Issue #2560 - Review of PathResource exception handling

jetty-server/src/main/java/org/eclipse/jetty/server/ResourceContentFactory.java

@@ -19,6 +19,7 @@
 package org.eclipse.jetty.server;
 
 import java.io.IOException;
+import java.nio.file.InvalidPathException;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -55,13 +56,20 @@ public class ResourceContentFactory implements ContentFactory
     public HttpContent getContent(String pathInContext,int maxBufferSize)
         throws IOException
     {
-        // try loading the content from our factory.
-        Resource resource=_factory.getResource(pathInContext);
-        HttpContent loaded = load(pathInContext,resource,maxBufferSize);
-        return loaded;
+        try
+        {
+            // try loading the content from our factory.
+            Resource resource = _factory.getResource(pathInContext);
+            HttpContent loaded = load(pathInContext, resource, maxBufferSize);
+            return loaded;
+        }
+        catch (Throwable t)
+        {
+            // Any error has potential to reveal fully qualified path
+            throw (InvalidPathException) new InvalidPathException(pathInContext, "Invalid PathInContext").initCause(t);
+        }
     }
-    
-    
+
     /* ------------------------------------------------------------ */
     private HttpContent load(String pathInContext, Resource resource, int maxBufferSize)
         throws IOException

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java

@@ -18,6 +18,11 @@
 
 package org.eclipse.jetty.servlet;
 
+import static org.hamcrest.CoreMatchers.anyOf;
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.not;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeTrue;
 
@@ -38,9 +43,11 @@ import javax.servlet.FilterConfig;
 import javax.servlet.ServletException;
 import javax.servlet.ServletRequest;
 import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jetty.http.DateGenerator;
 import org.eclipse.jetty.http.HttpContent;
+import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.server.HttpConfiguration;
 import org.eclipse.jetty.server.LocalConnector;
 import org.eclipse.jetty.server.ResourceContentFactory;
@@ -1248,6 +1255,20 @@ public class DefaultServletTest
         assertResponseContains("Content-Encoding: gzip",response);
     }
 
+    @Test
+    public void testControlCharacter() throws Exception
+    {
+        FS.ensureDirExists(docRoot);
+        ServletHolder defholder = context.addServlet(DefaultServlet.class, "/");
+        defholder.setInitParameter("resourceBase", docRoot.getAbsolutePath());
+
+        String rawResponse = connector.getResponse("GET /context/%0a HTTP/1.1\r\nHost: local\r\nConnection: close\r\n\r\n");
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+        System.out.println(response + "\n" + response.getContent());
+        assertThat("Response.status", response.getStatus(), anyOf(is(HttpServletResponse.SC_NOT_FOUND), is(HttpServletResponse.SC_INTERNAL_SERVER_ERROR)));
+        assertThat("Response.content", response.getContent(), is(not(containsString(docRoot.toString()))));
+    }
+
     @Test
     public void testIfModifiedSmall() throws Exception
     {

jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java

@@ -204,16 +204,16 @@ public class PathResource extends Resource
      * @param parent the parent path resource
      * @param childPath the child sub path
      */
-    private PathResource(PathResource parent, String childPath) throws MalformedURLException
+    private PathResource(PathResource parent, String childPath)
     {
         // Calculate the URI and the path separately, so that any aliasing done by
-        // FileSystem.getPath(path,childPath) is visiable as a difference to the URI
+        // FileSystem.getPath(path,childPath) is visible as a difference to the URI
         // obtained via URIUtil.addDecodedPath(uri,childPath)
 
         this.path = parent.path.getFileSystem().getPath(parent.path.toString(), childPath);
-        if (isDirectory() &&!childPath.endsWith("/"))
-            childPath+="/";
-        this.uri = URIUtil.addPath(parent.uri,childPath);
+        if (isDirectory() && !childPath.endsWith("/"))
+            childPath += "/";
+        this.uri = URIUtil.addPath(parent.uri, childPath);
         this.alias = checkAliasPath();
     }
 
@@ -242,10 +242,6 @@ public class PathResource extends Resource
         {
             path = Paths.get(uri);
         }
-        catch (InvalidPathException e)
-        {
-            throw e;
-        }
         catch (IllegalArgumentException e)
         {
             throw e;
@@ -286,7 +282,7 @@ public class PathResource extends Resource
     }
 
     @Override
-    public Resource addPath(final String subpath) throws IOException, MalformedURLException
+    public Resource addPath(final String subpath) throws IOException
     {
         String cpath = URIUtil.canonicalPath(subpath);