From 878ff231867c5d257eeb2340b739cd84dd233c26 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 26 May 2022 10:13:17 +0200 Subject: [PATCH] Fixes #8014 - Review HttpRequest URI construction. Now always adding a "/" before the path, if not already present. Parse CONNECT URIs as Authority Co-authored-by: Greg Wilkins (cherry picked from commit d1e64f469362bb9371d530cccded5ecb13fa1cb5) --- .../org/eclipse/jetty/client/HttpRequest.java | 8 +- .../jetty/client/HttpClientURITest.java | 45 ++++++++++ .../java/org/eclipse/jetty/http/HttpURI.java | 22 ++++- .../org/eclipse/jetty/http/HttpURITest.java | 86 +++++++++++++++++++ .../eclipse/jetty/proxy/ConnectHandler.java | 2 +- .../org/eclipse/jetty/server/Request.java | 13 ++- 6 files changed, 169 insertions(+), 7 deletions(-) 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 index 4b164f4a3d1a..9d461c40e66f 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java @@ -197,6 +197,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) @@ -925,14 +927,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 index ac5c1256530e..b31281baf18a 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientURITest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientURITest.java @@ -29,8 +29,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; @@ -82,6 +84,49 @@ 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 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(); + } + }); + + 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 index d6881c4f3a13..8ecc4aefded8 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java @@ -308,7 +308,7 @@ public void parseRequestTarget(String method, String uri) _uri = uri; if (HttpMethod.CONNECT.is(method)) - _path = uri; + parse(State.HOST, uri, 0, uri.length()); else parse(uri.startsWith("/") ? State.PATH : State.START, uri, 0, uri.length()); } @@ -1032,16 +1032,29 @@ public void setScheme(String scheme) */ public void setAuthority(String host, int port) { + if (host != null && !isPathValidForAuthority(_path)) + throw new IllegalArgumentException("Relative path with authority"); _host = host; _port = port; _uri = null; } + private boolean isPathValidForAuthority(String path) + { + if (path == null) + return true; + if (path.isEmpty() || "*".equals(path)) + return true; + return path.startsWith("/"); + } + /** * @param path the path */ public void setPath(String path) { + if (hasAuthority() && !isPathValidForAuthority(path)) + throw new IllegalArgumentException("Relative path with authority"); _uri = null; _path = null; if (path != null) @@ -1050,6 +1063,8 @@ public void setPath(String path) public void setPathQuery(String pathQuery) { + if (hasAuthority() && !isPathValidForAuthority(pathQuery)) + throw new IllegalArgumentException("Relative path with authority"); _uri = null; _path = null; _decodedPath = null; @@ -1063,6 +1078,11 @@ public void setPathQuery(String pathQuery) parse(State.PATH, pathQuery, 0, pathQuery.length()); } + private boolean hasAuthority() + { + return _host != null; + } + public void setQuery(String query) { _query = query; 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 index 7a50723d33f0..6f31cbc95edd 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java @@ -116,6 +116,32 @@ public void testParseRequestTarget() assertThat(uri.getPath(), is("/bar")); } + @Test + public void testCONNECT() + { + HttpURI uri = new HttpURI(); + + uri.parseRequestTarget("CONNECT", "host:80"); + assertThat(uri.getHost(), is("host")); + assertThat(uri.getPort(), is(80)); + assertThat(uri.getPath(), nullValue()); + + uri.parseRequestTarget("CONNECT", "host"); + assertThat(uri.getHost(), is("host")); + assertThat(uri.getPort(), is(-1)); + assertThat(uri.getPath(), nullValue()); + + uri.parseRequestTarget("CONNECT", "192.168.0.1:8080"); + assertThat(uri.getHost(), is("192.168.0.1")); + assertThat(uri.getPort(), is(8080)); + assertThat(uri.getPath(), nullValue()); + + uri.parseRequestTarget("CONNECT", "[::1]:8080"); + assertThat(uri.getHost(), is("[::1]")); + assertThat(uri.getPort(), is(8080)); + assertThat(uri.getPath(), nullValue()); + } + @Test public void testExtB() throws Exception { @@ -789,4 +815,64 @@ public void testEncodedQuery(String input, String expectedQuery) HttpURI httpURI = new HttpURI(input); assertThat("[" + input + "] .query", httpURI.getQuery(), is(expectedQuery)); } + + @Test + public void testRelativePathWithAuthority() + { + assertThrows(IllegalArgumentException.class, () -> + { + HttpURI httpURI = new HttpURI(); + httpURI.setAuthority("host", 0); + httpURI.setPath("path"); + }); + assertThrows(IllegalArgumentException.class, () -> + { + HttpURI httpURI = new HttpURI(); + httpURI.setAuthority("host", 8080); + httpURI.setPath(";p=v/url"); + }); + assertThrows(IllegalArgumentException.class, () -> + { + HttpURI httpURI = new HttpURI(); + httpURI.setAuthority("host", 0); + httpURI.setPath(";"); + }); + + assertThrows(IllegalArgumentException.class, () -> + { + HttpURI httpURI = new HttpURI(); + httpURI.setPath("path"); + httpURI.setAuthority("host", 0); + }); + assertThrows(IllegalArgumentException.class, () -> + { + HttpURI httpURI = new HttpURI(); + httpURI.setPath(";p=v/url"); + httpURI.setAuthority("host", 8080); + }); + assertThrows(IllegalArgumentException.class, () -> + { + HttpURI httpURI = new HttpURI(); + httpURI.setPath(";"); + httpURI.setAuthority("host", 0); + }); + + HttpURI uri = new HttpURI(); + uri.setPath("*"); + uri.setAuthority("host", 0); + assertEquals("//host*", uri.toString()); + uri = new HttpURI(); + uri.setAuthority("host", 0); + uri.setPath("*"); + assertEquals("//host*", uri.toString()); + + uri = new HttpURI(); + uri.setPath(""); + uri.setAuthority("host", 0); + assertEquals("//host", uri.toString()); + uri = new HttpURI(); + uri.setAuthority("host", 0); + uri.setPath(""); + assertEquals("//host", uri.toString()); + } } 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 index db41c94378ab..905adcbcdd70 100644 --- a/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ConnectHandler.java +++ b/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ConnectHandler.java @@ -195,7 +195,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques { if (HttpMethod.CONNECT.is(request.getMethod())) { - String serverAddress = request.getRequestURI(); + String serverAddress = baseRequest.getHttpURI().getAuthority(); if (LOG.isDebugEnabled()) LOG.debug("CONNECT request for {}", 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 index 8d3d6718703b..dd67bf9bc7e1 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -1836,9 +1836,18 @@ public void setMetaData(org.eclipse.jetty.http.MetaData.Request request) throw new BadMessageException(badMessage); } - _originalURI = uri.isAbsolute() && request.getHttpVersion() != HttpVersion.HTTP_2 ? uri.toString() : uri.getPathQuery(); + String encoded; + if (HttpMethod.CONNECT.is(request.getMethod())) + { + _originalURI = uri.getAuthority(); + encoded = "/"; + } + else + { + _originalURI = uri.isAbsolute() && request.getHttpVersion() != HttpVersion.HTTP_2 ? uri.toString() : uri.getPathQuery(); + encoded = uri.getPath(); + } - String encoded = uri.getPath(); String path; if (encoded == null) {