[Extensions] ExtensionRequestsHostPermissions examines host permissions

Previously, this method looked at both API and host permissions, which
is not accurate. Change this to look at host permissions explicitly.

Bug: 1473447
Change-Id: I36cc7c42874159bd6f7551c17cbea2b9ecd7a131
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4786967
Commit-Queue: Kelvin Jiang <kelvinjiang@chromium.org>
Reviewed-by: Emilia Paz <emiliapaz@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1185434}

chrome/browser/extensions/extension_context_menu_model.cc

@@ -505,7 +505,8 @@ void ExtensionContextMenuModel::InitMenuWithFeature(
 
   // Show section only when the extension requests host permissions.
   auto* permissions_manager = PermissionsManager::Get(profile_);
-  if (permissions_manager->ExtensionRequestsHostPermissions(*extension)) {
+  if (permissions_manager->ExtensionRequestsHostPermissionsOrActiveTab(
+          *extension)) {
     content::WebContents* web_contents = GetActiveWebContents();
     const GURL& url = web_contents->GetLastCommittedURL();
     // We store the origin to make sure it's the same when executing page access

extensions/browser/permissions_manager.cc

@@ -353,13 +353,6 @@ PermissionsManager::UserSiteAccess PermissionsManager::GetUserSiteAccess(
   DCHECK(
       !extension.permissions_data()->IsRestrictedUrl(gurl, /*error=*/nullptr));
 
-  // Extension with no host permissions but with active tab permission has "on
-  // click" access.
-  if (!ExtensionRequestsHostPermissions(extension) &&
-      HasActiveTabAndCanAccess(extension, gurl)) {
-    return UserSiteAccess::kOnClick;
-  }
-
   ExtensionSiteAccess site_access = GetSiteAccess(extension, gurl);
   if (site_access.has_all_sites_access) {
     return UserSiteAccess::kOnAllSites;
@@ -376,7 +369,7 @@ PermissionsManager::ExtensionSiteAccess PermissionsManager::GetSiteAccess(
   PermissionsManager::ExtensionSiteAccess extension_access;
 
   // Extension that doesn't request host permission has no access.
-  if (!ExtensionRequestsHostPermissions(extension)) {
+  if (!ExtensionRequestsHostPermissionsOrActiveTab(extension)) {
     return extension_access;
   }
 
@@ -443,10 +436,16 @@ PermissionsManager::ExtensionSiteAccess PermissionsManager::GetSiteAccess(
   return extension_access;
 }
 
-bool PermissionsManager::ExtensionRequestsHostPermissions(
+bool PermissionsManager::ExtensionRequestsHostPermissionsOrActiveTab(
     const Extension& extension) const {
-  return !PermissionsParser::GetRequiredPermissions(&extension).IsEmpty() ||
-         !PermissionsParser::GetOptionalPermissions(&extension).IsEmpty();
+  auto has_hosts_or_active_tab = [](const PermissionSet& permissions) {
+    return !permissions.effective_hosts().is_empty() ||
+           permissions.HasAPIPermission(mojom::APIPermissionID::kActiveTab);
+  };
+  return has_hosts_or_active_tab(
+             PermissionsParser::GetRequiredPermissions(&extension)) ||
+         has_hosts_or_active_tab(
+             PermissionsParser::GetOptionalPermissions(&extension));
 }
 
 bool PermissionsManager::CanAffectExtension(const Extension& extension) const {
@@ -456,7 +455,7 @@ bool PermissionsManager::CanAffectExtension(const Extension& extension) const {
 
   // The extension can be affected by runtime host permissions if it requests
   // host permissions.
-  return ExtensionRequestsHostPermissions(extension);
+  return ExtensionRequestsHostPermissionsOrActiveTab(extension);
 }
 
 bool PermissionsManager::CanUserSelectSiteAccess(

extensions/browser/permissions_manager.h

@@ -180,8 +180,9 @@ class PermissionsManager : public KeyedService {
   ExtensionSiteAccess GetSiteAccess(const Extension& extension,
                                     const GURL& url) const;
 
-  // Returns whether the extension requests host permissions.
-  bool ExtensionRequestsHostPermissions(const Extension& extension) const;
+  // Returns whether the extension requests host permissions or activeTab.
+  bool ExtensionRequestsHostPermissionsOrActiveTab(
+      const Extension& extension) const;
 
   // Returns true if the associated extension can be affected by
   // runtime host permissions.

extensions/browser/permissions_manager_unittest.cc

@@ -42,6 +42,9 @@ class PermissionsManagerUnittest : public ExtensionsTest {
       delete;
 
   scoped_refptr<const Extension> AddExtension(const std::string& name);
+  scoped_refptr<const Extension> AddExtensionWithAPIPermission(
+      const std::string& name,
+      const std::string& permission);
   scoped_refptr<const Extension> AddExtensionWithHostPermission(
       const std::string& name,
       const std::string& host_permission);
@@ -89,15 +92,15 @@ scoped_refptr<const Extension> PermissionsManagerUnittest::AddExtension(
 }
 
 scoped_refptr<const Extension>
-PermissionsManagerUnittest::AddExtensionWithHostPermission(
+PermissionsManagerUnittest::AddExtensionWithAPIPermission(
     const std::string& name,
-    const std::string& host_permission) {
+    const std::string& permission) {
   scoped_refptr<const extensions::Extension> extension =
       extensions::ExtensionBuilder(name)
           .SetManifestVersion(3)
-          .SetManifestKey("host_permissions",
-                          base::Value::List().Append(host_permission))
+          .AddPermission(permission)
           .Build();
+  DCHECK(extension->permissions_data()->HasAPIPermission(permission));
 
   ExtensionRegistryFactory::GetForBrowserContext(browser_context())
       ->AddEnabled(extension);
@@ -106,20 +109,27 @@ PermissionsManagerUnittest::AddExtensionWithHostPermission(
 }
 
 scoped_refptr<const Extension>
-PermissionsManagerUnittest::AddExtensionWithActiveTab(const std::string& name) {
+PermissionsManagerUnittest::AddExtensionWithHostPermission(
+    const std::string& name,
+    const std::string& host_permission) {
   scoped_refptr<const extensions::Extension> extension =
       extensions::ExtensionBuilder(name)
           .SetManifestVersion(3)
-          .AddPermission("activeTab")
+          .SetManifestKey("host_permissions",
+                          base::Value::List().Append(host_permission))
           .Build();
-  DCHECK(extension->permissions_data()->HasAPIPermission("activeTab"));
 
   ExtensionRegistryFactory::GetForBrowserContext(browser_context())
       ->AddEnabled(extension);
 
   return extension;
 }
 
+scoped_refptr<const Extension>
+PermissionsManagerUnittest::AddExtensionWithActiveTab(const std::string& name) {
+  return AddExtensionWithAPIPermission(name, "activeTab");
+}
+
 const base::Value* PermissionsManagerUnittest::GetRestrictedSitesFromPrefs() {
   const base::Value::Dict& permissions =
       extension_prefs_->GetPrefAsDictionary(kUserPermissions);
@@ -470,6 +480,27 @@ TEST_F(PermissionsManagerUnittest, CanUserSelectSiteAccess_ActiveTab) {
                                                  UserSiteAccess::kOnAllSites));
 }
 
+TEST_F(PermissionsManagerUnittest,
+       ExtensionRequestsHostPermissionsOrActiveTab) {
+  auto no_permissions_extension = AddExtension("Extension");
+  auto dnr_extension =
+      AddExtensionWithAPIPermission("DNR extension", "declarativeNetRequest");
+  auto active_tab_extension = AddExtensionWithActiveTab("ActiveTab Extension");
+  auto host_permissions_extension = AddExtensionWithHostPermission(
+      "RequestedUrl Extension", "*://*.requested.com/*");
+
+  // Verify that ExtensionRequestsHostPermissionsOrActiveTab returns true only
+  // for extensions that explicitly request host permissions or activeTab.
+  EXPECT_FALSE(manager_->ExtensionRequestsHostPermissionsOrActiveTab(
+      *no_permissions_extension));
+  EXPECT_FALSE(
+      manager_->ExtensionRequestsHostPermissionsOrActiveTab(*dnr_extension));
+  EXPECT_TRUE(manager_->ExtensionRequestsHostPermissionsOrActiveTab(
+      *active_tab_extension));
+  EXPECT_TRUE(manager_->ExtensionRequestsHostPermissionsOrActiveTab(
+      *host_permissions_extension));
+}
+
 class PermissionsManagerWithPermittedSitesUnitTest
     : public PermissionsManagerUnittest {
  public: