Block tabs.executeScript() from executing until user grants permission

chrome/browser/extensions/active_script_controller.cc

Index: chrome/browser/extensions/active_script_controller.cc
diff --git a/chrome/browser/extensions/active_script_controller.cc b/chrome/browser/extensions/active_script_controller.cc
index 36209aff27729a13b0aa4b29b2aaf815cfed78b6..b86801ffaabe5265d2bf3eaf36bee498059a63ca 100644
--- a/chrome/browser/extensions/active_script_controller.cc
+++ b/chrome/browser/extensions/active_script_controller.cc
@@ -4,6 +4,9 @@
 
 #include "chrome/browser/extensions/active_script_controller.h"
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "chrome/browser/extensions/extension_action.h"
@@ -12,6 +15,7 @@
 #include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
@@ -27,6 +31,7 @@ ActiveScriptController::ActiveScriptController(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       enabled_(FeatureSwitch::scripts_require_action()->IsEnabled()) {
+  CHECK(web_contents);
 }
 
 ActiveScriptController::~ActiveScriptController() {
@@ -48,43 +53,59 @@ ActiveScriptController* ActiveScriptController::GetForWebContents(
   return location_bar_controller->active_script_controller();
 }
 
-void ActiveScriptController::NotifyScriptExecuting(
-    const std::string& extension_id, int page_id) {
+bool ActiveScriptController::RequiresUserConsentForScriptInjection(
+    const std::string& extension_id,
+    int page_id) {
   content::NavigationEntry* visible_entry =
       web_contents()->GetController().GetVisibleEntry();
-  if (!visible_entry ||
-      extensions_executing_scripts_.count(extension_id) ||
-      visible_entry->GetPageID() != page_id) {
-    return;
-  }
+  if (!visible_entry || visible_entry->GetPageID() != page_id)
+    return false;
 
   const Extension* extension =
       ExtensionRegistry::Get(web_contents()->GetBrowserContext())
           ->enabled_extensions().GetByID(extension_id);
-  if (extension &&
-      PermissionsData::RequiresActionForScriptExecution(extension)) {
-    extensions_executing_scripts_.insert(extension_id);
-    LocationBarController::NotifyChange(web_contents());
+  if (!extension)
+    return false;
+
+  if (PermissionsData::RequiresActionForScriptExecution(extension)) {

[not at google - send to devlin, 2014/05/16 00:08:33]

extreme nit: can this be if (!PermissionData::...) return false? flows better
with the rest of the function.

[Devlin, 2014/05/16 16:13:46]

Done.

+    // If the feature is not enabled, we automatically allow all extensions to
+    // run scripts.
+    if (!enabled_)
+      permitted_extensions_.insert(extension->id());
+    return permitted_extensions_.count(extension->id()) == 0;
   }
+
+  return false;
 }
 
 void ActiveScriptController::OnAdInjectionDetected(
     const std::vector<std::string> ad_injectors) {
   size_t num_preventable_ad_injectors =
       base::STLSetIntersection<std::set<std::string> >(
-          ad_injectors, extensions_executing_scripts_).size();
+          ad_injectors, permitted_extensions_).size();
+
+  // We only log the permitted extensions metric if the feature is enabled,
+  // because otherwise the data will be boring (100% allowed).
+  if (enabled_) {
+    UMA_HISTOGRAM_COUNTS_100(
+        "Extensions.ActiveScriptController.PermittedExtensions",
+        permitted_extensions_.size());
+    UMA_HISTOGRAM_COUNTS_100(
+        "Extensions.ActiveScriptController.DeniedExtensions",

[not at google - send to devlin, 2014/05/16 00:08:33]

I think I made this comment earlier... but this stuff doesn't have anything to
do with ad injection, so should go in LogUMA.

[Devlin, 2014/05/16 16:13:46]

Done.

+        pending_requests_.size());
+  }
 
   UMA_HISTOGRAM_COUNTS_100(
       "Extensions.ActiveScriptController.PreventableAdInjectors",
       num_preventable_ad_injectors);
   UMA_HISTOGRAM_COUNTS_100(
-      "Extensions.ActiveScriptController.PreventableAdInjectors",
+      "Extensions.ActiveScriptController.UnpreventableAdInjectors",
       ad_injectors.size() - num_preventable_ad_injectors);

[not at google - send to devlin, 2014/05/16 00:08:33]

and this stuff now needs to take into account the IDs in pending_requests_.

[Devlin, 2014/05/16 16:13:46]

It shouldn't - in fact, that would make it less accurate, I think.

We notify this with all the ad injectors, and these metrics determine which of
them we COULD block (but didn't) and which we COULD NOT block (and obviously
didn't).  If something has a pending request, then it should NOT have injected
any ads.  The only extensions with pending requests are those that we haven't
permitted to run.  If it injected ads even though it has a pending request, then
it managed to even though it wasn't permitted, which makes it unpreventable.

Right?

(Sorry for the caps.)

 }
 
 ExtensionAction* ActiveScriptController::GetActionForExtension(
     const Extension* extension) {
-  if (!enabled_ || extensions_executing_scripts_.count(extension->id()) == 0)
+  if (!enabled_ || pending_requests_.count(extension->id()) == 0)
     return NULL;  // No action for this extension.
 
   ActiveScriptMap::iterator existing =
@@ -112,13 +133,60 @@ ExtensionAction* ActiveScriptController::GetActionForExtension(
 
 LocationBarController::Action ActiveScriptController::OnClicked(
     const Extension* extension) {
-  DCHECK(extensions_executing_scripts_.count(extension->id()) > 0);
+  DCHECK(extension);
+  PendingRequestMap::iterator iter =
+      pending_requests_.find(extension->id());
+  DCHECK(iter != pending_requests_.end());
+
+  // We add this to the list of permitted extensions and erase pending entries
+  // *before* running them to guard against the crazy case where running the
+  // callbacks adds more entries.
+  permitted_extensions_.insert(extension->id());
+  PendingRequestList requests;
+  iter->second.swap(requests);
+  pending_requests_.erase(extension->id());
+
+  // Run all pending injections for the given extension.
+  for (PendingRequestList::iterator request = requests.begin();
+       request != requests.end();
+       ++request) {
+    request->Run();
+  }
+
+  // Inform the location bar that the action is now gone.
+  LocationBarController::NotifyChange(web_contents());
+
   return LocationBarController::ACTION_NONE;
 }
 
 void ActiveScriptController::OnNavigated() {
   LogUMA();
-  extensions_executing_scripts_.clear();
+  permitted_extensions_.clear();
+  pending_requests_.clear();
+}
+
+void ActiveScriptController::OnNotifyExtensionScriptExecution(
+    const std::string& extension_id,
+    int page_id) {
+  if (!Extension::IdIsValid(extension_id)) {
+    NOTREACHED() << "'" << extension_id << "' is not a valid id.";
+    return;
+  }
+

[not at google - send to devlin, 2014/05/16 00:08:33]

maybe note here for future hapless readers that the reason this effectively does
nothing (just notifies) is that an upcoming CL will block :)

[Devlin, 2014/05/16 16:13:46]

Done.

+  if (RequiresUserConsentForScriptInjection(extension_id, page_id))
+    RequestScriptInjection(extension_id, base::Bind(&base::DoNothing));
+}
+
+void ActiveScriptController::RequestScriptInjection(
+    const std::string& extension_id,
+    const base::Closure& request) {
+  PendingRequestList& list = pending_requests_[extension_id];
+  list.push_back(request);
+
+  // If this was the first entry, notify the location bar that there's a new
+  // icon.
+  if (list.size() == 1u)
+    LocationBarController::NotifyChange(web_contents());
 }
 
 bool ActiveScriptController::OnMessageReceived(const IPC::Message& message) {
@@ -131,20 +199,10 @@ bool ActiveScriptController::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
-void ActiveScriptController::OnNotifyExtensionScriptExecution(
-    const std::string& extension_id,
-    int page_id) {
-  if (!Extension::IdIsValid(extension_id)) {
-    NOTREACHED() << "'" << extension_id << "' is not a valid id.";
-    return;
-  }
-  NotifyScriptExecuting(extension_id, page_id);
-}
-
 void ActiveScriptController::LogUMA() const {
   UMA_HISTOGRAM_COUNTS_100(
       "Extensions.ActiveScriptController.ShownActiveScriptsOnPage",
-      extensions_executing_scripts_.size());
+      pending_requests_.size());
 }
 
 }  // namespace extensions