Bug 1627220 - Skip expired permissions in getAllWithTypePrefix and getAllForPrincipal. r=baku

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

extensions/permissions/PermissionManager.cpp

@@ -1515,6 +1515,16 @@ nsresult PermissionManager::CreateTable() {
                          ")"));
 }
 
+// Returns whether the given combination of expire type and expire time are
+// expired. Note that EXPIRE_SESSION only honors expireTime if it is nonzero.
+bool PermissionManager::HasExpired(uint32_t aExpireType,
+                                     int64_t aExpireTime) {
+  return (aExpireType == nsIPermissionManager::EXPIRE_TIME ||
+          (aExpireType == nsIPermissionManager::EXPIRE_SESSION &&
+           aExpireTime != 0)) &&
+         aExpireTime <= EXPIRY_NOW;
+}
+
 NS_IMETHODIMP
 PermissionManager::AddFromPrincipal(nsIPrincipal* aPrincipal,
                                     const nsACString& aType,
@@ -1528,12 +1538,8 @@ PermissionManager::AddFromPrincipal(nsIPrincipal* aPrincipal,
                      aExpireType == nsIPermissionManager::EXPIRE_POLICY,
                  NS_ERROR_INVALID_ARG);
 
-  // Skip addition if the permission is already expired. Note that
-  // EXPIRE_SESSION only honors expireTime if it is nonzero.
-  if ((aExpireType == nsIPermissionManager::EXPIRE_TIME ||
-       (aExpireType == nsIPermissionManager::EXPIRE_SESSION &&
-        aExpireTime != 0)) &&
-      aExpireTime <= EXPIRY_NOW) {
+  // Skip addition if the permission is already expired.
+  if (HasExpired(aExpireType, aExpireTime)) {
     return NS_OK;
   }
 
@@ -2237,6 +2243,13 @@ NS_IMETHODIMP PermissionManager::GetAllWithTypePrefix(
         continue;
       }
 
+      // If the permission is expired, skip it. We're not deleting it here
+      // because we're iterating over a lot of permissions.
+      // It will be removed as part of the daily maintenance later.
+      if (HasExpired(permEntry.mExpireType, permEntry.mExpireTime)) {
+        continue;
+      }
+
       if (!aPrefix.IsEmpty() &&
           !StringBeginsWith(mTypeArray[permEntry.mType], aPrefix)) {
         continue;
@@ -2294,6 +2307,13 @@ PermissionManager::GetAllForPrincipal(
         continue;
       }
 
+      // If the permission is expired, skip it. We're not deleting it here
+      // because we're iterating over a lot of permissions.
+      // It will be removed as part of the daily maintenance later.
+      if (HasExpired(permEntry.mExpireType, permEntry.mExpireTime)) {
+        continue;
+      }
+
       // Stripped principal permissions overwrite regular ones
       // For each permission check if there is a stripped permission we should
       // use instead
@@ -2497,11 +2517,7 @@ PermissionManager::PermissionHashKey* PermissionManager::GetPermissionHashKey(
     PermissionEntry permEntry = entry->GetPermission(aType);
 
     // if the entry is expired, remove and keep looking for others.
-    // Note that EXPIRE_SESSION only honors expireTime if it is nonzero.
-    if ((permEntry.mExpireType == nsIPermissionManager::EXPIRE_TIME ||
-         (permEntry.mExpireType == nsIPermissionManager::EXPIRE_SESSION &&
-          permEntry.mExpireTime != 0)) &&
-        permEntry.mExpireTime <= EXPIRY_NOW) {
+    if (HasExpired(permEntry.mExpireType, permEntry.mExpireTime)) {
       entry = nullptr;
       RemoveFromPrincipal(aPrincipal, mTypeArray[aType]);
     } else if (permEntry.mPermission == nsIPermissionManager::UNKNOWN_ACTION) {
@@ -2564,11 +2580,7 @@ PermissionManager::PermissionHashKey* PermissionManager::GetPermissionHashKey(
     PermissionEntry permEntry = entry->GetPermission(aType);
 
     // if the entry is expired, remove and keep looking for others.
-    // Note that EXPIRE_SESSION only honors expireTime if it is nonzero.
-    if ((permEntry.mExpireType == nsIPermissionManager::EXPIRE_TIME ||
-         (permEntry.mExpireType == nsIPermissionManager::EXPIRE_SESSION &&
-          permEntry.mExpireTime != 0)) &&
-        permEntry.mExpireTime <= EXPIRY_NOW) {
+    if (HasExpired(permEntry.mExpireType, permEntry.mExpireTime)) {
       entry = nullptr;
       // If we need to remove a permission we mint a principal.  This is a bit
       // inefficient, but hopefully this code path isn't super common.

extensions/permissions/PermissionManager.h

@@ -362,6 +362,10 @@ class PermissionManager final : public nsIPermissionManager,
   // Returns -1 on failure
   int32_t GetTypeIndex(const nsACString& aType, bool aAdd);
 
+  // Returns whether the given combination of expire type and expire time are
+  // expired. Note that EXPIRE_SESSION only honors expireTime if it is nonzero.
+  bool HasExpired(uint32_t aExpireType, int64_t aExpireTime);
+
   // Returns PermissionHashKey for a given { host, isInBrowserElement } tuple.
   // This is not simply using PermissionKey because we will walk-up domains in
   // case of |host| contains sub-domains. Returns null if nothing found. Also

extensions/permissions/test/unit/test_permmanager_expiration.js

@@ -93,6 +93,12 @@ function* do_run_test() {
     1,
     pm.testPermissionFromPrincipal(principal, "test/expiration-perm-nexp")
   );
+  Assert.equal(1, pm.getAllWithTypePrefix("test/expiration-perm-exp3").length);
+  Assert.equal(
+    1,
+    pm.getAllWithTypePrefix("test/expiration-session-exp3").length
+  );
+  Assert.equal(1, pm.getAllWithTypePrefix("test/expiration-perm-nexp").length);
 
   // ... and the first one has
   do_timeout(10, continue_test);
@@ -106,6 +112,8 @@ function* do_run_test() {
     pm.testPermissionFromPrincipal(principal, "test/expiration-session-exp")
   );
 
+  Assert.equal(5, pm.getAllForPrincipal(principal).length);
+
   // ... and that the short-term one will
   do_timeout(200, continue_test);
   yield;
@@ -117,6 +125,13 @@ function* do_run_test() {
     0,
     pm.testPermissionFromPrincipal(principal, "test/expiration-session-exp2")
   );
+  Assert.equal(0, pm.getAllWithTypePrefix("test/expiration-perm-exp2").length);
+  Assert.equal(
+    0,
+    pm.getAllWithTypePrefix("test/expiration-session-exp2").length
+  );
+
+  Assert.equal(3, pm.getAllForPrincipal(principal).length);
 
   // Check that .getPermission returns a matching result
   Assert.equal(

toolkit/components/antitracking/test/xpcshell/test_purge_trackers.js

@@ -14,6 +14,7 @@ const { SiteDataTestUtils } = ChromeUtils.import(
 const { PermissionTestUtils } = ChromeUtils.import(
   "resource://testing-common/PermissionTestUtils.jsm"
 );
+const { setTimeout } = ChromeUtils.import("resource://gre/modules/Timer.jsm");
 
 XPCOMUtils.defineLazyServiceGetter(
   this,
@@ -231,3 +232,62 @@ add_task(async function() {
 
   UrlClassifierTestUtils.cleanupTestTrackers();
 });
+
+/**
+ * Test that we correctly delete cookies and storage for sites
+ * with an expired interaction permission.
+ */
+add_task(async function() {
+  await UrlClassifierTestUtils.addTestTrackers();
+
+  PermissionTestUtils.add(
+    TRACKING_PAGE,
+    "storageAccessAPI",
+    Services.perms.ALLOW_ACTION,
+    Services.perms.EXPIRE_TIME,
+    Date.now() + 500
+  );
+
+  SiteDataTestUtils.addToLocalStorage(TRACKING_PAGE);
+  SiteDataTestUtils.addToCookies(TRACKING_PAGE);
+  await SiteDataTestUtils.addToIndexedDB(TRACKING_PAGE);
+
+  // Purge while storage access permission exists.
+  await PurgeTrackerService.purgeTrackingCookieJars();
+
+  ok(
+    SiteDataTestUtils.hasCookies(TRACKING_PAGE),
+    "cookie remains while storage access permission exists."
+  );
+  ok(
+    SiteDataTestUtils.hasLocalStorage(TRACKING_PAGE),
+    "localStorage should not have been removed while storage access permission exists."
+  );
+  Assert.greater(
+    await SiteDataTestUtils.getQuotaUsage(TRACKING_PAGE),
+    0,
+    `We have data for ${TRACKING_PAGE}`
+  );
+
+  // Run purge after storage access permission has been removed.
+  // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+  await new Promise(c => setTimeout(c, 500));
+  await PurgeTrackerService.purgeTrackingCookieJars();
+
+  // Cookie should have been removed.
+  ok(
+    !SiteDataTestUtils.hasCookies(TRACKING_PAGE),
+    "cookie is removed after purge with no storage access permission."
+  );
+  ok(
+    !SiteDataTestUtils.hasLocalStorage(TRACKING_PAGE),
+    "localStorage should not have been removed while storage access permission exists."
+  );
+  Assert.equal(
+    await SiteDataTestUtils.getQuotaUsage(TRACKING_PAGE),
+    0,
+    "quota storage was deleted"
+  );
+
+  UrlClassifierTestUtils.cleanupTestTrackers();
+});