Merge pull request #6005 from eclipse/jetty-9.4.x-6001-default-accept…

…-ambiguous-uris

Fix #6001 separate compliance modes for ambiguous URI segments, params and separators

jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java

@@ -61,26 +61,29 @@ public enum HttpCompliance // TODO in Jetty-10 convert this enum to a class so t
      * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEGMENTS} and
      * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEPARATORS}.
      */
-    RFC2616_LEGACY(sectionsBySpec("RFC2616,-FIELD_COLON,-METHOD_CASE_SENSITIVE,-TRANSFER_ENCODING_WITH_CONTENT_LENGTH,-MULTIPLE_CONTENT_LENGTHS,-NO_AMBIGUOUS_PATH_SEGMENTS,-NO_AMBIGUOUS_PATH_SEPARATORS")),
+    RFC2616_LEGACY(sectionsBySpec("RFC2616,-FIELD_COLON,-METHOD_CASE_SENSITIVE,-TRANSFER_ENCODING_WITH_CONTENT_LENGTH,-MULTIPLE_CONTENT_LENGTHS")),
 
     /**
      * The strict RFC2616 support mode
      */
     RFC2616(sectionsBySpec("RFC2616")),
 
     /**
-     * Jetty's current RFC7230 support, which excludes
-     * {@link HttpComplianceSection#METHOD_CASE_SENSITIVE},
-     * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEGMENTS} and
-     * {@link HttpComplianceSection#NO_AMBIGUOUS_PATH_SEPARATORS}.
+     * Jetty's legacy RFC7230 support, which excludes
+     * {@link HttpComplianceSection#METHOD_CASE_SENSITIVE}.
      */
-    RFC7230_LEGACY(sectionsBySpec("RFC7230,-METHOD_CASE_SENSITIVE,-NO_AMBIGUOUS_PATH_SEGMENTS,-NO_AMBIGUOUS_PATH_SEPARATORS")),
+    RFC7230_LEGACY(sectionsBySpec("RFC7230,-METHOD_CASE_SENSITIVE")),
 
     /**
      * The RFC7230 support mode
      */
     RFC7230(sectionsBySpec("RFC7230")),
 
+    /**
+     * The RFC7230 support mode with no ambiguous URIs
+     */
+    RFC7230_NO_AMBIGUOUS_URIS(sectionsBySpec("RFC7230,NO_AMBIGUOUS_PATH_SEGMENTS,NO_AMBIGUOUS_PATH_SEPARATORS")),
+
     /**
      * Custom compliance mode that can be defined with System property <code>org.eclipse.jetty.http.HttpCompliance.CUSTOM0</code>
      */
@@ -124,16 +127,20 @@ static EnumSet<HttpComplianceSection> sectionsBySpec(String spec)
                 break;
 
             case "RFC2616":
+                i++;
                 sections = EnumSet.complementOf(EnumSet.of(
                     HttpComplianceSection.NO_FIELD_FOLDING,
-                    HttpComplianceSection.NO_HTTP_0_9));
-                i++;
+                    HttpComplianceSection.NO_HTTP_0_9,
+                    HttpComplianceSection.NO_AMBIGUOUS_PATH_SEGMENTS,
+                    HttpComplianceSection.NO_AMBIGUOUS_PATH_SEPARATORS));
                 break;
 
             case "*":
             case "RFC7230":
                 i++;
-                sections = EnumSet.allOf(HttpComplianceSection.class);
+                sections = EnumSet.complementOf(EnumSet.of(
+                    HttpComplianceSection.NO_AMBIGUOUS_PATH_SEGMENTS,
+                    HttpComplianceSection.NO_AMBIGUOUS_PATH_SEPARATORS));
                 break;
 
             default:

jetty-http/src/main/java/org/eclipse/jetty/http/HttpComplianceSection.java

@@ -33,7 +33,8 @@ public enum HttpComplianceSection
     TRANSFER_ENCODING_WITH_CONTENT_LENGTH("https://tools.ietf.org/html/rfc7230#section-3.3.1", "Transfer-Encoding and Content-Length"),
     MULTIPLE_CONTENT_LENGTHS("https://tools.ietf.org/html/rfc7230#section-3.3.1", "Multiple Content-Lengths"),
     NO_AMBIGUOUS_PATH_SEGMENTS("https://tools.ietf.org/html/rfc3986#section-3.3", "No ambiguous URI path segments"),
