[$csp2 adblockpluscore] Issue 6329 - Add the CSP filter type

lib/filterClasses.js

Index: lib/filterClasses.js
diff --git a/lib/filterClasses.js b/lib/filterClasses.js
index 1498ad8db2da7975ef3dce4916ef843d29d51420..67cb41ccea56eb2f03b837d28e7fdbd0f73f5de1 100644
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -97,7 +97,7 @@ 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
@@ -175,19 +175,60 @@ Filter.normalize = function(text)
   // Remove line breaks and such
   text = text.replace(/[^\S ]/g, "");
 
+  // Don't remove spaces inside comments
   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
+  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, "");
+
+  // For most regexp filters we strip all whitespace, but the values of $csp

[Manish Jethani, 2018/03/12 18:53:59]

So this doesn't work for the following cases:

 1.  foo?csp=b a r$csp=script-src  'self'
 2.  foo$bar=c s p = ba z,cs p = script-src  'self'

(In the second case, assume "bar" is a future option type that can take a value
that contains the substring "csp=")

I'm just wondering now, why not just use the following instead of all this:

  let index = text.indexOf("$");
  if (index >= 0)
  { 
    let options = text.substring(index + 1).split(",");
    for (let i = 0; i < options.length; i++)
    { 
      let csp = /^ *c *s *p *=/.exec(options[i]);
      if (csp)
      { 
        options[i] = csp[0].replace(/ +/g, "") +
                     options[i].substring(csp[0].length).replace(/ +/g, " ")
                     .trim();
      }
      else
      { 
        options[i] = options[i].replace(/ +/g, "");
      }
    }
    
    return text.substring(0, index + 1).replace(/ +/g, "") + options.join(",");
  }
  
  return text.replace(/ +/g, "");

This is easier to understand, technically more correct (will not get confused
with "csp=" in the option value or in the URL part of the filter), and performs
just as well.

[Sebastian Noack, 2018/03/12 23:19:01]

Splitting the options list into an array (for every filter that has options),
likely has measurable performance implications in the common case.

[Manish Jethani, 2018/03/13 06:59:30]

You mean in terms of memory consumption? I ran a test with both implementations
and the one I suggested above wasn't any slower than the original. Since strings
are immutable in JS, I would be surprised if String.split didn't just return
strings that simply pointed back to the same memory locations in the original
string.

If we want to avoid creating that array we could try with generators:

  function* splitOptions(string)
  {
    let startIndex = 0;
    let index = -1;

    while ((index = string.indexOf(",", startIndex = index + 1)) != -1)
      yield string.substring(startIndex, index + 1);

    yield string.substring(startIndex);
  }

  ...

  let index = text.indexOf("$");
  if (index >= 0)
  {
    let options = "";
    for (let fragment of splitOptions(text.substring(index)))
    {
      let csp = /^ *c *s *p *=/.exec(fragment);
      if (csp)
      {
        options += csp[0].replace(/ +/g, "") +
                   fragment.substring(csp[0].length).replace(/ +/g, " ").trim();
      }
      else
      {
        options += fragment.replace(/ +/g, "");
      }
    }

    return text.substring(0, index).replace(/ +/g, "") + options;
  }

This seems to be a bit slower but not too much.

[Manish Jethani, 2018/03/13 07:35:00]

OK, so I inlined that bit, now this seems to perform well:

  let index = text.indexOf("$");
  if (index >= 0)
  {
    let options = text.substring(index + 1);
    text = text.substring(0, index + 1).replace(/ +/g, "");

    for (let start = 0, end; end != options.length; start = end + 1)
    {
      end = options.indexOf(",", start);
      if (end == -1)
        end = options.length;

      let fragment = options.substring(start, end);
      let csp = /^ *c *s *p *=/.exec(fragment);
      if (csp)
      {
        text += csp[0].replace(/ +/g, "") +
                fragment.substring(csp[0].length).replace(/ +/g, " ").trim();
      }
      else
      {
        text += fragment.replace(/ +/g, "");
      }
    }

    return text;
  }

[kzar, 2018/03/14 13:54:37]

Well spotted, I've added some unit tests for those cases.

[Sebastian Noack, 2018/03/14 20:59:20]

Any reason, why this suggestion was ignored, without a comment?

[kzar, 2018/03/15 10:26:24]

I ignored that since it assumed the first '$' was the start of the options
string, which I pointed out elsewhere is not necessarily the case. Sorry I
thought that was obvious but I guess not.

[Manish Jethani, 2018/03/15 10:42:41]

I think Sebastian is referring to the rest of the code, i.e. the part where we
already have the correct index of the options part.

So basically starting with this:

  let options = text.substring(index + 1);
  text = text.substring(0, index + 1).replace(/ +/g, "");

  for (let start = 0, end; end != options.length; start = end + 1)
  ...

In my tests I found that this was much faster than using String.split, but
arguably it makes the code harder to read.

