From 890c0b26cb8e01fd061a80415330a2bf72a82e17 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Fri, 18 Oct 2019 14:05:15 -0700 Subject: [PATCH] Fixes #4203 and #4204 - Transfer-Encoding + Content-Length behaviors (#4205) * Issue #4203 - Updating test to verify report on Transfer-Encoding Signed-off-by: Joakim Erdfelt * Fixes #4203 - Transfer-Encoding + Content-Length is 400 Bad Request + Fixing validation to not be header order dependent. Signed-off-by: Joakim Erdfelt * Issue #4203 - Fixing hasTransferEncoding reset and testcase assumption Signed-off-by: Joakim Erdfelt * Issue #4204 - Transfer-Encoding RFC7230 behaviors + More test cases and implementation. Signed-off-by: Joakim Erdfelt * Issue #4204 - Transfer-Encoding RFC7230 behaviors + Adjusting HttpParser to handle the case where we have multiple Transfer-Encoding headers and none declare the 'chunked' token. Signed-off-by: Joakim Erdfelt * Issue #4204 - Transfer-Encoding RFC7230 behaviors + Making changes from PR review Signed-off-by: Joakim Erdfelt * Issue #4203 Transfer Encoding request with TE and no chunking is a Bad Request Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/http/HttpParser.java | 48 ++++- .../eclipse/jetty/http/HttpParserTest.java | 2 +- .../jetty/server/HttpConnectionTest.java | 190 +++++++++++++++--- .../jetty/server/PartialRFC2616Test.java | 9 +- .../jetty/test/rfcs/RFC2616BaseTest.java | 13 +- 5 files changed, 208 insertions(+), 54 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index 435579c9df0..4435610086b 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -169,6 +169,7 @@ public class HttpParser private Utf8StringBuilder _uri = new Utf8StringBuilder(INITIAL_URI_LENGTH); // Tune? private EndOfContent _endOfContent; private boolean _hasContentLength; + private boolean _hasTransferEncoding; private long _contentLength = -1; private long _contentPosition; private int _chunkLength; @@ -955,6 +956,9 @@ public class HttpParser switch (_header) { case CONTENT_LENGTH: + if (_hasTransferEncoding && complianceViolation(TRANSFER_ENCODING_WITH_CONTENT_LENGTH)) + throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Transfer-Encoding and Content-Length"); + if (_hasContentLength) { if (complianceViolation(MULTIPLE_CONTENT_LENGTHS)) @@ -964,9 +968,6 @@ public class HttpParser } _hasContentLength = true; - if (_endOfContent == EndOfContent.CHUNKED_CONTENT && complianceViolation(TRANSFER_ENCODING_WITH_CONTENT_LENGTH)) - throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Bad Content-Length"); - if (_endOfContent != EndOfContent.CHUNKED_CONTENT) { _contentLength = convertContentLength(_valueString); @@ -978,9 +979,15 @@ public class HttpParser break; case TRANSFER_ENCODING: + _hasTransferEncoding = true; + if (_hasContentLength && complianceViolation(TRANSFER_ENCODING_WITH_CONTENT_LENGTH)) throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Transfer-Encoding and Content-Length"); + // we encountered another Transfer-Encoding header, but chunked was already set + if (_endOfContent == EndOfContent.CHUNKED_CONTENT) + throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Bad Transfer-Encoding, chunked not last"); + if (HttpHeaderValue.CHUNKED.is(_valueString)) { _endOfContent = EndOfContent.CHUNKED_CONTENT; @@ -989,15 +996,26 @@ public class HttpParser else { List values = new QuotedCSV(_valueString).getValues(); - if (!values.isEmpty() && HttpHeaderValue.CHUNKED.is(values.get(values.size() - 1))) + int chunked = -1; + int len = values.size(); + for (int i = 0; i < len; i++) { - _endOfContent = EndOfContent.CHUNKED_CONTENT; - _contentLength = -1; + if (HttpHeaderValue.CHUNKED.is(values.get(i))) + { + if (chunked != -1) + throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Bad Transfer-Encoding, multiple chunked tokens"); + chunked = i; + // declared chunked + _endOfContent = EndOfContent.CHUNKED_CONTENT; + _contentLength = -1; + } + // we have a non-chunked token after a declared chunked token + else if (_endOfContent == EndOfContent.CHUNKED_CONTENT) + { + throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Bad Transfer-Encoding, chunked not last"); + } } - else if (values.stream().anyMatch(HttpHeaderValue.CHUNKED::is)) - throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Bad chunking"); } - break; case HOST: @@ -1139,6 +1157,17 @@ public class HttpParser return _handler.messageComplete(); } + // We found Transfer-Encoding headers, but none declared the 'chunked' token + if (_hasTransferEncoding && _endOfContent != EndOfContent.CHUNKED_CONTENT) + { + if (_responseHandler == null || _endOfContent != EndOfContent.EOF_CONTENT) + { + // Transfer-Encoding chunked not specified + // https://tools.ietf.org/html/rfc7230#section-3.3.1 + throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Bad Transfer-Encoding, chunked not last"); + } + } + // Was there a required host header? if (!_host && _version == HttpVersion.HTTP_1_1 && _requestHandler != null) { @@ -1818,6 +1847,7 @@ public class HttpParser _endOfContent = EndOfContent.UNKNOWN_CONTENT; _contentLength = -1; _hasContentLength = false; + _hasTransferEncoding = false; _contentPosition = 0; _responseStatus = 0; _contentChunk = null; diff --git a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java index f7bf640de51..3a90155b8a9 100644 --- a/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java +++ b/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java @@ -1050,7 +1050,7 @@ public class HttpParserTest assertEquals("GET", _methodOrVersion); assertEquals("/chunk", _uriOrStatus); assertEquals("HTTP/1.0", _versionOrReason); - assertThat(_bad, containsString("Bad chunking")); + assertThat(_bad, containsString("Bad Transfer-Encoding")); } @Test diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java index 408ebb1535a..715d5ede8a8 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java @@ -30,10 +30,13 @@ import java.io.IOException; import java.io.OutputStream; import java.io.PrintWriter; import java.io.StringReader; +import java.util.Arrays; import java.util.HashSet; +import java.util.List; import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import java.util.stream.Stream; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -54,6 +57,9 @@ import org.hamcrest.Matchers; 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 static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; @@ -263,43 +269,172 @@ public class HttpConnectionTest } } + static final int CHUNKED = -1; + static final int DQUOTED_CHUNKED = -2; + static final int BAD_CHUNKED = -3; + static final int UNKNOWN_TE = -4; + + public static Stream http11ContentLengthAndChunkedData() + { + return Stream.of( + Arguments.of(new int[]{CHUNKED, 8}), + Arguments.of(new int[]{8, CHUNKED}), + Arguments.of(new int[]{8, CHUNKED, 8}), + Arguments.of(new int[]{DQUOTED_CHUNKED, 8}), + Arguments.of(new int[]{8, DQUOTED_CHUNKED}), + Arguments.of(new int[]{8, DQUOTED_CHUNKED, 8}), + Arguments.of(new int[]{BAD_CHUNKED, 8}), + Arguments.of(new int[]{8, BAD_CHUNKED}), + Arguments.of(new int[]{8, BAD_CHUNKED, 8}), + Arguments.of(new int[]{UNKNOWN_TE, 8}), + Arguments.of(new int[]{8, UNKNOWN_TE}), + Arguments.of(new int[]{8, UNKNOWN_TE, 8}), + Arguments.of(new int[]{8, UNKNOWN_TE, CHUNKED, DQUOTED_CHUNKED, BAD_CHUNKED, 8}) + ); + } + /** * More then 1 Content-Length is a bad requests per HTTP rfcs. */ - @Test - public void testHttp11ContentLengthAndChunk() throws Exception + @ParameterizedTest + @MethodSource("http11ContentLengthAndChunkedData") + public void testHttp11ContentLengthAndChunk(int[] contentLengths) throws Exception { HttpParser.LOG.info("badMessage: 400 Bad messages EXPECTED..."); - int[][] contentLengths = { - {-1, 8}, - {8, -1}, - {8, -1, 8}, - }; - for (int x = 0; x < contentLengths.length; x++) + StringBuilder request = new StringBuilder(); + request.append("POST / HTTP/1.1\r\n"); + request.append("Host: local\r\n"); + for (int n = 0; n < contentLengths.length; n++) { - StringBuilder request = new StringBuilder(); - request.append("POST /?id=").append(Integer.toString(x)).append(" HTTP/1.1\r\n"); - request.append("Host: local\r\n"); - int[] clen = contentLengths[x]; - for (int n = 0; n < clen.length; n++) + switch (contentLengths[n]) { - if (clen[n] == -1) + case CHUNKED: request.append("Transfer-Encoding: chunked\r\n"); - else - request.append("Content-Length: ").append(Integer.toString(clen[n])).append("\r\n"); + break; + case DQUOTED_CHUNKED: + request.append("Transfer-Encoding: \"chunked\"\r\n"); + break; + case BAD_CHUNKED: + request.append("Transfer-Encoding: 'chunked'\r\n"); + break; + case UNKNOWN_TE: + request.append("Transfer-Encoding: bogus\r\n"); + break; + default: + request.append("Content-Length: ").append(contentLengths[n]).append("\r\n"); + break; } - request.append("Content-Type: text/plain\r\n"); - request.append("Connection: close\r\n"); - request.append("\r\n"); - request.append("8;\r\n"); // chunk header - request.append("abcdefgh"); // actual content of 8 bytes - request.append("\r\n0;\r\n"); // last chunk - - String rawResponse = connector.getResponse(request.toString()); - HttpTester.Response response = HttpTester.parseResponse(rawResponse); - assertThat("Response.status", response.getStatus(), is(HttpServletResponse.SC_BAD_REQUEST)); } + request.append("Content-Type: text/plain\r\n"); + request.append("\r\n"); + request.append("8;\r\n"); // chunk header + request.append("abcdefgh"); // actual content of 8 bytes + request.append("\r\n0;\r\n\r\n"); // last chunk + + String rawResponse = connector.getResponse(request.toString()); + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + assertThat("Response.status", response.getStatus(), is(HttpServletResponse.SC_BAD_REQUEST)); + } + + /** + * Examples of valid Chunked behaviors. + */ + public static Stream http11TransferEncodingChunked() + { + return Stream.of( + Arguments.of(Arrays.asList("chunked, ")), // results in 1 entry + Arguments.of(Arrays.asList(", chunked")), + + // invalid tokens with chunked as last + // no conflicts, chunked token is specified and is last, will result in chunked + Arguments.of(Arrays.asList("bogus, chunked")), + Arguments.of(Arrays.asList("'chunked', chunked")), // apostrophe characters with and without + Arguments.of(Arrays.asList("identity, chunked")), // identity was removed in RFC2616 errata and has been dropped in RFC7230 + + // multiple headers + Arguments.of(Arrays.asList("identity", "chunked")), // 2 separate headers + Arguments.of(Arrays.asList("", "chunked")) // 2 separate headers + ); + } + + /** + * Test Chunked Transfer-Encoding behavior indicated by + * https://tools.ietf.org/html/rfc7230#section-3.3.1 + */ + @ParameterizedTest + @MethodSource("http11TransferEncodingChunked") + public void testHttp11TransferEncodingChunked(List tokens) throws Exception + { + StringBuilder request = new StringBuilder(); + request.append("POST / HTTP/1.1\r\n"); + request.append("Host: local\r\n"); + tokens.forEach((token) -> request.append("Transfer-Encoding: ").append(token).append("\r\n")); + request.append("Content-Type: text/plain\r\n"); + request.append("\r\n"); + request.append("8;\r\n"); // chunk header + request.append("abcdefgh"); // actual content of 8 bytes + request.append("\r\n0;\r\n\r\n"); // last chunk + + System.out.println(request.toString()); + + String rawResponse = connector.getResponse(request.toString()); + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + assertThat("Response.status (" + response.getReason() + ")", response.getStatus(), is(HttpServletResponse.SC_OK)); + } + + public static Stream http11TransferEncodingInvalidChunked() + { + return Stream.of( + // == Results in 400 Bad Request + Arguments.of(Arrays.asList("bogus", "identity")), // 2 separate headers + + Arguments.of(Arrays.asList("bad")), + Arguments.of(Arrays.asList("identity")), // identity was removed in RFC2616 errata and has been dropped in RFC7230 + Arguments.of(Arrays.asList("'chunked'")), // apostrophe characters + Arguments.of(Arrays.asList("`chunked`")), // backtick "quote" characters + Arguments.of(Arrays.asList("[chunked]")), // bracketed (seen as mistake in several REST libraries) + Arguments.of(Arrays.asList("{chunked}")), // json'd (seen as mistake in several REST libraries) + Arguments.of(Arrays.asList("\u201Cchunked\u201D")), // opening and closing (fancy) double quotes characters + + // invalid tokens with chunked not as last + Arguments.of(Arrays.asList("chunked, bogus")), + Arguments.of(Arrays.asList("chunked, 'chunked'")), + Arguments.of(Arrays.asList("chunked, identity")), + Arguments.of(Arrays.asList("chunked, identity, chunked")), // duplicate chunked + Arguments.of(Arrays.asList("chunked", "identity")), // 2 separate header lines + + // multiple chunked tokens present + Arguments.of(Arrays.asList("chunked", "identity", "chunked")), // 3 separate header lines + Arguments.of(Arrays.asList("chunked", "chunked")), // 2 separate header lines + Arguments.of(Arrays.asList("chunked, chunked")) // on same line + ); + } + + /** + * Test bad Transfer-Encoding behavior as indicated by + * https://tools.ietf.org/html/rfc7230#section-3.3.1 + */ + @ParameterizedTest + @MethodSource("http11TransferEncodingInvalidChunked") + public void testHttp11TransferEncodingInvalidChunked(List tokens) throws Exception + { + HttpParser.LOG.info("badMessage: 400 Bad messages EXPECTED..."); + StringBuilder request = new StringBuilder(); + request.append("POST / HTTP/1.1\r\n"); + request.append("Host: local\r\n"); + tokens.forEach((token) -> request.append("Transfer-Encoding: ").append(token).append("\r\n")); + request.append("Content-Type: text/plain\r\n"); + request.append("\r\n"); + request.append("8;\r\n"); // chunk header + request.append("abcdefgh"); // actual content of 8 bytes + request.append("\r\n0;\r\n\r\n"); // last chunk + + System.out.println(request.toString()); + + String rawResponse = connector.getResponse(request.toString()); + HttpTester.Response response = HttpTester.parseResponse(rawResponse); + assertThat("Response.status", response.getStatus(), is(HttpServletResponse.SC_BAD_REQUEST)); } @Test @@ -541,11 +676,10 @@ public class HttpConnectionTest "Host: localhost\r\n" + "Transfer-Encoding: chunked\r\n" + "Content-Type: text/plain\r\n" + - "Connection: close\r\n" + "\r\n" + "A\r\n" + "0123456789\r\n" + - "0\r\n"); + "0\r\n\r\n"); int offset = 0; offset = checkContains(response, offset, "HTTP/1.1 200"); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/PartialRFC2616Test.java b/jetty-server/src/test/java/org/eclipse/jetty/server/PartialRFC2616Test.java index e4fd7e12c1b..3c1cba2909b 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/PartialRFC2616Test.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/PartialRFC2616Test.java @@ -35,6 +35,7 @@ import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -348,12 +349,10 @@ public class PartialRFC2616Test "\n"); offset = 0; response = endp.getResponse(); - offset = checkContains(response, offset, "HTTP/1.1 200 OK", "2. identity") + 10; - offset = checkContains(response, offset, "/R1", "2. identity") + 3; + offset = checkContains(response, offset, "HTTP/1.1 400 ", "2. identity") + 10; offset = 0; response = endp.getResponse(); - offset = checkContains(response, offset, "HTTP/1.1 200 OK", "2. identity") + 10; - offset = checkContains(response, offset, "/R2", "2. identity") + 3; + assertThat("There should be no next response as first one closed connection", response, is(nullValue())); } @Test @@ -385,7 +384,7 @@ public class PartialRFC2616Test "\n" + "abcdef"); response = endp.getResponse(); - offset = checkContains(response, offset, "HTTP/1.1 400 Bad", "3. ignore c-l") + 1; + offset = checkContains(response, offset, "HTTP/1.1 400 ", "3. ignore c-l") + 1; checkNotContained(response, offset, "/R2", "3. _content-length"); } diff --git a/tests/test-integration/src/test/java/org/eclipse/jetty/test/rfcs/RFC2616BaseTest.java b/tests/test-integration/src/test/java/org/eclipse/jetty/test/rfcs/RFC2616BaseTest.java index b0af7d6fe70..cd6ad90740c 100644 --- a/tests/test-integration/src/test/java/org/eclipse/jetty/test/rfcs/RFC2616BaseTest.java +++ b/tests/test-integration/src/test/java/org/eclipse/jetty/test/rfcs/RFC2616BaseTest.java @@ -368,20 +368,11 @@ public abstract class RFC2616BaseTest req1.append("\n"); req1.append("123\r\n"); - req1.append("GET /echo/R2 HTTP/1.1\n"); - req1.append("Host: localhost\n"); - req1.append("Connection: close\n"); - req1.append("\n"); - List responses = http.requests(req1); - assertEquals(2, responses.size(), "Response Count"); + assertEquals(1, responses.size(), "Response Count"); HttpTester.Response response = responses.get(0); - assertThat("4.4.2 Message Length / Response Code", response.getStatus(), is(HttpStatus.OK_200)); - assertThat("4.4.2 Message Length / Body", response.getContent(), Matchers.containsString("123\n")); - response = responses.get(1); - assertThat("4.4.2 Message Length / Response Code", response.getStatus(), is(HttpStatus.OK_200)); - assertEquals("", response.getContent(), "4.4.2 Message Length / No Body"); + assertThat("4.4.2 Message Length / Response Code", response.getStatus(), is(HttpStatus.BAD_REQUEST_400)); // 4.4.3 - // Client - do not send 'Content-Length' if entity-length