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

lib/filterClasses.js

Index: lib/filterClasses.js
===================================================================
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -20,6 +20,7 @@
  */
 
 let {FilterNotifier} = require("filterNotifier");
+let {Utils} = require("utils");

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

This is a pretty self-contained module, it shouldn't have a dependency on
utils.js.

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

Done.

 
 /**
  * Abstract base class for filters
@@ -216,14 +217,16 @@
  * Abstract base class for filters that can get hits
  * @param {String} text see Filter()
  * @param {String} domains  (optional) Domains that the filter is restricted to separated by domainSeparator e.g. "foo.com|bar.com|~baz.com"
+ * @param {String} siteKeys (optional) Public keys of websites that this filter should apply to, e.g. "foo|bar|~baz"

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

I don't really see how exceptions are useful here, original implementation
didn't have any. Keep it simple?

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

The issue stated that $sitekey should be handled exactly the same way as the
domain option. However, if you don't see any concerns in regard to backwards
compatibility when implementing it at a later point if needed, I don't mind
removing it.

  * @constructor
  * @augments Filter
  */
-function ActiveFilter(text, domains)
+function ActiveFilter(text, domains, siteKeys)
 {
   Filter.call(this, text);
 
   this.domainSource = domains;
+  this.siteKeySource = siteKeys;

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

Done.

 }
 exports.ActiveFilter = ActiveFilter;
 
@@ -381,6 +384,66 @@
   },
 
   /**
+   * String that the siteKey property should be generated from
+   * @type String
+   */
+  siteKeySource: null,
+
+  /**
+   * Map containing public keys of websites that this filter should apply to
+   * @type Object
+   */
+  get siteKeys()
+  {
+    let siteKeys = null;
+
+    if (this.siteKeySource)
+    {
+      let source = this.siteKeySource;
+      let list = source.split("|");
+      if (list.length == 1 && list[0][0] != "~")
+      {
+        // Fast track for the common one-sitekey scenario
+        siteKeys = {__proto__: null, "": false};
+        siteKeys[list[0]] = true;
+      }
+      else
+      {
+        let hasIncludes = false;
+        for (let i = 0; i < list.length; i++)
+        {
+          let siteKey = list[i];
+          if (siteKey == "")
+            continue;
+
+          let include;
+          if (siteKey[0] == "~")
+          {
+            include = false;
+            siteKey = siteKey.substr(1);
+          }
+          else
+          {
+            include = true;
+            hasIncludes = true;
+          }
+
+          if (!siteKeys)
+            siteKeys = Object.create(null);
+
+          siteKeys[siteKey] = include;
+        }
+        siteKeys[""] = !hasIncludes;
+      }
+
+      this.siteKeySource = null;
+    }
+
+    Object.defineProperty(this, "siteKeys", {value: siteKeys, enumerable: true});
+    return this.siteKeys;
+  },
+
+  /**
    * Checks whether this filter is active on a domain.
    */
   isActiveOnDomain: function(/**String*/ docDomain) /**Boolean*/
@@ -430,6 +493,44 @@
   },
 
   /**
+   * Checks whether the provided site key is valid and active on a page
+   */
+  isActiveOnPage: function(/**String*/ docDomain, /**String*/ docLocation, /**String*/ siteKey) /**Boolean*/

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

I don't think that this is the right place to perform sitekey validation, by the
time the matcher is called the sitekey should already be validated. This also
means that you don't need to pass document URL around everywhere and no new
function, you can simply extend isActiveOnDomain() in RegExpFilter to consider
one more parameter.

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

Done.