[kzar, 2018/03/15 11:38:50]

So instead of splitting the options string by "," instead using substring and
indexOf(",") to pick out each option?
Well fair enough, but I feel like optimising this rarely used code path at the
cost of code readability isn't worth it.

I would see the utility of moving the logic out into a generator function
*parseOptions which yields each option part however, since that could also be
used in the parsing code which is worth optimising. However if I got it right
using a generator here would make performance even worse than simply splitting
the string.

[Manish Jethani, 2018/03/15 12:00:11]

Yes, it just turned out to be faster.
I'm cool with using String.split here in the interested of
readability/maintainability.
Yeah, I tested with the generator, it performs worse. String.split is faster.

[Sebastian Noack, 2018/03/15 17:25:41]

I guess, if it's only in the code path hit for $csp filters, the performance
penalty is acceptable.

+  // filter options are allowed to contain single (non trailing) spaces.
+  let strippedText = text.replace(/\s/g, "");

[Manish Jethani, 2018/03/12 16:34:53]

Since we have already stripped out all non-space whitespace, we can just use " "
(space) instead of \s here?

  let strippedText = text.replace(/ /g, "");

[kzar, 2018/03/14 13:54:38]

I think you're right, Done.

+  if (!/csp=/i.test(strippedText))
+    return strippedText;
+
+  let optionsMatch = Filter.optionsRegExp.exec(strippedText);

[Manish Jethani, 2018/03/12 16:34:53]

We can just look for "$" here and if none is present then there are no options,
right? We don't have to match the regular expression.

Combined with the above then we could do:

  if (strippedText.indexOf("$") == -1 || !/[$,]csp=/i.test(strippedText))
    return strippedText;

[kzar, 2018/03/14 13:54:38]

No, that's not good enough unfortunately. There can be a $ (or multiple) with no
options part. I added a test case for that as well FWIW.

[Manish Jethani, 2018/03/14 18:10:49]

OK, but I wonder if we shouldn't just look for a "$" first anyway before running
the regular expression. I see the same thing being done in
RegExpFilter.fromText:

  let match = (text.indexOf("$") >= 0 ? Filter.optionsRegExp.exec(text) : null);
  if (match)
  {
    ...
  }

+  if (!optionsMatch)
+    return strippedText;
+
+  // We know where the options part starts in the filter text that's stripped
+  // of whitespace, next we must find the corresponding position in the original
+  // filter text.
+  let optionsPosition = 0;

[Manish Jethani, 2018/03/12 16:34:53]

This part is not being used at all.

[kzar, 2018/03/14 13:54:38]

Whoops, you're right. I've fixed that now.

