Remove content-length header leniency
Motivation: If the content-length does not parse as a number, leniency causes this to instead be parsed as the default value. This leads to bodies being silently ignored on requests which can be incredibly dangerous. Instead, if the content-length header is invalid, an exception should be thrown for upstream handling. Modifications: This commit removes the leniency in parsing the content-length header by allowing a number format exception, if thrown, to escape from the method rather than falling back to the default value. Result: In invalid content-length header will not be silently ignored.
This commit is contained in:
parent
6cd086050f
commit
9ad74e72e6
@ -151,7 +151,11 @@ public class HttpObjectAggregator
|
||||
|
||||
@Override
|
||||
protected boolean isContentLengthInvalid(HttpMessage start, int maxContentLength) {
|
||||
return getContentLength(start, -1L) > maxContentLength;
|
||||
try {
|
||||
return getContentLength(start, -1L) > maxContentLength;
|
||||
} catch (final NumberFormatException e) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
@Override
|
||||
|
@ -172,23 +172,19 @@ public final class HttpUtil {
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns the length of the content. Please note that this value is
|
||||
* not retrieved from {@link HttpContent#content()} but from the
|
||||
* {@code "Content-Length"} header, and thus they are independent from each
|
||||
* other.
|
||||
* Returns the length of the content or the specified default value if the message does not have the {@code
|
||||
* "Content-Length" header}. Please note that this value is not retrieved from {@link HttpContent#content()} but
|
||||
* from the {@code "Content-Length"} header, and thus they are independent from each other.
|
||||
*
|
||||
* @return the content length or {@code defaultValue} if this message does
|
||||
* not have the {@code "Content-Length"} header or its value is not
|
||||
* a number
|
||||
* @param message the message
|
||||
* @param defaultValue the default value
|
||||
* @return the content length or the specified default value
|
||||
* @throws NumberFormatException if the {@code "Content-Length"} header does not parse as a long
|
||||
*/
|
||||
public static long getContentLength(HttpMessage message, long defaultValue) {
|
||||
String value = message.headers().get(HttpHeaderNames.CONTENT_LENGTH);
|
||||
if (value != null) {
|
||||
try {
|
||||
return Long.parseLong(value);
|
||||
} catch (NumberFormatException ignore) {
|
||||
return defaultValue;
|
||||
}
|
||||
return Long.parseLong(value);
|
||||
}
|
||||
|
||||
// We know the content length if it's a Web Socket message even if
|
||||
|
@ -25,10 +25,14 @@ import java.util.Collections;
|
||||
import java.util.List;
|
||||
|
||||
import static io.netty.handler.codec.http.HttpHeadersTestUtils.of;
|
||||
import static org.hamcrest.Matchers.containsString;
|
||||
import static org.hamcrest.Matchers.hasToString;
|
||||
import static org.junit.Assert.assertEquals;
|
||||
import static org.junit.Assert.assertFalse;
|
||||
import static org.junit.Assert.assertNull;
|
||||
import static org.junit.Assert.assertThat;
|
||||
import static org.junit.Assert.assertTrue;
|
||||
import static org.junit.Assert.fail;
|
||||
|
||||
public class HttpUtilTest {
|
||||
|
||||
@ -130,12 +134,39 @@ public class HttpUtilTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testGetContentLengthDefaultValue() {
|
||||
HttpMessage message = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK);
|
||||
assertNull(message.headers().get(HttpHeaderNames.CONTENT_LENGTH));
|
||||
public void testGetContentLengthThrowsNumberFormatException() {
|
||||
final HttpMessage message = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK);
|
||||
message.headers().set(HttpHeaderNames.CONTENT_LENGTH, "bar");
|
||||
assertEquals("bar", message.headers().get(HttpHeaderNames.CONTENT_LENGTH));
|
||||
assertEquals(1L, HttpUtil.getContentLength(message, 1L));
|
||||
try {
|
||||
HttpUtil.getContentLength(message);
|
||||
fail();
|
||||
} catch (final NumberFormatException e) {
|
||||
// a number format exception is expected here
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testGetContentLengthIntDefaultValueThrowsNumberFormatException() {
|
||||
final HttpMessage message = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK);
|
||||
message.headers().set(HttpHeaderNames.CONTENT_LENGTH, "bar");
|
||||
try {
|
||||
HttpUtil.getContentLength(message, 1);
|
||||
fail();
|
||||
} catch (final NumberFormatException e) {
|
||||
// a number format exception is expected here
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testGetContentLengthLongDefaultValueThrowsNumberFormatException() {
|
||||
final HttpMessage message = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK);
|
||||
message.headers().set(HttpHeaderNames.CONTENT_LENGTH, "bar");
|
||||
try {
|
||||
HttpUtil.getContentLength(message, 1L);
|
||||
fail();
|
||||
} catch (final NumberFormatException e) {
|
||||
// a number format exception is expected here
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
|
Loading…
Reference in New Issue
Block a user