Bug 1484263 - Clean up manifest permission parser and add tests r=aswan
authorRob Wu <rob@robwu.nl>
Fri, 11 Jan 2019 18:56:18 +0000
changeset 510686 14fee8b5f596471935ef50d75f56d882977185c3
parent 510685 6685e8b9d8671c087c5190537aa22f17f32ee8bb
child 510687 dc4bd4ee4c223f9a91cbc0112b25f4847d180a56
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
bugs1484263
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 1484263 - Clean up manifest permission parser and add tests r=aswan The "permissions" array of the raw manifest is not (and should not) be used for permission checking, so it is not necessary to strip the "mozillaAddons" permission from it. This commit moves the validation of the "mozillaAddons" permission to classifyPermission, so that the "manifestPermissions" getter (that uses this method too) accurately reflects the supported permissions of an extension. New tests has been added to verify the permission warnings for some combinations of permissions. This also includes tests that verify that only privileged extensions can use "mozillaAddons" to unlock unrestricted schemes. Differential Revision: https://phabricator.services.mozilla.com/D5527
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/test/xpcshell/test_ext_permission_warnings.js
toolkit/components/extensions/test/xpcshell/xpcshell.ini
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -122,28 +122,31 @@ const CHILD_SHUTDOWN_TIMEOUT_MS = 8000;
  * @param {boolean} restrictSchemes
  *
  * @returns {object}
  *          An object with exactly one of the following properties:
  *          "origin" to indicate this is a host/origin permission.
  *          "api" to indicate this is an api permission
  *                (as used for webextensions experiments).
  *          "permission" to indicate this is a regular permission.
+ *          "invalid" to indicate that the given permission cannot be used.
  */
 function classifyPermission(perm, restrictSchemes) {
   let match = /^(\w+)(?:\.(\w+)(?:\.\w+)*)?$/.exec(perm);
   if (!match) {
     try {
       let {pattern} = new MatchPattern(perm, {restrictSchemes, ignorePath: true});
       return {origin: pattern};
     } catch (e) {
-      return {originInvalid: perm};
+      return {invalid: perm};
     }
   } else if (match[1] == "experiments" && match[2]) {
     return {api: match[2]};
+  } else if (perm === "mozillaAddons" && restrictSchemes) {
+    return {invalid: perm};
   }
   return {permission: perm};
 }
 
 const LOGGER_ID_BASE = "addons.webextension.";
 const UUID_MAP_PREF = "extensions.webextensions.uuids";
 const LEAVE_STORAGE_PREF = "extensions.webextensions.keepStorageOnUninstall";
 const LEAVE_UUID_PREF = "extensions.webextensions.keepUuidOnUninstall";
