Skip to content

Commit

Permalink
Added a UriCompliance.Violation.USER_INFO to deprecate user info in…
Browse files Browse the repository at this point in the history 
… `HttpURI` (#12012)

As per [RFC9110](https://datatracker.ietf.org/doc/html/rfc9110#name-deprecation-of-userinfo-in-) user info is deprecated in server implementations.
The new violation for USER_DATA is included by default in 12.0.x, but will be removed in 12.1.x
  • Loading branch information
@gregw
gregw authored Jul 9, 2024
1 parent 803fe5c commit c880d93
Show file tree
Hide file tree
Showing 7 changed files with 386 additions and 76 deletions.
191 changes: 138 additions & 53 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java
View file

Large diffs are not rendered by default.

10 changes: 8 additions & 2 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,12 @@ public enum Violation implements ComplianceViolation
* Allow path characters not allowed in the path portion of the URI and HTTP specs.
* <p>This would allow characters that fall outside of the {@code unreserved / pct-encoded / sub-delims / ":" / "@"} ABNF</p>
*/
ILLEGAL_PATH_CHARACTERS("https://datatracker.ietf.org/doc/html/rfc3986#section-3.3", "Illegal Path Character");
ILLEGAL_PATH_CHARACTERS("https://datatracker.ietf.org/doc/html/rfc3986#section-3.3", "Illegal Path Character"),

/**
* Allow user info in the authority portion of the URI and HTTP specs.
*/
USER_INFO("https://datatracker.ietf.org/doc/html/rfc9110#name-deprecation-of-userinfo-in-", "Deprecated User Info");

private final String _url;
private final String _description;
Expand Down Expand Up @@ -175,7 +180,8 @@ public String getDescription()
Violation.AMBIGUOUS_PATH_SEPARATOR,
Violation.AMBIGUOUS_PATH_ENCODING,
Violation.AMBIGUOUS_EMPTY_SEGMENT,
Violation.UTF16_ENCODINGS));
Violation.UTF16_ENCODINGS,
Violation.USER_INFO));

