Skip to content

Commit

Permalink
Check UriCompliance.Violation.ALLOW_BAD_UTF8 in query parameter extra…
Browse files Browse the repository at this point in the history 
…ction (#12671)

* Check UriCompliance.Violation.ALLOW_BAD_UTF8 in EE9 query parameter extraction.
* Check UriCompliance.Violation.ALLOW_BAD_UTF8 in core query parameter extraction.
* Added compliance violation listeners
  • Loading branch information
@gregw
gregw authored Jan 6, 2025
1 parent 81ccd4b commit 2d9dab6
Show file tree
Hide file tree
Showing 5 changed files with 189 additions and 18 deletions.
30 changes: 21 additions & 9 deletions jetty-core/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 @@ -20,6 +20,7 @@
import java.nio.ByteBuffer;
import java.nio.charset.Charset;
import java.nio.charset.IllegalCharsetNameException;
import java.nio.charset.StandardCharsets;
import java.nio.charset.UnsupportedCharsetException;
import java.nio.file.Path;
import java.security.Principal;
Expand All @@ -46,6 +47,7 @@
import org.eclipse.jetty.http.MultiPartCompliance;
import org.eclipse.jetty.http.MultiPartConfig;
import org.eclipse.jetty.http.Trailers;
import org.eclipse.jetty.http.UriCompliance;
import org.eclipse.jetty.io.Content;
import org.eclipse.jetty.io.EndPoint;
import org.eclipse.jetty.server.internal.CompletionStreamWrapper;
Expand Down Expand Up @@ -552,21 +554,31 @@ static Charset getCharset(Request request) throws IllegalCharsetNameException, U

static Fields extractQueryParameters(Request request)
{
String query = request.getHttpURI().getQuery();
if (StringUtil.isBlank(query))
return Fields.EMPTY;
Fields fields = new Fields(true);
if (StringUtil.isNotBlank(query))
UrlEncoded.decodeUtf8To(query, fields);
return fields;
return extractQueryParameters(request, null);
}

static Fields extractQueryParameters(Request request, Charset charset)
{
Fields fields = new Fields(true);
String query = request.getHttpURI().getQuery();
if (StringUtil.isNotBlank(query))
if (StringUtil.isBlank(query))
return Fields.EMPTY;
Fields fields = new Fields(true);

if (charset == null || StandardCharsets.UTF_8.equals(charset))
{
UriCompliance uriCompliance = request.getConnectionMetaData().getHttpConfiguration().getUriCompliance();
boolean allowBadUtf8 = uriCompliance.allows(UriCompliance.Violation.BAD_UTF8_ENCODING);
if (!UrlEncoded.decodeUtf8To(query, 0, query.length(), fields::add, allowBadUtf8))
{
HttpChannel httpChannel = HttpChannel.from(request);
if (httpChannel != null && httpChannel.getComplianceViolationListener() != null)
httpChannel.getComplianceViolationListener().onComplianceViolation(new ComplianceViolation.Event(uriCompliance, UriCompliance.Violation.BAD_UTF8_ENCODING, "query=" + query));
}
}
else
{
UrlEncoded.decodeTo(query, fields::add, charset);
}
return fields;
}

Expand Down
76 changes: 76 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 @@ -19,9 +19,11 @@
import java.util.EnumSet;
import java.util.List;
import java.util.Locale;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;

import org.eclipse.jetty.http.ComplianceViolation;
import org.eclipse.jetty.http.HttpCompliance;
import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpHeader;
Expand All @@ -33,13 +35,15 @@
import org.eclipse.jetty.server.handler.ContextHandler;
import org.eclipse.jetty.server.handler.DumpHandler;
import org.eclipse.jetty.util.Callback;
import org.eclipse.jetty.util.Fields;
import org.eclipse.jetty.util.component.LifeCycle;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.junit.jupiter.params.provider.ValueSource;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
Expand All @@ -49,6 +53,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;

public class RequestTest
{
Expand Down Expand Up @@ -541,4 +546,75 @@ private static void checkCookieResult(String containedCookie, String[] notContai
}
}
}

