Bug 1504018 - Skip host permissions for which a warning has been shown before r=aswan
authorRob Wu <rob@robwu.nl>
Fri, 11 Jan 2019 19:16:59 +0000
changeset 453584 dc4bd4ee4c223f9a91cbc0112b25f4847d180a56
parent 453583 14fee8b5f596471935ef50d75f56d882977185c3
child 453585 8583d9d483877e7e70bbf1514aca63068e6d9c77
push id35360
push usernbeleuzu@mozilla.com
push dateSat, 12 Jan 2019 09:39:47 +0000
treeherdermozilla-central@cb35977ae7a4 [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 - Skip host permissions for which a warning has been shown before r=aswan Permission warnings only include the host name (ignoring any scheme), so the comparison of old and new permissions should ignore schemes too. Any origin permission has to match the definition of "MatchPattern" as defined in toolkit/components/schemas/manifest.json. For normal (non-privileged extensions), this is either <all_urls>, or a pattern consisting of the "http", "https", "ws", "wss", "file", "ftp" schemes. Depends on D5527 Depends on D5527 Differential Revision: https://phabricator.services.mozilla.com/D14963
dom/chrome-webidl/MatchPattern.webidl
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/MatchPattern.cpp
toolkit/components/extensions/MatchPattern.h
toolkit/components/extensions/test/xpcshell/test_ext_permission_warnings.js
--- a/dom/chrome-webidl/MatchPattern.webidl
+++ b/dom/chrome-webidl/MatchPattern.webidl
@@ -52,16 +52,22 @@ interface MatchPattern {
 
   /**
    * Returns true if this pattern will match any host which would be matched
    * by the given pattern.
    */
   boolean subsumes(MatchPattern pattern);
 
   /**
+   * Returns true if this pattern will match any host which would be matched
+   * by the given pattern, ignoring the scheme.
+   */
+  boolean subsumesDomain(MatchPattern pattern);
+
+  /**
    * Returns true if there is any host which would be matched by both this
    * pattern and the given pattern.
    */
   boolean overlaps(MatchPattern pattern);
 
   /**
    * The match pattern string represented by this pattern.
    */
@@ -94,16 +100,22 @@ interface MatchPatternSet {
   boolean matchesCookie(Cookie cookie);
 
   /**
    * Returns true if any sub-pattern subsumes the given pattern.
    */
   boolean subsumes(MatchPattern pattern);
 
   /**
+   * Returns true if any sub-pattern subsumes the given pattern,
+   * ignoring any of the schemes in the patterns.
+   */
+  boolean subsumesDomain(MatchPattern pattern);
+
+  /**
    * Returns true if any sub-pattern overlaps the given pattern.
    */
   boolean overlaps(MatchPattern pattern);
 
   /**
    * Returns true if any sub-pattern overlaps any sub-pattern the given
    * pattern set.
    */
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -537,17 +537,18 @@ class ExtensionData {
     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);
     return {
-      origins: newPermissions.origins.filter(perm => !oldMatcher.subsumes(new MatchPattern(perm))),
+      // formatPermissionStrings ignores any scheme, so only look at the domain.
+      origins: newPermissions.origins.filter(perm => !oldMatcher.subsumesDomain(new MatchPattern(perm))),
       permissions: newPermissions.permissions.filter(perm => !oldPermissions.permissions.includes(perm)),
     };
   }
 
   canUseExperiment(manifest) {
     return this.experimentsAllowed && manifest.experiment_apis;
   }
 
@@ -1103,16 +1104,18 @@ class ExtensionData {
       if (permission == "<all_urls>") {
         allUrls = true;
         break;
       }
       let match = /^[a-z*]+:\/\/([^/]*)\//.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("*.")) {
         wildcards.add(match[1].slice(2));
       } else {
         sites.add(match[1]);
       }
     }
--- a/toolkit/components/extensions/MatchPattern.cpp
+++ b/toolkit/components/extensions/MatchPattern.cpp
@@ -533,16 +533,25 @@ bool MatchPatternSet::Subsumes(const Mat
   for (const auto& pattern : mPatterns) {
     if (pattern->Subsumes(aPattern)) {
       return true;
     }
   }
   return false;
 }
 
+bool MatchPatternSet::SubsumesDomain(const MatchPattern& aPattern) const {
+  for (const auto& pattern : mPatterns) {
+    if (pattern->SubsumesDomain(aPattern)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 bool MatchPatternSet::Overlaps(const MatchPatternSet& aPatternSet) const {
   for (const auto& pattern : aPatternSet.mPatterns) {
     if (Overlaps(*pattern)) {
       return true;
     }
   }
   return false;
 }
--- a/toolkit/components/extensions/MatchPattern.h
+++ b/toolkit/components/extensions/MatchPattern.h
@@ -192,16 +192,18 @@ class MatchPattern final : public nsISup
   }
 
   bool MatchesCookie(const CookieInfo& aCookie) const;
 
   bool MatchesDomain(const nsACString& aDomain) const;
 
   bool Subsumes(const MatchPattern& aPattern) const;
 
+  bool SubsumesDomain(const MatchPattern& aPattern) const;
+
   bool Overlaps(const MatchPattern& aPattern) const;
 
   bool DomainIsWildcard() const { return mMatchSubdomain && mDomain.IsEmpty(); }
 
   void GetPattern(nsAString& aPattern) const { aPattern = mPattern; }
 
   nsISupports* GetParentObject() const { return mParent; }
 
@@ -212,18 +214,16 @@ class MatchPattern final : public nsISup
   virtual ~MatchPattern() = default;
 
  private:
   explicit MatchPattern(nsISupports* aParent) : mParent(aParent) {}
 
   void Init(JSContext* aCx, const nsAString& aPattern, bool aIgnorePath,
             bool aRestrictSchemes, ErrorResult& aRv);
 
-  bool SubsumesDomain(const MatchPattern& aPattern) const;
-
   nsCOMPtr<nsISupports> mParent;
 
   // The normalized match pattern string that this object represents.
   nsString mPattern;
 
   // The set of atomized URI schemes that this pattern matches.
   RefPtr<AtomSet> mSchemes;
 
@@ -267,16 +267,18 @@ class MatchPatternSet final : public nsI
   bool Matches(const URLInfo& aURL, bool aExplicit, ErrorResult& aRv) const {
     return Matches(aURL, aExplicit);
   }
 
   bool MatchesCookie(const CookieInfo& aCookie) const;
 
   bool Subsumes(const MatchPattern& aPattern) const;
 
+  bool SubsumesDomain(const MatchPattern& aPattern) const;
+
   bool Overlaps(const MatchPattern& aPattern) const;
 
   bool Overlaps(const MatchPatternSet& aPatternSet) const;
 
   bool OverlapsAll(const MatchPatternSet& aPatternSet) const;
 
   void GetPatterns(ArrayType& aPatterns) {
     aPatterns.AppendElements(mPatterns);
--- a/toolkit/components/extensions/test/xpcshell/test_ext_permission_warnings.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_permission_warnings.js
@@ -15,16 +15,23 @@ function getPermissionWarnings(manifestP
   let info = {
     permissions: manifestPermissions,
     appName: DUMMY_APP_NAME,
   };
   let {msgs} = ExtensionData.formatPermissionStrings(info, bundle);
   return msgs;
 }
 
+async function getPermissionWarningsForUpdate(oldExtensionData, newExtensionData) {
+  let oldPerms = await getManifestPermissions(oldExtensionData);
+  let newPerms = await getManifestPermissions(newExtensionData);
+  let difference = Extension.comparePermissions(oldPerms, newPerms);
+  return getPermissionWarnings(difference);
+}
+
 // 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: {},
@@ -220,8 +227,67 @@ add_task(async function unprivileged_wit
     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.");
 });
 
+// Tests that an update with less permissions has no warning.
+add_task(async function update_drop_permission() {
+  let warnings = await getPermissionWarningsForUpdate({
+    manifest: {
+      permissions: ["<all_urls>", "https://a/", "http://b/"],
+    },
+  }, {
+    manifest: {
+      permissions: ["https://a/", "http://b/", "ftp://host_matching_all_urls/"],
+    },
+  });
+  deepEqual(warnings, [], "An update with fewer permissions should not have any warnings");
+});
+
+// Tests that an update that switches from "*://*/*" to "<all_urls>" does not
+// result in additional permission warnings.
+add_task(async function update_all_urls_permission() {
+  let warnings = await getPermissionWarningsForUpdate({
+    manifest: {
+      permissions: ["*://*/*"],
+    },
+  }, {
+    manifest: {
+      permissions: ["<all_urls>"],
+    },
+  });
+  deepEqual(warnings, [], "An update from a wildcard host to <all_urls> should not have any warnings");
+});
+
+// Tests that an update where a new permission whose domain overlaps with
+// an existing permission does not result in additional permission warnings.
+add_task(async function update_change_permissions() {
+  let warnings = await getPermissionWarningsForUpdate({
+    manifest: {
+      permissions: ["https://a/", "http://*.b/", "http://c/", "http://f/"],
+    },
+  }, {
+    manifest: {
+      permissions: [
+        // (no new warning) Unchanged permission from old extension.
+        "https://a/",
+        // (no new warning) Different schemes, host should match "*.b" wildcard.
+        "ftp://ftp.b/", "ws://ws.b/", "wss://wss.b",
+        "https://https.b/", "http://http.b/", "*://*.b/", "http://b/",
+
+        // (expect warning) Wildcard was added.
+        "http://*.c/",
+        // (no new warning) file:-scheme, but host "f" is same as "http://f/".
+        "file://f/",
+        // (expect warning) New permission was added.
+        "proxy",
+      ],
+    },
+  });
+  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");
+});