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

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 83 matching lines @@
  */
 Filter.regexpRegExp = /^(@@)?\/.*\/(?:\$~?[\w-]+(?:=[^,\s]+)?(?:,~?[\w-]+(?:=[^,\s]+)?)*)?$/;
 /**
  * Regular expression that options on a RegExp filter should match
  * @type {RegExp}
  */
 Filter.optionsRegExp = /\$(~?[\w-]+(?:=[^,]+)?(?:,~?[\w-]+(?:=[^,]+)?)*)$/;
 
 /**
  * Creates a filter of correct type from its text representation - does the
- * 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.

+ * basic normalization and parsing where possible before deferring to the right
  * constructor.
  * @param {string} text   as in Filter()
  * @return {Filter}
  */
 Filter.fromText = function(text)
 {
   let filter = Filter.knownFilters.get(text);

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

This means that two otherwise identical filters (post normalization) could be
treated as different filters after this change.

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

Any ideas? This whole change was a pain, perhaps there is a better approach
which I missed.

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

How about leaving the functions as they are, but changing the implementations a
little bit? For example, Filter.normalize could mostly work as it does, but we
could add an additional condition for "$csp=".

Pseudocode:

  Filter.normalize = function(text)
  {
    ...
    else if (textContainsCSPOption)
    {
      stripAllSpacesExceptSpacesInCSPOption;
    }
    return text.replace(/\s/g, "");
  }

So if there's a "$csp=" option in the text then it'll preserve the spaces within
that option's value, otherwise it will strip all spaces.

[Manish Jethani, 2018/02/21 14:49:18]

I meant of course:

    ...
    else if (textContainsCSPOption)
    {
      return stripAllSpacesExceptSpacesInCSPOption; /* return statement */
    }
    return text.replace(/\s/g, "");
  }

[kzar, 2018/03/06 14:30:38]

Well while I agree that doing filter normalisation as a separate step is
probably the right option (filter caching etc) I don't think your suggestion is
practical - I mean forget all this, how about we implement the pseudocode of
`blockAllAdverts(website); /* block adverts */` instead?

So far we strip all whitespace from the filters, so let's take an example
filter:

    whatev   er$  c  s P =csp,  do  main=   adblockplus.org

or another one

    whatev   er$  c  s p =csp,,

We don't know if the part following the '$' is a valid options string until we
have stripped the spaces and checked if the regexp matches. Nor do we know if
any of those options are content security policies until the whitespace is
stripped. We also don't want to do any more work than necessary in the (by far)
common case that a filter doesn't contain a $csp option.

So to do what you suggest I figure we'd have to do something like:

 1. Create a copy of the filter with all whitespace stripped.
 2. If /csp=/i doesn't match simply return that stripped copy, otherwise...
 3. Go back and figure out the positions where one or more whitespaces were
stripped relative to the stripped copy.
 4. See if the options regexp matches the stripped string, if so...
 5. Figure out the positions of all the $csp option values in the stripped copy.
 6. For each position that falls within a $csp option value check if some
whitespace was stripped there, if so insert a space back in.
 7. Finally return the stripped copy with the $csp spaces restored!

I don't think that's practical, even a rough implementation which took us to
number 5 was becoming unwieldy. Therefore I propose that instead we trim any
surrounding whitespace from the filter and replace whitespace with single
spaces. That's not as good sure, but its a whole lot simpler and I figure it's
good enough since;

 1.) The `Filter.normalize` function does not fully normalise filters anyway
(e.g. `foo$domain=FOO.COM` vs `foo$domain=foo.com`).
 2.) We can still trim / strip all whitespace from filter values and the regexp
part at the time of parsing, the same as how options are upper cased.
 3.) I think that in practice it's hard to do things like `foo$ c s p= bar` by
mistake. Trailing whitespace like `foo$csp=bar   ` sure but we'd still handle
that. (In my testing it works in practice too, at least with EasyList and AA.)

So anyway this comment is already far too long, sorry about that. I will close
this review and update the next one[1] to also include my suggested tweak to
`Filter.normalize`.

