Issue 6727 - Use string rather than map for single-domain filters

lib/filterClasses.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 368 matching lines @@
   domainSource: null,
 
   /**
    * Separator character used in domainSource property, must be
    * overridden by subclasses
    * @type {string}
    */
   domainSeparator: null,
 
   /**
-   * Determines whether the trailing dot in domain names isn't important and
-   * should be ignored, must be overridden by subclasses.
-   * @type {boolean}
-   */
-  ignoreTrailingDot: true,
-
-  /**
    * Determines whether domainSource is already upper-case,
    * can be overridden by subclasses.
    * @type {boolean}
    */
   domainSourceIsUpperCase: false,
 
   /**
-   * String specifying the domain that this filter should match on, or map
-   * containing domains that this filter should match on/not match on, or null
-   * if the filter should match on all domains
-   * @type {?Map.<string,boolean>|string}
+   * Map containing domains that this filter should match on/not match
+   * on or null if the filter should match on all domains
+   * @type {?Map.<string,boolean>}
    */
   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");
+    let prop = Object.getOwnPropertyDescriptor(this, "_domains");
     if (prop)
-      return prop.value;
+    {
+      let {value} = prop;
+      return typeof value == "string" ?
+               new Map([[value, true], ["", false]]) : 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);
       if (list.length == 1 && list[0][0] != "~")
       {
         // Fast track for the common one-domain scenario
-        if (this.ignoreTrailingDot)
-          list[0] = list[0].replace(/\.+$/, "");
         domains = list[0];
       }
       else
       {
         let hasIncludes = false;
         for (let i = 0; i < list.length; i++)
         {
           let domain = list[i];
-          if (this.ignoreTrailingDot)
-            domain = domain.replace(/\.+$/, "");
           if (domain == "")
             continue;
 
           let include;
           if (domain[0] == "~")
           {
             include = false;
             domain = domain.substr(1);
           }
           else
           {
             include = true;
             hasIncludes = true;
           }
 
           if (!domains)
             domains = new Map();
 
           domains.set(domain, include);
         }
         if (domains)
           domains.set("", !hasIncludes);
       }
 
       this.domainSource = null;
     }
 
