Bug 1280347 - Remove <all_urls> requirement for the "tab" context r=mixedpuppy

See https://bugzilla.mozilla.org/show_bug.cgi?id=1280347#c28

Differential Revision: https://phabricator.services.mozilla.com/D6831

--HG--
extra : moz-landing-system : lando

browser/components/extensions/child/ext-menus.js

@@ -195,9 +195,8 @@ this.menusInternal = class extends ExtensionAPI {
             return true;
           };
           if (checkValidArg("tab", "tabId")) {
-            if (!context.extension.hasPermission("tabs") ||
-                !context.extension.whiteListedHosts.subsumes(new MatchPattern("<all_urls>"))) {
-              throw new ExtensionError(`The "tab" context requires the "tabs" and "<all_urls>" permission.`);
+            if (!context.extension.hasPermission("tabs")) {
+              throw new ExtensionError(`The "tab" context requires the "tabs" permission.`);
             }
           }
           if (checkValidArg("bookmark", "bookmarkId")) {

browser/components/extensions/parent/ext-menus.js

@@ -348,7 +348,11 @@ var gMenuBuilder = {
         item.checked = true;
       }
 
-      if (contextData.tab) {
+      let {webExtContextData} = contextData;
+      if (contextData.tab &&
+          // If the menu context was overridden by the extension, do not grant
+          // activeTab since the extension also controls the tabId.
+          (!webExtContextData || webExtContextData.extensionId !== item.extension.id)) {
         item.tabManager.addActiveTabPermission(contextData.tab);
       }
 

browser/components/extensions/schemas/menus.json

@@ -442,7 +442,7 @@
                 "type": "integer",
                 "minimum": 0,
                 "optional": true,
-                "description": "Required when context is 'tab'. Requires 'tabs' and '<all_urls>' permission."
+                "description": "Required when context is 'tab'. Requires 'tabs' permission."
               }
             }
           }

browser/components/extensions/test/browser/browser_ext_menus_replace_menu_context.js

