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
authorEhsan Akhgari <ehsan@mozilla.com>
Wed, 17 Oct 2018 17:06:00 +0000
changeset 490099 762299961b3f7d84e36def128fbb5bb4c5403f5e
parent 490098 6bd88a7285587170add932d4e8558bf5444386ec
child 490100 7fc565646f9fdfb475b2be610739c44f2cb5effb
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersfrancois
bugs1499549
milestone64.0a1
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
dom/base/nsDocument.cpp
netwerk/base/nsChannelClassifier.cpp
toolkit/components/antitracking/AntiTrackingCommon.cpp
toolkit/components/antitracking/AntiTrackingCommon.h
toolkit/components/antitracking/test/browser/browser.ini
toolkit/components/antitracking/test/browser/browser_allowListSeparationInPrivateAndNormalWindows.js
--- a/dom/base/nsDocument.cpp
+++ b/dom/base/nsDocument.cpp
@@ -13781,18 +13781,23 @@ nsIDocument::RequestStorageAccess(mozill
   if (AntiTrackingCommon::ShouldHonorContentBlockingCookieRestrictions() &&
       StaticPrefs::network_cookie_cookieBehavior() ==
         nsICookieService::BEHAVIOR_REJECT_TRACKER) {
     // Only do something special for third-party tracking content.
     if (nsContentUtils::StorageDisabledByAntiTracking(this, nullptr)) {
       // 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);
 
       isTrackingWindow = true;
       // TODO: prompt for permission
     }
   }
--- a/netwerk/base/nsChannelClassifier.cpp
+++ b/netwerk/base/nsChannelClassifier.cpp
@@ -519,16 +519,17 @@ nsChannelClassifier::ShouldEnableTrackin
     if (!topWinURI && CachedPrefs::GetInstance()->IsAllowListExample()) {
       LOG(("nsChannelClassifier[%p]: Allowlisting test domain\n", this));
       rv = ios->NewURI(NS_LITERAL_CSTRING("http://allowlisted.example.com"),
                        nullptr, nullptr, getter_AddRefs(topWinURI));
       NS_ENSURE_SUCCESS(rv, rv);
     }
 
     rv = AntiTrackingCommon::IsOnContentBlockingAllowList(topWinURI,
+                                                          NS_UsePrivateBrowsing(aChannel),
                                                           aAnnotationsOnly ?
                                                             AntiTrackingCommon::eTrackingAnnotations :
                                                             AntiTrackingCommon::eTrackingProtection,
                                                           mIsAllowListed);
     if (NS_FAILED(rv)) {
       return rv; // normal for some loads, no need to print a warning
     }
 
--- a/toolkit/components/antitracking/AntiTrackingCommon.cpp
+++ b/toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ -6,16 +6,17 @@
 
 #include "AntiTrackingCommon.h"
 
 #include "mozilla/dom/ContentChild.h"
 #include "mozilla/ipc/MessageChannel.h"
 #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"
 #include "nsGlobalWindowInner.h"
 #include "nsCookiePermission.h"
 #include "nsICookieService.h"
 #include "nsIDocShell.h"
 #include "nsIHttpChannelInternal.h"
@@ -147,21 +148,21 @@ CookiesBehavior(nsIPrincipal* aPrincipal
   if (BasePrincipal::Cast(aPrincipal)->AddonPolicy()) {
     return nsICookieService::BEHAVIOR_ACCEPT;
   }
 
   return StaticPrefs::network_cookie_cookieBehavior();
 }
 
 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) {
     LOG_SPEC(("The top-level window (%s) is on the content blocking allow list, "
               "bail out early", _spec), aTopWinURI);
     return true;
   }
   if (NS_FAILED(rv)) {
@@ -172,33 +173,36 @@ CheckContentBlockingAllowList(nsIURI* aT
 }
 
 bool
 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 "
        "window wasn't accessible"));
   return false;
 }
 
 bool
 CheckContentBlockingAllowList(nsIHttpChannel* aChannel)
 {
   nsCOMPtr<nsIHttpChannelInternal> chan = do_QueryInterface(aChannel);
   if (chan) {
     nsCOMPtr<nsIURI> topWinURI;
     nsresult rv = chan->GetTopWindowURI(getter_AddRefs(topWinURI));
     if (NS_SUCCEEDED(rv)) {
-      return CheckContentBlockingAllowList(topWinURI);
+      return CheckContentBlockingAllowList(topWinURI,
+                                           NS_UsePrivateBrowsing(aChannel));
     }
   }
 
   LOG(("Could not check the content blocking allow list because the top "
        "window wasn't accessible"));
   return false;
 }
 
@@ -1080,16 +1084,17 @@ AntiTrackingCommon::MaybeIsFirstPartySto
             result == nsIPermissionManager::ALLOW_ACTION ?
               "success" : "failure"), parentPrincipalURI);
 
   return result == nsIPermissionManager::ALLOW_ACTION;
 }
 
 nsresult
 AntiTrackingCommon::IsOnContentBlockingAllowList(nsIURI* aTopWinURI,
+  bool aIsPrivateBrowsing,
   AntiTrackingCommon::ContentBlockingAllowListPurpose aPurpose,
   bool& aIsAllowListed)
 {
   aIsAllowListed = false;
 
   // For storage checks, check the storage pref, and for annotations checks,
   // check the corresponding pref as well.  This allows each set of checks to
   // be disabled individually if needed.
@@ -1128,29 +1133,33 @@ AntiTrackingCommon::IsOnContentBlockingA
   rv = ios->NewURI(escaped, nullptr, nullptr, getter_AddRefs(topWinURI));
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsCOMPtr<nsIPermissionManager> permMgr =
     do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
   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;
     }
   }
 
   if (!aIsAllowListed) {
     LOG(("No user override found"));
--- a/toolkit/components/antitracking/AntiTrackingCommon.h
+++ b/toolkit/components/antitracking/AntiTrackingCommon.h
@@ -133,16 +133,17 @@ public:
     eStorageChecks,
     eTrackingProtection,
     eTrackingAnnotations,
   };
 
   // Check whether a top window URI is on the content blocking allow list.
   static nsresult
   IsOnContentBlockingAllowList(nsIURI* aTopWinURI,
+                               bool aIsPrivateBrowsing,
                                ContentBlockingAllowListPurpose aPurpose,
                                bool& aIsAllowListed);
 
   // This method can be called on the parent process or on the content process.
   // The notification is propagated to the child channel if aChannel is a parent
   // channel proxy.
   //
   // aRejectedReason must be one of these values:
--- a/toolkit/components/antitracking/test/browser/browser.ini
+++ b/toolkit/components/antitracking/test/browser/browser.ini
@@ -10,16 +10,17 @@ support-files =
   3rdPartyUI.html
   3rdPartyWO.html
   3rdPartyOpen.html
   3rdPartyOpenUI.html
   empty.js
   popup.html
   storageAccessAPIHelpers.js
 
+[browser_allowListSeparationInPrivateAndNormalWindows.js]
 [browser_backgroundImageAssertion.js]
 [browser_blockingCookies.js]
 support-files = server.sjs
 [browser_blockingDOMCache.js]
 skip-if = (os == "win" && os_version == "6.1" && bits == 32 && !debug) # Bug 1491937
 [browser_blockingIndexedDb.js]
 [browser_blockingIndexedDbInWorkers.js]
 [browser_blockingLocalStorage.js]
new file mode 100644
--- /dev/null
+++ b/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
+