Fixes #8014 - Review HttpRequest URI construction. (#8015)

sbordet · gregw · web-flow · commit d1e64f469362 · 2022-05-26T18:13:17.000+10:00
Fixes #8014 - Review HttpRequest URI construction. Now always adding a "/" before the path, if not already present. Disabled flakey HTTP/3 test. Parse CONNECT URIs as Authority Co-authored-by: Greg Wilkins <gregw@webtide.com>
diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java
@@ -195,6 +195,8 @@ public Request path(String path)
             String rawPath = uri.getRawPath();
             if (rawPath == null)
                 rawPath = "";
+            if (!rawPath.startsWith("/"))
+                rawPath = "/" + rawPath;
             this.path = rawPath;
             String query = uri.getRawQuery();
             if (query != null)
@@ -949,14 +951,14 @@ private URI buildURI(boolean withQuery)
         return result;
     }
 
-    private URI newURI(String uri)
+    private URI newURI(String path)
     {
         try
         {
             // Handle specially the "OPTIONS *" case, since it is possible to create a URI from "*" (!).
-            if ("*".equals(uri))
+            if ("*".equals(path))
                 return null;
-            URI result = new URI(uri);
+            URI result = new URI(path);
             return result.isOpaque() ? null : result;
         }
         catch (URISyntaxException x)
diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientURITest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientURITest.java
@@ -24,8 +24,10 @@
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
 import java.util.Locale;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -36,6 +38,8 @@
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpMethod;
 import org.eclipse.jetty.http.HttpStatus;
+import org.eclipse.jetty.http.UriCompliance;
+import org.eclipse.jetty.server.HttpConfiguration;
 import org.eclipse.jetty.server.handler.AbstractHandler;
 import org.eclipse.jetty.toolchain.test.Net;
 import org.eclipse.jetty.util.Fields;
@@ -77,6 +81,52 @@ public void testIPv6Host(Scenario scenario) throws Exception
         assertEquals(HttpStatus.OK_200, request.send().getStatus());
     }
 
