Issue #6473 - canonicalPath refactor & fix alias check in PathResourc…

…e (Jetty-10) (#6478) Issue #6473 - canonicalPath refactor & fix alias check 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 * Compliance mode HttpURI uses UriCompliance.Violation Signed-off-by: Lachlan Roberts <lachlan@webtide.com> Signed-off-by: Simone Bordet <simone.bordet@gmail.com> Co-authored-by: Greg Wilkins <gregw@webtide.com>
jetty · Jun 29, 2021 · 3c32afa · 3c32afa
1 parent c753ca0
commit 3c32afa
Show file tree

Hide file tree

Showing 29 changed files with 768 additions and 516 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
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
@@ -25,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;
 
@@ -67,10 +66,6 @@ public enum Violation implements ComplianceViolation
          * Allow ambiguous path encoding within a URI segment e.g. <code>/%2557EB-INF</code>
          */
         AMBIGUOUS_PATH_ENCODING("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path encoding"),
-        /**
-         * Allow Non canonical ambiguous paths. eg <code>/foo/x%2f%2e%2e%/bar</code> provided to applications as <code>/foo/x/../bar</code>
-         */
-        NON_CANONICAL_AMBIGUOUS_PATHS("https://tools.ietf.org/html/rfc3986#section-3.3", "Non canonical ambiguous paths"),
         /**
          * Allow UTF-16 encoding eg <code>/foo%u2192bar</code>.
          */
@@ -125,10 +120,9 @@ public String getDescription()
 
     /**
      * Compliance mode that exactly follows <a href="https://tools.ietf.org/html/rfc3986">RFC3986</a>,
-     * including allowing all additional ambiguous URI Violations,
-     * except {@link Violation#NON_CANONICAL_AMBIGUOUS_PATHS}, thus ambiguous paths are canonicalized for safety.
+     * including allowing all additional ambiguous URI Violations.
      */
-    public static final UriCompliance RFC3986 = new UriCompliance("RFC3986", complementOf(of(Violation.NON_CANONICAL_AMBIGUOUS_PATHS)));
+    public static final UriCompliance RFC3986 = new UriCompliance("RFC3986", allOf(Violation.class));
 
     /**
      * Compliance mode that follows <a href="https://tools.ietf.org/html/rfc3986">RFC3986</a>
@@ -231,6 +225,11 @@ public static UriCompliance from(String spec)
             boolean exclude = element.startsWith("-");
             if (exclude)
                 element = element.substring(1);
+
+            // Ignore removed name. TODO: remove in future release.
+            if (element.equals("NON_CANONICAL_AMBIGUOUS_PATHS"))
+                continue;
+
             Violation section = Violation.valueOf(element);
             if (exclude)
                 violations.remove(section);
@@ -330,4 +329,14 @@ private static Set<Violation> copyOf(Set<Violation> violations)
             return EnumSet.noneOf(Violation.class);
         return EnumSet.copyOf(violations);
     }
+
+    public static String checkUriCompliance(UriCompliance compliance, HttpURI uri)
+    {
+        for (UriCompliance.Violation violation : UriCompliance.Violation.values())
+        {
+            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
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
@@ -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
@@ -40,20 +40,20 @@ public static String toRedirectURL(final HttpServletRequest request, String loca
             if (location.startsWith("/"))
             {
                 // absolute in context
-                location = URIUtil.canonicalEncodedPath(location);
+                location = URIUtil.canonicalURI(location);
             }
             else
             {
                 // relative to request
                 String path = request.getRequestURI();
                 String parent = (path.endsWith("/")) ? path : URIUtil.parentPath(path);
-                location = URIUtil.canonicalPath(URIUtil.addEncodedPaths(parent, location));
-                if (!location.startsWith("/"))
+                location = URIUtil.canonicalURI(URIUtil.addEncodedPaths(parent, location));
+                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
@@ -16,11 +16,11 @@
 import org.eclipse.jetty.http.HttpURI;
 import org.eclipse.jetty.server.Dispatcher;
 import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
 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")
@@ -62,7 +62,7 @@ public void testInvalidUrlWithMessage() throws Exception
     {
         _rule.setCode("405");
         _rule.setMessage("foo");
-        _request.setHttpURI(HttpURI.build(_request.getHttpURI(), "/%00/"));
+        _request.setHttpURI(HttpURI.from("/%01/"));
 
         String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);
 
@@ -72,10 +72,17 @@ 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");
-        _request.setHttpURI(HttpURI.build(_request.getHttpURI(), "/jsp/bean1.jsp%00"));
+
+        _request.setHttpURI(HttpURI.from("/jsp/bean1.jsp\000"));
 
         String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);
 
@@ -86,14 +93,7 @@ public void testInvalidJsp() throws Exception
     @Test
     public void testInvalidShamrock() throws Exception
     {
-        _rule.setCode("405");
-        _rule.setMessage("foo");
-        _request.setHttpURI(HttpURI.build(_request.getHttpURI(), "/jsp/shamrock-%00%E2%98%98.jsp"));
-
-        String result = _rule.matchAndApply(_request.getRequestURI(), _request, _response);
-
-        assertEquals(405, _response.getStatus());
-        assertEquals("foo", _request.getAttribute(Dispatcher.ERROR_MESSAGE));
+        assertThrows(IllegalArgumentException.class, () -> HttpURI.build(_request.getHttpURI(), "/jsp/shamrock-%00%E2%98%98.jsp"));
     }
 
     @Test
@@ -119,4 +119,3 @@ public void testCharacters() throws Exception
         //@checkstyle-enable-check : IllegalTokenText
     }
 }
-
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
@@ -15,6 +15,7 @@
 
 import java.io.IOException;
 import java.util.HashSet;
+import java.util.Objects;
 import java.util.Set;
 import javax.servlet.DispatcherType;
 import javax.servlet.RequestDispatcher;
@@ -27,6 +28,7 @@
 
 import org.eclipse.jetty.http.BadMessageException;
 import org.eclipse.jetty.http.HttpURI;
+import org.eclipse.jetty.http.UriCompliance;
 import org.eclipse.jetty.server.handler.ContextHandler;
 import org.eclipse.jetty.util.Attributes;
 import org.eclipse.jetty.util.MultiMap;
@@ -76,7 +78,7 @@ public void error(ServletRequest request, ServletResponse response) throws Servl
     @Override
     public void include(ServletRequest request, ServletResponse response) throws ServletException, IOException
     {
-        Request baseRequest = Request.getBaseRequest(request);
+        Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request));
 
         if (!(request instanceof HttpServletRequest))
             request = new ServletRequestHttpWrapper(request);
@@ -98,6 +100,10 @@ public void include(ServletRequest request, ServletResponse response) throws Ser
             }
             else
             {
+                Objects.requireNonNull(_uri);
+                // Check any URI violations against the compliance for this request
+                checkUriViolations(_uri, baseRequest);
+
                 IncludeAttributes attr = new IncludeAttributes(
                     old_attr,
                     baseRequest,
@@ -131,7 +137,7 @@ public void forward(ServletRequest request, ServletResponse response) throws Ser
 
     protected void forward(ServletRequest request, ServletResponse response, DispatcherType dispatch) throws ServletException, IOException
     {
-        Request baseRequest = Request.getBaseRequest(request);
+        Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request));
         Response baseResponse = baseRequest.getResponse();
         baseResponse.resetForForward();
 
@@ -159,6 +165,10 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc
             }
             else
             {
+                Objects.requireNonNull(_uri);
+                // Check any URI violations against the compliance for this request
+                checkUriViolations(_uri, baseRequest);
+
                 // If we have already been forwarded previously, then keep using the established
                 // original value. Otherwise, this is the first forward and we need to establish the values.
                 // Note: the established value on the original request for pathInfo and
@@ -230,6 +240,18 @@ protected void forward(ServletRequest request, ServletResponse response, Dispatc
         }
     }
 
+    private static void checkUriViolations(HttpURI uri, Request baseRequest)
+    {
+        if (uri.hasViolations())
+        {
+            HttpChannel channel = baseRequest.getHttpChannel();
+            UriCompliance compliance = channel == null || channel.getHttpConfiguration() == null ? null : channel.getHttpConfiguration().getUriCompliance();
+            String illegalState = UriCompliance.checkUriCompliance(compliance, uri);
+            if (illegalState != null)
+                throw new IllegalStateException(illegalState);
+        }
+    }
+
     @Override
     public String toString()
     {

diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
@@ -67,6 +67,7 @@
 import org.eclipse.jetty.http.BadMessageException;
 import org.eclipse.jetty.http.ComplianceViolation;
 import org.eclipse.jetty.http.HostPortHttpField;
+import org.eclipse.jetty.http.HttpCompliance;
 import org.eclipse.jetty.http.HttpCookie;
 import org.eclipse.jetty.http.HttpCookie.SetCookieHttpField;
 import org.eclipse.jetty.http.HttpField;
@@ -1687,28 +1688,13 @@ public void setMetaData(MetaData.Request request)
         _method = request.getMethod();
         _httpFields = request.getFields();
         final HttpURI uri = request.getURI();
-        boolean ambiguous = false;
         UriCompliance compliance = null;
         if (uri.hasViolations())
         {
-            ambiguous = uri.isAmbiguous();
             compliance = _channel == null || _channel.getHttpConfiguration() == null ? null : _channel.getHttpConfiguration().getUriCompliance();
-            if (uri.hasUtf16Encoding() && (compliance == null || !compliance.allows(UriCompliance.Violation.UTF16_ENCODINGS)))
-                throw new BadMessageException("UTF16 % encoding not supported");
-
-            if (ambiguous)
-            {
-                if (uri.hasAmbiguousSegment() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_SEGMENT)))
-                    throw new BadMessageException("Ambiguous segment in URI");
-                if (uri.hasAmbiguousEmptySegment() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_EMPTY_SEGMENT)))
-                    throw new BadMessageException("Ambiguous empty segment in URI");
-                if (uri.hasAmbiguousSeparator() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR)))
-                    throw new BadMessageException("Ambiguous segment in URI");
-                if (uri.hasAmbiguousParameter() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_PARAMETER)))
-                    throw new BadMessageException("Ambiguous path parameter in URI");
-                if (uri.hasAmbiguousEncoding() && (compliance == null || !compliance.allows(UriCompliance.Violation.AMBIGUOUS_PATH_ENCODING)))
-                    throw new BadMessageException("Ambiguous path encoding in URI");
-            }
+            String badMessage = UriCompliance.checkUriCompliance(compliance, uri);
+            if (badMessage != null)
+                throw new BadMessageException(badMessage);
         }
 
         if (uri.isAbsolute() && uri.hasAuthority() && uri.getPath() != null)
@@ -1740,7 +1726,6 @@ public void setMetaData(MetaData.Request request)
             }
             _uri = builder.asImmutable();
         }
-
         setSecure(HttpScheme.HTTPS.is(_uri.getScheme()));
 
         String encoded = _uri.getPath();
@@ -1751,17 +1736,15 @@ public void setMetaData(MetaData.Request request)
         else if (encoded.startsWith("/"))
         {
             path = (encoded.length() == 1) ? "/" : _uri.getDecodedPath();
-            // Strictly speaking if a URI is legal and encodes ambiguous segments, then they should be
-            // reflected in the decoded string version.  However, it can be ambiguous to provide a decoded path as
-            // a string, so we normalize again.  If an application wishes to see ambiguous URIs, then they must
-            // set the {@link UriCompliance.Violation#NON_CANONICAL_AMBIGUOUS_PATHS} compliance.
-            if (ambiguous && (compliance == null || !compliance.allows(UriCompliance.Violation.NON_CANONICAL_AMBIGUOUS_PATHS)))
-                path = URIUtil.canonicalPath(path);
         }
         else if ("*".equals(encoded) || HttpMethod.CONNECT.is(getMethod()))
+        {
             path = encoded;
+        }
         else
+        {
             path = null;
+        }
 
         if (path == null || path.isEmpty())
         {

diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
@@ -568,14 +568,14 @@ public void sendRedirect(int code, String location, boolean consumeAll) throws I
             if (location.startsWith("/"))
             {
                 // absolute in context
-                location = URIUtil.canonicalEncodedPath(location);
+                location = URIUtil.canonicalURI(location);
             }
             else
             {
                 // relative to request
                 String path = _channel.getRequest().getRequestURI();
                 String parent = (path.endsWith("/")) ? path : URIUtil.parentPath(path);
-                location = URIUtil.canonicalEncodedPath(URIUtil.addEncodedPaths(parent, location));
+                location = URIUtil.canonicalURI(URIUtil.addEncodedPaths(parent, location));
                 if (location != null && !location.startsWith("/"))
                     buf.append('/');
             }