Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(515)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months, 3 weeks ago by kzar
Modified:
7 months, 2 weeks ago
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
8 months, 3 weeks ago (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 ...
8 months, 1 week ago (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 ...
8 months, 1 week ago (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 ...
8 months ago (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 ...
7 months, 4 weeks ago (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 ...
7 months, 3 weeks ago (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 ...
7 months, 2 weeks ago (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: ...
7 months, 2 weeks ago (2018-03-07 00:13:55 UTC) #8
Manish Jethani
7 months, 2 weeks ago (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("");
  }
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5