Add ValueSplittingHeaderList option to disable cookie crumbling during QPACK encoding. No behavior change yet: cookie crumbling is still enabled in production. PiperOrigin-RevId: 658068196
diff --git a/quiche/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc b/quiche/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc index d3a616a..08daf1b 100644 --- a/quiche/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc +++ b/quiche/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc
@@ -572,12 +572,14 @@ } // Splits |*header_list| header values along '\0' or ';' separators. -QuicHeaderList SplitHeaderList(const quiche::HttpHeaderBlock& header_list) { +QuicHeaderList SplitHeaderList(const quiche::HttpHeaderBlock& header_list, + CookieCrumbling cookie_crumbling) { QuicHeaderList split_header_list; split_header_list.OnHeaderBlockStart(); size_t total_size = 0; - ValueSplittingHeaderList splitting_header_list(&header_list); + ValueSplittingHeaderList splitting_header_list(&header_list, + cookie_crumbling); for (const auto& header : splitting_header_list) { split_header_list.OnHeader(header.first, header.second); total_size += header.first.size() + header.second.size(); @@ -647,7 +649,8 @@ // Encoder splits |header_list| header values along '\0' or ';' separators. // Do the same here so that we get matching results. - QuicHeaderList expected_header_list = SplitHeaderList(header_list); + QuicHeaderList expected_header_list = + SplitHeaderList(header_list, CookieCrumbling::kEnabled); decoder.AddExpectedHeaderList(stream_id, std::move(expected_header_list)); header_block_transmitter.SendEncodedHeaderBlock(
diff --git a/quiche/quic/core/qpack/qpack_encoder.cc b/quiche/quic/core/qpack/qpack_encoder.cc index fda0e3c..0bdac87 100644 --- a/quiche/quic/core/qpack/qpack_encoder.cc +++ b/quiche/quic/core/qpack/qpack_encoder.cc
@@ -116,7 +116,8 @@ bool dynamic_table_insertion_blocked = false; bool blocked_stream_limit_exhausted = false; - for (const auto& header : ValueSplittingHeaderList(&header_list)) { + for (const auto& header : + ValueSplittingHeaderList(&header_list, CookieCrumbling::kEnabled)) { // These strings are owned by |header_list|. absl::string_view name = header.first; absl::string_view value = header.second;
diff --git a/quiche/quic/core/qpack/value_splitting_header_list.cc b/quiche/quic/core/qpack/value_splitting_header_list.cc index ad6d3a4..a411f35 100644 --- a/quiche/quic/core/qpack/value_splitting_header_list.cc +++ b/quiche/quic/core/qpack/value_splitting_header_list.cc
@@ -21,9 +21,11 @@ ValueSplittingHeaderList::const_iterator::const_iterator( const quiche::HttpHeaderBlock* header_list, - quiche::HttpHeaderBlock::const_iterator header_list_iterator) + quiche::HttpHeaderBlock::const_iterator header_list_iterator, + CookieCrumbling cookie_crumbling) : header_list_(header_list), header_list_iterator_(header_list_iterator), + cookie_crumbling_(cookie_crumbling), value_start_(0) { UpdateHeaderField(); } @@ -75,7 +77,11 @@ const absl::string_view original_value = header_list_iterator_->second; if (name == kCookieKey) { - value_end_ = original_value.find(kCookieSeparator, value_start_); + if (cookie_crumbling_ == CookieCrumbling::kEnabled) { + value_end_ = original_value.find(kCookieSeparator, value_start_); + } else { + value_end_ = absl::string_view::npos; + } } else { value_end_ = original_value.find(kNonCookieSeparator, value_start_); } @@ -93,18 +99,19 @@ } ValueSplittingHeaderList::ValueSplittingHeaderList( - const quiche::HttpHeaderBlock* header_list) - : header_list_(header_list) { + const quiche::HttpHeaderBlock* header_list, + CookieCrumbling cookie_crumbling) + : header_list_(header_list), cookie_crumbling_(cookie_crumbling) { QUICHE_DCHECK(header_list_); } ValueSplittingHeaderList::const_iterator ValueSplittingHeaderList::begin() const { - return const_iterator(header_list_, header_list_->begin()); + return const_iterator(header_list_, header_list_->begin(), cookie_crumbling_); } ValueSplittingHeaderList::const_iterator ValueSplittingHeaderList::end() const { - return const_iterator(header_list_, header_list_->end()); + return const_iterator(header_list_, header_list_->end(), cookie_crumbling_); } } // namespace quic
diff --git a/quiche/quic/core/qpack/value_splitting_header_list.h b/quiche/quic/core/qpack/value_splitting_header_list.h index 1ef5f7c..34d2d66 100644 --- a/quiche/quic/core/qpack/value_splitting_header_list.h +++ b/quiche/quic/core/qpack/value_splitting_header_list.h
@@ -11,6 +11,10 @@ namespace quic { +// Enumeration that specifies whether cookie crumbling should be used when +// sending QPACK headers. +enum class CookieCrumbling { kEnabled, kDisabled }; + // A wrapper class around Http2HeaderBlock that splits header values along ';' // separators (while also removing optional space following separator) for // cookies and along '\0' separators for other header fields. @@ -21,9 +25,9 @@ class QUICHE_EXPORT const_iterator { public: // |header_list| must outlive this object. - const_iterator( - const quiche::HttpHeaderBlock* header_list, - quiche::HttpHeaderBlock::const_iterator header_list_iterator); + const_iterator(const quiche::HttpHeaderBlock* header_list, + quiche::HttpHeaderBlock::const_iterator header_list_iterator, + CookieCrumbling cookie_crumbling); const_iterator(const const_iterator&) = default; const_iterator& operator=(const const_iterator&) = delete; @@ -41,13 +45,15 @@ const quiche::HttpHeaderBlock* const header_list_; quiche::HttpHeaderBlock::const_iterator header_list_iterator_; + const CookieCrumbling cookie_crumbling_; absl::string_view::size_type value_start_; absl::string_view::size_type value_end_; value_type header_field_; }; // |header_list| must outlive this object. - explicit ValueSplittingHeaderList(const quiche::HttpHeaderBlock* header_list); + explicit ValueSplittingHeaderList(const quiche::HttpHeaderBlock* header_list, + CookieCrumbling cookie_crumbling); ValueSplittingHeaderList(const ValueSplittingHeaderList&) = delete; ValueSplittingHeaderList& operator=(const ValueSplittingHeaderList&) = delete; @@ -56,6 +62,7 @@ private: const quiche::HttpHeaderBlock* const header_list_; + const CookieCrumbling cookie_crumbling_; }; } // namespace quic
diff --git a/quiche/quic/core/qpack/value_splitting_header_list_test.cc b/quiche/quic/core/qpack/value_splitting_header_list_test.cc index f71538d..22e14a3 100644 --- a/quiche/quic/core/qpack/value_splitting_header_list_test.cc +++ b/quiche/quic/core/qpack/value_splitting_header_list_test.cc
@@ -23,7 +23,7 @@ block["baz"] = "qux"; block["cookie"] = "foo; bar"; - ValueSplittingHeaderList headers(&block); + ValueSplittingHeaderList headers(&block, CookieCrumbling::kEnabled); ValueSplittingHeaderList::const_iterator it1 = headers.begin(); const int kEnd = 6; for (int i = 0; i < kEnd; ++i) { @@ -79,16 +79,17 @@ TEST(ValueSplittingHeaderListTest, Empty) { quiche::HttpHeaderBlock block; - ValueSplittingHeaderList headers(&block); + ValueSplittingHeaderList headers(&block, CookieCrumbling::kEnabled); EXPECT_THAT(headers, ElementsAre()); EXPECT_EQ(headers.begin(), headers.end()); } -TEST(ValueSplittingHeaderListTest, Split) { +// CookieCrumbling does not influence splitting non-cookie headers. +TEST(ValueSplittingHeaderListTest, SplitNonCookie) { struct { const char* name; absl::string_view value; - std::vector<const char*> expected_values; + std::vector<absl::string_view> expected_values; } kTestData[]{ // Empty value. {"foo", "", {""}}, @@ -96,13 +97,52 @@ {"foo", "bar", {"bar"}}, // Simple split. {"foo", {"bar\0baz", 7}, {"bar", "baz"}}, - {"cookie", "foo;bar", {"foo", "bar"}}, - {"cookie", "foo; bar", {"foo", "bar"}}, // Empty fragments with \0 separator. {"foo", {"\0", 1}, {"", ""}}, {"bar", {"foo\0", 4}, {"foo", ""}}, {"baz", {"\0bar", 4}, {"", "bar"}}, {"qux", {"\0foobar\0", 8}, {"", "foobar", ""}}, + }; + + for (size_t i = 0; i < ABSL_ARRAYSIZE(kTestData); ++i) { + quiche::HttpHeaderBlock block; + block[kTestData[i].name] = kTestData[i].value; + + { + ValueSplittingHeaderList headers(&block, CookieCrumbling::kEnabled); + auto it = headers.begin(); + for (absl::string_view expected_value : kTestData[i].expected_values) { + ASSERT_NE(it, headers.end()); + EXPECT_EQ(it->first, kTestData[i].name); + EXPECT_EQ(it->second, expected_value); + ++it; + } + EXPECT_EQ(it, headers.end()); + } + + { + ValueSplittingHeaderList headers(&block, CookieCrumbling::kDisabled); + auto it = headers.begin(); + for (absl::string_view expected_value : kTestData[i].expected_values) { + ASSERT_NE(it, headers.end()); + EXPECT_EQ(it->first, kTestData[i].name); + EXPECT_EQ(it->second, expected_value); + ++it; + } + EXPECT_EQ(it, headers.end()); + } + } +} + +TEST(ValueSplittingHeaderListTest, SplitCookie) { + struct { + const char* name; + absl::string_view value; + std::vector<absl::string_view> expected_values; + } kTestData[]{ + // Simple split. + {"cookie", "foo;bar", {"foo", "bar"}}, + {"cookie", "foo; bar", {"foo", "bar"}}, // Empty fragments with ";" separator. {"cookie", ";", {"", ""}}, {"cookie", "foo;", {"foo", ""}}, @@ -119,38 +159,74 @@ quiche::HttpHeaderBlock block; block[kTestData[i].name] = kTestData[i].value; - ValueSplittingHeaderList headers(&block); - auto it = headers.begin(); - for (const char* expected_value : kTestData[i].expected_values) { + { + ValueSplittingHeaderList headers(&block, CookieCrumbling::kEnabled); + auto it = headers.begin(); + for (absl::string_view expected_value : kTestData[i].expected_values) { + ASSERT_NE(it, headers.end()); + EXPECT_EQ(it->first, kTestData[i].name); + EXPECT_EQ(it->second, expected_value); + ++it; + } + EXPECT_EQ(it, headers.end()); + } + + { + // When cookie crumbling is disabled, `kTestData[i].value` is unchanged. + ValueSplittingHeaderList headers(&block, CookieCrumbling::kDisabled); + auto it = headers.begin(); ASSERT_NE(it, headers.end()); EXPECT_EQ(it->first, kTestData[i].name); - EXPECT_EQ(it->second, expected_value); + EXPECT_EQ(it->second, kTestData[i].value); ++it; + EXPECT_EQ(it, headers.end()); } - EXPECT_EQ(it, headers.end()); } } -TEST(ValueSplittingHeaderListTest, MultipleFields) { +TEST(ValueSplittingHeaderListTest, MultipleFieldsCookieCrumblingEnabled) { quiche::HttpHeaderBlock block; block["foo"] = absl::string_view("bar\0baz\0", 8); block["cookie"] = "foo; bar"; block["bar"] = absl::string_view("qux\0foo", 7); - ValueSplittingHeaderList headers(&block); + ValueSplittingHeaderList headers(&block, CookieCrumbling::kEnabled); EXPECT_THAT(headers, ElementsAre(Pair("foo", "bar"), Pair("foo", "baz"), Pair("foo", ""), Pair("cookie", "foo"), Pair("cookie", "bar"), Pair("bar", "qux"), Pair("bar", "foo"))); } -TEST(ValueSplittingHeaderListTest, CookieStartsWithSpace) { +TEST(ValueSplittingHeaderListTest, MultipleFieldsCookieCrumblingDisabled) { + quiche::HttpHeaderBlock block; + block["foo"] = absl::string_view("bar\0baz\0", 8); + block["cookie"] = "foo; bar"; + block["bar"] = absl::string_view("qux\0foo", 7); + + ValueSplittingHeaderList headers(&block, CookieCrumbling::kDisabled); + EXPECT_THAT(headers, ElementsAre(Pair("foo", "bar"), Pair("foo", "baz"), + Pair("foo", ""), Pair("cookie", "foo; bar"), + Pair("bar", "qux"), Pair("bar", "foo"))); +} + +TEST(ValueSplittingHeaderListTest, CookieStartsWithSpaceCrumblingEnabled) { quiche::HttpHeaderBlock block; block["foo"] = "bar"; block["cookie"] = " foo"; block["bar"] = "baz"; - ValueSplittingHeaderList headers(&block); + ValueSplittingHeaderList headers(&block, CookieCrumbling::kEnabled); + EXPECT_THAT(headers, ElementsAre(Pair("foo", "bar"), Pair("cookie", " foo"), + Pair("bar", "baz"))); +} + +TEST(ValueSplittingHeaderListTest, CookieStartsWithSpaceCrumblingDisabled) { + quiche::HttpHeaderBlock block; + block["foo"] = "bar"; + block["cookie"] = " foo"; + block["bar"] = "baz"; + + ValueSplittingHeaderList headers(&block, CookieCrumbling::kDisabled); EXPECT_THAT(headers, ElementsAre(Pair("foo", "bar"), Pair("cookie", " foo"), Pair("bar", "baz"))); }