Adds validation for 204 responses, which should carry no body. PiperOrigin-RevId: 416414380
diff --git a/http2/adapter/header_validator.cc b/http2/adapter/header_validator.cc index 321e958..0b042b1 100644 --- a/http2/adapter/header_validator.cc +++ b/http2/adapter/header_validator.cc
@@ -92,6 +92,9 @@ method_ = std::string(value); } pseudo_headers_.push_back(std::string(key)); + } else if (key == "content-length" && status_ == "204" && value != "0") { + // There should be no body in a "204 No Content" response. + return HEADER_FIELD_INVALID; } return HEADER_OK; }
diff --git a/http2/adapter/header_validator_test.cc b/http2/adapter/header_validator_test.cc index 35750d3..1f202d9 100644 --- a/http2/adapter/header_validator_test.cc +++ b/http2/adapter/header_validator_test.cc
@@ -222,6 +222,42 @@ } } +TEST(HeaderValidatorTest, Response204) { + HeaderValidator v; + + v.StartHeaderBlock(); + EXPECT_EQ(HeaderValidator::HEADER_OK, + v.ValidateSingleHeader(":status", "204")); + EXPECT_EQ(HeaderValidator::HEADER_OK, + v.ValidateSingleHeader("x-content", "is not present")); + EXPECT_TRUE(v.FinishHeaderBlock(HeaderType::RESPONSE)); +} + +TEST(HeaderValidatorTest, Response204WithContentLengthZero) { + HeaderValidator v; + + v.StartHeaderBlock(); + EXPECT_EQ(HeaderValidator::HEADER_OK, + v.ValidateSingleHeader(":status", "204")); + EXPECT_EQ(HeaderValidator::HEADER_OK, + v.ValidateSingleHeader("x-content", "is not present")); + EXPECT_EQ(HeaderValidator::HEADER_OK, + v.ValidateSingleHeader("content-length", "0")); + EXPECT_TRUE(v.FinishHeaderBlock(HeaderType::RESPONSE)); +} + +TEST(HeaderValidatorTest, Response204WithContentLength) { + HeaderValidator v; + + v.StartHeaderBlock(); + EXPECT_EQ(HeaderValidator::HEADER_OK, + v.ValidateSingleHeader(":status", "204")); + EXPECT_EQ(HeaderValidator::HEADER_OK, + v.ValidateSingleHeader("x-content", "is not present")); + EXPECT_EQ(HeaderValidator::HEADER_FIELD_INVALID, + v.ValidateSingleHeader("content-length", "1")); +} + TEST(HeaderValidatorTest, ResponseTrailerPseudoHeaders) { HeaderValidator v;
diff --git a/http2/adapter/http2_visitor_interface.h b/http2/adapter/http2_visitor_interface.h index 6d843a3..8e39ef4 100644 --- a/http2/adapter/http2_visitor_interface.h +++ b/http2/adapter/http2_visitor_interface.h
@@ -121,7 +121,10 @@ HEADER_CONNECTION_ERROR, // The application rejects the header and requests the stream be reset. HEADER_RST_STREAM, - // The header is a violation of HTTP messaging semantics and will be reset + // The header field is invalid and will be reset with error code + // PROTOCOL_ERROR. + HEADER_FIELD_INVALID, + // The headers are a violation of HTTP messaging semantics and will be reset // with error code PROTOCOL_ERROR. HEADER_HTTP_MESSAGING, };
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index 147d991..6410b97 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -326,6 +326,70 @@ spdy::SpdyFrameType::RST_STREAM})); } +TEST(OgHttp2AdapterClientTest, ClientHandles204WithContent) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor); + + testing::InSequence s; + + const std::vector<const Header> headers1 = + ToHeaders({{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}); + + const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr); + ASSERT_GT(stream_id1, 0); + + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0)); + + int result = adapter->Send(); + EXPECT_EQ(0, result); + visitor.Clear(); + + const std::string stream_frames = + TestFrameSequence() + .ServerPreface() + .Headers(1, {{":status", "204"}, {"content-length", "2"}}, + /*fin=*/false) + .Data(1, "hi") + .Serialize(); + + // Server preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":status", "204")); + EXPECT_CALL( + visitor, + OnErrorDebug("Invalid HTTP header field was received: frame type: 1, " + "stream: 1, name: [content-length], value: [2]")); + EXPECT_CALL( + visitor, + OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader)); + + const int64_t stream_result = adapter->ProcessBytes(stream_frames); + EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, 1, _, 0x0, + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::PROTOCOL_ERROR)); + + EXPECT_TRUE(adapter->want_write()); + result = adapter->Send(); + EXPECT_EQ(0, result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::RST_STREAM})); +} + TEST(NgHttp2AdapterTest, ClientHandlesTrailers) { DataSavingVisitor visitor; auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor);
diff --git a/http2/adapter/nghttp2_callbacks.cc b/http2/adapter/nghttp2_callbacks.cc index cb15a21..dc6df1d 100644 --- a/http2/adapter/nghttp2_callbacks.cc +++ b/http2/adapter/nghttp2_callbacks.cc
@@ -176,6 +176,7 @@ case Http2VisitorInterface::HEADER_CONNECTION_ERROR: return NGHTTP2_ERR_CALLBACK_FAILURE; case Http2VisitorInterface::HEADER_RST_STREAM: + case Http2VisitorInterface::HEADER_FIELD_INVALID: case Http2VisitorInterface::HEADER_HTTP_MESSAGING: return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; }
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index fccdf1d..50dce2c 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -340,6 +340,73 @@ spdy::SpdyFrameType::RST_STREAM})); } +TEST(OgHttp2AdapterClientTest, ClientHandles204WithContent) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + testing::InSequence s; + + const std::vector<const Header> headers1 = + ToHeaders({{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}); + + const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr); + ASSERT_GT(stream_id1, 0); + + 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)); + + int result = adapter->Send(); + EXPECT_EQ(0, result); + visitor.Clear(); + + const std::string stream_frames = + TestFrameSequence() + .ServerPreface() + .Headers(1, {{":status", "204"}, {"content-length", "2"}}, + /*fin=*/false) + .Data(1, "hi") + .Serialize(); + + // Server preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":status", "204")); + EXPECT_CALL( + visitor, + OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader)); + // TODO(b/210728715): Stop delivering events for streams with errors. + EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0)); + EXPECT_CALL(visitor, OnBeginDataForStream(1, 2)); + EXPECT_CALL(visitor, OnDataForStream(1, "hi")); + + const int64_t stream_result = adapter->ProcessBytes(stream_frames); + EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, 1, _, 0x0, + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR)); + + EXPECT_TRUE(adapter->want_write()); + result = adapter->Send(); + EXPECT_EQ(0, result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::RST_STREAM})); +} + TEST(OgHttp2AdapterClientTest, ClientHandlesTrailers) { DataSavingVisitor visitor; OgHttp2Adapter::Options options{.perspective = Perspective::kClient}; @@ -1354,9 +1421,9 @@ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); - EXPECT_CALL(visitor, - OnInvalidFrame( - 1, Http2VisitorInterface::InvalidFrameError::kHttpMessaging)); + EXPECT_CALL( + visitor, + OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader)); const int64_t stream_result = adapter->ProcessBytes(stream_frames); EXPECT_EQ(static_cast<int64_t>(stream_frames.size()), stream_result); @@ -2854,9 +2921,9 @@ EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https")); EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com")); EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/this/is/request/one")); - EXPECT_CALL(visitor, - OnInvalidFrame( - 1, Http2VisitorInterface::InvalidFrameError::kHttpMessaging)) + EXPECT_CALL( + visitor, + OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader)) .WillOnce(testing::Return(false)); EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kHeaderError));
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index 09bbe7e..346035a 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -229,9 +229,9 @@ } const auto validation_result = validator_.ValidateSingleHeader(key, value); if (validation_result != HeaderValidator::HEADER_OK) { - QUICHE_VLOG(2) << "RST_STREAM HEADER_HTTP_MESSAGING with result " + QUICHE_VLOG(2) << "RST_STREAM with result " << static_cast<int>(validation_result); - result_ = Http2VisitorInterface::HEADER_HTTP_MESSAGING; + result_ = Http2VisitorInterface::HEADER_FIELD_INVALID; return; } result_ = visitor_.OnHeaderForStream(stream_id_, key, value); @@ -1159,6 +1159,7 @@ QUICHE_DCHECK_NE(result, Http2VisitorInterface::HEADER_OK); const bool should_reset_stream = result == Http2VisitorInterface::HEADER_RST_STREAM || + result == Http2VisitorInterface::HEADER_FIELD_INVALID || result == Http2VisitorInterface::HEADER_HTTP_MESSAGING; if (should_reset_stream) { const Http2ErrorCode error_code = @@ -1167,7 +1168,8 @@ : Http2ErrorCode::PROTOCOL_ERROR; const spdy::SpdyErrorCode spdy_error_code = TranslateErrorCode(error_code); const Http2VisitorInterface::InvalidFrameError frame_error = - (result == Http2VisitorInterface::HEADER_RST_STREAM) + (result == Http2VisitorInterface::HEADER_RST_STREAM || + result == Http2VisitorInterface::HEADER_FIELD_INVALID) ? Http2VisitorInterface::InvalidFrameError::kHttpHeader : Http2VisitorInterface::InvalidFrameError::kHttpMessaging; auto it = streams_reset_.find(stream_id); @@ -1175,7 +1177,8 @@ EnqueueFrame( absl::make_unique<spdy::SpdyRstStreamIR>(stream_id, spdy_error_code)); - if (result == Http2VisitorInterface::HEADER_HTTP_MESSAGING) { + if (result == Http2VisitorInterface::HEADER_FIELD_INVALID || + result == Http2VisitorInterface::HEADER_HTTP_MESSAGING) { const bool ok = visitor_.OnInvalidFrame(stream_id, frame_error); if (!ok) { LatchErrorAndNotify(error_code, ConnectionError::kHeaderError);