Fix oghttp2 to reject 204 responses with a body, even with no content-length header. This CL updates OgHttp2Session::CanReceiveBody() to return false for responses with :status 204. This change aligns oghttp2 further with nghttp2 and GFE2: http://google3/third_party/quiche/common/balsa/balsa_headers.cc;l=1093;rcl=436357765. PiperOrigin-RevId: 436844954
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index fe4e32c..645575e 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -579,8 +579,19 @@ const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr); ASSERT_GT(stream_id1, 0); + const std::vector<Header> headers2 = + ToHeaders({{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/two"}}); + + const int32_t stream_id2 = adapter->SubmitRequest(headers2, nullptr, nullptr); + ASSERT_GT(stream_id2, stream_id1); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5)); EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id2, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id2, _, 0x5, 0)); int result = adapter->Send(); EXPECT_EQ(0, result); @@ -592,6 +603,8 @@ .Headers(1, {{":status", "204"}, {"content-length", "2"}}, /*fin=*/false) .Data(1, "hi") + .Headers(3, {{":status", "204"}}, /*fin=*/false) + .Data(3, "hi") .Serialize(); // Server preface (empty SETTINGS) @@ -609,6 +622,12 @@ EXPECT_CALL( visitor, OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader)); + EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(3)); + EXPECT_CALL(visitor, OnHeaderForStream(3, ":status", "204")); + EXPECT_CALL(visitor, OnEndHeadersForStream(3)); + EXPECT_CALL(visitor, OnFrameHeader(3, _, DATA, 0)); + EXPECT_CALL(visitor, OnBeginDataForStream(3, 2)); const int64_t stream_result = adapter->ProcessBytes(stream_frames); EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); @@ -620,12 +639,18 @@ OnFrameSent(RST_STREAM, 1, _, 0x0, static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::PROTOCOL_ERROR)); + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, 3, _, 0x0, + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); + EXPECT_CALL(visitor, OnCloseStream(3, Http2ErrorCode::PROTOCOL_ERROR)); EXPECT_TRUE(adapter->want_write()); result = adapter->Send(); EXPECT_EQ(0, result); - EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::SETTINGS, - SpdyFrameType::RST_STREAM})); + EXPECT_THAT(visitor.data(), + EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::RST_STREAM, + SpdyFrameType::RST_STREAM})); } TEST(NgHttp2AdapterTest, ClientHandles304WithContent) {
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index 2aa0255..775609a 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -671,10 +671,21 @@ const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr); ASSERT_GT(stream_id1, 0); + const std::vector<Header> headers2 = + ToHeaders({{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/two"}}); + + const int32_t stream_id2 = adapter->SubmitRequest(headers2, nullptr, nullptr); + ASSERT_GT(stream_id2, stream_id1); + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5)); EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id2, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id2, _, 0x5, 0)); int result = adapter->Send(); EXPECT_EQ(0, result); @@ -686,6 +697,8 @@ .Headers(1, {{":status", "204"}, {"content-length", "2"}}, /*fin=*/false) .Data(1, "hi") + .Headers(3, {{":status", "204"}}, /*fin=*/false) + .Data(3, "hi") .Serialize(); // Server preface (empty SETTINGS) @@ -699,6 +712,12 @@ EXPECT_CALL( visitor, OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader)); + EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(3)); + EXPECT_CALL(visitor, OnHeaderForStream(3, ":status", "204")); + EXPECT_CALL(visitor, OnEndHeadersForStream(3)); + EXPECT_CALL(visitor, OnFrameHeader(3, _, DATA, 0)); + EXPECT_CALL(visitor, OnBeginDataForStream(3, 2)); const int64_t stream_result = adapter->ProcessBytes(stream_frames); EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); @@ -710,12 +729,18 @@ OnFrameSent(RST_STREAM, 1, _, 0x0, static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR)); + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, 3, _, 0x0, + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); + EXPECT_CALL(visitor, OnCloseStream(3, Http2ErrorCode::HTTP2_NO_ERROR)); EXPECT_TRUE(adapter->want_write()); result = adapter->Send(); EXPECT_EQ(0, result); - EXPECT_THAT(visitor.data(), EqualsFrames({SpdyFrameType::SETTINGS, - SpdyFrameType::RST_STREAM})); + EXPECT_THAT(visitor.data(), + EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::RST_STREAM, + SpdyFrameType::RST_STREAM})); } TEST(OgHttp2AdapterTest, ClientHandles304WithContent) {
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index 25cd0e9..d8431dd 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -284,7 +284,7 @@ frame_contains_fin_ = false; } -// TODO(diannahu): Add checks for other response codes and request methods. +// TODO(diannahu): Add checks for request methods. bool OgHttp2Session::PassthroughHeadersHandler::CanReceiveBody() const { switch (header_type()) { case HeaderType::REQUEST_TRAILER: @@ -294,7 +294,9 @@ case HeaderType::RESPONSE: // 304 responses should not have a body: // https://httpwg.org/specs/rfc7230.html#rfc.section.3.3.2 - return status_header() != "304"; + // Neither should 204 responses: + // https://httpwg.org/specs/rfc7231.html#rfc.section.6.3.5 + return status_header() != "304" && status_header() != "204"; case HeaderType::REQUEST: return true; }