From 92a74d6734881cf067f54e71bf5188dc7fdbbd52 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 28 Jun 2021 15:52:14 +1000 Subject: [PATCH 1/6] Issue #6473 - canonicalPath refactor & fix alias check in PathResource Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/http/HttpURI.java | 154 +++++++++-- .../org/eclipse/jetty/http/UriCompliance.java | 58 +++- .../org/eclipse/jetty/http/HttpURITest.java | 132 +++++----- .../jetty/rewrite/handler/RedirectUtil.java | 4 +- .../rewrite/handler/ValidUrlRuleTest.java | 17 +- .../org/eclipse/jetty/server/Dispatcher.java | 34 ++- .../org/eclipse/jetty/server/Request.java | 33 +-- .../org/eclipse/jetty/server/Response.java | 4 +- .../jetty/server/handler/ContextHandler.java | 21 +- .../jetty/server/handler/ResourceHandler.java | 1 - .../jetty/server/HttpConnectionTest.java | 12 +- .../org/eclipse/jetty/server/RequestTest.java | 9 - .../ContextHandlerGetResourceTest.java | 34 ++- .../eclipse/jetty/servlet/DefaultServlet.java | 4 +- .../eclipse/jetty/servlet/RequestURITest.java | 4 +- .../servlets/DataRateLimitedServlet.java | 7 +- .../org/eclipse/jetty/servlets/PutFilter.java | 8 +- .../java/org/eclipse/jetty/util/URIUtil.java | 248 +++++++++--------- .../eclipse/jetty/util/Utf8Appendable.java | 1 - .../jetty/util/resource/PathResource.java | 20 +- .../eclipse/jetty/util/resource/Resource.java | 1 - .../jetty/util/resource/URLResource.java | 5 +- .../jetty/util/URIUtilCanonicalPathTest.java | 27 +- .../jetty/util/Utf8AppendableTest.java | 29 ++ .../eclipse/jetty/webapp/WebAppContext.java | 15 +- .../jetty/webapp/WebAppContextTest.java | 40 +++ .../jsp/JspAndDefaultWithoutAliasesTest.java | 37 ++- 27 files changed, 608 insertions(+), 351 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 42f829068e9c..596f1acc1424 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -15,7 +15,10 @@ import java.net.URI; import java.net.URISyntaxException; +import java.util.Collection; +import java.util.Collections; import java.util.EnumSet; +import java.util.Set; import org.eclipse.jetty.util.HostPort; import org.eclipse.jetty.util.Index; @@ -31,54 +34,92 @@ * via the static methods such as {@link #build()} and {@link #from(String)}. * * A URI such as - * http://user@host:port/path;ignored/info;param?query#ignored - * is split into the following undecoded elements: */ +@Deprecated public class PutFilter implements Filter { public static final String __PUT = "PUT"; @@ -80,7 +81,8 @@ public void init(FilterConfig config) throws ServletException _tmpdir = (File)_context.getAttribute("javax.servlet.context.tempdir"); - if (_context.getRealPath("/") == null) + String realPath = _context.getRealPath("/"); + if (realPath == null) throw new UnavailableException("Packed war"); String b = config.getInitParameter("baseURI"); @@ -90,7 +92,7 @@ public void init(FilterConfig config) throws ServletException } else { - File base = new File(_context.getRealPath("/")); + File base = new File(realPath); _baseURI = base.toURI().toString(); } @@ -284,7 +286,7 @@ public void handleDelete(HttpServletRequest request, HttpServletResponse respons public void handleMove(HttpServletRequest request, HttpServletResponse response, String pathInContext, File file) throws ServletException, IOException, URISyntaxException { - String newPath = URIUtil.canonicalEncodedPath(request.getHeader("new-uri")); + String newPath = URIUtil.canonicalURI(request.getHeader("new-uri")); if (newPath == null) { response.sendError(HttpServletResponse.SC_BAD_REQUEST); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java b/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java index f18e685f2e8b..b91b71c30081 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java @@ -471,6 +471,7 @@ public static String decodePath(String path, int offset, int length) if (u == 'u') { // UTF16 encoding is only supported with UriCompliance.Violation.UTF16_ENCODINGS. + // This is wrong. This is a codepoint not a char builder.append((char)(0xffff & TypeUtil.parseInt(path, i + 2, 4, 16))); i += 5; } @@ -776,143 +777,128 @@ public static String parentPath(String p) } /** - * Convert an encoded path to a canonical form. + * Convert a partial URI to a canonical form. *

- * All instances of "." and ".." are factored out. + * All segments of "." and ".." are factored out. * Null is returned if the path tries to .. above its root. *

* - * @param path the path to convert, decoded, with path separators '/' and no queries. + * @param uri the encoded URI from the path onwards, which may contain query strings and/or fragments * @return the canonical path, or null if path traversal above root. + * @see #canonicalPath(String) + * @see #canonicalURI(String) */ - public static String canonicalPath(String path) + public static String canonicalURI(String uri) { - // See https://tools.ietf.org/html/rfc3986#section-5.2.4 - - if (path == null || path.isEmpty()) - return path; + if (uri == null || uri.isEmpty()) + return uri; - int end = path.length(); + boolean slash = true; + int end = uri.length(); int i = 0; - int dots = 0; + // Initially just loop looking if we may need to normalize loop: while (i < end) { - char c = path.charAt(i); + char c = uri.charAt(i); switch (c) { case '/': - dots = 0; + slash = true; break; case '.': - if (dots == 0) - { - dots = 1; + if (slash) break loop; - } - dots = -1; + slash = false; break; + case '?': + case '#': + // Nothing to normalize so return original path + return uri; + default: - dots = -1; + slash = false; } i++; } + // Nothing to normalize so return original path if (i == end) - return path; + return uri; - StringBuilder canonical = new StringBuilder(path.length()); - canonical.append(path, 0, i); + // We probably need to normalize, so copy to path so far into builder + StringBuilder canonical = new StringBuilder(uri.length()); + canonical.append(uri, 0, i); + // Loop looking for single and double dot segments + int dots = 1; i++; - while (i <= end) + loop : while (i < end) { - char c = i < end ? path.charAt(i) : '\0'; + char c = uri.charAt(i); switch (c) { - case '\0': - if (dots == 2) - { - if (canonical.length() < 2) - return null; - canonical.setLength(canonical.length() - 1); - canonical.setLength(canonical.lastIndexOf("/") + 1); - } - break; - case '/': - switch (dots) - { - case 1: - break; - - case 2: - if (canonical.length() < 2) - return null; - canonical.setLength(canonical.length() - 1); - canonical.setLength(canonical.lastIndexOf("/") + 1); - break; - - default: - canonical.append(c); - } + if (doDotsSlash(canonical, dots)) + return null; + slash = true; dots = 0; break; + case '?': + case '#': + // finish normalization at a query + break loop; + case '.': - switch (dots) - { - case 0: - dots = 1; - break; - case 1: - dots = 2; - break; - case 2: - canonical.append("..."); - dots = -1; - break; - default: - canonical.append('.'); - } + // Count dots only if they are leading in the segment + if (dots > 0) + dots++; + else if (slash) + dots = 1; + else + canonical.append('.'); + slash = false; break; default: - switch (dots) - { - case 1: - canonical.append('.'); - break; - case 2: - canonical.append(".."); - break; - default: - } + // Add leading dots to the path + while (dots-- > 0) + canonical.append('.'); canonical.append(c); - dots = -1; + dots = 0; + slash = false; } - i++; } + + // process any remaining dots + if (doDots(canonical, dots)) + return null; + + // append any query + if (i < end) + canonical.append(uri, i, end); + return canonical.toString(); } /** - * Convert a path to a cananonical form. - *

- * All instances of "." and ".." are factored out. - *

+ * Convert a decoded URI path to a canonical form. *

+ * All segments of "." and ".." are factored out. * Null is returned if the path tries to .. above its root. *

