Make ActiveScriptController use Active Tab-style permissions

Also make ActiveScriptController used for all extensions with pseudo-all-hosts (e.g., *://*.com/*).
Add unittests for ActiveScriptController (to complement the browsertests).

BUG=362353

Review URL: https://codereview.chromium.org/293003008

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

chrome/browser/extensions/active_script_controller.cc

@@ -9,9 +9,11 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
+#include "chrome/browser/extensions/active_tab_permission_granter.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/location_bar_controller.h"
 #include "chrome/browser/extensions/tab_helper.h"
+#include "chrome/browser/sessions/session_id.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
@@ -70,8 +72,12 @@ ActiveScriptController* ActiveScriptController::GetForWebContents(
 bool ActiveScriptController::RequiresUserConsentForScriptInjection(
     const Extension* extension) {
   CHECK(extension);
-  if (!PermissionsData::RequiresActionForScriptExecution(extension))
+  if (!PermissionsData::RequiresActionForScriptExecution(
+          extension,
+          SessionID::IdForTab(web_contents()),
+          web_contents()->GetVisibleURL())) {
     return false;
+  }
 
   // If the feature is not enabled, we automatically allow all extensions to
   // run scripts.
@@ -165,6 +171,11 @@ LocationBarController::Action ActiveScriptController::OnClicked(
   iter->second.swap(requests);
   pending_requests_.erase(extension->id());
 
+  // Clicking to run the extension counts as granting it permission to run on
+  // the given tab.
+  TabHelper::FromWebContents(web_contents())->
+      active_tab_permission_granter()->GrantIfRequested(extension);
+
   // Run all pending injections for the given extension.
   for (PendingRequestList::iterator request = requests.begin();
        request != requests.end();

chrome/browser/extensions/active_tab_permission_granter.cc

@@ -13,6 +13,7 @@
 #include "extensions/common/permissions/permission_set.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "extensions/common/user_script.h"
+#include "url/gurl.h"
 
 using content::RenderProcessHost;
 using content::WebContentsObserver;
@@ -41,7 +42,19 @@ void ActiveTabPermissionGranter::GrantIfRequested(const Extension* extension) {
   APIPermissionSet new_apis;
   URLPatternSet new_hosts;
 
-  if (extension->HasAPIPermission(APIPermission::kActiveTab)) {
+  // If the extension requires action for script execution, we grant it
+  // active tab-style permissions, even if it doesn't have the activeTab
+  // permission in the manifest.
+  // We don't take tab id into account, because we want to know if the extension
+  // should require active tab in general (not for the current tab).
+  bool requires_action_for_script_execution =
+      PermissionsData::RequiresActionForScriptExecution(
+          extension,
+          -1,  // No tab id.
+          GURL::EmptyGURL());
+
+  if (extension->HasAPIPermission(APIPermission::kActiveTab) ||
+      requires_action_for_script_execution) {
     URLPattern pattern(UserScript::ValidUserScriptSchemes());
     // Pattern parsing could fail if this is an unsupported URL e.g. chrome://.
     if (pattern.Parse(web_contents()->GetURL().spec()) ==

chrome/chrome_tests_unit.gypi

@@ -837,6 +837,7 @@
         'browser/drive/fake_drive_service_unittest.cc',
         'browser/enumerate_modules_model_unittest_win.cc',
         'browser/extensions/active_tab_unittest.cc',
+        'browser/extensions/active_script_controller_unittest.cc',
         'browser/extensions/activity_log/activity_database_unittest.cc',
         'browser/extensions/activity_log/activity_log_enabled_unittest.cc',
         'browser/extensions/activity_log/activity_log_unittest.cc',

chrome/common/extensions/permissions/chrome_permission_message_provider.cc

@@ -13,7 +13,6 @@
 #include "extensions/common/url_pattern.h"
 #include "extensions/common/url_pattern_set.h"
 #include "grit/generated_resources.h"
-#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
@@ -23,47 +22,6 @@ namespace {
 
 typedef std::set<PermissionMessage> PermissionMsgSet;
 
-bool ShouldWarnAllHosts(const PermissionSet* permissions) {
-  if (permissions->HasEffectiveAccessToAllHosts())
-    return true;
-
-  const URLPatternSet& effective_hosts = permissions->effective_hosts();
-  for (URLPatternSet::const_iterator iter = effective_hosts.begin();
-       iter != effective_hosts.end();
-       ++iter) {
-    // If this doesn't even match subdomains, it can't possibly imply all hosts.
-    if (!iter->match_subdomains())
-      continue;
-
-    // If iter->host() is a recognized TLD, this will be 0. We don't include
-    // private TLDs, so that, e.g., *.appspot.com does not imply all hosts.
-    size_t registry_length =
-        net::registry_controlled_domains::GetRegistryLength(
-            iter->host(),
-            net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
-            net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
-    // If there was more than just a TLD in the host (e.g., *.foobar.com), it
-    // doesn't imply all hosts.
-    if (registry_length > 0)
-      continue;
-
-    // At this point the host could either be just a TLD ("com") or some unknown
-    // TLD-like string ("notatld"). To disambiguate between them construct a
-    // fake URL, and check the registry. This returns 0 if the TLD is
-    // unrecognized, or the length of the recognized TLD.
-    registry_length = net::registry_controlled_domains::GetRegistryLength(
-        base::StringPrintf("foo.%s", iter->host().c_str()),
-        net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
-        net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
-    // If we recognized this TLD, then this is a pattern like *.com, and it
-    // should imply all hosts.
-    if (registry_length > 0)
-      return true;
-  }
-
-  return false;
-}
-
 template<typename T>
 typename T::iterator FindMessageByID(T& messages, int id) {
   for (typename T::iterator it = messages.begin();
@@ -267,7 +225,7 @@ ChromePermissionMessageProvider::GetAPIPermissionMessages(
   // subset of what the "<all_urls>" access allows. Therefore we
   // display only the "<all_urls>" warning message if both permissions
   // are required.
-  if (ShouldWarnAllHosts(permissions)) {
+  if (permissions->ShouldWarnAllHosts()) {
     messages.erase(
         PermissionMessage(
             PermissionMessage::kDeclarativeWebRequest, base::string16()));
@@ -303,7 +261,7 @@ ChromePermissionMessageProvider::GetHostPermissionMessages(
   if (extension_type == Manifest::TYPE_PLATFORM_APP)
     return messages;
 
-  if (ShouldWarnAllHosts(permissions)) {
+  if (permissions->ShouldWarnAllHosts()) {
     messages.insert(PermissionMessage(
         PermissionMessage::kHostsAll,
         l10n_util::GetStringUTF16(IDS_EXTENSION_PROMPT_WARNING_ALL_HOSTS)));

extensions/common/permissions/permission_set.cc

@@ -8,12 +8,14 @@
 #include <iterator>
 #include <string>
 
+#include "base/strings/stringprintf.h"
 #include "extensions/common/permissions/permissions_info.h"
 #include "extensions/common/url_pattern.h"
 #include "extensions/common/url_pattern_set.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "url/gurl.h"
 
-using extensions::URLPatternSet;
+namespace extensions {
 
 namespace {
 
@@ -28,13 +30,11 @@ void AddPatternsAndRemovePaths(const URLPatternSet& set, URLPatternSet* out) {
 
 }  // namespace
 
-namespace extensions {
-
 //
 // PermissionSet
 //
 
-PermissionSet::PermissionSet() {}
+PermissionSet::PermissionSet() : should_warn_all_hosts_(UNINITIALIZED) {}
 
 PermissionSet::PermissionSet(
     const APIPermissionSet& apis,
@@ -43,7 +43,8 @@ PermissionSet::PermissionSet(
     const URLPatternSet& scriptable_hosts)
     : apis_(apis),
       manifest_permissions_(manifest_permissions),
-      scriptable_hosts_(scriptable_hosts) {
+      scriptable_hosts_(scriptable_hosts),
+      should_warn_all_hosts_(UNINITIALIZED) {
   AddPatternsAndRemovePaths(explicit_hosts, &explicit_hosts_);
   InitImplicitPermissions();
   InitEffectiveHosts();
@@ -230,6 +231,12 @@ bool PermissionSet::HasEffectiveAccessToAllHosts() const {
   return false;
 }
 
+bool PermissionSet::ShouldWarnAllHosts() const {
+  if (should_warn_all_hosts_ == UNINITIALIZED)
+    InitShouldWarnAllHosts();
+  return should_warn_all_hosts_ == WARN_ALL_HOSTS;
+}
+
 bool PermissionSet::HasEffectiveAccessToURL(const GURL& url) const {
   return effective_hosts().MatchesURL(url);
 }
@@ -262,4 +269,48 @@ void PermissionSet::InitEffectiveHosts() {
       explicit_hosts(), scriptable_hosts(), &effective_hosts_);
 }
 
+void PermissionSet::InitShouldWarnAllHosts() const {
+  if (HasEffectiveAccessToAllHosts()) {
+    should_warn_all_hosts_ = WARN_ALL_HOSTS;
+    return;
+  }
+
+  for (URLPatternSet::const_iterator iter = effective_hosts_.begin();
+       iter != effective_hosts_.end();
+       ++iter) {
+    // If this doesn't even match subdomains, it can't possibly imply all hosts.
+    if (!iter->match_subdomains())
+      continue;
+
+    // If iter->host() is a recognized TLD, this will be 0. We don't include
+    // private TLDs, so that, e.g., *.appspot.com does not imply all hosts.
+    size_t registry_length =
+        net::registry_controlled_domains::GetRegistryLength(
+            iter->host(),
+            net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
+            net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
+    // If there was more than just a TLD in the host (e.g., *.foobar.com), it
+    // doesn't imply all hosts.
+    if (registry_length > 0)
+      continue;
+
+    // At this point the host could either be just a TLD ("com") or some unknown
+    // TLD-like string ("notatld"). To disambiguate between them construct a
+    // fake URL, and check the registry. This returns 0 if the TLD is
+    // unrecognized, or the length of the recognized TLD.
+    registry_length = net::registry_controlled_domains::GetRegistryLength(
+        base::StringPrintf("foo.%s", iter->host().c_str()),
+        net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
+        net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
+    // If we recognized this TLD, then this is a pattern like *.com, and it
+    // should imply all hosts.
+    if (registry_length > 0) {
+      should_warn_all_hosts_ = WARN_ALL_HOSTS;
+      return;
+    }
+  }
+
+  should_warn_all_hosts_ = DONT_WARN_ALL_HOSTS;
+}
+
 }  // namespace extensions

extensions/common/permissions/permission_set.h

@@ -96,6 +96,11 @@ class PermissionSet
   // origins.
   bool HasEffectiveAccessToAllHosts() const;
 
+  // Returns true if this permission set has access to so many hosts, that we
+  // should treat it as all hosts for warning purposes.
+  // For example, '*://*.com/*'.
+  bool ShouldWarnAllHosts() const;
+
   // Returns true if this permission set includes effective access to |url|.
   bool HasEffectiveAccessToURL(const GURL& url) const;
 
@@ -121,14 +126,15 @@ class PermissionSet
 
   ~PermissionSet();
 
-  void AddAPIPermission(APIPermission::ID id);
-
   // Adds permissions implied independently of other context.
   void InitImplicitPermissions();
 
   // Initializes the effective host permission based on the data in this set.
   void InitEffectiveHosts();
 
+  // Initializes |has_access_to_most_hosts_|.
+  void InitShouldWarnAllHosts() const;
+
   // The api list is used when deciding if an extension can access certain
   // extension APIs and features.
   APIPermissionSet apis_;
@@ -147,6 +153,16 @@ class PermissionSet
 
   // The list of hosts this effectively grants access to.
   URLPatternSet effective_hosts_;
+
+  enum ShouldWarnAllHostsType {
+    UNINITIALIZED = 0,
+    WARN_ALL_HOSTS,
+    DONT_WARN_ALL_HOSTS
+  };
+  // Whether or not this permission set includes access to so many origins, we
+  // should treat it as all_hosts for warning purposes.
+  // Lazily initialized (and therefore mutable).
+  mutable ShouldWarnAllHostsType should_warn_all_hosts_;
 };
 
 }  // namespace extensions

extensions/common/permissions/permissions_data.cc

@@ -238,6 +238,25 @@ bool IsTrustedId(const std::string& extension_id) {
   return extension_id == std::string("nckgahadagoaajjgafhacjanaoiihapd");
 }
 
+// Returns true if the |extension| has tab-specific permission to operate on
+// the tab specified by |tab_id| with the given |url|.
+// Note that if this returns false, it doesn't mean the extension can't run on
+// the given tab, only that it does not have tab-specific permission to do so.
+bool HasTabSpecificPermissionToExecuteScript(
+    const Extension* extension,
+    int tab_id,
+    const GURL& url) {
+  if (tab_id >= 0) {
+    scoped_refptr<const PermissionSet> tab_permissions =
+        PermissionsData::GetTabSpecificPermissions(extension, tab_id);
+    if (tab_permissions.get() &&
+        tab_permissions->explicit_hosts().MatchesSecurityOrigin(url)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace
 
 struct PermissionsData::InitialPermissions {
@@ -413,6 +432,12 @@ bool PermissionsData::HasEffectiveAccessToAllHosts(const Extension* extension) {
   return GetActivePermissions(extension)->HasEffectiveAccessToAllHosts();
 }
 
+// static
+bool PermissionsData::ShouldWarnAllHosts(const Extension* extension) {
+  base::AutoLock auto_lock(extension->permissions_data()->runtime_lock_);
+  return GetActivePermissions(extension)->ShouldWarnAllHosts();
+}
+
 // static
 PermissionMessages PermissionsData::GetPermissionMessages(
     const Extension* extension) {
@@ -490,15 +515,8 @@ bool PermissionsData::CanExecuteScriptOnPage(const Extension* extension,
     return false;
   }
 
-  // If a tab ID is specified, try the tab-specific permissions.
-  if (tab_id >= 0) {
-    scoped_refptr<const PermissionSet> tab_permissions =
-        GetTabSpecificPermissions(extension, tab_id);
-    if (tab_permissions.get() &&
-        tab_permissions->explicit_hosts().MatchesSecurityOrigin(document_url)) {
-      return true;
-    }
-  }
+  if (HasTabSpecificPermissionToExecuteScript(extension, tab_id, top_frame_url))
+    return true;
 
   bool can_access = false;
 
@@ -562,14 +580,26 @@ bool PermissionsData::CanCaptureVisiblePage(const Extension* extension,
 
 // static
 bool PermissionsData::RequiresActionForScriptExecution(
-    const Extension* extension) {
+    const Extension* extension,
+    int tab_id,
+    const GURL& url) {
   // For now, the user should be notified when an extension with all hosts
-  // permission tries to execute a script on a page. Exceptions for policy-
-  // enabled and component extensions.
-  return extension->ShouldDisplayInExtensionSettings() &&
-         !Manifest::IsPolicyLocation(extension->location()) &&
-         !Manifest::IsComponentLocation(extension->location()) &&
-         HasEffectiveAccessToAllHosts(extension);
+  // permission tries to execute a script on a page, with exceptions for policy-
+  // enabled and component extensions. If this doesn't meet those criteria,
+  // return immediately.
+  if (!extension->ShouldDisplayInExtensionSettings() ||
+      Manifest::IsPolicyLocation(extension->location()) ||
+      Manifest::IsComponentLocation(extension->location()) ||
+      !ShouldWarnAllHosts(extension)) {
+    return false;
+  }
+
+  // If the extension has explicit permission to run on the given tab, then
+  // we don't need to alert the user.
+  if (HasTabSpecificPermissionToExecuteScript(extension, tab_id, url))
+    return false;
+
+  return true;
 }
 
 bool PermissionsData::ParsePermissions(Extension* extension,

extensions/common/permissions/permissions_data.h

@@ -135,6 +135,11 @@ class PermissionsData {
   // network, etc.)
   static bool HasEffectiveAccessToAllHosts(const Extension* extension);
 
+  // Whether the extension has access to so many hosts that we should treat it
+  // as "all_hosts" for warning purposes.
+  // For example, '*://*.com/*'.
+  static bool ShouldWarnAllHosts(const Extension* extension);
+
   // Returns the full list of permission messages that the given |extension|
   // should display at install time.
   static PermissionMessages GetPermissionMessages(const Extension* extension);
@@ -179,7 +184,9 @@ class PermissionsData {
 
   // Returns true if the user should be alerted that the |extension| is running
   // a script.
-  static bool RequiresActionForScriptExecution(const Extension* extension);
+  static bool RequiresActionForScriptExecution(const Extension* extension,
+                                               int tab_id,
+                                               const GURL& url);
 
   // Parse the permissions of a given extension in the initialization process.
   bool ParsePermissions(Extension* extension, base::string16* error);

extensions/common/permissions/permissions_data_unittest.cc

@@ -14,7 +14,9 @@
 #include "content/public/common/socket_permission_request.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
 #include "extensions/common/id_util.h"
+#include "extensions/common/manifest.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/permissions/api_permission.h"
 #include "extensions/common/permissions/permission_set.h"
@@ -22,7 +24,9 @@
 #include "extensions/common/permissions/socket_permission.h"
 #include "extensions/common/switches.h"
 #include "extensions/common/url_pattern_set.h"
+#include "extensions/common/value_builder.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
 
 using base::UTF16ToUTF8;
 using content::SocketPermissionRequest;
@@ -34,6 +38,8 @@ namespace extensions {
 
 namespace {
 
+const char kAllHostsPermission[] = "*://*/*";
+
 bool CheckSocketPermission(
     scoped_refptr<Extension> extension,
     SocketPermissionRequest::OperationType type,
@@ -44,6 +50,43 @@ bool CheckSocketPermission(
       extension.get(), APIPermission::kSocket, &param);
 }
 
+// Creates and returns an extension with the given |id|, |host_permissions|, and
+// manifest |location|.
+scoped_refptr<const Extension> GetExtensionWithHostPermission(
+    const std::string& id,
+    const std::string& host_permissions,
+    Manifest::Location location) {
+  ListBuilder permissions;
+  if (!host_permissions.empty())
+    permissions.Append(host_permissions);
+
+  return ExtensionBuilder()
+      .SetManifest(
+          DictionaryBuilder()
+              .Set("name", id)
+              .Set("description", "an extension")
+              .Set("manifest_version", 2)
+              .Set("version", "1.0.0")
+              .Set("permissions", permissions.Pass())
+              .Build())
+      .SetLocation(location)
+      .SetID(id)
+      .Build();
+}
+
+bool RequiresActionForScriptExecution(const std::string& extension_id,
+                                      const std::string& host_permissions,
+                                      Manifest::Location location) {
+  scoped_refptr<const Extension> extension =
+      GetExtensionWithHostPermission(extension_id,
+                                     host_permissions,
+                                     location);
+  return PermissionsData::RequiresActionForScriptExecution(
+      extension,
+      -1,  // Ignore tab id for these.
+      GURL::EmptyGURL());
+}
+
 }  // namespace
 
 TEST(ExtensionPermissionsTest, EffectiveHostPermissions) {
@@ -153,6 +196,46 @@ TEST(ExtensionPermissionsTest, SocketPermissions) {
         "239.255.255.250", 1900));
 }
 
+TEST(ExtensionPermissionsTest, RequiresActionForScriptExecution) {
+  // Extensions with all_hosts should require action.
+  EXPECT_TRUE(RequiresActionForScriptExecution(
+      "all_hosts_permissions", kAllHostsPermission, Manifest::INTERNAL));
+  // Extensions with nearly all hosts are treated the same way.
+  EXPECT_TRUE(RequiresActionForScriptExecution(
+      "pseudo_all_hosts_permissions", "*://*.com/*", Manifest::INTERNAL));
+  // Extensions with explicit permissions shouldn't require action.
+  EXPECT_FALSE(RequiresActionForScriptExecution(
+      "explicit_permissions", "https://www.google.com/*", Manifest::INTERNAL));
+  // Policy extensions are exempt...
+  EXPECT_FALSE(RequiresActionForScriptExecution(
+      "policy", kAllHostsPermission, Manifest::EXTERNAL_POLICY));
+  // ... as are component extensions.
+  EXPECT_FALSE(RequiresActionForScriptExecution(
+      "component", kAllHostsPermission, Manifest::COMPONENT));
+  // Throw in an external pref extension to make sure that it's not just working
+  // for everything non-internal.
+  EXPECT_TRUE(RequiresActionForScriptExecution(
+      "external_pref", kAllHostsPermission, Manifest::EXTERNAL_PREF));
+
+  // If we grant an extension tab permissions, then it should no longer require
+  // action.
+  scoped_refptr<const Extension> extension =
+      GetExtensionWithHostPermission("all_hosts_permissions",
+                                     kAllHostsPermission,
+                                     Manifest::INTERNAL);
+  URLPatternSet allowed_hosts;
+  allowed_hosts.AddPattern(
+      URLPattern(URLPattern::SCHEME_HTTPS, "https://www.google.com/*"));
+  scoped_refptr<PermissionSet> tab_permissions(
+      new PermissionSet(APIPermissionSet(),
+                        ManifestPermissionSet(),
+                        allowed_hosts,
+                        URLPatternSet()));
+  PermissionsData::UpdateTabSpecificPermissions(extension, 0, tab_permissions);
+  EXPECT_FALSE(PermissionsData::RequiresActionForScriptExecution(
+      extension, 0, GURL("https://www.google.com/")));
+}
+
 TEST(ExtensionPermissionsTest, GetPermissionMessages_ManyAPIPermissions) {
   scoped_refptr<Extension> extension;
   extension = LoadManifest("permissions", "many-apis.json");

extensions/renderer/user_script_slave.cc

@@ -18,6 +18,7 @@
 #include "extensions/common/extension_set.h"
 #include "extensions/common/manifest_handlers/csp_info.h"
 #include "extensions/common/permissions/permissions_data.h"
+#include "extensions/renderer/extension_helper.h"
 #include "extensions/renderer/extensions_renderer_client.h"
 #include "extensions/renderer/script_context.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
@@ -183,7 +184,10 @@ void UserScriptSlave::InjectScripts(WebFrame* frame,
     const Extension* extension = GetExtension(injection->extension_id());
     DCHECK(extension);
 
-    if (PermissionsData::RequiresActionForScriptExecution(extension)) {
+    if (PermissionsData::RequiresActionForScriptExecution(
+            extension,
+            ExtensionHelper::Get(top_render_view)->tab_id(),
+            document_url)) {
       // TODO(rdevlin.cronin): Right now, this is just a notification, but soon
       // we should block without user consent.
       top_render_view->Send(