Bug 1499549 - Ensure that we only check the corresponding Content Blocking exception list when testing whether a top-level document is on the Content Blocking allow list r=francois

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

--HG--
extra : moz-landing-system : lando

dom/base/nsDocument.cpp

@@ -13786,8 +13786,13 @@ nsIDocument::RequestStorageAccess(mozilla::ErrorResult& aRv)
       // Note: If this has returned true, the top-level document is guaranteed
       // to not be on the Content Blocking allow list.
       DebugOnly<bool> isOnAllowList = false;
+      // If we have a parent document, it has to be non-private since we verified
+      // earlier that our own document is non-private and a private document can
+      // never have a non-private document as its child.
+      MOZ_ASSERT_IF(parent, !nsContentUtils::IsInPrivateBrowsing(parent));
       MOZ_ASSERT_IF(NS_SUCCEEDED(AntiTrackingCommon::IsOnContentBlockingAllowList(
                                    parent->GetDocumentURI(),
+                                   false,
                                    AntiTrackingCommon::eStorageChecks,
                                    isOnAllowList)),
                     !isOnAllowList);

netwerk/base/nsChannelClassifier.cpp

@@ -524,6 +524,7 @@ nsChannelClassifier::ShouldEnableTrackingProtectionInternal(
     }
 
     rv = AntiTrackingCommon::IsOnContentBlockingAllowList(topWinURI,
+                                                          NS_UsePrivateBrowsing(aChannel),
                                                           aAnnotationsOnly ?
                                                             AntiTrackingCommon::eTrackingAnnotations :
                                                             AntiTrackingCommon::eTrackingProtection,

toolkit/components/antitracking/AntiTrackingCommon.cpp

@@ -11,6 +11,7 @@
 #include "mozilla/AbstractThread.h"
 #include "mozilla/IntegerPrintfMacros.h"
 #include "mozilla/Logging.h"
+#include "mozilla/Pair.h"
 #include "mozilla/StaticPrefs.h"
 #include "mozIThirdPartyUtil.h"
 #include "nsContentUtils.h"
@@ -152,11 +153,11 @@ CookiesBehavior(nsIPrincipal* aPrincipal)
 }
 
 bool
