Don't add Brotli Accept-Encoding when already present

It's possible for a server to advertise brotli encoding when it isn't normally
expected to. In these cases, we were previously triggering a DCHECK and
potentially adding a second Brotli Accept-Encoding. This has been changed so
that the Brotli Accept-Encoding is now only added when it is not already
present.

Bug: 879762
Change-Id: I5f9bb8eeec0cc03ee4953cd8fdf7fd7c15a92067
Reviewed-on: https://chromium-review.googlesource.com/1200386
Commit-Queue: Jered Gray <jegray@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590031}

components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <limits>
+#include <set>
 #include <utility>
 
 #include "base/metrics/histogram_functions.h"
@@ -30,6 +31,7 @@
 #include "net/base/proxy_server.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_response_headers.h"
+#include "net/http/http_util.h"
 #include "net/nqe/effective_connection_type.h"
 #include "net/proxy_resolution/proxy_info.h"
 #include "net/proxy_resolution/proxy_resolution_service.h"
@@ -708,17 +710,16 @@ void DataReductionProxyNetworkDelegate::MaybeAddBrotliToAcceptEncodingHeader(
   request_headers->GetHeader(net::HttpRequestHeaders::kAcceptEncoding,
                              &header_value);
 
-  // Brotli should not be already present in the header since the URL is non-
-  // cryptographic. This is an approximate check, and would trigger even if the
-  // accept-encoding header contains an encoding that has prefix |kBrotli|.
-  DCHECK_EQ(std::string::npos, header_value.find(kBrotli));
-
-  request_headers->RemoveHeader(net::HttpRequestHeaders::kAcceptEncoding);
-  if (!header_value.empty())
-    header_value += ", ";
-  header_value += kBrotli;
-  request_headers->SetHeader(net::HttpRequestHeaders::kAcceptEncoding,
-                             header_value);
+  // Only add Brotli to the header if it is not already present.
+  std::set<std::string> header_entry_set;
+  if (net::HttpUtil::ParseAcceptEncoding(header_value, &header_entry_set) &&
+      header_entry_set.find(kBrotli) == header_entry_set.end()) {
+    if (!header_value.empty())
+      header_value += ", ";
+    header_value += kBrotli;
+    request_headers->SetHeader(net::HttpRequestHeaders::kAcceptEncoding,
+                               header_value);
+  }
 }
 
 void DataReductionProxyNetworkDelegate::MaybeAddChromeProxyECTHeader(

components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc

@@ -9,6 +9,7 @@
 
 #include <algorithm>
 #include <map>
+#include <set>
 #include <string>
 #include <utility>
 
@@ -574,8 +575,45 @@ class DataReductionProxyNetworkDelegateTest : public testing::Test {
                                      .append(host)
                                      .append(
                                          "\r\n"
-                                         "Proxy-Connection: keep-alive\r\n"
-                                         "User-Agent:\r\n");
+                                         "Proxy-Connection: keep-alive\r\n");
+    std::string user_agent_header = "User-Agent:\r\n";
+
+    // Set the base Accept-Encoding header value; Brotli may be added to it.
+    std::string accept_encoding_header_value;
+    bool accept_encoding_header_value_includes_brotli = false;
+    bool has_accept_encoding_request_header =
+        request_headers &&
+        request_headers->HasHeader(net::HttpRequestHeaders::kAcceptEncoding);
+    if (has_accept_encoding_request_header) {
+      request_headers->GetHeader(net::HttpRequestHeaders::kAcceptEncoding,
+                                 &accept_encoding_header_value);
+      // Check for if the Accept-Encoding header value already includes Brotli.
+      std::set<std::string> accept_encoding_header_entry_set;
+      if (net::HttpUtil::ParseAcceptEncoding(
+              accept_encoding_header_value,
+              &accept_encoding_header_entry_set) &&
+          accept_encoding_header_entry_set.find("br") !=
+              accept_encoding_header_entry_set.end()) {
+        accept_encoding_header_value_includes_brotli = true;
+      }
+    } else {
+      accept_encoding_header_value = "gzip, deflate";
+    }
+
+    // Add Brotli to the Accept-Encoding header value if it is expected and not
+    // already included. Brotli is expected if the request went to the network
+    // (i.e., it was not a cached response), and it is a case where the data
+    // reduction proxy network delegate adds Brotli to the header.
+    if (expect_brotli && !expect_cached &&
+        !accept_encoding_header_value_includes_brotli) {
+      if (!accept_encoding_header_value.empty())
+        accept_encoding_header_value += ", ";
+      accept_encoding_header_value += "br";
+      accept_encoding_header_value_includes_brotli = true;
+    }
+
+    std::string accept_encoding_header =
+        "Accept-Encoding: " + accept_encoding_header_value + "\r\n";
 
     std::string accept_language_header("Accept-Language: en-us,fr\r\n");
     std::string ect_header = "chrome-proxy-ect: " +
@@ -583,27 +621,19 @@ class DataReductionProxyNetworkDelegateTest : public testing::Test {
                                  net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN)) +
                              "\r\n";
 
