Issue 431/432 - Remove special handling for the $sitekey option

lib/contentPolicy.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 152 matching lines @@
     let docDomain = getHostname(wndLocation);
     let match = null;
     if (!match && Prefs.enabled)
     {
       let testWnd = wnd;
       let parentWndLocation = getWindowLocation(testWnd);
       while (true)
       {
         let testWndLocation = parentWndLocation;
         parentWndLocation = (testWnd == testWnd.parent ? testWndLocation : getWindowLocation(testWnd.parent));
-        match = Policy.isWhitelisted(testWndLocation, parentWndLocation);
 
-        if (!(match instanceof WhitelistFilter))
-        {
-          let keydata = (testWnd.document && testWnd.document.documentElement ? testWnd.document.documentElement.getAttribute("data-adblockkey") : null);
-          if (keydata && keydata.indexOf("_") >= 0)
-          {
-            let [key, signature] = keydata.split("_", 2);
-            let keyMatch = defaultMatcher.matchesByKey(testWndLocation, key.replace(/=/g, ""), docDomain);
-            if (keyMatch && Utils.crypto)
-            {
-              // Website specifies a key that we know but is the signature valid?
-              let uri = Services.io.newURI(testWndLocation, null, null);
-              let params = [
-                uri.path.replace(/#.*/, ""),  // REQUEST_URI
-                uri.asciiHost,                // HTTP_HOST
-                Utils.httpProtocol.userAgent  // HTTP_USER_AGENT
-              ];
-              if (Utils.verifySignature(key, signature, params.join("\0")))
-                match = keyMatch;
-            }
-          }
-        }
+        let siteKey = getSiteKey(node, testWnd);
+        match = Policy.isWhitelisted(testWndLocation, parentWndLocation, siteKey);

[Wladimir Palant, 2014/08/01 10:50:21]

The logic here seems wrong to me. I've extended the description in
https://issues.adblockplus.org/ticket/431 to explain what I mean - the sitekey
shouldn't just apply to the current frame but also all frames below it.

What this boils down IMHO:

  let [sitekey, sitekeyWnd] = getSitekey(wnd);

This function should walk up from wnd, retrieve the sitekeys and validate them.
If a valid one is found it should be returned along with the window it belongs
to.

The whitelisting check then should create its own copies of these variables and
call getSitekey() again whenever it arrives at the frame belonging to the
sitekey (only after processing this frame and only if it isn't the top frame
already of course).

Note that this approach also makes sure the sitekey isn't retrieved twice
(currently you do it for the whitelisting check and actual request check).

[Thomas Greiner, 2014/08/11 17:18:15]

Done.

 
         if (match instanceof WhitelistFilter)
         {
           FilterStorage.increaseHitCount(match, wnd);
           RequestNotifier.addNodeData(testWnd.document, topWnd, Policy.type.DOCUMENT, getHostname(parentWndLocation), false, testWndLocation, match);
           return true;
         }
 
         if (testWnd.parent == testWnd)
           break;
@@ skipping 47 matching lines @@
         FilterStorage.increaseHitCount(exception, wnd);
         RequestNotifier.addNodeData(node, topWnd, contentType, docDomain, thirdParty, locationText, exception);
         return true;
       }
     }
 
     let thirdParty = (contentType == Policy.type.ELEMHIDE ? false : isThirdParty(location, docDomain));
 
     if (!match && Prefs.enabled)
     {
-      match = defaultMatcher.matchesAny(locationText, Policy.typeDescr[contentType] || "", docDomain, thirdParty);
+      let docLocation = getWindowLocation(wnd);
+      let siteKey = getSiteKey(node, wnd);
+      match = defaultMatcher.matchesAny(locationText, Policy.typeDescr[contentType] || "", docDomain, thirdParty, docLocation, siteKey);
       if (match instanceof BlockingFilter && node.ownerDocument && !(contentType in Policy.nonVisual))
       {
         let prefCollapse = (match.collapse != null ? match.collapse : !Prefs.fastcollapse);
         if (collapse || prefCollapse)
           schedulePostProcess(node);
       }
 
       // Track mouse events for objects
       if (!match && contentType == Policy.type.OBJECT && node.nodeType == Ci.nsIDOMNode.ELEMENT_NODE)
       {
@@ skipping 16 matching lines @@
    * @return {Boolean}
    */
   isBlockableScheme: function(location)
   {
     return !(location.scheme in Policy.whitelistSchemes);
   },
 
   /**
    * Checks whether a page is whitelisted.
    * @param {String} url
-   * @param {String} [parentUrl] location of the parent page
+   * @param {String} parentUrl location of the parent page
+   * @param {String} siteKey public key provided on the page
    * @return {Filter} filter that matched the URL or null if not whitelisted
    */
-  isWhitelisted: function(url, parentUrl)
+  isWhitelisted: function(url, parentUrl, siteKey)

[Wladimir Palant, 2014/08/01 10:50:21]

If you change the parameters here you probably want to adjust the other callers:
isWindowWhitelisted() here, appSupport.js, ui.js.

[Thomas Greiner, 2014/08/11 17:18:15]

Done.

   {
     if (!url)
       return null;
 
     // Do not apply exception rules to schemes on our whitelistschemes list.
     let match = /^([\w\-]+):/.exec(url);
     if (match && match[1] in Policy.whitelistSchemes)
       return null;
 
     if (!parentUrl)
       parentUrl = url;

[Wladimir Palant, 2014/08/01 10:50:21]

If parentUrl is no longer optional then this code should be unnecessary.

[Thomas Greiner, 2014/08/11 17:18:15]

Technically, it's still optional since it's only required when a sitekey was
passed. I added the "optional" keyword to the parameter description.

 
     // Ignore fragment identifier
     let index = url.indexOf("#");
     if (index >= 0)
       url = url.substring(0, index);
 
-    let result = defaultMatcher.matchesAny(url, "DOCUMENT", getHostname(parentUrl), false);
+    let result = defaultMatcher.matchesAny(url, "DOCUMENT", getHostname(parentUrl), false, parentUrl, siteKey);
     return (result instanceof WhitelistFilter ? result : null);
   },
 
   /**
    * Checks whether the page loaded in a window is whitelisted.
    * @param wnd {nsIDOMWindow}
    * @return {Filter} matching exception rule or null if not whitelisted
    */
   isWindowWhitelisted: function(wnd)
   {
@@ skipping 352 matching lines @@
   {
     return Utils.unwrapURL(url).host;
   }
   catch(e)
   {
     return null;
   }
 }
 
 /**
+ * Retrieves the sitekey of a window.
+ */
+function getSiteKey(node, wnd)

[Wladimir Palant, 2014/08/01 10:50:21]

Why pass in node parameter that isn't used?

[Thomas Greiner, 2014/08/11 17:18:15]

Done.

+{
+  if (wnd.document && wnd.document.documentElement)
+    return wnd.document.documentElement.getAttribute("data-adblockkey");
+
+  return null;
+}
+
+/**
  * Retrieves the location of a window.
  * @param wnd {nsIDOMWindow}
  * @return {String} window location or null on failure
  */
 function getWindowLocation(wnd)
 {
   if ("name" in wnd && wnd.name == "messagepane")
   {
     // Thunderbird branch
     try
@@ skipping 61 matching lines @@
   if (!wnd || wnd.closed)
     return;
 
   if (entry.type == Policy.type.OBJECT)
   {
     node.removeEventListener("mouseover", objectMouseEventHander, true);
     node.removeEventListener("mouseout", objectMouseEventHander, true);
   }
   Policy.processNode(wnd, node, entry.type, Utils.makeURI(entry.location), true);
 }