Add verification that google URL has a valid TLD.

components/google/core/browser/google_util.cc

Index: components/google/core/browser/google_util.cc
diff --git a/components/google/core/browser/google_util.cc b/components/google/core/browser/google_util.cc
index f3e8fd432b8e35afac9835c7e8727b6ff0ac2f27..2f561e535b2c90a4c8521b436ffb3db225110832 100644
--- a/components/google/core/browser/google_util.cc
+++ b/components/google/core/browser/google_util.cc
@@ -46,13 +46,17 @@ bool IsPathHomePageBase(base::StringPiece path) {
 
 // True if the given canonical |host| is "[www.]<domain_in_lower_case>.<TLD>"
 // with a valid TLD. If |subdomain_permission| is ALLOW_SUBDOMAIN, we check
-// against host "*.<domain_in_lower_case>.<TLD>" instead.
+// against host "*.<domain_in_lower_case>.<TLD>" instead. Will return the TLD
+// string in |tld|, if specified and the |host| can be parsed.
 bool IsValidHostName(base::StringPiece host,
                      base::StringPiece domain_in_lower_case,
-                     SubdomainPermission subdomain_permission) {
+                     SubdomainPermission subdomain_permission,
+                     base::StringPiece* tld) {
+

[Peter Kasting, 2016/12/15 01:21:59]

Nit: Don't add a blank line here.

[Maria, 2016/12/15 17:56:33]

Done.

   // Fast path to avoid searching the registry set.
   if (host.find(domain_in_lower_case) == base::StringPiece::npos)
     return false;
+
   size_t tld_length =
       net::registry_controlled_domains::GetCanonicalHostRegistryLength(
           host, net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
@@ -63,6 +67,10 @@ bool IsValidHostName(base::StringPiece host,
   // Removes the tld and the preceding dot.
   base::StringPiece host_minus_tld =
       host.substr(0, host.length() - tld_length - 1);
+
+  if (tld)
+    *tld = host.substr(host.length() - tld_length);
+
   if (base::LowerCaseEqualsASCII(host_minus_tld, domain_in_lower_case))
     return true;
 
@@ -92,7 +100,50 @@ bool IsCanonicalHostGoogleHostname(base::StringPiece canonical_host,
   if (base_url.is_valid() && (canonical_host == base_url.host_piece()))
     return true;
 
-  return IsValidHostName(canonical_host, "google", subdomain_permission);
+  base::StringPiece tld;
+  bool valid = IsValidHostName(canonical_host, "google", subdomain_permission,
+                               &tld);
+  if (!valid || tld.empty())
+    return valid;

[Peter Kasting, 2016/12/15 01:21:59]

Why check "|| tld.empty()" here?

It seems like this can never happen if |valid| is true, since IsValidHostName()
checks for 0/npos for the tld_length and returns false.

It's also confusing, since it somehow the TLD could be empty on a valid
hostname, we'd return true here, which would be wrong.

So it seems better to rewrite as just:

  if (!IsValidHostName(...))
    return false;

We don't even need to DCHECK(!tld.empty()), since an empty TLD would not cause
correctness problems (it'd just do an unnecessary set check).

[Maria, 2016/12/15 17:56:33]

Done.

+
+  static std::set<std::string> google_tlds = {"ac", "ad", "ae", "af", "ag",

[Peter Kasting, 2016/12/15 01:21:59]

Use CR_DEFINE_STATIC_LOCAL for this.

[Maria, 2016/12/15 17:56:33]

I don't think I can because CR_DEFINE_STATIC_LOCAL macro is defined as
"static type& name = *new type arguments", which wouldn't work with initializer
lists that I'm using here.

[Peter Kasting, 2016/12/15 19:00:25]

I think this should work:

CR_DEFINE_STATIC_LOCAL(std::set<std::string>, google_tlds, ({...}));

I've seen this elsewhere, e.g.
https://cs.chromium.org/chromium/src/chrome/browser/global_keyboard_shortcuts...
.

+      "al", "am", "as", "at", "aw", "az", "ba", "be", "bf", "bg", "bi", "biz",
+      "bj", "bm", "bn", "bo", "bs", "bt", "by", "bz", "ca", "cat", "cc", "cd",
+      "cf", "cg", "ch", "ci", "cl", "cm", "cn", "co", "co.ao", "co.at", "co.ba",
+      "co.bi", "co.bw", "co.ci", "co.ck", "co.cr", "co.gg", "co.gl", "co.gy",
+      "co.hu", "co.id", "co.il", "co.im", "co.in", "co.it", "co.je", "co.jp",
+      "co.ke", "co.kr", "co.ls", "co.ma", "co.mu", "co.mw", "co.mz", "co.nz",
+      "co.pn", "co.rs", "co.th", "co.tt", "co.tz", "co.ua", "co.ug", "co.uk",
+      "co.uz", "co.ve", "co.vi", "co.za", "co.zm", "co.zw", "com", "com.af",
+      "com.ag", "com.ai", "com.ar", "com.au", "com.az", "com.bd", "com.bh",
+      "com.bi", "com.bn", "com.bo", "com.br", "com.bs", "com.by", "com.bz",
+      "com.cn", "com.co", "com.cu", "com.cy", "com.do", "com.dz", "com.ec",
+      "com.eg", "com.er", "com.et", "com.fj", "com.ge", "com.gh", "com.gi",
+      "com.gl", "com.gp", "com.gr", "com.gt", "com.gy", "com.hk", "com.hn",
+      "com.hr", "com.ht", "com.iq", "com.jm", "com.jo", "com.kg", "com.kh",
+      "com.ki", "com.kw", "com.kz", "com.lb", "com.lc", "com.lk", "com.lv",
+      "com.ly", "com.mk", "com.mm", "com.mt", "com.mu", "com.mw", "com.mx",
+      "com.my", "com.na", "com.nc", "com.nf", "com.ng", "com.ni", "com.np",
+      "com.nr", "com.om", "com.pa", "com.pe", "com.pg", "com.ph", "com.pk",
+      "com.pl", "com.pr", "com.ps", "com.pt", "com.py", "com.qa", "com.ru",
+      "com.sa", "com.sb", "com.sc", "com.sg", "com.sl", "com.sv", "com.tj",
+      "com.tm", "com.tn", "com.tr", "com.tt", "com.tw", "com.ua", "com.uy",
+      "com.uz", "com.vc", "com.ve", "com.vi", "com.vn", "com.ws", "cv", "cx",
+      "cz", "de", "dj", "dk", "dm", "do", "dz", "ec", "ee", "es", "eu", "fi",
+      "fm", "fr", "ga", "gd", "ge", "gf", "gg", "gl", "gm", "gp", "gr", "gw",
+      "gy", "hk", "hn", "hr", "ht", "hu", "ie", "im", "in", "info", "in.rs",
+      "io", "iq", "is", "it", "it.ao", "je", "jo", "jobs", "jp", "kg", "ki",
+      "kids.us", "km", "kn", "kr", "kz", "la", "li", "lk", "lt", "lu", "lv",
+      "ma", "md", "me", "mg", "mh", "mk", "ml", "mn", "mobi", "mr", "ms", "mu",
+      "mv", "mw", "mx", "name", "ne", "ne.jp", "net", "net.in", "net.nz", "nf",
+      "ng", "nl", "no", "nom.es", "nr", "nu", "off.ai", "org", "org.af",
+      "org.es", "org.in", "org.nz", "org.uk", "pf", "ph", "pk", "pl", "pn",
+      "pr", "pro", "ps", "pt", "qa", "re", "ro", "rs", "ru", "rw", "sc", "se",
+      "sg", "sh", "si", "sk", "sl", "sm", "sn", "so", "sr", "st", "sz", "td",
+      "tel", "tg", "tk", "tl", "tm", "tn", "to", "tt", "tv", "tw", "ua", "ug",
+      "us", "uz", "vc", "vg", "vn", "vu", "ws", "yt"};
+
+  return google_tlds.find(tld.as_string()) != google_tlds.end();

[Peter Kasting, 2016/12/15 01:21:59]

Nit: Reads more clearly:

  return base::ContainsKey(google_tlds, tld.as_string());

[Maria, 2016/12/15 17:56:33]

Done.

 }
 
 }  // namespace
@@ -233,7 +284,8 @@ bool IsYoutubeDomainUrl(const GURL& url,
                         SubdomainPermission subdomain_permission,
                         PortPermission port_permission) {
   return IsValidURL(url, port_permission) &&
-      IsValidHostName(url.host_piece(), "youtube", subdomain_permission);
+      IsValidHostName(url.host_piece(), "youtube", subdomain_permission,
+                      nullptr);
 }
 
 }  // namespace google_util