-    NO_AMBIGUOUS_PATH_SEPARATORS("https://tools.ietf.org/html/rfc3986#section-3.3", "No ambiguous URI path separators");
+    NO_AMBIGUOUS_PATH_SEPARATORS("https://tools.ietf.org/html/rfc3986#section-3.3", "No ambiguous URI path separators"),
+    NO_AMBIGUOUS_PATH_PARAMETERS("https://tools.ietf.org/html/rfc3986#section-3.3", "No ambiguous URI path parameters");
 
     final String url;
     final String description;

jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

@@ -72,7 +72,8 @@ private enum State
     enum Ambiguous
     {
         SEGMENT,
-        SEPARATOR
+        SEPARATOR,
+        PARAM
     }
 
     /**
@@ -637,8 +638,10 @@ private void checkSegment(String uri, int segment, int end, boolean param)
         if (!_ambiguous.contains(Ambiguous.SEGMENT))
         {
             Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment);
-            if (ambiguous == Boolean.TRUE   || (param && ambiguous == Boolean.FALSE))
+            if (ambiguous == Boolean.TRUE)
                 _ambiguous.add(Ambiguous.SEGMENT);
+            else if (param && ambiguous == Boolean.FALSE)
+                _ambiguous.add(Ambiguous.PARAM);
         }
     }
 
@@ -658,6 +661,14 @@ public boolean hasAmbiguousSeparator()
         return _ambiguous.contains(Ambiguous.SEPARATOR);
     }
 
+    /**
+     * @return True if the URI has a possibly ambiguous path parameter like '..;'
+     */
+    public boolean hasAmbiguousParameter()
+    {
+        return _ambiguous.contains(Ambiguous.PARAM);
+    }
+
     /**
      * @return True if the URI has either an {@link #hasAmbiguousSegment()} or {@link #hasAmbiguousSeparator()}.
      */

jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java

@@ -21,8 +21,10 @@
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
+import java.util.EnumSet;
 import java.util.stream.Stream;
 
+import org.eclipse.jetty.http.HttpURI.Ambiguous;
 import org.eclipse.jetty.util.MultiMap;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
