Fixes the conversion of a Span<const Header> to spdy::SpdyHeaderBlock in oghttp2 when the span contains multiple header fields with the same name. PiperOrigin-RevId: 423107278
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index ff92a65..f2dbd67 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -3254,6 +3254,67 @@ absl::StrJoin(visitor.GetMetadata(1), "")); } +TEST(NgHttp2AdapterTest, RepeatedHeaderNames) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); + EXPECT_FALSE(adapter->want_write()); + + const std::string frames = TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}, + {"accept", "text/plain"}, + {"accept", "text/html"}}, + /*fin=*/true) + .Serialize(); + + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + // Stream 1 + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "GET")); + 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, OnHeaderForStream(1, "accept", "text/plain")); + EXPECT_CALL(visitor, OnHeaderForStream(1, "accept", "text/html")); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnEndStream(1)); + + int64_t result = adapter->ProcessBytes(frames); + EXPECT_EQ(frames.size(), static_cast<size_t>(result)); + + const std::vector<Header> headers1 = ToHeaders( + {{":status", "200"}, {"content-length", "10"}, {"content-length", "10"}}); + auto body1 = absl::make_unique<TestDataFrameSource>(visitor, true); + body1->AppendPayload("perfection"); + body1->EndData(); + + int submit_result = adapter->SubmitResponse(1, headers1, std::move(body1)); + ASSERT_EQ(0, submit_result); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, 1, _, 0x4)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x4, 0)); + EXPECT_CALL(visitor, OnFrameSent(DATA, 1, 10, 0x1, 0)); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::HEADERS, + spdy::SpdyFrameType::DATA})); +} + TEST(NgHttp2AdapterTest, ServerRespondsToRequestWithTrailers) { DataSavingVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); @@ -3638,6 +3699,60 @@ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS})); } +TEST(NgHttp2AdapterInteractionTest, + ClientServerInteractionRepeatedHeaderNames) { + DataSavingVisitor client_visitor; + auto client_adapter = NgHttp2Adapter::CreateClientAdapter(client_visitor); + + client_adapter->SubmitSettings({}); + + const std::vector<Header> headers1 = + ToHeaders({{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}, + {"accept", "text/plain"}, + {"accept", "text/html"}}); + + const int32_t stream_id1 = + client_adapter->SubmitRequest(headers1, nullptr, nullptr); + ASSERT_GT(stream_id1, 0); + + EXPECT_CALL(client_visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(client_visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + EXPECT_CALL(client_visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5)); + EXPECT_CALL(client_visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0)); + int send_result = client_adapter->Send(); + EXPECT_EQ(0, send_result); + + DataSavingVisitor server_visitor; + auto server_adapter = NgHttp2Adapter::CreateServerAdapter(server_visitor); + + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(server_visitor, OnFrameHeader(0, _, SETTINGS, 0)); + EXPECT_CALL(server_visitor, OnSettingsStart()); + EXPECT_CALL(server_visitor, OnSetting(_)).Times(testing::AnyNumber()); + EXPECT_CALL(server_visitor, OnSettingsEnd()); + // Stream 1 + EXPECT_CALL(server_visitor, OnFrameHeader(1, _, HEADERS, 5)); + EXPECT_CALL(server_visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(server_visitor, OnHeaderForStream(1, ":method", "GET")); + EXPECT_CALL(server_visitor, OnHeaderForStream(1, ":scheme", "http")); + EXPECT_CALL(server_visitor, + OnHeaderForStream(1, ":authority", "example.com")); + EXPECT_CALL(server_visitor, + OnHeaderForStream(1, ":path", "/this/is/request/one")); + EXPECT_CALL(server_visitor, OnHeaderForStream(1, "accept", "text/plain")); + EXPECT_CALL(server_visitor, OnHeaderForStream(1, "accept", "text/html")); + EXPECT_CALL(server_visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(server_visitor, OnEndStream(1)); + + int64_t result = server_adapter->ProcessBytes(client_visitor.data()); + EXPECT_EQ(client_visitor.data().size(), static_cast<size_t>(result)); +} + TEST(NgHttp2AdapterTest, ServerForbidsWindowUpdateOnIdleStream) { DataSavingVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index ee67139..d785891 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -2637,6 +2637,71 @@ absl::StrJoin(visitor.GetMetadata(1), "")); } +TEST(OgHttp2AdapterServerTest, RepeatedHeaderNames) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kServer}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + EXPECT_FALSE(adapter->want_write()); + + const std::string frames = TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "GET"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}, + {"accept", "text/plain"}, + {"accept", "text/html"}}, + /*fin=*/true) + .Serialize(); + + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + // Stream 1 + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "GET")); + 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, OnHeaderForStream(1, "accept", "text/plain")); + EXPECT_CALL(visitor, OnHeaderForStream(1, "accept", "text/html")); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnEndStream(1)); + + int64_t result = adapter->ProcessBytes(frames); + EXPECT_EQ(frames.size(), static_cast<size_t>(result)); + + const std::vector<Header> headers1 = ToHeaders( + {{":status", "200"}, {"content-length", "10"}, {"content-length", "10"}}); + auto body1 = absl::make_unique<TestDataFrameSource>(visitor, true); + body1->AppendPayload("perfection"); + body1->EndData(); + + int submit_result = adapter->SubmitResponse(1, headers1, std::move(body1)); + ASSERT_EQ(0, submit_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(HEADERS, 1, _, 0x4)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, 1, _, 0x4, 0)); + EXPECT_CALL(visitor, OnFrameSent(DATA, 1, 10, 0x1, 0)); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), + EqualsFrames( + {spdy::SpdyFrameType::SETTINGS, spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::HEADERS, spdy::SpdyFrameType::DATA})); +} + TEST(OgHttp2AdapterServerTest, ServerRespondsToRequestWithTrailers) { DataSavingVisitor visitor; OgHttp2Adapter::Options options{.perspective = Perspective::kServer}; @@ -3752,6 +3817,60 @@ client_adapter->Send(); } +TEST(OgHttp2AdapterInteractionTest, + ClientServerInteractionRepeatedHeaderNames) { + DataSavingVisitor client_visitor; + OgHttp2Adapter::Options client_options{.perspective = Perspective::kClient}; + auto client_adapter = OgHttp2Adapter::Create(client_visitor, client_options); + + const std::vector<Header> headers1 = + ToHeaders({{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}, + {"accept", "text/plain"}, + {"accept", "text/html"}}); + + const int32_t stream_id1 = + client_adapter->SubmitRequest(headers1, nullptr, nullptr); + ASSERT_GT(stream_id1, 0); + + EXPECT_CALL(client_visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(client_visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + EXPECT_CALL(client_visitor, OnBeforeFrameSent(HEADERS, stream_id1, _, 0x5)); + EXPECT_CALL(client_visitor, OnFrameSent(HEADERS, stream_id1, _, 0x5, 0)); + int send_result = client_adapter->Send(); + EXPECT_EQ(0, send_result); + + DataSavingVisitor server_visitor; + OgHttp2Adapter::Options server_options{.perspective = Perspective::kServer}; + auto server_adapter = OgHttp2Adapter::Create(server_visitor, server_options); + + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(server_visitor, OnFrameHeader(0, _, SETTINGS, 0)); + EXPECT_CALL(server_visitor, OnSettingsStart()); + EXPECT_CALL(server_visitor, OnSetting(_)).Times(testing::AnyNumber()); + EXPECT_CALL(server_visitor, OnSettingsEnd()); + // Stream 1 + EXPECT_CALL(server_visitor, OnFrameHeader(1, _, HEADERS, 5)); + EXPECT_CALL(server_visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(server_visitor, OnHeaderForStream(1, ":method", "GET")); + EXPECT_CALL(server_visitor, OnHeaderForStream(1, ":scheme", "http")); + EXPECT_CALL(server_visitor, + OnHeaderForStream(1, ":authority", "example.com")); + EXPECT_CALL(server_visitor, + OnHeaderForStream(1, ":path", "/this/is/request/one")); + EXPECT_CALL(server_visitor, OnHeaderForStream(1, "accept", "text/plain")); + EXPECT_CALL(server_visitor, OnHeaderForStream(1, "accept", "text/html")); + EXPECT_CALL(server_visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(server_visitor, OnEndStream(1)); + + int64_t result = server_adapter->ProcessBytes(client_visitor.data()); + EXPECT_EQ(client_visitor.data().size(), static_cast<size_t>(result)); +} + TEST(OgHttp2AdapterServerTest, ServerForbidsNewStreamBelowWatermark) { DataSavingVisitor visitor; OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
diff --git a/http2/adapter/oghttp2_util.cc b/http2/adapter/oghttp2_util.cc index 3ed13dc..ed0e6f6 100644 --- a/http2/adapter/oghttp2_util.cc +++ b/http2/adapter/oghttp2_util.cc
@@ -8,7 +8,7 @@ for (const Header& header : headers) { absl::string_view name = GetStringView(header.first).first; absl::string_view value = GetStringView(header.second).first; - block[name] = value; + block.AppendValueOrAddHeader(name, value); } return block; }
diff --git a/http2/adapter/oghttp2_util_test.cc b/http2/adapter/oghttp2_util_test.cc new file mode 100644 index 0000000..6f29b26 --- /dev/null +++ b/http2/adapter/oghttp2_util_test.cc
@@ -0,0 +1,83 @@ +#include "http2/adapter/oghttp2_util.h" + +#include <utility> +#include <vector> + +#include "http2/adapter/http2_protocol.h" +#include "http2/adapter/test_frame_sequence.h" +#include "common/platform/api/quiche_test.h" + +namespace http2 { +namespace adapter { +namespace test { +namespace { + +using HeaderPair = std::pair<absl::string_view, absl::string_view>; + +TEST(ToHeaderBlock, EmptySpan) { + spdy::SpdyHeaderBlock block = ToHeaderBlock({}); + EXPECT_TRUE(block.empty()); +} + +TEST(ToHeaderBlock, ExampleRequestHeaders) { + const std::vector<HeaderPair> pairs = {{":authority", "example.com"}, + {":method", "GET"}, + {":path", "/example.html"}, + {":scheme", "http"}, + {"accept", "text/plain, text/html"}}; + const std::vector<Header> headers = ToHeaders(pairs); + spdy::SpdyHeaderBlock block = ToHeaderBlock(headers); + EXPECT_THAT(block, testing::ElementsAreArray(pairs)); +} + +TEST(ToHeaderBlock, ExampleResponseHeaders) { + const std::vector<HeaderPair> pairs = { + {":status", "403"}, + {"content-length", "1023"}, + {"x-extra-info", "humblest apologies"}}; + const std::vector<Header> headers = ToHeaders(pairs); + spdy::SpdyHeaderBlock block = ToHeaderBlock(headers); + EXPECT_THAT(block, testing::ElementsAreArray(pairs)); +} + +TEST(ToHeaderBlock, RepeatedRequestHeaderNames) { + const std::vector<HeaderPair> pairs = { + {":authority", "example.com"}, {":method", "GET"}, + {":path", "/example.html"}, {":scheme", "http"}, + {"cookie", "chocolate_chips=yes"}, {"accept", "text/plain, text/html"}, + {"cookie", "raisins=no"}}; + const std::vector<HeaderPair> expected = { + {":authority", "example.com"}, + {":method", "GET"}, + {":path", "/example.html"}, + {":scheme", "http"}, + {"cookie", "chocolate_chips=yes; raisins=no"}, + {"accept", "text/plain, text/html"}}; + const std::vector<Header> headers = ToHeaders(pairs); + spdy::SpdyHeaderBlock block = ToHeaderBlock(headers); + EXPECT_THAT(block, testing::ElementsAreArray(expected)); +} + +TEST(ToHeaderBlock, RepeatedResponseHeaderNames) { + const std::vector<HeaderPair> pairs = { + {":status", "403"}, {"x-extra-info", "sorry"}, + {"content-length", "1023"}, {"x-extra-info", "humblest apologies"}, + {"content-length", "1024"}, {"set-cookie", "chocolate_chips=yes"}, + {"set-cookie", "raisins=no"}}; + const std::vector<HeaderPair> expected = { + {":status", "403"}, + {"x-extra-info", absl::string_view("sorry\0humblest apologies", 24)}, + {"content-length", absl::string_view("1023" + "\0" + "1024", + 9)}, + {"set-cookie", absl::string_view("chocolate_chips=yes\0raisins=no", 30)}}; + const std::vector<Header> headers = ToHeaders(pairs); + spdy::SpdyHeaderBlock block = ToHeaderBlock(headers); + EXPECT_THAT(block, testing::ElementsAreArray(expected)); +} + +} // namespace +} // namespace test +} // namespace adapter +} // namespace http2