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 278af07753b..8d4a2cd58ba 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 @@ -824,12 +824,11 @@ public class HttpConnectionTest @Test public void testBadURIencoding() throws Exception { - // The URI is being leniently decoded, leaving the "%x" alone String response = connector.getResponse("GET /bad/encoding%x HTTP/1.1\r\n" + "Host: localhost\r\n" + "Connection: close\r\n" + "\r\n"); - checkContains(response, 0, "HTTP/1.1 200"); + checkContains(response, 0, "HTTP/1.1 400"); } @Test diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java index 7da99af1b56..4bda5549721 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java @@ -251,6 +251,24 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture } } + @Test + public void testBadURI() throws Exception + { + configureServer(new HelloWorldHandler()); + + try (Socket client = newSocket(_serverURI.getHost(), _serverURI.getPort())) + { + OutputStream os = client.getOutputStream(); + + os.write("GET /%xx HTTP/1.0\r\n\r\n".getBytes(StandardCharsets.ISO_8859_1)); + os.flush(); + + // Read the response. + String response = readResponse(client); + + assertThat(response, Matchers.containsString("HTTP/1.1 400 ")); + } + } @Test public void testExceptionThrownInHandlerLoop() throws Exception { 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 73a3e77e705..e41f8b8127a 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 @@ -470,70 +470,25 @@ public class URIUtil builder = new Utf8StringBuilder(path.length()); builder.append(path, offset, i - offset); } - - // lenient percent decoding - if (i >= end) + if ((i + 2) < end) { - // [LENIENT] a percent sign at end of string. - builder.append('%'); - i = end; - } - else if (end > (i + 1)) - { - char type = path.charAt(i + 1); - if (type == 'u') + char u = path.charAt(i + 1); + if (u == 'u') { - // We have a possible (deprecated) microsoft unicode code point "%u####" - // - not recommended to use as it's limited to 2 bytes. - if ((i + 5) >= end) - { - // [LENIENT] we have a partial "%u####" at the end of a string. - builder.append(path, i, (end - i)); - i = end; - } - else - { - // this seems wrong, as we are casting to a char, but that's the known - // limitation of this deprecated encoding (only 2 bytes allowed) - if (StringUtil.isHex(path, i + 2, 4)) - { - builder.append((char)(0xffff & TypeUtil.parseInt(path, i + 2, 4, 16))); - i += 5; - } - else - { - // [LENIENT] copy the "%u" as-is. - builder.append(path, i, 2); - i += 1; - } - } - } - else if (end > (i + 2)) - { - // we have a possible "%##" encoding - if (StringUtil.isHex(path, i + 1, 2)) - { - builder.append((byte)TypeUtil.parseInt(path, i + 1, 2, 16)); - i += 2; - } - else - { - builder.append(path, i, 3); - i += 2; - } + // TODO remove %u support in jetty-10 + // this is wrong. This is a codepoint not a char + builder.append((char)(0xffff & TypeUtil.parseInt(path, i + 2, 4, 16))); + i += 5; } else { - // [LENIENT] incomplete "%##" sequence at end of string - builder.append(path, i, (end - i)); - i = end; + builder.append((byte)(0xff & (TypeUtil.convertHexDigit(u) * 16 + TypeUtil.convertHexDigit(path.charAt(i + 2))))); + i += 2; } } else { - // [LENIENT] the "%" at the end of the string - builder.append(path, i, (end - i)); - i = end; + throw new IllegalArgumentException("Bad URI % encoding"); } break; @@ -571,10 +526,18 @@ public class URIUtil } catch (NotUtf8Exception e) { - LOG.warn(path.substring(offset, offset + length) + " " + e); - LOG.debug(e); + LOG.debug(path.substring(offset, offset + length) + " " + e); return decodeISO88591Path(path, offset, length); } + catch (IllegalArgumentException e) + { + throw e; + } + catch (Exception e) + { + throw new IllegalArgumentException("cannot decode URI", e); + } + } /* Decode a URI path and strip parameters of ISO-8859-1 path @@ -599,13 +562,14 @@ public class URIUtil char u = path.charAt(i + 1); if (u == 'u') { - // TODO this is wrong. This is a codepoint not a char + // TODO remove %u encoding support in jetty-10 + // This is wrong. This is a codepoint not a char builder.append((char)(0xffff & TypeUtil.parseInt(path, i + 2, 4, 16))); i += 5; } else { - builder.append((byte)(0xff & (TypeUtil.convertHexDigit(u) * 16 + TypeUtil.convertHexDigit(path.charAt(i + 2))))); + builder.append((char)(0xff & (TypeUtil.convertHexDigit(u) * 16 + TypeUtil.convertHexDigit(path.charAt(i + 2))))); i += 2; } } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java index 1354b2e3398..d3ba74a0e40 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/URIUtilTest.java @@ -33,6 +33,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; 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; /** @@ -94,18 +95,6 @@ public class URIUtilTest // Deprecated Microsoft Percent-U encoding arguments.add(Arguments.of("abc%u3040", "abc\u3040")); - - // Lenient decode - arguments.add(Arguments.of("abc%xyz", "abc%xyz")); // not a "%##" - arguments.add(Arguments.of("abc%", "abc%")); // percent at end of string - arguments.add(Arguments.of("abc%A", "abc%A")); // incomplete "%##" at end of string - arguments.add(Arguments.of("abc%uvwxyz", "abc%uvwxyz")); // not a valid "%u####" - arguments.add(Arguments.of("abc%uEFGHIJ", "abc%uEFGHIJ")); // not a valid "%u####" - arguments.add(Arguments.of("abc%uABC", "abc%uABC")); // incomplete "%u####" - arguments.add(Arguments.of("abc%uAB", "abc%uAB")); // incomplete "%u####" - arguments.add(Arguments.of("abc%uA", "abc%uA")); // incomplete "%u####" - arguments.add(Arguments.of("abc%u", "abc%u")); // incomplete "%u####" - return arguments.stream(); } @@ -117,6 +106,60 @@ public class URIUtilTest assertEquals(expectedPath, path); } + public static Stream decodeBadPathSource() + { + List arguments = new ArrayList<>(); + + // Test for null character (real world ugly test case) + // TODO is this a bad decoding or a bad URI ? + // arguments.add(Arguments.of("/%00/")); + + // Deprecated Microsoft Percent-U encoding + // TODO still supported for now ? + // arguments.add(Arguments.of("abc%u3040")); + + // Bad %## encoding + arguments.add(Arguments.of("abc%xyz")); + + // Incomplete %## encoding + arguments.add(Arguments.of("abc%")); + arguments.add(Arguments.of("abc%A")); + + // Invalid microsoft %u#### encoding + arguments.add(Arguments.of("abc%uvwxyz")); + arguments.add(Arguments.of("abc%uEFGHIJ")); + + // Incomplete microsoft %u#### encoding + arguments.add(Arguments.of("abc%uABC")); + arguments.add(Arguments.of("abc%uAB")); + arguments.add(Arguments.of("abc%uA")); + arguments.add(Arguments.of("abc%u")); + + // Invalid UTF-8 and ISO8859-1 + // TODO currently ISO8859 is too forgiving to detect these + /* + arguments.add(Arguments.of("abc%C3%28")); // invalid 2 octext sequence + arguments.add(Arguments.of("abc%A0%A1")); // invalid 2 octext sequence + arguments.add(Arguments.of("abc%e2%28%a1")); // invalid 3 octext sequence + arguments.add(Arguments.of("abc%e2%82%28")); // invalid 3 octext sequence + arguments.add(Arguments.of("abc%f0%28%8c%bc")); // invalid 4 octext sequence + arguments.add(Arguments.of("abc%f0%90%28%bc")); // invalid 4 octext sequence + arguments.add(Arguments.of("abc%f0%28%8c%28")); // invalid 4 octext sequence + arguments.add(Arguments.of("abc%f8%a1%a1%a1%a1")); // valid sequence, but not unicode + arguments.add(Arguments.of("abc%fc%a1%a1%a1%a1%a1")); // valid sequence, but not unicode + arguments.add(Arguments.of("abc%f8%a1%a1%a1")); // incomplete sequence + */ + + return arguments.stream(); + } + + @ParameterizedTest(name = "[{index}] {0}") + @MethodSource("decodeBadPathSource") + public void testBadDecodePath(String encodedPath) + { + assertThrows(IllegalArgumentException.class, () -> URIUtil.decodePath(encodedPath)); + } + @Test public void testDecodePathSubstring() { diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceAliasTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceAliasTest.java index bb668a183d6..263117fa99f 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceAliasTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceAliasTest.java @@ -18,93 +18,155 @@ package org.eclipse.jetty.util.resource; -import java.io.File; +import java.io.IOException; import java.net.MalformedURLException; +import java.nio.file.Files; +import java.nio.file.InvalidPathException; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Function; +import java.util.stream.Stream; import org.eclipse.jetty.toolchain.test.FS; -import org.eclipse.jetty.toolchain.test.MavenTestingUtils; -import org.junit.jupiter.api.BeforeAll; -import org.junit.jupiter.api.BeforeEach; +import org.eclipse.jetty.toolchain.test.jupiter.WorkDir; +import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension; +import org.eclipse.jetty.util.log.Log; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +@ExtendWith(WorkDirExtension.class) public class ResourceAliasTest { - static File __dir; + public WorkDir workDir; - @BeforeAll - public static void beforeClass() + public static Stream> resourceTypes() { - __dir = MavenTestingUtils.getTargetTestingDir("RAT"); + List> types = new ArrayList<>(); + + types.add((path) -> new PathResource(path)); + types.add((path) -> new FileResource(path.toFile())); + + return types.stream(); } - @BeforeEach - public void before() + @ParameterizedTest + @MethodSource("resourceTypes") + public void testPercentPaths(Function resourceType) throws IOException { - FS.ensureDirExists(__dir); - FS.ensureEmpty(__dir); + Path baseDir = workDir.getEmptyPathDir(); + + Path foo = baseDir.resolve("%foo"); + Files.createDirectories(foo); + + Path bar = foo.resolve("bar%"); + Files.createDirectories(bar); + + Path text = bar.resolve("test.txt"); + FS.touch(text); + + // At this point we have a path .../%foo/bar%/test.txt present on the filesystem. + // This would also apply for paths found in JAR files (like META-INF/resources/%foo/bar%/test.txt) + + assertTrue(Files.exists(text)); + + Resource baseResource = resourceType.apply(baseDir); + assertTrue(baseResource.exists(), "baseResource exists"); + + Resource fooResource = baseResource.addPath("%foo"); + assertTrue(fooResource.exists(), "fooResource exists"); + assertTrue(fooResource.isDirectory(), "fooResource isDir"); + if (fooResource instanceof FileResource) + assertTrue(fooResource.isAlias(), "fooResource isAlias"); + else + assertFalse(fooResource.isAlias(), "fooResource isAlias"); + + Resource barResource = fooResource.addPath("bar%"); + assertTrue(barResource.exists(), "barResource exists"); + assertTrue(barResource.isDirectory(), "barResource isDir"); + if (fooResource instanceof FileResource) + assertTrue(barResource.isAlias(), "barResource isAlias"); + else + assertFalse(barResource.isAlias(), "barResource isAlias"); + + Resource textResource = barResource.addPath("test.txt"); + assertTrue(textResource.exists(), "textResource exists"); + assertFalse(textResource.isDirectory(), "textResource isDir"); } @Test public void testNullCharEndingFilename() throws Exception { - File file = new File(__dir, "test.txt"); - assertFalse(file.exists()); - assertTrue(file.createNewFile()); - assertTrue(file.exists()); + Path baseDir = workDir.getEmptyPathDir(); - File file0 = new File(__dir, "test.txt\0"); - if (!file0.exists()) - return; // this file system does not suffer this problem - - assertTrue(file0.exists()); // This is an alias! - - Resource dir = Resource.newResource(__dir); - - // Test not alias paths - Resource resource = Resource.newResource(file); - assertTrue(resource.exists()); - assertNull(resource.getAlias()); - resource = Resource.newResource(file.getAbsoluteFile()); - assertTrue(resource.exists()); - assertNull(resource.getAlias()); - resource = Resource.newResource(file.toURI()); - assertTrue(resource.exists()); - assertNull(resource.getAlias()); - resource = Resource.newResource(file.toURI().toString()); - assertTrue(resource.exists()); - assertNull(resource.getAlias()); - resource = dir.addPath("test.txt"); - assertTrue(resource.exists()); - assertNull(resource.getAlias()); - - // Test alias paths - resource = Resource.newResource(file0); - assertTrue(resource.exists()); - assertNotNull(resource.getAlias()); - resource = Resource.newResource(file0.getAbsoluteFile()); - assertTrue(resource.exists()); - assertNotNull(resource.getAlias()); - resource = Resource.newResource(file0.toURI()); - assertTrue(resource.exists()); - assertNotNull(resource.getAlias()); - resource = Resource.newResource(file0.toURI().toString()); - assertTrue(resource.exists()); - assertNotNull(resource.getAlias()); + Path file = baseDir.resolve("test.txt"); + FS.touch(file); try { - resource = dir.addPath("test.txt\0"); + Path file0 = baseDir.resolve("test.txt\0"); + if (!Files.exists(file0)) + return; // this file system does get tricked by ending filenames + + assertThat(file0 + " exists", Files.exists(file0), is(true)); // This is an alias! + + Resource dir = Resource.newResource(baseDir); + + // Test not alias paths + Resource resource = Resource.newResource(file); + assertTrue(resource.exists()); + assertNull(resource.getAlias()); + resource = Resource.newResource(file.toAbsolutePath()); + assertTrue(resource.exists()); + assertNull(resource.getAlias()); + resource = Resource.newResource(file.toUri()); + assertTrue(resource.exists()); + assertNull(resource.getAlias()); + resource = Resource.newResource(file.toUri().toString()); + assertTrue(resource.exists()); + assertNull(resource.getAlias()); + resource = dir.addPath("test.txt"); + assertTrue(resource.exists()); + assertNull(resource.getAlias()); + + // Test alias paths + resource = Resource.newResource(file0); assertTrue(resource.exists()); assertNotNull(resource.getAlias()); + resource = Resource.newResource(file0.toAbsolutePath()); + assertTrue(resource.exists()); + assertNotNull(resource.getAlias()); + resource = Resource.newResource(file0.toUri()); + assertTrue(resource.exists()); + assertNotNull(resource.getAlias()); + resource = Resource.newResource(file0.toUri().toString()); + assertTrue(resource.exists()); + assertNotNull(resource.getAlias()); + + try + { + resource = dir.addPath("test.txt\0"); + assertTrue(resource.exists()); + assertNotNull(resource.getAlias()); + } + catch (MalformedURLException e) + { + assertTrue(true); + } } - catch (MalformedURLException e) + catch (InvalidPathException e) { - assertTrue(true); + // this file system does allow null char ending filenames + Log.getRootLogger().ignore(e); } } }