[Extensions] Make activeTab sticky while on same origin

An extension is granted activeTab permission when the user invokes it on
a certain site. Currently, this grants host permission to the extension
until the user performs a (not-same-document) navigation or closes the
tab.

With RuntimeHostPermissions, we apply a similar model to extensions with
withheld permissions, but make the permission grant "sticky" while the
user is on the same origin - that is, the permission is only revoked on
cross-origin navigation or tab close.

Apply this behavior to activeTab as well. There's not really an
increased security risk here, since the extension already has access to
the site. This also simplifies the code by reducing the differences
between activeTab and RuntimeHostPermissions.

Update tests for activeTab to reflect the new behavior, and update
public documentation to include the change.

Bug: 898617
Change-Id: I6772cf7d88ed53ceb4540e06adf33d5f17bbb4b0
Reviewed-on: https://chromium-review.googlesource.com/c/1298420Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604805}

chrome/browser/extensions/active_tab_apitest.cc

@@ -63,11 +63,6 @@ class ExtensionActiveTabTest : public ExtensionApiTest,
     ASSERT_EQ(ShouldEnableRuntimeHostPermissions(),
               base::FeatureList::IsEnabled(
                   extensions_features::kRuntimeHostPermissions));
-
-    const char* runtime_host_permissions_arg =
-        ShouldEnableRuntimeHostPermissions() ? "RuntimeHostPermissionsEnabled"
-                                             : "RuntimeHostPermissionsDisabled";
-    SetCustomArg(runtime_host_permissions_arg);
   }
 
  private:

chrome/browser/extensions/active_tab_permission_granter.cc

@@ -8,7 +8,6 @@
 #include <utility>
 #include <vector>
 
-#include "base/feature_list.h"
 #include "base/no_destructor.h"
 #include "chrome/browser/extensions/extension_action_runner.h"
 #include "chrome/browser/extensions/extension_util.h"
@@ -20,7 +19,6 @@
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/process_manager.h"
-#include "extensions/common/extension_features.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/permissions/permission_set.h"
 #include "extensions/common/permissions/permissions_data.h"
@@ -198,25 +196,16 @@ void ActiveTabPermissionGranter::DidFinishNavigation(
   }
 
   // Only clear the granted permissions for cross-origin navigations.