@@ -285,77 +287,83 @@ public static Stream<Arguments> decodePathTests()
         return Arrays.stream(new Object[][]
             {
                 // Simple path example
-                {"http://host/path/info", "/path/info", false, false},
-                {"//host/path/info", "/path/info", false, false},
-                {"/path/info", "/path/info", false, false},
+                {"http://host/path/info", "/path/info", EnumSet.noneOf(Ambiguous.class)},
+                {"//host/path/info", "/path/info", EnumSet.noneOf(Ambiguous.class)},
+                {"/path/info", "/path/info", EnumSet.noneOf(Ambiguous.class)},
 
                 // legal non ambiguous relative paths
-                {"http://host/../path/info", null, false, false},
-                {"http://host/path/../info", "/info", false, false},
-                {"http://host/path/./info", "/path/info", false, false},
-                {"//host/path/../info", "/info", false, false},
-                {"//host/path/./info", "/path/info", false, false},
-                {"/path/../info", "/info", false, false},
-                {"/path/./info", "/path/info", false, false},
-                {"path/../info", "info", false, false},
-                {"path/./info", "path/info", false, false},
+                {"http://host/../path/info", null, EnumSet.noneOf(Ambiguous.class)},
+                {"http://host/path/../info", "/info", EnumSet.noneOf(Ambiguous.class)},
+                {"http://host/path/./info", "/path/info", EnumSet.noneOf(Ambiguous.class)},
+                {"//host/path/../info", "/info", EnumSet.noneOf(Ambiguous.class)},
+                {"//host/path/./info", "/path/info", EnumSet.noneOf(Ambiguous.class)},
+                {"/path/../info", "/info", EnumSet.noneOf(Ambiguous.class)},
+                {"/path/./info", "/path/info", EnumSet.noneOf(Ambiguous.class)},
+                {"path/../info", "info", EnumSet.noneOf(Ambiguous.class)},
+                {"path/./info", "path/info", EnumSet.noneOf(Ambiguous.class)},
 
                 // illegal paths
-                {"//host/../path/info", null, false, false},
-                {"/../path/info", null, false, false},
-                {"../path/info", null, false, false},
-                {"/path/%XX/info", null, false, false},
-                {"/path/%2/F/info", null, false, false},
-
-                // ambiguous dot encodings or parameter inclusions
-                {"scheme://host/path/%2e/info", "/path/./info", true, false},
-                {"scheme:/path/%2e/info", "/path/./info", true, false},
-                {"/path/%2e/info", "/path/./info", true, false},
-                {"path/%2e/info/", "path/./info/", true, false},
-                {"/path/%2e%2e/info", "/path/../info", true, false},
-                {"/path/%2e%2e;/info", "/path/../info", true, false},
-                {"/path/%2e%2e;param/info", "/path/../info", true, false},
-                {"/path/%2e%2e;param;other/info;other", "/path/../info", true, false},
-                {"/path/.;/info", "/path/./info", true, false},
-                {"/path/.;param/info", "/path/./info", true, false},
-                {"/path/..;/info", "/path/../info", true, false},
-                {"/path/..;param/info", "/path/../info", true, false},
-                {"%2e/info", "./info", true, false},
-                {"%2e%2e/info", "../info", true, false},
-                {"%2e%2e;/info", "../info", true, false},
-                {".;/info", "./info", true, false},
-                {".;param/info", "./info", true, false},
-                {"..;/info", "../info", true, false},
-                {"..;param/info", "../info", true, false},
-                {"%2e", ".", true, false},
-                {"%2e.", "..", true, false},
-                {".%2e", "..", true, false},
-                {"%2e%2e", "..", true, false},
+                {"//host/../path/info", null, EnumSet.noneOf(Ambiguous.class)},
+                {"/../path/info", null, EnumSet.noneOf(Ambiguous.class)},
+                {"../path/info", null, EnumSet.noneOf(Ambiguous.class)},
+                {"/path/%XX/info", null, EnumSet.noneOf(Ambiguous.class)},
+                {"/path/%2/F/info", null, EnumSet.noneOf(Ambiguous.class)},
+
+                // ambiguous dot encodings
+                {"scheme://host/path/%2e/info", "/path/./info", EnumSet.of(Ambiguous.SEGMENT)},
+                {"scheme:/path/%2e/info", "/path/./info", EnumSet.of(Ambiguous.SEGMENT)},
+                {"/path/%2e/info", "/path/./info", EnumSet.of(Ambiguous.SEGMENT)},
+                {"path/%2e/info/", "path/./info/", EnumSet.of(Ambiguous.SEGMENT)},
+                {"/path/%2e%2e/info", "/path/../info", EnumSet.of(Ambiguous.SEGMENT)},
+                {"/path/%2e%2e;/info", "/path/../info", EnumSet.of(Ambiguous.SEGMENT)},
+                {"/path/%2e%2e;param/info", "/path/../info", EnumSet.of(Ambiguous.SEGMENT)},
+                {"/path/%2e%2e;param;other/info;other", "/path/../info", EnumSet.of(Ambiguous.SEGMENT)},
+                {"%2e/info", "./info", EnumSet.of(Ambiguous.SEGMENT)},
+                {"%2e%2e/info", "../info", EnumSet.of(Ambiguous.SEGMENT)},
+                {"%2e%2e;/info", "../info", EnumSet.of(Ambiguous.SEGMENT)},
+                {"%2e", ".", EnumSet.of(Ambiguous.SEGMENT)},
+                {"%2e.", "..", EnumSet.of(Ambiguous.SEGMENT)},
+                {".%2e", "..", EnumSet.of(Ambiguous.SEGMENT)},
+                {"%2e%2e", "..", EnumSet.of(Ambiguous.SEGMENT)},
+
+                // ambiguous parameter inclusions
+                {"/path/.;/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)},
+                {"/path/.;param/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)},
+                {"/path/..;/info", "/path/../info", EnumSet.of(Ambiguous.PARAM)},
+                {"/path/..;param/info", "/path/../info", EnumSet.of(Ambiguous.PARAM)},
+                {".;/info", "./info", EnumSet.of(Ambiguous.PARAM)},
+                {".;param/info", "./info", EnumSet.of(Ambiguous.PARAM)},
+                {"..;/info", "../info", EnumSet.of(Ambiguous.PARAM)},
+                {"..;param/info", "../info", EnumSet.of(Ambiguous.PARAM)},
 
                 // ambiguous segment separators
-                {"/path/%2f/info", "/path///info", false, true},
-                {"%2f/info", "//info", false, true},
-                {"%2F/info", "//info", false, true},
-                {"/path/%2f../info", "/path//../info", false, true},
-                {"/path/%2f/..;/info", "/path///../info", true, true},
+                {"/path/%2f/info", "/path///info", EnumSet.of(Ambiguous.SEPARATOR)},
+                {"%2f/info", "//info", EnumSet.of(Ambiguous.SEPARATOR)},
+                {"%2F/info", "//info", EnumSet.of(Ambiguous.SEPARATOR)},
+                {"/path/%2f../info", "/path//../info", EnumSet.of(Ambiguous.SEPARATOR)},
+
+                // combinations
+                {"/path/%2f/..;/info", "/path///../info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM)},
+                {"/path/%2f/..;/%2e/info", "/path///.././info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM, Ambiguous.SEGMENT)},
 
                 // Non ascii characters
-                {"http://localhost:9000/x\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", "/x\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", false, false},
-                {"http://localhost:9000/\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", "/\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", false, false},
+                {"http://localhost:9000/x\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", "/x\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", EnumSet.noneOf(Ambiguous.class)},
+                {"http://localhost:9000/\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", "/\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32\uD83C\uDF32", EnumSet.noneOf(Ambiguous.class)},
             }).map(Arguments::of);
     }
 
     @ParameterizedTest
     @MethodSource("decodePathTests")
-    public void testDecodedPath(String input, String decodedPath, boolean ambiguousSegment, boolean ambiguousSeparator)
+    public void testDecodedPath(String input, String decodedPath, EnumSet<Ambiguous> expected)
     {
         try
         {
             HttpURI uri = new HttpURI(input);
             assertThat(uri.getDecodedPath(), is(decodedPath));
-            assertThat(uri.hasAmbiguousSegment(), is(ambiguousSegment));
-            assertThat(uri.hasAmbiguousSeparator(), is(ambiguousSeparator));
-            assertThat(uri.isAmbiguous(), is(ambiguousSegment || ambiguousSeparator));
+            assertThat(uri.isAmbiguous(), is(!expected.isEmpty()));
+            assertThat(uri.hasAmbiguousSegment(), is(expected.contains(Ambiguous.SEGMENT)));
+            assertThat(uri.hasAmbiguousSeparator(), is(expected.contains(Ambiguous.SEPARATOR)));
+            assertThat(uri.hasAmbiguousParameter(), is(expected.contains(Ambiguous.PARAM)));
         }
         catch (Exception e)
         {

jetty-server/src/main/java/org/eclipse/jetty/server/Request.java

@@ -1836,6 +1836,8 @@ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request)
                 throw new BadMessageException("Ambiguous segment in URI");
             if (uri.hasAmbiguousSeparator() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_SEPARATORS)))
                 throw new BadMessageException("Ambiguous separator in URI");
+            if (uri.hasAmbiguousParameter() && (compliance == null || compliance.sections().contains(HttpComplianceSection.NO_AMBIGUOUS_PATH_PARAMETERS)))
+                throw new BadMessageException("Ambiguous path parameter in URI");
         }
 
         _originalURI = uri.isAbsolute() && request.getHttpVersion() != HttpVersion.HTTP_2 ? uri.toString() : uri.getPathQuery();

jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java

@@ -49,6 +49,7 @@
 
 import org.eclipse.jetty.http.BadMessageException;
 import org.eclipse.jetty.http.HttpCompliance;
+import org.eclipse.jetty.http.HttpComplianceSection;
 import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.http.MimeTypes;
 import org.eclipse.jetty.server.LocalConnector.LocalEndPoint;
@@ -1837,7 +1838,7 @@ public void testGetterSafeFromNullPointerException()
     }
 
     @Test
