Parse headers with no whitespaces (#856)

Motivation: The whitespace between the `:` and a header value is optional [1] but we consider it as required [2] hence skip a character after the `:`. As the result, header value may miss the first character. [1] https://tools.ietf.org/html/rfc7230#section-3.2 [2] `header-field = field-name ":" OWS field-value OWS` Modifications: - Do not skip a character after `:`; - Add more tests for headers with no whitespace and for mixed whitespaces; Result: Headers parser does not skip the first value character when there is no whitespace after `:`.
apple · Nov 8, 2019 · e3528e6 · e3528e6
1 parent 1364258
commit e3528e6
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 3 deletions.
diff --git a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpObjectDecoder.java b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpObjectDecoder.java
@@ -560,15 +560,15 @@ private void parseHeaderLine(HttpHeaders headers, ByteBuf buffer, final int lfIn
         // We assume the allocator will not leak memory, and so we retain + slice to avoid copying data.
         CharSequence name = newAsciiString(newBufferFrom(buffer.retainedSlice(headerStart, headerEnd - headerStart)));
         if (buffer.getByte(headerEnd) != COLON_BYTE) {
-            valueStart = buffer.forEachByte(headerEnd + 1, nonControlIndex - headerEnd, FIND_COLON);
+            valueStart = buffer.forEachByte(headerEnd + 1, nonControlIndex - headerEnd, FIND_COLON) + 1;
             if (valueStart < 0) {
                 throw new IllegalArgumentException("unable to find colon");
             }
         }
-        if (nonControlIndex <= valueStart) {
+        if (nonControlIndex < valueStart) {
             headers.add(name, emptyAsciiString());
         } else {
-            valueStart = buffer.forEachByte(valueStart + 1, nonControlIndex - valueStart, FIND_NON_LINEAR_WHITESPACE);
+            valueStart = buffer.forEachByte(valueStart, nonControlIndex - valueStart + 1, FIND_NON_LINEAR_WHITESPACE);
             // Find End Of String
             int valueEnd;
             if (valueStart < 0 || (valueEnd = buffer.forEachByteDesc(valueStart, lfIndex - valueStart - 1,

diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpRequestDecoderTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpRequestDecoderTest.java
@@ -111,6 +111,42 @@ public void contentLengthNoTrailersHeaderWhiteSpace() {
         assertFalse(channel.finishAndReleaseAll());
     }
 
+    @Test
+    public void contentLengthNoTrailersHeaderNoWhiteSpace() {
+        EmbeddedChannel channel = newEmbeddedChannel();
+        byte[] content = new byte[128];
+        ThreadLocalRandom.current().nextBytes(content);
+        byte[] beforeContentBytes = ("GET /some/path?foo=bar&baz=yyy HTTP/1.1" + "\r\n" +
+                "Connection:keep-alive" + "\r\n" +
+                "User-Agent:unit-test" + "\r\n" +
+                "SingleCharacterNoWhiteSpace:a" + "\r\n" +
+                "Content-Length:" + content.length + "\r\n" + "\r\n").getBytes(US_ASCII);
+        assertTrue(channel.writeInbound(wrappedBuffer(beforeContentBytes)));
+        assertTrue(channel.writeInbound(wrappedBuffer(content)));
+
+        validateHttpRequest(channel, content.length);
+        assertFalse(channel.finishAndReleaseAll());
+    }
+
+    @Test
+    public void contentLengthNoTrailersHeaderMixedWhiteSpace() {
+        EmbeddedChannel channel = newEmbeddedChannel();
+        byte[] content = new byte[128];
+        ThreadLocalRandom.current().nextBytes(content);
+        byte[] beforeContentBytes = ("GET /some/path?foo=bar&baz=yyy HTTP/1.1" + "\r\n" +
+                "Connection   :keep-alive" + "\r\n" +
+                "   User-Agent   :unit-test" + "\r\n" +
+                "Empty:" + "\r\n" +
+                "EmptyWhitespace:   " + "\r\n" +
+                "SingleCharacterNoWhiteSpace: a" + "\r\n" +
+                "Content-Length:   " + content.length + "   " + "\r\n" + "\r\n").getBytes(US_ASCII);
+        assertTrue(channel.writeInbound(wrappedBuffer(beforeContentBytes)));
+        assertTrue(channel.writeInbound(wrappedBuffer(content)));
+
+        validateHttpRequest(channel, content.length);
+        assertFalse(channel.finishAndReleaseAll());
+    }
+
     @Test
     public void chunkedNoTrailers() {
         EmbeddedChannel channel = newEmbeddedChannel();
@@ -425,6 +461,13 @@ private static void validateHttpRequest(EmbeddedChannel channel, int expectedCon
     private static void assertStandardHeaders(HttpHeaders headers) {
         assertSingleHeaderValue(headers, "connecTion", KEEP_ALIVE);
         assertSingleHeaderValue(headers, USER_AGENT, "unit-test");
+        if (headers.contains("Empty")) {
+            assertSingleHeaderValue(headers, "Empty", "");
+            assertSingleHeaderValue(headers, "EmptyWhitespace", "");
+        }
+        if (headers.contains("SingleCharacterNoWhiteSpace")) {
+            assertSingleHeaderValue(headers, "SingleCharacterNoWhiteSpace", "a");
+        }
     }
 
     static void assertSingleHeaderValue(HttpHeaders headers, CharSequence name, CharSequence expectedValue) {

diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpResponseDecoderTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpResponseDecoderTest.java
@@ -118,6 +118,42 @@ public void contentLengthNoTrailersHeaderWhiteSpace() {
         assertFalse(channel.finishAndReleaseAll());
     }
 
+    @Test
+    public void contentLengthNoTrailersHeaderNoWhiteSpace() {
+        EmbeddedChannel channel = newEmbeddedChannel();
+        byte[] content = new byte[128];
+        ThreadLocalRandom.current().nextBytes(content);
+        byte[] beforeContentBytes = ("HTTP/1.1 200 OK" + "\r\n" +
+                "Connection:keep-alive" + "\r\n" +
+                "Server:unit-test" + "\r\n" +
+                "SingleCharacterNoWhiteSpace:a" + "\r\n" +
+                "Content-Length:" + content.length + "\r\n" + "\r\n").getBytes(US_ASCII);
+        assertTrue(channel.writeInbound(wrappedBuffer(beforeContentBytes)));
+        assertTrue(channel.writeInbound(wrappedBuffer(content)));
+
+        validateHttpResponse(channel, content.length);
+        assertFalse(channel.finishAndReleaseAll());
+    }
+
+    @Test
+    public void contentLengthNoTrailersHeaderMixedWhiteSpace() {
+        EmbeddedChannel channel = newEmbeddedChannel();
+        byte[] content = new byte[128];
+        ThreadLocalRandom.current().nextBytes(content);
+        byte[] beforeContentBytes = ("HTTP/1.1 200 OK" + "\r\n" +
+                "Connection   :keep-alive" + "\r\n" +
+                "   Server   :unit-test" + "\r\n" +
+                "Empty:" + "\r\n" +
+                "EmptyWhitespace:   " + "\r\n" +
+                "SingleCharacterNoWhiteSpace: a" + "\r\n" +
+                "Content-Length:   " + content.length + "   " + "\r\n" + "\r\n").getBytes(US_ASCII);
+        assertTrue(channel.writeInbound(wrappedBuffer(beforeContentBytes)));
+        assertTrue(channel.writeInbound(wrappedBuffer(content)));
+
+        validateHttpResponse(channel, content.length);
+        assertFalse(channel.finishAndReleaseAll());
+    }
+
     @Test
     public void chunkedNoTrailers() {
         EmbeddedChannel channel = newEmbeddedChannel();
@@ -427,6 +463,13 @@ private static void validateHttpResponse(EmbeddedChannel channel, int expectedCo
     private static void assertStandardHeaders(HttpHeaders headers) {
         assertSingleHeaderValue(headers, CONNECTION, KEEP_ALIVE);
         assertSingleHeaderValue(headers, "seRver", "unit-test");
+        if (headers.contains("Empty")) {
+            assertSingleHeaderValue(headers, "Empty", "");
+            assertSingleHeaderValue(headers, "EmptyWhitespace", "");
+        }
+        if (headers.contains("SingleCharacterNoWhiteSpace")) {
+            assertSingleHeaderValue(headers, "SingleCharacterNoWhiteSpace", "a");
+        }
     }
 
     private static EmbeddedChannel newEmbeddedChannel() {