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..bc6829bc3cdd4c3af627dfc4693bb820f40c6181 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,31 +53,40 @@ ActiveScriptController* ActiveScriptController::GetForWebContents(
   return location_bar_controller->active_script_controller();
 }
 
-void ActiveScriptController::NotifyScriptExecuting(
-    const std::string& extension_id, int page_id) {
+void ActiveScriptController::InjectScriptIfHasPermission(
+    const std::string& extension_id,
+    int page_id,
+    const base::Closure& callback) {
   content::NavigationEntry* visible_entry =
       web_contents()->GetController().GetVisibleEntry();
-  if (!visible_entry ||
-      extensions_executing_scripts_.count(extension_id) ||
-      visible_entry->GetPageID() != page_id) {
+  if (!visible_entry || visible_entry->GetPageID() != page_id)
     return;
-  }
 
   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;
+
+  AddOrProcessRequest(extension, callback);
 }
 
 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();

[not at google - send to devlin, 2014/05/15 22:21:44]

if the feature isn't enabled then *every* extension is going to end up in
|permitted_extensions|, even those which weren't even going to be shown (see
below), so this is basically going to be the number of ad injectors.

I think the bug is basically that you shouldn't automatically be populating
permitted_extensions if it isn't enabled.

I also think I'm getting confused because permitted extensions vs pending
requests are getting a little bit mixed up if the feature is/isn't allowed. I
can see why it's that way, but the comment around here should be:

"if we're disabled, we permitted every script injection automatically. in this
case we interpret the UMA slightly differently... yada yada"

in fact I think it's wrong to be using the same UMA keys if it's enabled vs
disabled, it's just going to get muddled.

is there a sensible way to test this? I've never seen UMA tested which is funny
because there are often bugs.

[Devlin, 2014/05/15 23:52:35]

Yep, definitely a bug there with the permitted.  It should be better now,
because only count them as permitted if we would have shown a badge.

I think that the preventable/non-preventable ad injections should be good
whether the feature is enabled or disabled.  The script will only inject if it's
permitted (whether it's done explicitly or implicitly), so we can tell which
ones we let through.  We could also add in a "PreventedAdInjectors", which is
the number that we successfully blocked.

+
+  // We only log the permitted extensions metric if the feature is enabled,
+  // because otherwise the data will be boring (100% allowed).

[not at google - send to devlin, 2014/05/15 22:21:44]

I would have expected 0%

[Devlin, 2014/05/15 23:52:35]

Well, we implicitly allow script injection when it's disabled. So 100% allowed,
and 0% denied.

Is there a metric scheme you'd prefer here?  These were mostly just the first
ones that came to mind.

+  if (enabled_) {
+    UMA_HISTOGRAM_COUNTS_100(
+        "Extensions.ActiveScriptController.PermittedExtensions",
+        permitted_extensions_.size());
+    UMA_HISTOGRAM_COUNTS_100(
+        "Extensions.ActiveScriptController.DeniedExtensions",
+        pending_requests_.size());
+  }
 
   UMA_HISTOGRAM_COUNTS_100(
       "Extensions.ActiveScriptController.PreventableAdInjectors",
@@ -84,7 +98,7 @@ void ActiveScriptController::OnAdInjectionDetected(
 
 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 +126,72 @@ 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());
+
+  // Run all pending injections for the given extension.
+  PendingRequestList& list = iter->second;
+  for (PendingRequestList::iterator request = list.begin();
+       request != list.end();
+       ++request) {
+    request->Run();
+  }
+
+  // Remove the extension's pending request entry, and indicate that it is
+  // allowed to run on the current page.
+  pending_requests_.erase(extension->id());
+  permitted_extensions_.insert(extension->id());

[not at google - send to devlin, 2014/05/15 22:21:44]

this code (lines 134...145) has the potential, in crazy circumstances, of subtle
bugs creeping in if callers re-entrantly call into this class again (namely
InjectScriptIfHasPermission; or either of the 2 variations you will have).

in other words, move this state-update to before you run the requests.

to do so efficiently, perhaps:

permitted_extensions_->insert(extension->id());

PendingExtensionList requests;
iter->second.swap(&requests);
pending_request_.erase(extension->id());

etc.

[Devlin, 2014/05/15 23:52:35]

That would indeed be _crazy_ circumstances.  But we've seen crazy stuff, so good
point. ;)

+

[not at google - send to devlin, 2014/05/15 22:21:44]

also, TODO grant active-tab-esque permission.

[Devlin, 2014/05/15 23:52:35]

Done.

+  // Inform the location bar that the action is now gone.

[not at google - send to devlin, 2014/05/15 22:21:44]

something to consider in future (just occurred to me): when we do start
recording if the user wants to record their consent for a domain, we're going to
run into issues with the code that de-dups hosts.

[Devlin, 2014/05/15 23:52:35]

Ooh, that sounds like fun...

+  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;
+  }
+
+  InjectScriptIfHasPermission(
+      extension_id,
+      page_id,
+      base::Closure(base::Bind(&base::DoNothing)));
+}
+
+void ActiveScriptController::AddOrProcessRequest(
+    const Extension* extension,
+    const base::Closure& request) {
+  // If the feature is not enabled, we automatically allow all extensions to
+  // run scripts.

[not at google - send to devlin, 2014/05/15 22:21:44]

how did this get above the RequiresActionForScriptExecution() check? now it
means that basically everything ends up in |permitted_extensions_| and the UMA
not very useful (see comment up there; it might be a different mixup happening
here).

[Devlin, 2014/05/15 23:52:35]

Whoops, good point.  Let's fix that... Done.

+  if (!enabled_)
+    permitted_extensions_.insert(extension->id());
+
+  // If the extension does not require permissions, run it immediately.

[not at google - send to devlin, 2014/05/15 22:21:44]

TODO take into account tab-specific permissions

[Devlin, 2014/05/15 23:52:35]

Done. (added comment in .h above permitted_extensions_.)

+  if (!PermissionsData::RequiresActionForScriptExecution(extension) ||
+      permitted_extensions_.count(extension->id())) {
+    request.Run();
+    return;
+  }
+
+  PendingRequestList* list = &pending_requests_[extension->id()];

[not at google - send to devlin, 2014/05/15 22:21:44]

nit: why not just PendingRequestList& list?

[Devlin, 2014/05/15 23:52:35]

Only 'cuz I never see non-const & in chrome.  But, since the only style rule
against non-const & is for passing, let's start a trend. ;)

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

indeed.

+  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 +204,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