Skip to content

Commit 89d2ac7

Browse files
gregwgregw
authored
Regular handling of bad URIs (#14011)
Remove the differences in how bad URI are handled. * Regular handling of bad URIs
1 parent 0c4120e commit 89d2ac7

File tree

5 files changed

+182
-15
lines changed

5 files changed

+182
-15
lines changed

‎jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpSenderOverHTTP.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,9 @@ public HeadersCallback(HttpExchange exchange, HttpContent content, Callback call
198198
String query = request.getQuery();
199199
if (query != null)
200200
path += "?" + query;
201-
metaData = new MetaData.Request(request.getMethod(), new HttpURI(path), request.getVersion(), request.getHeaders(), contentLength);
201+
HttpURI uri = new HttpURI();
202+
uri.parseRequestTarget(request.getMethod(), path);
203+
metaData = new MetaData.Request(request.getMethod(), uri, request.getVersion(), request.getHeaders(), contentLength);
202204
metaData.setTrailerSupplier(request.getTrailers());
203205

204206
if (!expects100Continue(request))

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

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,8 @@ private void parse(State state, final String uri, final int offset, final int en
433433
switch (c)
434434
{
435435
case '/':
436-
mark = i;
436+
pathMark = mark = i;
437+
segment = mark + 1;
437438
state = State.HOST_OR_PATH;
438439
break;
439440
case ';':
@@ -471,6 +472,8 @@ private void parse(State state, final String uri, final int offset, final int en
471472
pathMark = segment = i;
472473
state = State.PATH;
473474
break;
475+
case ':':
476+
throw new IllegalArgumentException("Bad Scheme");
474477
default:
475478
mark = i;
476479
if (_scheme == null)
@@ -491,7 +494,7 @@ private void parse(State state, final String uri, final int offset, final int en
491494
{
492495
case ':':
493496
// must have been a scheme
494-
_scheme = uri.substring(mark, i);
497+
_scheme = URIUtil.validateScheme(uri.substring(mark, i));
495498
// Start again with scheme set
496499
state = State.START;
497500
break;
@@ -596,8 +599,19 @@ private void parse(State state, final String uri, final int offset, final int en
596599
encoded = true;
597600
break;
598601
case '#':
602+
case '?':
599603
case ';':
600-
throw new IllegalArgumentException("Bad authority");
604+
if (encodedCharacters > 0)
605+
throw new IllegalArgumentException("Bad authority");
606+
_host = uri.substring(mark, i);
607+
if (_host.isEmpty())
608+
throw new IllegalArgumentException("Bad authority");
609+
encoded = false;
610+
pathMark = mark = i;
611+
segment = mark + 1;
612+
state = State.PATH;
613+
i--;
614+
break;
601615

602616
default:
603617
if (encodedCharacters > 0)
@@ -621,8 +635,13 @@ else if (!isUnreservedPctEncodedOrSubDelim(c))
621635
case '/':
622636
throw new IllegalArgumentException("No closing ']' for ipv6 in " + uri);
623637
case ']':
624-
c = uri.charAt(++i);
625-
_host = uri.substring(mark, i);
638+
i++;
639+
String host = uri.substring(mark, i);
640+
URIUtil.validateInetAddress(host);
641+
_host = host;
642+
if (i == end)
643+
break;
644+
c = uri.charAt(i);
626645
if (c == ':')
627646
{
628647
mark = i + 1;
@@ -637,7 +656,7 @@ else if (!isUnreservedPctEncodedOrSubDelim(c))
637656
case ':':
638657
break;
639658
default:
640-
if (!isHexDigit(c))
659+
if (!isHexDigit(c) && c != '.')
641660
throw new IllegalArgumentException("Bad authority");
642661
break;
643662
}
@@ -805,6 +824,7 @@ else if (c == '/')
805824
break;
806825
case SCHEME_OR_PATH:
807826
case HOST_OR_PATH:
827+
checkSegment(uri, segment, end, false);
808828
_path = uri.substring(mark, end);
809829
break;
810830
case HOST:

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

Lines changed: 80 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@
3838
import static org.hamcrest.Matchers.notNullValue;
3939
import static org.hamcrest.Matchers.nullValue;
4040
import static org.junit.jupiter.api.Assertions.assertEquals;
41+
import static org.junit.jupiter.api.Assertions.assertFalse;
42+
import static org.junit.jupiter.api.Assertions.assertNull;
4143
import static org.junit.jupiter.api.Assertions.assertThrows;
4244
import static org.junit.jupiter.api.Assertions.assertTrue;
4345
import static org.junit.jupiter.api.Assertions.fail;
@@ -158,6 +160,19 @@ public void testAt() throws Exception
158160
assertEquals("/@foo/bar", uri.getPath());
159161
}
160162

163+
/**
164+
* Test of an HttpURI of just a "/".
165+
*/
166+
@Test
167+
public void testFromSlash()
168+
{
169+
HttpURI uri = new HttpURI("/");
170+
assertFalse(uri.hasViolations());
171+
assertNull(uri.getScheme());
172+
assertNull(uri.getAuthority());
173+
assertEquals("/", uri.getPath());
174+
}
175+
161176
@Test
162177
public void testParams() throws Exception
163178
{
@@ -684,6 +699,8 @@ public static Stream<Arguments> parseData()
684699

685700
// Simple IPv6 host no port (default path)
686701
Arguments.of("http://[2001:db8::1]/", "http", "[2001:db8::1]", null, "/", null, null, null),
702+
Arguments.of("http://[0:0:0:0:0:ffff:127.0.0.1]/", "http", "[0:0:0:0:0:ffff:127.0.0.1]", null, "/", null, null, null),
703+
Arguments.of("http://[::ffff:127.0.0.1]/", "http", "[::ffff:127.0.0.1]", null, "/", null, null, null),
687704

688705
// Scheme-less IPv6, host with port (default path)
689706
Arguments.of("//[2001:db8::1]:8080/", null, "[2001:db8::1]", "8080", "/", null, null, null),
@@ -725,7 +742,8 @@ public void testParseString(String input, String scheme, String host, Integer po
725742
assertThat("[" + input + "] .param", httpUri.getParam(), is(param));
726743
assertThat("[" + input + "] .query", httpUri.getQuery(), is(query));
727744
assertThat("[" + input + "] .fragment", httpUri.getFragment(), is(fragment));
728-
assertThat("[" + input + "] .toString", httpUri.toString(), is(input));
745+
if (!input.contains(":ffff:127.0.0.1"))
746+
assertThat("[" + input + "] .toString", httpUri.toString(), is(input));
729747
}
730748
catch (URISyntaxException e)
731749
{
@@ -758,7 +776,8 @@ public void testParseURI(String input, String scheme, String host, Integer port,
758776
HttpURI httpUri = new HttpURI(input);
759777

760778
assertThat("[" + input + "] .scheme", httpUri.getScheme(), is(scheme));
761-
assertThat("[" + input + "] .host", httpUri.getHost(), is(host));
779+
if (!input.contains(":ffff:127.0.0.1"))
780+
assertThat("[" + input + "] .host", httpUri.getHost(), is(host));
762781
assertThat("[" + input + "] .port", httpUri.getPort(), is(port == null ? -1 : port));
763782
assertThat("[" + input + "] .path", httpUri.getPath(), is(path));
764783
assertThat("[" + input + "] .param", httpUri.getParam(), is(param));
@@ -871,6 +890,24 @@ public void testRelativePathWithAuthority()
871890
assertEquals("//host", uri.toString());
872891
}
873892

893+
public static Stream<String> badSchemes()
894+
{
895+
return Stream.of(
896+
"://host/path",
897+
"\t://host/path",
898+
" ://host/path",
899+
"unknown^://host/path",
900+
"http^://host/path"
901+
);
902+
}
903+
904+
@ParameterizedTest
905+
@MethodSource("badSchemes")
906+
public void testBadSchemes(String uri)
907+
{
908+
assertThrows(IllegalArgumentException.class, () -> new HttpURI(uri));
909+
}
910+
874911
public static Stream<String> badAuthorities()
875912
{
876913
return Stream.of(
@@ -890,9 +927,11 @@ public static Stream<String> badAuthorities()
890927
"https://user@host:notport/path",
891928
"https://user:password@host:notport/path",
892929
"https://user @host.com/",
893-
"https://user#@host.com/",
894930
"https://[notIpv6]/",
895-
"https://bad[0::1::2::3::4]/"
931+
"https://bad[0::1::2::3::4]/",
932+
"http://[normal.com@]vulndetector.com/",
933+
"http://normal.com[user@vulndetector].com/",
934+
"http://normal.com[@]vulndetector.com/"
896935
);
897936
}
898937

@@ -902,4 +941,41 @@ public void testBadAuthority(String uri)
902941
{
903942
assertThrows(IllegalArgumentException.class, () -> new HttpURI(uri));
904943
}
944+
945+
public static Stream<Arguments> authoritiesNoPath()
946+
{
947+
return Stream.of(
948+
Arguments.of("http://good.com#@evil.com", "good.com", null, "@evil.com"),
949+
Arguments.of("http://[email protected]", "good.com", "@evil.com", null)
950+
);
951+
}
952+
953+
@ParameterizedTest
954+
@MethodSource("authoritiesNoPath")
955+
public void testAuthorityNoPath(String uri, String authority, String query, String fragment)
956+
{
957+
HttpURI httpURI = new HttpURI(uri);
958+
assertThat(httpURI.getAuthority(), is(authority));
959+
assertThat(httpURI.getPath(), is(""));
960+
assertThat(httpURI.getQuery(), is(query));
961+
assertThat(httpURI.getFragment(), is(fragment));
962+
}
963+
964+
public static Stream<Arguments> connectURIs()
965+
{
966+
return Stream.of(
967+
Arguments.of("localhost:8080"),
968+
Arguments.of("127.0.0.1:8080"),
969+
Arguments.of("[::1]:8080")
970+
);
971+
}
972+
973+
@ParameterizedTest
974+
@MethodSource("connectURIs")
975+
public void testConnect(String authority)
976+
{
977+
HttpURI httpURI = new HttpURI();
978+
httpURI.parseRequestTarget(HttpMethod.CONNECT.asString(), authority);
979+
assertThat(httpURI.getAuthority(), is(authority));
980+
}
905981
}

‎jetty-server/src/test/java/org/eclipse/jetty/server/handler/NcsaRequestLogTest.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -358,9 +358,9 @@ public void testUseragentWithout(String logType) throws Exception
358358
setup(logType);
359359
testHandlerServerStart();
360360

361-
_connector.getResponse("GET http://[:1]/foo HTTP/1.1\nReferer: http://other.site\n\n");
361+
_connector.getResponse("GET http://[::1]/foo HTTP/1.1\nReferer: http://other.site\n\n");
362362
String log = _entries.poll(5, TimeUnit.SECONDS);
363-
assertThat(log, containsString("GET http://[:1]/foo "));
363+
assertThat(log, containsString("GET http://[::1]/foo "));
364364
assertThat(log, containsString(" 400 50 \"http://other.site\" \"-\""));
365365
}
366366

@@ -371,9 +371,9 @@ public void testUseragentWith(String logType) throws Exception
371371
setup(logType);
372372
testHandlerServerStart();
373373

374-
_connector.getResponse("GET http://[:1]/foo HTTP/1.1\nReferer: http://other.site\nUser-Agent: Mozilla/5.0 (test)\n\n");
374+
_connector.getResponse("GET http://[::1]/foo HTTP/1.1\nReferer: http://other.site\nUser-Agent: Mozilla/5.0 (test)\n\n");
375375
String log = _entries.poll(5, TimeUnit.SECONDS);
376-
assertThat(log, containsString("GET http://[:1]/foo "));
376+
assertThat(log, containsString("GET http://[::1]/foo "));
377377
assertThat(log, containsString(" 400 50 \"http://other.site\" \"Mozilla/5.0 (test)\""));
378378
}
379379

‎jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
package org.eclipse.jetty.util;
2020

21+
import java.net.InetAddress;
2122
import java.net.URI;
2223
import java.net.URISyntaxException;
2324
import java.nio.charset.Charset;
@@ -1345,6 +1346,74 @@ public static URI addPath(URI uri, String path)
13451346
return URI.create(buf.toString());
13461347
}
13471348

1349+
private static boolean isHexDigit(char c)
1350+
{
1351+
return (((c >= 'a') && (c <= 'f')) || // ALPHA (lower)
1352+
((c >= 'A') && (c <= 'F')) || // ALPHA (upper)
1353+
((c >= '0') && (c <= '9')));
1354+
}
1355+
1356+
/**
1357+
* Validate an IPv4 or IPv6 address.
1358+
* @param inetAddress the address to validate
1359+
* @throws IllegalArgumentException if the address is not valid
1360+
*/
1361+
public static void validateInetAddress(String inetAddress)
1362+
{
1363+
try
1364+
{
1365+
InetAddress ignored = InetAddress.getByName(inetAddress);
1366+
}
1367+
catch (Throwable e)
1368+
{
1369+
throw new IllegalArgumentException("Bad [IPv6] address", e);
1370+
}
1371+
}
1372+
1373+
/**
1374+
* Validate and normalize the scheme,
1375+
*
1376+
* @param scheme The scheme to normalize
1377+
* @return The normalized version of the scheme
1378+
* @throws IllegalArgumentException If the scheme is not valid
1379+
*/
1380+
public static String validateScheme(String scheme)
1381+
{
1382+
if (scheme == null || scheme.isEmpty())
1383+
throw new IllegalArgumentException("Bad scheme");
1384+
1385+
// scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
1386+
StringBuilder toLowerCase = null;
1387+
for (int i = 0; i < scheme.length(); i++)
1388+
{
1389+
char c = scheme.charAt(i);
1390+
if (c >= 'A' && c <= 'Z')
1391+
{
1392+
if (toLowerCase == null)
1393+
{
1394+
toLowerCase = new StringBuilder(scheme.length());
1395+
toLowerCase.append(scheme, 0, i);
1396+
}
1397+
toLowerCase.append(Character.toLowerCase(c));
1398+
}
1399+
else if (c >= 'a' && c <= 'z' ||
1400+
(i > 0 && (c >= '0' && c <= '9' ||
1401+
c == '.' ||
1402+
c == '+' ||
1403+
c == '-')))
1404+
{
1405+
if (toLowerCase != null)
1406+
toLowerCase.append(c);
1407+
}
1408+
else
1409+
{
1410+
throw new IllegalArgumentException("Bad scheme");
1411+
}
1412+
}
1413+
1414+
return toLowerCase == null ? scheme : toLowerCase.toString();
1415+
}
1416+
13481417
/**
13491418
* Combine two query strings into one. Each query string should not contain the beginning '?' character, but
13501419
* may contain multiple parameters separated by the '{@literal &}' character.

0 commit comments

Comments
 (0)