@@ -648,18 +651,18 @@ class ExtensionData {
         }
 
         let type = classifyPermission(perm, restrictSchemes);
         if (type.origin) {
           perm = type.origin;
           originPermissions.add(perm);
         } else if (type.api) {
           apiNames.add(type.api);
-        } else if (type.originInvalid) {
-          this.manifestWarning(`Invalid host permission: ${perm}`);
+        } else if (type.invalid) {
+          this.manifestWarning(`Invalid extension permission: ${perm}`);
           continue;
         }
 
         permissions.add(perm);
       }
 
       if (this.id) {
         // An extension always gets permission to its own url.
@@ -1540,34 +1543,18 @@ class Extension extends ExtensionData {
 
   saveStartupData() {
     if (this.dontSaveStartupData) {
       return;
     }
     XPIProvider.setStartupData(this.id, this.startupData);
   }
 
-  async _parseManifest() {
-    let manifest = await super.parseManifest();
-    if (manifest && manifest.permissions.has("mozillaAddons") &&
-        !this.isPrivileged) {
-      Cu.reportError(`Stripping mozillaAddons permission from ${this.id}`);
-      manifest.permissions.delete("mozillaAddons");
-      let i = manifest.manifest.permissions.indexOf("mozillaAddons");
-      if (i >= 0) {
-        manifest.manifest.permissions.splice(i, 1);
-      } else {
-        throw new Error("Could not find mozilaAddons in original permissions array");
-      }
-    }
-    return manifest;
-  }
-
   parseManifest() {
-    return StartupCache.manifests.get(this.manifestCacheKey, () => this._parseManifest());
+    return StartupCache.manifests.get(this.manifestCacheKey, () => super.parseManifest());
   }
 
   async cachePermissions() {
     let manifestData = await this.parseManifest();
 
     manifestData.originPermissions = this.whiteListedHosts.patterns.map(pat => pat.pattern);
     manifestData.permissions = this.permissions;
     return StartupCache.manifests.set(this.manifestCacheKey, manifestData);
new file mode 100644
--- /dev/null
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_permission_warnings.js
@@ -0,0 +1,227 @@
+"use strict";
+
+let {ExtensionTestCommon} = ChromeUtils.import("resource://testing-common/ExtensionTestCommon.jsm", {});
+
+const bundle = Services.strings.createBundle("chrome://browser/locale/browser.properties");
+const DUMMY_APP_NAME = "Dummy brandName";
+
+async function getManifestPermissions(extensionData) {
+  let extension = ExtensionTestCommon.generate(extensionData);
+  await extension.loadManifest();
+  return extension.manifestPermissions;
+}
+
+function getPermissionWarnings(manifestPermissions) {
+  let info = {
+    permissions: manifestPermissions,
+    appName: DUMMY_APP_NAME,
+  };
+  let {msgs} = ExtensionData.formatPermissionStrings(info, bundle);
+  return msgs;
+}
+
+// Tests that the expected permission warnings are generated for various
+// combinations of host permissions.
+add_task(async function host_permissions() {
+  let {PluralForm} = ChromeUtils.import("resource://gre/modules/PluralForm.jsm", {});
+
+  let permissionTestCases = [{
+    description: "Empty manifest without permissions",
+    manifest: {},
+    expectedOrigins: [],
+    expectedWarnings: [],
+  }, {
+    description: "Invalid match patterns",
+    manifest: {
+      permissions: [
+        "https:///",
+        "https://",
+        "https://*",
+        "about:ugh",
+        "about:*",
+        "about://*/",
+        "resource://*/",
+      ],
+    },
+    expectedOrigins: [],
+    expectedWarnings: [],
+  }, {
+    description: "moz-extension: permissions",
+    manifest: {
+      permissions: ["moz-extension://*/*", "moz-extension://uuid/"],
+    },
+    // moz-extension:-origin does not appear in the permission list,
+    // but it is implicitly granted anyway.
+    expectedOrigins: [],
+    expectedWarnings: [],
+  }, {
+    description: "*. host permission",
+    manifest: {
+      // This permission is rejected by the manifest and ignored.
+      permissions: ["http://*./"],
+    },
+    expectedOrigins: [],
+    expectedWarnings: [],
+  }, {
+    description: "<all_urls> permission",
+    manifest: {
+      permissions: ["<all_urls>"],
+    },
+    expectedOrigins: ["<all_urls>"],
+    expectedWarnings: [
+      bundle.GetStringFromName("webextPerms.hostDescription.allUrls"),
+    ],
+  }, {
+    description: "file: permissions",
+    manifest: {
+      permissions: ["file://*/"],
+    },
+    expectedOrigins: ["file://*/"],
+    expectedWarnings: [
+      bundle.GetStringFromName("webextPerms.hostDescription.allUrls"),
+    ],
+  }, {
+    description: "http: permission",
+    manifest: {
+      permissions: ["http://*/"],
+    },
+    expectedOrigins: ["http://*/"],
+    expectedWarnings: [
+      bundle.GetStringFromName("webextPerms.hostDescription.allUrls"),
+    ],
+  }, {
+    description: "*://*/ permission",
+    manifest: {
+      permissions: ["*://*/"],
+    },
+    expectedOrigins: ["*://*/"],
+    expectedWarnings: [
+      bundle.GetStringFromName("webextPerms.hostDescription.allUrls"),
+    ],
+  }, {
+    description: "content_script[*].matches",
+    manifest: {
+      content_scripts: [{
+        // This test uses the manifest file without loading the content script
+        // file, so we can use a non-existing dummy file.
+        js: ["dummy.js"],
+        matches: ["https://*/"],
+      }],
+    },
+    expectedOrigins: ["https://*/"],
+    expectedWarnings: [
+      bundle.GetStringFromName("webextPerms.hostDescription.allUrls"),
+    ],
+  }, {
+    description: "A few host permissions",
+    manifest: {
+      permissions: ["http://a/", "http://*.b/", "http://c/*"],
+    },
+    expectedOrigins: ["http://a/", "http://*.b/", "http://c/*"],
+    expectedWarnings: [
+      // Wildcard hosts take precedence in the permission list.
+      bundle.formatStringFromName("webextPerms.hostDescription.wildcard", ["b"], 1),
+      bundle.formatStringFromName("webextPerms.hostDescription.oneSite", ["a"], 1),
+      bundle.formatStringFromName("webextPerms.hostDescription.oneSite", ["c"], 1),
+    ],
+  }, {
+    description: "many host permission",
+    manifest: {
+      permissions: ["http://a/", "http://b/", "http://c/", "http://d/", "http://e/*",
+                    "http://*.1/", "http://*.2/", "http://*.3/", "http://*.4/"],
+    },
+    expectedOrigins: ["http://a/", "http://b/", "http://c/", "http://d/", "http://e/*",
+                      "http://*.1/", "http://*.2/", "http://*.3/", "http://*.4/"],
+    expectedWarnings: [
+      // Wildcard hosts take precedence in the permission list.
+      bundle.formatStringFromName("webextPerms.hostDescription.wildcard", ["1"], 1),
+      bundle.formatStringFromName("webextPerms.hostDescription.wildcard", ["2"], 1),
+      bundle.formatStringFromName("webextPerms.hostDescription.wildcard", ["3"], 1),
+      bundle.formatStringFromName("webextPerms.hostDescription.wildcard", ["4"], 1),
+      bundle.formatStringFromName("webextPerms.hostDescription.oneSite", ["a"], 1),
+      bundle.formatStringFromName("webextPerms.hostDescription.oneSite", ["b"], 1),
+      bundle.formatStringFromName("webextPerms.hostDescription.oneSite", ["c"], 1),
+      PluralForm.get(2, bundle.GetStringFromName("webextPerms.hostDescription.tooManySites")).replace("#1", "2"),
+    ],
+  }];
+  for (let {description, manifest, expectedOrigins, expectedWarnings} of permissionTestCases) {
+    let manifestPermissions = await getManifestPermissions({
+      manifest,
+    });
+
+    deepEqual(manifestPermissions.origins, expectedOrigins, `Expected origins (${description})`);
+    deepEqual(manifestPermissions.permissions, [], `Expected no non-host permissions (${description})`);
+
+    let warnings = getPermissionWarnings(manifestPermissions);
+    deepEqual(warnings, expectedWarnings, `Expected warnings (${description})`);
+  }
+});
+
+// Tests that the expected permission warnings are generated for a mix of host
+// permissions and API permissions.
+add_task(async function api_permissions() {
+  let manifestPermissions = await getManifestPermissions({
+    manifest: {
+      permissions: [
+        "activeTab", "webNavigation", "tabs", "nativeMessaging",
+        "http://x/", "http://*.x/", "http://*.tld/",
+      ],
+    },
+  });
+  deepEqual(manifestPermissions, {
+    origins: ["http://x/", "http://*.x/", "http://*.tld/"],
+    permissions: ["activeTab", "webNavigation", "tabs", "nativeMessaging"],
+  }, "Expected origins and permissions");
+
+  deepEqual(getPermissionWarnings(manifestPermissions), [
+    // Host permissions first, with wildcards on top.
+    bundle.formatStringFromName("webextPerms.hostDescription.wildcard", ["x"], 1),
+    bundle.formatStringFromName("webextPerms.hostDescription.wildcard", ["tld"], 1),
+    bundle.formatStringFromName("webextPerms.hostDescription.oneSite", ["x"], 1),
+    // nativeMessaging permission warning first of all permissions.
+    bundle.formatStringFromName("webextPerms.description.nativeMessaging", [DUMMY_APP_NAME], 1),
+    // Other permissions in alphabetical order.
+    // Note: activeTab has no permission warning string.
+    bundle.GetStringFromName("webextPerms.description.tabs"),
+    bundle.GetStringFromName("webextPerms.description.webNavigation"),
+  ], "Expected warnings");
+});
+
+// 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/"],
+    },
+  });
+  deepEqual(manifestPermissions, {
+    origins: ["resource://*/*", "http://a/"],
+    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/"],
+    },
+  });
+  deepEqual(manifestPermissions, {
+    origins: ["http://a/"],
+    permissions: [],
+  }, "Expected origins and permissions for unprivileged add-on with mozillaAddons");
+
+  deepEqual(getPermissionWarnings(manifestPermissions), [
+    bundle.formatStringFromName("webextPerms.hostDescription.oneSite", ["a"], 1),
+  ], "Expected warnings for unprivileged add-on with mozillaAddons permission.");
+});
+
--- a/toolkit/components/extensions/test/xpcshell/xpcshell.ini
+++ b/toolkit/components/extensions/test/xpcshell/xpcshell.ini
@@ -33,16 +33,17 @@ tags = webextensions in-process-webexten
 [test_csp_validator.js]
 [test_ext_contexts.js]
 [test_ext_json_parser.js]
 [test_ext_manifest_content_security_policy.js]
 [test_ext_manifest_incognito.js]
 [test_ext_manifest_minimum_chrome_version.js]
 [test_ext_manifest_minimum_opera_version.js]
 [test_ext_manifest_themes.js]
+[test_ext_permission_warnings.js]
 [test_ext_schemas.js]
 [test_ext_schemas_roots.js]
 [test_ext_schemas_async.js]
 [test_ext_schemas_allowed_contexts.js]
 [test_ext_schemas_interactive.js]
 [test_ext_schemas_manifest_permissions.js]
 [test_ext_schemas_privileged.js]
 [test_ext_schemas_revoke.js]