Resubmit: Block content scripts from executing until user grants permission

chrome/browser/extensions/active_script_controller_browsertest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/extensions/active_script_controller.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_test_message_listener.h"
 #include "chrome/browser/extensions/location_bar_controller.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/extensions/test_extension_dir.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/test/browser_test_utils.h"
 #include "extensions/common/feature_switch.h"
+#include "extensions/common/switches.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
 
 namespace {
 
 const char kAllHostsScheme[] = "*://*/*";
 const char kExplicitHostsScheme[] = "http://127.0.0.1/*";
 const char kBackgroundScript[] =
@@ skipping 25 matching lines @@
 
 enum RequiresConsent {
   REQUIRES_CONSENT,
   DOES_NOT_REQUIRE_CONSENT
 };
 
 }  // namespace
 
 class ActiveScriptControllerBrowserTest : public ExtensionBrowserTest {
  public:
-  ActiveScriptControllerBrowserTest()
-      : feature_override_(FeatureSwitch::scripts_require_action(),
-                          FeatureSwitch::OVERRIDE_ENABLED) {}
+  ActiveScriptControllerBrowserTest() {}
 
+  virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE;
   virtual void CleanUpOnMainThread() OVERRIDE;
 
   // Returns an extension with the given |host_type| and |injection_type|. If
   // one already exists, the existing extension will be returned. Othewrwise,
   // one will be created.
   // This could potentially return NULL if LoadExtension() fails.
   const Extension* GetOrCreateExtension(HostType host_type,
                                         InjectionType injection_type);
 
  private:
-  FeatureSwitch::ScopedOverride feature_override_;
   ScopedVector<TestExtensionDir> test_extension_dirs_;
   std::vector<const Extension*> extensions_;
 };
 
+void ActiveScriptControllerBrowserTest::SetUpCommandLine(
+    base::CommandLine* command_line) {
+  ExtensionBrowserTest::SetUpCommandLine(command_line);
+  // We append the actual switch to the commandline because it needs to be
+  // passed over to the renderer, which a FeatureSwitch::ScopedOverride will
+  // not do.
+  command_line->AppendSwitch(switches::kEnableScriptsRequireAction);
+}
+
 void ActiveScriptControllerBrowserTest::CleanUpOnMainThread() {
   test_extension_dirs_.clear();
 }
 
 const Extension* ActiveScriptControllerBrowserTest::GetOrCreateExtension(
     HostType host_type, InjectionType injection_type) {
   std::string name =
       base::StringPrintf(
           "%s %s",
           injection_type == CONTENT_SCRIPT ?
@@ skipping 166 matching lines @@
 
   // If the extension has permission, we should be able to simply wait for it
   // to execute.
   if (requires_consent_ == DOES_NOT_REQUIRE_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) {
+  if (inject_success_listener_->was_satisfied()) {
     return testing::AssertionFailure() <<
         name_ << "'s script ran without permission.";
   }
 
   // If we reach this point, we should always have an action.
   DCHECK(action);
 
   // Grant permission by clicking on the extension action.
   location_bar_controller->OnClicked(action);
 
@@ skipping 26 matching lines @@
   return location_bar_controller ?
       location_bar_controller->active_script_controller() : NULL;
 }
 
 ExtensionAction* ActiveScriptTester::GetAction() {
   ActiveScriptController* controller = GetActiveScriptController();
   return controller ? controller->GetActionForExtension(extension_) : NULL;
 }
 
 IN_PROC_BROWSER_TEST_F(ActiveScriptControllerBrowserTest,
-                       ActiveScriptsAreDisplayed) {
+                       ActiveScriptsAreDisplayedAndDelayExecution) {
   base::FilePath active_script_path =
       test_data_dir_.AppendASCII("active_script");
 
   const char* kExtensionNames[] = {
       "inject_scripts_all_hosts",
       "inject_scripts_explicit_hosts",
       "content_scripts_all_hosts",
       "content_scripts_explicit_hosts"
   };
 
@@ skipping 25 matching lines @@
           CONTENT_SCRIPT),
       ActiveScriptTester(
           kExtensionNames[3],
           GetOrCreateExtension(EXPLICIT_HOSTS, CONTENT_SCRIPT),
           browser(),
           DOES_NOT_REQUIRE_CONSENT,
           CONTENT_SCRIPT),
   };
 
   // Navigate to an URL (which matches the explicit host specified in the
-  // extension content_scripts_explicit_hosts). All three extensions should
+  // extension content_scripts_explicit_hosts). All four extensions should
   // inject the script.
   ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL("/extensions/test_file.html"));
 
   for (size_t i = 0u; i < arraysize(testers); ++i)
     EXPECT_TRUE(testers[i].Verify()) << kExtensionNames[i];
 }
 
