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)