+  let offset = 0;
+  while (offset > -1)
+  {
+    optionsPosition = text.substring(optionsPosition).indexOf("$");
+    offset = strippedText.substring(offset, optionsPosition).indexOf("$");
+  }
+
+  // Finally with that we can generally strip whitespace, being careful to not
+  // to for $csp filter values.
+  let parts = [];
+  let position = 0;
+  let cspRegexp = /(c\s*s\s*p\s*=)([^,]+)/ig;
+  let cspMatch;
+  while (cspMatch = cspRegexp.exec(text))
+  {
+    parts.push(

[Manish Jethani, 2018/03/12 16:34:53]

We've already stripped whitespace once, now we're stripping it again. This only
affects filters with a $csp option, but I wonder if we shouldn't just check for
$csp before stripping whitespace the first time.

i.e. above:

  if (text.indexOf("$") == -1 || !/[$,] *c *s *p *=/i.test(text))
    return text.replace(/ /g, "");

[kzar, 2018/03/14 13:54:37]

No because we're expecting thousands of non-csp filters and maybe 5 csp filters.
We should optimise for the common case.

+      text.substring(position, cspMatch.index + cspMatch[1].length)
+          .replace(/\s/g, "")
+    );
+    parts.push(
+      text.substr(cspMatch.index + cspMatch[1].length, cspMatch[2].length)
+          .replace(/\s+/g, " ")
+          .trim()
+    );
+    position = cspMatch.index + cspMatch[0].length;
+  }
+  parts.push(text.substr(position).replace(/\s/g, ""));
+  return parts.join("");
 };
 
 /**
@@ -727,11 +768,12 @@ RegExpFilter.fromText = function(text)
   let sitekeys = null;
   let thirdParty = null;
   let collapse = null;
+  let csp = null;
   let options;
   let match = (text.indexOf("$") >= 0 ? Filter.optionsRegExp.exec(text) : null);
   if (match)
   {
-    options = match[1].toUpperCase().split(",");
+    options = match[1].split(",");
     text = match.input.substr(0, match.index);
     for (let option of options)
     {
@@ -740,14 +782,31 @@ RegExpFilter.fromText = function(text)
       if (separatorIndex >= 0)
       {
         value = option.substr(separatorIndex + 1);
-        option = option.substr(0, separatorIndex);
+        option = option.substr(0, separatorIndex).toUpperCase();
+
+        if (option == "CSP")
+          value = value.trim();
+        else
+          value = value.replace(/\s/g, "");
       }
+      else
+        option = option.toUpperCase();
+
       option = option.replace(/-/, "_");
+
       if (option in RegExpFilter.typeMap)
       {
         if (contentType == null)
           contentType = 0;
         contentType |= RegExpFilter.typeMap[option];
+
+        if (option == "CSP" && typeof value != "undefined")
+        {
+          if (csp)
+            csp.push(value);
+          else
+            csp = [value];
+        }
       }
       else if (option[0] == "~" && option.substr(1) in RegExpFilter.typeMap)
       {
@@ -760,7 +819,7 @@ RegExpFilter.fromText = function(text)
       else if (option == "~MATCH_CASE")
         matchCase = false;
       else if (option == "DOMAIN" && typeof value != "undefined")
-        domains = value;
+        domains = value.toUpperCase();
       else if (option == "THIRD_PARTY")
         thirdParty = true;
       else if (option == "~THIRD_PARTY")
@@ -770,18 +829,30 @@ RegExpFilter.fromText = function(text)
       else if (option == "~COLLAPSE")
         collapse = false;
       else if (option == "SITEKEY" && typeof value != "undefined")
-        sitekeys = value;
+        sitekeys = value.toUpperCase();
       else
         return new InvalidFilter(origText, "filter_unknown_option");
     }
   }
+  text = text.replace(/\s/g, "");
 
   try
   {
     if (blocking)
     {
+      if (csp)
+      {
+        csp = csp.join("; ").toLowerCase();
+
+        // Prevent filters from injecting report-uri or report-to directives
+        // since they are a privacy concern. Regexp based upon reBadCSP[1].
+        // [1] - https://github.com/gorhill/uBlock/blob/67e06f53b4d73df6179f6d320553a55da4ead40e/src/js/static-net-filtering.js#L1362
+        if (/(;|^)\s*report-(to|uri)\b/.test(csp))
+          return new InvalidFilter(origText, "filter_invalid_csp");
+      }
+
       return new BlockingFilter(origText, text, contentType, matchCase, domains,
-                                thirdParty, sitekeys, collapse);
+                                thirdParty, sitekeys, collapse, csp);
     }
     return new WhitelistFilter(origText, text, contentType, matchCase, domains,
                                thirdParty, sitekeys);
@@ -805,6 +876,7 @@ RegExpFilter.typeMap = {
   DOCUMENT: 64,
   WEBSOCKET: 128,
   WEBRTC: 256,
+  CSP: 512,
   XBL: 1,
   PING: 1024,
   XMLHTTPREQUEST: 2048,
@@ -821,9 +893,10 @@ RegExpFilter.typeMap = {
   GENERICHIDE: 0x80000000
 };
 
-// DOCUMENT, ELEMHIDE, POPUP, GENERICHIDE and GENERICBLOCK options shouldn't
-// be there by default
-RegExpFilter.prototype.contentType &= ~(RegExpFilter.typeMap.DOCUMENT |
+// CSP, DOCUMENT, ELEMHIDE, POPUP, GENERICHIDE and GENERICBLOCK options
+// shouldn't be there by default
+RegExpFilter.prototype.contentType &= ~(RegExpFilter.typeMap.CSP |
+                                        RegExpFilter.typeMap.DOCUMENT |
                                         RegExpFilter.typeMap.ELEMHIDE |
                                         RegExpFilter.typeMap.POPUP |
                                         RegExpFilter.typeMap.GENERICHIDE |
@@ -840,16 +913,19 @@ RegExpFilter.prototype.contentType &= ~(RegExpFilter.typeMap.DOCUMENT |
  * @param {string} sitekeys see RegExpFilter()
  * @param {boolean} collapse
  *   defines whether the filter should collapse blocked content, can be null
+ * @param {string} [csp]
+ *   Content Security Policy to inject when the filter matches
  * @constructor
  * @augments RegExpFilter
  */
 function BlockingFilter(text, regexpSource, contentType, matchCase, domains,
-                        thirdParty, sitekeys, collapse)
+                        thirdParty, sitekeys, collapse, csp)
 {
   RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains,
                     thirdParty, sitekeys);
 
   this.collapse = collapse;
+  this.csp = csp;
 }
 exports.BlockingFilter = BlockingFilter;
 
@@ -861,7 +937,13 @@ BlockingFilter.prototype = extend(RegExpFilter, {
    * Can be null (use the global preference).
    * @type {boolean}
    */
-  collapse: null
+  collapse: null,
+
+  /**
+   * Content Security Policy to inject for matching requests.
+   * @type {string}
+   */
+  csp: null
 });
 
 /**