Support bypassList in Proxy Settings API.

chrome/browser/extensions/extension_proxy_api.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/extensions/extension_proxy_api.h"
 
 #include "base/string_util.h"
+#include "base/string_tokenizer.h"
 #include "base/values.h"
 #include "chrome/browser/prefs/proxy_config_dictionary.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/common/pref_names.h"
 #include "net/proxy/proxy_config.h"
 
 namespace {
 
 // The scheme for which to use a manually specified proxy, not of the proxy URI
@@ skipping 24 matching lines @@
                               "ftp",
                               "socks" };
 
 // String literals in dictionaries used to communicate with extension.
 const char kProxyCfgMode[] = "mode";
 const char kProxyCfgPacScript[] = "pacScript";
 const char kProxyCfgPacScriptUrl[] = "url";
 const char kProxyCfgRules[] = "rules";
 const char kProxyCfgRuleHost[] = "host";
 const char kProxyCfgRulePort[] = "port";
+const char kProxyCfgBypassList[] = "bypassList";
 const char kProxyCfgScheme[] = "scheme";
 
 COMPILE_ASSERT(SCHEME_MAX == SCHEME_SOCKS, SCHEME_MAX_must_equal_SCHEME_SOCKS);
 COMPILE_ASSERT(arraysize(field_name) == SCHEME_MAX + 1,
                field_name_array_is_wrong_size);
 COMPILE_ASSERT(arraysize(scheme_name) == SCHEME_MAX + 1,
                scheme_name_array_is_wrong_size);
 COMPILE_ASSERT(SCHEME_ALL == 0, singleProxy_must_be_first_option);
 
+bool TokenizeToStringList(
+    const std::string& in, const std::string& delims, ListValue** out) {
+  scoped_ptr<ListValue> result(new ListValue);
+  StringTokenizer entries(in, delims);
+  while (entries.GetNext()) {
+    result->Append(Value::CreateStringValue(entries.token()));
+  }
+  *out = result.release();
+  return true;
+}
+
+bool JoinStringList(
+    ListValue* list, const std::string& joiner, std::string* out) {
+  std::string result;
+  for (size_t i = 0; i < list->GetSize(); ++i) {
+    if (!result.empty())
+      result.append(joiner);
+    std::string entry;
+    if (!list->GetString(i, &entry))
+      return false;
+    result.append(entry);
+  }
+  *out = result;
+  return true;
+}
+
 // Converts a proxy server description |dict| as passed by the API caller
 // (e.g. for the http proxy in the rules element) and converts it to a
 // ProxyServer. Returns true if successful.
 bool GetProxyServer(const DictionaryValue* dict,
                     net::ProxyServer::Scheme default_scheme,
                     net::ProxyServer* proxy_server) {
   std::string scheme_string;  // optional.
   dict->GetString(kProxyCfgScheme, &scheme_string);
 
   net::ProxyServer::Scheme scheme =
@@ skipping 68 matching lines @@
       proxy_pref.append(scheme_name[i]);
       proxy_pref.append("=");
       proxy_pref.append(proxy_server[i].ToURI());
     }
   }
 
   *out = proxy_pref;
   return true;
 }
 
+// Retrieves the "bypassList" from a proxy "rules" element passed by the API
+// caller in a proxy configuration and returns it as a joined string.

[eroman, 2011/02/11 04:52:28]

The wording of this comment doesn't seem right.

[battre, 2011/02/11 15:06:03]

Done.

+// Returns true if successful (i.e. string could be delivered or no "bypassList"
+// exists in the |proxy_rules|).
+bool GetBypassList(DictionaryValue* proxy_rules, std::string* out) {
+  if (!proxy_rules)
+    return false;
+
+  ListValue* bypass_list;
+  if (!proxy_rules->HasKey(kProxyCfgBypassList)) {
+    *out = "";
+    return true;
+  }
+  if (!proxy_rules->GetList(kProxyCfgBypassList, &bypass_list))
+    return false;
+
+  return JoinStringList(bypass_list, ",", out);
+}
+
 }  // namespace
 
 void ProxySettingsFunction::ApplyPreference(const char* pref_path,
                                             Value* pref_value,
                                             bool incognito) {
   Profile* use_profile = profile();
   if (use_profile->IsOffTheRecord())
     use_profile = use_profile->GetOriginalProfile();
 
   use_profile->GetExtensionService()->extension_prefs()->
@@ skipping 39 matching lines @@
   }
 
   DictionaryValue* proxy_rules = NULL;
   proxy_config->GetDictionary(kProxyCfgRules, &proxy_rules);
   std::string proxy_rules_string;
   if (proxy_rules && !GetProxyRules(proxy_rules, &proxy_rules_string)) {
     LOG(ERROR) << "Invalid 'rules' specified. "
                << "Setting custom proxy settings failed.";
     return false;
   }
