Clarify OgHttp2Session header validation RST_STREAMs. This CL further differentiates between application-requested and header validation RST_STREAMs in OgHttp2Session. More header validation RST_STREAMs now use header result HEADER_HTTP_MESSAGING instead of HEADER_RST_STREAM, and the visitor callback OnInvalidFrame() is now invoked only with HEADER_HTTP_MESSAGING (excluding application-requested HEADER_RST_STREAM). This change is important for certain codec_impl_test cases to pass with oghttp2, including HeaderNameWithUnderscoreAreRejectedByDefault and ManyRequestHeadersInvokeResetStream. PiperOrigin-RevId: 414565859
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index 13d79a8..cc0cdac 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -2544,9 +2544,6 @@ EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/this/is/request/one")); EXPECT_CALL(visitor, OnHeaderForStream(1, "accept", "some bogus value!")) .WillOnce(testing::Return(Http2VisitorInterface::HEADER_RST_STREAM)); - EXPECT_CALL( - visitor, - OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader)); EXPECT_CALL(visitor, OnFrameHeader(1, 4, WINDOW_UPDATE, 0)); EXPECT_CALL(visitor, OnWindowUpdate(1, 2000)); EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0)); @@ -2610,9 +2607,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::kHttpHeader)) + EXPECT_CALL(visitor, + OnInvalidFrame( + 1, Http2VisitorInterface::InvalidFrameError::kHttpMessaging)) .WillOnce(testing::Return(false)); EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kHeaderError)); @@ -2628,12 +2625,12 @@ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, 4, 0x0)); EXPECT_CALL(visitor, OnFrameSent(RST_STREAM, 1, 4, 0x0, - static_cast<int>(Http2ErrorCode::INTERNAL_ERROR))); + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR)); EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0)); EXPECT_CALL(visitor, OnFrameSent(GOAWAY, 0, _, 0x0, - static_cast<int>(Http2ErrorCode::INTERNAL_ERROR))); + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); int send_result = adapter->Send(); // Some bytes should have been serialized.
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index a313755..19f5dad 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -225,15 +225,11 @@ return; } const auto validation_result = validator_.ValidateSingleHeader(key, value); - if (validation_result == HeaderValidator::HEADER_VALUE_INVALID_STATUS) { - QUICHE_VLOG(2) << "RST_STREAM: invalid status found"; + if (validation_result != HeaderValidator::HEADER_OK) { + QUICHE_VLOG(2) << "RST_STREAM HEADER_HTTP_MESSAGING with result " + << static_cast<int>(validation_result); result_ = Http2VisitorInterface::HEADER_HTTP_MESSAGING; return; - } else if (validation_result != HeaderValidator::HEADER_OK) { - QUICHE_VLOG(2) << "RST_STREAM: invalid header found"; - // TODO(birenroy): consider updating this to return HEADER_HTTP_MESSAGING. - result_ = Http2VisitorInterface::HEADER_RST_STREAM; - return; } result_ = visitor_.OnHeaderForStream(stream_id_, key, value); } @@ -243,9 +239,9 @@ size_t /* compressed_header_bytes */) { if (result_ == Http2VisitorInterface::HEADER_OK) { if (!validator_.FinishHeaderBlock(type_)) { - QUICHE_VLOG(1) - << "FinishHeaderBlock returned false; returning HEADER_RST_STREAM"; - result_ = Http2VisitorInterface::HEADER_RST_STREAM; + QUICHE_VLOG(1) << "FinishHeaderBlock returned false; returning " + "HEADER_HTTP_MESSAGING"; + result_ = Http2VisitorInterface::HEADER_HTTP_MESSAGING; } } if (frame_contains_fin_ && IsResponse(type_) && @@ -1144,9 +1140,11 @@ EnqueueFrame( absl::make_unique<spdy::SpdyRstStreamIR>(stream_id, spdy_error_code)); - const bool ok = visitor_.OnInvalidFrame(stream_id, frame_error); - if (!ok) { - LatchErrorAndNotify(error_code, ConnectionError::kHeaderError); + if (result == Http2VisitorInterface::HEADER_HTTP_MESSAGING) { + const bool ok = visitor_.OnInvalidFrame(stream_id, frame_error); + if (!ok) { + LatchErrorAndNotify(error_code, ConnectionError::kHeaderError); + } } } } else if (result == Http2VisitorInterface::HEADER_CONNECTION_ERROR) {