From 9ad74e72e66e36ec7323ca9594611d2cb8b9fc89 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 19 Jun 2017 11:16:39 -0400 Subject: [PATCH] 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. --- .../codec/http/HttpObjectAggregator.java | 6 ++- .../io/netty/handler/codec/http/HttpUtil.java | 20 ++++----- .../handler/codec/http/HttpUtilTest.java | 41 ++++++++++++++++--- 3 files changed, 49 insertions(+), 18 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java index 66801cfe72..3b3fa42207 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java @@ -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 diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java index 70db092a55..76b06fe6ed 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java @@ -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 diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java index e45e7bc142..1488fc5f0e 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpUtilTest.java @@ -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