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

lib/filterClasses.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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /**
  * @fileOverview Definition of Filter class and its subclasses.
  */
 
 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
  *
  * @param {String} text   string representation of the filter
  * @constructor
  */
 function Filter(text)
 {
   this.text = text;
@@ skipping 175 matching lines @@
 
   /**
    * See Filter.serialize()
    */
   serialize: function(buffer) {}
 };
 
 /**
  * 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.

+ * @param {String} [domains] Domains that the filter is restricted to separated by domainSeparator e.g. "foo.com|bar.com|~baz.com"
  * @constructor
  * @augments Filter
  */
-function ActiveFilter(text, domains, siteKeys)
+function ActiveFilter(text, domains)
 {
   Filter.call(this, text);
 
   this.domainSource = domains;
-  this.siteKeySource = siteKeys;

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

Done.

 }
 exports.ActiveFilter = ActiveFilter;
 
 ActiveFilter.prototype =
 {
   __proto__: Filter.prototype,
 
   _disabled: false,
   _hitCount: 0,
   _lastHit: 0,
@@ skipping 80 matching lines @@
    * @type Boolean
    */
   domainSourceIsUpperCase: false,
 
   /**
    * Map containing domains that this filter should match on/not match on or null if the filter should match on all domains
    * @type Object
    */
   get domains()
   {
+    // Despite this property being cached, the getter is called
+    // several times on Safari, due to WebKit bug 132872
+    let prop = Object.getOwnPropertyDescriptor(this, "domains");
+    if (prop)
+      return prop.value;
+
     let domains = null;
 
     if (this.domainSource)
     {
       let source = this.domainSource;
       if (!this.domainSourceIsUpperCase) {
         // RegExpFilter already have uppercase domains
         source = source.toUpperCase();
       }
       let list = source.split(this.domainSeparator);
@@ skipping 37 matching lines @@
       }
 
       this.domainSource = null;
     }
 
     Object.defineProperty(this, "domains", {value: domains, enumerable: true});
     return this.domains;
   },
 
   /**
-   * 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;
-  },
+   * Array containing public keys of websites that this filter should apply to
+   * @type Array of String
+   */
+  sitekeys: null,
 
   /**
    * Checks whether this filter is active on a domain.
-   */
-  isActiveOnDomain: function(/**String*/ docDomain) /**Boolean*/
-  {
+   * @param {String} docDomain domain name of the document that loads the URL
+   * @param {String} [sitekey] public key provided by the document
+   * @return {Boolean} true in case of the filter being active
+   */
+  isActiveOnDomain: function(docDomain, sitekey)
+  {
+    // Sitekeys are case-sensitive so we shouldn't convert them to upper-case to avoid false
+    // positives here. Instead we need to change the way filter options are parsed.
+    if (this.sitekeys && (!sitekey || this.sitekeys.indexOf(sitekey.toUpperCase()) < 0))
+      return false;
+
     // If no domains are set the rule matches everywhere
     if (!this.domains)
       return true;
 
     // If the document has no host name, match only if the filter isn't restricted to specific domains
     if (!docDomain)
       return this.domains[""];
 
     if (this.ignoreTrailingDot)
       docDomain = docDomain.replace(/\.+$/, "");
@@ skipping 22 matching lines @@
 
     if (this.ignoreTrailingDot)
       docDomain = docDomain.replace(/\.+$/, "");
     docDomain = docDomain.toUpperCase();
 
     for (let domain in this.domains)
       if (this.domains[domain] && domain != docDomain && (domain.length <= docDomain.length || domain.indexOf("." + docDomain) != domain.length - docDomain.length - 1))
         return false;
 
     return true;
-  },
-
-  /**
-   * 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)
   {
     if (this._disabled || this._hitCount || this._lastHit)
     {
       Filter.prototype.serialize.call(this, buffer);
       if (this._disabled)
         buffer.push("disabled=true");
       if (this._hitCount)
         buffer.push("hitCount=" + this._hitCount);
       if (this._lastHit)
         buffer.push("lastHit=" + this._lastHit);
     }
   }
 };
 
 /**
  * Abstract base class for RegExp-based filters
  * @param {String} text see Filter()
  * @param {String} regexpSource filter part that the regular expression should be build from
- * @param {Number} contentType  (optional) Content types the filter applies to, combination of values from RegExpFilter.typeMap
- * @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"
+ * @param {Number} [contentType] Content types the filter applies to, combination of values from RegExpFilter.typeMap
+ * @param {Boolean} [matchCase] Defines whether the filter should distinguish between lower and upper case letters
+ * @param {String} [domains] Domains that the filter is restricted to, e.g. "foo.com|bar.com|~baz.com"
+ * @param {Boolean} [thirdParty] Defines whether the filter should apply to third-party or first-party content only
+ * @param {String} [sitekeys] Public keys of websites that this filter should apply to
  * @constructor
  * @augments ActiveFilter
  */
-function RegExpFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, siteKeys)
-{
-  ActiveFilter.call(this, text, domains, siteKeys);
+function RegExpFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, sitekeys)
+{
+  ActiveFilter.call(this, text, domains, sitekeys);
 
   if (contentType != null)
     this.contentType = contentType;
   if (matchCase)
     this.matchCase = matchCase;
   if (thirdParty != null)
     this.thirdParty = thirdParty;
+  if (sitekeys != null)
+    this.sitekeySource = sitekeys;
 
   if (regexpSource.length >= 2 && regexpSource[0] == "/" && regexpSource[regexpSource.length - 1] == "/")
   {
     // The filter is a regular expression - convert it immediately to catch syntax errors
     let regexp = new RegExp(regexpSource.substr(1, regexpSource.length - 2), this.matchCase ? "" : "i");
     Object.defineProperty(this, "regexp", {value: regexp});
   }
   else
   {
     // No need to convert this filter to regular expression yet, do it on demand
@@ skipping 26 matching lines @@
    * Expression from which a regular expression should be generated - for delayed creation of the regexp property
    * @type String
    */
   regexpSource: null,
   /**
    * Regular expression to be used when testing against this filter
    * @type RegExp
    */
   get regexp()
   {
+    // Despite this property being cached, the getter is called
+    // several times on Safari, due to WebKit bug 132872
+    let prop = Object.getOwnPropertyDescriptor(this, "regexp");
+    if (prop)
+      return prop.value;
+
     // Remove multiple wildcards
     let source = this.regexpSource
       .replace(/\*+/g, "*")        // remove multiple wildcards
       .replace(/\^\|$/, "^")       // remove anchors following separator placeholder
       .replace(/\W/g, "\\$&")      // escape special symbols
       .replace(/\\\*/g, ".*")      // replace wildcards by .*
       // process separator placeholders (all ANSI characters but alphanumeric characters and _%.-)
       .replace(/\\\^/g, "(?:[\\x00-\\x24\\x26-\\x2C\\x2F\\x3A-\\x40\\x5B-\\x5E\\x60\\x7B-\\x7F]|$)")
       .replace(/^\\\|\\\|/, "^[\\w\\-]+:\\/+(?!\\/)(?:[^\\/]+\\.)?") // process extended anchor at expression start
       .replace(/^\\\|/, "^")       // process anchor at expression start
@@ skipping 15 matching lines @@
    * @type Boolean
    */
   matchCase: false,
   /**
    * Defines whether the filter should apply to third-party or first-party content only. Can be null (apply to all content).
    * @type Boolean
    */
   thirdParty: null,
 
   /**
+   * String that the sitekey property should be generated from
+   * @type String
+   */
+  sitekeySource: null,
+
+  /**
+   * Array containing public keys of websites that this filter should apply to
+   * @type Array of String
+   */
+  get sitekeys()
+  {
+    // Despite this property being cached, the getter is called
+    // several times on Safari, due to WebKit bug 132872
+    let prop = Object.getOwnPropertyDescriptor(this, "sitekeys");
+    if (prop)
+      return prop.value;
+
+    let sitekeys = null;
+
+    if (this.sitekeySource)
+    {
+      sitekeys = this.sitekeySource.split("|");
+      this.sitekeySource = null;
+    }
+
+    Object.defineProperty(this, "sitekeys", {value: sitekeys, enumerable: true});
+    return this.sitekeys;
+  },
+
+  /**
    * Tests whether the URL matches this filter
    * @param {String} location URL to be tested
    * @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
+   * @param {String} sitekey public key provided by the document
    * @return {Boolean} true in case of a match
    */
-  matches: function(location, contentType, docDomain, thirdParty, docLocation, siteKey)
+  matches: function(location, contentType, docDomain, thirdParty, sitekey)
   {
     if (this.regexp.test(location) &&
         (RegExpFilter.typeMap[contentType] & this.contentType) != 0 &&
         (this.thirdParty == null || this.thirdParty == thirdParty) &&
-        this.isActiveOnDomain(docDomain) &&
-        this.isActiveOnPage(docDomain, docLocation, siteKey))
+        this.isActiveOnDomain(docDomain, sitekey))
     {
       return true;
     }
 
     return false;
   }
 };
 
 // Required to optimize Matcher, see also RegExpFilter.prototype.length
 Object.defineProperty(RegExpFilter.prototype, "0",
@@ skipping 11 matching lines @@
   let origText = text;
   if (text.indexOf("@@") == 0)
   {
     blocking = false;
     text = text.substr(2);
   }
 
   let contentType = null;
   let matchCase = null;
   let domains = null;
-  let siteKeys = null;
+  let sitekeys = null;
   let thirdParty = null;
   let collapse = null;
   let options;
   let match = (text.indexOf("$") >= 0 ? Filter.optionsRegExp.exec(text) : null);
   if (match)
   {
     options = match[1].toUpperCase().split(",");
     text = match.input.substr(0, match.index);
     for (let option of options)
     {
@@ skipping 25 matching lines @@
         domains = value;
       else if (option == "THIRD_PARTY")
         thirdParty = true;
       else if (option == "~THIRD_PARTY")
         thirdParty = false;
       else if (option == "COLLAPSE")
         collapse = true;
       else if (option == "~COLLAPSE")
         collapse = false;
       else if (option == "SITEKEY" && typeof value != "undefined")
-        siteKeys = value;
+        sitekeys = value;
       else
         return new InvalidFilter(origText, "Unknown option " + option.toLowerCase());
     }
   }
 
   if (!blocking && (contentType == null || (contentType & RegExpFilter.typeMap.DOCUMENT)) &&
       (!options || options.indexOf("DOCUMENT") < 0) && !/^\|?[\w\-]+:/.test(text))
   {
     // Exception filters shouldn't apply to pages by default unless they start with a protocol name
     if (contentType == null)
       contentType = RegExpFilter.prototype.contentType;
     contentType &= ~RegExpFilter.typeMap.DOCUMENT;
   }
 
   try
   {
     if (blocking)
-      return new BlockingFilter(origText, text, contentType, matchCase, domains, thirdParty, siteKeys, collapse);
+      return new BlockingFilter(origText, text, contentType, matchCase, domains, thirdParty, sitekeys, collapse);
     else
-      return new WhitelistFilter(origText, text, contentType, matchCase, domains, thirdParty, siteKeys);
+      return new WhitelistFilter(origText, text, contentType, matchCase, domains, thirdParty, sitekeys);
   }
   catch (e)
   {
     return new InvalidFilter(origText, e);
   }
 }
 
 /**
  * Maps type strings like "SCRIPT" or "OBJECT" to bit masks
  */
@@ skipping 23 matching lines @@
 RegExpFilter.prototype.contentType &= ~(RegExpFilter.typeMap.ELEMHIDE | RegExpFilter.typeMap.POPUP);
 
 /**
  * Class for blocking filters
  * @param {String} text see Filter()
  * @param {String} regexpSource see RegExpFilter()
  * @param {Number} contentType see RegExpFilter()
  * @param {Boolean} matchCase see RegExpFilter()
  * @param {String} domains see RegExpFilter()
  * @param {Boolean} thirdParty see RegExpFilter()
- * @param {String} siteKeys 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, siteKeys, collapse)
-{
-  RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains, thirdParty, siteKeys);
+function BlockingFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, sitekeys, collapse)
+{
+  RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains, thirdParty, sitekeys);
 
   this.collapse = collapse;
 }
 exports.BlockingFilter = BlockingFilter;
 
 BlockingFilter.prototype =
 {
   __proto__: RegExpFilter.prototype,
 
   /**
    * Defines whether the filter should collapse blocked content. Can be null (use the global preference).
    * @type Boolean
    */
   collapse: null
 };
 
 /**
  * Class for whitelist filters
  * @param {String} text see Filter()
  * @param {String} regexpSource see RegExpFilter()
  * @param {Number} contentType see RegExpFilter()
  * @param {Boolean} matchCase see RegExpFilter()
  * @param {String} domains see RegExpFilter()
  * @param {Boolean} thirdParty see RegExpFilter()
- * @param {String} siteKeys see RegExpFilter()
+ * @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, siteKeys);
+function WhitelistFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, sitekeys)
+{
+  RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains, thirdParty, sitekeys);
 }
 exports.WhitelistFilter = WhitelistFilter;
 
 WhitelistFilter.prototype =
 {
   __proto__: RegExpFilter.prototype
 }
 
 /**
  * Base class for element hiding filters
  * @param {String} text see Filter()
- * @param {String} domains    (optional) Host names or domains the filter should be restricted to
+ * @param {String} [domains] Host names or domains the filter should be restricted to
  * @param {String} selector   CSS selector for the HTML elements that should be hidden
  * @constructor
  * @augments ActiveFilter
  */
 function ElemHideBase(text, domains, selector)
 {
   ActiveFilter.call(this, text, domains || null);
 
   if (domains)
     this.selectorDomain = domains.replace(/,~[^,]+/g, "").replace(/^~[^,]+,?/, "").toLowerCase();
@@ skipping 113 matching lines @@
 function ElemHideException(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideException = ElemHideException;
 
 ElemHideException.prototype =
 {
   __proto__: ElemHideBase.prototype
 };