Track permissions granted to extensions in prefs

chrome/common/extensions/extension.cc

Index: chrome/common/extensions/extension.cc
diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index 4d338224de956c5947c0476e5b7d0d34c342fd80..a8741a181492475bce5f04e8e000b33a1ef8973a 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -1079,52 +1079,65 @@ bool Extension::FormatPEMForFileOutput(const std::string input,
 }
 
 // static
-// TODO(aa): A problem with this code is that we silently allow upgrades to
-// extensions that require less permissions than the current version, but then
-// we don't silently allow them to go back. In order to fix this, we would need
-// to remember the max set of permissions we ever granted a single extension.
-bool Extension::IsPrivilegeIncrease(const Extension* old_extension,
+bool Extension::IsPrivilegeIncrease(const std::set<std::string>& granted_apis,
+                                    const ExtensionExtent& granted_extent,
+                                    const Extension* old_extension,
                                     const Extension* new_extension) {
-  // If the old extension had native code access, we don't need to go any
-  // further. Things can't get any worse.
-  if (old_extension->plugins().size() > 0)
-    return false;
+  // COMPONENT extensions should never go through this code path.
+  DCHECK(new_extension->location() != Extension::COMPONENT);

[Aaron Boodman, 2010/11/22 07:57:53]

Two things:

1) DCHECK is for wusses, and causes lots of problems. It is compiled out of
release builds, so execution continues past broken assumptions and we finally
crash somewhere else, long past the actual point of failure. When you find
yourself using DCHECK consider whether a) this really cannot happen ever unless
a programmer has made a mistake (in which case use CHECK), or b) this could
happen if some external system was misbehaving or not meeting our expectations
(in which case handle the error gracefully).

2) In this case, I think the [D]CHECK is not buying anything. This is a nice
generic function that we might want to use in other scenarios in the future. In
the current configuration we do not call it with component extensions, but there
is no reason to lock it down.

[jstritar, 2010/11/22 23:01:08]

Thanks for the explanation... I went a little crazy with the DHCECKs :). I
removed this one, and most of the others.

 
-  // Otherwise, if the new extension has a plugin, it's a privilege increase.
-  if (new_extension->plugins().size() > 0)
-    return true;
+  if (old_extension) {
+    // If the old extension had native code access, we don't need to go any
+    // further. Things can't get any worse.
+    if (old_extension->plugins().size() > 0)
+      return false;
+
+    // Otherwise, if the new extension has a plugin, it's a privilege increase.
+    if (new_extension->plugins().size() > 0)
+      return true;
+  }
 
-  // If we are increasing the set of hosts we have access to (not
-  // counting scheme differences), it's a privilege increase.
-  if (!old_extension->HasEffectiveAccessToAllHosts()) {
+  // If the extension hadn't been granted access to all hosts in the past, then
+  // see if the extension requires more host permissions.
+  if (!HasEffectiveAccessToAllHosts(granted_extent, granted_apis)) {
     if (new_extension->HasEffectiveAccessToAllHosts())
       return true;
 
-    // TODO(erikkay) This will trip when you add a new distinct hostname,
-    // but we should unique based on RCD as well.  crbug.com/57042
-    std::vector<std::string> old_hosts = old_extension->GetDistinctHosts();
-    std::vector<std::string> new_hosts = new_extension->GetDistinctHosts();
+    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_only;
+    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_only, new_only.end()));
-    if (new_only.size())
+                        old_hosts_set.begin(), old_hosts_set.end(),
+                        std::inserter(new_hosts_only, new_hosts_only.end()));
+
+    if (new_hosts_only.size())
       return true;
   }
 
-  std::set<string16> old_messages =
-      old_extension->GetSimplePermissionMessages();
-  std::set<string16> new_messages =
-      new_extension->GetSimplePermissionMessages();
-  std::set<string16> new_only;
-  std::set_difference(new_messages.begin(), new_messages.end(),
-                      old_messages.begin(), old_messages.end(),
-                      std::inserter(new_only, new_only.end()));
+  std::set<std::string> new_apis = new_extension->api_permissions();
+  std::set<std::string> new_apis_only;
+  std::set_difference(new_apis.begin(), new_apis.end(),
+                      granted_apis.begin(), granted_apis.end(),
+                      std::inserter(new_apis_only, new_apis_only.end()));
 