/**
* Test to ensure that response.write() will add Content-Length on HTTP/1.1 responses.
*/
@ParameterizedTest
@ValueSource(strings = { "true", "false"})
public void testBadUtf8Query(boolean allowBadUtf8) throws Exception
{
server.stop();
List<ComplianceViolation.Event> events = new CopyOnWriteArrayList<>();
ComplianceViolation.Listener listener = new ComplianceViolation.Listener()
{
@Override
public void onComplianceViolation(ComplianceViolation.Event event)
{
events.add(event);
}
};

if (allowBadUtf8)
{
for (Connector connector : server.getConnectors())
{
HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class);
if (httpConnectionFactory != null)
{
HttpConfiguration httpConfiguration = httpConnectionFactory.getHttpConfiguration();
httpConfiguration.setUriCompliance(UriCompliance.DEFAULT.with("test", UriCompliance.Violation.BAD_UTF8_ENCODING));
httpConfiguration.addComplianceViolationListener(listener);
}
}
}

server.setHandler(new Handler.Abstract.NonBlocking()
{
@Override
public boolean handle(Request request, Response response, Callback callback)
{
if (allowBadUtf8)
{
Fields fields = Request.extractQueryParameters(request);
assertThat(fields.getValue("param"), is("bad_�"));
assertThat(fields.getValue("other"), is("short�"));
}
else
{
assertThrows(IllegalArgumentException.class, () -> Request.extractQueryParameters(request));
}
callback.succeeded();
return true;
}
});
server.start();

