Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29680684: [$csp1 adblockpluscore] Issue 6329 - Allow whitespace in filter option values (Closed)

Created:
Jan. 26, 2018, 5:32 p.m. by kzar
Modified:
March 7, 2018, 5:59 a.m.
CC:
sergei
Visibility:
Public.

Description

Issue 6329 - Allow whitespace in filter option values This change is necessary for the addition of the $csp filter option since whitespace in Content Security Policies is significant. I've skipped (most of) the normalisation step, instead doing it at the point the filter is parsed. My key insight there was that we always seem to call Filter.fromText directly afterward the normalise function anyway. This makes the code a little harder to follow perhaps, but has the benefit that we don't wastefully parse element hiding filters during normalisation, only to repeat much of that work again in Filters.fromText. Note that normalisation is not identical as before for RegExpFilters, previously something like "example $ domain=foo.com" might have worked, but no longer.

Patch Set 1 #

Total comments: 22

Patch Set 2 : Addressed some of Manish's initial feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+66 lines, -50 lines) Patch
M lib/filterClasses.js View 1 6 chunks +37 lines, -49 lines 0 comments Download
M lib/synchronizer.js View 1 chunk +1 line, -1 line 0 comments Download
M test/filterClasses.js View 1 1 chunk +28 lines, -0 lines 0 comments Download

Messages

Total messages: 9
kzar
Patch Set 1
Jan. 26, 2018, 6:11 p.m. (2018-01-26 18:11:39 UTC) #1
Manish Jethani
https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js#newcode104 lib/filterClasses.js:104: * basic normalisation and parsing where possible before deferring ...
Feb. 5, 2018, 4:09 p.m. (2018-02-05 16:09:58 UTC) #2
Manish Jethani
https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js#newcode721 lib/filterClasses.js:721: let options = match[1].replace(/\s/g, ""); On 2018/02/05 16:09:58, Manish ...
Feb. 6, 2018, 5:14 a.m. (2018-02-06 05:14:39 UTC) #3
kzar
Patch Set 2 : Addressed some of Manish's initial feedback https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js#newcode104 ...
Feb. 13, 2018, 11:56 a.m. (2018-02-13 11:56:47 UTC) #4
Manish Jethani
Sorry about the delay. https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js#newcode111 lib/filterClasses.js:111: let filter = Filter.knownFilters.get(text); On ...
Feb. 20, 2018, 4 p.m. (2018-02-20 16:00:58 UTC) #5
Manish Jethani
https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js#newcode111 lib/filterClasses.js:111: let filter = Filter.knownFilters.get(text); On 2018/02/20 16:00:57, Manish Jethani ...
Feb. 21, 2018, 2:49 p.m. (2018-02-21 14:49:18 UTC) #6
kzar
https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js#newcode111 lib/filterClasses.js:111: let filter = Filter.knownFilters.get(text); On 2018/02/21 14:49:18, Manish Jethani ...
March 6, 2018, 2:30 p.m. (2018-03-06 14:30:38 UTC) #7
Manish Jethani
https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js#newcode111 lib/filterClasses.js:111: let filter = Filter.knownFilters.get(text); On 2018/03/06 14:30:38, kzar wrote: ...
March 7, 2018, 12:13 a.m. (2018-03-07 00:13:55 UTC) #8
Manish Jethani
March 7, 2018, 5:59 a.m. (2018-03-07 05:59:59 UTC) #9
Message was sent while issue was closed.
https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29680684/diff/29680685/lib/filterClasses.j...
lib/filterClasses.js:111: let filter = Filter.knownFilters.get(text);
On 2018/03/07 00:13:55, Manish Jethani wrote:

> How about adding these lines at the end of Filter.normalize:
>
> [snip]

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("");
  }

Powered by Google App Engine
This is Rietveld