-  // If there are any new permission messages, then it's an increase.
-  if (!new_only.empty())
+  // Ignore API permissions that don't require user approval when deciding if
+  // an extension has increased its privileges.
+  size_t new_api_count = 0;
+  for (std::set<std::string>::iterator i = new_apis_only.begin();
+       i != new_apis_only.end(); ++i) {
+    if (GetPermissionMessageId(*i))
+      new_api_count++;
+  }
+
+  if (new_api_count)
     return true;
 
   return false;
@@ -1706,29 +1719,31 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_key,
         }
       }
 
-      // Otherwise, it's a host pattern permission.
+      // Check if it's a host pattern permission.
       URLPattern pattern = URLPattern(CanExecuteScriptEverywhere() ?
           URLPattern::SCHEME_ALL :
           (UserScript::kValidUserScriptSchemes |
               URLPattern::SCHEME_CHROMEUI) & ~URLPattern::SCHEME_FILE);
 
-      if (URLPattern::PARSE_SUCCESS != pattern.Parse(permission_str)) {
-        *error = ExtensionErrorUtils::FormatErrorMessage(
-            errors::kInvalidPermission, base::IntToString(i));
-        return false;
-      }
+      if (URLPattern::PARSE_SUCCESS == pattern.Parse(permission_str)) {
+        if (!CanSpecifyHostPermission(pattern)) {
+          *error = ExtensionErrorUtils::FormatErrorMessage(
+              errors::kInvalidPermissionScheme, base::IntToString(i));
+          return false;
+        }
 
-      if (!CanSpecifyHostPermission(pattern)) {
-        *error = ExtensionErrorUtils::FormatErrorMessage(
-            errors::kInvalidPermissionScheme, base::IntToString(i));
-        return false;
-      }
+        // The path component is not used for host permissions, so we force it
+        // to match all paths.
+        pattern.set_path("/*");
 
-      // The path component is not used for host permissions, so we force it to
-      // match all paths.
-      pattern.set_path("/*");
+        host_permissions_.push_back(pattern);
+      }
 
-      host_permissions_.push_back(pattern);
+      // If it's not a host permission, then it's probably an unknown API
+      // permission. Do not throw an error so extensions can retain
+      // backwards compatability (http://crbug.com/42742).
+      // TODO(jstritar): We could add better validation of API permissions here
+      //   if we'd like to improve error messages.
     }
   }
 
@@ -2122,32 +2137,30 @@ bool Extension::CanExecuteScriptOnPage(
   return false;
 }
 
-bool Extension::HasEffectiveAccessToAllHosts() const {
+// static
+bool Extension::HasEffectiveAccessToAllHosts(
+    const ExtensionExtent& effective_host_permissions,
+    const std::set<std::string>& api_permissions) {
   // Some APIs effectively grant access to every site.  New ones should be
   // added here.  (I'm looking at you, network API)
-  if (HasApiPermission(kProxyPermission))
+  if (HasApiPermission(api_permissions, kProxyPermission))
     return true;
 
-  for (URLPatternList::const_iterator host = host_permissions().begin();
-       host != host_permissions().end(); ++host) {
+  const URLPatternList patterns = effective_host_permissions.patterns();
+  for (URLPatternList::const_iterator host = patterns.begin();
+       host != patterns.end(); ++host) {
     if (host->match_subdomains() && host->host().empty())
       return true;
   }
 
-  for (UserScriptList::const_iterator content_script =
-       content_scripts().begin();
-       content_script != content_scripts().end(); ++content_script) {
-    UserScript::PatternList::const_iterator pattern =
-        content_script->url_patterns().begin();
-    for (; pattern != content_script->url_patterns().end(); ++pattern) {
-      if (pattern->match_subdomains() && pattern->host().empty())
-        return true;
-    }
-  }
-
   return false;
 }
 
+bool Extension::HasEffectiveAccessToAllHosts() const {
+  return HasEffectiveAccessToAllHosts(GetEffectiveHostPermissions(),
+                                      api_permissions());
+}
+
 bool Extension::IsAPIPermission(const std::string& str) const {
   for (size_t i = 0; i < Extension::kNumPermissions; ++i) {
     if (str == Extension::kPermissions[i].name) {