String request = """
GET /foo?param=bad_%e0%b8&other=short%a HTTP/1.1\r
Host: local\r
\r
""";
HttpTester.Response response = HttpTester.parseResponse(connector.getResponse(request));
assertEquals(HttpStatus.OK_200, response.getStatus());
if (allowBadUtf8)
{
assertThat(events.size(), is(1));
assertThat(events.get(0).violation(), is(UriCompliance.Violation.BAD_UTF8_ENCODING));
}
else
{
assertThat(events.size(), is(0));
}
}
}
55 changes: 47 additions & 8 deletions jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/UrlEncoded.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@
import java.nio.charset.StandardCharsets;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.BiConsumer;
import java.util.function.Supplier;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -223,7 +225,7 @@ public static void decodeTo(String content, BiConsumer<String, String> adder, Ch

if (StandardCharsets.UTF_8.equals(charset))
{
decodeUtf8To(content, 0, content.length(), adder);
decodeUtf8To(content, 0, content.length(), adder, false);
return;
}

Expand Down Expand Up @@ -318,7 +320,7 @@ public static void decodeUtf8To(String query, Fields fields)
@Deprecated
public static void decodeUtf8To(String query, int offset, int length, MultiMap<String> map)
{
decodeUtf8To(query, offset, length, map::add);
decodeUtf8To(query, offset, length, map::add, false);
}

/**
Expand All @@ -331,28 +333,58 @@ public static void decodeUtf8To(String query, int offset, int length, MultiMap<S
*/
public static void decodeUtf8To(String uri, int offset, int length, Fields fields)
{
decodeUtf8To(uri, offset, length, fields::add);
decodeUtf8To(uri, offset, length, fields::add, false);
}

private static void decodeUtf8To(String query, int offset, int length, BiConsumer<String, String> adder)
/**
* <p>Decodes URI query parameters as UTF8 string</p>
*
* @param query the URI string.
* @param offset the offset at which query parameters start.
* @param length the length of query parameters string to parse.
* @param adder the method to call to add decoded parameters.
* @param allowBadUtf8 if {@code true} allow bad UTF-8 and insert the replacement character.
* @return {@code true} if the string was decoded without any bad UTF-8
* @throws org.eclipse.jetty.util.Utf8StringBuilder.Utf8IllegalArgumentException if there is illegal UTF-8 and `allowsBadUtf8` is {@code false}
*/
public static boolean decodeUtf8To(String query, int offset, int length, BiConsumer<String, String> adder, boolean allowBadUtf8)
throws Utf8StringBuilder.Utf8IllegalArgumentException
{
Utf8StringBuilder buffer = new Utf8StringBuilder();
String key = null;
String value;

AtomicBoolean badUtf8;
Supplier<Utf8StringBuilder.Utf8IllegalArgumentException> onCodingError;

if (allowBadUtf8)
{
badUtf8 = new AtomicBoolean(false);
onCodingError = () ->
{
badUtf8.set(true);
return null;
};
}
else
{
badUtf8 = null;
onCodingError = Utf8StringBuilder.Utf8IllegalArgumentException::new;
}

int end = offset + length;
for (int i = offset; i < end; i++)
{
char c = query.charAt(i);
switch (c)
{
case '&':
value = buffer.takeCompleteString(Utf8StringBuilder.Utf8IllegalArgumentException::new);
value = buffer.takeCompleteString(onCodingError);
if (key != null)
{
adder.accept(key, value);
}
else if (value != null && value.length() > 0)
else if (value != null && !value.isEmpty())
{
adder.accept(value, "");
}
Expand All @@ -365,7 +397,7 @@ else if (value != null && value.length() > 0)
buffer.append(c);
break;
}
key = buffer.takeCompleteString(Utf8StringBuilder.Utf8IllegalArgumentException::new);
key = buffer.takeCompleteString(onCodingError);
break;

case '+':
Expand All @@ -379,6 +411,11 @@ else if (value != null && value.length() > 0)
char lo = query.charAt(++i);
buffer.append(decodeHexByte(hi, lo));
}
else if (allowBadUtf8)
{
buffer.append(Utf8StringBuilder.REPLACEMENT);
i = end;
}
else
{
throw new Utf8StringBuilder.Utf8IllegalArgumentException();
Expand All @@ -393,13 +430,15 @@ else if (value != null && value.length() > 0)

if (key != null)
{
value = buffer.takeCompleteString(Utf8StringBuilder.Utf8IllegalArgumentException::new);
value = buffer.takeCompleteString(onCodingError);
adder.accept(key, value);
}
else if (buffer.length() > 0)
{
adder.accept(buffer.toCompleteString(), "");
}

return badUtf8 == null || !badUtf8.get();
}

/**
Expand Down
18 changes: 17 additions & 1 deletion jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
import org.eclipse.jetty.http.MimeTypes;
import org.eclipse.jetty.http.MultiPartCompliance;
import org.eclipse.jetty.http.SetCookieParser;
import org.eclipse.jetty.http.UriCompliance;
import org.eclipse.jetty.io.Connection;
import org.eclipse.jetty.io.RuntimeIOException;
import org.eclipse.jetty.security.UserIdentity;
Expand Down Expand Up @@ -421,7 +422,22 @@ private void extractQueryParameters()
try
{
_queryParameters = new Fields(true);
UrlEncoded.decodeTo(query, _queryParameters::add, _queryEncoding);

if (StandardCharsets.UTF_8.equals(_queryEncoding) || _queryEncoding == null && UrlEncoded.ENCODING.equals(StandardCharsets.UTF_8))
{
UriCompliance uriCompliance = getHttpChannel().getHttpConfiguration().getUriCompliance();
boolean allowBadUtf8 = uriCompliance.allows(UriCompliance.Violation.BAD_UTF8_ENCODING);
if (!UrlEncoded.decodeUtf8To(query, 0, query.length(), _queryParameters::add, allowBadUtf8))
{
ComplianceViolation.Listener complianceViolationListener = getComplianceViolationListener();
if (complianceViolationListener != null)
complianceViolationListener.onComplianceViolation(new ComplianceViolation.Event(uriCompliance, UriCompliance.Violation.BAD_UTF8_ENCODING, "query=" + query));
}
}
else
{
UrlEncoded.decodeTo(query, _queryParameters::add, _queryEncoding);
}
}
catch (IllegalStateException | IllegalArgumentException e)
{
Expand Down
28 changes: 28 additions & 0 deletions jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,34 @@ public void testRequestCharacterEncoding() throws Exception
assertEquals("utf-8", result.get());
}

@Test
public void testBadUtf8Query() throws Exception
{
_server.stop();
_connector.getConnectionFactory(HttpConnectionFactory.class)
.getHttpConfiguration().setUriCompliance(UriCompliance.DEFAULT.with("test", UriCompliance.Violation.BAD_UTF8_ENCODING));
_server.start();

_handler._checker = (request, response) ->
{
String param = request.getParameter("param");
String other = request.getParameter("other");
return param != null && param.equals("bad_�") && other != null && other.equals("short�");
};

//Send a request with query string with illegal hex code to cause
//an exception parsing the params
String request = """
GET /?param=bad_%e0%b8&other=short%a HTTP/1.1\r
Host: whatever\r
Connection: close
""";

String responses = _connector.getResponse(request);
assertThat(responses, startsWith("HTTP/1.1 200"));
}

@Test
public void testParamExtraction() throws Exception
{
Expand Down

0 comments on commit 2d9dab6

Please sign in to comment.