commit e87010ff958c9397ee861e00d7747de6f3938e19
author QUICHE team <quiche-dev@google.com> Mon Dec 06 16:08:24 2021 -0800
committer Copybara-Service <copybara-worker@google.com> Mon Dec 06 16:09:06 2021 -0800
tree 994747ed520567c39082e073b69bc6ce7aacaaf2
parent 971a7c5e3fb407cce1fc1de1ea148b0159aff57c

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

http2/adapter/oghttp2_adapter_test.cc

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.

http2/adapter/oghttp2_session.cc

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) {