Enforce that WEBTRANSPORT_STREAM is always sent at the beginning of the stream if draft-07 or later is used. This also fixes the way parsing WEBTRANSPORT_STREAM is enabled. Currently, we enable it iff the local peer is willing to use WebTransport (which is incorrect). After this CL, the code follows the following (correct and tested) logic: 0. Always wait for peer SETTINGS to be available. 1. If WebTransport is not supported, WEBTRANSPORT_STREAM is treated as a regular frame. 2. If WebTransport is at draft-02, WEBTRANSPORT_STREAM can occur at any position of the stream. 3. If WebTransport is at draft-07 or later, WEBTRANSPORT_STREAM can only occur at the beginning of the stream. PiperOrigin-RevId: 542272544
diff --git a/quiche/quic/core/http/http_decoder.cc b/quiche/quic/core/http/http_decoder.cc index 5facad8..94cc367 100644 --- a/quiche/quic/core/http/http_decoder.cc +++ b/quiche/quic/core/http/http_decoder.cc
@@ -31,10 +31,9 @@ } // anonymous namespace -HttpDecoder::HttpDecoder(Visitor* visitor) : HttpDecoder(visitor, Options()) {} -HttpDecoder::HttpDecoder(Visitor* visitor, Options options) +HttpDecoder::HttpDecoder(Visitor* visitor) : visitor_(visitor), - allow_web_transport_stream_(options.allow_web_transport_stream), + allow_web_transport_stream_(false), state_(STATE_READING_FRAME_TYPE), current_frame_type_(0), current_length_field_length_(0),
diff --git a/quiche/quic/core/http/http_decoder.h b/quiche/quic/core/http/http_decoder.h index 0e49c3f..d582841 100644 --- a/quiche/quic/core/http/http_decoder.h +++ b/quiche/quic/core/http/http_decoder.h
@@ -27,11 +27,6 @@ // session. class QUIC_EXPORT_PRIVATE HttpDecoder { public: - struct QUIC_EXPORT_PRIVATE Options { - // Indicates that WEBTRANSPORT_STREAM should be parsed. - bool allow_web_transport_stream = false; - }; - class QUIC_EXPORT_PRIVATE Visitor { public: virtual ~Visitor() {} @@ -119,7 +114,6 @@ // |visitor| must be non-null, and must outlive HttpDecoder. explicit HttpDecoder(Visitor* visitor); - explicit HttpDecoder(Visitor* visitor, Options options); ~HttpDecoder(); @@ -146,6 +140,9 @@ // Returns true if input data processed so far ends on a frame boundary. bool AtFrameBoundary() const { return state_ == STATE_READING_FRAME_TYPE; } + // Indicates that WEBTRANSPORT_STREAM should be parsed. + void EnableWebTransportStreamParsing() { allow_web_transport_stream_ = true; } + std::string DebugString() const; private:
diff --git a/quiche/quic/core/http/http_decoder_test.cc b/quiche/quic/core/http/http_decoder_test.cc index 79544b3..252b637 100644 --- a/quiche/quic/core/http/http_decoder_test.cc +++ b/quiche/quic/core/http/http_decoder_test.cc
@@ -995,10 +995,9 @@ } TEST(HttpDecoderTestNoFixture, WebTransportStream) { - HttpDecoder::Options options; - options.allow_web_transport_stream = true; testing::StrictMock<MockHttpDecoderVisitor> visitor; - HttpDecoder decoder(&visitor, options); + HttpDecoder decoder(&visitor); + decoder.EnableWebTransportStreamParsing(); // WebTransport stream for session ID 0x104, with four bytes of extra data. std::string input = absl::HexStringToBytes("40414104ffffffff"); @@ -1008,10 +1007,9 @@ } TEST(HttpDecoderTestNoFixture, WebTransportStreamError) { - HttpDecoder::Options options; - options.allow_web_transport_stream = true; testing::StrictMock<MockHttpDecoderVisitor> visitor; - HttpDecoder decoder(&visitor, options); + HttpDecoder decoder(&visitor); + decoder.EnableWebTransportStreamParsing(); std::string input = absl::HexStringToBytes("404100"); EXPECT_CALL(visitor, OnWebTransportStreamFrameType(_, _));
diff --git a/quiche/quic/core/http/quic_spdy_stream.cc b/quiche/quic/core/http/quic_spdy_stream.cc index 234b861..bcd5493 100644 --- a/quiche/quic/core/http/quic_spdy_stream.cc +++ b/quiche/quic/core/http/quic_spdy_stream.cc
@@ -168,16 +168,6 @@ : "Client:" \ " ") -namespace { -HttpDecoder::Options HttpDecoderOptionsForBidiStream( - QuicSpdySession* spdy_session) { - HttpDecoder::Options options; - options.allow_web_transport_stream = - spdy_session->WillNegotiateWebTransport(); - return options; -} -} // namespace - QuicSpdyStream::QuicSpdyStream(QuicStreamId id, QuicSpdySession* spdy_session, StreamType type) : QuicStream(id, spdy_session, /*is_static=*/false, type), @@ -191,8 +181,7 @@ trailers_decompressed_(false), trailers_consumed_(false), http_decoder_visitor_(std::make_unique<HttpDecoderVisitor>(this)), - decoder_(http_decoder_visitor_.get(), - HttpDecoderOptionsForBidiStream(spdy_session)), + decoder_(http_decoder_visitor_.get()), sequencer_offset_(0), is_decoder_processing_input_(false), ack_listener_(nullptr), @@ -822,6 +811,13 @@ return; } + if (spdy_session_->SupportsWebTransport()) { + // We do this here, since at this point, we have passed the + // ShouldProcessIncomingRequests() check above, meaning we know for a fact + // if we should be parsing WEBTRANSPORT_STREAM or not. + decoder_.EnableWebTransportStreamParsing(); + } + iovec iov; while (session()->connection()->connected() && !reading_stopped() && decoder_.error() == QUIC_NO_ERROR) { @@ -1124,19 +1120,36 @@ QuicByteCount header_length, WebTransportSessionId session_id) { QUIC_DVLOG(1) << ENDPOINT << " Received WEBTRANSPORT_STREAM on stream " << id() << " for session " << session_id; + QuicStreamOffset offset = sequencer()->NumBytesConsumed(); sequencer()->MarkConsumed(header_length); - if (headers_payload_length_ > 0 || headers_decompressed_) { - std::string error = - absl::StrCat("Stream ", id(), - " attempted to convert itself into a WebTransport data " - "stream, but it already has HTTP data on it"); - QUIC_PEER_BUG(WEBTRANSPORT_STREAM received on HTTP request) - << ENDPOINT << error; - OnUnrecoverableError(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM, - error); - return; + absl::optional<WebTransportHttp3Version> version = + spdy_session_->SupportedWebTransportVersion(); + QUICHE_DCHECK(version.has_value()); + if (version == WebTransportHttp3Version::kDraft02) { + if (headers_payload_length_ > 0 || headers_decompressed_) { + std::string error = + absl::StrCat("Stream ", id(), + " attempted to convert itself into a WebTransport data " + "stream, but it already has HTTP data on it"); + QUIC_PEER_BUG(WEBTRANSPORT_STREAM received on HTTP request) + << ENDPOINT << error; + OnUnrecoverableError(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM, + error); + return; + } + } else { + if (offset > 0) { + std::string error = + absl::StrCat("Stream ", id(), + " received WEBTRANSPORT_STREAM at a non-zero offset"); + QUIC_DLOG(ERROR) << ENDPOINT << error; + OnUnrecoverableError(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM, + error); + return; + } } + if (QuicUtils::IsOutgoingStreamId(spdy_session_->version(), id(), spdy_session_->perspective())) { std::string error = absl::StrCat(
diff --git a/quiche/quic/core/http/quic_spdy_stream_test.cc b/quiche/quic/core/http/quic_spdy_stream_test.cc index 024eefd..5fdd890 100644 --- a/quiche/quic/core/http/quic_spdy_stream_test.cc +++ b/quiche/quic/core/http/quic_spdy_stream_test.cc
@@ -15,7 +15,9 @@ #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "quiche/quic/core/crypto/null_encrypter.h" +#include "quiche/quic/core/http/http_constants.h" #include "quiche/quic/core/http/http_encoder.h" +#include "quiche/quic/core/http/http_frames.h" #include "quiche/quic/core/http/quic_spdy_session.h" #include "quiche/quic/core/http/spdy_utils.h" #include "quiche/quic/core/http/web_transport_http3.h" @@ -51,6 +53,7 @@ using testing::AtLeast; using testing::DoAll; using testing::ElementsAre; +using testing::HasSubstr; using testing::Invoke; using testing::InvokeWithoutArgs; using testing::MatchesRegex; @@ -3112,6 +3115,98 @@ EXPECT_EQ(stream_->id(), stream_->web_transport()->id()); } +TEST_P(QuicSpdyStreamTest, IncomingWebTransportStreamWhenUnsupported) { + if (!UsesHttp3()) { + return; + } + + Initialize(kShouldProcessData); + // Support WebTransport locally, but not by the peer. + session_->set_local_http_datagram_support(HttpDatagramSupport::kRfc); + session_->EnableWebTransport(); + session_->OnSettingsFrame(SettingsFrame()); + + StrictMock<MockHttp3DebugVisitor> debug_visitor; + session_->set_debug_visitor(&debug_visitor); + + std::string webtransport_stream_frame = + absl::HexStringToBytes("40410400000000"); + QuicStreamFrame stream_frame(stream_->id(), /* fin = */ false, + /* offset = */ 0, webtransport_stream_frame); + + EXPECT_CALL(debug_visitor, OnUnknownFrameReceived(stream_->id(), 0x41, 4)); + stream_->OnStreamFrame(stream_frame); + EXPECT_TRUE(stream_->web_transport_stream() == nullptr); +} + +TEST_P(QuicSpdyStreamTest, IncomingWebTransportStream) { + if (!UsesHttp3()) { + return; + } + + Initialize(kShouldProcessData); + session_->set_local_http_datagram_support(HttpDatagramSupport::kRfc); + session_->EnableWebTransport(); + SettingsFrame settings; + settings.values[SETTINGS_WEBTRANS_MAX_SESSIONS_DRAFT07] = 10; + settings.values[SETTINGS_H3_DATAGRAM] = 1; + session_->OnSettingsFrame(settings); + + std::string webtransport_stream_frame = absl::HexStringToBytes("404110"); + QuicStreamFrame stream_frame(stream_->id(), /* fin = */ false, + /* offset = */ 0, webtransport_stream_frame); + + EXPECT_CALL(*session_, CreateIncomingStream(0x10)); + stream_->OnStreamFrame(stream_frame); + EXPECT_TRUE(stream_->web_transport_stream() != nullptr); +} + +TEST_P(QuicSpdyStreamTest, IncomingWebTransportStreamWithPaddingDraft02) { + if (!UsesHttp3()) { + return; + } + + Initialize(kShouldProcessData); + session_->set_local_http_datagram_support(HttpDatagramSupport::kRfc); + session_->EnableWebTransport(); + SettingsFrame settings; + settings.values[SETTINGS_WEBTRANS_DRAFT00] = 1; + settings.values[SETTINGS_H3_DATAGRAM] = 1; + session_->OnSettingsFrame(settings); + + std::string webtransport_stream_frame = absl::HexStringToBytes("2100404110"); + QuicStreamFrame stream_frame(stream_->id(), /* fin = */ false, + /* offset = */ 0, webtransport_stream_frame); + + EXPECT_CALL(*session_, CreateIncomingStream(0x10)); + stream_->OnStreamFrame(stream_frame); + EXPECT_TRUE(stream_->web_transport_stream() != nullptr); +} + +TEST_P(QuicSpdyStreamTest, IncomingWebTransportStreamWithPaddingDraft07) { + if (!UsesHttp3()) { + return; + } + + Initialize(kShouldProcessData); + session_->set_local_http_datagram_support(HttpDatagramSupport::kRfc); + session_->EnableWebTransport(); + SettingsFrame settings; + settings.values[SETTINGS_WEBTRANS_MAX_SESSIONS_DRAFT07] = 10; + settings.values[SETTINGS_H3_DATAGRAM] = 1; + session_->OnSettingsFrame(settings); + + std::string webtransport_stream_frame = absl::HexStringToBytes("2100404110"); + QuicStreamFrame stream_frame(stream_->id(), /* fin = */ false, + /* offset = */ 0, webtransport_stream_frame); + + EXPECT_CALL(*connection_, + CloseConnection(QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM, + HasSubstr("non-zero offset"), _)); + stream_->OnStreamFrame(stream_frame); + EXPECT_TRUE(stream_->web_transport_stream() == nullptr); +} + TEST_P(QuicSpdyStreamTest, ReceiveHttpDatagram) { if (!UsesHttp3()) { return;