+  {
+    // If no site keys are set the rule matches everywhere
+    if (!this.siteKeys)
+      return true;
+
+    // If the document has not provided a sitekey, match only if the filter isn't restricted by sitekeys
+    if (!docDomain || !siteKey)
+      return this.siteKeys[""];
+
+    // If the document has provided an invalid sitekey the filter should not match
+    if (siteKey.indexOf("_") < 0)
+      return false;
+
+    let [key, signature] = siteKey.split("_", 2);
+    let formattedKey = key.replace(/=/g, "").toUpperCase();
+    if (!(formattedKey in this.siteKeys) || !Utils.crypto)
+      return this.siteKeys[""];
+
+    // Website specifies a key that we know but is the signature valid?
+    let uri = Services.io.newURI(docLocation, 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 this.siteKeys[formattedKey];
+
+    return !this.siteKeys[formattedKey];
+  },
+
+  /**
    * See Filter.serialize()
    */
   serialize: function(buffer)
@@ -455,12 +556,13 @@
  * @param {Boolean} matchCase   (optional) Defines whether the filter should distinguish between lower and upper case letters
  * @param {String} domains      (optional) Domains that the filter is restricted to, e.g. "foo.com|bar.com|~baz.com"
  * @param {Boolean} thirdParty  (optional) Defines whether the filter should apply to third-party or first-party content only
+ * @param {String} siteKeys     (optional) Public keys of websites that this filter should apply to, e.g. "foo|bar|~baz"
  * @constructor
  * @augments ActiveFilter
  */
-function RegExpFilter(text, regexpSource, contentType, matchCase, domains, thirdParty)
+function RegExpFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, siteKeys)
 {
-  ActiveFilter.call(this, text, domains);
+  ActiveFilter.call(this, text, domains, siteKeys);
 
   if (contentType != null)
     this.contentType = contentType;
@@ -554,14 +656,17 @@
    * @param {String} contentType content type identifier of the URL
    * @param {String} docDomain domain name of the document that loads the URL
    * @param {Boolean} thirdParty should be true if the URL is a third-party request
+   * @param {String} docLocation location of the document that loads the URL
+   * @param {String} siteKey public key provided by the document
    * @return {Boolean} true in case of a match
    */
-  matches: function(location, contentType, docDomain, thirdParty)
+  matches: function(location, contentType, docDomain, thirdParty, docLocation, siteKey)
   {
     if (this.regexp.test(location) &&
         (RegExpFilter.typeMap[contentType] & this.contentType) != 0 &&
         (this.thirdParty == null || this.thirdParty == thirdParty) &&
-        this.isActiveOnDomain(docDomain))
+        this.isActiveOnDomain(docDomain) &&
+        this.isActiveOnPage(docDomain, docLocation, siteKey))
     {
       return true;
     }
@@ -639,7 +744,7 @@
       else if (option == "~COLLAPSE")
         collapse = false;
       else if (option == "SITEKEY" && typeof value != "undefined")
-        siteKeys = value.split(/\|/);
+        siteKeys = value;
       else
         return new InvalidFilter(origText, "Unknown option " + option.toLowerCase());
     }
@@ -653,13 +758,11 @@
       contentType = RegExpFilter.prototype.contentType;
     contentType &= ~RegExpFilter.typeMap.DOCUMENT;
   }
-  if (!blocking && siteKeys)
-    contentType = RegExpFilter.typeMap.DOCUMENT;
 
   try
   {
     if (blocking)
-      return new BlockingFilter(origText, text, contentType, matchCase, domains, thirdParty, collapse);
+      return new BlockingFilter(origText, text, contentType, matchCase, domains, thirdParty, siteKeys, collapse);
     else
       return new WhitelistFilter(origText, text, contentType, matchCase, domains, thirdParty, siteKeys);
   }
@@ -705,13 +808,14 @@
  * @param {Boolean} matchCase see RegExpFilter()
  * @param {String} domains see RegExpFilter()
  * @param {Boolean} thirdParty see RegExpFilter()
+ * @param {String} siteKeys see RegExpFilter()
  * @param {Boolean} collapse  defines whether the filter should collapse blocked content, can be null
  * @constructor
  * @augments RegExpFilter
  */
-function BlockingFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, collapse)
+function BlockingFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, siteKeys, collapse)
 {
-  RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains, thirdParty);
+  RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains, thirdParty, siteKeys);
 
   this.collapse = collapse;
 }
@@ -736,28 +840,19 @@
  * @param {Boolean} matchCase see RegExpFilter()
  * @param {String} domains see RegExpFilter()
  * @param {Boolean} thirdParty see RegExpFilter()
- * @param {String[]} siteKeys public keys of websites that this filter should apply to
+ * @param {String} siteKeys see RegExpFilter()
  * @constructor
  * @augments RegExpFilter
  */
 function WhitelistFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, siteKeys)
 {
-  RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains, thirdParty);
-
-  if (siteKeys != null)
-    this.siteKeys = siteKeys;
+  RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains, thirdParty, siteKeys);
 }
 exports.WhitelistFilter = WhitelistFilter;
 
 WhitelistFilter.prototype =
 {
-  __proto__: RegExpFilter.prototype,
-
-  /**
-   * List of public keys of websites that this filter should apply to
-   * @type String[]
-   */
-  siteKeys: null
+  __proto__: RegExpFilter.prototype
 }
 
 /**