quic: turn up status tests (#15648)

Tagging a bunch of quic test failures with more details.
Adapting the status code tests to QUIC and fixing QUIC code to validate.
minor refactors to core code.

Risk Level: low (minor core refactors)
Testing: new integration tests
Docs Changes: n/a
Release Notes: n/a
part of #14829
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

source/common/http/utility.cc

@@ -442,10 +442,18 @@ std::string Utility::makeSetCookieValue(const std::string& key, const std::strin
 }
 
 uint64_t Utility::getResponseStatus(const ResponseHeaderMap& headers) {
+  auto status = Utility::getResponseStatusNoThrow(headers);
+  if (!status.has_value()) {
+    throw CodecClientException(":status must be specified and a valid unsigned long");
+  }
+  return status.value();
+}
+
+absl::optional<uint64_t> Utility::getResponseStatusNoThrow(const ResponseHeaderMap& headers) {
   const HeaderEntry* header = headers.Status();
   uint64_t response_code;
   if (!header || !absl::SimpleAtoi(headers.getStatusValue(), &response_code)) {
-    throw CodecClientException(":status must be specified and a valid unsigned long");
+    return absl::nullopt;
   }
   return response_code;
 }

source/common/http/utility.h

@@ -259,6 +259,13 @@ std::string makeSetCookieValue(const std::string& key, const std::string& value,
  */
 uint64_t getResponseStatus(const ResponseHeaderMap& headers);
 
+/**
+ * Get the response status from the response headers.
+ * @param headers supplies the headers to get the status from.
+ * @return absl::optional<uint64_t> the response code or absl::nullopt if the headers are invalid.
+ */
+absl::optional<uint64_t> getResponseStatusNoThrow(const ResponseHeaderMap& headers);
+
 /**
  * Determine whether these headers are a valid Upgrade request or response.
  * This function returns true if the following HTTP headers and values are present:

source/common/quic/envoy_quic_client_stream.cc

@@ -126,15 +126,24 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
     return;
   }
   quic::QuicSpdyStream::OnInitialHeadersComplete(fin, frame_len, header_list);
-  ASSERT(headers_decompressed() && !header_list.empty());
+  if (!headers_decompressed() || header_list.empty()) {
+    Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD);
+    return;
+  }
 
   ENVOY_STREAM_LOG(debug, "Received headers: {}.", *this, header_list.DebugString());
   if (fin) {
     end_stream_decoded_ = true;
   }
   std::unique_ptr<Http::ResponseHeaderMapImpl> headers =
       quicHeadersToEnvoyHeaders<Http::ResponseHeaderMapImpl>(header_list);
-  const uint64_t status = Http::Utility::getResponseStatus(*headers);
+  const absl::optional<uint64_t> optional_status =
+      Http::Utility::getResponseStatusNoThrow(*headers);
+  if (!optional_status.has_value()) {
+    Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD);
+    return;
+  }
+  const uint64_t status = optional_status.value();
   if (Http::CodeUtility::is1xx(status)) {
     if (status == enumToInt(Http::Code::SwitchingProtocols)) {
       // HTTP3 doesn't support the HTTP Upgrade mechanism or 101 (Switching Protocols) status code.

source/common/quic/envoy_quic_utils.cc

@@ -82,6 +82,8 @@ Http::StreamResetReason quicRstErrorToEnvoyLocalResetReason(quic::QuicRstStreamE
     return Http::StreamResetReason::LocalRefusedStreamReset;
   case quic::QUIC_STREAM_CONNECTION_ERROR:
     return Http::StreamResetReason::ConnectionFailure;
+  case quic::QUIC_BAD_APPLICATION_PAYLOAD:
+    return Http::StreamResetReason::ProtocolError;
   default:
     return Http::StreamResetReason::LocalReset;
   }

test/common/quic/envoy_quic_client_stream_test.cc

@@ -253,7 +253,7 @@ TEST_P(EnvoyQuicClientStreamTest, ResetUpon101SwitchProtocol) {
   const auto result = quic_stream_->encodeHeaders(request_headers_, false);
   EXPECT_TRUE(result.ok());
 
-  EXPECT_CALL(stream_callbacks_, onResetStream(Http::StreamResetReason::LocalReset, _));
+  EXPECT_CALL(stream_callbacks_, onResetStream(Http::StreamResetReason::ProtocolError, _));
   // Receive several 10x headers, only the first 100 Continue header should be
   // delivered.
   if (quic::VersionUsesHttp3(quic_version_.transport_version)) {

test/integration/protocol_integration_test.cc

@@ -341,7 +341,7 @@ TEST_P(ProtocolIntegrationTest, ResponseWithHostHeader) {
 
 // Tests missing headers needed for H/1 codec first line.
 TEST_P(DownstreamProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) {
-  EXCLUDE_UPSTREAM_HTTP3; // buffer bug.
+  EXCLUDE_UPSTREAM_HTTP3; // tsan flake?
   useAccessLog("%RESPONSE_CODE_DETAILS%");
   config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": "
                            "type.googleapis.com/google.protobuf.Empty } }");
@@ -377,7 +377,7 @@ TEST_P(DownstreamProtocolIntegrationTest, FaultyFilterWithConnect) {
   // TODO(danzh) re-enable after plumbing through http2 option
   // "allow_connect".
   EXCLUDE_DOWNSTREAM_HTTP3;
-  EXCLUDE_UPSTREAM_HTTP3; // buffer bug.
+  EXCLUDE_UPSTREAM_HTTP3; // use after free.
   // Faulty filter that removed host in a CONNECT request.
   config_helper_.addConfigModifier(
       [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
@@ -1051,7 +1051,7 @@ TEST_P(ProtocolIntegrationTest, HittingEncoderFilterLimit) {
 // connection error is not detected under these circumstances.
 #if !defined(__APPLE__)
 TEST_P(ProtocolIntegrationTest, 100ContinueAndClose) {
-  EXCLUDE_UPSTREAM_HTTP3;
+  EXCLUDE_UPSTREAM_HTTP3; // CI fails with 503 in access logs.
   testEnvoyHandling100Continue(false, "", true);
 }
 #endif
@@ -1081,7 +1081,8 @@ TEST_P(ProtocolIntegrationTest, EnvoyProxyingLateMultiple1xx) {
 TEST_P(ProtocolIntegrationTest, TwoRequests) { testTwoRequests(); }
 
 TEST_P(ProtocolIntegrationTest, TwoRequestsWithForcedBackup) {
-  EXCLUDE_UPSTREAM_HTTP3; // Hits assert in switchStreamBlockState.
+  // hits assert "Upper stream buffer limit is reached before response body is delivered."
+  EXCLUDE_UPSTREAM_HTTP3;
   testTwoRequests(true);
 }
 
@@ -1260,57 +1261,54 @@ TEST_P(ProtocolIntegrationTest, 304WithBody) {
 }
 
 TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) {
-  EXCLUDE_UPSTREAM_HTTP3; // Protocol-specific framing.
+  useAccessLog("%RESPONSE_CODE_DETAILS%");
   initialize();
 
-  FakeRawConnectionPtr fake_upstream_connection;
-  // HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames
   codec_client_ = makeHttpConnection(lookupPort("http"));
   auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
-
-  ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
-  ASSERT(fake_upstream_connection != nullptr);
-
   if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) {
+    // The http1 codec won't send illegal status codes so send raw HTTP/1.1
+    FakeRawConnectionPtr fake_upstream_connection;
+    ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
+    ASSERT(fake_upstream_connection != nullptr);
     ASSERT_TRUE(fake_upstream_connection->write(
         "HTTP/1.1 11111111111111111111111111111111111111111111111111111111111111111 OK\r\n",
         false));
     ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
   } else {
-    Http::Http2::Http2Frame::SettingsFlags settings_flags =
-        static_cast<Http::Http2::Http2Frame::SettingsFlags>(0);
-    Http2Frame setting_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags);
-    // Ack settings
-    settings_flags = static_cast<Http::Http2::Http2Frame::SettingsFlags>(1); // ack
-    Http2Frame ack_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags);
-    ASSERT(fake_upstream_connection->write(std::string(setting_frame))); // empty settings
-    ASSERT(fake_upstream_connection->write(std::string(ack_frame)));     // ack setting
-    Http::Http2::Http2Frame overflowed_status = Http::Http2::Http2Frame::makeHeadersFrameWithStatus(
-        "11111111111111111111111111111111111111111111111111111111111111111", 0);
-    ASSERT_TRUE(fake_upstream_connection->write(std::string(overflowed_status)));
+    waitForNextUpstreamRequest();
+    default_response_headers_.setStatus(
+        "11111111111111111111111111111111111111111111111111111111111111111");
+    upstream_request_->encodeHeaders(default_response_headers_, false);
+    if (upstreamProtocol() == FakeHttpConnection::Type::HTTP2) {
+      ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
+    } else {
+      // TODO(#14829) QUIC won't disconnect on invalid messaging.
+      ASSERT_TRUE(upstream_request_->waitForReset());
+    }
   }
 
-  ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
   response->waitForEndStream();
   EXPECT_EQ("502", response->headers().getStatusValue());
+  EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("protocol_error"));
 }
 
 TEST_P(ProtocolIntegrationTest, MissingStatus) {
-  EXCLUDE_UPSTREAM_HTTP3; // Protocol-specific framing.
   initialize();
 
-  FakeRawConnectionPtr fake_upstream_connection;
   // HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames
   codec_client_ = makeHttpConnection(lookupPort("http"));
   auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
 
-  ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
-  ASSERT(fake_upstream_connection != nullptr);
-
   if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) {
+    FakeRawConnectionPtr fake_upstream_connection;
+    ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
+    ASSERT(fake_upstream_connection != nullptr);
     ASSERT_TRUE(fake_upstream_connection->write("HTTP/1.1 OK\r\n", false));
     ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
-  } else {
+  } else if (upstreamProtocol() == FakeHttpConnection::Type::HTTP2) {
+    FakeRawConnectionPtr fake_upstream_connection;
+    ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection));
     Http::Http2::Http2Frame::SettingsFlags settings_flags =
         static_cast<Http::Http2::Http2Frame::SettingsFlags>(0);
     Http2Frame setting_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags);
@@ -1321,9 +1319,15 @@ TEST_P(ProtocolIntegrationTest, MissingStatus) {
     ASSERT(fake_upstream_connection->write(std::string(ack_frame)));     // ack setting
     Http::Http2::Http2Frame missing_status = Http::Http2::Http2Frame::makeHeadersFrameNoStatus(0);
     ASSERT_TRUE(fake_upstream_connection->write(std::string(missing_status)));
+    ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
+  } else {
+    waitForNextUpstreamRequest();
+    default_response_headers_.removeStatus();
+    upstream_request_->encodeHeaders(default_response_headers_, false);
+    // TODO(#14829) QUIC won't disconnect on invalid messaging.
+    ASSERT_TRUE(upstream_request_->waitForReset());
   }
 
-  ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
   response->waitForEndStream();
   EXPECT_EQ("502", response->headers().getStatusValue());
 }
@@ -1332,7 +1336,7 @@ TEST_P(ProtocolIntegrationTest, MissingStatus) {
 TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingConcatenated) {
   // TODO(danzh) re-enable this test after quic headers size become configurable.
   EXCLUDE_DOWNSTREAM_HTTP3
-  EXCLUDE_UPSTREAM_HTTP3;
+  EXCLUDE_UPSTREAM_HTTP3; // QUIC_STREAM_EXCESSIVE_LOAD
   initialize();
 
   codec_client_ = makeHttpConnection(lookupPort("http"));
@@ -1357,7 +1361,7 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingConcatenated) {
 TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingMany) {
   // TODO(danzh) re-enable this test after quic headers size become configurable.
   EXCLUDE_DOWNSTREAM_HTTP3
-  EXCLUDE_UPSTREAM_HTTP3;
+  EXCLUDE_UPSTREAM_HTTP3; // QUIC_STREAM_EXCESSIVE_LOAD
   // Set header count limit to 2010.
   uint32_t max_count = 2010;
   config_helper_.addConfigModifier(
@@ -1570,7 +1574,7 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeRequestUrlRejected) {
 TEST_P(DownstreamProtocolIntegrationTest, LargeRequestUrlAccepted) {
   // TODO(danzh) re-enable this test after quic headers size become configurable.
   EXCLUDE_DOWNSTREAM_HTTP3
-  EXCLUDE_UPSTREAM_HTTP3;
+  EXCLUDE_UPSTREAM_HTTP3; // requires configurable header limits.
   // Send one 95 kB URL with limit 96 kB headers.
   testLargeRequestUrl(95, 96);
 }
@@ -1582,7 +1586,7 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeRequestHeadersRejected) {
 
 TEST_P(DownstreamProtocolIntegrationTest, LargeRequestHeadersAccepted) {
   EXCLUDE_DOWNSTREAM_HTTP3
-  EXCLUDE_UPSTREAM_HTTP3;
+  EXCLUDE_UPSTREAM_HTTP3; // requires configurable header limits.
   // Send one 95 kB header with limit 96 kB and 100 headers.
   testLargeRequestHeaders(95, 1, 96, 100);
 }
@@ -1602,7 +1606,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestHeadersAccepted) {
 TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) {
   // QUICHE doesn't limit number of headers.
   EXCLUDE_DOWNSTREAM_HTTP3
-  EXCLUDE_UPSTREAM_HTTP3; // buffer bug.
+  EXCLUDE_UPSTREAM_HTTP3; // CI asan use-after-free
   // Default header (and trailer) count limit is 100.
   config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1());
   config_helper_.addConfigModifier(setEnableUpstreamTrailersHttp1());
@@ -1630,7 +1634,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) {
 }
 
 TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersAccepted) {
-  EXCLUDE_UPSTREAM_HTTP3;
+  EXCLUDE_UPSTREAM_HTTP3; // assert failure: validHeaderString
   // Set header (and trailer) count limit to 200.
   uint32_t max_count = 200;
   config_helper_.addConfigModifier(