[$csp1 adblockpluscore] Issue 6329 - Allow whitespace in filter option values

lib/filterClasses.js

Index: lib/filterClasses.js
diff --git a/lib/filterClasses.js b/lib/filterClasses.js
index 1498ad8db2da7975ef3dce4916ef843d29d51420..1cf4f48e192c33afc7d9fb3d731c5a0f4de94946 100644
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -87,7 +87,7 @@ Filter.knownFilters = new Map();
  * Regular expression that element hiding filters should match
  * @type {RegExp}
  */
-Filter.elemhideRegExp = /^([^/*|@"!]*?)#([@?])?#(.+)$/;
+Filter.elemhideRegExp = /^([^/*|@"!]*?)(#([@?])?#)(.+)$/;
 /**
  * Regular expression that RegExp filters specified as RegExps should match
  * @type {RegExp}
@@ -97,12 +97,12 @@ Filter.regexpRegExp = /^(@@)?\/.*\/(?:\$~?[\w-]+(?:=[^,\s]+)?(?:,~?[\w-]+(?:=[^,
  * Regular expression that options on a RegExp filter should match
  * @type {RegExp}
  */
-Filter.optionsRegExp = /\$(~?[\w-]+(?:=[^,\s]+)?(?:,~?[\w-]+(?:=[^,\s]+)?)*)$/;
+Filter.optionsRegExp = /\$(~?[\w-]+(?:=[^,]+)?(?:,~?[\w-]+(?:=[^,]+)?)*)$/;
 
 /**
  * Creates a filter of correct type from its text representation - does the
- * basic parsing and calls the right constructor then.
- *
+ * basic normalisation and parsing where possible before deferring to the right

[Manish Jethani, 2018/02/05 16:09:57]

I noticed that the documentation is now using the British spelling of
"normalization". I don't have a preference either way other than maybe for
consistency it should use the American spelling.

[kzar, 2018/02/13 11:56:46]

Kind of ironic heh, Done.

+ * constructor.
  * @param {string} text   as in Filter()
  * @return {Filter}
  */
@@ -115,20 +115,25 @@ Filter.fromText = function(text)
   let match = (text.includes("#") ? Filter.elemhideRegExp.exec(text) : null);
   if (match)
   {
+    let [, domain, seperator, type, selector] = match;
+
+    domain = domain.replace(/\s/g, "");
+    selector = selector.trim();
+
     let propsMatch;
-    if (!match[2] &&
-        (propsMatch = /\[-abp-properties=(["'])([^"']+)\1\]/.exec(match[3])))
+    if (!type &&
+        (propsMatch = /\[-abp-properties=(["'])([^"']+)\1\]/.exec(selector)))
     {
       // This is legacy CSS properties syntax, convert to current syntax
-      let prefix = match[3].substr(0, propsMatch.index);
+      let prefix = selector.substr(0, propsMatch.index);
       let expression = propsMatch[2];
-      let suffix = match[3].substr(propsMatch.index + propsMatch[0].length);
-      return Filter.fromText(`${match[1]}#?#` +
+      let suffix = selector.substr(propsMatch.index + propsMatch[0].length);
+      return Filter.fromText(`${domain}#?#` +
           `${prefix}:-abp-properties(${expression})${suffix}`);
     }
 
     filter = ElemHideBase.fromText(
-      text, match[1], match[2], match[3]
+      domain + seperator + selector, domain, type, selector
     );
   }
   else if (text[0] == "!")
@@ -162,32 +167,16 @@ Filter.fromObject = function(obj)
 };
 
 /**
- * Removes unnecessary whitespaces from filter text, will only return null if
- * the input parameter is null.
+ * Strip linebreaks etc and then trim whitespace from the filter text.
+ * Will only return null if the input parameter is null.
  * @param {string} text
  * @return {string}
  */
-Filter.normalize = function(text)
+Filter.stripJunk = function(text)
 {
   if (!text)
     return text;
-
-  // Remove line breaks and such
-  text = text.replace(/[^\S ]/g, "");
-
-  if (/^\s*!/.test(text))
-  {
-    // Don't remove spaces inside comments
-    return text.trim();
-  }
-  else if (Filter.elemhideRegExp.test(text))
-  {
-    // Special treatment for element hiding filters, right side is allowed to
-    // contain spaces
-    let [, domain, separator, selector] = /^(.*?)(#@?#?)(.*)$/.exec(text);
-    return domain.replace(/\s/g, "") + separator + selector.trim();
-  }
-  return text.replace(/\s/g, "");
+  return text.replace(/[^\S ]/g, "").trim();
 };
 
 /**
@@ -713,34 +702,33 @@ Object.defineProperty(RegExpFilter.prototype, "0", {
  */
 RegExpFilter.fromText = function(text)
 {
-  let blocking = true;
-  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 thirdParty = null;
   let collapse = null;
-  let options;
+  let origText;

[Manish Jethani, 2018/02/05 16:09:58]

Why not initialize origText to null for consistency?

[kzar, 2018/02/13 11:56:46]

Done.

+
   let match = (text.indexOf("$") >= 0 ? Filter.optionsRegExp.exec(text) : null);
-  if (match)
+  if (!match)
   {
-    options = match[1].toUpperCase().split(",");
-    text = match.input.substr(0, match.index);
-    for (let option of options)
+    origText = text = text.replace(/\s/g, "");
+  }
+  else
+  {
+    text = match.input.substring(0, match.index).replace(/\s/g, "");
+    let options = match[1].replace(/\s/g, "");

[Manish Jethani, 2018/02/05 16:09:58]

I don't get this part. Doesn't this mean that "$csp=script-src 'none'" will
become "$csp=script-src'none'"?

[Manish Jethani, 2018/02/06 05:14:39]

I see, so the other patch builds on this. Got it.

[kzar, 2018/02/13 11:56:46]

Sorry I should have left a comment on the review here to explain, but yep you
got it.

+    origText = text + "$" + options;
+
+    for (let option of options.toUpperCase().split(","))
     {
       let value = null;
       let separatorIndex = option.indexOf("=");
       if (separatorIndex >= 0)
       {
         value = option.substr(separatorIndex + 1);
-        option = option.substr(0, separatorIndex);
+        option = option.substring(0, separatorIndex);

[Manish Jethani, 2018/02/05 16:09:58]

Any particular reason this is now using String.substring?

[kzar, 2018/02/13 11:56:47]

Well since I noticed that `substr` was being used mistakenly all this time, but
happened to work since the offset was always 0. Quite misleading.

[Manish Jethani, 2018/02/20 16:00:57]

Why is it a mistake to use substr?

Anyway, the change is unrelated so I think it should be part of a separate patch
(and then the change should be made all over the code).

       }
       option = option.replace(/-/, "_");
       if (option in RegExpFilter.typeMap)
@@ -778,13 +766,13 @@ RegExpFilter.fromText = function(text)
 
   try
   {
-    if (blocking)
+    if (text.indexOf("@@") == 0)
     {
-      return new BlockingFilter(origText, text, contentType, matchCase, domains,
-                                thirdParty, sitekeys, collapse);
+      return new WhitelistFilter(origText, text.substr(2), contentType,
+                                 matchCase, domains, thirdParty, sitekeys);
     }
-    return new WhitelistFilter(origText, text, contentType, matchCase, domains,
-                               thirdParty, sitekeys);
+    return new BlockingFilter(origText, text, contentType, matchCase, domains,
+                              thirdParty, sitekeys, collapse);
   }
   catch (e)
   {