Skip to content

Commit d1e64f4

Browse files
sbordetgregw
sbordet
and
gregw
authored
Fixes #8014 - Review HttpRequest URI construction. (#8015)
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 <[email protected]>
1 parent 99c743c commit d1e64f4

File tree

7 files changed

+152
-24
lines changed

7 files changed

+152
-24
lines changed

‎jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,8 @@ public Request path(String path)
195195
String rawPath = uri.getRawPath();
196196
if (rawPath == null)
197197
rawPath = "";
198+
if (!rawPath.startsWith("/"))
199+
rawPath = "/" + rawPath;
198200
this.path = rawPath;
199201
String query = uri.getRawQuery();
200202
if (query != null)
@@ -949,14 +951,14 @@ private URI buildURI(boolean withQuery)
949951
return result;
950952
}
951953

952-
private URI newURI(String uri)
954+
private URI newURI(String path)
953955
{
954956
try
955957
{
956958
// Handle specially the "OPTIONS *" case, since it is possible to create a URI from "*" (!).
957-
if ("*".equals(uri))
959+
if ("*".equals(path))
958960
return null;
959-
URI result = new URI(uri);
961+
URI result = new URI(path);
960962
return result.isOpaque() ? null : result;
961963
}
962964
catch (URISyntaxException x)

‎jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientURITest.java‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,10 @@
2424
import java.net.URLEncoder;
2525
import java.nio.charset.StandardCharsets;
2626
import java.util.Locale;
27+
import java.util.concurrent.CountDownLatch;
2728
import java.util.concurrent.ExecutionException;
2829
import java.util.concurrent.TimeUnit;
30+
import java.util.concurrent.atomic.AtomicReference;
2931
import javax.servlet.ServletException;
3032
import javax.servlet.http.HttpServletRequest;
3133
import javax.servlet.http.HttpServletResponse;
@@ -36,6 +38,8 @@
3638
import org.eclipse.jetty.http.HttpHeader;
3739
import org.eclipse.jetty.http.HttpMethod;
3840
import org.eclipse.jetty.http.HttpStatus;
41+
import org.eclipse.jetty.http.UriCompliance;
42+
import org.eclipse.jetty.server.HttpConfiguration;
3943
import org.eclipse.jetty.server.handler.AbstractHandler;
4044
import org.eclipse.jetty.toolchain.test.Net;
4145
import org.eclipse.jetty.util.Fields;
@@ -77,6 +81,52 @@ public void testIPv6Host(Scenario scenario) throws Exception
7781
assertEquals(HttpStatus.OK_200, request.send().getStatus());
7882
}
7983

