Reject 304 responses with bodies in oghttp2.
This CL updates oghttp2 to track, based on a stream's headers, whether the
stream can receive a body following those headers. That tracking is used to
reject 304 responses with bodies in this CL and can be expanded to other
scenarios (e.g., with other response codes or request methods) in the future.
ProtocolIntegrationTest.304WithBody with oghttp2:
http://sponge2/b1fcc5f6-9628-4579-b27b-25949278af8e
PiperOrigin-RevId: 423337666
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc
index 9771f3c..84387c1 100644
--- a/http2/adapter/nghttp2_adapter_test.cc
+++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -390,6 +390,67 @@
spdy::SpdyFrameType::RST_STREAM}));
}
+TEST(NgHttp2AdapterTest, ClientHandles304WithContent) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor);
+
+ testing::InSequence s;
+
+ const std::vector<Header> headers1 =
+ ToHeaders({{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}});
+
+ const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr);
+ ASSERT_GT(stream_id1, 0);
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0));
+
+ int result = adapter->Send();
+ EXPECT_EQ(0, result);
+ visitor.Clear();
+
+ const std::string stream_frames =
+ TestFrameSequence()
+ .ServerPreface()
+ .Headers(1, {{":status", "304"}, {"content-length", "2"}},
+ /*fin=*/false)
+ .Data(1, "hi")
+ .Serialize();
+
+ // Server preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":status", "304"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, "content-length", "2"));
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0));
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, 2));
+
+ const int64_t stream_result = adapter->ProcessBytes(stream_frames);
+ EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result));
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 1, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+ EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::PROTOCOL_ERROR));
+
+ EXPECT_TRUE(adapter->want_write());
+ result = adapter->Send();
+ EXPECT_EQ(0, result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::RST_STREAM}));
+}
+
TEST(NgHttp2AdapterTest, ClientHandles304WithContentLength) {
DataSavingVisitor visitor;
auto adapter = NgHttp2Adapter::CreateClientAdapter(visitor);
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc
index 47638dd..58b33f0 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -469,6 +469,70 @@
spdy::SpdyFrameType::RST_STREAM}));
}
+TEST(OgHttp2AdapterClientTest, ClientHandles304WithContent) {
+ DataSavingVisitor visitor;
+ OgHttp2Adapter::Options options{.perspective = Perspective::kClient};
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
+
+ testing::InSequence s;
+
+ const std::vector<Header> headers1 =
+ ToHeaders({{":method", "GET"},
+ {":scheme", "http"},
+ {":authority", "example.com"},
+ {":path", "/this/is/request/one"}});
+
+ const int32_t stream_id1 = adapter->SubmitRequest(headers1, nullptr, nullptr);
+ ASSERT_GT(stream_id1, 0);
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5));
+ EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0));
+
+ int result = adapter->Send();
+ EXPECT_EQ(0, result);
+ visitor.Clear();
+
+ const std::string stream_frames =
+ TestFrameSequence()
+ .ServerPreface()
+ .Headers(1, {{":status", "304"}, {"content-length", "2"}},
+ /*fin=*/false)
+ .Data(1, "hi")
+ .Serialize();
+
+ // Server preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, ":status", "304"));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, "content-length", "2"));
+ EXPECT_CALL(visitor, OnEndHeadersForStream(1));
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 0));
+ EXPECT_CALL(visitor, OnBeginDataForStream(1, 2));
+
+ const int64_t stream_result = adapter->ProcessBytes(stream_frames);
+ EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result));
+
+ EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 1, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+ EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR));
+
+ EXPECT_TRUE(adapter->want_write());
+ result = adapter->Send();
+ EXPECT_EQ(0, result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS,
+ spdy::SpdyFrameType::RST_STREAM}));
+}
+
TEST(OgHttp2AdapterClientTest, ClientHandles304WithContentLength) {
DataSavingVisitor visitor;
OgHttp2Adapter::Options options{.perspective = Perspective::kClient};
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc
index 547ea15..256e23d 100644
--- a/http2/adapter/oghttp2_session.cc
+++ b/http2/adapter/oghttp2_session.cc
@@ -285,6 +285,23 @@
frame_contains_fin_ = false;
}
+// TODO(diannahu): Add checks for other response codes and request methods.
+bool OgHttp2Session::PassthroughHeadersHandler::CanReceiveBody() const {
+ switch (header_type()) {
+ case HeaderType::REQUEST_TRAILER:
+ case HeaderType::RESPONSE_TRAILER:
+ case HeaderType::RESPONSE_100:
+ return false;
+ case HeaderType::RESPONSE:
+ // 304 responses should not have a body:
+ // https://httpwg.org/specs/rfc7230.html#rfc.section.3.3.2
+ return status_header() != "304";
+ case HeaderType::REQUEST:
+ return true;
+ }
+ return true;
+}
+
// A visitor that extracts an int64_t from each type of a ProcessBytesResult.
struct OgHttp2Session::ProcessBytesResultVisitor {
int64_t operator()(const int64_t bytes) const { return bytes; }
@@ -1021,6 +1038,12 @@
decoder_.StopProcessing();
}
+ if (!iter->second.can_receive_body && length > 0) {
+ EnqueueFrame(absl::make_unique<spdy::SpdyRstStreamIR>(
+ stream_id, spdy::ERROR_CODE_PROTOCOL_ERROR));
+ return;
+ }
+
// Validate against the content-length if it exists.
if (iter->second.remaining_content_length.has_value()) {
if (length > *iter->second.remaining_content_length) {
@@ -1114,17 +1137,16 @@
headers_handler_.status_header()[0] == '1') {
// If response headers carried a 1xx response code, final response headers
// should still be forthcoming.
- it->second.received_header_type = HeaderType::RESPONSE_100;
- } else {
- it->second.received_header_type = headers_handler_.header_type();
+ headers_handler_.set_header_type(HeaderType::RESPONSE_100);
}
- if (headers_handler_.header_type() == HeaderType::REQUEST ||
- (headers_handler_.header_type() == HeaderType::RESPONSE &&
- headers_handler_.status_header() != "304")) {
- // 304 response content-length values should be ignored:
- // https://httpwg.org/specs/rfc7230.html#rfc.section.3.3.2
+ it->second.received_header_type = headers_handler_.header_type();
+
+ // Track the content-length if the headers indicate that a body can follow.
+ it->second.can_receive_body = headers_handler_.CanReceiveBody();
+ if (it->second.can_receive_body) {
it->second.remaining_content_length = headers_handler_.content_length();
}
+
headers_handler_.set_stream_id(0);
}
}
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h
index a1b96d2..53df0d4 100644
--- a/http2/adapter/oghttp2_session.h
+++ b/http2/adapter/oghttp2_session.h
@@ -228,6 +228,7 @@
bool half_closed_remote = false;
// Indicates that `outbound_body` temporarily cannot produce data.
bool data_deferred = false;
+ bool can_receive_body = true;
};
using StreamStateMap = absl::flat_hash_map<Http2StreamId, StreamState>;
@@ -258,7 +259,7 @@
void OnHeader(absl::string_view key, absl::string_view value) override;
void OnHeaderBlockEnd(size_t /* uncompressed_header_bytes */,
size_t /* compressed_header_bytes */) override;
- absl::string_view status_header() {
+ absl::string_view status_header() const {
QUICHE_DCHECK(type_ == HeaderType::RESPONSE ||
type_ == HeaderType::RESPONSE_100);
return validator_.status_header();
@@ -270,6 +271,7 @@
void SetMaxFieldSize(uint32_t field_size) {
validator_.SetMaxFieldSize(field_size);
}
+ bool CanReceiveBody() const;
private:
OgHttp2Session& session_;
