Fix issue that causes some extensions to be disabled right after installation.

Add an option to Extension::GetDistinctHosts that strips the RCDs off the hosts.

BUG=65138
TEST=ExtensionsServiceTest, ExtensionTest

Review URL: http://codereview.chromium.org/5642001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@68446 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 147 insertions, 35 deletions

diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index f83202c..530c0be 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -320,20 +320,47 @@ std::set<string16> Extension::GetSimplePermissionMessages() const {
   return messages;
 }
 
-std::vector<std::string> Extension::GetDistinctHosts() const {
-  return GetDistinctHosts(GetEffectiveHostPermissions().patterns());
+// static
+std::vector<std::string> Extension::GetDistinctHostsForDisplay(
+    const URLPatternList& list) {
+  return GetDistinctHosts(list, true);
 }
 
 // static
-std::vector<std::string> Extension::GetDistinctHosts(
-    const URLPatternList& host_patterns) {
+bool Extension::IsElevatedHostList(
+    const URLPatternList& old_list, const URLPatternList& new_list) {
+  // TODO(jstritar): This is overly conservative with respect to subdomains.
+  // For example, going from *.google.com to www.google.com will be
+  // considered an elevation, even though it is not (http://crbug.com/65337).
+
+  std::vector<std::string> new_hosts = GetDistinctHosts(new_list, false);
+  std::vector<std::string> old_hosts = GetDistinctHosts(old_list, false);
+
+  std::set<std::string> old_hosts_set(old_hosts.begin(), old_hosts.end());
+  std::set<std::string> new_hosts_set(new_hosts.begin(), new_hosts.end());
+  std::set<std::string> new_hosts_only;
+
+  std::set_difference(new_hosts_set.begin(), new_hosts_set.end(),
+                      old_hosts_set.begin(), old_hosts_set.end(),
+                      std::inserter(new_hosts_only, new_hosts_only.begin()));
+
+  return new_hosts_only.size() > 0;
+}
 
+// static
+std::vector<std::string> Extension::GetDistinctHosts(
+    const URLPatternList& host_patterns, bool include_rcd) {
   // Vector because we later want to access these by index.
   std::vector<std::string> distinct_hosts;
 
   std::set<std::string> rcd_set;
   for (size_t i = 0; i < host_patterns.size(); ++i) {
     std::string candidate = host_patterns[i].host();
+
+    // Add the subdomain wildcard back to the host, if necessary.
+    if (host_patterns[i].match_subdomains())
+      candidate = "*." + candidate;
+
     size_t registry = net::RegistryControlledDomainService::GetRegistryLength(
         candidate, false);
     if (registry && registry != std::string::npos) {
@@ -341,6 +368,8 @@ std::vector<std::string> Extension::GetDistinctHosts(
       if (rcd_set.count(no_rcd))
         continue;
       rcd_set.insert(no_rcd);
+      if (!include_rcd)
+        candidate = no_rcd;
     }
     if (std::find(distinct_hosts.begin(), distinct_hosts.end(), candidate) ==
                   distinct_hosts.end()) {
@@ -355,7 +384,9 @@ string16 Extension::GetHostPermissionMessage() const {
   if (HasEffectiveAccessToAllHosts())
     return l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_ALL_HOSTS);
 
-  std::vector<std::string> hosts = GetDistinctHosts();
+  std::vector<std::string> hosts = GetDistinctHostsForDisplay(
+      GetEffectiveHostPermissions().patterns());
+
   if (hosts.size() == 1) {
     return l10n_util::GetStringFUTF16(IDS_EXTENSION_PROMPT_WARNING_1_HOST,
                                       UTF8ToUTF16(hosts[0]));
@@ -1120,20 +1151,8 @@ bool Extension::IsPrivilegeIncrease(const bool granted_full_access,
 
     const ExtensionExtent new_extent =
         new_extension->GetEffectiveHostPermissions();
-    std::vector<std::string> new_hosts =
-        GetDistinctHosts(new_extent.patterns());
-    std::vector<std::string> old_hosts =
-        GetDistinctHosts(granted_extent.patterns());
-
-    std::set<std::string> old_hosts_set(old_hosts.begin(), old_hosts.end());
-    std::set<std::string> new_hosts_set(new_hosts.begin(), new_hosts.end());
-    std::set<std::string> new_hosts_only;
-
-    std::set_difference(new_hosts_set.begin(), new_hosts_set.end(),
-                        old_hosts_set.begin(), old_hosts_set.end(),
-                        std::inserter(new_hosts_only, new_hosts_only.begin()));
 
-    if (new_hosts_only.size())
+    if (IsElevatedHostList(granted_extent.patterns(), new_extent.patterns()))
       return true;
   }
 
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index a8a05f5..0d171d8 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -123,13 +123,21 @@ class Extension : public base::RefCountedThreadSafe<Extension> {
   // should display at install time.
   std::vector<string16> GetPermissionMessages() const;
 
-  // Returns the distinct hosts that should be displayed in the install UI. This
-  // discards some of the detail that is present in the manifest to make it as
-  // easy as possible to process by users. In particular we disregard the scheme
-  // and path components of URLPatterns and de-dupe the result.
-  static std::vector<std::string> GetDistinctHosts(
-      const URLPatternList& host_patterns);
-  std::vector<std::string> GetDistinctHosts() const;
+  // Returns the distinct hosts that should be displayed in the install UI
+  // for the URL patterns |list|. This discards some of the detail that is
+  // present in the manifest to make it as easy as possible to process by
+  // users. In particular we disregard the scheme and path components of
+  // URLPatterns and de-dupe the result, which includes filtering out common
+  // hosts with differing RCDs. (NOTE: when de-duping hosts with common RCDs,
+  // the first pattern is returned and the rest discarded)
+  static std::vector<std::string> GetDistinctHostsForDisplay(
+      const URLPatternList& list);
+
+  // Compares two URLPatternLists for security equality by returning whether
+  // the URL patterns in |new_list| contain additional distinct hosts compared
+  // to |old_list|.
+  static bool IsElevatedHostList(
+      const URLPatternList& old_list, const URLPatternList& new_list);
 
   // Icon sizes used by the extension system.
   static const int kIconSizes[];
@@ -456,6 +464,22 @@ class Extension : public base::RefCountedThreadSafe<Extension> {
   // sure the drive letter is uppercase.
   static FilePath MaybeNormalizePath(const FilePath& path);
 
+  // Returns the distinct hosts that can be displayed in the install UI or be
+  // used for privilege comparisons. This discards some of the detail that is
+  // present in the manifest to make it as easy as possible to process by users.
+  // In particular we disregard the scheme and path components of URLPatterns
+  // and de-dupe the result, which includes filtering out common hosts with
+  // differing RCDs. If |include_rcd| is true, then the de-duped result
+  // will be the first full entry, including its RCD. So if the list was
+  // "*.google.co.uk" and "*.google.com", the returned value would just be
+  // "*.google.co.uk". Keeping the RCD in the result is useful for display
+  // purposes when you want to show the user one sample hostname from the list.
+  // If you need to compare two URLPatternLists for security equality, then set
+  // |include_rcd| to false, which will return a result like "*.google.",
+  // regardless of the order of the patterns.
+  static std::vector<std::string> GetDistinctHosts(
+      const URLPatternList& host_patterns, bool include_rcd);
+
   Extension(const FilePath& path, Location location);
   ~Extension();
 
diff --git a/chrome/common/extensions/extension_unittest.cc b/chrome/common/extensions/extension_unittest.cc
index 07bd6c6..1130302 100644
--- a/chrome/common/extensions/extension_unittest.cc
+++ b/chrome/common/extensions/extension_unittest.cc
@@ -827,7 +827,7 @@ TEST(ExtensionTest, IsPrivilegeIncrease) {
     const char* granted_apis[10];
     const char* granted_hosts[10];
     bool full_access;
-    bool expect_success;
+    bool expect_increase;
   } kTests[] = {
     { "allhosts1", {NULL}, {"http://*/", NULL}, false,
       false },  // all -> all
@@ -839,7 +839,7 @@ TEST(ExtensionTest, IsPrivilegeIncrease) {
       false },  // http://a,http://b -> http://a,http://b
     { "hosts2", {NULL},
       {"http://www.google.com/", "http://www.reddit.com/", NULL}, false,
-      false },  // http://a,http://b -> https://a,http://*.b
+      true },  // http://a,http://b -> https://a,http://*.b
     { "hosts3", {NULL},
       {"http://www.google.com/", "http://www.reddit.com/", NULL}, false,
       false },  // http://a,http://b -> http://a
@@ -899,7 +899,7 @@ TEST(ExtensionTest, IsPrivilegeIncrease) {
     if (!new_extension.get())
       continue;
 
-    EXPECT_EQ(kTests[i].expect_success,
+    EXPECT_EQ(kTests[i].expect_increase,
               Extension::IsPrivilegeIncrease(kTests[i].full_access,
                                              granted_apis,
                                              granted_hosts,
@@ -1103,7 +1103,7 @@ TEST(ExtensionTest, ApiPermissions) {
   }
 }
 
-TEST(ExtensionTest, GetDistinctHosts) {
+TEST(ExtensionTest, GetDistinctHostsForDisplay) {
   std::vector<std::string> expected;
   expected.push_back("www.foo.com");
   expected.push_back("www.bar.com");
@@ -1121,7 +1121,7 @@ TEST(ExtensionTest, GetDistinctHosts) {
     actual.push_back(
         URLPattern(URLPattern::SCHEME_HTTP, "http://www.baz.com/path"));
     CompareLists(expected,
-                 Extension::GetDistinctHosts(actual));
+                 Extension::GetDistinctHostsForDisplay(actual));
   }
 
   {
@@ -1133,7 +1133,7 @@ TEST(ExtensionTest, GetDistinctHosts) {
     actual.push_back(
         URLPattern(URLPattern::SCHEME_HTTP, "http://www.baz.com/path"));
     CompareLists(expected,
-                 Extension::GetDistinctHosts(actual));
+                 Extension::GetDistinctHostsForDisplay(actual));
   }
 
   {
@@ -1143,7 +1143,7 @@ TEST(ExtensionTest, GetDistinctHosts) {
     actual.push_back(
         URLPattern(URLPattern::SCHEME_HTTPS, "https://www.bar.com/path"));
     CompareLists(expected,
-                 Extension::GetDistinctHosts(actual));
+                 Extension::GetDistinctHostsForDisplay(actual));
   }
 
   {
@@ -1153,7 +1153,7 @@ TEST(ExtensionTest, GetDistinctHosts) {
     actual.push_back(
         URLPattern(URLPattern::SCHEME_HTTP, "http://www.bar.com/pathypath"));
     CompareLists(expected,
-                 Extension::GetDistinctHosts(actual));
+                 Extension::GetDistinctHostsForDisplay(actual));
   }
 
   {
@@ -1169,7 +1169,7 @@ TEST(ExtensionTest, GetDistinctHosts) {
     expected.push_back("bar.com");
 
     CompareLists(expected,
-                 Extension::GetDistinctHosts(actual));
+                 Extension::GetDistinctHostsForDisplay(actual));
   }
 
   {
@@ -1196,10 +1196,79 @@ TEST(ExtensionTest, GetDistinctHosts) {
     expected.push_back("www.foo.xyzzy");
 
     CompareLists(expected,
-                 Extension::GetDistinctHosts(actual));
+                 Extension::GetDistinctHostsForDisplay(actual));
+  }
+
+  {
+    SCOPED_TRACE("wildcards");
+
+    actual.push_back(
+        URLPattern(URLPattern::SCHEME_HTTP, "http://*.google.com/*"));
+
+    expected.push_back("*.google.com");
+
+    CompareLists(expected,
+                 Extension::GetDistinctHostsForDisplay(actual));
   }
 }
 
+TEST(ExtensionTest, IsElevatedHostList) {
+  URLPatternList list1;
+  URLPatternList list2;
+
+  list1.push_back(
+      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com.hk/path"));
+  list1.push_back(
+      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com/path"));
+
+  // Test that the host order does not matter.
+  list2.push_back(
+      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com/path"));
+  list2.push_back(
+      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com.hk/path"));
+
+  EXPECT_FALSE(Extension::IsElevatedHostList(list1, list2));
+  EXPECT_FALSE(Extension::IsElevatedHostList(list2, list1));
+
+  // Test that paths are ignored.
+  list2.clear();
+  list2.push_back(
+      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com/*"));
+  EXPECT_FALSE(Extension::IsElevatedHostList(list1, list2));
+  EXPECT_FALSE(Extension::IsElevatedHostList(list2, list1));
+
+  // Test that RCDs are ignored.
+  list2.clear();
+  list2.push_back(
+      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com.hk/*"));
+  EXPECT_FALSE(Extension::IsElevatedHostList(list1, list2));
+  EXPECT_FALSE(Extension::IsElevatedHostList(list2, list1));
+
+  // Test that subdomain wildcards are handled properly.
+  list2.clear();
+  list2.push_back(
+      URLPattern(URLPattern::SCHEME_HTTP, "http://*.google.com.hk/*"));
+  EXPECT_TRUE(Extension::IsElevatedHostList(list1, list2));
+  //TODO(jstritar): Does not match subdomains properly. http://crbug.com/65337
+  //EXPECT_FALSE(Extension::IsElevatedHostList(list2, list1));
+
+  // Test that different domains count as different hosts.
+  list2.clear();
+  list2.push_back(
+      URLPattern(URLPattern::SCHEME_HTTP, "http://www.google.com/path"));
+  list2.push_back(
+      URLPattern(URLPattern::SCHEME_HTTP, "http://www.example.org/path"));
+  EXPECT_TRUE(Extension::IsElevatedHostList(list1, list2));
+  EXPECT_FALSE(Extension::IsElevatedHostList(list2, list1));
+
+  // Test that different subdomains count as different hosts.
+  list2.clear();
+  list2.push_back(
+      URLPattern(URLPattern::SCHEME_HTTP, "http://mail.google.com/*"));
+  EXPECT_TRUE(Extension::IsElevatedHostList(list1, list2));
+  EXPECT_TRUE(Extension::IsElevatedHostList(list2, list1));
+}
+
 TEST(ExtensionTest, GenerateId) {
   std::string result;
   EXPECT_FALSE(Extension::GenerateId("", &result));