+// Test that removing an extension with pending injections a) removes the
+// pending injections for that extension, and b) does not affect pending
+// injections for other extensions.
+IN_PROC_BROWSER_TEST_F(ActiveScriptControllerBrowserTest,
+                       RemoveExtensionWithPendingInjections) {
+  const Extension* extension1 = GetOrCreateExtension(ALL_HOSTS, CONTENT_SCRIPT);
+  ASSERT_TRUE(extension1);
+  const Extension* extension2 = GetOrCreateExtension(ALL_HOSTS, EXECUTE_SCRIPT);
+  ASSERT_TRUE(extension2);
+
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(web_contents);
+  ActiveScriptController* active_script_controller =
+      ActiveScriptController::GetForWebContents(web_contents);
+  ASSERT_TRUE(active_script_controller);
+
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("/extensions/test_file.html"));
+
+  // Both extensions should have pending requests.
+  EXPECT_TRUE(active_script_controller->GetActionForExtension(extension1));
+  EXPECT_TRUE(active_script_controller->GetActionForExtension(extension2));
+
+  // Unload one of the extensions.
+  UnloadExtension(extension2->id());
+
+  // This is slight hack to achieve a RunPendingInRenderer() method. Since IPCs
+  // are sent synchronously, the renderer will be notified of the extension
+  // being unloaded before the script is executed, and, since ExecuteScript() is
+  // synchronous, the renderer is guaranteed to be done updating scripts.
+  EXPECT_TRUE(content::ExecuteScript(web_contents, "1 == 1;"));
+
+  // We should have pending requests for extension1, but not the removed
+  // extension2.
+  EXPECT_TRUE(active_script_controller->GetActionForExtension(extension1));
+  EXPECT_FALSE(active_script_controller->GetActionForExtension(extension2));
+
+  // We should still be able to run the request for extension1.
+  ExtensionTestMessageListener inject_success_listener(
+      new ExtensionTestMessageListener(kInjectSucceeded,
+                                       false /* won't reply */));
+  active_script_controller->OnClicked(extension1);
+  inject_success_listener.WaitUntilSatisfied();

[not at google - send to devlin, 2014/06/02 21:28:17]

it doesn't seem like you're necessarily testing renderer logic here. the
GetActionForExtension(..) is browser-side logic, and all you're doing here is
clicking on the icon? how about, like with the other tests (Verify() etc),
assert that the script did get injected, the other one didn't, and that no icon
is showing anymore?

[Devlin, 2014/06/02 22:19:16]

We are testing that the script gets injected by waiting until the listener is
satisfied.  If it's not injected, this will fail.  We can also check that the
icon disappears, but that seems like it's simply testing what was tested above,
a pattern which we frequently discourage because it leads to obscenely long
tests which give no added benefit.  But maybe I'm missing something - is there a
reason the execution of the action would be significantly different?

[not at google - send to devlin, 2014/06/02 22:20:19]

good point. renderer is tested.

+}
+
+// A version of the test with the flag off, in order to test that everything
+// still works as expected.
+class FlagOffActiveScriptControllerBrowserTest
+    : public ActiveScriptControllerBrowserTest {
+ private:
+  // Simply don't append the flag.
+  virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE {
+    ExtensionBrowserTest::SetUpCommandLine(command_line);
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(FlagOffActiveScriptControllerBrowserTest,
+                       ScriptsExecuteWhenFlagAbsent) {
+  const char* kExtensionNames[] = {
+    "content_scripts_all_hosts",
+    "inject_scripts_all_hosts",
+  };
+  ActiveScriptTester testers[] = {
+    ActiveScriptTester(
+          kExtensionNames[0],
+          GetOrCreateExtension(ALL_HOSTS, CONTENT_SCRIPT),
+          browser(),
+          DOES_NOT_REQUIRE_CONSENT,
+          CONTENT_SCRIPT),
+      ActiveScriptTester(
+          kExtensionNames[1],
+          GetOrCreateExtension(ALL_HOSTS, EXECUTE_SCRIPT),
+          browser(),
+          DOES_NOT_REQUIRE_CONSENT,
+          EXECUTE_SCRIPT),
+  };
+
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("/extensions/test_file.html"));
+
+  for (size_t i = 0u; i < arraysize(testers); ++i)
+    EXPECT_TRUE(testers[i].Verify()) << kExtensionNames[i];
+}
+
 }  // namespace extensions