Skip to content

Commit

Permalink
Allow no SP after status-code for HTTP/1.x responses (#2247)
Browse files Browse the repository at this point in the history
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 (golang/go@d71d08a#diff-0e6945dfde3e4dcaba7cfd394a85328b8137c42e152b4486dbfb6756ad69a779R145-R146)
 - Rust HttpClient (hyperium/http#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`.
  • Loading branch information
idelpivnitskiy authored Jun 9, 2022
1 parent 1f6e4ba commit d05e52a
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -263,16 +263,26 @@ 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);
} catch (IllegalCharacterException cause) {
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
Expand Down Expand Up @@ -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"));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ void tearDown() throws Exception {

abstract EmbeddedChannel channelSpecException();

abstract boolean isDecodingRequest();

abstract String startLine();

abstract HttpMetaData assertStartLine(EmbeddedChannel channel);
Expand Down Expand Up @@ -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";
}

Expand Down Expand Up @@ -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())));
}

Expand Down Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ EmbeddedChannel channelSpecException() {
return channelSpecException;
}

@Override
boolean isDecodingRequest() {
return true;
}

@Override
String startLine() {
return "GET / HTTP/1.1";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -104,6 +106,11 @@ EmbeddedChannel channelSpecException() {
return channelSpecException;
}

@Override
boolean isDecodingRequest() {
return false;
}

@Override
String startLine() {
return "HTTP/1.1 204 No Content";
Expand Down Expand Up @@ -131,19 +138,14 @@ void illegalPrefaceCharacter() {

@Test
void noVersion() {
assertDecoderException("200 OK" + "\r\n", "Invalid start-line");
assertDecoderException("200 OK" + "\r\n", "Invalid HTTP version");
}

@Test
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");
Expand Down Expand Up @@ -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("");
Expand Down

0 comments on commit d05e52a

Please sign in to comment.