From 65de2c6690103212a933e5491fbe94d35fcdd498 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 22 May 2018 10:30:30 -0500 Subject: [PATCH 1/2] Issue #2560 - Review of PathResource exception handling Signed-off-by: Joakim Erdfelt --- .../jetty/servlet/DefaultServletTest.java | 20 ++++++++++++++ .../jetty/util/resource/PathResource.java | 27 ++++++++++--------- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java index 6916c87f669..290332123a8 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java +++ b/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,19 @@ 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); + 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 { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java index 6c7b3325377..e0c9ae4342b 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java @@ -204,17 +204,24 @@ 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); - this.alias = checkAliasPath(); + try + { + this.path = parent.path.getFileSystem().getPath(parent.path.toString(), childPath); + if (isDirectory() && !childPath.endsWith("/")) + childPath += "/"; + this.uri = URIUtil.addPath(parent.uri, childPath); + this.alias = checkAliasPath(); + } + catch(InvalidPathException e) + { + throw (InvalidPathException) new InvalidPathException(childPath, e.getReason()).initCause(e); + } } /** @@ -242,10 +249,6 @@ public class PathResource extends Resource { path = Paths.get(uri); } - catch (InvalidPathException e) - { - throw e; - } catch (IllegalArgumentException e) { throw e; @@ -286,7 +289,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); From ad4dceb1c08679baa2a6a64356fcde5309e13fd8 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Thu, 31 May 2018 11:31:36 -0500 Subject: [PATCH 2/2] Issue #2560 - Moving InvalidPath logic from PathResource to ResourceContentFactory Signed-off-by: Joakim Erdfelt --- .../jetty/server/ResourceContentFactory.java | 20 +++++++++++++------ .../jetty/servlet/DefaultServletTest.java | 1 + .../jetty/util/resource/PathResource.java | 17 +++++----------- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceContentFactory.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceContentFactory.java index ef8613d9004..f89290bd635 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceContentFactory.java +++ b/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 diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java index 290332123a8..c2449c7d7c0 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java @@ -1264,6 +1264,7 @@ public class DefaultServletTest 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())))); } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java index e0c9ae4342b..0dfb3ffd274 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java @@ -210,18 +210,11 @@ public class PathResource extends Resource // FileSystem.getPath(path,childPath) is visible as a difference to the URI // obtained via URIUtil.addDecodedPath(uri,childPath) - try - { - this.path = parent.path.getFileSystem().getPath(parent.path.toString(), childPath); - if (isDirectory() && !childPath.endsWith("/")) - childPath += "/"; - this.uri = URIUtil.addPath(parent.uri, childPath); - this.alias = checkAliasPath(); - } - catch(InvalidPathException e) - { - throw (InvalidPathException) new InvalidPathException(childPath, e.getReason()).initCause(e); - } + this.path = parent.path.getFileSystem().getPath(parent.path.toString(), childPath); + if (isDirectory() && !childPath.endsWith("/")) + childPath += "/"; + this.uri = URIUtil.addPath(parent.uri, childPath); + this.alias = checkAliasPath(); } /**