Make oghttp2 reject connection-specific headers during header validation.
Discovered while thinking about GFE3 behavior (rejecting these headers with
nghttp2), thanks to ++pgal@'s investigation. This change brings oghttp2 into
further alignment with nghttp2:
- Request: http://google3/third_party/nghttp2/src/lib/nghttp2_http.c;l=203-209;rcl=436881423
- Response: http://google3/third_party/nghttp2/src/lib/nghttp2_http.c;l=283-289;rcl=436881423
PiperOrigin-RevId: 437266121
diff --git a/http2/adapter/header_validator.cc b/http2/adapter/header_validator.cc
index 82424f6..1220634 100644
--- a/http2/adapter/header_validator.cc
+++ b/http2/adapter/header_validator.cc
@@ -4,6 +4,7 @@
#include "absl/strings/escaping.h"
#include "absl/strings/numbers.h"
+#include "http2/http2_constants.h"
#include "common/platform/api/quiche_logging.h"
namespace http2 {
@@ -182,6 +183,10 @@
}
} else if (key == "te" && value != "trailers") {
return HEADER_FIELD_INVALID;
+ } else if (key == "upgrade" || GetInvalidHttp2HeaderSet().contains(key)) {
+ // TODO(b/78024822): Remove the "upgrade" here once it's added to
+ // GetInvalidHttp2HeaderSet().
+ return HEADER_FIELD_INVALID;
}
return HEADER_OK;
}
diff --git a/http2/adapter/header_validator_test.cc b/http2/adapter/header_validator_test.cc
index d1fd642..85ff3bb 100644
--- a/http2/adapter/header_validator_test.cc
+++ b/http2/adapter/header_validator_test.cc
@@ -1,5 +1,8 @@
#include "http2/adapter/header_validator.h"
+#include <utility>
+#include <vector>
+
#include "absl/strings/str_cat.h"
#include "absl/types/optional.h"
#include "common/platform/api/quiche_test.h"
@@ -487,6 +490,24 @@
v.ValidateSingleHeader("te", "trailers, deflate"));
}
+TEST(HeaderValidatorTest, ConnectionSpecificHeaders) {
+ const std::vector<Header> connection_headers = {
+ {"connection", "keep-alive"}, {"proxy-connection", "keep-alive"},
+ {"keep-alive", "timeout=42"}, {"transfer-encoding", "chunked"},
+ {"upgrade", "h2c"},
+ };
+ for (const auto& [connection_key, connection_value] : connection_headers) {
+ HeaderValidator v;
+ v.StartHeaderBlock();
+ for (const auto& [sample_key, sample_value] : kSampleRequestPseudoheaders) {
+ EXPECT_EQ(HeaderValidator::HEADER_OK,
+ v.ValidateSingleHeader(sample_key, sample_value));
+ }
+ EXPECT_EQ(HeaderValidator::HEADER_FIELD_INVALID,
+ v.ValidateSingleHeader(connection_key, connection_value));
+ }
+}
+
} // namespace test
} // namespace adapter
} // namespace http2
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc
index 645575e..a90e7ed 100644
--- a/http2/adapter/nghttp2_adapter_test.cc
+++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -6082,6 +6082,150 @@
SpdyFrameType::RST_STREAM}));
}
+TEST(NgHttp2AdapterTest, ServerHandlesConnectionSpecificHeaders) {
+ DataSavingVisitor visitor;
+ auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
+
+ testing::InSequence s;
+
+ const std::string stream_frames =
+ TestFrameSequence()
+ .ClientPreface()
+ .Headers(1,
+ {{":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/"},
+ {":method", "GET"},
+ {"connection", "keep-alive"}},
+ /*fin=*/true)
+ .Headers(3,
+ {{":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/"},
+ {":method", "GET"},
+ {"proxy-connection", "keep-alive"}},
+ /*fin=*/true)
+ .Headers(5,
+ {{":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/"},
+ {":method", "GET"},
+ {"keep-alive", "timeout=42"}},
+ /*fin=*/true)
+ .Headers(7,
+ {{":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/"},
+ {":method", "GET"},
+ {"transfer-encoding", "chunked"}},
+ /*fin=*/true)
+ .Headers(9,
+ {{":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/"},
+ {":method", "GET"},
+ {"upgrade", "h2c"}},
+ /*fin=*/true)
+ .Serialize();
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ // All streams contain a connection-specific header and should be rejected.
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4);
+ EXPECT_CALL(
+ visitor,
+ OnErrorDebug("Invalid HTTP header field was received: frame type: 1, "
+ "stream: 1, name: [connection], value: [keep-alive]"));
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
+ EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(3));
+ EXPECT_CALL(visitor, OnHeaderForStream(3, _, _)).Times(4);
+ EXPECT_CALL(
+ visitor,
+ OnErrorDebug("Invalid HTTP header field was received: frame type: 1, "
+ "stream: 3, name: [proxy-connection], value: [keep-alive]"));
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(3, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
+ EXPECT_CALL(visitor, OnFrameHeader(5, _, HEADERS, 5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(5));
+ EXPECT_CALL(visitor, OnHeaderForStream(5, _, _)).Times(4);
+ EXPECT_CALL(
+ visitor,
+ OnErrorDebug("Invalid HTTP header field was received: frame type: 1, "
+ "stream: 5, name: [keep-alive], value: [timeout=42]"));
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(5, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
+ EXPECT_CALL(visitor, OnFrameHeader(7, _, HEADERS, 5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(7));
+ EXPECT_CALL(visitor, OnHeaderForStream(7, _, _)).Times(4);
+ EXPECT_CALL(
+ visitor,
+ OnErrorDebug("Invalid HTTP header field was received: frame type: 1, "
+ "stream: 7, name: [transfer-encoding], value: [chunked]"));
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(7, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
+ EXPECT_CALL(visitor, OnFrameHeader(9, _, HEADERS, 5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(9));
+ EXPECT_CALL(visitor, OnHeaderForStream(9, _, _)).Times(4);
+ EXPECT_CALL(
+ visitor,
+ OnErrorDebug("Invalid HTTP header field was received: frame type: 1, "
+ "stream: 9, name: [upgrade], value: [h2c]"));
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(9, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
+
+ 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_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 3, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+ EXPECT_CALL(visitor, OnCloseStream(3, Http2ErrorCode::PROTOCOL_ERROR));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 5, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 5, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+ EXPECT_CALL(visitor, OnCloseStream(5, Http2ErrorCode::PROTOCOL_ERROR));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 7, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 7, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+ EXPECT_CALL(visitor, OnCloseStream(7, Http2ErrorCode::PROTOCOL_ERROR));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 9, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 9, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+ EXPECT_CALL(visitor, OnCloseStream(9, Http2ErrorCode::PROTOCOL_ERROR));
+
+ EXPECT_TRUE(adapter->want_write());
+ int result = adapter->Send();
+ EXPECT_EQ(0, result);
+ EXPECT_THAT(
+ visitor.data(),
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::RST_STREAM,
+ SpdyFrameType::RST_STREAM, SpdyFrameType::RST_STREAM,
+ SpdyFrameType::RST_STREAM, SpdyFrameType::RST_STREAM}));
+}
+
} // namespace
} // namespace test
} // namespace adapter
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc
index 775609a..9e3b1cc 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -6433,6 +6433,134 @@
SpdyFrameType::RST_STREAM}));
}
+TEST(OgHttp2AdapterTest, ServerHandlesConnectionSpecificHeaders) {
+ DataSavingVisitor visitor;
+ OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
+ auto adapter = OgHttp2Adapter::Create(visitor, options);
+
+ testing::InSequence s;
+
+ const std::string stream_frames =
+ TestFrameSequence()
+ .ClientPreface()
+ .Headers(1,
+ {{":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/"},
+ {":method", "GET"},
+ {"connection", "keep-alive"}},
+ /*fin=*/true)
+ .Headers(3,
+ {{":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/"},
+ {":method", "GET"},
+ {"proxy-connection", "keep-alive"}},
+ /*fin=*/true)
+ .Headers(5,
+ {{":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/"},
+ {":method", "GET"},
+ {"keep-alive", "timeout=42"}},
+ /*fin=*/true)
+ .Headers(7,
+ {{":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/"},
+ {":method", "GET"},
+ {"transfer-encoding", "chunked"}},
+ /*fin=*/true)
+ .Headers(9,
+ {{":scheme", "https"},
+ {":authority", "example.com"},
+ {":path", "/"},
+ {":method", "GET"},
+ {"upgrade", "h2c"}},
+ /*fin=*/true)
+ .Serialize();
+
+ // Client preface (empty SETTINGS)
+ EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0));
+ EXPECT_CALL(visitor, OnSettingsStart());
+ EXPECT_CALL(visitor, OnSettingsEnd());
+
+ // All streams contain a connection-specific header and should be rejected.
+ EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
+ EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4);
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(1, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
+ EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(3));
+ EXPECT_CALL(visitor, OnHeaderForStream(3, _, _)).Times(4);
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(3, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
+ EXPECT_CALL(visitor, OnFrameHeader(5, _, HEADERS, 5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(5));
+ EXPECT_CALL(visitor, OnHeaderForStream(5, _, _)).Times(4);
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(5, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
+ EXPECT_CALL(visitor, OnFrameHeader(7, _, HEADERS, 5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(7));
+ EXPECT_CALL(visitor, OnHeaderForStream(7, _, _)).Times(4);
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(7, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
+ EXPECT_CALL(visitor, OnFrameHeader(9, _, HEADERS, 5));
+ EXPECT_CALL(visitor, OnBeginHeadersForStream(9));
+ EXPECT_CALL(visitor, OnHeaderForStream(9, _, _)).Times(4);
+ EXPECT_CALL(
+ visitor,
+ OnInvalidFrame(9, Http2VisitorInterface::InvalidFrameError::kHttpHeader));
+
+ 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, _, 0x0));
+ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
+ 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_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 3, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+ EXPECT_CALL(visitor, OnCloseStream(3, Http2ErrorCode::HTTP2_NO_ERROR));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 5, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 5, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+ EXPECT_CALL(visitor, OnCloseStream(5, Http2ErrorCode::HTTP2_NO_ERROR));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 7, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 7, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+ EXPECT_CALL(visitor, OnCloseStream(7, Http2ErrorCode::HTTP2_NO_ERROR));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 9, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 9, _, 0x0,
+ static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
+ EXPECT_CALL(visitor, OnCloseStream(9, Http2ErrorCode::HTTP2_NO_ERROR));
+
+ EXPECT_TRUE(adapter->want_write());
+ int result = adapter->Send();
+ EXPECT_EQ(0, result);
+ EXPECT_THAT(
+ visitor.data(),
+ EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::SETTINGS,
+ SpdyFrameType::RST_STREAM, SpdyFrameType::RST_STREAM,
+ SpdyFrameType::RST_STREAM, SpdyFrameType::RST_STREAM,
+ SpdyFrameType::RST_STREAM}));
+}
+
TEST(OgHttp2AdapterTest, ServerUsesCustomWindowUpdateStrategy) {
// Test the use of a custom WINDOW_UPDATE strategy.
DataSavingVisitor visitor;
