diff --git a/extensions/permissions/PermissionManager.cpp b/extensions/permissions/PermissionManager.cpp index 31df4e06ca66..fc1eed52f6a0 100644 --- a/extensions/permissions/PermissionManager.cpp +++ b/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. diff --git a/extensions/permissions/PermissionManager.h b/extensions/permissions/PermissionManager.h index 09ba99032779..6dda8e27f605 100644 --- a/extensions/permissions/PermissionManager.h +++ b/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 diff --git a/extensions/permissions/test/unit/test_permmanager_expiration.js b/extensions/permissions/test/unit/test_permmanager_expiration.js index e249cb94ee73..5b043a2ba2e1 100644 --- a/extensions/permissions/test/unit/test_permmanager_expiration.js +++ b/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( diff --git a/toolkit/components/antitracking/test/xpcshell/test_purge_trackers.js b/toolkit/components/antitracking/test/xpcshell/test_purge_trackers.js index 83bc3e2b5a27..de0f1fefe145 100644 --- a/toolkit/components/antitracking/test/xpcshell/test_purge_trackers.js +++ b/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(); +});