-  //
-  // See http://crbug.com/404243 for why. Currently we only differentiate
-  // between same-origin and cross-origin navigations when the
-  // script-require-action flag is on. It's not clear it's good for general
-  // activeTab consumption (we likely need to build some UI around it first).
-  // However, features::kRuntimeHostPermissions is all-but unusable without
-  // this behaviour.
-  if (base::FeatureList::IsEnabled(
-          extensions_features::kRuntimeHostPermissions)) {
-    const content::NavigationEntry* navigation_entry =
-        web_contents()->GetController().GetVisibleEntry();
-    if (!navigation_entry ||
-        (navigation_entry->GetURL().GetOrigin() !=
-            navigation_handle->GetPreviousURL().GetOrigin())) {
-      ClearActiveExtensionsAndNotify();
-    }
-  } else {
-    ClearActiveExtensionsAndNotify();
+  // TODO(devlin): We likely shouldn't be using the visible entry. Instead,
+  // we should use WebContents::GetLastCommittedURL().
+  const content::NavigationEntry* navigation_entry =
+      web_contents()->GetController().GetVisibleEntry();
+  if (navigation_entry && navigation_entry->GetURL().GetOrigin() ==
+                              navigation_handle->GetPreviousURL().GetOrigin()) {
+    return;
   }
+
+  ClearActiveExtensionsAndNotify();
 }
 
 void ActiveTabPermissionGranter::WebContentsDestroyed() {

chrome/browser/extensions/active_tab_unittest.cc

@@ -16,9 +16,11 @@
 #include "chrome/browser/extensions/extension_service_test_base.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/extensions/tab_helper.h"
+#include "chrome/browser/extensions/test_extension_system.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "chrome/test/base/testing_profile.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_details.h"
@@ -31,6 +33,7 @@
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_browser_thread.h"
 #include "content/public/test/web_contents_tester.h"
+#include "extensions/browser/disable_reason.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/test_extension_registry_observer.h"
 #include "extensions/common/constants.h"
@@ -45,6 +48,7 @@
 
 #if defined(OS_CHROMEOS)
 #include "base/run_loop.h"
+#include "chrome/browser/chromeos/app_mode/kiosk_app_manager.h"
 #include "chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.h"
 #include "chrome/browser/chromeos/login/users/chrome_user_manager_impl.h"
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
@@ -69,23 +73,16 @@ namespace extensions {
 namespace {
 
 scoped_refptr<const Extension> CreateTestExtension(
-    const std::string& id,
+    const std::string& name,
     bool has_active_tab_permission,
     bool has_tab_capture_permission) {
-  ListBuilder permissions;
+  ExtensionBuilder builder(name);
   if (has_active_tab_permission)
-    permissions.Append("activeTab");
+    builder.AddPermission("activeTab");
   if (has_tab_capture_permission)
-    permissions.Append("tabCapture");
-  return ExtensionBuilder()
-      .SetManifest(DictionaryBuilder()
-                       .Set("name", "Extension with ID " + id)
-                       .Set("version", "1.0")
-                       .Set("manifest_version", 2)
-                       .Set("permissions", permissions.Build())
-                       .Build())
-      .SetID(id)
-      .Build();
+    builder.AddPermission("tabCapture");
+
+  return builder.Build();
 }
 
 enum PermittedFeature {
@@ -137,6 +134,24 @@ class ActiveTabTest : public ChromeRenderViewHostTestHarness {
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     TabHelper::CreateForWebContents(web_contents());
+
+    // We need to add extensions to the ExtensionService; else trying to commit
+    // any of their URLs fails and redirects to about:blank.
+    ExtensionService* service =
+        static_cast<TestExtensionSystem*>(ExtensionSystem::Get(profile()))
+            ->CreateExtensionService(base::CommandLine::ForCurrentProcess(),
+                                     base::FilePath(), false);
+    service->AddExtension(extension.get());
+    service->AddExtension(another_extension.get());
+    service->AddExtension(extension_without_active_tab.get());
+    service->AddExtension(extension_with_tab_capture.get());
+  }
+
+  void TearDown() override {
+#if defined(OS_CHROMEOS)
+    chromeos::KioskAppManager::Shutdown();
+#endif
+    ChromeRenderViewHostTestHarness::TearDown();
   }
 
   int tab_id() { return SessionTabHelper::IdForTab(web_contents()).id(); }
@@ -252,24 +267,18 @@ TEST_F(ActiveTabTest, GrantToSinglePage) {
   EXPECT_TRUE(IsBlocked(another_extension, mail_google));
   EXPECT_TRUE(IsBlocked(extension_without_active_tab, mail_google));
 
-  // Reloading the page should clear the active permissions.
+  // Reloading the page should not clear the active permissions, since the
+  // user remains on the same site.
   content::NavigationSimulator::Reload(web_contents());
 
-  EXPECT_TRUE(IsBlocked(extension, google));
+  EXPECT_TRUE(IsAllowed(extension, google));
   EXPECT_TRUE(IsBlocked(another_extension, google));
   EXPECT_TRUE(IsBlocked(extension_without_active_tab, google));
 
-  EXPECT_FALSE(HasTabsPermission(extension));
+  EXPECT_TRUE(HasTabsPermission(extension));
   EXPECT_FALSE(HasTabsPermission(another_extension));
   EXPECT_FALSE(HasTabsPermission(extension_without_active_tab));
 
-  // But they should still be able to be granted again.
-  active_tab_permission_granter()->GrantIfRequested(extension.get());
-
-  EXPECT_TRUE(IsAllowed(extension, google));
-  EXPECT_TRUE(IsBlocked(another_extension, google));
-  EXPECT_TRUE(IsBlocked(extension_without_active_tab, google));
-
   // And grant a few more times redundantly for good measure.
   active_tab_permission_granter()->GrantIfRequested(extension.get());
   active_tab_permission_granter()->GrantIfRequested(extension.get());
@@ -349,6 +358,7 @@ TEST_F(ActiveTabTest, CapturingPagesWithActiveTab) {
   for (const GURL& url : test_urls) {
     SCOPED_TRACE(url);
     NavigateAndCommit(url);
+    EXPECT_EQ(url, web_contents()->GetLastCommittedURL());
     // By default, there should be no access.
     EXPECT_FALSE(extension->permissions_data()->CanCaptureVisiblePage(
         url, tab_id(), nullptr /*error*/));
@@ -363,7 +373,7 @@ TEST_F(ActiveTabTest, CapturingPagesWithActiveTab) {
   }
 }
 
-TEST_F(ActiveTabTest, Uninstalling) {
+TEST_F(ActiveTabTest, Unloading) {
   // Some semi-arbitrary setup.
   GURL google("http://www.google.com");
   NavigateAndCommit(google);
@@ -373,11 +383,10 @@ TEST_F(ActiveTabTest, Uninstalling) {
   EXPECT_TRUE(IsGrantedForTab(extension.get(), web_contents()));
   EXPECT_TRUE(IsAllowed(extension, google));
 
-  // Uninstalling the extension should clear its tab permissions.
-  ExtensionRegistry* registry =
-      ExtensionRegistry::Get(web_contents()->GetBrowserContext());
-  registry->TriggerOnUnloaded(extension.get(),
-                              UnloadedExtensionReason::DISABLE);
+  // Unloading the extension should clear its tab permissions.
+  ExtensionSystem::Get(web_contents()->GetBrowserContext())
+      ->extension_service()
+      ->DisableExtension(extension->id(), disable_reason::DISABLE_USER_ACTION);
 
   // Note: can't EXPECT_FALSE(IsAllowed) here because uninstalled extensions
   // are just that... considered to be uninstalled, and the manager might
@@ -440,8 +449,8 @@ TEST_F(ActiveTabTest, SameDocumentNavigations) {
 
   EXPECT_FALSE(IsAllowed(extension, google));
   EXPECT_FALSE(IsAllowed(extension, google_h1));
-  EXPECT_FALSE(IsAllowed(extension, chromium));
-  EXPECT_FALSE(IsAllowed(extension, chromium_h1));
+  EXPECT_TRUE(IsAllowed(extension, chromium));
+  EXPECT_TRUE(IsAllowed(extension, chromium_h1));
 }
 
 TEST_F(ActiveTabTest, ChromeUrlGrants) {

chrome/common/extensions/docs/templates/articles/activeTab.html

@@ -19,13 +19,23 @@
 <h1>The activeTab permission</h1>
 
 <p>
-The <code>activeTab</code> permission gives an extension temporary access to the currently active tab when the user <em>invokes</em> the extension - for example by clicking its <a href="browserAction">browser action</a>. Access to the tab lasts until the tab is navigated or closed.
+The <code>activeTab</code> permission gives an extension temporary access to the
+currently active tab when the user <em>invokes</em> the extension - for example
+by clicking its <a href="browserAction">browser action</a>. Access to the tab
+lasts while the user is on that page, and is revoked when the user navigates
+away or closes the tab.
 </p>
 
 <p>
 This serves as an alternative for many uses of <code>&lt;all_urls&gt;</code>, but displays <em>no warning message</em> during installation:
 </p>
 
+<b>Note:</b> From M72 onwards, the <code>activeTab</code> permission will be
+granted until the user navigates to a different origin. That is, if the user
+invokes the extension on https://example.com and then navigates to
+https://example.com/foo, the extension will continue to have access to the page.
+If the user navigates to https://chromium.org, access is revoked.
+
 <table id="active-tab-images" class="simple">
   <tr>
     <th>Without activeTab</th>
@@ -85,7 +95,11 @@ Without <code>activeTab</code>, this extension would need to request full, persi
 </p>
 
 <p>
-In contrast, an extension with the <code>activeTab</code> permission only obtains access to a tab in response to an explicit user gesture. If the extension is compromised the attacker would need to wait for the user to invoke the extension before obtaining access. And that access only lasts until the tab is navigated or closed.
+In contrast, an extension with the <code>activeTab</code> permission only
+obtains access to a tab in response to an explicit user gesture. If the
+extension is compromised the attacker would need to wait for the user to invoke
+the extension before obtaining access. And that access only lasts until the tab
+is navigated or is closed.
 </p>
 
 <h2 id="what-activeTab-allows">What activeTab allows</h2>

chrome/test/data/extensions/api_test/active_tab/background.js

@@ -42,8 +42,6 @@ var injectIframe =
     'iframe.src = "' + iframeUrl + '";\n' +
     'document.body.appendChild(iframe);\n';
 
-var testConfig;  // Populated in response to chrome.test.getConfig().
-
 var runCount = 0;
 chrome.browserAction.onClicked.addListener(function(tab) {
   runCount++;
@@ -80,16 +78,12 @@ chrome.webNavigation.onCompleted.addListener(function(details) {
   if (!details.url.endsWith('page.html'))
     return;
 
-  assertTrue(!!testConfig);
-  assertTrue(
-      testConfig.customArg == 'RuntimeHostPermissionsEnabled' ||
-      testConfig.customArg == 'RuntimeHostPermissionsDisabled');
-
   navigationCount++;
   chrome.test.sendMessage(navigationCount.toString());
 
-  var expectHasAccess = navigationCount === 2 &&
-      testConfig.customArg === 'RuntimeHostPermissionsEnabled';
+  // The second navigation remains on the same site, so we should still have
+  // access.
+  var expectHasAccess = navigationCount === 2;
 
   if (expectHasAccess) {
     chrome.tabs.executeScript({code: 'true'}, callbackPass());
@@ -112,7 +106,4 @@ chrome.webNavigation.onCompleted.addListener(function(details) {
   assertFalse(canXhr(details.url));
 });
 
-chrome.test.getConfig(function(config) {
-  testConfig = config;
-  chrome.test.sendMessage('ready');
-});
+chrome.test.sendMessage('ready');