-    public void testAmbiguousSegments() throws Exception
+    public void testAmbiguousParameters() throws Exception
     {
         _handler._checker = (request, response) -> true;
 
@@ -1848,13 +1849,30 @@ public void testAmbiguousSegments() throws Exception
         _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230);
         assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400"));
 
-        _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230_LEGACY);
+        HttpCompliance.CUSTOM0.sections().clear();
+        HttpCompliance.CUSTOM0.sections().addAll(HttpCompliance.RFC7230.sections());
+        HttpCompliance.CUSTOM0.sections().remove(HttpComplianceSection.NO_AMBIGUOUS_PATH_PARAMETERS);
+
+        _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.CUSTOM0);
         assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200"));
+    }
 
-        _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC2616);
+    @Test
+    public void testAmbiguousSegments() throws Exception
+    {
+        _handler._checker = (request, response) -> true;
+
+        String request = "GET /ambiguous/%2e%2e/path HTTP/1.0\r\n" +
+            "Host: whatever\r\n" +
+            "\r\n";
+
+        _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230_NO_AMBIGUOUS_URIS);
         assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400"));
 
-        _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC2616_LEGACY);
+        _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230);
+        assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200"));
+
+        _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC2616);
         assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200"));
     }
 
@@ -1867,16 +1885,13 @@ public void testAmbiguousSeparators() throws Exception
             "Host: whatever\r\n" +
             "\r\n";
 