84+
@ParameterizedTest
85+
@ArgumentsSource(ScenarioProvider.class)
86+
public void testPathWithPathParameter(Scenario scenario) throws Exception
87+
{
88+
AtomicReference<CountDownLatch> serverLatchRef = new AtomicReference<>();
89+
start(scenario, new EmptyServerHandler()
90+
{
91+
@Override
92+
protected void service(String target, org.eclipse.jetty.server.Request jettyRequest, HttpServletRequest request, HttpServletResponse response)
93+
{
94+
if (jettyRequest.getHttpURI().hasAmbiguousEmptySegment())
95+
response.setStatus(400);
96+
serverLatchRef.get().countDown();
97+
}
98+
});
99+
// Allow empty segments to test them.
100+
connector.getContainedBeans(HttpConfiguration.class)
101+
.forEach(httpConfig -> httpConfig.setUriCompliance(UriCompliance.from("DEFAULT,AMBIGUOUS_EMPTY_SEGMENT")));
102+
103+
serverLatchRef.set(new CountDownLatch(1));
104+
ContentResponse response1 = client.newRequest("localhost", connector.getLocalPort())
105+
.scheme(scenario.getScheme())
106+
.path("/url;p=v")
107+
.send();
108+
assertEquals(HttpStatus.OK_200, response1.getStatus());
109+
assertTrue(serverLatchRef.get().await(5, TimeUnit.SECONDS));
110+
111+
// Ambiguous empty segment.
112+
serverLatchRef.set(new CountDownLatch(1));
113+
ContentResponse response2 = client.newRequest("localhost", connector.getLocalPort())
114+
.scheme(scenario.getScheme())
115+
.path(";p=v/url")
116+
.send();
117+
assertEquals(HttpStatus.BAD_REQUEST_400, response2.getStatus());
118+
assertTrue(serverLatchRef.get().await(5, TimeUnit.SECONDS));
119+
120+
// Ambiguous empty segment.
121+
serverLatchRef.set(new CountDownLatch(1));
122+
ContentResponse response3 = client.newRequest("localhost", connector.getLocalPort())
123+
.scheme(scenario.getScheme())
124+
.path(";@host.org/url")
125+
.send();
126+
assertEquals(HttpStatus.BAD_REQUEST_400, response3.getStatus());
127+
assertTrue(serverLatchRef.get().await(5, TimeUnit.SECONDS));
128+
}
129+
80130
@ParameterizedTest
81131
@ArgumentsSource(ScenarioProvider.class)
82132
public void testIDNHost(Scenario scenario) throws Exception

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

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ static Immutable from(String uri)
119119
static Immutable from(String method, String uri)
120120
{
121121
if (HttpMethod.CONNECT.is(method))
122-
return new Immutable(uri);
122+
return HttpURI.build().uri(method, uri).asImmutable();
123123
if (uri.startsWith("/"))
124124
return HttpURI.build().pathQuery(uri).asImmutable();
125125
return HttpURI.from(uri);
@@ -628,6 +628,8 @@ public String asString()
628628
*/
629629
public Mutable authority(String host, int port)
630630
{
631+
if (host != null && !isPathValidForAuthority(_path))
632+
throw new IllegalArgumentException("Relative path with authority");
631633
_user = null;
632634
_host = host;
633635
_port = port;
@@ -636,19 +638,30 @@ public Mutable authority(String host, int port)
636638
}
637639

638640
/**
639-
* @param hostport the host and port combined
641+
* @param hostPort the host and port combined
640642
* @return this mutable
641643
*/
642-
public Mutable authority(String hostport)
644+
public Mutable authority(String hostPort)
643645
{
644-
HostPort hp = new HostPort(hostport);
646+
if (hostPort != null && !isPathValidForAuthority(_path))
647+
throw new IllegalArgumentException("Relative path with authority");
648+
HostPort hp = new HostPort(hostPort);
645649
_user = null;
646650
_host = hp.getHost();
647651
_port = hp.getPort();
648652
_uri = null;
649653
return this;
650654
}
651655

656+
private boolean isPathValidForAuthority(String path)
657+
{
658+
if (path == null)
659+
return true;
660+
if (path.isEmpty() || "*".equals(path))
661+
return true;
662+
return path.startsWith("/");
663+
}
664+
652665
public Mutable clear()
653666
{
654667
_scheme = null;
@@ -775,6 +788,8 @@ public int hashCode()
775788

776789
public Mutable host(String host)
777790
{
791+
if (host != null && !isPathValidForAuthority(_path))
792+
throw new IllegalArgumentException("Relative path with authority");
778793
_host = host;
779794
_uri = null;
780795
return this;
@@ -834,10 +849,12 @@ public Mutable param(String param)
834849

835850
/**
836851
* @param path the path
837-
* @return this Mutuble
852+
* @return this Mutable
838853
*/
839854
public Mutable path(String path)
840855
{
856+
if (hasAuthority() && !isPathValidForAuthority(path))
857+
throw new IllegalArgumentException("Relative path with authority");
841858
_uri = null;
842859
_path = path;
843860
_decodedPath = null;
@@ -846,6 +863,8 @@ public Mutable path(String path)
846863

847864
public Mutable pathQuery(String pathQuery)
848865
{
866+
if (hasAuthority() && !isPathValidForAuthority(pathQuery))
867+
throw new IllegalArgumentException("Relative path with authority");
849868
_uri = null;
850869
_path = null;
851870
_decodedPath = null;
@@ -911,10 +930,7 @@ public Mutable uri(HttpURI uri)
911930
_query = uri.getQuery();
912931
_uri = null;
913932
_decodedPath = uri.getDecodedPath();
914-
if (uri.hasAmbiguousSeparator())
915-
_violations.add(Violation.AMBIGUOUS_PATH_SEPARATOR);
916-
if (uri.hasAmbiguousSegment())
917-
_violations.add(Violation.AMBIGUOUS_PATH_SEGMENT);
933+
_violations.addAll(uri.getViolations());
918934
return this;
919935
}
920936

@@ -931,8 +947,7 @@ public Mutable uri(String method, String uri)
931947
if (HttpMethod.CONNECT.is(method))
932948
{
933949
clear();
934-
_uri = uri;
935-
_path = uri;
950+
parse(State.HOST, uri);
936951
}
937952
else if (uri.startsWith("/"))
938953
{

‎jetty-http/src/test/java/org/eclipse/jetty/http/HttpURITest.java‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,32 @@ public void testParseRequestTarget()
177177
assertThat(uri.getPath(), is("/bar"));
178178
}
179179

180+
@Test
181+
public void testCONNECT()
182+
{
183+
HttpURI uri;
184+
185+
uri = HttpURI.from("CONNECT", "host:80");
186+
assertThat(uri.getHost(), is("host"));
187+
assertThat(uri.getPort(), is(80));
188+
assertThat(uri.getPath(), nullValue());
189+
190+
uri = HttpURI.from("CONNECT", "host");
191+
assertThat(uri.getHost(), is("host"));
192+
assertThat(uri.getPort(), is(-1));
193+
assertThat(uri.getPath(), nullValue());
194+
195+
uri = HttpURI.from("CONNECT", "192.168.0.1:8080");
196+
assertThat(uri.getHost(), is("192.168.0.1"));
197+
assertThat(uri.getPort(), is(8080));
198+
assertThat(uri.getPath(), nullValue());
199+
200+
uri = HttpURI.from("CONNECT", "[::1]:8080");
201+
assertThat(uri.getHost(), is("[::1]"));
202+
assertThat(uri.getPort(), is(8080));
203+
assertThat(uri.getPath(), nullValue());
204+
}
205+
180206
@Test
181207
public void testAt()
182208
{
@@ -824,4 +850,46 @@ public void testEncodedQuery(String input, String expectedQuery)
824850
HttpURI httpURI = HttpURI.build(input);
825851
assertThat("[" + input + "] .query", httpURI.getQuery(), is(expectedQuery));
826852
}
853+
854+
@Test
855+
public void testRelativePathWithAuthority()
856+
{
857+
assertThrows(IllegalArgumentException.class, () -> HttpURI.build()
858+
.authority("host")
859+
.path("path"));
860+
assertThrows(IllegalArgumentException.class, () -> HttpURI.build()
861+
.authority("host", 8080)
862+
.path(";p=v/url"));
863+
assertThrows(IllegalArgumentException.class, () -> HttpURI.build()
864+
.host("host")
865+
.path(";"));
866+
867+
assertThrows(IllegalArgumentException.class, () -> HttpURI.build()
868+
.path("path")
869+
.authority("host"));
870+
assertThrows(IllegalArgumentException.class, () -> HttpURI.build()
871+
.path(";p=v/url")
872+
.authority("host", 8080));
873+
assertThrows(IllegalArgumentException.class, () -> HttpURI.build()
874+
.path(";")
875+
.host("host"));
876+
877+
HttpURI.Mutable uri = HttpURI.build()
878+
.path("*")
879+
.authority("host");
880+
assertEquals("//host*", uri.asString());
881+
uri = HttpURI.build()
882+
.authority("host")
883+
.path("*");
884+
assertEquals("//host*", uri.asString());
885+
886+
uri = HttpURI.build()
887+
.path("")
888+
.authority("host");
889+
assertEquals("//host", uri.asString());
890+
uri = HttpURI.build()
891+
.authority("host")
892+
.path("");
893+
assertEquals("//host", uri.asString());
894+
}
827895
}

‎jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ConnectHandler.java‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,6 @@
3333
import org.eclipse.jetty.http.HttpHeader;
3434
import org.eclipse.jetty.http.HttpHeaderValue;
3535
import org.eclipse.jetty.http.HttpMethod;
36-
import org.eclipse.jetty.http.HttpURI;
37-
import org.eclipse.jetty.http.HttpVersion;
3836
import org.eclipse.jetty.io.ByteBufferPool;
3937
import org.eclipse.jetty.io.Connection;
4038
import org.eclipse.jetty.io.EndPoint;
@@ -193,12 +191,7 @@ public void handle(String target, Request jettyRequest, HttpServletRequest reque
193191
String tunnelProtocol = jettyRequest.getMetaData().getProtocol();
194192
if (HttpMethod.CONNECT.is(request.getMethod()) && tunnelProtocol == null)
195193
{
196-
String serverAddress = target;
197-
if (HttpVersion.HTTP_2.is(request.getProtocol()))
198-
{
199-
HttpURI httpURI = jettyRequest.getHttpURI();
200-
serverAddress = httpURI.getHost() + ":" + httpURI.getPort();
201-
}
194+
String serverAddress = jettyRequest.getHttpURI().getAuthority();
202195
if (LOG.isDebugEnabled())
203196
LOG.debug("CONNECT request for {}", serverAddress);
204197
handleConnect(jettyRequest, request, response, serverAddress);

‎jetty-server/src/main/java/org/eclipse/jetty/server/Request.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1749,6 +1749,7 @@ public void setMetaData(MetaData.Request request)
17491749
if (field instanceof HostPortHttpField)
17501750
{
17511751
HostPortHttpField authority = (HostPortHttpField)field;
1752+
17521753
builder.host(authority.getHost()).port(authority.getPort());
17531754
}
17541755
else

‎tests/test-http-client-transport/src/test/java/org/eclipse/jetty/http/client/HttpClientLoadTest.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,7 @@ public void testIterative(Transport transport) throws Exception
118118
public void testConcurrent(Transport transport) throws Exception
119119
{
120120
// 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);
123122

124123
init(transport);
125124
scenario.start(new LoadHandler(), client ->

0 commit comments

Comments
 (0)