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

chrome/browser/extensions/active_script_controller_browsertest.cc

Index: chrome/browser/extensions/active_script_controller_browsertest.cc
diff --git a/chrome/browser/extensions/active_script_controller_browsertest.cc b/chrome/browser/extensions/active_script_controller_browsertest.cc
index 5be5b714ebf3ed821a0e460ea20e349849c76832..cd607ceb1b661a574202c052cd9e88f45b42cc86 100644
--- a/chrome/browser/extensions/active_script_controller_browsertest.cc
+++ b/chrome/browser/extensions/active_script_controller_browsertest.cc
@@ -18,6 +18,18 @@
 
 namespace extensions {
 
+namespace {
+
+const char kInjectAttempted[] = "inject attempted";
+const char kInjectSucceeded[] = "inject succeeded";
+
+enum InjectionType {
+  CONTENT_SCRIPT,
+  EXECUTE_SCRIPT
+};
+
+}  // namespace
+
 class ActiveScriptControllerBrowserTest : public ExtensionBrowserTest {
  public:
   ActiveScriptControllerBrowserTest()
@@ -31,7 +43,8 @@ class ActiveScriptTester {
  public:
   ActiveScriptTester(const std::string& name,
                      const Extension* extension,
-                     bool expect_has_action);
+                     bool requires_consent,
+                     InjectionType type);
   ~ActiveScriptTester();
 
   testing::AssertionResult Verify(Browser* browser);
@@ -43,25 +56,39 @@ class ActiveScriptTester {
   // The extension associated with this tester.
   const Extension* extension_;
 
-  // Whether or not we expect the extension to have an action for script
-  // injection.
-  bool expect_has_action_;
+  // Whether or not the extension has permission to run the script without
+  // asking the user.
+  bool requires_consent_;
+
+  // The type of injection this tester uses.
+  InjectionType type_;
 
   // All of these extensions should inject a script (either through content
   // scripts or through chrome.tabs.executeScript()) that sends a message with
-  // their names (for verification).
-  // Be prepared to wait for each extension to inject the script.
-  linked_ptr<ExtensionTestMessageListener> listener_;
+  // the |kInjectSucceeded| message.
+  linked_ptr<ExtensionTestMessageListener> inject_attempt_listener_;
+
+  // After trying to inject the script, extensions sending the script via
+  // chrome.tabs.executeScript() send a |kInjectAttempted| message.
+  linked_ptr<ExtensionTestMessageListener> inject_success_listener_;
 };
 
 ActiveScriptTester::ActiveScriptTester(const std::string& name,
                                        const Extension* extension,
-                                       bool expect_has_action)
+                                       bool requires_consent,
+                                       InjectionType type)
     : name_(name),
       extension_(extension),
-      expect_has_action_(expect_has_action),
-      listener_(
-          new ExtensionTestMessageListener(name, false /* won't reply */)) {
+      requires_consent_(requires_consent),
+      type_(type),
+      inject_attempt_listener_(
+          new ExtensionTestMessageListener(kInjectAttempted,
+                                           false /* won't reply */)),
+      inject_success_listener_(
+          new ExtensionTestMessageListener(kInjectSucceeded,
+                                           false /* won't reply */)) {
+  inject_attempt_listener_->set_extension_id(extension->id());
+  inject_success_listener_->set_extension_id(extension->id());
 }
 
 ActiveScriptTester::~ActiveScriptTester() {
@@ -71,7 +98,17 @@ testing::AssertionResult ActiveScriptTester::Verify(Browser* browser) {
   if (!extension_)
     return testing::AssertionFailure() << "Could not load extension: " << name_;
 
-  listener_->WaitUntilSatisfied();
+  // If it's not a content script, the Extension lets us know when it has
+  // attempted to inject the script.
+  // This means there is a potential for a race condition with content scripts;
+  // however, since they are all injected at document_start, this shouldn't be

[not at google - send to devlin, 2014/05/15 00:12:36]

document_start will be an issue actually. if you recall, strictly speaking we
*can't* actually delay anything other than document_idle. this is a complexity
we'll need to deal with eventually.

you may need to notify the browser of each script execution stage... start, end,
and idle... and take into account each case separately.

[Devlin, 2014/05/15 17:45:59]

I think this one deserves a bit more thought.

So, we still have to handle start and end somehow.  We can't just let them run
(because then all we do is encourage people not to use idle), and we can't just
block them and not show the user.  So, we have to presumably do _something_ for
them.  IIRC, our main two options were:
- Tell the user to refresh the page (or do it for them) and run the script at
the right time.
- Inject it delayed anyway, and trust the user will understand (e.g., if I have
an ad blocker, I'd expect to see all the ads disappear once I allow the scripts
to run, and wouldn't have a problem with the visual jarring).

In either of these cases, this test case holds true.  The script should *not*
run (if it is not permitted to do so).  After some action is taken, the script
*should* run.  The action may change (e.g., instead of a click, it may be a
click and refresh), but that is a minor change.

The biggest thing is that I'm very hesitant to drastically increase the number
of messages sent from the renderer (for this, we'd have to notify at every
document point, and even if no scripts run), all for the sake of a single
browser test.

WDYT?

[not at google - send to devlin, 2014/05/15 18:26:55]

I agree, it requires thought, and it's complex enough to implement in a
follow-up. Here are my thoughts:

The default injection stage is idle, and documentation on how to do otherwise is
hard to find :) my point being that I expect extensions will only use
document_start/end because they really do have to. I can't say for all
extensions why they want that, but it doesn't take much imagination to think up
cases where just running them delayed won't work. E.g. injecting a style tag
which must come first for specificity, or a script tag which must override the
Date prototype; and pretending that it worked is a lie.

I expect that the simplest behaviour will be to just ignore this fact and indeed
do delay everything. Simplest, because it's most appropriate for dogfooding, and
I want to get as far as possible here without adding any UI.

I don't think these messages will be just for testing when we do fix this.

+  // a problem. If these tests start failing, though, that might be it.
+  if (type_ == EXECUTE_SCRIPT)
+    inject_attempt_listener_->WaitUntilSatisfied();
+
+  // Make sure all running tasks are complete.

[not at google - send to devlin, 2014/05/15 00:12:36]

what tasks? the active script controller ones?

[Devlin, 2014/05/15 17:45:59]

Yeah.  Just making sure that if anything is running, it finishes.

+  content::RunAllPendingInMessageLoop();
+
   content::WebContents* web_contents =
       browser ? browser->tab_strip_model()->GetActiveWebContents() : NULL;
 
@@ -82,16 +119,59 @@ testing::AssertionResult ActiveScriptTester::Verify(Browser* browser) {
   if (!tab_helper)
     return testing::AssertionFailure() << "Could not find tab helper.";
 
+  LocationBarController* location_bar_controller =
+      tab_helper->location_bar_controller();
+  if (!location_bar_controller) {
+    return testing::AssertionFailure()
+        << "Could not find location bar controller";
+  }
+
   ActiveScriptController* controller =
       tab_helper->location_bar_controller()->active_script_controller();

[not at google - send to devlin, 2014/05/15 00:12:36]

s/tab_helper->location_bar_controller()/location_bar_controller

[Devlin, 2014/05/15 17:45:59]

Whoops, done.

   if (!controller)
     return testing::AssertionFailure() << "Could not find controller.";
 
   bool has_action = controller->GetActionForExtension(extension_) != NULL;
-  if (expect_has_action_ != has_action) {
+
+  // An extension should have an action displayed iff it requires user consent.
+  if (requires_consent_ != has_action) {
+    return testing::AssertionFailure()
+        << "Improper action status for " << name_ << ": expected "
+        << requires_consent_ << ", found " << has_action;
+  }
+
+  // If the extension has permission, we should be able to simply wait for it
+  // to execute.
+  if (!requires_consent_) {
+    inject_success_listener_->WaitUntilSatisfied();
+    return testing::AssertionSuccess();
+  }
+
+  // Otherwise, we don't have permission, and have to grant it. Ensure the
+  // script has *not* already executed.
+  // Currently, it's okay for content scripts to execute, because we don't
+  // block them.
+  // TODO(rdevlin.cronin): Fix this.
+  if (inject_success_listener_->was_satisfied() && type_ != CONTENT_SCRIPT) {
+    return testing::AssertionFailure() <<
+        name_ << "'s script ran without permission.";
+  }
+
+  ExtensionAction* action = controller->GetActionForExtension(extension_);
+  // We've already verified this exists, so just a sanity check.
+  DCHECK(action);

[not at google - send to devlin, 2014/05/15 00:12:36]

in theory the action could have disappeared, like if between now and waiting for
the inject listener there was some bug which caused it to disappear.

point being, make it a test failure not a DCHECK.

[Devlin, 2014/05/15 17:45:59]

Actually... it can't.

After line 134 (when we check if we have an action), we only wait at line 146,
and then return right after.  If we didn't get an action at line 134, we will
return at line 138 (if we were supposed to have an action) or line 147 (if we
weren't).  Otherwise, the flow is synchronous.  Went ahead and made it a bit
more clear by only fetching the action once.

+
+  // Grant permission by clicking on the extension action.
+  location_bar_controller->OnClicked(action);
+
+  // Now, the extension should be able to inject the script.
+  inject_success_listener_->WaitUntilSatisfied();
+
+  // The Action should have disappeared.
+  has_action = controller->GetActionForExtension(extension_) != NULL;

[not at google - send to devlin, 2014/05/15 00:12:36]

you do this a couple of times. how about a (private) HasAction() function? you
have all the state (and you could pass in the Browser on construction so that
the state is there right from creation)

in general this method could do with functions like "location_bar_controller()".

[Devlin, 2014/05/15 17:45:59]

Done (though the end result is much longer).

+  if (has_action) {
     return testing::AssertionFailure()
-        << "Improper action status: expected " << expect_has_action_
-        << ", found " << has_action;
+        << "Extension " << name_ << " has lingering action.";
   }
 
   return testing::AssertionSuccess();
@@ -103,28 +183,40 @@ IN_PROC_BROWSER_TEST_F(ActiveScriptControllerBrowserTest,
       test_data_dir_.AppendASCII("active_script");
 
   const char* kExtensionNames[] = {
-      "content_scripts_all_hosts",
       "inject_scripts_all_hosts",
+      "inject_scripts_explicit_hosts",
+      "content_scripts_all_hosts",
       "content_scripts_explicit_hosts"
   };
 
   // First, we load up three extensions:
-  // - An extension with a content script that runs on all hosts,
   // - An extension that injects scripts into all hosts,
+  // - An extension that injects scripts into explicit hosts,
+  // - An extension with a content script that runs on all hosts,
   // - An extension with a content script that runs on explicit hosts.
+  // The extensions that operate on explicit hosts have permission; the ones
+  // that request all hosts require user consent.
   ActiveScriptTester testers[] = {
       ActiveScriptTester(
           kExtensionNames[0],
           LoadExtension(active_script_path.AppendASCII(kExtensionNames[0])),
-          true /* expect action */),
+          true /* needs consent */,

[not at google - send to devlin, 2014/05/15 00:12:36]

how about making this an enum so you don't need to write all of these comments

[Devlin, 2014/05/15 17:45:59]

The reason was because if statements like
  if (requires_consent_ != has_action) fail;
are much nicer than
  if ((requires_consent_ == REQUIRES_CONSENT && !has_action) ||
      (requires_consent_ == DOES_NOT_REQUIRE_CONSENT && has_action)) fail;

But I don't feel strongly enough about it, so, done. :)

+          EXECUTE_SCRIPT),
       ActiveScriptTester(
           kExtensionNames[1],
           LoadExtension(active_script_path.AppendASCII(kExtensionNames[1])),
-          true /* expect action */),
+          false /* does not need consent */,
+          EXECUTE_SCRIPT),
       ActiveScriptTester(
           kExtensionNames[2],
           LoadExtension(active_script_path.AppendASCII(kExtensionNames[2])),
-          false /* expect action */)
+          true /* needs consent */,
+          CONTENT_SCRIPT),
+      ActiveScriptTester(
+          kExtensionNames[3],
+          LoadExtension(active_script_path.AppendASCII(kExtensionNames[3])),
+          false /* does not need consent */,
+          CONTENT_SCRIPT),
   };
 
   // Navigate to an URL (which matches the explicit host specified in the
@@ -135,7 +227,7 @@ IN_PROC_BROWSER_TEST_F(ActiveScriptControllerBrowserTest,
       browser(), embedded_test_server()->GetURL("/extensions/test_file.html"));
 
   for (size_t i = 0u; i < arraysize(testers); ++i)
-    ASSERT_TRUE(testers[i].Verify(browser())) << kExtensionNames[i];
+    EXPECT_TRUE(testers[i].Verify(browser())) << kExtensionNames[i];
 }
 
 }  // namespace extensions