Temporary fix to prevent the DRP from being activated improperly.

chrome/browser/net/pref_proxy_config_tracker_impl.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/net/pref_proxy_config_tracker_impl.h"
 
 #include "base/bind.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
+#include "base/strings/string_util.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/prefs/proxy_config_dictionary.h"
 #include "chrome/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
+#include "net/proxy/proxy_list.h"
+#include "net/proxy/proxy_server.h"
 
 using content::BrowserThread;
 
+namespace {
+
+// Removes any Data Reduction Proxies like *.googlezip.net from |proxy_list|.

[eroman, 2015/05/12 00:43:01]

Please add more context on why this is being done, along with a link to the bug.

[sclittle, 2015/05/12 01:13:59]

Done, added to the comment of the RemoveGooglezipDataReductionProxies function
below.

+void RemoveGooglezipDataReductionProxiesFromList(net::ProxyList* proxy_list) {
+  if (proxy_list->IsEmpty())
+    return;
+
+  net::ProxyList replacement_list;

[eroman, 2015/05/12 00:43:01]

Please do this in two phases:
  (1) Scan the list to see if it contains at least 1 proxy ending in
".googlezip.net"
  (2) If it does then build the replacement list and swap.

The reason I would rather do it this way, is so there is less changes to the
normal codeflow (i.e. .googlezip.net is no in there).

[sclittle, 2015/05/12 01:13:59]

Done.

+  for (const net::ProxyServer& proxy : proxy_list->GetAll()) {
+    if (!proxy.is_valid() || proxy.is_direct() ||
+        !EndsWith(proxy.host_port_pair().host(), ".googlezip.net", true)) {
+      replacement_list.AddProxyServer(proxy);
+    }
+  }
+
+  if (replacement_list.IsEmpty())
+    replacement_list.AddProxyServer(net::ProxyServer::Direct());
+  if (replacement_list.size() != proxy_list->size())

[eroman, 2015/05/12 00:43:01]

This doesn't look correct. What if the list were:

  ["http://foo.googlezip.net]

In that case we will rewrite it to:

  ["direct://"]

Because of the line above. In which case sizes will be unchanged (1) however
replacement is in fact needed.

See my comment above which I think will also address this (do it in two phases).

[sclittle, 2015/05/12 01:13:59]

Thanks, I just noticed this as well when testing now.

+    *proxy_list = replacement_list;
+}
+
+// Remove any Data Reduction Proxies like *.googlezip.net from |proxy_rules|.
+// TODO(sclittle): Add UMA to record how often this method is called, and how

[eroman, 2015/05/12 00:43:01]

When to do you plan on doing this? If we are going to be adding this hack to M43
it should also have appropriate measurements.

[sclittle, 2015/05/12 01:13:59]

I'm planning on adding the measurements in M44. I'm trying to minimize the code
that gets merged into M43 here. It seems about as useful for us to measure these
stats in M44, since the full fix for this will likely also be in M44.

+// often it actually removes a *.googlezip.net proxy. This method should be
+// removed once it stops actually finding and removing *.googlezip.net proxies
+// from the proxy rules.
+void RemoveGooglezipDataReductionProxies(
+    net::ProxyConfig::ProxyRules* proxy_rules) {
+  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->fallback_proxies);
+  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->proxies_for_ftp);
+  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->proxies_for_http);
+  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->proxies_for_https);
+  RemoveGooglezipDataReductionProxiesFromList(&proxy_rules->single_proxies);
+}
+
+}  // namespace
+
 //============================= ChromeProxyConfigService =======================
 
 ChromeProxyConfigService::ChromeProxyConfigService(
     net::ProxyConfigService* base_service)
     : base_service_(base_service),
       pref_config_state_(ProxyPrefs::CONFIG_UNSET),
       pref_config_read_pending_(true),
       registered_observer_(false) {
 }
 
@@ skipping 141 matching lines @@
 // static
 net::ProxyConfigService::ConfigAvailability
     PrefProxyConfigTrackerImpl::GetEffectiveProxyConfig(
         ProxyPrefs::ConfigState pref_state,
         const net::ProxyConfig& pref_config,
         net::ProxyConfigService::ConfigAvailability system_availability,
         const net::ProxyConfig& system_config,
         bool ignore_fallback_config,
         ProxyPrefs::ConfigState* effective_config_state,
         net::ProxyConfig* effective_config) {
+  net::ProxyConfigService::ConfigAvailability rv;
   *effective_config_state = pref_state;
 
   if (PrefPrecedes(pref_state)) {
     *effective_config = pref_config;
-    return net::ProxyConfigService::CONFIG_VALID;
-  }
-
-  // If there's no system proxy config, fall back to prefs or default.
-  if (system_availability == net::ProxyConfigService::CONFIG_UNSET) {
+    rv = net::ProxyConfigService::CONFIG_VALID;
+  } else if (system_availability == net::ProxyConfigService::CONFIG_UNSET) {

[eroman, 2015/05/12 00:43:01]

OK. alternately you could have extracted it to a helper function and kept the
"return" short-circuits.

[sclittle, 2015/05/12 01:13:59]

Acknowledged. I'll leave it this way unless you'd prefer it as a separate helper
function.

+    // If there's no system proxy config, fall back to prefs or default.
     if (pref_state == ProxyPrefs::CONFIG_FALLBACK && !ignore_fallback_config)
       *effective_config = pref_config;
     else
       *effective_config = net::ProxyConfig::CreateDirect();
-    return net::ProxyConfigService::CONFIG_VALID;
+    rv = net::ProxyConfigService::CONFIG_VALID;
+  } else {
+    *effective_config_state = ProxyPrefs::CONFIG_SYSTEM;
+    *effective_config = system_config;
+    rv = system_availability;
   }
 
-  *effective_config_state = ProxyPrefs::CONFIG_SYSTEM;
-  *effective_config = system_config;
-  return system_availability;
+  // Remove any Data Reduction Proxies like *.googlezip.net from the proxy
+  // config rules, since specifying a DRP in the proxy rules is not a supported
+  // means of activating the DRP, and could cause requests to be sent to the DRP
+  // without the appropriate authentication headers and without using any of the
+  // DRP bypass logic. This prevents the Data Reduction Proxy from being
+  // improperly activated via the proxy pref.
+  // TODO(sclittle): This is a temporary hotfix for http://crbug.com/476610, and
+  // should be removed once that bug is fixed and verified.
+  if (rv == net::ProxyConfigService::CONFIG_VALID)
+    RemoveGooglezipDataReductionProxies(&effective_config->proxy_rules());
+
+  return rv;
 }
 
 // static
 void PrefProxyConfigTrackerImpl::RegisterPrefs(PrefRegistrySimple* registry) {
   base::DictionaryValue* default_settings =
       ProxyConfigDictionary::CreateSystem();
   registry->RegisterDictionaryPref(prefs::kProxy, default_settings);
 }
 
 // static
@@ skipping 134 matching lines @@
       (config_state_ != ProxyPrefs::CONFIG_UNSET &&
        !pref_config_.Equals(new_config))) {
     config_state_ = config_state;
     if (config_state_ != ProxyPrefs::CONFIG_UNSET)
       pref_config_ = new_config;
     update_pending_ = true;
   }
   if (update_pending_)
     OnProxyConfigChanged(config_state, new_config);
 }