/**
* Compliance mode that allows all URI Violations, including allowing ambiguous paths in non-canonical form, and illegal characters
Expand Down
44 changes: 39 additions & 5 deletions jetty-core/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 @@ -34,6 +34,7 @@
import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.Matchers.sameInstance;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
Expand All @@ -59,6 +60,7 @@ public void testBuilder()

assertThat(uri.getScheme(), is("http"));
assertThat(uri.getUser(), is("user:password"));
assertTrue(uri.hasViolation(Violation.USER_INFO));
assertThat(uri.getHost(), is("host"));
assertThat(uri.getPort(), is(8888));
assertThat(uri.getPath(), is("/ignored/../p%61th;ignored/info;param"));
Expand All @@ -81,6 +83,7 @@ public void testBuilder()

assertThat(uri.getScheme(), is("https"));
assertThat(uri.getUser(), nullValue());
assertFalse(uri.hasViolation(Violation.USER_INFO));
assertThat(uri.getHost(), is("[::1]"));
assertThat(uri.getPort(), is(8080));
assertThat(uri.getPath(), is("/some%20encoded/evening;id=12345"));
Expand All @@ -98,6 +101,7 @@ public void testExample()

assertThat(uri.getScheme(), is("http"));
assertThat(uri.getUser(), is("user:password"));
assertTrue(uri.hasViolation(Violation.USER_INFO));
assertThat(uri.getHost(), is("host"));
assertThat(uri.getPort(), is(8888));
assertThat(uri.getPath(), is("/ignored/../p%61th;ignored/info;param"));
Expand Down Expand Up @@ -155,11 +159,8 @@ public void testParse()
assertThat(uri.getHost(), is("foo"));
assertThat(uri.getPath(), is("/bar"));

// We do allow nulls if not encoded. This can be used for testing 2nd line of defence.
builder.uri("http://fo\000/bar");
uri = builder.asImmutable();
assertThat(uri.getHost(), is("fo\000"));
assertThat(uri.getPath(), is("/bar"));
// We do not allow nulls if not encoded.
assertThrows(IllegalArgumentException.class, () -> builder.uri("http://fo\000/bar").asImmutable());
}

@Test
Expand Down Expand Up @@ -327,6 +328,7 @@ public void testBasicAuthCredentials()
assertEquals("http://user:[email protected]:8888/blah", uri.toString());
assertEquals(uri.getAuthority(), "example.com:8888");
assertEquals(uri.getUser(), "user:password");
assertTrue(uri.hasViolation(Violation.USER_INFO));
}

@Test
Expand Down Expand Up @@ -1198,4 +1200,36 @@ public void testFromBad(String scheme, String server, int port, String expectedS
HttpURI httpURI = HttpURI.from(scheme, server, port, null);
assertThat(httpURI.asString(), is(expectedStr));
}

public static Stream<String> badAuthorities()
{
return Stream.of(
"http://#host/path",
"https:// host/path",
"https://h st/path",
"https://h\000st/path",
"https://h%GGst/path",
"https://host%/path",
"https://host%0/path",
"https://host%u001f/path",
"https://host%:8080/path",
"https://host%0:8080/path",
"https://user%@host/path",
"https://user%0@host/path",
"https://host:notport/path",
"https://user@host:notport/path",
"https://user:password@host:notport/path",
"https://user @host.com/",
"https://user#@host.com/",
"https://[notIpv6]/",
"https://bad[0::1::2::3::4]/"
);
}

@ParameterizedTest
@MethodSource("badAuthorities")
public void testBadAuthority(String uri)
{
assertThrows(IllegalArgumentException.class, () -> HttpURI.from(uri));
}
}
15 changes: 15 additions & 0 deletions jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ public class HttpConfiguration implements Dumpable
private long _minResponseDataRate;
private HttpCompliance _httpCompliance = HttpCompliance.RFC7230;
private UriCompliance _uriCompliance = UriCompliance.DEFAULT;
private UriCompliance _redirectUriCompliance = null; // TODO default to UriCompliance.DEFAULT in 12.1 ?;
private CookieCompliance _requestCookieCompliance = CookieCompliance.RFC6265;
private CookieCompliance _responseCookieCompliance = CookieCompliance.RFC6265;
private MultiPartCompliance _multiPartCompliance = MultiPartCompliance.RFC7578;
Expand Down Expand Up @@ -159,6 +160,7 @@ public HttpConfiguration(HttpConfiguration config)
_notifyRemoteAsyncErrors = config._notifyRemoteAsyncErrors;
_relativeRedirectAllowed = config._relativeRedirectAllowed;
_uriCompliance = config._uriCompliance;
_redirectUriCompliance = config._redirectUriCompliance;
_serverAuthority = config._serverAuthority;
_localAddress = config._localAddress;
}
Expand Down Expand Up @@ -598,11 +600,24 @@ public UriCompliance getUriCompliance()
return _uriCompliance;
}

public UriCompliance getRedirectUriCompliance()
{
return _redirectUriCompliance;
}

public void setUriCompliance(UriCompliance uriCompliance)
{
_uriCompliance = uriCompliance;
}

/**
* @param uriCompliance The {@link UriCompliance} to apply in {@link Response#toRedirectURI(Request, String)} or {@code null}.
*/
public void setRedirectUriCompliance(UriCompliance uriCompliance)
{
_redirectUriCompliance = uriCompliance;
}

/**
* @return The CookieCompliance used for parsing request {@code Cookie} headers.
* @see #getResponseCookieCompliance()
Expand Down
49 changes: 33 additions & 16 deletions jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.http.Trailers;
import org.eclipse.jetty.http.UriCompliance;
import org.eclipse.jetty.io.ByteBufferPool;
import org.eclipse.jetty.io.Content;
import org.eclipse.jetty.io.QuietException;
Expand Down Expand Up @@ -301,25 +302,32 @@ static void sendRedirect(Request request, Response response, Callback callback,
return;
}

if (consumeAvailable)
try
{
while (true)
if (consumeAvailable)
{
Content.Chunk chunk = response.getRequest().read();
if (chunk == null)
while (true)
{
response.getHeaders().put(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE);
break;
Content.Chunk chunk = response.getRequest().read();
if (chunk == null)
{
response.getHeaders().put(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE);
break;
}
chunk.release();
if (chunk.isLast())
break;
}
chunk.release();
if (chunk.isLast())
break;
}
}

response.getHeaders().put(HttpHeader.LOCATION, toRedirectURI(request, location));
response.setStatus(code);
response.write(true, null, callback);
response.getHeaders().put(HttpHeader.LOCATION, toRedirectURI(request, location));
response.setStatus(code);
response.write(true, null, callback);
}
catch (Throwable failure)
{
callback.failed(failure);
}
}

/**
Expand All @@ -333,6 +341,8 @@ static void sendRedirect(Request request, Response response, Callback callback,
*/
static String toRedirectURI(Request request, String location)
{
HttpConfiguration httpConfiguration = request.getConnectionMetaData().getHttpConfiguration();

// is the URI absolute already?
if (!URIUtil.hasScheme(location))
{
Expand All @@ -353,12 +363,19 @@ static String toRedirectURI(Request request, String location)
throw new IllegalStateException("redirect path cannot be above root");

// if relative redirects are not allowed?
if (!request.getConnectionMetaData().getHttpConfiguration().isRelativeRedirectAllowed())
{
if (!httpConfiguration.isRelativeRedirectAllowed())
// make the location an absolute URI
location = URIUtil.newURI(uri.getScheme(), Request.getServerName(request), Request.getServerPort(request), location, null);
}
}

UriCompliance redirectCompliance = httpConfiguration.getRedirectUriCompliance();
if (redirectCompliance != null)
{
String violations = UriCompliance.checkUriCompliance(redirectCompliance, HttpURI.from(location), null);
if (StringUtil.isNotBlank(violations))
throw new IllegalArgumentException(violations);
}

return location;
}

