ONC: Fix handling of invalid proxy scheme.

chromeos/network/onc/onc_utils.cc

 // Copyright (c) 2012 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 "chromeos/network/onc/onc_utils.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include "base/base64.h"
@@ skipping 18 matching lines @@
 #include "chromeos/network/onc/onc_normalizer.h"
 #include "chromeos/network/onc/onc_signature.h"
 #include "chromeos/network/onc/onc_translator.h"
 #include "chromeos/network/onc/onc_utils.h"
 #include "chromeos/network/onc/onc_validator.h"
 #include "components/device_event_log/device_event_log.h"
 #include "components/onc/onc_pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "components/proxy_config/proxy_config_dictionary.h"
 #include "components/signin/core/account_id/account_id.h"
+#include "components/url_formatter/url_fixer.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 #include "crypto/encryptor.h"
 #include "crypto/hmac.h"
 #include "crypto/symmetric_key.h"
 #include "net/base/host_port_pair.h"
 #include "net/cert/pem_tokenizer.h"
 #include "net/cert/x509_certificate.h"
 #include "net/proxy/proxy_bypass_rules.h"
 #include "net/proxy/proxy_config.h"
@@ skipping 875 matching lines @@
 
   if (type == ::onc::proxy::kDirect) {
     proxy_dict.reset(ProxyConfigDictionary::CreateDirect());
   } else if (type == ::onc::proxy::kWPAD) {
     proxy_dict.reset(ProxyConfigDictionary::CreateAutoDetect());
   } else if (type == ::onc::proxy::kPAC) {
     std::string pac_url;
     onc_proxy_settings.GetStringWithoutPathExpansion(::onc::proxy::kPAC,
                                                      &pac_url);
     GURL url(pac_url);
-    DCHECK(url.is_valid()) << "Invalid URL in ProxySettings.PAC";
+    if (!url.is_valid()) {

[Peter Kasting, 2017/01/06 01:59:33]

Why check for an invalid GURL before running it through fixup?  Why not just:

  GURL url(url_formatter::FixupURL(pac_url, std::string()));

[stevenjb, 2017/01/06 17:36:14]

Done.

+      // Assume the scheme is missing, use http.
+      url = GURL(url_formatter::FixupURL(pac_url, std::string()));
+      if (!url.is_valid()) {
+        LOG(WARNING) << "Invalid URL in ProxySettings.PAC: " << pac_url;

[Peter Kasting, 2017/01/06 01:59:33]

Generally, I suggest people avoid LOGs, as they bloat the binary and we don't
normally collect and act on them in some meaningful way.  Is there something
useful you're going to do with this logging data?  If not, avoid logging.

[stevenjb, 2017/01/06 17:36:14]

Done.

+        url = GURL();

[Peter Kasting, 2017/01/06 01:59:33]

Do you need to do this?  Is it safe to just call .possibly_invalid_spec() below
instead, or will something later blow up if the URL is invalid?  If the latter,
we could save a line by doing:

    proxy_dict.reset(ProxyConfigDictionary::CreatePacScript(
        url.is_valid() ? url.spec() : std::string(), false));

[stevenjb, 2017/01/06 17:36:14]

Something later will blow up if this is invalid. Also, this will provide
immediate feedback in the UI that the url is invalid.

Combined the lines.

+      }
+    }
     proxy_dict.reset(ProxyConfigDictionary::CreatePacScript(url.spec(), false));
   } else if (type == ::onc::proxy::kManual) {
     const base::DictionaryValue* manual_dict = nullptr;
     onc_proxy_settings.GetDictionaryWithoutPathExpansion(::onc::proxy::kManual,
                                                          &manual_dict);
     std::string manual_spec;
     AppendProxyServerForScheme(*manual_dict, ::onc::proxy::kFtp, &manual_spec);
     AppendProxyServerForScheme(*manual_dict, ::onc::proxy::kHttp, &manual_spec);
     AppendProxyServerForScheme(*manual_dict, ::onc::proxy::kSocks,
                                &manual_spec);
@@ skipping 373 matching lines @@
                          const PrefService* local_state_prefs,
                          const NetworkState& network) {
   ::onc::ONCSource ignored_onc_source;
   const base::DictionaryValue* policy = onc::GetPolicyForNetwork(
       profile_prefs, local_state_prefs, network, &ignored_onc_source);
   return policy != NULL;
 }
 
 }  // namespace onc
 }  // namespace chromeos