Issue 6735 - Store domains in lower case

lib/filterClasses.js

Index: lib/filterClasses.js
===================================================================
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -382,38 +382,38 @@
   /**
    * Separator character used in domainSource property, must be
    * overridden by subclasses
    * @type {string}
    */
   domainSeparator: null,
 
   /**
-   * Determines whether domainSource is already upper-case,
+   * Determines whether domainSource is already lower-case,
    * can be overridden by subclasses.
    * @type {boolean}
    */
-  domainSourceIsUpperCase: false,
+  domainSourceIsLowerCase: false,
 
   /**
    * 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()
   {
     let domains = null;
 
     if (this.domainSource)
     {
       let source = this.domainSource;
-      if (!this.domainSourceIsUpperCase)
+      if (!this.domainSourceIsLowerCase)
       {
-        // RegExpFilter already have uppercase domains
-        source = source.toUpperCase();
+        // RegExpFilter already have lowercase domains
+        source = source.toLowerCase();
       }
       let list = source.split(this.domainSeparator);
       if (list.length == 1 && list[0][0] != "~")
       {
         // Fast track for the common one-domain scenario
         domains = new Map([["", false], [list[0], true]]);
       }
       else
@@ -480,17 +480,17 @@
     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.get("");
 
-    docDomain = docDomain.replace(/\.+$/, "").toUpperCase();
+    docDomain = docDomain.replace(/\.+$/, "").toLowerCase();
 
     while (true)
     {
       let isDomainIncluded = this.domains.get(docDomain);
       if (typeof isDomainIncluded != "undefined")
         return isDomainIncluded;
 
       let nextDot = docDomain.indexOf(".");
@@ -506,17 +506,17 @@
    * @param {string} docDomain
    * @return {boolean}
    */
   isActiveOnlyOnDomain(docDomain)
   {
     if (!docDomain || !this.domains || this.domains.get(""))
       return false;
 
-    docDomain = docDomain.replace(/\.+$/, "").toUpperCase();
+    docDomain = docDomain.replace(/\.+$/, "").toLowerCase();
 
     for (let [domain, isIncluded] of this.domains)
     {
       if (isIncluded && domain != docDomain)
       {
         if (domain.length <= docDomain.length)
           return false;
 
@@ -607,19 +607,19 @@
     // No need to convert this filter to regular expression yet, do it on demand
     this.regexpSource = regexpSource;
   }
 }
 exports.RegExpFilter = RegExpFilter;
 
 RegExpFilter.prototype = extend(ActiveFilter, {
   /**
-   * @see ActiveFilter.domainSourceIsUpperCase
+   * @see ActiveFilter.domainSourceIsLowerCase
    */
-  domainSourceIsUpperCase: true,
+  domainSourceIsLowerCase: true,
 
   /**
    * Number of filters contained, will always be 1 (required to
    * optimize Matcher).
    * @type {number}
    */
   length: 1,
 
@@ -750,50 +750,67 @@
     {
       let value = null;
       let separatorIndex = option.indexOf("=");
       if (separatorIndex >= 0)
       {
         value = option.substr(separatorIndex + 1);
         option = option.substr(0, separatorIndex);
       }
-      option = option.replace(/-/, "_").toUpperCase();
-      if (option in RegExpFilter.typeMap)
-      {
-        contentType |= RegExpFilter.typeMap[option];
+
+      let inverse = option[0] == "~";

[Manish Jethani, 2018/06/08 05:00:41]

I'm just calling it "inverse" because "option" is implied (this is the options
loop).

[kzar, 2018/06/08 09:30:32]

Acknowledged.

+      if (inverse)
+        option = option.substr(1);
 
-        if (option == "CSP" && value)
-          csp = value;
-      }
-      else if (option[0] == "~" && option.substr(1) in RegExpFilter.typeMap)
+      let type = RegExpFilter.typeMap[option.replace(/-/, "_").toUpperCase()];

[Manish Jethani, 2018/06/08 05:00:41]

Lookup happens only once in the RegExpFilter.typeMap object.

[kzar, 2018/06/08 09:30:32]

Nice.

+      if (type)
       {
-        if (contentType == null)
-          ({contentType} = RegExpFilter.prototype);
-        contentType &= ~RegExpFilter.typeMap[option.substr(1)];
+        if (inverse)
+        {
+          if (contentType == null)
+            ({contentType} = RegExpFilter.prototype);
+          contentType &= ~type;
+        }
+        else
+        {
+          contentType |= type;
+
+          if (type == RegExpFilter.typeMap.CSP && value)

[Manish Jethani, 2018/06/08 05:00:41]

Integer comparison.

+            csp = value;
+        }
       }
-      else if (option == "MATCH_CASE")
-        matchCase = true;
-      else if (option == "~MATCH_CASE")
-        matchCase = false;
-      else if (option == "DOMAIN" && value)
-        domains = value.toUpperCase();
-      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" && value)
-        sitekeys = value.toUpperCase();
-      else if (option == "REWRITE" && value)
-        rewrite = value;
       else
-        return new InvalidFilter(origText, "filter_unknown_option");
+      {
+        switch (option.toLowerCase())

[Manish Jethani, 2018/06/08 05:00:41]

Only lowercase if we get this far.

switch is cleaner than if-else in my opinion, let me know if you disagree.

+        {
+          case "match-case":
+            matchCase = !inverse;

[Manish Jethani, 2018/06/08 05:00:41]

!inverse

[kzar, 2018/06/08 09:30:31]

(Heh, it does make you wonder if storing !inverse as a variable would be better,
but I can't think of a good name.)

+            break;
+          case "domain":
+            if (value)

[Manish Jethani, 2018/06/08 05:00:41]

Note: This has a good side effect. Previously foo$domain would throw an error, I
don't see why it should. It should just apply to any domain in that case. I mean
the option should be ignored (it's not an "unknown option", it's a known option
with no value).

[kzar, 2018/06/08 09:30:32]

Seems reasonable to me, but I wonder if this was intentional. Do you have an
opinion Segei?

[sergei, 2018/06/08 13:28:11]

Formally, if the option is present and there are no values then the filter is
domain specific with no domains and simply put it should be never applied. BTW,
previously the code returned an instance of InvalidFilter, not thrown an
exception.

If it's a special case then it deserves a test IMO.

I would like to prefer to fail earlier in terms of rather mark the filter as
invalid, however if it conflicts with the idea to allow empty options, then it's
fine for me but could you please add a test for it?

[Manish Jethani, 2018/06/08 14:12:39]

For known options that must have a value but none is given, what do you think
about returning an invalid filter with a different reason like
"filter_missing_value" or "filter_invalid_value"? Previously the reason was
"filter_unknown_option" was not exactly correct.

Anyway, let's try to stay compatible here and not change the behavior, so I'll
try to keep it the same for now. Let me update the patch.

+              domains = value.toLowerCase();
+            break;
+          case "third-party":
+            thirdParty = !inverse;
+            break;
+          case "collapse":
+            collapse = !inverse;
+            break;
+          case "sitekey":
+            if (value)
+              sitekeys = value.toUpperCase();
+            break;
+          case "rewrite":
+            if (value)

[Manish Jethani, 2018/06/08 05:00:41]

Note that this also simplifies how we can accept the blank value for the rewrite
option in the other patch.

[kzar, 2018/06/08 09:30:31]

Acknowledged.

+              rewrite = value;
+            break;
+          default:
+            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)