-        _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230);
+        _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230_NO_AMBIGUOUS_URIS);
         assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400"));
 
-        _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230_LEGACY);
+        _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230);
         assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200"));
 
         _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC2616);
-        assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 400"));
-
-        _connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC2616_LEGACY);
         assertThat(_connector.getResponse(request), startsWith("HTTP/1.1 200"));
     }
 

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DefaultServletTest.java

@@ -47,14 +47,12 @@
 import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jetty.http.DateGenerator;
-import org.eclipse.jetty.http.HttpCompliance;
 import org.eclipse.jetty.http.HttpContent;
 import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.server.HttpConfiguration;
-import org.eclipse.jetty.server.HttpConnectionFactory;
 import org.eclipse.jetty.server.LocalConnector;
 import org.eclipse.jetty.server.ResourceContentFactory;
 import org.eclipse.jetty.server.ResourceService;
@@ -118,7 +116,6 @@ public void init() throws Exception
 
         connector = new LocalConnector(server);
         connector.getConnectionFactory(HttpConfiguration.ConnectionFactory.class).getHttpConfiguration().setSendServerVersion(false);
-        connector.getBean(HttpConnectionFactory.class).setHttpCompliance(HttpCompliance.RFC7230_LEGACY); // allow ambiguous path segments
 
         File extraJarResources = MavenTestingUtils.getTestResourceFile(ODD_JAR);
         URL[] urls = new URL[]{extraJarResources.toURI().toURL()};
@@ -460,6 +457,13 @@ public static Stream<Arguments> contextBreakoutScenarios()
                 (response) -> assertThat(response.getContent(), not(containsString("Sssh")))
             );
 
+            scenarios.addScenario(
+                "GET " + prefix + "/..;/..;/sekret/pass",
+                "GET " + prefix + "/..;/..;/sekret/pass HTTP/1.0\r\n\r\n",
+                prefix.endsWith("?") ? HttpStatus.NOT_FOUND_404 : HttpStatus.BAD_REQUEST_400,
+                (response) -> assertThat(response.getContent(), not(containsString("Sssh")))
+            );
+
             scenarios.addScenario(
                 "GET " + prefix + "/%2E%2E/%2E%2E/sekret/pass",
                 "GET " + prefix + "/ HTTP/1.0\r\n\r\n",