Expand Down
71 changes: 71 additions & 0 deletions jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;

import org.eclipse.jetty.http.HttpCompliance;
import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpStatus;
Expand Down Expand Up @@ -100,6 +101,76 @@ public void testEncodedPath() throws Exception
assertEquals(HttpStatus.BAD_REQUEST_400, response.getStatus());
}

public static Stream<Arguments> getUriTests()
{
return Stream.of(
Arguments.of(UriCompliance.DEFAULT, "/", 200, "local"),
Arguments.of(UriCompliance.DEFAULT, "https://local/", 200, "local"),
Arguments.of(UriCompliance.DEFAULT, "https://other/", 400, "Authority!=Host"),
Arguments.of(UriCompliance.UNSAFE, "https://other/", 200, "other"),
Arguments.of(UriCompliance.DEFAULT, "https://user@local/", 400, "Deprecated User Info"),
Arguments.of(UriCompliance.LEGACY, "https://user@local/", 200, "local"),
Arguments.of(UriCompliance.LEGACY, "https://user@local:port/", 400, "Bad Request"),
Arguments.of(UriCompliance.LEGACY, "https://user@local:8080/", 400, "Authority!=Host"),
Arguments.of(UriCompliance.UNSAFE, "https://user@local:8080/", 200, "local:8080"),
Arguments.of(UriCompliance.DEFAULT, "https://user:password@local/", 400, "Deprecated User Info"),
Arguments.of(UriCompliance.LEGACY, "https://user:password@local/", 200, "local"),
Arguments.of(UriCompliance.DEFAULT, "https://user@other/", 400, "Deprecated User Info"),
Arguments.of(UriCompliance.LEGACY, "https://user@other/", 400, "Authority!=Host"),
Arguments.of(UriCompliance.DEFAULT, "https://user:password@other/", 400, "Deprecated User Info"),
Arguments.of(UriCompliance.LEGACY, "https://user:password@other/", 400, "Authority!=Host"),
Arguments.of(UriCompliance.UNSAFE, "https://user:password@other/", 200, "other"),
Arguments.of(UriCompliance.DEFAULT, "/%2F/", 400, "Ambiguous URI path separator"),
Arguments.of(UriCompliance.UNSAFE, "/%2F/", 200, "local")
);
}

@ParameterizedTest
@MethodSource("getUriTests")
public void testGETUris(UriCompliance compliance, String uri, int status, String content) throws Exception
{
server.stop();
for (Connector connector: server.getConnectors())
{
HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class);
if (httpConnectionFactory != null)
{
HttpConfiguration httpConfiguration = httpConnectionFactory.getHttpConfiguration();
httpConfiguration.setUriCompliance(compliance);
if (compliance == UriCompliance.UNSAFE)
httpConfiguration.setHttpCompliance(HttpCompliance.RFC2616_LEGACY);
}
}

server.setHandler(new Handler.Abstract()
{
@Override
public boolean handle(Request request, Response response, Callback callback)
{
String msg = String.format("authority=\"%s\"", request.getHttpURI().getAuthority());
response.getHeaders().put(HttpHeader.CONTENT_TYPE, "text/plain;charset=utf-8");
Content.Sink.write(response, true, msg, callback);
return true;
}
});
server.start();
String request = """
GET %s HTTP/1.1\r
Host: local\r
Connection: close\r
\r
""".formatted(uri);
HttpTester.Response response = HttpTester.parseResponse(connector.getResponse(request));
assertThat(response.getStatus(), is(status));
if (content != null)
{
if (status == 200)
assertThat(response.getContent(), is("authority=\"%s\"".formatted(content)));
else
assertThat(response.getContent(), containsString(content));
}
}

@Test
public void testAmbiguousPathSep() throws Exception
{
Expand Down
Loading

0 comments on commit c880d93

Please sign in to comment.