@@ -24,7 +24,7 @@ add_task(async function overrideContext_with_context() {
       browser.test.assertEq("testTabAccess", msg, `Expected message in ${browser.runtime.id}`);
       let tab = await browser.tabs.get(tabId);
       if (!tab.url) { // tabs or activeTab not active.
-        browser.test.sendMessage("testTabAccessDone", false);
+        browser.test.sendMessage("testTabAccessDone", "tab_no_url");
         return;
       }
       try {
@@ -32,10 +32,12 @@ add_task(async function overrideContext_with_context() {
           code: "document.URL",
         });
         browser.test.assertEq("http://example.com/?SomeTab", url, "Expected successful executeScript");
+        browser.test.sendMessage("testTabAccessDone", "executeScript_ok");
+        return;
       } catch (e) {
-        browser.test.fail(`Failed to execute script at ${tabId} (${tab.url}): ${e}`);
+        browser.test.assertEq("Missing host permission for the tab", e.message, "Expected error message");
+        browser.test.sendMessage("testTabAccessDone", "executeScript_failed");
       }
-      browser.test.sendMessage("testTabAccessDone", true);
     });
     browser.menus.onShown.addListener((info, tab) => {
       browser.test.sendMessage("onShown", {
@@ -66,7 +68,7 @@ add_task(async function overrideContext_with_context() {
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       applications: {gecko: {id: "@menu-test-extension"}},
-      permissions: ["menus", "menus.overrideContext", "tabs", "bookmarks", "<all_urls>"],
+      permissions: ["menus", "menus.overrideContext", "tabs", "bookmarks"],
     },
     files: {
       "tab.html": `
@@ -85,6 +87,9 @@ add_task(async function overrideContext_with_context() {
         let testCases = [{
           context: "tab",
           tabId: tab.id,
+        }, {
+          context: "tab",
+          tabId: tab.id,
         }, {
           context: "bookmark",
           bookmarkId: bookmark.id,
@@ -145,14 +150,15 @@ add_task(async function overrideContext_with_context() {
       `${makeWidgetId(otherExtension.id)}-menuitem-_tab_context`,
     ], "Expected menu items after changing context to tab");
 
-    // Extension should already be able to execute script due to host permissions.
     extension.sendMessage("testTabAccess", tabId);
-    ok(await extension.awaitMessage("testTabAccessDone"),
-       "Extension has access via permissions");
+    is(await extension.awaitMessage("testTabAccessDone"),
+       "executeScript_failed",
+       "executeScript should fail due to the lack of permissions.");
 
     otherExtension.sendMessage("testTabAccess", tabId);
-    isnot(await otherExtension.awaitMessage("testTabAccessDone"),
-          "Other extension should not have activeTab permissions yet.");
+    is(await otherExtension.awaitMessage("testTabAccessDone"),
+       "tab_no_url",
+       "Other extension should not have activeTab permissions yet.");
 
     // Click on the menu item of the other extension to unlock host permissions.
     let menuItems = menu.getElementsByAttribute("label", "tab_context");
@@ -165,13 +171,44 @@ add_task(async function overrideContext_with_context() {
       tabId,
     }, "Expected onClicked details after changing context to tab");
 
+    extension.sendMessage("testTabAccess", tabId);
+    is(await extension.awaitMessage("testTabAccessDone"),
+       "executeScript_failed",
+       "executeScript of extension that created the menu should still fail.");
+
     otherExtension.sendMessage("testTabAccess", tabId);
-    ok(await otherExtension.awaitMessage("testTabAccessDone"),
+    is(await otherExtension.awaitMessage("testTabAccessDone"),
+       "executeScript_ok",
        "Other extension should have activeTab permissions.");
   }
 
   {
-    // Test case 2: context=bookmark
+    // Test case 2: context=tab, click on menu item of extension..
+    let menu = await openContextMenu("a");
+    await extension.awaitMessage("oncontextmenu_in_dom");
+
+    // The previous test has already verified the visible menu items,
+    // so we skip checking the onShown result and only test clicking.
+    await extension.awaitMessage("onShown");
+    await otherExtension.awaitMessage("onShown");
+    let menuItems = menu.getElementsByAttribute("label", "tab_context");
+    is(menuItems.length, 2, "There are two menu items with label 'tab_context'");
+    await closeExtensionContextMenu(menuItems[0]);
+
+    Assert.deepEqual(await extension.awaitMessage("onClicked"), {
+      menuItemId: "tab_context",
+      bookmarkId: undefined,
+      tabId,
+    }, "Expected onClicked details after changing context to tab");
+
+    extension.sendMessage("testTabAccess", tabId);
+    is(await extension.awaitMessage("testTabAccessDone"),
+       "executeScript_failed",
+       "activeTab permission should not be available to the extension that created the menu.");
+  }
+
+  {
+    // Test case 3: context=bookmark
     let menu = await openContextMenu("a");
     await extension.awaitMessage("oncontextmenu_in_dom");
     for (let ext of [extension, otherExtension]) {
@@ -193,7 +230,7 @@ add_task(async function overrideContext_with_context() {
   }
 
   {
-    // Test case 3: context=tab, invalid tabId.
+    // Test case 4: context=tab, invalid tabId.
     let menu = await openContextMenu("a");
     await extension.awaitMessage("oncontextmenu_in_dom");
     // When an invalid tabId is used, all extension menu logic is skipped and

browser/components/extensions/test/browser/browser_ext_menus_replace_menu_permissions.js

@@ -19,7 +19,7 @@ add_task(async function overrideContext_permissions() {
     const CONTEXT_OPTIONS_TAB = {context: "tab", tabId: 1};
     const CONTEXT_OPTIONS_BOOKMARK = {context: "bookmark", bookmarkId: "x"};
 
-    const E_PERM_TAB = /The "tab" context requires the "tabs" and "<all_urls>" permission/;
+    const E_PERM_TAB = /The "tab" context requires the "tabs" permission/;
     const E_PERM_BOOKMARK = /The "bookmark" context requires the "bookmarks" permission/;
 
     function assertAllowed(contextOptions) {
@@ -94,30 +94,12 @@ add_task(async function overrideContext_permissions() {
       browser.test.log("Active permissions: tabs");
       yield;
 
-      assertNotAllowed(CONTEXT_OPTIONS_BOOKMARK, E_PERM_BOOKMARK);
-      // "tabs" permission was granted, but host permissions are required too.
-      assertNotAllowed(CONTEXT_OPTIONS_TAB, E_PERM_TAB);
-
-      await requestPermissions({origins: ["<all_urls>"]});
-      await browser.permissions.remove({permissions: ["tabs"]});
-      browser.test.log("Active permissions: <all_urls>");
-      yield;
-
-      // Just host permissions are not sufficient for either context.
-      assertNotAllowed(CONTEXT_OPTIONS_BOOKMARK, E_PERM_BOOKMARK);
-      assertNotAllowed(CONTEXT_OPTIONS_TAB, E_PERM_TAB);
-
-      await requestPermissions({permissions: ["tabs"]});
-      browser.test.log("Active permissions: <all_urls>, tabs");
-      yield;
-
       assertNotAllowed(CONTEXT_OPTIONS_BOOKMARK, E_PERM_BOOKMARK);
       assertAllowed(CONTEXT_OPTIONS_TAB);
-      await browser.permissions.remove({origins: ["<all_urls>"]});
-      browser.test.log("Active permissions: tabs without hosts");
+      await browser.permissions.remove({permissions: ["tabs"]});
+      browser.test.log("Active permissions: none");
       yield;
 
-      // Host permissions were revoked, access should be blocked again.
       assertNotAllowed(CONTEXT_OPTIONS_TAB, E_PERM_TAB);
 
       await browser.permissions.remove({permissions: ["menus.overrideContext"]});
@@ -142,7 +124,7 @@ add_task(async function overrideContext_permissions() {
     useAddonManager: "temporary", // To automatically show sidebar on load.
     manifest: {
       permissions: ["menus"],
-      optional_permissions: ["menus.overrideContext", "tabs", "bookmarks", "<all_urls>"],
+      optional_permissions: ["menus.overrideContext", "tabs", "bookmarks"],
       sidebar_action: {
         default_panel: "sidebar.html",
       },