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 500171 762299961b3f7d84e36def128fbb5bb4c5403f5e
parent 500170 6bd88a7285587170add932d4e8558bf5444386ec
child 500172 7fc565646f9fdfb475b2be610739c44f2cb5effb
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfrancois
bugs1499549
milestone64.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 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
+