+    @ParameterizedTest
+    @ArgumentsSource(ScenarioProvider.class)
+    public void testPathWithPathParameter(Scenario scenario) throws Exception
+    {
+        AtomicReference<CountDownLatch> serverLatchRef = new AtomicReference<>();
+        start(scenario, new EmptyServerHandler()
+        {
+            @Override
+            protected void service(String target, org.eclipse.jetty.server.Request jettyRequest, HttpServletRequest request, HttpServletResponse response)
+            {
+                if (jettyRequest.getHttpURI().hasAmbiguousEmptySegment())
+                    response.setStatus(400);
+                serverLatchRef.get().countDown();
+            }
+        });
+        // Allow empty segments to test them.
+        connector.getContainedBeans(HttpConfiguration.class)
+            .forEach(httpConfig -> httpConfig.setUriCompliance(UriCompliance.from("DEFAULT,AMBIGUOUS_EMPTY_SEGMENT")));
+
+        serverLatchRef.set(new CountDownLatch(1));
+        ContentResponse response1 = client.newRequest("localhost", connector.getLocalPort())
+            .scheme(scenario.getScheme())
+            .path("/url;p=v")
+            .send();
+        assertEquals(HttpStatus.OK_200, response1.getStatus());
+        assertTrue(serverLatchRef.get().await(5, TimeUnit.SECONDS));
+
+        // Ambiguous empty segment.
+        serverLatchRef.set(new CountDownLatch(1));
+        ContentResponse response2 = client.newRequest("localhost", connector.getLocalPort())
+            .scheme(scenario.getScheme())
+            .path(";p=v/url")
+            .send();
+        assertEquals(HttpStatus.BAD_REQUEST_400, response2.getStatus());
+        assertTrue(serverLatchRef.get().await(5, TimeUnit.SECONDS));
+
+        // Ambiguous empty segment.
+        serverLatchRef.set(new CountDownLatch(1));
+        ContentResponse response3 = client.newRequest("localhost", connector.getLocalPort())
+            .scheme(scenario.getScheme())
+            .path(";@host.org/url")
+            .send();
+        assertEquals(HttpStatus.BAD_REQUEST_400, response3.getStatus());
+        assertTrue(serverLatchRef.get().await(5, TimeUnit.SECONDS));
+    }
+
     @ParameterizedTest
     @ArgumentsSource(ScenarioProvider.class)
     public void testIDNHost(Scenario scenario) throws Exception
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
@@ -119,7 +119,7 @@ static Immutable from(String uri)
     static Immutable from(String method, String uri)
     {
         if (HttpMethod.CONNECT.is(method))
-            return new Immutable(uri);
+            return HttpURI.build().uri(method, uri).asImmutable();
         if (uri.startsWith("/"))
             return HttpURI.build().pathQuery(uri).asImmutable();
         return HttpURI.from(uri);
@@ -628,6 +628,8 @@ public String asString()
          */
         public Mutable authority(String host, int port)
         {
+            if (host != null && !isPathValidForAuthority(_path))
+                throw new IllegalArgumentException("Relative path with authority");
             _user = null;
             _host = host;
             _port = port;
@@ -636,19 +638,30 @@ public Mutable authority(String host, int port)
         }
 
         /**
-         * @param hostport the host and port combined
+         * @param hostPort the host and port combined
          * @return this mutable
          */
-        public Mutable authority(String hostport)
+        public Mutable authority(String hostPort)
         {
-            HostPort hp = new HostPort(hostport);
+            if (hostPort != null && !isPathValidForAuthority(_path))
+                throw new IllegalArgumentException("Relative path with authority");
+            HostPort hp = new HostPort(hostPort);
             _user = null;
             _host = hp.getHost();
             _port = hp.getPort();
             _uri = null;
             return this;
         }
 
+        private boolean isPathValidForAuthority(String path)
+        {
+            if (path == null)
+                return true;
+            if (path.isEmpty() || "*".equals(path))
+                return true;
+            return path.startsWith("/");
+        }
+
         public Mutable clear()
         {
             _scheme = null;
@@ -775,6 +788,8 @@ public int hashCode()
 
         public Mutable host(String host)
         {
+            if (host != null && !isPathValidForAuthority(_path))
+                throw new IllegalArgumentException("Relative path with authority");
             _host = host;
             _uri = null;
             return this;
@@ -834,10 +849,12 @@ public Mutable param(String param)
 
         /**
          * @param path the path
-         * @return this Mutuble
+         * @return this Mutable
          */
         public Mutable path(String path)
         {
+            if (hasAuthority() && !isPathValidForAuthority(path))
+                throw new IllegalArgumentException("Relative path with authority");
             _uri = null;
             _path = path;
             _decodedPath = null;
@@ -846,6 +863,8 @@ public Mutable path(String path)
 
         public Mutable pathQuery(String pathQuery)
         {
+            if (hasAuthority() && !isPathValidForAuthority(pathQuery))
+                throw new IllegalArgumentException("Relative path with authority");
             _uri = null;
             _path = null;
             _decodedPath = null;
@@ -911,10 +930,7 @@ public Mutable uri(HttpURI uri)
             _query = uri.getQuery();
             _uri = null;
             _decodedPath = uri.getDecodedPath();
-            if (uri.hasAmbiguousSeparator())
-                _violations.add(Violation.AMBIGUOUS_PATH_SEPARATOR);
-            if (uri.hasAmbiguousSegment())
-                _violations.add(Violation.AMBIGUOUS_PATH_SEGMENT);
+            _violations.addAll(uri.getViolations());
             return this;
         }
 
@@ -931,8 +947,7 @@ public Mutable uri(String method, String uri)
             if (HttpMethod.CONNECT.is(method))
             {
                 clear();
-                _uri = uri;
-                _path = uri;
+                parse(State.HOST, uri);
             }
             else if (uri.startsWith("/"))
             {
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
@@ -177,6 +177,32 @@ public void testParseRequestTarget()
         assertThat(uri.getPath(), is("/bar"));
     }
 
+    @Test
+    public void testCONNECT()
+    {
+        HttpURI uri;
+
+        uri = HttpURI.from("CONNECT", "host:80");
+        assertThat(uri.getHost(), is("host"));
+        assertThat(uri.getPort(), is(80));
+        assertThat(uri.getPath(), nullValue());
+
+        uri = HttpURI.from("CONNECT", "host");
+        assertThat(uri.getHost(), is("host"));
+        assertThat(uri.getPort(), is(-1));
+        assertThat(uri.getPath(), nullValue());
+
+        uri = HttpURI.from("CONNECT", "192.168.0.1:8080");
+        assertThat(uri.getHost(), is("192.168.0.1"));
+        assertThat(uri.getPort(), is(8080));
+        assertThat(uri.getPath(), nullValue());
+
+        uri = HttpURI.from("CONNECT", "[::1]:8080");
+        assertThat(uri.getHost(), is("[::1]"));
+        assertThat(uri.getPort(), is(8080));
+        assertThat(uri.getPath(), nullValue());
+    }
+
     @Test
     public void testAt()
     {
@@ -824,4 +850,46 @@ public void testEncodedQuery(String input, String expectedQuery)
         HttpURI httpURI = HttpURI.build(input);
         assertThat("[" + input + "] .query", httpURI.getQuery(), is(expectedQuery));
     }
+
+    @Test
+    public void testRelativePathWithAuthority()
+    {
+        assertThrows(IllegalArgumentException.class, () -> HttpURI.build()
+            .authority("host")
+            .path("path"));
+        assertThrows(IllegalArgumentException.class, () -> HttpURI.build()
+            .authority("host", 8080)
+            .path(";p=v/url"));
+        assertThrows(IllegalArgumentException.class, () -> HttpURI.build()
+            .host("host")
+            .path(";"));
+
+        assertThrows(IllegalArgumentException.class, () -> HttpURI.build()
+            .path("path")
+            .authority("host"));
+        assertThrows(IllegalArgumentException.class, () -> HttpURI.build()
+            .path(";p=v/url")
+            .authority("host", 8080));
+        assertThrows(IllegalArgumentException.class, () -> HttpURI.build()
+            .path(";")
+            .host("host"));
+
+        HttpURI.Mutable uri = HttpURI.build()
+            .path("*")
+            .authority("host");
+        assertEquals("//host*", uri.asString());
+        uri = HttpURI.build()
+            .authority("host")
+            .path("*");
+        assertEquals("//host*", uri.asString());
+
+        uri = HttpURI.build()
+            .path("")
+            .authority("host");
+        assertEquals("//host", uri.asString());
+        uri = HttpURI.build()
+            .authority("host")
+            .path("");
+        assertEquals("//host", uri.asString());
+    }
 }
diff --git a/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ConnectHandler.java b/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ConnectHandler.java
@@ -33,8 +33,6 @@
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpHeaderValue;
 import org.eclipse.jetty.http.HttpMethod;
-import org.eclipse.jetty.http.HttpURI;
-import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.io.ByteBufferPool;
 import org.eclipse.jetty.io.Connection;
 import org.eclipse.jetty.io.EndPoint;
@@ -193,12 +191,7 @@ public void handle(String target, Request jettyRequest, HttpServletRequest reque
         String tunnelProtocol = jettyRequest.getMetaData().getProtocol();
         if (HttpMethod.CONNECT.is(request.getMethod()) && tunnelProtocol == null)
         {
-            String serverAddress = target;
-            if (HttpVersion.HTTP_2.is(request.getProtocol()))
-            {
-                HttpURI httpURI = jettyRequest.getHttpURI();
-                serverAddress = httpURI.getHost() + ":" + httpURI.getPort();
-            }
+            String serverAddress = jettyRequest.getHttpURI().getAuthority();
             if (LOG.isDebugEnabled())
                 LOG.debug("CONNECT request for {}", serverAddress);
             handleConnect(jettyRequest, request, response, serverAddress);
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
@@ -1749,6 +1749,7 @@ public void setMetaData(MetaData.Request request)
                 if (field instanceof HostPortHttpField)
                 {
                     HostPortHttpField authority = (HostPortHttpField)field;
+
                     builder.host(authority.getHost()).port(authority.getPort());
                 }
                 else
diff --git a/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientLoadTest.java b/tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientLoadTest.java
@@ -118,8 +118,7 @@ public void testIterative(Transport transport) throws Exception
     public void testConcurrent(Transport transport) throws Exception
     {
         // TODO: cannot run HTTP/3 (or UDP) in Jenkins.
-        if ("ci".equals(System.getProperty("env")))
-            Assumptions.assumeTrue(transport != Transport.H3);
+        Assumptions.assumeTrue(transport != Transport.H3);
 
         init(transport);
         scenario.start(new LoadHandler(), client ->

Original file line number	Diff line number	Diff line change
`@@ -1749,6 +1749,7 @@ public void setMetaData(MetaData.Request request)`
`1749`	`1749`	`if (field instanceof HostPortHttpField)`
`1750`	`1750`	`{`
`1751`	`1751`	`HostPortHttpField authority = (HostPortHttpField)field;`
	`1752`	`+`
`1752`	`1753`	`builder.host(authority.getHost()).port(authority.getPort());`
`1753`	`1754`	`}`
`1754`	`1755`	`else`
Original file line number	Diff line number	Diff line change
`@@ -118,8 +118,7 @@ public void testIterative(Transport transport) throws Exception`
`118`	`118`	`public void testConcurrent(Transport transport) throws Exception`
`119`	`119`	`{`
`120`	`120`	`// TODO: cannot run HTTP/3 (or UDP) in Jenkins.`
`121`		`- if ("ci".equals(System.getProperty("env")))`
`122`		`- Assumptions.assumeTrue(transport != Transport.H3);`
	`121`	`+ Assumptions.assumeTrue(transport != Transport.H3);`
`123`	`122`
`124`	`123`	`init(transport);`
`125`	`124`	`scenario.start(new LoadHandler(), client ->`