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 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;
-            }
-          }
-        }
+        match = Policy.isWhitelisted(testWndLocation, parentWndLocation, sitekey);
 
         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
+        {
+          if (testWnd == sitekeyWnd)
+            [sitekey, sitekeyWnd] = getSitekey(testWnd.parent);
           testWnd = testWnd.parent;
+        }

[Wladimir Palant, 2014/08/14 19:58:15]

Nit: I'd put these lines without an |else| block here, it doesn't look right
with your changes.

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

       }
     }
 
     // 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)
     {
-      match = defaultMatcher.matchesAny(locationText, Policy.typeDescr[contentType] || "", docDomain, thirdParty);
+      match = defaultMatcher.matchesAny(locationText, Policy.typeDescr[contentType] || "", docDomain, thirdParty, sitekey);

[Wladimir Palant, 2014/08/14 19:58:15]

This is the sitekey of the top-level window, not the one of the frame making the
request - you overwrote it above.

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

       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} (optional) parentUrl location of the parent page
+   * @param {String} (optional) sitekey public key provided on the page

[Wladimir Palant, 2014/08/14 19:58:15]

Please see
https://code.google.com/p/jsdoc-toolkit/wiki/TagParam#Optional_Parameters for
the correct syntax.

[Thomas Greiner, 2014/08/29 15:57:29]

Done. I was using the syntax used in lib/filterClasses.js so I changed it there
as well.

    * @return {Filter} filter that matched the URL or null if not whitelisted
    */
-  isWhitelisted: function(url, parentUrl)
+  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;
 
     // 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, 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)
   {
-    return Policy.isWhitelisted(getWindowLocation(wnd));
+    let url = getWindowLocation(wnd);
+    let parentUrl = (wnd.parent) ? getWindowLocation(wnd.parent) : url;
+    let [sitekey, sitekeyWnd] = getSitekey(wnd);
+    return Policy.isWhitelisted(url, parentUrl, sitekey);

[Wladimir Palant, 2014/08/14 19:58:15]

This function is meant to be used from the UI, its result should match what we
show for the "whitelisted" state - meaning no parent URLs (always a top-level
window) and no sitekeys. So please revert it to the original state.

Note that the JSDoc documentation can certainly be improved here. Also, the
implementation of getWindowLocation() that this relies on isn't quite correct
(application-specific branching should be moved into appSupport.js, missing
Utils.unwrapURL() call). Might be a good idea to create follow-up issues to
clean this up.

[Thomas Greiner, 2014/08/29 15:57:29]

Done. I'll create an issue for that.

   },
 
-
   /**
    * 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;
 
     for (let node of nodes)
@@ skipping 338 matching lines @@
   {
     return Utils.unwrapURL(url).host;
   }
   catch(e)
   {
     return null;
   }
 }
 
 /**
+ * Retrieves the sitekey of a window.
+ */
+function getSitekey(wnd)
+{
+  let sitekey = null;
+
+  if (Utils.crypto)

[Wladimir Palant, 2014/08/14 19:58:15]

This is a pointless assumption about the inner workings of verifySignature().
Given that we stopped supporting Gecko 1.9.2 a long time ago, Utils.crypto
should always be set unless something is badly wrong - nothing to optimize here.

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

+  {
+    while (wnd)

[Wladimir Palant, 2014/08/14 19:58:15]

This is misleading: we don't expect wnd to ever become null. I'd rather have
|while (true)| here as in the original code, to indicate hat this isn't a real
break condition. Alternatively you end the loop with:

  wnd = (wnd !== wnd.parent ? wnd.parent : null);

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

+    {
+      if (wnd.document && wnd.document.documentElement)
+      {
+        let foundKey = wnd.document.documentElement.getAttribute("data-adblockkey");
+        if (foundKey && foundKey.indexOf("_") > - 1)

[Wladimir Palant, 2014/08/14 19:58:15]

The original code uses ">= 0" here, why change it into "> -1"? And there
definitely should be no space between - and 1.

Besides, why was this code changed at all? You moved it so now I have to compare
all of it line by line. The changes seem pointless, I don't consider foundKey a
better variable name than keydata - it sounds like a boolean flag but isn't.

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

+        {
+          let [key, signature] = foundKey.split("_", 2);
+
+          // 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")))
+            sitekey = foundKey;

[Wladimir Palant, 2014/08/14 19:58:15]

The current code will ignore a valid key further up in the hierarchy if an
invalid one is found. How about |return [key, wnd];| here insteadof breaking out
of the look below.

Note that only the key part should be returned, the signature is irrelevant once
it has been validated.

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

+        }
+        break;
+      }
+
+      if (wnd === wnd.top)

[Wladimir Palant, 2014/08/14 19:58:15]

I would compare to the parent here, as in the original code - the parent is what
we care about, to avoid an endless loop. Comparing to wnd.top unnecessarily adds
the assumption that wnd.parent === wnd can only be true for the top window.

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

+        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")
   {
     // 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);
 }