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 144 matching lines @@
   processNode: function(wnd, node, contentType, location, collapse)
   {
     let topWnd = wnd.top;
     if (!topWnd || !topWnd.location || !topWnd.location.href)
       return true;
 
     let originWindow = Utils.getOriginWindow(wnd);
     let wndLocation = originWindow.location.href;
     let docDomain = getHostname(wndLocation);
     let match = null;
+    let [sitekey, sitekeyWnd] = getSitekey(wnd);
     if (!match && Prefs.enabled)
     {
       let testWnd = wnd;
+      let testSitekey = sitekey;
+      let testSitekeyWnd = sitekeyWnd;
       let parentWndLocation = getWindowLocation(testWnd);
       while (true)
       {
         let testWndLocation = parentWndLocation;
         parentWndLocation = (testWnd == testWnd.parent ? testWndLocation : getWindowLocation(testWnd.parent));
-
-        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.

+        match = Policy.isWhitelisted(testWndLocation, parentWndLocation, testSitekey);
 
         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;
-        else
-          testWnd = testWnd.parent;
+
+        if (testWnd == testSitekeyWnd)
+          [testSitekey, testSitekeyWnd] = getSitekey(testWnd.parent);
+        testWnd = testWnd.parent;
       }
     }
 
     // Data loaded by plugins should be attached to the document
     if (contentType == Policy.type.OBJECT_SUBREQUEST && node instanceof Ci.nsIDOMElement)
       node = node.ownerDocument;
 
     // Fix type for objects misrepresented as frames or images
     if (contentType != Policy.type.OBJECT && (node instanceof Ci.nsIDOMHTMLObjectElement || node instanceof Ci.nsIDOMHTMLEmbedElement))
       contentType = Policy.type.OBJECT;
@@ skipping 35 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)
     {
-      let docLocation = getWindowLocation(wnd);
-      let siteKey = getSiteKey(node, wnd);
-      match = defaultMatcher.matchesAny(locationText, Policy.typeDescr[contentType] || "", docDomain, thirdParty, docLocation, siteKey);
+      match = defaultMatcher.matchesAny(locationText, Policy.typeDescr[contentType] || "", docDomain, thirdParty, 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} siteKey public key provided on the 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, 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.

+  isWhitelisted: function(url, parentUrl, sitekey)
   {
     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, parentUrl, siteKey);
+    let result = defaultMatcher.matchesAny(url, "DOCUMENT", getHostname(parentUrl), false, sitekey);
     return (result instanceof WhitelistFilter ? result : null);
   },
 
   /**
-   * Checks whether the page loaded in a window is whitelisted.
+   * Checks whether the page loaded in a window is whitelisted for indication in the UI.
    * @param wnd {nsIDOMWindow}
    * @return {Filter} matching exception rule or null if not whitelisted
    */
   isWindowWhitelisted: function(wnd)
   {
     return Policy.isWhitelisted(getWindowLocation(wnd));
   },
-
 
   /**
    * Asynchronously re-checks filters for given nodes.
    */
   refilterNodes: function(/**Node[]*/ nodes, /**RequestEntry*/ entry)
   {
     // Ignore nodes that have been blocked already
     if (entry.filter && !(entry.filter instanceof WhitelistFilter))
       return;
 
@@ skipping 341 matching lines @@
   }
   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;
+function getSitekey(wnd)
+{
+  let sitekey = null;
+
+  while (true)
+  {
+    if (wnd.document && wnd.document.documentElement)
+    {
+      let keydata = wnd.document.documentElement.getAttribute("data-adblockkey");
+      if (keydata && keydata.indexOf("_") >= 0)
+      {
+        let [key, signature] = keydata.split("_", 2);
+        key = key.replace(/=/g, "");
+
+        // Website specifies a key but is the signature valid?
+        let uri = Services.io.newURI(getWindowLocation(wnd), null, null);
+        let host = uri.asciiHost;
+        if (uri.port > 0)
+          host += ":" + uri.port;
+        let params = [
+          uri.path.replace(/#.*/, ""),  // REQUEST_URI
+          host,                         // HTTP_HOST
+          Utils.httpProtocol.userAgent  // HTTP_USER_AGENT
+        ];
+        if (Utils.verifySignature(key, signature, params.join("\0")))
+          return [key, wnd];
+      }
+    }
+
+    if (wnd === wnd.parent)
+      break;
+
+    wnd = wnd.parent;
+  }
+
+  return [sitekey, wnd];
 }
 
 /**
  * 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")
@@ skipping 64 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);
 }