+  std::string bypass_list;
+  if (proxy_rules && !GetBypassList(proxy_rules, &bypass_list)) {

[eroman, 2011/02/11 04:52:28]

There are string encoding problems in this file.

Note that strings originating from the javascript are wide. When calling
DictionaryValue:GetString() with a std::string, really we are getting UTF-8
encoded strings.

However a lot of this file is doing tokenizing assuming an ASCII input. And
certainly the net::* proxy parsing functions which are ultimately passed these
inputs, is similarly expecting ASCII input.

I generally recommend avoiding this pitfall by only ever extracting strings from
Value's using string16 -- this forces the code to have to deal with unicode of
the string, and avoids passing it around to functions that take ASCII inputs
(since you get compiletime failures!)

For now you might want to call DictionaryValue::GetStringAsASCII(), so the other
code doesn't need to be prepared to handle UTF8, and then just file some
TODOs/bugs on handling wide inputs.

You might be wandering whether it makes sense for javascript to be passing wide
strings -- yes it does. As far as the network stack is concerned, hostnames are
always ASCII. However as far as the user is concerned, international hostnames
can be represented using unicode (there is a translation layer to ASCII called
punycode).

This same issues has come up for PAC scripts, which similarly are javascript
code doing proxy stuff. We currently handle wide hostnames issued by PAC scripts
correctly (in the dnsResolve()/dnsResolveEx() bindings. However there is an
outstanding bug to create UTF16 aware parsing routines for proxy lists. See
http://crbug.com/47234.

[battre, 2011/02/11 15:06:03]

I have replaced all GetString calls with GetStringASCII where extension_api.json
prescribes an enum value and therefore guarantees an ASCII value.

For URLs I have used GetString(string16*), and bail out if IsStringASCII(string)
returns false. Otherwise I convert the string with UTF16ToASCII().

I have added TODO(battre)s with a reference to http://crbug.com/72692 in the
code.

So for now, the proxy settings API will accept only ASCII strings. I will do the
UTF-8 CodeLab and see how I can resolve this.

Done.

+    LOG(ERROR) << "Invalid 'bypassList' specified. "
+               << "Setting custom proxy settings failed.";
+    return false;
+  }
 
-  // not supported, yet.
-  std::string bypass_list;
-
   DictionaryValue* result_proxy_config = NULL;
   switch (mode_enum) {
     case ProxyPrefs::MODE_DIRECT:
       result_proxy_config = ProxyConfigDictionary::CreateDirect();
       break;
     case ProxyPrefs::MODE_AUTO_DETECT:
       result_proxy_config = ProxyConfigDictionary::CreateAutoDetect();
       break;
     case ProxyPrefs::MODE_PAC_SCRIPT: {
       if (!pac_dict) {
@@ skipping 86 matching lines @@
       if (!dict.GetPacUrl(&pac_url)) {
         LOG(ERROR) << "Missing pac url";
         return false;
       }
       DictionaryValue* pac_dict = new DictionaryValue;
       pac_dict->SetString(kProxyCfgPacScriptUrl, pac_url);
       api_proxy_config->Set(kProxyCfgPacScript, pac_dict);
       break;
     }
     case ProxyPrefs::MODE_FIXED_SERVERS: {
-      // TODO(battre): Handle bypass list.
+      DictionaryValue* rules_dict = new DictionaryValue;
+
       std::string proxy_servers;
       if (!dict.GetProxyServer(&proxy_servers)) {
-        LOG(ERROR) << "Missing proxy servers";
+        LOG(ERROR) << "Missing proxy servers in configuration";
         return false;
       }
-      DictionaryValue* rules_dict = new DictionaryValue;
       if (!ParseRules(proxy_servers, rules_dict)) {
         LOG(ERROR) << "Could not parse proxy rules";
         return false;
       }
+
+      bool hasBypassList = dict.HasBypassList();
+      if (hasBypassList) {
+        std::string bypass_list_string;
+        if (!dict.GetBypassList(&bypass_list_string)) {
+          LOG(ERROR) << "Invalid bypassList in configuration";
+          return false;

[eroman, 2011/02/11 04:52:28]

See comment below.

[battre, 2011/02/11 15:06:03]

Done.

+        }
+        ListValue* bypass_list = NULL;
+        if (TokenizeToStringList(bypass_list_string, ",;", &bypass_list)) {
+          rules_dict->Set(kProxyCfgBypassList, bypass_list);
+        } else {
+          LOG(ERROR) << "Error parsing bypassList " << bypass_list_string;
+          return false;

[eroman, 2011/02/11 04:52:28]

Isn't this going to leak |rules_dict|?

I suggest putting rules_dict into a scoped_ptr<> to avoid that.

[battre, 2011/02/11 15:06:03]

Done.

+        }
+      }
       api_proxy_config->Set(kProxyCfgRules, rules_dict);
       break;
     }
     case ProxyPrefs::kModeCount:
       NOTREACHED();
   }
   return true;
 }
 
 bool GetCurrentProxySettingsFunction::ParseRules(const std::string& rules,
@@ skipping 50 matching lines @@
       break;
     case net::ProxyServer::SCHEME_DIRECT:
     case net::ProxyServer::SCHEME_INVALID:
       NOTREACHED();
       return out;
   }
   out->SetString(kProxyCfgRuleHost, proxy.host_port_pair().host());
   out->SetInteger(kProxyCfgRulePort, proxy.host_port_pair().port());
   return out;
 }