Skip to content

Commit

Permalink
Fixes #8014 - Review HttpRequest URI construction. (#8146)
Browse files Browse the repository at this point in the history 
Now always adding a "/" before the path, if not already present.
Parse CONNECT URIs as Authority

Co-authored-by: Greg Wilkins <[email protected]>
(cherry picked from commit d1e64f4)
  • Loading branch information
@sbordet
sbordet authored Jun 8, 2022
1 parent 18653c4 commit 4ca8afb
Show file tree
Hide file tree
Showing 6 changed files with 169 additions and 7 deletions.
8 changes: 5 additions & 3 deletions jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
45 changes: 45 additions & 0 deletions jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientURITest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<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();
}
});

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
22 changes: 21 additions & 1 deletion jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down Expand Up @@ -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)
Expand All @@ -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;
Expand All @@ -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;
Expand Down
86 changes: 86 additions & 0 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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());
}
}
2 changes: 1 addition & 1 deletion jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ConnectHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
13 changes: 11 additions & 2 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down

0 comments on commit 4ca8afb

Please sign in to comment.