Signal error on PUSH_PROMISE and CANCEL_PUSH frames. Since HttpEncoder is not capable of sending MAX_PUSH_ID frames, receiving these frames is a violation of the spec. For simplicity, use the error code H3_FRAME_ERROR. This is incorrect, because receiving a PUSH_PROMISE frame by the client on the control stream, or receiving a CANCEL_PUSH frame either by the client or by the server on the control stream should trigger an H3_ID_ERROR, and these frames in other cases should trigger a H3_FRAME_UNEXPECTED error. However, it is expected to be exceedingly rare for any endpoint to send such frames without receiving a MAX_PUSH_ID frame, so the error code does not matter as much as actually closing the connection. Protected by FLAGS_quic_reloadable_flag_quic_error_on_http3_push. PiperOrigin-RevId: 370762371 Change-Id: I18d4d29e95faf9919858bada73ef0b5912afbd8b
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc index 859d174..6d9290a 100644 --- a/quic/core/http/http_decoder.cc +++ b/quic/core/http/http_decoder.cc
@@ -38,11 +38,15 @@ error_(QUIC_NO_ERROR), error_detail_(""), ignore_old_priority_update_( - GetQuicReloadableFlag(quic_ignore_old_priority_update_frame)) { + GetQuicReloadableFlag(quic_ignore_old_priority_update_frame)), + error_on_http3_push_(GetQuicReloadableFlag(quic_error_on_http3_push)) { QUICHE_DCHECK(visitor_); if (ignore_old_priority_update_) { QUIC_RELOADABLE_FLAG_COUNT(quic_ignore_old_priority_update_frame); } + if (error_on_http3_push_) { + QUIC_RELOADABLE_FLAG_COUNT(quic_error_on_http3_push); + } } HttpDecoder::~HttpDecoder() {} @@ -177,6 +181,20 @@ current_frame_type_)); return false; } + + if (error_on_http3_push_) { + if (current_frame_type_ == + static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH)) { + RaiseError(QUIC_HTTP_FRAME_ERROR, "CANCEL_PUSH frame received."); + return false; + } + if (current_frame_type_ == + static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE)) { + RaiseError(QUIC_HTTP_FRAME_ERROR, "PUSH_PROMISE frame received."); + return false; + } + } + state_ = STATE_READING_FRAME_LENGTH; return true; } @@ -243,11 +261,19 @@ visitor_->OnHeadersFrameStart(header_length, current_frame_length_); break; case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH): + if (error_on_http3_push_) { + QUICHE_NOTREACHED(); + break; + } break; case static_cast<uint64_t>(HttpFrameType::SETTINGS): continue_processing = visitor_->OnSettingsFrameStart(header_length); break; case static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE): + if (error_on_http3_push_) { + QUICHE_NOTREACHED(); + break; + } // This edge case needs to be handled here, because ReadFramePayload() // does not get called if |current_frame_length_| is zero. if (current_frame_length_ == 0) { @@ -318,7 +344,11 @@ break; } case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH): { - continue_processing = BufferOrParsePayload(reader); + if (error_on_http3_push_) { + QUICHE_NOTREACHED(); + } else { + continue_processing = BufferOrParsePayload(reader); + } break; } case static_cast<uint64_t>(HttpFrameType::SETTINGS): { @@ -326,6 +356,10 @@ break; } case static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE): { + if (error_on_http3_push_) { + QUICHE_NOTREACHED(); + break; + } PushId push_id; if (current_frame_length_ == remaining_frame_length_) { // A new Push Promise frame just arrived. @@ -443,9 +477,13 @@ break; } case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH): { - // If frame payload is not empty, FinishParsing() is skipped. - QUICHE_DCHECK_EQ(0u, current_frame_length_); - continue_processing = BufferOrParsePayload(reader); + if (error_on_http3_push_) { + QUICHE_NOTREACHED(); + } else { + // If frame payload is not empty, FinishParsing() is skipped. + QUICHE_DCHECK_EQ(0u, current_frame_length_); + continue_processing = BufferOrParsePayload(reader); + } break; } case static_cast<uint64_t>(HttpFrameType::SETTINGS): { @@ -455,7 +493,11 @@ break; } case static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE): { - continue_processing = visitor_->OnPushPromiseFrameEnd(); + if (error_on_http3_push_) { + QUICHE_NOTREACHED(); + } else { + continue_processing = visitor_->OnPushPromiseFrameEnd(); + } break; } case static_cast<uint64_t>(HttpFrameType::GOAWAY): { @@ -578,6 +620,10 @@ switch (current_frame_type_) { case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH): { + if (error_on_http3_push_) { + QUICHE_NOTREACHED(); + return false; + } CancelPushFrame frame; if (!reader->ReadVarInt62(&frame.push_id)) { RaiseError(QUIC_HTTP_FRAME_ERROR, @@ -791,6 +837,7 @@ QuicByteCount HttpDecoder::MaxFrameLength(uint64_t frame_type) { switch (frame_type) { case static_cast<uint64_t>(HttpFrameType::CANCEL_PUSH): + // TODO(b/171463363): Remove. return sizeof(PushId); case static_cast<uint64_t>(HttpFrameType::SETTINGS): // This limit is arbitrary. @@ -798,6 +845,7 @@ case static_cast<uint64_t>(HttpFrameType::GOAWAY): return VARIABLE_LENGTH_INTEGER_LENGTH_8; case static_cast<uint64_t>(HttpFrameType::MAX_PUSH_ID): + // TODO(b/171463363): Remove. return sizeof(PushId); case static_cast<uint64_t>(HttpFrameType::PRIORITY_UPDATE): // This limit is arbitrary.
diff --git a/quic/core/http/http_decoder.h b/quic/core/http/http_decoder.h index 5580d44..3a970fb 100644 --- a/quic/core/http/http_decoder.h +++ b/quic/core/http/http_decoder.h
@@ -45,6 +45,7 @@ // processed. At that point it is safe to consume |header_length| bytes. // Called when a CANCEL_PUSH frame has been successfully parsed. + // TODO(b/171463363): Remove. virtual bool OnCancelPushFrame(const CancelPushFrame& frame) = 0; // Called when a MAX_PUSH_ID frame has been successfully parsed. @@ -83,6 +84,7 @@ // Called when a HEADERS frame has been completely processed. virtual bool OnHeadersFrameEnd() = 0; + // TODO(b/171463363): Remove all. // Called when a PUSH_PROMISE frame has been received. virtual bool OnPushPromiseFrameStart(QuicByteCount header_length) = 0; // Called when the Push ID field of a PUSH_PROMISE frame has been parsed. @@ -230,7 +232,7 @@ void BufferFrameType(QuicDataReader* reader); // Buffers at most |remaining_push_id_length_| from |reader| to - // |push_id_buffer_|. + // |push_id_buffer_|. TODO(b/171463363): Remove. void BufferPushId(QuicDataReader* reader); // Sets |error_| and |error_detail_| accordingly. @@ -278,8 +280,10 @@ // Remaining length that's needed for the frame's type field. QuicByteCount remaining_type_field_length_; // Length of PUSH_PROMISE frame's push id. + // TODO(b/171463363): Remove. QuicByteCount current_push_id_length_; // Remaining length that's needed for PUSH_PROMISE frame's push id field. + // TODO(b/171463363): Remove. QuicByteCount remaining_push_id_length_; // Last error. QuicErrorCode error_; @@ -292,11 +296,16 @@ // Remaining unparsed type field data. std::array<char, sizeof(uint64_t)> type_buffer_; // Remaining unparsed push id data. + // TODO(b/171463363): Remove. std::array<char, sizeof(uint64_t)> push_id_buffer_; // Latched value of // gfe2_reloadable_flag_quic_ignore_old_priority_update_frame. const bool ignore_old_priority_update_; + + // Latched value of + // gfe2_reloadable_flag_quic_error_on_http3_push. + const bool error_on_http3_push_; }; } // namespace quic
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc index ab0e31f..95c1c5c 100644 --- a/quic/core/http/http_decoder_test.cc +++ b/quic/core/http/http_decoder_test.cc
@@ -249,6 +249,14 @@ "01" // length "01"); // Push Id + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + EXPECT_CALL(visitor_, OnError(&decoder_)); + EXPECT_EQ(1u, ProcessInput(input)); + EXPECT_THAT(decoder_.error(), IsError(QUIC_HTTP_FRAME_ERROR)); + EXPECT_EQ("CANCEL_PUSH frame received.", decoder_.error_detail()); + return; + } + // Visitor pauses processing. EXPECT_CALL(visitor_, OnCancelPushFrame(CancelPushFrame({1}))) .WillOnce(Return(false)); @@ -277,6 +285,14 @@ "C000000000000101"), // push id 257 "Headers"); // headers + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + EXPECT_CALL(visitor_, OnError(&decoder_)); + EXPECT_EQ(1u, ProcessInput(input)); + EXPECT_THAT(decoder_.error(), IsError(QUIC_HTTP_FRAME_ERROR)); + EXPECT_EQ("PUSH_PROMISE frame received.", decoder_.error_detail()); + return; + } + // Visitor pauses processing. EXPECT_CALL(visitor_, OnPushPromiseFrameStart(2)).WillOnce(Return(false)); EXPECT_CALL(visitor_, OnPushPromiseFramePushId(257, 8, 7)) @@ -338,6 +354,10 @@ } TEST_F(HttpDecoderTest, CorruptPushPromiseFrame) { + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + return; + } + InSequence s; std::string input = absl::HexStringToBytes( @@ -733,6 +753,10 @@ } TEST_F(HttpDecoderTest, PushPromiseFrameNoHeaders) { + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + return; + } + InSequence s; std::string input = absl::HexStringToBytes( "05" // type (PUSH_PROMISE) @@ -768,6 +792,10 @@ } TEST_F(HttpDecoderTest, MalformedFrameWithOverlyLargePayload) { + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + return; + } + std::string input = absl::HexStringToBytes( "03" // type (CANCEL_PUSH) "10" // length @@ -841,97 +869,183 @@ } TEST_F(HttpDecoderTest, CorruptFrame) { - InSequence s; + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + InSequence s; - struct { - const char* const input; - const char* const error_message; - } kTestData[] = {{"\x03" // type (CANCEL_PUSH) - "\x01" // length - "\x40", // first byte of two-byte varint push id - "Unable to read CANCEL_PUSH push_id."}, - {"\x03" // type (CANCEL_PUSH) - "\x04" // length - "\x05" // valid push id - "foo", // superfluous data - "Superfluous data in CANCEL_PUSH frame."}, - {"\x0D" // type (MAX_PUSH_ID) - "\x01" // length - "\x40", // first byte of two-byte varint push id - "Unable to read MAX_PUSH_ID push_id."}, - {"\x0D" // type (MAX_PUSH_ID) - "\x04" // length - "\x05" // valid push id - "foo", // superfluous data - "Superfluous data in MAX_PUSH_ID frame."}, - {"\x07" // type (GOAWAY) - "\x01" // length - "\x40", // first byte of two-byte varint stream id - "Unable to read GOAWAY ID."}, - {"\x07" // type (GOAWAY) - "\x04" // length - "\x05" // valid stream id - "foo", // superfluous data - "Superfluous data in GOAWAY frame."}, - {"\x40\x89" // type (ACCEPT_CH) - "\x01" // length - "\x40", // first byte of two-byte varint origin length - "Unable to read ACCEPT_CH origin."}, - {"\x40\x89" // type (ACCEPT_CH) - "\x01" // length - "\x05", // valid origin length but no origin string - "Unable to read ACCEPT_CH origin."}, - {"\x40\x89" // type (ACCEPT_CH) - "\x04" // length - "\x05" // valid origin length - "foo", // payload ends before origin ends - "Unable to read ACCEPT_CH origin."}, - {"\x40\x89" // type (ACCEPT_CH) - "\x04" // length - "\x03" // valid origin length - "foo", // payload ends at end of origin: no value - "Unable to read ACCEPT_CH value."}, - {"\x40\x89" // type (ACCEPT_CH) - "\x05" // length - "\x03" // valid origin length - "foo" // payload ends at end of origin: no value - "\x40", // first byte of two-byte varint value length - "Unable to read ACCEPT_CH value."}, - {"\x40\x89" // type (ACCEPT_CH) - "\x08" // length - "\x03" // valid origin length - "foo" // origin - "\x05" // valid value length - "bar", // payload ends before value ends - "Unable to read ACCEPT_CH value."}}; + struct { + const char* const input; + const char* const error_message; + } kTestData[] = {{"\x0D" // type (MAX_PUSH_ID) + "\x01" // length + "\x40", // first byte of two-byte varint push id + "Unable to read MAX_PUSH_ID push_id."}, + {"\x0D" // type (MAX_PUSH_ID) + "\x04" // length + "\x05" // valid push id + "foo", // superfluous data + "Superfluous data in MAX_PUSH_ID frame."}, + {"\x07" // type (GOAWAY) + "\x01" // length + "\x40", // first byte of two-byte varint stream id + "Unable to read GOAWAY ID."}, + {"\x07" // type (GOAWAY) + "\x04" // length + "\x05" // valid stream id + "foo", // superfluous data + "Superfluous data in GOAWAY frame."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x01" // length + "\x40", // first byte of two-byte varint origin length + "Unable to read ACCEPT_CH origin."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x01" // length + "\x05", // valid origin length but no origin string + "Unable to read ACCEPT_CH origin."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x04" // length + "\x05" // valid origin length + "foo", // payload ends before origin ends + "Unable to read ACCEPT_CH origin."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x04" // length + "\x03" // valid origin length + "foo", // payload ends at end of origin: no value + "Unable to read ACCEPT_CH value."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x05" // length + "\x03" // valid origin length + "foo" // payload ends at end of origin: no value + "\x40", // first byte of two-byte varint value length + "Unable to read ACCEPT_CH value."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x08" // length + "\x03" // valid origin length + "foo" // origin + "\x05" // valid value length + "bar", // payload ends before value ends + "Unable to read ACCEPT_CH value."}}; - for (const auto& test_data : kTestData) { - { - HttpDecoder decoder(&visitor_); - EXPECT_CALL(visitor_, OnAcceptChFrameStart(_)).Times(AnyNumber()); - EXPECT_CALL(visitor_, OnError(&decoder)); + for (const auto& test_data : kTestData) { + { + HttpDecoder decoder(&visitor_); + EXPECT_CALL(visitor_, OnAcceptChFrameStart(_)).Times(AnyNumber()); + EXPECT_CALL(visitor_, OnError(&decoder)); - absl::string_view input(test_data.input); - decoder.ProcessInput(input.data(), input.size()); - EXPECT_THAT(decoder.error(), IsError(QUIC_HTTP_FRAME_ERROR)); - EXPECT_EQ(test_data.error_message, decoder.error_detail()); - } - { - HttpDecoder decoder(&visitor_); - EXPECT_CALL(visitor_, OnAcceptChFrameStart(_)).Times(AnyNumber()); - EXPECT_CALL(visitor_, OnError(&decoder)); - - absl::string_view input(test_data.input); - for (auto c : input) { - decoder.ProcessInput(&c, 1); + absl::string_view input(test_data.input); + decoder.ProcessInput(input.data(), input.size()); + EXPECT_THAT(decoder.error(), IsError(QUIC_HTTP_FRAME_ERROR)); + EXPECT_EQ(test_data.error_message, decoder.error_detail()); } - EXPECT_THAT(decoder.error(), IsError(QUIC_HTTP_FRAME_ERROR)); - EXPECT_EQ(test_data.error_message, decoder.error_detail()); + { + HttpDecoder decoder(&visitor_); + EXPECT_CALL(visitor_, OnAcceptChFrameStart(_)).Times(AnyNumber()); + EXPECT_CALL(visitor_, OnError(&decoder)); + + absl::string_view input(test_data.input); + for (auto c : input) { + decoder.ProcessInput(&c, 1); + } + EXPECT_THAT(decoder.error(), IsError(QUIC_HTTP_FRAME_ERROR)); + EXPECT_EQ(test_data.error_message, decoder.error_detail()); + } + } + } else { + InSequence s; + + struct { + const char* const input; + const char* const error_message; + } kTestData[] = {{"\x03" // type (CANCEL_PUSH) + "\x01" // length + "\x40", // first byte of two-byte varint push id + "Unable to read CANCEL_PUSH push_id."}, + {"\x03" // type (CANCEL_PUSH) + "\x04" // length + "\x05" // valid push id + "foo", // superfluous data + "Superfluous data in CANCEL_PUSH frame."}, + {"\x0D" // type (MAX_PUSH_ID) + "\x01" // length + "\x40", // first byte of two-byte varint push id + "Unable to read MAX_PUSH_ID push_id."}, + {"\x0D" // type (MAX_PUSH_ID) + "\x04" // length + "\x05" // valid push id + "foo", // superfluous data + "Superfluous data in MAX_PUSH_ID frame."}, + {"\x07" // type (GOAWAY) + "\x01" // length + "\x40", // first byte of two-byte varint stream id + "Unable to read GOAWAY ID."}, + {"\x07" // type (GOAWAY) + "\x04" // length + "\x05" // valid stream id + "foo", // superfluous data + "Superfluous data in GOAWAY frame."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x01" // length + "\x40", // first byte of two-byte varint origin length + "Unable to read ACCEPT_CH origin."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x01" // length + "\x05", // valid origin length but no origin string + "Unable to read ACCEPT_CH origin."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x04" // length + "\x05" // valid origin length + "foo", // payload ends before origin ends + "Unable to read ACCEPT_CH origin."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x04" // length + "\x03" // valid origin length + "foo", // payload ends at end of origin: no value + "Unable to read ACCEPT_CH value."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x05" // length + "\x03" // valid origin length + "foo" // payload ends at end of origin: no value + "\x40", // first byte of two-byte varint value length + "Unable to read ACCEPT_CH value."}, + {"\x40\x89" // type (ACCEPT_CH) + "\x08" // length + "\x03" // valid origin length + "foo" // origin + "\x05" // valid value length + "bar", // payload ends before value ends + "Unable to read ACCEPT_CH value."}}; + + for (const auto& test_data : kTestData) { + { + HttpDecoder decoder(&visitor_); + EXPECT_CALL(visitor_, OnAcceptChFrameStart(_)).Times(AnyNumber()); + EXPECT_CALL(visitor_, OnError(&decoder)); + + absl::string_view input(test_data.input); + decoder.ProcessInput(input.data(), input.size()); + EXPECT_THAT(decoder.error(), IsError(QUIC_HTTP_FRAME_ERROR)); + EXPECT_EQ(test_data.error_message, decoder.error_detail()); + } + { + HttpDecoder decoder(&visitor_); + EXPECT_CALL(visitor_, OnAcceptChFrameStart(_)).Times(AnyNumber()); + EXPECT_CALL(visitor_, OnError(&decoder)); + + absl::string_view input(test_data.input); + for (auto c : input) { + decoder.ProcessInput(&c, 1); + } + EXPECT_THAT(decoder.error(), IsError(QUIC_HTTP_FRAME_ERROR)); + EXPECT_EQ(test_data.error_message, decoder.error_detail()); + } } } } TEST_F(HttpDecoderTest, EmptyCancelPushFrame) { + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + return; + } + std::string input = absl::HexStringToBytes( "03" // type (CANCEL_PUSH) "00"); // frame length @@ -959,6 +1073,10 @@ // Regression test for https://crbug.com/1001823. TEST_F(HttpDecoderTest, EmptyPushPromiseFrame) { + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + return; + } + std::string input = absl::HexStringToBytes( "05" // type (PUSH_PROMISE) "00"); // frame length
diff --git a/quic/core/http/quic_receive_control_stream.cc b/quic/core/http/quic_receive_control_stream.cc index fbd7b2e..83a8940 100644 --- a/quic/core/http/quic_receive_control_stream.cc +++ b/quic/core/http/quic_receive_control_stream.cc
@@ -70,7 +70,6 @@ spdy_session()->debug_visitor()->OnCancelPushFrameReceived(frame); } - // TODO(b/151841240): Handle CANCEL_PUSH frames instead of ignoring them. return ValidateFrameType(HttpFrameType::CANCEL_PUSH); }
diff --git a/quic/core/http/quic_receive_control_stream_test.cc b/quic/core/http/quic_receive_control_stream_test.cc index 1c24808..b81d0ba 100644 --- a/quic/core/http/quic_receive_control_stream_test.cc +++ b/quic/core/http/quic_receive_control_stream_test.cc
@@ -311,7 +311,10 @@ push_promise_frame); EXPECT_CALL( *connection_, - CloseConnection(QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, _, _)) + CloseConnection(GetQuicReloadableFlag(quic_error_on_http3_push) + ? QUIC_HTTP_FRAME_ERROR + : QUIC_HTTP_FRAME_UNEXPECTED_ON_CONTROL_STREAM, + _, _)) .WillOnce( Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _, _)); @@ -382,13 +385,21 @@ "01" // payload length "01"); // push ID - EXPECT_CALL(*connection_, - CloseConnection(QUIC_HTTP_MISSING_SETTINGS_FRAME, - "First frame received on control stream is type " - "3, but it must be SETTINGS.", - _)) - .WillOnce( - Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_FRAME_ERROR, + "CANCEL_PUSH frame received.", _)) + .WillOnce( + Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); + } else { + EXPECT_CALL( + *connection_, + CloseConnection(QUIC_HTTP_MISSING_SETTINGS_FRAME, + "First frame received on control stream is type " + "3, but it must be SETTINGS.", + _)) + .WillOnce( + Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); + } EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _, _)); EXPECT_CALL(session_, OnConnectionClosed(_, _));
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc index c638a0a..dbf5def 100644 --- a/quic/core/http/quic_spdy_session_test.cc +++ b/quic/core/http/quic_spdy_session_test.cc
@@ -2894,10 +2894,7 @@ session_.OnHttp3GoAway(stream_id2); } -// Test that receipt of CANCEL_PUSH frame does not result in closing the -// connection. -// TODO(b/151841240): Handle CANCEL_PUSH frames instead of ignoring them. -TEST_P(QuicSpdySessionTestClient, IgnoreCancelPush) { +TEST_P(QuicSpdySessionTestClient, CloseConnectionOnCancelPush) { if (!VersionUsesHttp3(transport_version())) { return; } @@ -2934,7 +2931,18 @@ "00"); // push ID QuicStreamFrame data3(receive_control_stream_id, /* fin = */ false, offset, cancel_push_frame); - EXPECT_CALL(debug_visitor, OnCancelPushFrameReceived(_)); + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_FRAME_ERROR, + "CANCEL_PUSH frame received.", _)) + .WillOnce( + Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); + EXPECT_CALL(*connection_, + SendConnectionClosePacket(QUIC_HTTP_FRAME_ERROR, _, + "CANCEL_PUSH frame received.")); + } else { + // CANCEL_PUSH is ignored. + EXPECT_CALL(debug_visitor, OnCancelPushFrameReceived(_)); + } session_.OnStreamFrame(data3); } @@ -3093,10 +3101,7 @@ session_.OnStopSendingFrame(stop_sending_encoder_stream); } -// Test that receipt of CANCEL_PUSH frame does not result in closing the -// connection. -// TODO(b/151841240): Handle CANCEL_PUSH frames instead of ignoring them. -TEST_P(QuicSpdySessionTestServer, IgnoreCancelPush) { +TEST_P(QuicSpdySessionTestServer, CloseConnectionOnCancelPush) { if (!VersionUsesHttp3(transport_version())) { return; } @@ -3133,7 +3138,18 @@ "00"); // push ID QuicStreamFrame data3(receive_control_stream_id, /* fin = */ false, offset, cancel_push_frame); - EXPECT_CALL(debug_visitor, OnCancelPushFrameReceived(_)); + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + EXPECT_CALL(*connection_, CloseConnection(QUIC_HTTP_FRAME_ERROR, + "CANCEL_PUSH frame received.", _)) + .WillOnce( + Invoke(connection_, &MockQuicConnection::ReallyCloseConnection)); + EXPECT_CALL(*connection_, + SendConnectionClosePacket(QUIC_HTTP_FRAME_ERROR, _, + "CANCEL_PUSH frame received.")); + } else { + // CANCEL_PUSH is ignored. + EXPECT_CALL(debug_visitor, OnCancelPushFrameReceived(_)); + } session_.OnStreamFrame(data3); }
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc index bb464bd..8b814b8 100644 --- a/quic/core/http/quic_spdy_stream_test.cc +++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -2772,6 +2772,9 @@ // TODO(b/171463363): Remove. TEST_P(QuicSpdyStreamTest, PushPromiseOnDataStream) { Initialize(kShouldProcessData); + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + return; + } if (!UsesHttp3()) { return; } @@ -2802,6 +2805,9 @@ TEST_P(QuicSpdyStreamTest, OnStreamHeaderBlockArgumentDoesNotIncludePushedHeaderBlock) { Initialize(kShouldProcessData); + if (GetQuicReloadableFlag(quic_error_on_http3_push)) { + return; + } if (!UsesHttp3()) { return; }
diff --git a/quic/core/quic_error_codes.h b/quic/core/quic_error_codes.h index 32816b8..9fa422f 100644 --- a/quic/core/quic_error_codes.h +++ b/quic/core/quic_error_codes.h
@@ -459,8 +459,12 @@ // Received multiple close offset. QUIC_STREAM_MULTIPLE_OFFSET = 130, - // Internal error codes for HTTP/3 errors. + // HTTP/3 errors. + + // Frame payload larger than what HttpDecoder is willing to buffer. QUIC_HTTP_FRAME_TOO_LARGE = 131, + // Malformed HTTP/3 frame, or PUSH_PROMISE or CANCEL_PUSH received (which is + // an error because MAX_PUSH_ID is never sent). QUIC_HTTP_FRAME_ERROR = 132, // A frame that is never allowed on a request stream is received. QUIC_HTTP_FRAME_UNEXPECTED_ON_SPDY_STREAM = 133,
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 3f7ca67..4ed13ff 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -44,6 +44,7 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_enable_version_rfcv1, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_encrypted_control_frames, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_encrypted_goaway, true) +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_error_on_http3_push, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_dispatcher_sent_error_code, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_on_stream_reset, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_stateless_reset, true)