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

lib/contentPolicy.js

Index: lib/contentPolicy.js
===================================================================
--- a/lib/contentPolicy.js
+++ b/lib/contentPolicy.js
@@ -162,6 +162,7 @@
     let wndLocation = originWindow.location.href;
     let docDomain = getHostname(wndLocation);
     let match = null;
+    let [sitekey, sitekeyWnd] = getSitekey(wnd);
     if (!match && Prefs.enabled)
     {
       let testWnd = wnd;
@@ -170,29 +171,7 @@
       {
         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)
         {
@@ -204,7 +183,11 @@
         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.

       }
     }
 
@@ -260,7 +243,7 @@
 
     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);
@@ -297,10 +280,11 @@
   /**
    * 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;
@@ -318,7 +302,7 @@
     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);
   },
 
@@ -329,10 +313,12 @@
    */
   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.
    */
@@ -691,6 +677,50 @@
 }
 
 /**
+ * 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