* - * @param path the path to convert (expects URI/URL form, encoded, and with path separators '/') + * @param path the decoded URI path to convert. Any special characters (e.g. '?', "#") are assumed to be part of + * the path segments. * @return the canonical path, or null if path traversal above root. + * @see #canonicalURI(String) */ - public static String canonicalEncodedPath(String path) + public static String canonicalPath(String path) { if (path == null || path.isEmpty()) return path; @@ -921,8 +907,8 @@ public static String canonicalEncodedPath(String path) int end = path.length(); int i = 0; - loop: - while (i < end) + // Initially just loop looking if we may need to normalize + loop: while (i < end) { char c = path.charAt(i); switch (c) @@ -937,9 +923,6 @@ public static String canonicalEncodedPath(String path) slash = false; break; - case '?': - return path; - default: slash = false; } @@ -947,56 +930,31 @@ public static String canonicalEncodedPath(String path) i++; } + // Nothing to normalize so return original path if (i == end) return path; + // We probably need to normalize, so copy to path so far into builder StringBuilder canonical = new StringBuilder(path.length()); canonical.append(path, 0, i); + // Loop looking for single and double dot segments int dots = 1; i++; - while (i <= end) + while (i < end) { - char c = i < end ? path.charAt(i) : '\0'; + char c = path.charAt(i); switch (c) { - case '\0': case '/': - case '?': - switch (dots) - { - case 0: - if (c != '\0') - canonical.append(c); - break; - - case 1: - if (c == '?') - canonical.append(c); - break; - - case 2: - if (canonical.length() < 2) - return null; - canonical.setLength(canonical.length() - 1); - canonical.setLength(canonical.lastIndexOf("/") + 1); - if (c == '?') - canonical.append(c); - break; - default: - while (dots-- > 0) - { - canonical.append('.'); - } - if (c != '\0') - canonical.append(c); - } - + if (doDotsSlash(canonical, dots)) + return null; slash = true; dots = 0; break; case '.': + // Count dots only if they are leading in the segment if (dots > 0) dots++; else if (slash) @@ -1007,20 +965,66 @@ else if (slash) break; default: + // Add leading dots to the path while (dots-- > 0) - { canonical.append('.'); - } canonical.append(c); dots = 0; slash = false; } - i++; } + + // process any remaining dots + if (doDots(canonical, dots)) + return null; + return canonical.toString(); } + private static boolean doDots(StringBuilder canonical, int dots) + { + switch (dots) + { + case 0: + case 1: + break; + case 2: + if (canonical.length() < 2) + return true; + canonical.setLength(canonical.length() - 1); + canonical.setLength(canonical.lastIndexOf("/") + 1); + break; + default: + while (dots-- > 0) + canonical.append('.'); + } + return false; + } + + private static boolean doDotsSlash(StringBuilder canonical, int dots) + { + switch (dots) + { + case 0: + canonical.append('/'); + break; + case 1: + break; + case 2: + if (canonical.length() < 2) + return true; + canonical.setLength(canonical.length() - 1); + canonical.setLength(canonical.lastIndexOf("/") + 1); + break; + default: + while (dots-- > 0) + canonical.append('.'); + canonical.append('/'); + } + return false; + } + /** * Convert a path to a compact form. * All instances of "//" and "///" etc. are factored out to single "/" diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8Appendable.java b/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8Appendable.java index f5e70fc7f352..ea7305ff86e2 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8Appendable.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/Utf8Appendable.java @@ -213,7 +213,6 @@ public boolean append(byte[] b, int offset, int length, int maxChars) protected void appendByte(byte b) throws IOException { - if (b > 0 && _state == UTF8_ACCEPT) { _appendable.append((char)(b & 0xFF)); 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 244ae2ad6338..755c4b5f3299 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 @@ -56,7 +56,7 @@ public class PathResource extends Resource private final URI uri; private final boolean belongsToDefaultFileSystem; - private final Path checkAliasPath() + private Path checkAliasPath() { Path abs = path; @@ -68,7 +68,6 @@ private final Path checkAliasPath() * we will just use the original URI to construct the * alias reference Path. */ - if (!URIUtil.equalsIgnoreEncodings(uri, path.toUri())) { try @@ -85,9 +84,11 @@ private final Path checkAliasPath() } if (!abs.isAbsolute()) - { abs = path.toAbsolutePath(); - } + + Path normal = path.normalize(); + if (!abs.equals(normal)) + return normal; try { @@ -233,8 +234,7 @@ public PathResource(Path path) LOG.debug("Unable to get real/canonical path for {}", path, e); } - // cleanup any lingering relative path nonsense (like "/./" and "/../") - this.path = absPath.normalize(); + this.path = absPath; assertValidPath(path); this.uri = this.path.toUri(); @@ -254,7 +254,7 @@ private PathResource(PathResource parent, String childPath) // Calculate the URI and the path separately, so that any aliasing done by // FileSystem.getPath(path,childPath) is visible as a difference to the URI // obtained via URIUtil.addDecodedPath(uri,childPath) - + // The checkAliasPath normalization checks will only work correctly if the getPath implementation here does not normalize. this.path = parent.path.getFileSystem().getPath(parent.path.toString(), childPath); if (isDirectory() && !childPath.endsWith("/")) childPath += "/"; @@ -350,12 +350,10 @@ public boolean isSame(Resource resource) @Override public Resource addPath(final String subpath) throws IOException { - String cpath = URIUtil.canonicalPath(subpath); - - if ((cpath == null) || (cpath.length() == 0)) + if ((subpath == null) || (subpath.length() == 0)) throw new MalformedURLException(subpath); - if ("/".equals(cpath)) + if ("/".equals(subpath)) return this; // subpaths are always under PathResource diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java index a45421a2a2ac..03afc1fd2efc 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java @@ -477,7 +477,6 @@ public URI getAlias() */ public String getListHTML(String base, boolean parent, String query) throws IOException { - base = URIUtil.canonicalPath(base); if (base == null || !isDirectory()) return null; diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java index 74a260c2ecda..b15d3e26f07b 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java @@ -280,9 +280,8 @@ public String[] list() public Resource addPath(String path) throws IOException { - Objects.requireNonNull(path, "Path may not be null"); - - path = URIUtil.canonicalPath(path); + if (path == null) + throw new MalformedURLException("null path"); return newResource(URIUtil.addEncodedPaths(_url.toExternalForm(), URIUtil.encodePath(path)), _useCaches); } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java index 6a9c88d494ea..d8bac6a9ea5f 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java @@ -22,10 +22,11 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; public class URIUtilCanonicalPathTest { - public static Stream data() + public static Stream paths() { String[][] canonical = { @@ -83,6 +84,7 @@ public static Stream data() {"/foo/../bar//", "/bar//"}, {"/ctx/../bar/../ctx/all/index.txt", "/ctx/all/index.txt"}, {"/down/.././index.html", "/index.html"}, + {"/aaa/bbb/ccc/..", "/aaa/bbb/"}, // Path traversal up past root {"..", null}, @@ -95,10 +97,8 @@ public static Stream data() {"a/../..", null}, {"/foo/../../bar", null}, - // Query parameter specifics - {"/ctx/dir?/../index.html", "/ctx/index.html"}, - {"/get-files?file=/etc/passwd", "/get-files?file=/etc/passwd"}, - {"/get-files?file=../../../../../passwd", null}, + // Encoded ? + {"/ctx/dir%3f/../index.html", "/ctx/index.html"}, // Known windows shell quirks {"file.txt ", "file.txt "}, // with spaces @@ -120,7 +120,6 @@ public static Stream data() {"/%2e%2e/", "/%2e%2e/"}, // paths with parameters are not elided - // canonicalPath() is not responsible for decoding characters {"/foo/.;/bar", "/foo/.;/bar"}, {"/foo/..;/bar", "/foo/..;/bar"}, {"/foo/..;/..;/bar", "/foo/..;/..;/bar"}, @@ -139,9 +138,23 @@ public static Stream data() } @ParameterizedTest - @MethodSource("data") + @MethodSource("paths") public void testCanonicalPath(String input, String expectedResult) { + // Check canonicalPath assertThat(URIUtil.canonicalPath(input), is(expectedResult)); + + // Check canonicalURI + if (expectedResult == null) + assertThat(URIUtil.canonicalURI(input), nullValue()); + else + { + // mostly encodedURI will be the same + assertThat(URIUtil.canonicalURI(input), is(expectedResult)); + // but will terminate on fragments and queries + assertThat(URIUtil.canonicalURI(input + "?/foo/../bar/."), is(expectedResult + "?/foo/../bar/.")); + assertThat(URIUtil.canonicalURI(input + "#/foo/../bar/."), is(expectedResult + "#/foo/../bar/.")); + } } + } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/Utf8AppendableTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/Utf8AppendableTest.java index 384a2aaf6458..4786da17eb01 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/Utf8AppendableTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/Utf8AppendableTest.java @@ -151,6 +151,35 @@ public void testInvalidUTF8(Class impl) throws UnsupportedEncodi }); } + @ParameterizedTest + @MethodSource("implementations") + public void testInvalidZeroUTF8(Class impl) throws UnsupportedEncodingException + { + // From https://datatracker.ietf.org/doc/html/rfc3629#section-10 + assertThrows(Utf8Appendable.NotUtf8Exception.class, () -> + { + Utf8Appendable buffer = impl.getDeclaredConstructor().newInstance(); + buffer.append((byte)0xC0); + buffer.append((byte)0x80); + }); + } + + @ParameterizedTest + @MethodSource("implementations") + public void testInvalidAlternateDotEncodingUTF8(Class impl) throws UnsupportedEncodingException + { + // From https://datatracker.ietf.org/doc/html/rfc3629#section-10 + assertThrows(Utf8Appendable.NotUtf8Exception.class, () -> + { + Utf8Appendable buffer = impl.getDeclaredConstructor().newInstance(); + buffer.append((byte)0x2f); + buffer.append((byte)0xc0); + buffer.append((byte)0xae); + buffer.append((byte)0x2e); + buffer.append((byte)0x2f); + }); + } + @ParameterizedTest @MethodSource("implementations") public void testFastFail1(Class impl) throws Exception diff --git a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java index ecd65c66fcc6..1a2af27da4c7 100644 --- a/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java +++ b/jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebAppContext.java @@ -400,7 +400,7 @@ public Resource getResource(String uriInContext) throws MalformedURLException if (uriInContext == null || !uriInContext.startsWith(URIUtil.SLASH)) throw new MalformedURLException(uriInContext); - IOException ioe = null; + MalformedURLException mue = null; Resource resource = null; int loop = 0; while (uriInContext != null && loop++ < 100) @@ -413,16 +413,16 @@ public Resource getResource(String uriInContext) throws MalformedURLException uriInContext = getResourceAlias(uriInContext); } - catch (IOException e) + catch (MalformedURLException e) { LOG.trace("IGNORED", e); - if (ioe == null) - ioe = e; + if (mue == null) + mue = e; } } - if (ioe instanceof MalformedURLException) - throw (MalformedURLException)ioe; + if (mue != null) + throw mue; return resource; } @@ -1438,6 +1438,9 @@ public void checkListener(Class listener) throws Illega @Override public URL getResource(String path) throws MalformedURLException { + if (path == null) + return null; + Resource resource = WebAppContext.this.getResource(path); if (resource == null || !resource.exists()) return null; diff --git a/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java b/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java index 83f67fdc2d2d..765a21867813 100644 --- a/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java +++ b/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/WebAppContextTest.java @@ -53,6 +53,7 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -328,6 +329,45 @@ public void testProtectedTarget() throws Exception assertThat(HttpTester.parseResponse(connector.getResponse("GET //WEB-INF/test.xml HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n")).getStatus(), is(HttpStatus.NOT_FOUND_404)); assertThat(HttpTester.parseResponse(connector.getResponse("GET /WEB-INF%2ftest.xml HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n")).getStatus(), is(HttpStatus.NOT_FOUND_404)); } + + @ParameterizedTest + @ValueSource(strings = { + "/WEB-INF", + "/WEB-INF/", + "/WEB-INF/test.xml", + "/web-inf/test.xml", + "/%2e/WEB-INF/test.xml", + "/%2e/%2e/WEB-INF/test.xml", + "/foo/%2e%2e/WEB-INF/test.xml", + "/%2E/WEB-INF/test.xml", + "//WEB-INF/test.xml", + "/WEB-INF%2ftest.xml", + "/.%00/WEB-INF/test.xml", + "/WEB-INF%00/test.xml" + }) + public void testProtectedTargetFailure(String path) throws Exception + { + Server server = newServer(); + + LocalConnector connector = new LocalConnector(server); + server.addConnector(connector); + connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.LEGACY); + + HandlerList handlers = new HandlerList(); + ContextHandlerCollection contexts = new ContextHandlerCollection(); + WebAppContext context = new WebAppContext(); + Path testWebapp = MavenTestingUtils.getProjectDirPath("src/test/webapp"); + context.setBaseResource(new PathResource(testWebapp)); + context.setContextPath("/"); + server.setHandler(handlers); + handlers.addHandler(contexts); + contexts.addHandler(context); + + server.start(); + + assertThat(HttpTester.parseResponse(connector.getResponse("GET " + path + " HTTP/1.1\r\nHost: localhost:8080\r\nConnection: close\r\n\r\n")).getStatus(), + Matchers.anyOf(is(HttpStatus.NOT_FOUND_404), is(HttpStatus.BAD_REQUEST_400))); + } @Test public void testNullPath() throws Exception diff --git a/tests/test-integration/src/test/java/org/eclipse/jetty/test/jsp/JspAndDefaultWithoutAliasesTest.java b/tests/test-integration/src/test/java/org/eclipse/jetty/test/jsp/JspAndDefaultWithoutAliasesTest.java index 3cbf66f8ac37..f841c023ecc4 100644 --- a/tests/test-integration/src/test/java/org/eclipse/jetty/test/jsp/JspAndDefaultWithoutAliasesTest.java +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/jsp/JspAndDefaultWithoutAliasesTest.java @@ -22,8 +22,12 @@ import java.util.List; import java.util.stream.Stream; +import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.security.HashLoginService; +import org.eclipse.jetty.server.Connector; +import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.NetworkConnector; +import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.servlet.DefaultServlet; import org.eclipse.jetty.servlet.ServletContextHandler; @@ -56,13 +60,21 @@ public static Stream aliases() data.add(Arguments.of("/dump.jsp")); data.add(Arguments.of("/dump.jsp/")); - data.add(Arguments.of("/dump.jsp%00")); - data.add(Arguments.of("/dump.jsp%00x")); - data.add(Arguments.of("/dump.jsp%00x/dump.jsp")); - data.add(Arguments.of("/dump.jsp%00/dump.jsp")); - data.add(Arguments.of("/dump.jsp%00/index.html")); - data.add(Arguments.of("/dump.jsp%00/")); - data.add(Arguments.of("/dump.jsp%00x/")); + data.add(Arguments.of("/dump.jsp%1e")); + data.add(Arguments.of("/dump.jsp%1ex")); + data.add(Arguments.of("/dump.jsp%1ex/dump.jsp")); + data.add(Arguments.of("/dump.jsp%1e/dump.jsp")); + data.add(Arguments.of("/dump.jsp%1e/index.html")); + data.add(Arguments.of("/dump.jsp%1e/")); + data.add(Arguments.of("/dump.jsp%1ex/")); + // The _00_ is later replaced with a real null character in a customizer + data.add(Arguments.of("/dump.jsp_00_")); + data.add(Arguments.of("/dump.jsp_00_")); + data.add(Arguments.of("/dump.jsp_00_/dump.jsp")); + data.add(Arguments.of("/dump.jsp_00_/dump.jsp")); + data.add(Arguments.of("/dump.jsp_00_/index.html")); + data.add(Arguments.of("/dump.jsp_00_/")); + data.add(Arguments.of("/dump.jsp_00_/")); return data.stream(); } @@ -98,6 +110,17 @@ public static void startServer() throws Exception // add context server.setHandler(context); + // Add customizer to convert "_00_" to a real null + server.getContainedBeans(HttpConfiguration.class).forEach(config -> + { + config.addCustomizer((connector, channelConfig, request) -> + { + HttpURI uri = request.getHttpURI(); + if (uri.getPath().contains("_00_")) + request.setHttpURI(HttpURI.build(uri, uri.getPath().replace("_00_", "\000"), uri.getParam(), uri.getQuery())); + }); + }); + server.start(); int port = ((NetworkConnector)server.getConnectors()[0]).getLocalPort(); From 4cf9ca70c26512d62815cf979a81d1c55c45b74f Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 29 Jun 2021 00:07:56 +0200 Subject: [PATCH 2/6] Issue #6473 - Improve alias checking in PathResource. * Reverted %-escape handling for URI query parts. * Performing canonicalization in ServletContext.getResource(), and improving alias checking in ContextHandler.getResource(). * Performing canonicalization checks in Resource.addPath() to avoid navigation above of the root. * Test added and fixed. * Various cleanups. * Improved javadoc and comments Signed-off-by: Simone Bordet --- .../java/org/eclipse/jetty/http/HttpURI.java | 62 ++++++++----------- .../org/eclipse/jetty/http/HttpURITest.java | 21 +++++++ .../maven/plugin/MavenWebAppContext.java | 1 + .../jetty/rewrite/handler/RedirectUtil.java | 4 +- .../rewrite/handler/ValidUrlRuleTest.java | 14 ++++- .../jetty/server/handler/ContextHandler.java | 12 ++-- .../jetty/server/handler/ResourceHandler.java | 2 - .../jetty/server/HttpConnectionTest.java | 6 ++ .../ContextHandlerGetResourceTest.java | 21 ++++--- .../eclipse/jetty/servlet/RequestURITest.java | 45 +++++++++++++- .../java/org/eclipse/jetty/util/URIUtil.java | 1 - .../jetty/util/resource/PathResource.java | 25 ++++---- .../eclipse/jetty/util/resource/Resource.java | 8 ++- .../jetty/util/resource/URLResource.java | 7 ++- .../jetty/util/URIUtilCanonicalPathTest.java | 20 ++++++ .../jetty/util/resource/ResourceTest.java | 18 ++++++ 16 files changed, 197 insertions(+), 70 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 596f1acc1424..e9db3f2fc5d7 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -34,7 +34,7 @@ * via the static methods such as {@link #build()} and {@link #from(String)}. * * A URI such as - * http://user@host:port/path;param1/%2e/info;param2?query#fragment + * {@code http://user@host:port/path;param1/%2e/info;param2?query#fragment} * this class will split it into the following optional elements:
    *
  • {@link #getScheme()} - http:
    • *
  • {@link #getAuthority()} - //name@host:port
    • @@ -62,11 +62,13 @@ * Thus this class avoid and/or detects such ambiguities. Furthermore, by decoding characters and * removing parameters before relative path normalization, ambiguous paths will be resolved in such * a way to be non-standard-but-non-ambiguous to down stream interpretation of the decoded path string. - * The violations are recorded and available by API such as {@link #hasViolation(Violation)} so that requests + * The violations are recorded and available by API such as {@link #hasAmbiguousSegment()} so that requests * containing them may be rejected in case the non-standard-but-non-ambiguous interpretations - * are not satisfactory for a given compliance configuration. Implementations that wish to - * process ambiguous URI paths must configure the compliance modes to accept them and then perform - * their own decoding of {@link #getPath()}. + * are not satisfactory for a given compliance configuration. + *

    + *

    + * Implementations that wish to process ambiguous URI paths must configure the compliance modes + * to accept them and then perform their own decoding of {@link #getPath()}. *

    *

    * If there are multiple path parameters, only the last one is returned by {@link #getParam()}. @@ -80,32 +82,32 @@ public interface HttpURI enum Violation { /** - * Ambiguous path segments e.g. /foo/%2E%2E/bar + * Ambiguous path segments e.g. {@code /foo/%2E%2E/bar} */ SEGMENT("Ambiguous path segments"), /** - * Ambiguous path separator within a URI segment e.g. /foo%2Fbar + * Ambiguous path separator within a URI segment e.g. {@code /foo%2Fbar} */ SEPARATOR("Ambiguous path separator"), /** - * Ambiguous path parameters within a URI segment e.g. /foo/..;/bar + * Ambiguous path parameters within a URI segment e.g. {@code /foo/..;/bar} */ PARAM("Ambiguous path parameters"), /** - * Ambiguous double encoding within a URI segment e.g. /%2557EB-INF + * Ambiguous double encoding within a URI segment e.g. {@code /%2557EB-INF} */ ENCODING("Ambiguous double encoding"), /** - * Ambiguous empty segments e.g. /foo//bar + * Ambiguous empty segments e.g. {@code /foo//bar} */ EMPTY("Ambiguous empty segments"), /** - * Non standard UTF-16 encoding eg /foo%u2192bar. + * Non standard UTF-16 encoding eg {@code /foo%u2192bar}. */ UTF16("Non standard UTF-16 encoding"); @@ -217,12 +219,12 @@ static Immutable build(HttpURI schemeHostPort, HttpURI uri) boolean isAbsolute(); /** - * @return True if the URI has either an {@link #hasAmbiguousParameter()}, {@link #hasAmbiguousSegment()} or {@link #hasAmbiguousSeparator()}. + * @return True if the URI has any ambiguous {@link Violation}s. */ boolean isAmbiguous(); /** - * @return True if the URI has any Violations. + * @return True if the URI has any {@link Violation}s. */ boolean hasViolations(); @@ -1319,6 +1321,8 @@ else if (c == '/') switch (encodedValue) { case 0: + // Byte 0 cannot be present in a UTF-8 sequence in any position + // other than as the NUL ASCII byte which we do not wish to allow. throw new IllegalArgumentException("Illegal character in path"); case '/': _violations.add(Violation.SEPARATOR); @@ -1405,26 +1409,11 @@ else if (c == '/') } case QUERY: { - switch (c) + if (c == '#') { - case '%': - encodedCharacters = 2; - break; - case 'u': - case 'U': - if (encodedCharacters == 1) - _violations.add(Violation.UTF16); - encodedCharacters = 0; - break; - case '#': - _query = uri.substring(mark, i); - mark = i + 1; - state = State.FRAGMENT; - encodedCharacters = 0; - break; - default: - encodedCharacters = 0; - break; + _query = uri.substring(mark, i); + mark = i + 1; + state = State.FRAGMENT; } break; } @@ -1439,7 +1428,9 @@ else if (c == '/') break; } default: + { throw new IllegalStateException(state.toString()); + } } } @@ -1493,8 +1484,8 @@ else if (_path != null) { // The RFC requires this to be canonical before decoding, but this can leave dot segments and dot dot segments // which are not canonicalized and could be used in an attempt to bypass security checks. - String decodeNonCanonical = URIUtil.decodePath(_path); - _decodedPath = URIUtil.canonicalPath(decodeNonCanonical); + String decodedNonCanonical = URIUtil.decodePath(_path); + _decodedPath = URIUtil.canonicalPath(decodedNonCanonical); if (_decodedPath == null) throw new IllegalArgumentException("Bad URI"); } @@ -1515,7 +1506,8 @@ private void checkSegment(String uri, int segment, int end, boolean param) // This method is called once for every segment parsed. // A URI like "/foo/" has two segments: "foo" and an empty segment. // Empty segments are only ambiguous if they are not the last segment - // So if this method is called for any segment and we have previously seen an empty segment, then it was ambiguous + // So if this method is called for any segment and we have previously + // seen an empty segment, then it was ambiguous. if (_emptySegment) _violations.add(Violation.EMPTY); diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java index 45e20953fa10..0d56bba97fbc 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java @@ -36,6 +36,7 @@ import static org.junit.jupiter.api.Assertions.fail; import static org.junit.jupiter.api.Assumptions.assumeTrue; +// @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck public class HttpURITest { @Test @@ -358,6 +359,7 @@ public static Stream decodePathTests() {"/f%6f%6F/bar", "/foo/bar", EnumSet.noneOf(Violation.class)}, {"/f%u006f%u006F/bar", "/foo/bar", EnumSet.of(Violation.UTF16)}, {"/f%u0001%u0001/bar", "/f\001\001/bar", EnumSet.of(Violation.UTF16)}, + {"/foo/%u20AC/bar", "/foo/\u20AC/bar", EnumSet.of(Violation.UTF16)}, // illegal paths {"//host/../path/info", null, EnumSet.noneOf(Violation.class)}, @@ -369,6 +371,9 @@ public static Stream decodePathTests() {"/path/%u000X/info", null, EnumSet.noneOf(Violation.class)}, {"/path/Fo%u0000/info", null, EnumSet.noneOf(Violation.class)}, {"/path/Fo%00/info", null, EnumSet.noneOf(Violation.class)}, + {"/path/Foo/info%u0000", null, EnumSet.noneOf(Violation.class)}, + {"/path/Foo/info%00", null, EnumSet.noneOf(Violation.class)}, + {"/path/%U20AC", null, EnumSet.noneOf(Violation.class)}, {"%2e%2e/info", null, EnumSet.noneOf(Violation.class)}, {"%u002e%u002e/info", null, EnumSet.noneOf(Violation.class)}, {"%2e%2e;/info", null, EnumSet.noneOf(Violation.class)}, @@ -803,4 +808,20 @@ public void testCompareToJavaNetURI(String input, String scheme, String host, In assertThat("[" + input + "] .fragment", httpUri.getFragment(), is(javaUri.getFragment())); assertThat("[" + input + "] .toString", httpUri.toString(), is(javaUri.toASCIIString())); } + + public static Stream queryData() + { + return Stream.of( + new String[]{"/path?p=%U20AC", "p=%U20AC"}, + new String[]{"/path?p=%u20AC", "p=%u20AC"} + ).map(Arguments::of); + } + + @ParameterizedTest + @MethodSource("queryData") + public void testEncodedQuery(String input, String expectedQuery) + { + HttpURI httpURI = HttpURI.build(input); + assertThat("[" + input + "] .query", httpURI.getQuery(), is(expectedQuery)); + } } diff --git a/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java b/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java index c246f9688511..74a60614ba0a 100644 --- a/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java +++ b/jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/MavenWebAppContext.java @@ -361,6 +361,7 @@ public Resource getResource(String uriInContext) throws MalformedURLException // /WEB-INF/classes if ((resource == null || !resource.exists()) && uriInContext != null && _classes != null) { + // Canonicalize again to look for the resource inside /WEB-INF subdirectories. String uri = URIUtil.canonicalPath(uriInContext); if (uri == null) return null; diff --git a/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectUtil.java b/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectUtil.java index 058dac8293da..750c4ad2ea4c 100644 --- a/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectUtil.java +++ b/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/RedirectUtil.java @@ -48,12 +48,12 @@ public static String toRedirectURL(final HttpServletRequest request, String loca String path = request.getRequestURI(); String parent = (path.endsWith("/")) ? path : URIUtil.parentPath(path); location = URIUtil.canonicalURI(URIUtil.addEncodedPaths(parent, location)); - if (!location.startsWith("/")) + if (location != null && !location.startsWith("/")) url.append('/'); } if (location == null) - throw new IllegalStateException("path cannot be above root"); + throw new IllegalStateException("redirect path cannot be above root"); url.append(location); location = url.toString(); diff --git a/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java b/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java index 363aedb3ad0c..17f370e60948 100644 --- a/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java +++ b/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/ValidUrlRuleTest.java @@ -20,6 +20,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @SuppressWarnings("unused") @@ -71,6 +72,12 @@ public void testInvalidUrlWithMessage() throws Exception @Test public void testInvalidJsp() throws Exception + { + assertThrows(IllegalArgumentException.class, () -> HttpURI.build(_request.getHttpURI(), "/jsp/bean1.jsp%00")); + } + + @Test + public void testInvalidJspWithNullByte() throws Exception { _rule.setCode("405"); _rule.setMessage("foo"); @@ -83,6 +90,12 @@ public void testInvalidJsp() throws Exception assertEquals("foo", _request.getAttribute(Dispatcher.ERROR_MESSAGE)); } + @Test + public void testInvalidShamrock() throws Exception + { + assertThrows(IllegalArgumentException.class, () -> HttpURI.build(_request.getHttpURI(), "/jsp/shamrock-%00%E2%98%98.jsp")); + } + @Test public void testValidShamrock() throws Exception { @@ -106,4 +119,3 @@ public void testCharacters() throws Exception //@checkstyle-enable-check : IllegalTokenText } } - diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 6fe16df2bfb6..f496fa782c63 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -1926,13 +1926,11 @@ public Resource getResource(String path) throws MalformedURLException if (_baseResource == null) return null; - // Does the path go above the current scope? - path = URIUtil.canonicalPath(path); - if (path == null) - return null; - try { + // addPath with accept non-canonical paths that don't go above the root, + // but will treat them as aliases. So unless allowed by an AliasChecker + // they will be rejected below. Resource resource = _baseResource.addPath(path); if (checkAlias(path, resource)) @@ -2274,6 +2272,10 @@ else if (path.charAt(0) != '/') @Override public URL getResource(String path) throws MalformedURLException { + // This is an API call from the application which may have arbitrary non canonical paths passed + // Thus we canonicalize here, to avoid the enforcement of only canonical paths in + // ContextHandler.this.getResource(path). + path = URIUtil.canonicalPath(path); if (path == null) return null; Resource resource = ContextHandler.this.getResource(path); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java index 5a571fada0f2..bd7ba44c5297 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java @@ -168,8 +168,6 @@ public Resource getResource(String path) throws IOException else if (_context != null) { r = _context.getResource(path); - if (r != null) - return r; } if ((r == null || !r.exists()) && path.endsWith("/jetty-dir.css")) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java index a7e56e725a03..b6d301285445 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java @@ -823,6 +823,12 @@ public void testBadUTF8FallsbackTo8859() throws Exception LOG.info("badMessage: bad encoding expected ..."); String response; + response = connector.getResponse("GET /foo/bar%c0%00 HTTP/1.1\r\n" + + "Host: localhost\r\n" + + "Connection: close\r\n" + + "\r\n"); + checkContains(response, 0, "HTTP/1.1 400"); + response = connector.getResponse("GET /bad/utf8%c1 HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: close\r\n" + diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java index 4f021b6c55f8..d304c0bb2160 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerGetResourceTest.java @@ -230,16 +230,23 @@ public void testDoesNotExistResource() throws Exception @Test public void testAlias() throws Exception { - Resource resource = context.getResource("/./index.html"); - assertNotNull(resource); - assertFalse(resource.isAlias()); + String path = "/./index.html"; + Resource resource = context.getResource(path); + assertNull(resource); + URL resourceURL = context.getServletContext().getResource(path); + assertFalse(resourceURL.getPath().contains("/./")); - resource = context.getResource("/down/../index.html"); - assertNotNull(resource); - assertFalse(resource.isAlias()); + path = "/down/../index.html"; + resource = context.getResource(path); + assertNull(resource); + resourceURL = context.getServletContext().getResource(path); + assertFalse(resourceURL.getPath().contains("/../")); - resource = context.getResource("//index.html"); + path = "//index.html"; + resource = context.getResource(path); assertNull(resource); + resourceURL = context.getServletContext().getResource(path); + assertNull(resourceURL); } @ParameterizedTest diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java index 3ee3295d5ea7..13228219a49b 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java @@ -71,7 +71,8 @@ public static Stream data() ret.add(Arguments.of("/hello%u0025world", "/hello%u0025world", null)); ret.add(Arguments.of("/hello-euro-%E2%82%AC", "/hello-euro-%E2%82%AC", null)); ret.add(Arguments.of("/hello-euro?%E2%82%AC", "/hello-euro", "%E2%82%AC")); - // test the ascii control characters (just for completeness) + // Test the ascii control characters (just for completeness). + // Zero is not allowed in UTF-8 sequences so start from 1. for (int i = 0x1; i < 0x1f; i++) { String raw = String.format("/hello%%%02Xworld", i); @@ -196,7 +197,6 @@ public void testGetRequestURIHTTP10(String rawpath, String expectedReqUri, Strin // Read the response. String response = readResponse(client); - // TODO: is HTTP/1.1 response appropriate for an HTTP/1.0 request? assertThat(response, Matchers.containsString("HTTP/1.1 200 OK")); assertThat(response, Matchers.containsString("RequestURI: " + expectedReqUri)); assertThat(response, Matchers.containsString("QueryString: " + expectedQuery)); @@ -223,4 +223,45 @@ public void testGetRequestURIHTTP11(String rawpath, String expectedReqUri, Strin assertThat(response, Matchers.containsString("QueryString: " + expectedQuery)); } } + + public static Stream badData() + { + List ret = new ArrayList<>(); + ret.add(Arguments.of("/hello\000")); + ret.add(Arguments.of("/hello%00")); + ret.add(Arguments.of("/hello%u0000")); + ret.add(Arguments.of("/hello\000/world")); + ret.add(Arguments.of("/hello%00world")); + ret.add(Arguments.of("/hello%u0000world")); + ret.add(Arguments.of("/hello%GG")); + ret.add(Arguments.of("/hello%;/world")); + ret.add(Arguments.of("/hello/../../world")); + ret.add(Arguments.of("/hello/..;/world")); + ret.add(Arguments.of("/hello/..;?/world")); + ret.add(Arguments.of("/hello/%#x/../world")); + ret.add(Arguments.of("/../hello/world")); + ret.add(Arguments.of("/hello%u00u00/world")); + ret.add(Arguments.of("hello")); + + return ret.stream(); + } + + @ParameterizedTest + @MethodSource("badData") + public void testGetBadRequestsURIHTTP10(String rawpath) throws Exception + { + try (Socket client = newSocket(serverURI.getHost(), serverURI.getPort())) + { + OutputStream os = client.getOutputStream(); + + String request = String.format("GET %s HTTP/1.0\r\n\r\n", rawpath); + os.write(request.getBytes(StandardCharsets.ISO_8859_1)); + os.flush(); + + // Read the response. + String response = readResponse(client); + + assertThat(response, Matchers.containsString("HTTP/1.1 400 ")); + } + } } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java b/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java index b91b71c30081..d706b230b010 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java @@ -533,7 +533,6 @@ public static String decodePath(String path, int offset, int length) { throw new IllegalArgumentException("cannot decode URI", e); } - } /* Decode a URI path and strip parameters of ISO-8859-1 path 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 755c4b5f3299..69c438e4adab 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 @@ -86,8 +86,11 @@ private Path checkAliasPath() if (!abs.isAbsolute()) abs = path.toAbsolutePath(); + // Any normalization difference means it's an alias, + // and we don't want to bother further to follow + // symlinks as it's an alias anyway. Path normal = path.normalize(); - if (!abs.equals(normal)) + if (!isSameName(abs, normal)) return normal; try @@ -97,11 +100,8 @@ private Path checkAliasPath() if (Files.exists(path)) { Path real = abs.toRealPath(FOLLOW_LINKS); - if (!isSameName(abs, real)) - { return real; - } } } catch (IOException e) @@ -348,20 +348,23 @@ public boolean isSame(Resource resource) } @Override - public Resource addPath(final String subpath) throws IOException + public Resource addPath(final String subPath) throws IOException { - if ((subpath == null) || (subpath.length() == 0)) - throw new MalformedURLException(subpath); + // Check that the path is within the root, + // but use the original path to create the + // resource, to preserve aliasing. + if (URIUtil.canonicalPath(subPath) == null) + throw new MalformedURLException(subPath); - if ("/".equals(subpath)) + if ("/".equals(subPath)) return this; - // subpaths are always under PathResource - // compensate for input subpaths like "/subdir" + // Sub-paths are always under PathResource + // compensate for input sub-paths like "/subdir" // where default resolve behavior would be // to treat that like an absolute path - return new PathResource(this, subpath); + return new PathResource(this, subPath); } private void assertValidPath(Path path) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java index 03afc1fd2efc..585855dd0cb2 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/Resource.java @@ -419,10 +419,12 @@ public abstract boolean renameTo(Resource dest) * Returns the resource contained inside the current resource with the * given name, which may or may not exist. * - * @param path The path segment to add, which is not encoded + * @param path The path segment to add, which is not encoded. The path may be non canonical, but if so then + * the resulting Resource will return true from {@link #isAlias()}. * @return the Resource for the resolved path within this Resource, never null * @throws IOException if unable to resolve the path - * @throws MalformedURLException if the resolution of the path fails because the input path parameter is malformed. + * @throws MalformedURLException if the resolution of the path fails because the input path parameter is malformed, or + * a relative path attempts to access above the root resource. */ public abstract Resource addPath(String path) throws IOException, MalformedURLException; @@ -477,6 +479,8 @@ public URI getAlias() */ public String getListHTML(String base, boolean parent, String query) throws IOException { + // This method doesn't check aliases, so it is OK to canonicalize here. + base = URIUtil.canonicalPath(base); if (base == null || !isDirectory()) return null; diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java index b15d3e26f07b..6ed655080333 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/URLResource.java @@ -280,8 +280,11 @@ public String[] list() public Resource addPath(String path) throws IOException { - if (path == null) - throw new MalformedURLException("null path"); + // Check that the path is within the root, + // but use the original path to create the + // resource, to preserve aliasing. + if (URIUtil.canonicalPath(path) == null) + throw new MalformedURLException(path); return newResource(URIUtil.addEncodedPaths(_url.toExternalForm(), URIUtil.encodePath(path)), _useCaches); } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java index d8bac6a9ea5f..d6e11da38ce1 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilCanonicalPathTest.java @@ -146,7 +146,9 @@ public void testCanonicalPath(String input, String expectedResult) // Check canonicalURI if (expectedResult == null) + { assertThat(URIUtil.canonicalURI(input), nullValue()); + } else { // mostly encodedURI will be the same @@ -157,4 +159,22 @@ public void testCanonicalPath(String input, String expectedResult) } } + public static Stream queries() + { + String[][] data = + { + {"/ctx/../dir?/../index.html", "/dir?/../index.html"}, + {"/get-files?file=/etc/passwd", "/get-files?file=/etc/passwd"}, + {"/get-files?file=../../../../../passwd", "/get-files?file=../../../../../passwd"} + }; + return Stream.of(data).map(Arguments::of); + } + + @ParameterizedTest + @MethodSource("queries") + public void testQuery(String input, String expectedPath) + { + String actual = URIUtil.canonicalURI(input); + assertThat(actual, is(expectedPath)); + } } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceTest.java index 1ee036f58548..b1787fe2d726 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceTest.java @@ -16,6 +16,7 @@ import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.net.MalformedURLException; import java.net.URI; import java.net.URL; import java.nio.file.Path; @@ -40,6 +41,8 @@ import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; public class ResourceTest { @@ -320,4 +323,19 @@ public void testEqualsWindowsCaseInsensitiveDrive() throws Exception assertEquals(rb, ra); } + + @Test + public void testClimbAboveBase() throws Exception + { + Resource resource = Resource.newResource("/foo/bar"); + assertThrows(MalformedURLException.class, () -> resource.addPath("..")); + + Resource same = resource.addPath("."); + assertNotNull(same); + assertTrue(same.isAlias()); + + assertThrows(MalformedURLException.class, () -> resource.addPath("./..")); + + assertThrows(MalformedURLException.class, () -> resource.addPath("./../bar")); + } } From 7b2d94b286fda992b7dcca500ac7618552a84ea5 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 29 Jun 2021 14:09:51 +1000 Subject: [PATCH 3/6] Fix failing tests. Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/http/HttpURI.java | 22 ------------------- .../org/eclipse/jetty/server/Dispatcher.java | 8 ++++--- .../eclipse/jetty/servlet/RequestURITest.java | 6 +++-- .../java/org/eclipse/jetty/util/URIUtil.java | 12 +++++++++- 4 files changed, 20 insertions(+), 28 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index e9db3f2fc5d7..f1611992be1c 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -18,7 +18,6 @@ import java.util.Collection; import java.util.Collections; import java.util.EnumSet; -import java.util.Set; import org.eclipse.jetty.util.HostPort; import org.eclipse.jetty.util.Index; @@ -178,11 +177,6 @@ static Immutable from(String scheme, String host, int port, String pathQuery) return new Mutable(scheme, host, port, pathQuery).asImmutable(); } - static Immutable build(HttpURI schemeHostPort, HttpURI uri) - { - return new Immutable(schemeHostPort, uri); - } - Immutable asImmutable(); String asString(); @@ -322,22 +316,6 @@ private Immutable(String uri) _decodedPath = null; } - private Immutable(HttpURI schemeHostPort, HttpURI uri) - { - _scheme = schemeHostPort.getScheme(); - _user = schemeHostPort.getUser(); - _host = schemeHostPort.getHost(); - _port = schemeHostPort.getPort(); - _path = uri.getPath(); - _param = uri.getParam(); - _query = uri.getQuery(); - _fragment = uri.getFragment(); - _uri = null; - _decodedPath = uri.getDecodedPath(); - if (uri.hasViolations()) - _violations.addAll(uri.getViolations()); - } - @Override public Immutable asImmutable() { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java index 44a8dfc7eb43..e8a917424864 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java @@ -27,10 +27,8 @@ import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.http.BadMessageException; -import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.UriCompliance; -import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.util.Attributes; import org.eclipse.jetty.util.MultiMap; @@ -185,7 +183,11 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc source_mapping, old_uri.getQuery())); - baseRequest.setHttpURI(HttpURI.build(old_uri, _uri)); + String query = _uri.getQuery(); + if (query == null) + query = old_uri.getQuery(); + + baseRequest.setHttpURI(HttpURI.build(old_uri, _uri.getPath(), _uri.getParam(), query)); baseRequest.setContext(_contextHandler.getServletContext(), _pathInContext); baseRequest.setServletPathMapping(null); diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java index 13228219a49b..8c08fe06be09 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/RequestURITest.java @@ -71,6 +71,10 @@ public static Stream data() ret.add(Arguments.of("/hello%u0025world", "/hello%u0025world", null)); ret.add(Arguments.of("/hello-euro-%E2%82%AC", "/hello-euro-%E2%82%AC", null)); ret.add(Arguments.of("/hello-euro?%E2%82%AC", "/hello-euro", "%E2%82%AC")); + + ret.add(Arguments.of("/hello/..;/world", "/hello/..;/world", null)); + ret.add(Arguments.of("/hello/..;?/world", "/hello/..;", "/world")); + // Test the ascii control characters (just for completeness). // Zero is not allowed in UTF-8 sequences so start from 1. for (int i = 0x1; i < 0x1f; i++) @@ -236,8 +240,6 @@ public static Stream badData() ret.add(Arguments.of("/hello%GG")); ret.add(Arguments.of("/hello%;/world")); ret.add(Arguments.of("/hello/../../world")); - ret.add(Arguments.of("/hello/..;/world")); - ret.add(Arguments.of("/hello/..;?/world")); ret.add(Arguments.of("/hello/%#x/../world")); ret.add(Arguments.of("/../hello/world")); ret.add(Arguments.of("/hello%u00u00/world")); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java b/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java index d706b230b010..1a8b65aad919 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java @@ -785,7 +785,6 @@ public static String parentPath(String p) * @param uri the encoded URI from the path onwards, which may contain query strings and/or fragments * @return the canonical path, or null if path traversal above root. * @see #canonicalPath(String) - * @see #canonicalURI(String) */ public static String canonicalURI(String uri) { @@ -885,6 +884,17 @@ else if (slash) return canonical.toString(); } + /** + * @param path the encoded URI from the path onwards, which may contain query strings and/or fragments + * @return the canonical path, or null if path traversal above root. + * @deprecated Use {@link #canonicalURI(String)} + */ + @Deprecated + public static String canonicalEncodedPath(String path) + { + return canonicalURI(path); + } + /** * Convert a decoded URI path to a canonical form. *

    From 37c657ae1259cdfe946fc348f0f8951eabbc44cb Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 29 Jun 2021 15:59:34 +1000 Subject: [PATCH 4/6] fix javadoc grammar Signed-off-by: Lachlan Roberts --- jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index f1611992be1c..9f9aab0ac1e7 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -34,7 +34,7 @@ * * A URI such as * {@code http://user@host:port/path;param1/%2e/info;param2?query#fragment} - * this class will split it into the following optional elements:

      + * are split into the following optional elements:
        *
      • {@link #getScheme()} - http:
        • *
      • {@link #getAuthority()} - //name@host:port
        • *
      • {@link #getHost()} - host
        • From 66b1cbb60e539ce8ff22a78fdffcf04cddf32c4a Mon Sep 17 00:00:00 2001 From: Lachlan Date: Tue, 29 Jun 2021 17:23:53 +1000 Subject: [PATCH 5/6] Update jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java Co-authored-by: Greg Wilkins --- jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 9f9aab0ac1e7..84e13576db18 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -34,7 +34,7 @@ * * A URI such as * {@code http://user@host:port/path;param1/%2e/info;param2?query#fragment} - * are split into the following optional elements:
          + * is split into the following optional elements:
            *
          • {@link #getScheme()} - http:
            • *
          • {@link #getAuthority()} - //name@host:port
            • *
          • {@link #getHost()} - host
            • From 9d98fe7a4d610d3dacaedd2f0971be6a6590eb9f Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 29 Jun 2021 21:32:01 +1000 Subject: [PATCH 6/6] Compliance mode HttpURI uses UriCompliance.Violation Signed-off-by: Greg Wilkins --- .../java/org/eclipse/jetty/http/HttpURI.java | 184 ++++-------------- .../org/eclipse/jetty/http/UriCompliance.java | 41 +--- .../org/eclipse/jetty/http/HttpURITest.java | 178 ++++++++--------- .../jetty/server/handler/ContextHandler.java | 9 + 4 files changed, 144 insertions(+), 268 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java index 84e13576db18..1294874db368 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -19,6 +19,7 @@ import java.util.Collections; import java.util.EnumSet; +import org.eclipse.jetty.http.UriCompliance.Violation; import org.eclipse.jetty.util.HostPort; import org.eclipse.jetty.util.Index; import org.eclipse.jetty.util.StringUtil; @@ -75,54 +76,6 @@ **/ public interface HttpURI { - /** - * Violations of safe URI interpretations - */ - enum Violation - { - /** - * Ambiguous path segments e.g. {@code /foo/%2E%2E/bar} - */ - SEGMENT("Ambiguous path segments"), - - /** - * Ambiguous path separator within a URI segment e.g. {@code /foo%2Fbar} - */ - SEPARATOR("Ambiguous path separator"), - - /** - * Ambiguous path parameters within a URI segment e.g. {@code /foo/..;/bar} - */ - PARAM("Ambiguous path parameters"), - - /** - * Ambiguous double encoding within a URI segment e.g. {@code /%2557EB-INF} - */ - ENCODING("Ambiguous double encoding"), - - /** - * Ambiguous empty segments e.g. {@code /foo//bar} - */ - EMPTY("Ambiguous empty segments"), - - /** - * Non standard UTF-16 encoding eg {@code /foo%u2192bar}. - */ - UTF16("Non standard UTF-16 encoding"); - - private final String _message; - - Violation(String message) - { - _message = message; - } - - String getMessage() - { - return _message; - } - } - static Mutable build() { return new Mutable(); @@ -236,29 +189,50 @@ static Immutable from(String scheme, String host, int port, String pathQuery) /** * @return True if the URI has a possibly ambiguous segment like '..;' or '%2e%2e' */ - boolean hasAmbiguousSegment(); + default boolean hasAmbiguousSegment() + { + return hasViolation(Violation.AMBIGUOUS_PATH_SEGMENT); + } /** * @return True if the URI empty segment that is ambiguous like '//' or '/;param/'. */ - boolean hasAmbiguousEmptySegment(); + default boolean hasAmbiguousEmptySegment() + { + return hasViolation(Violation.AMBIGUOUS_EMPTY_SEGMENT); + } /** * @return True if the URI has a possibly ambiguous separator of %2f */ - boolean hasAmbiguousSeparator(); + default boolean hasAmbiguousSeparator() + { + return hasViolation(Violation.AMBIGUOUS_PATH_SEPARATOR); + } /** * @return True if the URI has a possibly ambiguous path parameter like '..;' */ - boolean hasAmbiguousParameter(); + default boolean hasAmbiguousParameter() + { + return hasViolation(Violation.AMBIGUOUS_PATH_PARAMETER); + } /** * @return True if the URI has an encoded '%' character. */ - boolean hasAmbiguousEncoding(); + default boolean hasAmbiguousEncoding() + { + return hasViolation(Violation.AMBIGUOUS_PATH_ENCODING); + } - boolean hasUtf16Encoding(); + /** + * @return True if the URI has UTF16 '%u' encodings. + */ + default boolean hasUtf16Encoding() + { + return hasViolation(Violation.UTF16_ENCODINGS); + } default URI toURI() { @@ -466,7 +440,7 @@ public boolean isAbsolute() @Override public boolean isAmbiguous() { - return !_violations.isEmpty() && !(_violations.size() == 1 && _violations.contains(Violation.UTF16)); + return !_violations.isEmpty() && !(_violations.size() == 1 && _violations.contains(Violation.UTF16_ENCODINGS)); } @Override @@ -484,43 +458,7 @@ public boolean hasViolation(Violation violation) @Override public Collection getViolations() { - return Collections.unmodifiableSet(_violations); - } - - @Override - public boolean hasAmbiguousSegment() - { - return _violations.contains(Violation.SEGMENT); - } - - @Override - public boolean hasAmbiguousEmptySegment() - { - return _violations.contains(Violation.EMPTY); - } - - @Override - public boolean hasAmbiguousSeparator() - { - return _violations.contains(Violation.SEPARATOR); - } - - @Override - public boolean hasAmbiguousParameter() - { - return _violations.contains(Violation.PARAM); - } - - @Override - public boolean hasAmbiguousEncoding() - { - return _violations.contains(Violation.ENCODING); - } - - @Override - public boolean hasUtf16Encoding() - { - return _violations.contains(Violation.UTF16); + return Collections.unmodifiableCollection(_violations); } @Override @@ -851,7 +789,7 @@ public boolean isAbsolute() @Override public boolean isAmbiguous() { - return !_violations.isEmpty() && !(_violations.size() == 1 && _violations.contains(Violation.UTF16)); + return !_violations.isEmpty() && !(_violations.size() == 1 && _violations.contains(Violation.UTF16_ENCODINGS)); } @Override @@ -869,43 +807,7 @@ public boolean hasViolation(Violation violation) @Override public Collection getViolations() { - return Collections.unmodifiableSet(_violations); - } - - @Override - public boolean hasAmbiguousSegment() - { - return _violations.contains(Violation.SEGMENT); - } - - @Override - public boolean hasAmbiguousEmptySegment() - { - return _violations.contains(Violation.EMPTY); - } - - @Override - public boolean hasAmbiguousSeparator() - { - return _violations.contains(Violation.SEPARATOR); - } - - @Override - public boolean hasAmbiguousParameter() - { - return _violations.contains(Violation.PARAM); - } - - @Override - public boolean hasAmbiguousEncoding() - { - return _violations.contains(Violation.ENCODING); - } - - @Override - public boolean hasUtf16Encoding() - { - return _violations.contains(Violation.UTF16); + return Collections.unmodifiableCollection(_violations); } public Mutable normalize() @@ -1010,9 +912,9 @@ public Mutable uri(HttpURI uri) _uri = null; _decodedPath = uri.getDecodedPath(); if (uri.hasAmbiguousSeparator()) - _violations.add(Violation.SEPARATOR); + _violations.add(Violation.AMBIGUOUS_PATH_SEPARATOR); if (uri.hasAmbiguousSegment()) - _violations.add(Violation.SEGMENT); + _violations.add(Violation.AMBIGUOUS_PATH_SEGMENT); return this; } @@ -1287,7 +1189,7 @@ else if (c == '/') { if (encodedCharacters == 2 && c == 'u' && !encodedUtf16) { - _violations.add(Violation.UTF16); + _violations.add(Violation.UTF16_ENCODINGS); encodedUtf16 = true; encodedCharacters = 4; continue; @@ -1299,14 +1201,14 @@ else if (c == '/') switch (encodedValue) { case 0: - // Byte 0 cannot be present in a UTF-8 sequence in any position - // other than as the NUL ASCII byte which we do not wish to allow. + // Byte 0 cannot be present in a UTF-8 sequence in any position + // other than as the NUL ASCII byte which we do not wish to allow. throw new IllegalArgumentException("Illegal character in path"); case '/': - _violations.add(Violation.SEPARATOR); + _violations.add(Violation.AMBIGUOUS_PATH_SEPARATOR); break; case '%': - _violations.add(Violation.ENCODING); + _violations.add(Violation.AMBIGUOUS_PATH_ENCODING); break; default: break; @@ -1487,7 +1389,7 @@ private void checkSegment(String uri, int segment, int end, boolean param) // So if this method is called for any segment and we have previously // seen an empty segment, then it was ambiguous. if (_emptySegment) - _violations.add(Violation.EMPTY); + _violations.add(Violation.AMBIGUOUS_EMPTY_SEGMENT); if (end == segment) { @@ -1498,7 +1400,7 @@ private void checkSegment(String uri, int segment, int end, boolean param) // If this empty segment is the first segment then it is ambiguous. if (segment == 0) { - _violations.add(Violation.EMPTY); + _violations.add(Violation.AMBIGUOUS_EMPTY_SEGMENT); return; } @@ -1515,12 +1417,12 @@ private void checkSegment(String uri, int segment, int end, boolean param) if (ambiguous == Boolean.TRUE) { // The segment is always ambiguous. - _violations.add(Violation.SEGMENT); + _violations.add(Violation.AMBIGUOUS_PATH_SEGMENT); } else if (param && ambiguous == Boolean.FALSE) { // The segment is ambiguous only when followed by a parameter. - _violations.add(Violation.PARAM); + _violations.add(Violation.AMBIGUOUS_PATH_PARAMETER); } } } diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java b/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java index b82b13c7e5a5..be8ceffdc77e 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java @@ -14,7 +14,6 @@ package org.eclipse.jetty.http; import java.util.Arrays; -import java.util.EnumMap; import java.util.EnumSet; import java.util.Objects; import java.util.Set; @@ -26,7 +25,6 @@ import static java.util.Arrays.asList; import static java.util.Collections.unmodifiableSet; import static java.util.EnumSet.allOf; -import static java.util.EnumSet.complementOf; import static java.util.EnumSet.noneOf; import static java.util.EnumSet.of; @@ -332,45 +330,12 @@ private static Set copyOf(Set violations) return EnumSet.copyOf(violations); } - private static final EnumMap __uriViolations = new EnumMap<>(HttpURI.Violation.class); - - static - { - // create a map from Violation to compliance in a loop, so that any new violations added are detected with ISE - for (HttpURI.Violation violation : HttpURI.Violation.values()) - { - switch (violation) - { - case SEPARATOR: - __uriViolations.put(violation, Violation.AMBIGUOUS_PATH_SEPARATOR); - break; - case SEGMENT: - __uriViolations.put(violation, Violation.AMBIGUOUS_PATH_SEGMENT); - break; - case PARAM: - __uriViolations.put(violation, Violation.AMBIGUOUS_PATH_PARAMETER); - break; - case ENCODING: - __uriViolations.put(violation, Violation.AMBIGUOUS_PATH_ENCODING); - break; - case EMPTY: - __uriViolations.put(violation, Violation.AMBIGUOUS_EMPTY_SEGMENT); - break; - case UTF16: - __uriViolations.put(violation, Violation.UTF16_ENCODINGS); - break; - default: - throw new IllegalStateException(); - } - } - } - public static String checkUriCompliance(UriCompliance compliance, HttpURI uri) { - for (HttpURI.Violation violation : HttpURI.Violation.values()) + for (UriCompliance.Violation violation : UriCompliance.Violation.values()) { - if (uri.hasViolation(violation) && (compliance == null || !compliance.allows(__uriViolations.get(violation)))) - return violation.getMessage(); + if (uri.hasViolation(violation) && (compliance == null || !compliance.allows(violation))) + return violation.getDescription(); } return null; } diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java index 0d56bba97fbc..83701d915624 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java @@ -19,7 +19,7 @@ import java.util.EnumSet; import java.util.stream.Stream; -import org.eclipse.jetty.http.HttpURI.Violation; +import org.eclipse.jetty.http.UriCompliance.Violation; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -357,9 +357,9 @@ public static Stream decodePathTests() // encoded paths {"/f%6f%6F/bar", "/foo/bar", EnumSet.noneOf(Violation.class)}, - {"/f%u006f%u006F/bar", "/foo/bar", EnumSet.of(Violation.UTF16)}, - {"/f%u0001%u0001/bar", "/f\001\001/bar", EnumSet.of(Violation.UTF16)}, - {"/foo/%u20AC/bar", "/foo/\u20AC/bar", EnumSet.of(Violation.UTF16)}, + {"/f%u006f%u006F/bar", "/foo/bar", EnumSet.of(Violation.UTF16_ENCODINGS)}, + {"/f%u0001%u0001/bar", "/f\001\001/bar", EnumSet.of(Violation.UTF16_ENCODINGS)}, + {"/foo/%u20AC/bar", "/foo/\u20AC/bar", EnumSet.of(Violation.UTF16_ENCODINGS)}, // illegal paths {"//host/../path/info", null, EnumSet.noneOf(Violation.class)}, @@ -390,60 +390,60 @@ public static Stream decodePathTests() {"..;param/info", null, EnumSet.noneOf(Violation.class)}, // ambiguous dot encodings - {"scheme://host/path/%2e/info", "/path/info", EnumSet.of(Violation.SEGMENT)}, - {"scheme:/path/%2e/info", "/path/info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e/info", "/path/info", EnumSet.of(Violation.SEGMENT)}, - {"path/%2e/info/", "path/info/", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e/info", "/info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.SEGMENT)}, - {"%2e/info", "info", EnumSet.of(Violation.SEGMENT)}, - {"%u002e/info", "info", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, - - {"%2e", "", EnumSet.of(Violation.SEGMENT)}, - {"%u002e", "", EnumSet.of(Violation.SEGMENT, Violation.UTF16)}, + {"scheme://host/path/%2e/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"scheme:/path/%2e/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"path/%2e/info/", "path/info/", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"%2e/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"%u002e/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.UTF16_ENCODINGS)}, + + {"%2e", "", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"%u002e", "", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.UTF16_ENCODINGS)}, // empty segment treated as ambiguous - {"/foo//bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, - {"/foo//../bar", "/foo/bar", EnumSet.of(Violation.EMPTY)}, - {"/foo///../../../bar", "/bar", EnumSet.of(Violation.EMPTY)}, + {"/foo//bar", "/foo//bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo//../bar", "/foo/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo///../../../bar", "/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"/foo/./../bar", "/bar", EnumSet.noneOf(Violation.class)}, - {"/foo//./bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, + {"/foo//./bar", "/foo//bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"foo/bar", "foo/bar", EnumSet.noneOf(Violation.class)}, {"foo;/bar", "foo/bar", EnumSet.noneOf(Violation.class)}, - {";/bar", "/bar", EnumSet.of(Violation.EMPTY)}, - {";?n=v", "", EnumSet.of(Violation.EMPTY)}, + {";/bar", "/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {";?n=v", "", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"?n=v", "", EnumSet.noneOf(Violation.class)}, {"#n=v", "", EnumSet.noneOf(Violation.class)}, {"", "", EnumSet.noneOf(Violation.class)}, {"http:/foo", "/foo", EnumSet.noneOf(Violation.class)}, // ambiguous parameter inclusions - {"/path/.;/info", "/path/info", EnumSet.of(Violation.PARAM)}, - {"/path/.;param/info", "/path/info", EnumSet.of(Violation.PARAM)}, - {"/path/..;/info", "/info", EnumSet.of(Violation.PARAM)}, - {"/path/..;param/info", "/info", EnumSet.of(Violation.PARAM)}, - {".;/info", "info", EnumSet.of(Violation.PARAM)}, - {".;param/info", "info", EnumSet.of(Violation.PARAM)}, + {"/path/.;/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/.;param/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/..;/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/..;param/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {".;/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {".;param/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, // ambiguous segment separators - {"/path/%2f/info", "/path///info", EnumSet.of(Violation.SEPARATOR)}, - {"%2f/info", "//info", EnumSet.of(Violation.SEPARATOR)}, - {"%2F/info", "//info", EnumSet.of(Violation.SEPARATOR)}, - {"/path/%2f../info", "/path/info", EnumSet.of(Violation.SEPARATOR)}, + {"/path/%2f/info", "/path///info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, + {"%2f/info", "//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, + {"%2F/info", "//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, + {"/path/%2f../info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, // ambiguous encoding - {"/path/%25/info", "/path/%/info", EnumSet.of(Violation.ENCODING)}, - {"/path/%u0025/info", "/path/%/info", EnumSet.of(Violation.ENCODING, Violation.UTF16)}, - {"%25/info", "%/info", EnumSet.of(Violation.ENCODING)}, - {"/path/%25../info", "/path/%../info", EnumSet.of(Violation.ENCODING)}, - {"/path/%u0025../info", "/path/%../info", EnumSet.of(Violation.ENCODING, Violation.UTF16)}, + {"/path/%25/info", "/path/%/info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING)}, + {"/path/%u0025/info", "/path/%/info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING, Violation.UTF16_ENCODINGS)}, + {"%25/info", "%/info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING)}, + {"/path/%25../info", "/path/%../info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING)}, + {"/path/%u0025../info", "/path/%../info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING, Violation.UTF16_ENCODINGS)}, // combinations - {"/path/%2f/..;/info", "/path//info", EnumSet.of(Violation.SEPARATOR, Violation.PARAM)}, - {"/path/%u002f/..;/info", "/path//info", EnumSet.of(Violation.SEPARATOR, Violation.PARAM, Violation.UTF16)}, - {"/path/%2f/..;/%2e/info", "/path//info", EnumSet.of(Violation.SEPARATOR, Violation.PARAM, Violation.SEGMENT)}, + {"/path/%2f/..;/info", "/path//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/%u002f/..;/info", "/path//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_PARAMETER, Violation.UTF16_ENCODINGS)}, + {"/path/%2f/..;/%2e/info", "/path//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_PARAMETER, Violation.AMBIGUOUS_PATH_SEGMENT)}, // Non ascii characters // @checkstyle-disable-check : AvoidEscapedUnicodeCharactersCheck @@ -462,15 +462,15 @@ public void testDecodedPath(String input, String decodedPath, EnumSet HttpURI uri = HttpURI.from(input); assertThat(uri.getDecodedPath(), is(decodedPath)); EnumSet ambiguous = EnumSet.copyOf(expected); - ambiguous.retainAll(EnumSet.complementOf(EnumSet.of(Violation.UTF16))); + ambiguous.retainAll(EnumSet.complementOf(EnumSet.of(Violation.UTF16_ENCODINGS))); assertThat(uri.isAmbiguous(), is(!ambiguous.isEmpty())); - assertThat(uri.hasAmbiguousSegment(), is(ambiguous.contains(Violation.SEGMENT))); - assertThat(uri.hasAmbiguousSeparator(), is(ambiguous.contains(Violation.SEPARATOR))); - assertThat(uri.hasAmbiguousParameter(), is(ambiguous.contains(Violation.PARAM))); - assertThat(uri.hasAmbiguousEncoding(), is(ambiguous.contains(Violation.ENCODING))); + assertThat(uri.hasAmbiguousSegment(), is(ambiguous.contains(Violation.AMBIGUOUS_PATH_SEGMENT))); + assertThat(uri.hasAmbiguousSeparator(), is(ambiguous.contains(Violation.AMBIGUOUS_PATH_SEPARATOR))); + assertThat(uri.hasAmbiguousParameter(), is(ambiguous.contains(Violation.AMBIGUOUS_PATH_PARAMETER))); + assertThat(uri.hasAmbiguousEncoding(), is(ambiguous.contains(Violation.AMBIGUOUS_PATH_ENCODING))); - assertThat(uri.hasUtf16Encoding(), is(expected.contains(Violation.UTF16))); + assertThat(uri.hasUtf16Encoding(), is(expected.contains(Violation.UTF16_ENCODINGS))); } catch (Exception e) { @@ -507,69 +507,69 @@ public static Stream testPathQueryTests() {"..;param/info", null, null}, // ambiguous dot encodings - {"/path/%2e/info", "/path/info", EnumSet.of(Violation.SEGMENT)}, - {"path/%2e/info/", "path/info/", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e/info", "/info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.SEGMENT)}, - {"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.SEGMENT)}, - {"%2e/info", "info", EnumSet.of(Violation.SEGMENT)}, - {"%2e", "", EnumSet.of(Violation.SEGMENT)}, + {"/path/%2e/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"path/%2e/info/", "path/info/", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e;/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e;param/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2e%2e;param;other/info;other", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"%2e/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"%2e", "", EnumSet.of(Violation.AMBIGUOUS_PATH_SEGMENT)}, // empty segment treated as ambiguous {"/", "/", EnumSet.noneOf(Violation.class)}, {"/#", "/", EnumSet.noneOf(Violation.class)}, {"/path", "/path", EnumSet.noneOf(Violation.class)}, {"/path/", "/path/", EnumSet.noneOf(Violation.class)}, - {"//", "//", EnumSet.of(Violation.EMPTY)}, - {"/foo//", "/foo//", EnumSet.of(Violation.EMPTY)}, - {"/foo//bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, - {"//foo/bar", "//foo/bar", EnumSet.of(Violation.EMPTY)}, + {"//", "//", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo//", "/foo//", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo//bar", "/foo//bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"//foo/bar", "//foo/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"/foo?bar", "/foo", EnumSet.noneOf(Violation.class)}, {"/foo#bar", "/foo", EnumSet.noneOf(Violation.class)}, {"/foo;bar", "/foo", EnumSet.noneOf(Violation.class)}, {"/foo/?bar", "/foo/", EnumSet.noneOf(Violation.class)}, {"/foo/#bar", "/foo/", EnumSet.noneOf(Violation.class)}, {"/foo/;param", "/foo/", EnumSet.noneOf(Violation.class)}, - {"/foo/;param/bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, - {"/foo//bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, - {"/foo//bar//", "/foo//bar//", EnumSet.of(Violation.EMPTY)}, - {"//foo//bar//", "//foo//bar//", EnumSet.of(Violation.EMPTY)}, - {"/foo//../bar", "/foo/bar", EnumSet.of(Violation.EMPTY)}, - {"/foo///../../../bar", "/bar", EnumSet.of(Violation.EMPTY)}, + {"/foo/;param/bar", "/foo//bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo//bar", "/foo//bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo//bar//", "/foo//bar//", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"//foo//bar//", "//foo//bar//", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo//../bar", "/foo/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {"/foo///../../../bar", "/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"/foo/./../bar", "/bar", EnumSet.noneOf(Violation.class)}, - {"/foo//./bar", "/foo//bar", EnumSet.of(Violation.EMPTY)}, + {"/foo//./bar", "/foo//bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"foo/bar", "foo/bar", EnumSet.noneOf(Violation.class)}, {"foo;/bar", "foo/bar", EnumSet.noneOf(Violation.class)}, - {";/bar", "/bar", EnumSet.of(Violation.EMPTY)}, - {";?n=v", "", EnumSet.of(Violation.EMPTY)}, + {";/bar", "/bar", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, + {";?n=v", "", EnumSet.of(Violation.AMBIGUOUS_EMPTY_SEGMENT)}, {"?n=v", "", EnumSet.noneOf(Violation.class)}, {"#n=v", "", EnumSet.noneOf(Violation.class)}, {"", "", EnumSet.noneOf(Violation.class)}, // ambiguous parameter inclusions - {"/path/.;/info", "/path/info", EnumSet.of(Violation.PARAM)}, - {"/path/.;param/info", "/path/info", EnumSet.of(Violation.PARAM)}, - {"/path/..;/info", "/info", EnumSet.of(Violation.PARAM)}, - {"/path/..;param/info", "/info", EnumSet.of(Violation.PARAM)}, - {".;/info", "info", EnumSet.of(Violation.PARAM)}, - {".;param/info", "info", EnumSet.of(Violation.PARAM)}, + {"/path/.;/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/.;param/info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/..;/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/..;param/info", "/info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {".;/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, + {".;param/info", "info", EnumSet.of(Violation.AMBIGUOUS_PATH_PARAMETER)}, // ambiguous segment separators - {"/path/%2f/info", "/path///info", EnumSet.of(Violation.SEPARATOR)}, - {"%2f/info", "//info", EnumSet.of(Violation.SEPARATOR)}, - {"%2F/info", "//info", EnumSet.of(Violation.SEPARATOR)}, - {"/path/%2f../info", "/path/info", EnumSet.of(Violation.SEPARATOR)}, + {"/path/%2f/info", "/path///info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, + {"%2f/info", "//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, + {"%2F/info", "//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, + {"/path/%2f../info", "/path/info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR)}, // ambiguous encoding - {"/path/%25/info", "/path/%/info", EnumSet.of(Violation.ENCODING)}, - {"%25/info", "%/info", EnumSet.of(Violation.ENCODING)}, - {"/path/%25../info", "/path/%../info", EnumSet.of(Violation.ENCODING)}, + {"/path/%25/info", "/path/%/info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING)}, + {"%25/info", "%/info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING)}, + {"/path/%25../info", "/path/%../info", EnumSet.of(Violation.AMBIGUOUS_PATH_ENCODING)}, // combinations - {"/path/%2f/..;/info", "/path//info", EnumSet.of(Violation.SEPARATOR, Violation.PARAM)}, - {"/path/%2f/..;/%2e/info", "/path//info", EnumSet.of(Violation.SEPARATOR, Violation.PARAM, Violation.SEGMENT)}, - {"/path/%2f/%25/..;/%2e//info", "/path////info", EnumSet.of(Violation.SEPARATOR, Violation.PARAM, Violation.SEGMENT, Violation.ENCODING, Violation.EMPTY)}, + {"/path/%2f/..;/info", "/path//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_PARAMETER)}, + {"/path/%2f/..;/%2e/info", "/path//info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_PARAMETER, Violation.AMBIGUOUS_PATH_SEGMENT)}, + {"/path/%2f/%25/..;/%2e//info", "/path////info", EnumSet.of(Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_PARAMETER, Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_ENCODING, Violation.AMBIGUOUS_EMPTY_SEGMENT)}, }).map(Arguments::of); } @@ -587,11 +587,11 @@ public void testPathQuery(String input, String decodedPath, EnumSet e HttpURI uri = HttpURI.build().pathQuery(input); assertThat(uri.getDecodedPath(), is(decodedPath)); assertThat(uri.isAmbiguous(), is(!expected.isEmpty())); - assertThat(uri.hasAmbiguousEmptySegment(), is(expected.contains(Violation.EMPTY))); - assertThat(uri.hasAmbiguousSegment(), is(expected.contains(Violation.SEGMENT))); - assertThat(uri.hasAmbiguousSeparator(), is(expected.contains(Violation.SEPARATOR))); - assertThat(uri.hasAmbiguousParameter(), is(expected.contains(Violation.PARAM))); - assertThat(uri.hasAmbiguousEncoding(), is(expected.contains(Violation.ENCODING))); + assertThat(uri.hasAmbiguousEmptySegment(), is(expected.contains(Violation.AMBIGUOUS_EMPTY_SEGMENT))); + assertThat(uri.hasAmbiguousSegment(), is(expected.contains(Violation.AMBIGUOUS_PATH_SEGMENT))); + assertThat(uri.hasAmbiguousSeparator(), is(expected.contains(Violation.AMBIGUOUS_PATH_SEPARATOR))); + assertThat(uri.hasAmbiguousParameter(), is(expected.contains(Violation.AMBIGUOUS_PATH_PARAMETER))); + assertThat(uri.hasAmbiguousEncoding(), is(expected.contains(Violation.AMBIGUOUS_PATH_ENCODING))); } public static Stream parseData() diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index f496fa782c63..9e4735621c3a 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -2222,6 +2222,7 @@ public RequestDispatcher getRequestDispatcher(String uriInContext) try { String contextPath = getContextPath(); + // uriInContext is canonicalized by HttpURI. HttpURI.Mutable uri = HttpURI.build(uriInContext); String pathInfo = uri.getDecodedPath(); if (StringUtil.isEmpty(pathInfo)) @@ -2244,6 +2245,10 @@ public RequestDispatcher getRequestDispatcher(String uriInContext) @Override public String getRealPath(String path) { + // This is an API call from the application which may have arbitrary non canonical paths passed + // Thus we canonicalize here, to avoid the enforcement of only canonical paths in + // ContextHandler.this.getResource(path). + path = URIUtil.canonicalPath(path); if (path == null) return null; if (path.length() == 0) @@ -2308,6 +2313,10 @@ public InputStream getResourceAsStream(String path) @Override public Set getResourcePaths(String path) { + // This is an API call from the application which may have arbitrary non canonical paths passed + // Thus we canonicalize here, to avoid the enforcement of only canonical paths in + // ContextHandler.this.getResource(path). + path = URIUtil.canonicalPath(path); if (path == null) return null; return ContextHandler.this.getResourcePaths(path);