[1] - https://codereview.adblockplus.org/29680689/

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

You know my suggestion was way more specific than "blockAllAdverts" :)

How about adding these lines at the end of Filter.normalize:

  let {index, 1: options} = optionsWithSpacesRegExp.exec(text) || {};
  if (options)
  {
    return text.substring(0, index).replace(/ /g, "") + "$" +
           options.split(",").map(normalizeOption).join(",");
  }

  return text.replace(/ /g, "");

optionsWithSpacesRegExp and normalizeOption are defined as follows:

  const optionsWithSpacesRegExp =
    /\$( *~? ?[ \w-]+(?:=[^,]+)?(?:, *~? ?[ \w-]+(?:=[^,]+)?)*)$/;

  const normalizeOption = option =>
  {
    let [, name, value] = /(.*?)(=.*)/.exec(option) || [, option, ""];
    name = name.replace(/ /g, "");
    if (name.toUpperCase() != "CSP")
      value = value.replace(/ /g, "");
    return name + value;
  };

We could also keep the uppercasing here (rather change it to lowercasing) so we
don't have to do it in other places.

What do you think?

I do like the idea of keeping this patch so that the other one remains easier to
review.

I really don't think we have to change the behavior of Filter.normalize at all
with respect to non-CSP filters (i.e. all filters that are currently supported).

We could easily just test for "csp=" first as an optimization.

i.e.

  let {index, 1: options} = /c *s *p *=/i.test(text) &&
                            optionsWithSpacesRegExp.exec(text) || {};

[Manish Jethani, 2018/03/07 05:59:58]

Another way to do it is by splitting on "csp=":

  let index = text.indexOf("$");
  if (index >= 0)
  {
    let fragments = text.substring(index + 1)
                    .split(/((?:^|,) *c *s *p *=)([^,]+)/i);
    let csp = false;
    for (let i = 0; i < fragments.length; i++)
    {
      if (csp)
      {
        csp = false;
      }
      else
      {
        fragments[i] = fragments[i].replace(/ +/g, "");
        csp = /^,?csp=$/i.test(fragments[i]);
      }
    }

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

   if (filter)
     return filter;
 
   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();
@@ skipping 579 matching lines @@
  * @return {Filter}
  */
 RegExpFilter.fromText = function(text)
 {
   let contentType = null;
   let matchCase = null;
   let domains = null;
   let sitekeys = null;
   let thirdParty = null;
   let collapse = null;
-  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 origText = null;
 
   let match = (text.indexOf("$") >= 0 ? Filter.optionsRegExp.exec(text) : null);
   if (!match)
   {
     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.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)
       {
         if (contentType == null)
           contentType = 0;
         contentType |= RegExpFilter.typeMap[option];
       }
       else if (option[0] == "~" && option.substr(1) in RegExpFilter.typeMap)
       {
@@ skipping 27 matching lines @@
     if (text.indexOf("@@") == 0)
     {
       return new WhitelistFilter(origText, text.substr(2), contentType,
                                  matchCase, domains, thirdParty, sitekeys);
     }
     return new BlockingFilter(origText, text, contentType, matchCase, domains,
                               thirdParty, sitekeys, collapse);
   }
   catch (e)
   {
     return new InvalidFilter(origText, "filter_invalid_regexp");

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

origText is no longer the original text so it has basically lost its meaning. On
the other hand it's not very useful either. Maybe we can get rid of the extra
variable?

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

The point of `origText` is to show a more useful warning to the user when they
have added an invalid filter. It's still useful for that purpose, though I agree
the name sucks. I'm open to suggestions there.

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

What if we keep origText pointing to text as it was passed in (with no
modifications)?

   }
 };
 
 /**
  * Maps type strings like "SCRIPT" or "OBJECT" to bit masks
  */
 RegExpFilter.typeMap = {
   OTHER: 1,
   SCRIPT: 2,
   IMAGE: 4,
@@ skipping 217 matching lines @@
  */
 function ElemHideEmulationFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
 ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
   type: "elemhideemulation"
 });