-    Object.defineProperty(this, "domains", {value: domains, enumerable: true});
+    Object.defineProperty(this, "_domains", {value: domains});
     return this.domains;
   },
 
   /**
    * Array containing public keys of websites that this filter should apply to
    * @type {?string[]}
    */
   sitekeys: null,
 
   /**
@@ skipping 13 matching lines @@
       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 typeof this.domains != "string" && this.domains.get("");
-
-    if (this.ignoreTrailingDot)
-      docDomain = docDomain.replace(/\.+$/, "");
-    docDomain = docDomain.toUpperCase();
-
-    if (typeof this.domains == "string")
-    {
-      return docDomain == this.domains ||
-             docDomain.endsWith("." + this.domains);
-    }
+      return this.domains.get("");
+
+    docDomain = docDomain.replace(/\.+$/, "").toUpperCase();
 
     while (true)
     {
       let isDomainIncluded = this.domains.get(docDomain);
       if (typeof isDomainIncluded != "undefined")
         return isDomainIncluded;
 
       let nextDot = docDomain.indexOf(".");
       if (nextDot < 0)
         break;
       docDomain = docDomain.substr(nextDot + 1);
     }
     return this.domains.get("");
   },
 
   /**
    * Checks whether this filter is active only on a domain and its subdomains.
    * @param {string} docDomain
    * @return {boolean}
    */
   isActiveOnlyOnDomain(docDomain)
   {
-    if (!docDomain || !this.domains ||
-        typeof this.domains != "string" && this.domains.get(""))

[sergei, 2018/05/28 09:22:07]

IMO, perhaps it would be safer to rather check that `this.domains instanceof
Map` instead of typeof.

BTW, despite it's correct, wouldn't it be better to wrap the logical AND into
the braces () in order to improve readability because I'm not sure how many
people actually remember that && has a higher priority than || in JS?

[kzar, 2018/06/05 16:51:30]

Disagree on this one, people that don't understand JavaScript's logical
operators shouldn't really be messing with this code IMO.

[Manish Jethani, 2018/06/06 11:16:31]

This is one of those things that can lead to an endless debate, so in the
interest of consistency I'll leave it as it is. It seems in the rest of the code
we assume knowledge of logical operator precedence. I personally do prefer to
group things logically though.

[Manish Jethani, 2018/06/06 12:31:18]

typeof is typically faster than instanceof, so where there's a choice I think it
makes sense to use the former.

-    {
+    if (!docDomain || !this.domains || this.domains.get(""))
       return false;
-    }
-
-    if (this.ignoreTrailingDot)
-      docDomain = docDomain.replace(/\.+$/, "");
-    docDomain = docDomain.toUpperCase();
-
-    if (typeof this.domains == "string")
-    {
-      return docDomain == this.domains ||
-             this.domains.endsWith("." + docDomain);
-    }
+
+    docDomain = docDomain.replace(/\.+$/, "").toUpperCase();
 
     for (let [domain, isIncluded] of this.domains)
     {
       if (isIncluded && domain != docDomain)
       {
         if (domain.length <= docDomain.length)
           return false;
 
         if (!domain.endsWith("." + docDomain))
           return false;
       }
     }
 
     return true;
   },
 
   /**
    * Checks whether this filter is generic or specific
    * @return {boolean}
    */
   isGeneric()
   {
     return !(this.sitekeys && this.sitekeys.length) &&
-            (!this.domains ||
-             typeof this.domains != "string" && this.domains.get(""));
+            (!this.domains || this.domains.get(""));
   },
 
   /**
    * See Filter.serialize()
    * @inheritdoc
    */
   serialize(buffer)
   {
     if (this._disabled || this._hitCount || this._lastHit)
     {
@@ skipping 93 matching lines @@
   {
     // 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;
 
     let source = Filter.toRegExp(this.regexpSource);
     let regexp = new RegExp(source, this.matchCase ? "" : "i");
     Object.defineProperty(this, "regexp", {value: regexp});
-    delete this.regexpSource;
+    this.regexpSource = null;
     return regexp;
   },
   /**
    * Content types the filter applies to, combination of values from
    * RegExpFilter.typeMap
    * @type {number}
    */
   contentType: 0x7FFFFFFF,
   /**
    * Defines whether the filter should distinguish between lower and
@@ skipping 136 matching lines @@
         collapse = false;
       else if (option == "SITEKEY" && value)
         sitekeys = value.toUpperCase();
       else if (option == "REWRITE" && value)
         rewrite = value;
       else
         return new InvalidFilter(origText, "filter_unknown_option");
     }
   }
 
+  // For security reasons, never match $rewrite filters
+  // against requests that might load any code to be executed.
+  if (rewrite != null)
+  {
+    if (contentType == null)
+      ({contentType} = RegExpFilter.prototype);
+    contentType &= ~(RegExpFilter.typeMap.SCRIPT |
+                     RegExpFilter.typeMap.SUBDOCUMENT |
+                     RegExpFilter.typeMap.OBJECT |
+                     RegExpFilter.typeMap.OBJECT_SUBREQUEST);
+  }
+
   try
   {
     if (blocking)
     {
       if (csp && Filter.invalidCSPRegExp.test(csp))
         return new InvalidFilter(origText, "filter_invalid_csp");
 
       return new BlockingFilter(origText, text, contentType, matchCase, domains,
                                 thirdParty, sitekeys, collapse, csp, rewrite);
     }
@@ skipping 63 matching lines @@
  *   BlockingFilter.prototype.rewrite.
  * @constructor
  * @augments RegExpFilter
  */
 function BlockingFilter(text, regexpSource, contentType, matchCase, domains,
                         thirdParty, sitekeys, collapse, csp, rewrite)
 {
   RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains,
                     thirdParty, sitekeys);
 
-  this.collapse = collapse;
-  this.csp = csp;
-  this.rewrite = rewrite;
+  if (collapse != null)
+    this.collapse = collapse;
+
+  if (csp != null)
+    this.csp = csp;
+
+  if (rewrite != null)
+    this.rewrite = rewrite;
 }
 exports.BlockingFilter = BlockingFilter;
 
 BlockingFilter.prototype = extend(RegExpFilter, {
   type: "blocking",
 
   /**
    * Defines whether the filter should collapse blocked content.
    * Can be null (use the global preference).
    * @type {?boolean}
@@ skipping 65 matching lines @@
  *                           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.selectorDomains = domains.replace(/,~[^,]+/g, "")
-                                  .replace(/^~[^,]+,?/, "").toLowerCase();
-  }
-
   // Braces are being escaped to prevent CSS rule injection.
   this.selector = selector.replace("{", "\\7B ").replace("}", "\\7D ");
 }
 exports.ElemHideBase = ElemHideBase;
 
 ElemHideBase.prototype = extend(ActiveFilter, {
   /**
    * @see ActiveFilter.domainSeparator
    */
   domainSeparator: ",",
 
-  /**
-   * @see ActiveFilter.ignoreTrailingDot
-   */
-  ignoreTrailingDot: false,
-
-  /**
-   * Host names or domains the filter should be restricted to (can be null for
-   * no restriction)
-   * @type {?string}
-   */
-  selectorDomains: null,
   /**
    * CSS selector for the HTML elements that should be hidden
    * @type {string}
    */
   selector: null
 });
 
 /**
  * Creates an element hiding filter from a pre-parsed text representation
  *
@@ skipping 76 matching lines @@
  */
 function ElemHideEmulationFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
 ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
   type: "elemhideemulation"
 });