From b1d6d5675b87e6d4a5ea6575427fe083c7be2da9 Mon Sep 17 00:00:00 2001 From: Idel Pivnitskiy Date: Wed, 8 Jun 2022 17:46:37 -0700 Subject: [PATCH] Allow no SP after status-code for HTTP/1.x responses Motivation: While RFC7230, section 3.1.2 (https://datatracker.ietf.org/doc/html/rfc7230#section-3.1.2) requires a `SP` before a `reason-phrase`, even if it's an empty phrase, many implementations don't include that `SP` character. Many clients parse a response without trailing `SP` character with no error. Those implementations are: - Apache HTTP Client - JDK HttpClient - Netty - Tomcat (https://bz.apache.org/bugzilla/show_bug.cgi?id=60362) - Go HttpClient (https://github.com/golang/go/commit/d71d08af5ab15c7b166d92a31219c4c218438841#diff-0e6945dfde3e4dcaba7cfd394a85328b8137c42e152b4486dbfb6756ad69a779R145-R146) - Rust HttpClient (https://github.com/hyperium/http/issues/345) - httpd with Passenger (https://bugzilla.redhat.com/show_bug.cgi?id=1032733) Moreover, neither HTTP/2 nor HTTP/3 support reason-phrase. For this reasons, many HTTP/1.x implementations (Tomcat, Rust, etc.) decided to drop including the reason-phrase in serialized response for performance reasons. Instead, they use the reason-phrase defined by RFC for all known status codes and use `Unknown` for non-standard status codes. Modifications: - `HttpObjectDecoder`: second `SP` is optional for `HttpResponseDecoder`; - Enhance tests to verify new behavior; Result: ServiceTalk HttpClient parses responses with no `SP` after status code. Example: `HTTP/1.1 200\r\n`. --- .../http/netty/HttpObjectDecoder.java | 18 ++++++++++--- .../http/netty/HttpObjectDecoderTest.java | 10 ++++--- .../http/netty/HttpRequestDecoderTest.java | 5 ++++ .../http/netty/HttpResponseDecoderTest.java | 26 ++++++++++++++----- 4 files changed, 45 insertions(+), 14 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 index 34c69583ad..a48b5aac9b 100644 --- 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 @@ -263,7 +263,7 @@ protected final void decode(final ChannelHandlerContext ctx, final ByteBuf buffe } final int bStart = aEnd + 1; // Expect a single WS - final int bEnd; + int bEnd; try { bEnd = buffer.forEachByte(bStart, nonControlIndex - bStart + 1, isDecodingRequest() ? FIND_VCHAR_END : FIND_WS); @@ -271,8 +271,18 @@ protected final void decode(final ChannelHandlerContext ctx, final ByteBuf buffe throw new StacklessDecoderException( "Invalid start-line: HTTP request-target contains an illegal character", cause); } - if (bEnd < 0 || bEnd == bStart) { - throw newStartLineError("second"); + if (bEnd < 0) { + if (isDecodingRequest()) { + throw newStartLineError("second"); + } else { // Response can be without a reason-phrase: "HTTP/1.1 200\r\n" + bEnd = nonControlIndex + 1; + } + } + if (bEnd == bStart) { // Happens when there are two SP next to each other + throw new DecoderException("Invalid start-line: incorrect number of components, cannot find the " + + (isDecodingRequest() ? "request-target" : "status-code") + ", expected: " + + (isDecodingRequest() ? "method SP request-target SP HTTP-version" : + "HTTP-version SP status-code SP reason-phrase")); } final int cStart = bEnd + 1; // Expect a single WS @@ -908,7 +918,7 @@ private static int findLF(final ByteBuf buffer, final int fromIndex, final int t } private DecoderException newStartLineError(final String place) { - throw new DecoderException("Invalid start-line: incorrect number of components, cannot find the " + place + + return new DecoderException("Invalid start-line: incorrect number of components, cannot find the " + place + " SP, expected: " + (isDecodingRequest() ? "method SP request-target SP HTTP-version" : "HTTP-version SP status-code SP reason-phrase")); } diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpObjectDecoderTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpObjectDecoderTest.java index 5e1ecf0859..7d3944b194 100644 --- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpObjectDecoderTest.java +++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpObjectDecoderTest.java @@ -86,6 +86,8 @@ void tearDown() throws Exception { abstract EmbeddedChannel channelSpecException(); + abstract boolean isDecodingRequest(); + abstract String startLine(); abstract HttpMetaData assertStartLine(EmbeddedChannel channel); @@ -255,11 +257,11 @@ private static ByteBuf content(int contentLength) { return wrappedBuffer(content); } - private EmbeddedChannel channel(boolean crlf) { + EmbeddedChannel channel(boolean crlf) { return crlf ? channel() : channelSpecException(); } - private static String br(boolean crlf) { + static String br(boolean crlf) { return crlf ? "\r\n" : "\n"; } @@ -822,7 +824,7 @@ private void unexpectedTrailersAfterContentLength(boolean crlf) { // https://tools.ietf.org/html/rfc7230#section-3.3 DecoderException e = assertThrows(DecoderException.class, () -> writeMsg("TrailerStatus: good" + br + br, channel)); - assertThat(e.getMessage(), startsWith("Invalid start-line")); + assertThat(e.getMessage(), startsWith(isDecodingRequest() ? "Invalid start-line" : "Invalid HTTP version")); assertThat(channel.inboundMessages(), is(not(empty()))); } @@ -851,7 +853,7 @@ private void smuggleZeroContentLength(boolean smuggleBeforeContentLength, boolea "Smuggled: " + startLine() + br + br + "Content-Length: 0" + br : "Content-Length: 0" + br + "Smuggled: " + startLine() + br + br) + "Connection: keep-alive" + br + br, channel)); - assertThat(e.getMessage(), startsWith("Invalid start-line")); + assertThat(e.getMessage(), startsWith(isDecodingRequest() ? "Invalid start-line" : "Invalid HTTP version")); HttpMetaData metaData = assertStartLine(channel); assertSingleHeaderValue(metaData.headers(), HOST, "servicetalk.io"); 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 index 0aa74387fe..91a417d10f 100644 --- 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 @@ -68,6 +68,11 @@ EmbeddedChannel channelSpecException() { return channelSpecException; } + @Override + boolean isDecodingRequest() { + return true; + } + @Override String startLine() { return "GET / HTTP/1.1"; 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 index 09be429030..79614b7508 100644 --- 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 @@ -42,6 +42,8 @@ import io.netty.channel.embedded.EmbeddedChannel; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import java.util.ArrayDeque; import java.util.ArrayList; @@ -104,6 +106,11 @@ EmbeddedChannel channelSpecException() { return channelSpecException; } + @Override + boolean isDecodingRequest() { + return false; + } + @Override String startLine() { return "HTTP/1.1 204 No Content"; @@ -131,7 +138,7 @@ void illegalPrefaceCharacter() { @Test void noVersion() { - assertDecoderException("200 OK" + "\r\n", "Invalid start-line"); + assertDecoderException("200 OK" + "\r\n", "Invalid HTTP version"); } @Test @@ -139,11 +146,6 @@ void noStatusCode() { assertDecoderException("HTTP/1.1 OK" + "\r\n", "Invalid start-line"); } - @Test - void noSpAfterStatusCode() { - assertDecoderException("HTTP/1.1 200" + "\r\n", "Invalid start-line"); - } - @Test void invalidStartLineOrder() { assertDecoderException("HTTP/1.1 OK 200" + "\r\n", "Invalid start-line"); @@ -224,6 +226,18 @@ void validStartLineWithCustomHttpVersion() { assertFalse(channel.finishAndReleaseAll()); } + @ParameterizedTest(name = "statusCode={0}, crlf={1}") + @CsvSource({"200,true", "200,false", "299,true", "299,false"}) + void noSpAfterStatusCodeNoReasonPhrase(int statusCode, boolean crlf) { + HttpResponseStatus status = HttpResponseStatus.of(statusCode, ""); + EmbeddedChannel channel = channel(crlf); + String br = br(crlf); + writeMsg("HTTP/1.1 " + status.code() /* no SP */ + br + br, channel); + assertResponseLine(HTTP_1_1, status, channel); + assertEmptyTrailers(channel); + assertFalse(channel.finishAndReleaseAll()); + } + @Test void emptyReasonPhrase() { testReasonPhrase("");