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 a6654c9f28701ad6a10781ed527a4b6f12e486db..bca46e362cb8f97ca066dbe2680c2dbb699ed64e 100644
--- a/components/google/core/browser/google_util.cc
+++ b/components/google/core/browser/google_util.cc
@@ -38,6 +38,44 @@ namespace google_util {
 
 namespace {
 
+// TODO(mariakhomenko): figure out how to keep this list updated.
+static const char* const g_google_tld_list[] = {"ac", "ad", "ae", "af", "ag",

[Peter Kasting, 2016/11/15 22:11:56]

Nit: constexpr

If this remains used by only one function, prefer to declare this in the
function that actually uses it, so its intent is maximally clear.

[Maria, 2016/11/15 23:36:31]

Switched to constexpr.

+    "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"};
+
 bool gUseMockLinkDoctorBaseURLForTesting = false;
 
 bool IsPathHomePageBase(base::StringPiece path) {
@@ -89,7 +127,26 @@ 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);
+  bool valid = IsValidHostName(canonical_host, "google", subdomain_permission);
+  if (!valid)
+    return valid;
+
+  // Validate that we have a TLD that Google has registered.
+  size_t tld_length =
+        net::registry_controlled_domains::GetCanonicalHostRegistryLength(

[Peter Kasting, 2016/11/15 22:11:56]

This implementation computes the registry length (which is not cheap) both in
IsValidHostName() and here.  I'd like to see us find a way to only compute this
once.

It seems like besides here, the only other caller of IsValidHostName() is
IsYoutubeDomainUrl() (whose very existence makes my skin crawl; if at all
possible can we kill this?), which probably wants a similar TLD check.

[Maria, 2016/11/15 23:36:31]

Done -- switched to return tld from IsValidHostName.

I don't think we can get rid of IsYoutubeDomainUrl easily. I looked at the use
and it seems there are two:
1) safe browsing, which is also used for policy restrictions -- looks like it's
used to block youtube in some domain policies?
2) UMA variations headers are sent there -- I don't know how that's used, but if
we need that for experiment tracking, I don't see a way around that

If we wanted to check TLDs on youtube domain, we could do that as well -- but
that would be a separate list.

+            canonical_host,
+            net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
+            net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
+
+  base::StringPiece tld(
+      canonical_host.substr(canonical_host.length() - tld_length,
+                            base::StringPiece::npos));
+  for (size_t i = 0; g_google_tld_list[i]; i++) {
+    if (g_google_tld_list[i] == tld) {
+      return true;
+    }
+  }
+  return false;

[Peter Kasting, 2016/11/15 22:11:57]

This linear search is less-efficient than it could be even using your array
datatype, because your TLD list is alphabetically sorted.

But even more efficient would be to use a set or trie.

[Maria, 2016/11/15 23:36:31]

Agree. I was thinking about that. The reason I went with file-global const array
is because it's created once and reused. Whereas, if we used a data-structure,
then we need to either generate it every time the function is called or somehow
manage its memory (intentionally leak it?). Do you have thoughts on the right
approach here?

[Peter Kasting, 2016/11/15 23:46:50]

CR_DEFINE_STATIC_LOCAL within the function is the way to do this if all access
is single-threaded.  If you need something threadsafe, use a base::LazyInstance.

I don't know offhand what the threading requirements here are.

 }
 
 }  // namespace