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 <simone.bordet@gmail.com>

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
- * <code>http://user@host:port/path;param1/%2e/info;param2?query#fragment</code>
+ * {@code http://user@host:port/path;param1/%2e/info;param2?query#fragment}
  * this class will split it into the following optional elements:<ul>
  * <li>{@link #getScheme()} - http:</li>
  * <li>{@link #getAuthority()} - //name@host:port</li>
@@ -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.
+ * </p>
+ * <p>
+ * 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()}.
  * </p>
  * <p>
  * 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. <code>/foo/%2E%2E/bar</code>
+         * 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</code>
+         * 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</code>
+         * 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</code>
+         * Ambiguous double encoding within a URI segment e.g. {@code /%2557EB-INF}
          */
         ENCODING("Ambiguous double encoding"),
 
         /**
-         * Ambiguous empty segments e.g. <code>/foo//bar</code>
+         * Ambiguous empty segments e.g. {@code /foo//bar}
          */
         EMPTY("Ambiguous empty segments"),
 
         /**
-         * Non standard UTF-16 encoding eg <code>/foo%u2192bar</code>.
+         * 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);
 

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<Arguments> 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<Arguments> 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<Arguments> 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));
+    }
 }

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;

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();

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
     }
 }
-

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);

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"))

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" +

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