-CheckContentBlockingAllowList(nsIURI* aTopWinURI)
+CheckContentBlockingAllowList(nsIURI* aTopWinURI, bool aIsPrivateBrowsing)
 {
   bool isAllowed = false;
   nsresult rv =
-    AntiTrackingCommon::IsOnContentBlockingAllowList(aTopWinURI,
+    AntiTrackingCommon::IsOnContentBlockingAllowList(aTopWinURI, aIsPrivateBrowsing,
                                                      AntiTrackingCommon::eStorageChecks,
                                                      isAllowed);
   if (NS_SUCCEEDED(rv) && isAllowed) {
@@ -177,7 +178,9 @@ CheckContentBlockingAllowList(nsPIDOMWindowInner* aWindow)
   nsPIDOMWindowOuter* top = aWindow->GetScriptableTop();
   if (top) {
     nsIURI* topWinURI = top->GetDocumentURI();
-    return CheckContentBlockingAllowList(topWinURI);
+    nsIDocument* doc = top->GetExtantDoc();
+    bool isPrivateBrowsing = doc ? nsContentUtils::IsInPrivateBrowsing(doc) : false;
+    return CheckContentBlockingAllowList(topWinURI, isPrivateBrowsing);
   }
 
   LOG(("Could not check the content blocking allow list because the top "
@@ -193,7 +196,8 @@ CheckContentBlockingAllowList(nsIHttpChannel* aChannel)
     nsCOMPtr<nsIURI> topWinURI;
     nsresult rv = chan->GetTopWindowURI(getter_AddRefs(topWinURI));
     if (NS_SUCCEEDED(rv)) {
-      return CheckContentBlockingAllowList(topWinURI);
+      return CheckContentBlockingAllowList(topWinURI,
+                                           NS_UsePrivateBrowsing(aChannel));
     }
   }
 
@@ -1085,6 +1089,7 @@ AntiTrackingCommon::MaybeIsFirstPartyStorageAccessGrantedFor(nsPIDOMWindowInner*
 
 nsresult
 AntiTrackingCommon::IsOnContentBlockingAllowList(nsIURI* aTopWinURI,
+  bool aIsPrivateBrowsing,
   AntiTrackingCommon::ContentBlockingAllowListPurpose aPurpose,
   bool& aIsAllowListed)
 {
@@ -1133,19 +1138,23 @@ AntiTrackingCommon::IsOnContentBlockingAllowList(nsIURI* aTopWinURI,
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Check both the normal mode and private browsing mode user override permissions.
-  const char* types[] = {
-    "trackingprotection",
-    "trackingprotection-pb"
+  Pair<const char*, bool> types[] = {
+    {"trackingprotection", false},
+    {"trackingprotection-pb", true}
   };
 
   for (size_t i = 0; i < ArrayLength(types); ++i) {
+    if (aIsPrivateBrowsing != types[i].second()) {
+      continue;
+    }
+
     uint32_t permissions = nsIPermissionManager::UNKNOWN_ACTION;
-    rv = permMgr->TestPermission(topWinURI, types[i], &permissions);
+    rv = permMgr->TestPermission(topWinURI, types[i].first(), &permissions);
     NS_ENSURE_SUCCESS(rv, rv);
 
     if (permissions == nsIPermissionManager::ALLOW_ACTION) {
       aIsAllowListed = true;
-      LOG_SPEC(("Found user override type %s for %s", types[i], _spec),
+      LOG_SPEC(("Found user override type %s for %s", types[i].first(), _spec),
                topWinURI);
       // Stop checking the next permisson type if we decided to override.
       break;

toolkit/components/antitracking/AntiTrackingCommon.h

@@ -138,6 +138,7 @@ public:
   // Check whether a top window URI is on the content blocking allow list.
   static nsresult
   IsOnContentBlockingAllowList(nsIURI* aTopWinURI,
+                               bool aIsPrivateBrowsing,
                                ContentBlockingAllowListPurpose aPurpose,
                                bool& aIsAllowListed);
 

toolkit/components/antitracking/test/browser/browser.ini

@@ -15,6 +15,7 @@ support-files =
   popup.html
   storageAccessAPIHelpers.js
 
+[browser_allowListSeparationInPrivateAndNormalWindows.js]
 [browser_backgroundImageAssertion.js]
 [browser_blockingCookies.js]
 support-files = server.sjs

toolkit/components/antitracking/test/browser/browser_allowListSeparationInPrivateAndNormalWindows.js

@@ -0,0 +1,47 @@
+ChromeUtils.import("resource://gre/modules/Services.jsm");
+
+// This test works by setting up an exception for the private window allow list
+// manually, and it then expects to see some blocking notifications (note the
+// document.cookie setter in the blocking callback.)
+// If the exception lists aren't handled separately, we'd get confused and put
+// the pages loaded under this test in the allow list, which would result in
+// the test not passing because no blocking notifications would be observed.
+
+// Testing the reverse case would also be interesting, but unfortunately there
+// isn't a super easy way to do that with our antitracking test framework since
+// private windows wouldn't send any blocking notifications as they don't have
+// storage access in the first place.
+
+add_task(async _ => {
+  let uri = Services.io.newURI("https://example.net");
+  Services.perms.add(uri, "trackingprotection-pb",
+                     Services.perms.ALLOW_ACTION);
+
+  registerCleanupFunction(_ => {
+    Services.perms.removeAll();
+  });
+});
+
+AntiTracking.runTest("Test that we don't honour a private allow list exception in a normal window",
+  // Blocking callback
+  async _ => {
+    document.cookie = "name=value";
+  },
+
+  // Non blocking callback
+  async _ => {
+    // Nothing to do here.
+  },
+
+  // Cleanup callback
+  async _ => {
+    await new Promise(resolve => {
+      Services.clearData.deleteData(Ci.nsIClearDataService.CLEAR_ALL, value => resolve());
+    });
+  },
+  null, // no extra prefs
+  false, // run the window.open() test
+  false, // run the user interaction test
+  true, // expect blocking notifications
+  false); // run in a normal window
+