Bug 1504018 - Support unrestricted schemes in permission warnings r=aswan
authorRob Wu <rob@robwu.nl>
Fri, 11 Jan 2019 19:19:06 +0000
changeset 510688 8583d9d483877e7e70bbf1514aca63068e6d9c77
parent 510687 dc4bd4ee4c223f9a91cbc0112b25f4847d180a56
child 510689 d288304b11fa701f20c691e8e5f5f2f9f6520b31
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaswan
bugs1504018
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1504018 - Support unrestricted schemes in permission warnings r=aswan If an extension with the "mozillaAddons" permission is updated, the permission diffing logic should support restricted schemes. Otherwise the MatchPattern will throw and prevent the update from being installed. `Extension.comparePermissions` is called with the result of `.userPermissions`, which in turn is equivalent to the result of the `manifestPermissions` getter. This already filters out restricted schemes if needed. Therefore we can unconditionally use `restrictSchemes:false` in `comparePermissions`. And update the regexp in formatPermissionStrings to support permissions that start with "about:", since the "MatchPatternUnestricted" type in toolkit/components/extensions/schemas/manifest.json supports this, and the lack of "//" in about:-URLs prevents the scheme from being matched by the existing pattern. Depends on D14963 Differential Revision: https://phabricator.services.mozilla.com/D14964
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/test/xpcshell/test_ext_permission_warnings.js
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -535,20 +535,20 @@ class ExtensionData {
     result.permissions = [...this.permissions]
       .filter(p => !result.origins.includes(p) && !EXP_PATTERN.test(p));
     return result;
   }
 
   // Compute the difference between two sets of permissions, suitable
   // for presenting to the user.
   static comparePermissions(oldPermissions, newPermissions) {
-    let oldMatcher = new MatchPatternSet(oldPermissions.origins);
+    let oldMatcher = new MatchPatternSet(oldPermissions.origins, {restrictSchemes: false});
     return {
       // formatPermissionStrings ignores any scheme, so only look at the domain.
-      origins: newPermissions.origins.filter(perm => !oldMatcher.subsumesDomain(new MatchPattern(perm))),
+      origins: newPermissions.origins.filter(perm => !oldMatcher.subsumesDomain(new MatchPattern(perm, {restrictSchemes: false}))),
       permissions: newPermissions.permissions.filter(perm => !oldPermissions.permissions.includes(perm)),
     };
   }
 
   canUseExperiment(manifest) {
     return this.experimentsAllowed && manifest.experiment_apis;
   }
 
@@ -1100,17 +1100,20 @@ class ExtensionData {
 
     // First classify our host permissions
     let allUrls = false, wildcards = new Set(), sites = new Set();
     for (let permission of perms.origins) {
       if (permission == "<all_urls>") {
         allUrls = true;
         break;
       }
-      let match = /^[a-z*]+:\/\/([^/]*)\//.exec(permission);
+
+      // Privileged extensions may request access to "about:"-URLs, such as
+      // about:reader.
+      let match = /^[a-z*]+:\/\/([^/]*)\/|^about:/.exec(permission);
       if (!match) {
         throw new Error(`Unparseable host permission ${permission}`);
       }
       // Note: the scheme is ignored in the permission warnings. If this ever
       // changes, update the comparePermissions method as needed.
       if (!match[1] || match[1] == "*") {
         allUrls = true;
       } else if (match[1].startsWith("*.")) {
--- a/toolkit/components/extensions/test/xpcshell/test_ext_permission_warnings.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_permission_warnings.js
@@ -196,35 +196,35 @@ add_task(async function api_permissions(
 
 // Tests that the expected permission warnings are generated for a mix of host
 // permissions and API permissions, for a privileged extension that uses the
 // mozillaAddons permission.
 add_task(async function privileged_with_mozillaAddons() {
   let manifestPermissions = await getManifestPermissions({
     isPrivileged: true,
     manifest: {
-      permissions: ["mozillaAddons", "mozillaAddons", "mozillaAddons", "resource://*/*", "http://a/"],
+      permissions: ["mozillaAddons", "mozillaAddons", "mozillaAddons", "resource://x/*", "http://a/", "about:reader*"],
     },
   });
   deepEqual(manifestPermissions, {
-    origins: ["resource://*/*", "http://a/"],
+    origins: ["resource://x/*", "http://a/", "about:reader*"],
     permissions: ["mozillaAddons"],
   }, "Expected origins and permissions for privileged add-on with mozillaAddons");
 
   deepEqual(getPermissionWarnings(manifestPermissions), [
     bundle.GetStringFromName("webextPerms.hostDescription.allUrls"),
   ], "Expected warnings for privileged add-on with mozillaAddons permission.");
 });
 
 // Similar to the privileged_with_mozillaAddons test, except the test extension
 // is unprivileged and not allowed to use the mozillaAddons permission.
 add_task(async function unprivileged_with_mozillaAddons() {
   let manifestPermissions = await getManifestPermissions({
     manifest: {
-      permissions: ["mozillaAddons", "mozillaAddons", "mozillaAddons", "resource://*/*", "http://a/"],
+      permissions: ["mozillaAddons", "mozillaAddons", "mozillaAddons", "resource://x/*", "http://a/", "about:reader*"],
     },
   });
   deepEqual(manifestPermissions, {
     origins: ["http://a/"],
     permissions: [],
   }, "Expected origins and permissions for unprivileged add-on with mozillaAddons");
 
   deepEqual(getPermissionWarnings(manifestPermissions), [
@@ -286,8 +286,43 @@ add_task(async function update_change_pe
       ],
     },
   });
   deepEqual(warnings, [
     bundle.formatStringFromName("webextPerms.hostDescription.wildcard", ["c"], 1),
     bundle.formatStringFromName("webextPerms.description.proxy", [DUMMY_APP_NAME], 1),
   ], "Expected permission warnings for new permissions only");
 });
+
+// Tests that a privileged extension with the mozillaAddons permission can be
+// updated without errors.
+add_task(async function update_privileged_with_mozillaAddons() {
+  let warnings = await getPermissionWarningsForUpdate({
+    isPrivileged: true,
+    manifest: {
+      permissions: ["mozillaAddons", "resource://a/"],
+    },
+  }, {
+    isPrivileged: true,
+    manifest: {
+      permissions: ["mozillaAddons", "resource://a/", "resource://b/"],
+    },
+  });
+  deepEqual(warnings, [
+    bundle.formatStringFromName("webextPerms.hostDescription.oneSite", ["b"], 1),
+  ], "Expected permission warnings for new host only");
+});
+
+// Tests that an unprivileged extension cannot get privileged permissions
+// through an update.
+add_task(async function update_unprivileged_with_mozillaAddons() {
+  // Unprivileged
+  let warnings = await getPermissionWarningsForUpdate({
+    manifest: {
+      permissions: ["mozillaAddons", "resource://a/"],
+    },
+  }, {
+    manifest: {
+      permissions: ["mozillaAddons", "resource://a/", "resource://b/"],
+    },
+  });
+  deepEqual(warnings, [], "resource:-scheme is unsupported for unprivileged extensions");
+});