Issue #6302 - Treat empty path segments as ambiguous.

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

@@ -50,9 +50,29 @@ public interface HttpURI
 {
     enum Ambiguous
     {
+        /**
+         * URI contains ambiguous path segments e.g. {@code /foo/%2e%2e/bar}
+         */
         SEGMENT,
+
+        /**
+         * URI contains ambiguous empty segments e.g. {@code /foo//bar} or {@code /foo/;param/}, but not {@code /foo/}
+         */
+        EMPTY,
+
+        /**
+         * URI contains ambiguous path separator within a URI segment e.g. {@code /foo/b%2fr}
+         */
         SEPARATOR,
+
+        /**
+         * URI contains ambiguous path encoding within a URI segment e.g. {@code /%2557EB-INF}
+         */
         ENCODING,
+
+        /**
+         * URI contains ambiguous path parameters within a URI segment e.g. {@code /foo/..;/bar}
+         */
         PARAM
     }
 
@@ -150,6 +170,11 @@ static Immutable from(String scheme, String host, int port, String pathQuery)
      */
     boolean hasAmbiguousSegment();
 
+    /**
+     * @return True if the URI empty segment that is ambiguous like '//' or '/;param/'.
+     */
+    boolean hasAmbiguousEmptySegment();
+
     /**
      * @return True if the URI has a possibly ambiguous separator of %2f
      */
@@ -377,13 +402,19 @@ public boolean isAmbiguous()
         @Override
         public boolean hasAmbiguousSegment()
         {
-            return _ambiguous.contains(Mutable.Ambiguous.SEGMENT);
+            return _ambiguous.contains(Ambiguous.SEGMENT);
+        }
+
+        @Override
+        public boolean hasAmbiguousEmptySegment()
+        {
+            return _ambiguous.contains(Ambiguous.EMPTY);
         }
 
         @Override
         public boolean hasAmbiguousSeparator()
         {
-            return _ambiguous.contains(Mutable.Ambiguous.SEPARATOR);
+            return _ambiguous.contains(Ambiguous.SEPARATOR);
         }
 
         @Override
@@ -468,6 +499,7 @@ private enum State
         private String _uri;
         private String _decodedPath;
         private final EnumSet<Ambiguous> _ambiguous = EnumSet.noneOf(Ambiguous.class);
+        private boolean _emptySegment;
 
         private Mutable()
         {
@@ -727,6 +759,12 @@ public boolean hasAmbiguousSegment()
             return _ambiguous.contains(Mutable.Ambiguous.SEGMENT);
         }
 
+        @Override
+        public boolean hasAmbiguousEmptySegment()
+        {
+            return _ambiguous.contains(Ambiguous.EMPTY);
+        }
+
         @Override
         public boolean hasAmbiguousSeparator()
         {
@@ -904,6 +942,7 @@ private void parse(State state, final String uri)
             boolean dot = false; // set to true if the path containers . or .. segments
             int escapedTwo = 0; // state of parsing a %2<x>
             int end = uri.length();
+            _emptySegment = false;
             for (int i = 0; i < end; i++)
             {
                 char c = uri.charAt(i);
@@ -919,16 +958,21 @@ private void parse(State state, final String uri)
                                 state = State.HOST_OR_PATH;
                                 break;
                             case ';':
+                                checkSegment(uri, segment, i, true);
                                 mark = i + 1;
                                 state = State.PARAM;
                                 break;
                             case '?':
                                 // assume empty path (if seen at start)
+                                checkSegment(uri, segment, i, false);
                                 _path = "";
                                 mark = i + 1;
                                 state = State.QUERY;
                                 break;
                             case '#':
+                                // assume empty path (if seen at start)
+                                checkSegment(uri, segment, i, false);
+                                _path = "";
                                 mark = i + 1;
                                 state = State.FRAGMENT;
                                 break;
@@ -1130,7 +1174,9 @@ else if (c == '/')
                                 state = State.FRAGMENT;
                                 break;
                             case '/':
-                                checkSegment(uri, segment, i, false);
+                                // There is no leading segment when parsing only a path that starts with slash.
+                                if (i != 0)
+                                    checkSegment(uri, segment, i, false);
                                 segment = i + 1;
                                 break;
                             case '.':
@@ -1218,6 +1264,9 @@ else if (c == '/')
             switch (state)
             {
                 case START:
+                    _path = "";
+                    checkSegment(uri, segment, end, false);
+                    break;
                 case ASTERISK:
                     break;
                 case SCHEME_OR_PATH:
@@ -1279,13 +1328,45 @@ else if (_path != null)
          */
         private void checkSegment(String uri, int segment, int end, boolean param)
         {
-            if (!_ambiguous.contains(Ambiguous.SEGMENT))
+            // 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 
+            if (_emptySegment)
+                _ambiguous.add(Ambiguous.EMPTY);
+
+            if (end == segment)
+            {
+                // Empty segments are not ambiguous if followed by a '#', '?' or end of string.
+                if (end >= uri.length() || ("#?".indexOf(uri.charAt(end)) >= 0))
+                    return;
+
+                // If this empty segment is the first segment then it is ambiguous.
+                if (segment == 0)
+                {
+                    _ambiguous.add(Ambiguous.EMPTY);
+                    return;
+                }
+
+                // Otherwise remember we have seen an empty segment, which is check if we see a subsequent segment.
+                if (!_emptySegment)
+                {
+                    _emptySegment = true;
+                    return;
+                }
+            }
+
+            // Look for segment in the ambiguous segment index.
+            Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment);
+            if (ambiguous == Boolean.TRUE)
+            {
+                // The segment is always ambiguous.
+                _ambiguous.add(Ambiguous.SEGMENT);
+            }
+            else if (param && ambiguous == Boolean.FALSE)
             {
-                Boolean ambiguous = __ambiguousSegments.get(uri, segment, end - segment);
-                if (ambiguous == Boolean.TRUE)
-                    _ambiguous.add(Ambiguous.SEGMENT);
-                else if (param && ambiguous == Boolean.FALSE)
-                    _ambiguous.add(Ambiguous.PARAM);
+                // The segment is ambiguous only when followed by a parameter.
+                _ambiguous.add(Ambiguous.PARAM);
             }
         }
     }

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

@@ -50,6 +50,10 @@ public enum Violation implements ComplianceViolation
          * Allow ambiguous path segments e.g. <code>/foo/%2e%2e/bar</code>
          */
         AMBIGUOUS_PATH_SEGMENT("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI path segment"),
+        /**
+         * Allow ambiguous empty segments e.g. <code>//</code>
+         */
+        AMBIGUOUS_EMPTY_SEGMENT("https://tools.ietf.org/html/rfc3986#section-3.3", "Ambiguous URI empty segment"),
         /**
          * Allow ambiguous path separator within a URI segment e.g. <code>/foo/b%2fr</code>
          */
@@ -104,9 +108,10 @@ public String getDescription()
     public static final UriCompliance DEFAULT = new UriCompliance("DEFAULT", of(Violation.AMBIGUOUS_PATH_SEPARATOR));
 
     /**
-     * LEGACY compliance mode that models Jetty-9.4 behavior by allowing {@link Violation#AMBIGUOUS_PATH_SEGMENT}, {@link Violation#AMBIGUOUS_PATH_SEPARATOR} and {@link Violation#AMBIGUOUS_PATH_ENCODING}.
+     * LEGACY compliance mode that models Jetty-9.4 behavior by allowing {@link Violation#AMBIGUOUS_PATH_SEGMENT},
+     * {@link Violation#AMBIGUOUS_EMPTY_SEGMENT}, {@link Violation#AMBIGUOUS_PATH_SEPARATOR} and {@link Violation#AMBIGUOUS_PATH_ENCODING}.
      */
-    public static final UriCompliance LEGACY = new UriCompliance("LEGACY", of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_ENCODING));
+    public static final UriCompliance LEGACY = new UriCompliance("LEGACY", of(Violation.AMBIGUOUS_PATH_SEGMENT, Violation.AMBIGUOUS_PATH_SEPARATOR, Violation.AMBIGUOUS_PATH_ENCODING, Violation.AMBIGUOUS_EMPTY_SEGMENT));
 
     /**
      * Compliance mode that exactly follows RFC3986, including allowing all additional ambiguous URI Violations,

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

@@ -28,6 +28,7 @@
 import static org.hamcrest.Matchers.nullValue;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -361,6 +362,21 @@ public static Stream<Arguments> decodePathTests()
                 {".%2e", "..", EnumSet.of(Ambiguous.SEGMENT)},
                 {"%2e%2e", "..", EnumSet.of(Ambiguous.SEGMENT)},
 
+                // empty segment treated as ambiguous
+                {"/foo//bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)},
+                {"/foo//../bar", "/foo/bar", EnumSet.of(Ambiguous.EMPTY)},
+                {"/foo///../../../bar", "/bar", EnumSet.of(Ambiguous.EMPTY)},
+                {"/foo/./../bar", "/bar", EnumSet.noneOf(Ambiguous.class)},
+                {"/foo//./bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)},
+                {"foo/bar", "foo/bar", EnumSet.noneOf(Ambiguous.class)},
+                {"foo;/bar", "foo/bar", EnumSet.noneOf(Ambiguous.class)},
+                {";/bar", "/bar", EnumSet.of(Ambiguous.EMPTY)},
+                {";?n=v", "", EnumSet.of(Ambiguous.EMPTY)},
+                {"?n=v", "", EnumSet.noneOf(Ambiguous.class)},
+                {"#n=v", "", EnumSet.noneOf(Ambiguous.class)},
+                {"", "", EnumSet.noneOf(Ambiguous.class)},
+                {"http:/foo", "/foo", EnumSet.noneOf(Ambiguous.class)},
+
                 // ambiguous parameter inclusions
                 {"/path/.;/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)},
                 {"/path/.;param/info", "/path/./info", EnumSet.of(Ambiguous.PARAM)},
@@ -377,6 +393,11 @@ public static Stream<Arguments> decodePathTests()
                 {"%2F/info", "//info", EnumSet.of(Ambiguous.SEPARATOR)},
                 {"/path/%2f../info", "/path//../info", EnumSet.of(Ambiguous.SEPARATOR)},
 
+                // ambiguous encoding
+                {"/path/%25/info", "/path/%/info", EnumSet.of(Ambiguous.ENCODING)},
+                {"%25/info", "%/info", EnumSet.of(Ambiguous.ENCODING)},
+                {"/path/%25../info", "/path/%../info", EnumSet.of(Ambiguous.ENCODING)},
+
                 // 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)},
@@ -401,10 +422,125 @@ public void testDecodedPath(String input, String decodedPath, EnumSet<Ambiguous>
             assertThat(uri.hasAmbiguousSegment(), is(expected.contains(Ambiguous.SEGMENT)));
             assertThat(uri.hasAmbiguousSeparator(), is(expected.contains(Ambiguous.SEPARATOR)));
             assertThat(uri.hasAmbiguousParameter(), is(expected.contains(Ambiguous.PARAM)));
+            assertThat(uri.hasAmbiguousEncoding(), is(expected.contains(Ambiguous.ENCODING)));
         }
         catch (Exception e)
         {
             assertThat(decodedPath, nullValue());
         }
     }
+
+    public static Stream<Arguments> testPathQueryTests()
+    {
+        return Arrays.stream(new Object[][]
+            {
+                // Simple path example
+                {"/path/info", "/path/info", EnumSet.noneOf(Ambiguous.class)},
+
+                // legal non ambiguous relative paths
+                {"/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
+                {"/../path/info", null, null},
+                {"../path/info", null, null},
+                {"/path/%XX/info", null, null},
+                {"/path/%2/F/info", null, null},
+
+                // ambiguous dot encodings
+                {"/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)},
+
+                // empty segment treated as ambiguous
+                {"/", "/", EnumSet.noneOf(Ambiguous.class)},
+                {"/#", "/", EnumSet.noneOf(Ambiguous.class)},
+                {"/path", "/path", EnumSet.noneOf(Ambiguous.class)},
+                {"/path/", "/path/", EnumSet.noneOf(Ambiguous.class)},
+                {"//", "//", EnumSet.of(Ambiguous.EMPTY)},
+                {"/foo//", "/foo//", EnumSet.of(Ambiguous.EMPTY)},
+                {"/foo//bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)},
+                {"//foo/bar", "//foo/bar", EnumSet.of(Ambiguous.EMPTY)},
+                {"/foo?bar", "/foo", EnumSet.noneOf(Ambiguous.class)},
+                {"/foo#bar", "/foo", EnumSet.noneOf(Ambiguous.class)},
+                {"/foo;bar", "/foo", EnumSet.noneOf(Ambiguous.class)},
+                {"/foo/?bar", "/foo/", EnumSet.noneOf(Ambiguous.class)},
+                {"/foo/#bar", "/foo/", EnumSet.noneOf(Ambiguous.class)},
+                {"/foo/;param", "/foo/", EnumSet.noneOf(Ambiguous.class)},
+                {"/foo/;param/bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)},
+                {"/foo//bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)},
+                {"/foo//bar//", "/foo//bar//", EnumSet.of(Ambiguous.EMPTY)},
+                {"//foo//bar//", "//foo//bar//", EnumSet.of(Ambiguous.EMPTY)},
+                {"/foo//../bar", "/foo/bar", EnumSet.of(Ambiguous.EMPTY)},
+                {"/foo///../../../bar", "/bar", EnumSet.of(Ambiguous.EMPTY)},
+                {"/foo/./../bar", "/bar", EnumSet.noneOf(Ambiguous.class)},
+                {"/foo//./bar", "/foo//bar", EnumSet.of(Ambiguous.EMPTY)},
+                {"foo/bar", "foo/bar", EnumSet.noneOf(Ambiguous.class)},
+                {"foo;/bar", "foo/bar", EnumSet.noneOf(Ambiguous.class)},
+                {";/bar", "/bar", EnumSet.of(Ambiguous.EMPTY)},
+                {";?n=v", "", EnumSet.of(Ambiguous.EMPTY)},
+                {"?n=v", "", EnumSet.noneOf(Ambiguous.class)},
+                {"#n=v", "", EnumSet.noneOf(Ambiguous.class)},
+                {"", "", EnumSet.noneOf(Ambiguous.class)},
+
+                // 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", 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)},
+
+                // ambiguous encoding
+                {"/path/%25/info", "/path/%/info", EnumSet.of(Ambiguous.ENCODING)},
+                {"%25/info", "%/info", EnumSet.of(Ambiguous.ENCODING)},
+                {"/path/%25../info", "/path/%../info", EnumSet.of(Ambiguous.ENCODING)},
+
+                // 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)},
+                {"/path/%2f/%25/..;/%2e//info", "/path///%/.././/info", EnumSet.of(Ambiguous.SEPARATOR, Ambiguous.PARAM, Ambiguous.SEGMENT, Ambiguous.ENCODING, Ambiguous.EMPTY)},
+            }).map(Arguments::of);
+    }
+
+    @ParameterizedTest
+    @MethodSource("testPathQueryTests")
+    public void testPathQuery(String input, String decodedPath, EnumSet<Ambiguous> expected)
+    {
+        // If expected is null then it is a bad URI and should throw.
+        if (expected == null)
+        {
+            assertThrows(Throwable.class, () -> HttpURI.build().pathQuery(input));
+            return;
+        }
+
+        HttpURI uri = HttpURI.build().pathQuery(input);
+        assertThat(uri.getDecodedPath(), is(decodedPath));
+        assertThat(uri.isAmbiguous(), is(!expected.isEmpty()));
+        assertThat(uri.hasAmbiguousEmptySegment(), is(expected.contains(Ambiguous.EMPTY)));
+        assertThat(uri.hasAmbiguousSegment(), is(expected.contains(Ambiguous.SEGMENT)));
+        assertThat(uri.hasAmbiguousSeparator(), is(expected.contains(Ambiguous.SEPARATOR)));
+        assertThat(uri.hasAmbiguousParameter(), is(expected.contains(Ambiguous.PARAM)));
+        assertThat(uri.hasAmbiguousEncoding(), is(expected.contains(Ambiguous.ENCODING)));
+    }
 }