[$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..f43dc92ba6a280ef749fb19116fc8c4fe615baaf 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
@@ -173,21 +173,51 @@ Filter.normalize = function(text)
     return text;
 
   // Remove line breaks and such
-  text = text.replace(/[^\S ]/g, "");
+  text = text.replace(/[^\S ]+/g, "");
 
-  if (/^\s*!/.test(text))
-  {
-    // Don't remove spaces inside comments
+  // Don't remove spaces inside comments
+  if (/^ *!/.test(text))

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

It seems previously, comments were allowed do be preceded by any kind of withe
space (\s), now you are explicitly checking for the space character. Why?

[Manish Jethani, 2018/03/15 08:09:59]

In the preceding code we're stripping out all non-space whitespace characters
(e.g. "\n") using /[^\S ]/ so we end up with only spaces and non-whitespace
characters. Now we only need to strip out spaces.

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

(See this comment as well
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.j...
)

     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 domain.replace(/ +/g, "") + separator + selector.trim();

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

Similarly, here.

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

See our responses above.

+  }
+
+  // For most regexp filters we strip all whitespace.
+  let strippedText = text.replace(/ +/g, "");
+  if (!strippedText.includes("$") || !/csp=/i.test(strippedText))

[Manish Jethani, 2018/03/15 08:23:17]

Can we make this /\bcsp=/i instead?

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

Done.

+    return strippedText;
+
+  let optionsMatch = Filter.optionsRegExp.exec(strippedText);
+  if (!optionsMatch)
+    return strippedText;
+
+  // But since the values of $csp filter options are allowed to contain single
+  // (non trailing) spaces we have to be more careful if they might be present.
+  let beforeOptions = strippedText.substring(0, optionsMatch.index);
+  let optionsText = text;
+  for (let i = beforeOptions.split("$").length; i > 0; i--)
+    optionsText = optionsText.substr(optionsText.indexOf("$") + 1);

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

It seems the same can be achieved with less code and without creating an array:

  for (let i; (i = optionsText.indexOf("$")) != -1;)
    optionsText = optionsText.substr(i + 1);

[Manish Jethani, 2018/03/15 08:09:59]

This only looks for the last "$" in the string, which can be achieved using
String.lastIndexOf, but as Dave points out it's not correct. There could be
legit "$" within the options text as well. This is not actually true right now,
but may be in the future; however, since we already don't support commas in the
value of the $csp option, we should also not support "$" in the value. If the
filter author needs to use a "$" they can encode it as "%24" [1]. Am I right?

I can't think of an actual reason why we'd want to support "$" in the options
part of the filter. The most complex option is in fact $csp, but even here
there's no instance of needing to support "$" in the value [2]

[1]:
https://stackoverflow.com/questions/1547899/which-characters-make-a-url-invalid
[2]:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Po...

[Manish Jethani, 2018/03/15 09:11:06]

Actually I still don't get why we have to do this. We should run the options
regular expression on the unstripped text.

  if (!text.includes("$") || !/\bc *s *p *=/i.test(text))
    return text.replace(/ +/g, "");

  let optionsMatch = Filter.optionsRegExp.exec(text);
  if (!optionsMatch)
    return text.replace(/ +/g, "");

  ...

  return beforeOptions.replace(/ +/g, "") + "$" + options.join();

optionsMatch already points to the correct location, plus we have skipped one
unnecessary cycle of stripping spaces out of the options text.

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

Then the options regexp might not match depending on the whitespace. Yes we
could change the optionsRegexp to add an optional whitespace inbetween every
character, but I don't want to do that since it's already huge and also it's
used later in the common case for parsing regexp filters.

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

I can't tell if you're trolling or not at this point. If we don't care about
slightly changing how existing filters are parsed why didn't we go with the
original suggestion of doing something like `text.trim().replace(/ +/, " ")`?

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

Sorry, then I must have misunderstood. Are "$" supported in option values right
now? If they are not already supported, why do we care about supporting them
now? We already know that the value of the $csp option doesn't need to have "$"
in it.

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

Sorry, yeah, I see what you mean. It wouldn't work.

[Manish Jethani, 2018/03/15 11:25:47]

OK, I think I understand what you mean.

  normalize("foo$domain=bar.com$")

This currently works, and it will continue to work because there's no "csp=" in
there.

  normalize("foo$csp=script-src 'self',domain=bar.com$")

This currently "works" (the $csp option isn't normalized correctly), and it'll
break if we only take the last "$" to be the starting point of the options text.
Well, so it's a "breaking" change, I agree, but what does it actually break?
There are no actual filters that would break because of this. It's only a
breaking change in theory. We can "officially" not support "$" in option values
from now on, and nobody would be affected by it. So if we are to be practical
I'd say let's just take the index of the last "$". If in the future there's a
case where an option value needs to have an actual "$", then we can switch to
more complicated code.

Let me know what you think.

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

Well the part of the filter following a "$" is considered to be the option
string if it matches the options regexp, otherwise it's not and that part is
considered to be a part of the filter's regexp. Take this example:

  somead$domain=valid.com|$invalid.com

So far it blocks requests matching /somead/ for the domain valid.com. With your
suggestion it would instead block requests matching
/somead\$domain=valid\.com\|\$invalid\.com/ for all domains.

FWIW I agree that this probably doesn't matter much in practice, but I have had
questions about these types of edge cases from users (vai Mapx) before. Take
this filter

  /$file/analytics.

IIRC they were confused why it worked, but one like /$file didn't. Well that's
since "$file" is considered a valid options string and not used for the regexp
where as $file/analytics" isn't.

[kzar, 2018/03/15 11:43:52]

Sure or we could "officially" not support filters with random spaces inside just
the same. So we just do `.trim().replace(/ /+, " ")`, if the user gets lucky
their filter still works, but if the space was in an option value name (or
similar) things break. I think it's too late to talk about being practical in
this codereview to be honest.
Well that's speculation, hopefully few users would be anyway. But that also
applies to my above point.

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

But the two are not comparable, are they? If we don't support spaces, we break
filters that used to work; if we don't support "$" in a filter that _also_
contains "csp=" we break nothing.

A filter can contain spaces because the person who wrote it thought they could
include a space after the "$" or after each ",", I mean how do they know better?
That's not the case with "$", there's no reason to include a "$" in an option
value (can you think of any?). You would have to very deliberately add a "$" for
who-knows-why. (Note: a domain name can't contain "$")
I'm not aware of the history of this normalize function, but I wouldn't expect
option names or value to contain spaces in practice. I suspect the code strips
out _all_ spaces instead of just spaces between option names and value is
because it's easier and faster to do text.replace(/ /g, "") than to split the
string up and remove spaces around each name and each value. If that's the case,
by all means let's just not care about spaces within option names and non-CSP
option values.

It's totally legit though to write a filter like "foo $ domain = bar.com " and
expect it to work.

Anyway, so if you feel strongly about this then I don't mind reverting to the
original text.replace(/ +/g, " ") though you'd be in a better position to say if
it would break anything (literally we'd be changing the output of the function
for potentially lots of actual filters out there).

[Manish Jethani, 2018/03/15 12:14:21]

To sum this up, the normalize function looks good to me as it is, though we
could optimize further, but that's your call.

I'm looking at the other parts of this patch now.

[kzar, 2018/03/15 13:13:33]

Thanks OK. (If you can see how to optimise the common code path further I'm
interested, but not for the csp code path unless it doesn't hurt readability
further.)

[Manish Jethani, 2018/03/15 13:47:33]

Acknowledged.

+
+  let options = optionsText.split(",");
+  for (let i = 0; i < options.length; i++)
+  {
+    let option = options[i];
+    let cspMatch = /^ *c *s *p *=/i.exec(option);
+    if (cspMatch)
+    {
+      options[i] = option.substring(0, cspMatch[0].length).replace(/ +/g, "") +

[Manish Jethani, 2018/03/15 08:23:17]

Well option.substring(0, cspMatch[0].length) is the same as cspMatch[0] so can
we skip the unnecessary substring here? We already have the string we need.

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

Done.

+                   option.substr(cspMatch[0].length).trim().replace(/ +/g, " ");

[Manish Jethani, 2018/03/15 08:23:17]

Any reason why this uses substr instead of substring? We need to pick a favorite
and stick with it unless necessary otherwise, and I think substring is the best
choice. substr wasn't even standardized until much later.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

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

substr and substring are the same unless the second parameter is passed, since
the second parameter wasn't passed here I went with the shorter option, which
seems consistent with the rest of this file.

I'm more worried about this use of substr[1] since it passes the second
parameter as an index, it just happens to work since the offset is always 0. But
you already disagreed that I should change that...

[1] -
https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.j...

+    }
+    else
+      options[i] = option.replace(/ +/g, "");
   }
-  return text.replace(/\s/g, "");
+
+  return beforeOptions + "$" + options.join();
 };
 
 /**
@@ -727,11 +757,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)
     {
@@ -742,12 +773,20 @@ RegExpFilter.fromText = function(text)
         value = option.substr(separatorIndex + 1);
         option = option.substr(0, separatorIndex);
       }
-      option = option.replace(/-/, "_");
+      option = option.replace(/-/, "_").toUpperCase();
       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 +799,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,7 +809,7 @@ 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");
     }
@@ -780,8 +819,19 @@ RegExpFilter.fromText = function(text)
   {
     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 +855,7 @@ RegExpFilter.typeMap = {
   DOCUMENT: 64,
   WEBSOCKET: 128,
   WEBRTC: 256,
+  CSP: 512,
   XBL: 1,
   PING: 1024,
   XMLHTTPREQUEST: 2048,
@@ -821,9 +872,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 +892,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 +916,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
 });
 
 /**