Skip to content

Commit

Permalink
Fixes #8014 - Review HttpRequest URI construction. (#8015)
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@sbordet @gregw
sbordet and gregw committed May 26, 2022
1 parent 99c743c commit d1e64f4
Show file tree
Hide file tree
Showing 7 changed files with 152 additions and 24 deletions.
8 changes: 5 additions & 3 deletions jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java
View file
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
50 changes: 50 additions & 0 deletions jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientURITest.java
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down
37 changes: 26 additions & 11 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}

Expand All @@ -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("/"))
{
Expand Down
68 changes: 68 additions & 0 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
View file
Expand Up @@ -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()
{
Expand Down Expand Up @@ -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());
}
}
9 changes: 1 addition & 8 deletions jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ConnectHandler.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions ...http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientLoadTest.java
View file
Expand Up @@ -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 ->
Expand Down

0 comments on commit d1e64f4

Please sign in to comment.