Skip to content

Commit

Permalink
Adds the ability to skip Cookie crumbling in HpackEncoder.
Browse files Browse the repository at this point in the history
This is preparation for fixing the issue described in envoyproxy/envoy#32611.

Protected by new code path, not yet enabled in the GFE binary.

PiperOrigin-RevId: 613330788
  • Loading branch information
birenroy authored and copybara-github committed Mar 6, 2024
1 parent f373496 commit c186a51
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 4 deletions.
21 changes: 17 additions & 4 deletions quiche/spdy/core/hpack/hpack_encoder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ HpackEncoder::HpackEncoder()
listener_(NoOpListener),
should_index_(DefaultPolicy),
enable_compression_(true),
should_emit_table_size_(false) {}
should_emit_table_size_(false),
crumble_cookies_(true) {}

HpackEncoder::~HpackEncoder() = default;

Expand All @@ -101,7 +102,11 @@ std::string HpackEncoder::EncodeHeaderBlock(
// Note that there can only be one "cookie" header, because header_set is
// a map.
found_cookie = true;
CookieToCrumbs(header, &regular_headers);
if (crumble_cookies_) {
CookieToCrumbs(header, &regular_headers);
} else {
DecomposeRepresentation(header, &regular_headers);
}
} else if (!header.first.empty() &&
header.first[0] == kPseudoHeaderPrefix) {
DecomposeRepresentation(header, &pseudo_headers);
Expand Down Expand Up @@ -302,7 +307,11 @@ HpackEncoder::Encoderator::Encoderator(const Http2HeaderBlock& header_set,
// Note that there can only be one "cookie" header, because header_set
// is a map.
found_cookie = true;
CookieToCrumbs(header, &regular_headers_);
if (encoder_->crumble_cookies_) {
CookieToCrumbs(header, &regular_headers_);
} else {
DecomposeRepresentation(header, &regular_headers_);
}
} else if (!header.first.empty() &&
header.first[0] == kPseudoHeaderPrefix) {
DecomposeRepresentation(header, &pseudo_headers_);
Expand All @@ -321,7 +330,11 @@ HpackEncoder::Encoderator::Encoderator(const Representations& representations,
: encoder_(encoder), has_next_(true) {
for (const auto& header : representations) {
if (header.first == "cookie") {
CookieToCrumbs(header, &regular_headers_);
if (encoder_->crumble_cookies_) {
CookieToCrumbs(header, &regular_headers_);
} else {
DecomposeRepresentation(header, &regular_headers_);
}
} else if (!header.first.empty() &&
header.first[0] == kPseudoHeaderPrefix) {
pseudo_headers_.push_back(header);
Expand Down
7 changes: 7 additions & 0 deletions quiche/spdy/core/hpack/hpack_encoder.h
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ class QUICHE_EXPORT HpackEncoder {

void DisableCompression() { enable_compression_ = false; }

// Disables the deconstruction of Cookie header values into individual
// components, as described in
// https://httpwg.org/specs/rfc9113.html#CompressCookie. The deconstructed
// representation can cause problems for some HTTP/2 endpoints.
void DisableCookieCrumbling() { crumble_cookies_ = false; }

// Returns the current dynamic table size, including the 32 bytes per entry
// overhead mentioned in RFC 7541 section 4.1.
size_t GetDynamicTableSize() const { return header_table_.size(); }
Expand Down Expand Up @@ -142,6 +148,7 @@ class QUICHE_EXPORT HpackEncoder {
IndexingPolicy should_index_;
bool enable_compression_;
bool should_emit_table_size_;
bool crumble_cookies_;
};

} // namespace spdy
Expand Down
42 changes: 42 additions & 0 deletions quiche/spdy/core/hpack/hpack_encoder_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,39 @@ TEST_P(HpackEncoderTestWithDefaultStrategy, EncodeRepresentations) {
EXPECT_GE(kInitialDynamicTableSize, encoder_.GetDynamicTableSize());
}

TEST_P(HpackEncoderTestWithDefaultStrategy, WithoutCookieCrumbling) {
EXPECT_EQ(kInitialDynamicTableSize, encoder_.GetDynamicTableSize());
encoder_.SetHeaderListener(
[this](absl::string_view name, absl::string_view value) {
this->SaveHeaders(name, value);
});
encoder_.DisableCookieCrumbling();

const std::vector<std::pair<absl::string_view, absl::string_view>>
header_list = {{"cookie", "val1; val2;val3"},
{":path", "/home"},
{"accept", "text/html, text/plain,application/xml"},
{"cookie", "val4"},
{"withnul", absl::string_view("one\0two", 7)}};
ExpectNonIndexedLiteralWithNameIndex(peer_.table()->GetByName(":path"),
"/home");
ExpectIndexedLiteral(peer_.table()->GetByName("cookie"), "val1; val2;val3");
ExpectIndexedLiteral(peer_.table()->GetByName("accept"),
"text/html, text/plain,application/xml");
ExpectIndexedLiteral(peer_.table()->GetByName("cookie"), "val4");
ExpectIndexedLiteral("withnul", absl::string_view("one\0two", 7));

CompareWithExpectedEncoding(header_list);
EXPECT_THAT(
headers_observed_,
ElementsAre(Pair(":path", "/home"), Pair("cookie", "val1; val2;val3"),
Pair("accept", "text/html, text/plain,application/xml"),
Pair("cookie", "val4"),
Pair("withnul", absl::string_view("one\0two", 7))));
// Insertions and evictions have happened over the course of the test.
EXPECT_GE(kInitialDynamicTableSize, encoder_.GetDynamicTableSize());
}

TEST_P(HpackEncoderTestWithDefaultStrategy, DynamicTableGrows) {
EXPECT_EQ(kInitialDynamicTableSize, encoder_.GetDynamicTableSize());
peer_.table()->SetMaxSize(4096);
Expand Down Expand Up @@ -446,6 +479,15 @@ TEST_P(HpackEncoderTest, CookieHeaderIsCrumbled) {
CompareWithExpectedEncoding(headers);
}

TEST_P(HpackEncoderTest, CookieHeaderIsNotCrumbled) {
encoder_.DisableCookieCrumbling();
ExpectIndexedLiteral(peer_.table()->GetByName("cookie"), "a=bb; c=dd; e=ff");

Http2HeaderBlock headers;
headers["cookie"] = "a=bb; c=dd; e=ff";
CompareWithExpectedEncoding(headers);
}

TEST_P(HpackEncoderTest, MultiValuedHeadersNotCrumbled) {
ExpectIndexedLiteral("foo", "bar, baz");
Http2HeaderBlock headers;
Expand Down

0 comments on commit c186a51

Please sign in to comment.