-    // Brotli is included in accept-encoding header only if the request went
-    // to the network (i.e., it was not a cached response), and if data
-    // reduction ptroxy network delegate added Brotli to the header.
-    std::string accept_encoding_header =
-        expect_brotli && !expect_cached
-            ? "Accept-Encoding: gzip, deflate, br\r\n"
-            : "Accept-Encoding: gzip, deflate\r\n";
-
     std::string suffix_headers =
         std::string("Chrome-Proxy: ") +
         io_data()->test_request_options()->GetHeaderValueForTesting() +
         std::string("\r\n\r\n");
 
-    std::string mock_write = prefix_headers + accept_language_header +
-                             ect_header + accept_encoding_header +
-                             suffix_headers;
-
-    if (expect_cached || !expect_brotli) {
-      // Order of headers is different if the headers were modified by data
-      // reduction proxy network delegate.
-      mock_write = prefix_headers + accept_encoding_header +
+    // If an Accept-Encoding header was provided, then Accept-Encoding appears
+    // before User-Agent; otherwise, it appears after it.
+    std::string mock_write;
+    if (has_accept_encoding_request_header) {
+      mock_write = prefix_headers + accept_encoding_header + user_agent_header +
+                   accept_language_header + ect_header + suffix_headers;
+    } else {
+      mock_write = prefix_headers + user_agent_header + accept_encoding_header +
                    accept_language_header + ect_header + suffix_headers;
     }
 
@@ -625,39 +655,35 @@ class DataReductionProxyNetworkDelegateTest : public testing::Test {
     if (!expect_cached) {
       EXPECT_EQ(response_body_size,
                 request->received_response_content_length());
-      EXPECT_NE(0, request->GetTotalSentBytes());
-      EXPECT_NE(0, request->GetTotalReceivedBytes());
-      EXPECT_FALSE(request->was_cached());
-      VerifyBrotliPresent(request.get(), expect_brotli);
-    } else {
-      EXPECT_TRUE(request->was_cached());
-      std::string content_encoding_value;
-      request->GetResponseHeaderByName("Content-Encoding",
-                                       &content_encoding_value);
-      EXPECT_EQ(expect_brotli, content_encoding_value == "br");
+      VerifyBrotliPresent(request.get(),
+                          accept_encoding_header_value_includes_brotli,
+                          expect_brotli);
     }
+    EXPECT_EQ(expect_cached, request->GetTotalSentBytes() == 0);
+    EXPECT_EQ(expect_cached, request->GetTotalReceivedBytes() == 0);
+    EXPECT_EQ(expect_cached, request->was_cached());
   }
 
-  void VerifyBrotliPresent(net::URLRequest* request, bool expect_brotli) {
+  void VerifyBrotliPresent(net::URLRequest* request,
+                           bool expect_accept_encoding_brotli,
+                           bool expect_content_encoding_brotli) {
     net::HttpRequestHeaders request_headers_sent;
     EXPECT_TRUE(request->GetFullRequestHeaders(&request_headers_sent));
     std::string accept_encoding_value;
     EXPECT_TRUE(request_headers_sent.GetHeader("Accept-Encoding",
                                                &accept_encoding_value));
-    EXPECT_NE(std::string::npos, accept_encoding_value.find("gzip"));
+
+    std::set<std::string> accept_encoding_value_set;
+    net::HttpUtil::ParseAcceptEncoding(accept_encoding_value,
+                                       &accept_encoding_value_set);
+    EXPECT_EQ(expect_accept_encoding_brotli,
+              accept_encoding_value_set.find("br") !=
+                  accept_encoding_value_set.end());
 
     std::string content_encoding_value;
     request->GetResponseHeaderByName("Content-Encoding",
                                      &content_encoding_value);
-
-    if (expect_brotli) {
-      // Brotli should be the last entry in the Accept-Encoding header.
-      EXPECT_EQ(accept_encoding_value.length() - 2,
-                accept_encoding_value.find("br"));
-      EXPECT_EQ("br", content_encoding_value);
-    } else {
-      EXPECT_EQ(std::string::npos, accept_encoding_value.find("br"));
-    }
+    EXPECT_EQ(expect_content_encoding_brotli, content_encoding_value == "br");
   }
 
   void FetchURLRequestAndVerifyPageIdDirective(base::Optional<uint64_t> page_id,
@@ -1729,7 +1755,7 @@ TEST_F(DataReductionProxyNetworkDelegateTest,
   EXPECT_NE(0, request->GetTotalReceivedBytes());
   EXPECT_FALSE(request->was_cached());
   // Brotli should be added to Accept Encoding header only if secure proxy is in
-  VerifyBrotliPresent(request.get(), false);
+  VerifyBrotliPresent(request.get(), false, false);
 }
 
 // Test that Brotli is not added to the accept-encoding header when it is
@@ -1773,6 +1799,54 @@ TEST_F(DataReductionProxyNetworkDelegateTest, BrotliAdvertisement) {
   FetchURLRequestAndVerifyBrotli(nullptr, response_headers, true, true);
 }
 
+// Test that Brotli is not added a second time to the Accept-Encoding header
+// when it is enabled globally but already present in the pre-existing header.
+TEST_F(DataReductionProxyNetworkDelegateTest,
+       BrotliAdvertisementAcceptEncodingIncludesBr) {
+  Init(USE_SECURE_PROXY, true /* enable_brotli_globally */);
+
+  net::HttpRequestHeaders request_headers;
+  request_headers.AddHeaderFromString("Accept-Encoding: gzip, deflate, br");
+
+  std::string response_headers =
+      "HTTP/1.1 200 OK\r\n"
+      "Via: 1.1 Chrome-Compression-Proxy\r\n"
+      "Chrome-Proxy: ofcl=200\r\n"
+      "Cache-Control: max-age=1200\r\n"
+      "Content-Encoding: br\r\n"
+      "Vary: accept-encoding\r\n";
+  response_headers += "\r\n";
+
+  FetchURLRequestAndVerifyBrotli(&request_headers, response_headers, false,
+                                 true);
+  FetchURLRequestAndVerifyBrotli(&request_headers, response_headers, true,
+                                 true);
+}
+
+// Test that Brotli is correctly added to the Accept-Encoding header when it is
+// enabled globally and the pre-existing header is empty.
+TEST_F(DataReductionProxyNetworkDelegateTest,
+       BrotliAdvertisementAcceptEncodingEmpty) {
+  Init(USE_SECURE_PROXY, true /* enable_brotli_globally */);
+
+  net::HttpRequestHeaders request_headers;
+  request_headers.AddHeaderFromString("Accept-Encoding:");
+
+  std::string response_headers =
+      "HTTP/1.1 200 OK\r\n"
+      "Via: 1.1 Chrome-Compression-Proxy\r\n"
+      "Chrome-Proxy: ofcl=200\r\n"
+      "Cache-Control: max-age=1200\r\n"
+      "Content-Encoding: br\r\n"
+      "Vary: accept-encoding\r\n";
+  response_headers += "\r\n";
+
+  FetchURLRequestAndVerifyBrotli(&request_headers, response_headers, false,
+                                 true);
+  FetchURLRequestAndVerifyBrotli(&request_headers, response_headers, true,
+                                 true);
+}
+
 TEST_F(DataReductionProxyNetworkDelegateTest, IncrementingMainFramePageId) {
   // This is unaffacted by brotil and insecure proxy.
   Init(USE_SECURE_PROXY, false /* enable_brotli_globally */);