HttpURI.Mutable path changes should clear out any existing path violations before parsing the new path. (#12306)

* HttpURI.Mutable path changes should clear out any existing path violations before parsing the new path.

Fixes: #11298

Author: joakime

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

@@ -994,6 +994,9 @@ public Mutable path(String path)
                 throw new IllegalArgumentException("Relative path with authority");
             if (!URIUtil.isPathValid(path))
                 throw new IllegalArgumentException("Path not correctly encoded: " + path);
+            // since we are resetting the path, lets clear out the path specific violations.
+            if (_violations != null)
+                _violations.removeIf(UriCompliance::isPathViolation);
             _uri = null;
             _path = null;
             _canonicalPath = null;
@@ -1016,6 +1019,9 @@ public Mutable pathQuery(String pathQuery)
         {
             if (hasAuthority() && !isPathValidForAuthority(pathQuery))
                 throw new IllegalArgumentException("Relative path with authority");
+            // since we are resetting the path, lets clear out the path specific violations.
+            if (_violations != null)
+                _violations.removeIf(UriCompliance::isPathViolation);
             _uri = null;
             _path = null;
             _canonicalPath = null;

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

@@ -145,13 +145,29 @@ public String getDescription()
     }
 
     public static final Set<Violation> NO_VIOLATION = Collections.unmodifiableSet(EnumSet.noneOf(Violation.class));
+
+    /**
+     * Set of violations that can trigger a HttpURI.isAmbiguous violation.
+     */
     public static final Set<Violation> AMBIGUOUS_VIOLATIONS = Collections.unmodifiableSet(EnumSet.of(
         Violation.AMBIGUOUS_EMPTY_SEGMENT,
         Violation.AMBIGUOUS_PATH_ENCODING,
         Violation.AMBIGUOUS_PATH_PARAMETER,
         Violation.AMBIGUOUS_PATH_SEGMENT,
         Violation.AMBIGUOUS_PATH_SEPARATOR));
 
+    /**
+     * List of Violations that apply only to the HttpURI.path section.
+     */
+    private static final Set<Violation> PATH_VIOLATIONS = Collections.unmodifiableSet(EnumSet.of(
+        Violation.AMBIGUOUS_EMPTY_SEGMENT,
+        Violation.AMBIGUOUS_PATH_ENCODING,
+        Violation.AMBIGUOUS_PATH_PARAMETER,
+        Violation.AMBIGUOUS_PATH_SEGMENT,
+        Violation.AMBIGUOUS_PATH_SEPARATOR,
+        Violation.SUSPICIOUS_PATH_CHARACTERS,
+        Violation.ILLEGAL_PATH_CHARACTERS));
+
     /**
      * Compliance mode that exactly follows <a href="https://tools.ietf.org/html/rfc3986">RFC3986</a>,
      * excluding all URI Violations.
@@ -352,6 +368,17 @@ public UriCompliance without(String name, Violation... violations)
         return new UriCompliance(name, remainder);
     }
 
+    /**
+     * Test if violation is referencing a HttpURI.path violation.
+     *
+     * @param violation the violation to test.
+     * @return true if violation is a path violation.
+     */
+    public static boolean isPathViolation(UriCompliance.Violation violation)
+    {
+        return PATH_VIOLATIONS.contains(violation);
+    }
+
     @Override
     public String toString()
     {

jetty-core/jetty-rewrite/src/test/java/org/eclipse/jetty/rewrite/handler/CompactPathRuleTest.java

@@ -13,13 +13,18 @@
 
 package org.eclipse.jetty.rewrite.handler;
 
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.Objects;
+import java.util.Properties;
 import java.util.stream.Stream;
 
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpTester;
 import org.eclipse.jetty.http.HttpURI;
 import org.eclipse.jetty.http.UriCompliance;
-import org.eclipse.jetty.io.Content;
 import org.eclipse.jetty.server.Handler;
 import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Response;
@@ -36,14 +41,20 @@ public static Stream<Arguments> scenarios()
     {
         return Stream.of(
             // shouldn't change anything
-            Arguments.of("/foo", null, "/foo", null, "/foo"),
-            Arguments.of("/", null, "/", null, "/"),
+            Arguments.of("/foo", null, "/foo", "", "/foo"),
+            Arguments.of("/", null, "/", "", "/"),
             // simple compact path
-            Arguments.of("////foo", null, "/foo", null, "/foo"),
+            Arguments.of("////foo", null, "/foo", "", "/foo"),
             // with simple query
             Arguments.of("//foo//bar", "a=b", "/foo/bar", "a=b", "/foo/bar?a=b"),
             // with query that has double slashes (should preserve slashes in query)
-            Arguments.of("//foo//bar", "a=b//c", "/foo/bar", "a=b//c", "/foo/bar?a=b//c")
+            Arguments.of("//foo//bar", "a=b//c", "/foo/bar", "a=b//c", "/foo/bar?a=b//c"),
+            // with ambiguous path parameter
+            Arguments.of("//foo/..;/bar", "a=b//c", "/bar", "a=b//c", "/bar?a=b//c"),
+            // with ambiguous path separator (not changed)
+            Arguments.of("//foo/b%2far", "a=b//c", "/foo/b%2Far", "a=b//c", "/foo/b%2Far?a=b//c"),
+            // with ambiguous path encoding (not changed)
+            Arguments.of("//foo/%2562ar", "a=b//c", "/foo/%2562ar", "a=b//c", "/foo/%2562ar?a=b//c")
         );
     }
 
@@ -59,24 +70,67 @@ public void testCompactPathRule(String inputPath, String inputQuery, String expe
             @Override
             public boolean handle(Request request, Response response, Callback callback)
             {
-                Content.Sink.write(response, true, request.getHttpURI().getPathQuery(), callback);
+                Properties props = new Properties();
+                HttpURI httpURI = request.getHttpURI();
+                props.setProperty("uri.path", of(httpURI.getPath()));
+                props.setProperty("uri.query", of(httpURI.getQuery()));
+                props.setProperty("uri.pathQuery", of(httpURI.getPathQuery()));
+                props.setProperty("uri.hasViolations", of(httpURI.hasViolations()));
+                props.setProperty("uri.isAmbiguous", of(httpURI.isAmbiguous()));
+                props.setProperty("uri.hasAmbiguousEmptySegment", of(httpURI.hasAmbiguousEmptySegment()));
+                props.setProperty("uri.hasAmbiguousEncoding", of(httpURI.hasAmbiguousEncoding()));
+                props.setProperty("uri.hasAmbiguousParameter", of(httpURI.hasAmbiguousParameter()));
+                props.setProperty("uri.hasAmbiguousSeparator", of(httpURI.hasAmbiguousSeparator()));
+                props.setProperty("uri.hasAmbiguousSegment", of(httpURI.hasAmbiguousSegment()));
+                try (ByteArrayOutputStream out = new ByteArrayOutputStream())
+                {
+                    props.store(out, "HttpURI State");
+                    response.write(true, ByteBuffer.wrap(out.toByteArray()), callback);
+                }
+                catch (IOException e)
+                {
+                    callback.failed(e);
+                }
                 return true;
             }
+
+            private String of(Object obj)
+            {
+                if (obj == null)
+                    return "";
+                if (obj instanceof Boolean)
+                    return Boolean.toString((Boolean)obj);
+                return Objects.toString(obj);
+            }
         });
 
 
         String request = """
             GET %s HTTP/1.1
             Host: localhost
-                        
+            
             """.formatted(HttpURI.build().path(inputPath).query(inputQuery));
 
         HttpTester.Response response = HttpTester.parseResponse(_connector.getResponse(request));
-        System.err.println(response.getReason());
         assertEquals(HttpStatus.OK_200, response.getStatus());
-        HttpURI.Mutable result = HttpURI.build(response.getContent());
-        assertEquals(expectedPath, result.getPath());
-        assertEquals(expectedQuery, result.getQuery());
-        assertEquals(expectedPathQuery, result.getPathQuery());
+        Properties props = new Properties();
+        try (ByteArrayInputStream in = new ByteArrayInputStream(response.getContentBytes()))
+        {
+            props.load(in);
+            assertEquals(expectedPath, props.getProperty("uri.path"));
+            assertEquals(expectedQuery, props.getProperty("uri.query"));
+            assertEquals(expectedPathQuery, props.getProperty("uri.pathQuery"));
+
+            boolean ambiguousPathSep = inputPath.contains("%2f");
+            boolean ambiguousPathEncoding = inputPath.contains("%25");
+
+            assertEquals(Boolean.toString(ambiguousPathSep || ambiguousPathEncoding), props.getProperty("uri.isAmbiguous"));
+            assertEquals(Boolean.toString(ambiguousPathSep || ambiguousPathEncoding), props.getProperty("uri.hasViolations"));
+            assertEquals("false", props.getProperty("uri.hasAmbiguousEmptySegment"));
+            assertEquals(Boolean.toString(ambiguousPathEncoding), props.getProperty("uri.hasAmbiguousEncoding"));
+            assertEquals("false", props.getProperty("uri.hasAmbiguousParameter"));
+            assertEquals(Boolean.toString(ambiguousPathSep), props.getProperty("uri.hasAmbiguousSeparator"));
+            assertEquals("false", props.getProperty("uri.hasAmbiguousSegment"));
+        }
     }
 }