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

Issue 29680689: [$csp2 adblockpluscore] Issue 6329 - Add the CSP filter type (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 10 months ago by kzar
Modified:
1 year, 8 months ago
CC:
sergei
Visibility:
Public.

Description

Issue 6329 - Add the CSP filter type See also the (now closed) review[1] which proceeded this. [1] - https://codereview.adblockplus.org/29680684/

Patch Set 1 #

Total comments: 6

Patch Set 2 : Rebased, brought back Filter.normalize, other changes #

Patch Set 3 : Check for report-to and report-uri directives more carefully #

Total comments: 4

Patch Set 4 : Fully normalise whitespace, avoid strict equality #

Total comments: 20

Patch Set 5 : New approach, added more unit tests #

Patch Set 6 : Use space literal instead of \s where possible #

Total comments: 10

Patch Set 7 : Strip whitespace in batches where possible #

Patch Set 8 : Addressed some of Manish's feedback #

Total comments: 25

Patch Set 9 : Addressed some more nits #

Total comments: 21

Patch Set 10 : Prevent the referrer directive #

Total comments: 2

Patch Set 11 : Added test case for case insensitive forbidden directives #

Patch Set 12 : Improve comments #

Total comments: 14

Patch Set 13 : Added more tests, blacklisted directives and added a tiny optimisation #

Total comments: 2

Patch Set 14 : Go with Manish's do ... while ... suggestion #

Total comments: 2

Patch Set 15 : Removed duplicate base-uri directive from blacklist #

Total comments: 7

Patch Set 16 : Addressed Sebastian's feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+158 lines, -28 lines) Patch
M lib/filterClasses.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 11 chunks +83 lines, -22 lines 0 comments Download
M test/filterClasses.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 7 chunks +75 lines, -6 lines 0 comments Download

Messages

Total messages: 58
kzar
Patch Set 1
1 year, 10 months ago (2018-01-26 18:11:53 UTC) #1
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29680690/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29680690/lib/filterClasses.js#newcode758 lib/filterClasses.js:758: csp = value.toLowerCase(); We could avoid uppercasing and then ...
1 year, 10 months ago (2018-02-06 05:54:20 UTC) #2
kzar
Patch Set 2 : Rebased, brought back Filter.normalize, other changes Patch Set 3 : Check ...
1 year, 9 months ago (2018-03-06 15:37:53 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29715744/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29715744/lib/filterClasses.js#newcode746 lib/filterClasses.js:746: if (option == "CSP") I've left a comment on ...
1 year, 9 months ago (2018-03-07 00:22:13 UTC) #4
kzar
Patch Set 4 : Fully normalise whitespace, avoid strict equality https://codereview.adblockplus.org/29680689/diff/29715744/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29715744/lib/filterClasses.js#newcode746 ...
1 year, 9 months ago (2018-03-12 13:35:59 UTC) #5
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js#newcode192 lib/filterClasses.js:192: let strippedText = text.replace(/\s/g, ""); Since we have already ...
1 year, 9 months ago (2018-03-12 16:34:53 UTC) #6
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js#newcode190 lib/filterClasses.js:190: // For most regexp filters we strip all whitespace, ...
1 year, 9 months ago (2018-03-12 18:53:59 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js#newcode190 lib/filterClasses.js:190: // For most regexp filters we strip all whitespace, ...
1 year, 9 months ago (2018-03-12 23:19:02 UTC) #8
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js#newcode190 lib/filterClasses.js:190: // For most regexp filters we strip all whitespace, ...
1 year, 9 months ago (2018-03-13 06:59:30 UTC) #9
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js#newcode190 lib/filterClasses.js:190: // For most regexp filters we strip all whitespace, ...
1 year, 9 months ago (2018-03-13 07:35:00 UTC) #10
kzar
Patch Set 5 : New approach, added more unit tests Patch Set 6 : Use ...
1 year, 9 months ago (2018-03-14 13:54:38 UTC) #11
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29722732/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29722732/lib/filterClasses.js#newcode187 lib/filterClasses.js:187: return domain.replace(/ /g, "") + separator + selector.trim(); By ...
1 year, 9 months ago (2018-03-14 15:27:31 UTC) #12
kzar
Patch Set 7 : Strip whitespace in batches where possible https://codereview.adblockplus.org/29680689/diff/29722732/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29722732/lib/filterClasses.js#newcode187 ...
1 year, 9 months ago (2018-03-14 17:28:59 UTC) #13
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js#newcode196 lib/filterClasses.js:196: let optionsMatch = Filter.optionsRegExp.exec(strippedText); On 2018/03/14 13:54:38, kzar wrote: ...
1 year, 9 months ago (2018-03-14 18:10:50 UTC) #14
kzar
Patch Set 8 : Addressed some of Manish's feedback https://codereview.adblockplus.org/29680689/diff/29722732/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29722732/lib/filterClasses.js#newcode192 lib/filterClasses.js:192: ...
1 year, 9 months ago (2018-03-14 19:48:25 UTC) #15
Sebastian Noack
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js#newcode190 lib/filterClasses.js:190: // For most regexp filters we strip all whitespace, ...
1 year, 8 months ago (2018-03-14 20:59:21 UTC) #16
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js#newcode179 lib/filterClasses.js:179: if (/^ *!/.test(text)) On 2018/03/14 20:59:20, Sebastian Noack wrote: ...
1 year, 8 months ago (2018-03-15 08:09:59 UTC) #17
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js#newcode192 lib/filterClasses.js:192: if (!strippedText.includes("$") || !/csp=/i.test(strippedText)) Can we make this /\bcsp=/i ...
1 year, 8 months ago (2018-03-15 08:23:17 UTC) #18
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js#newcode204 lib/filterClasses.js:204: optionsText = optionsText.substr(optionsText.indexOf("$") + 1); On 2018/03/15 08:09:59, Manish ...
1 year, 8 months ago (2018-03-15 09:11:07 UTC) #19
kzar
Patch Set 9 : Addressed some more nits https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js#newcode190 lib/filterClasses.js:190: // ...
1 year, 8 months ago (2018-03-15 10:26:26 UTC) #20
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js#newcode190 lib/filterClasses.js:190: // For most regexp filters we strip all whitespace, ...
1 year, 8 months ago (2018-03-15 10:42:42 UTC) #21
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode202 lib/filterClasses.js:202: let optionsText = text; By the way, I was ...
1 year, 8 months ago (2018-03-15 11:13:26 UTC) #22
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js#newcode204 lib/filterClasses.js:204: optionsText = optionsText.substr(optionsText.indexOf("$") + 1); On 2018/03/15 10:42:42, Manish ...
1 year, 8 months ago (2018-03-15 11:25:48 UTC) #23
kzar
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js#newcode190 lib/filterClasses.js:190: // For most regexp filters we strip all whitespace, ...
1 year, 8 months ago (2018-03-15 11:38:50 UTC) #24
kzar
https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js#newcode204 lib/filterClasses.js:204: optionsText = optionsText.substr(optionsText.indexOf("$") + 1); On 2018/03/15 11:25:47, Manish ...
1 year, 8 months ago (2018-03-15 11:43:53 UTC) #25
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js#newcode190 lib/filterClasses.js:190: // For most regexp filters we strip all whitespace, ...
1 year, 8 months ago (2018-03-15 12:00:12 UTC) #26
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js#newcode204 lib/filterClasses.js:204: optionsText = optionsText.substr(optionsText.indexOf("$") + 1); On 2018/03/15 12:00:11, Manish ...
1 year, 8 months ago (2018-03-15 12:14:21 UTC) #27
kzar
https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js#newcode204 lib/filterClasses.js:204: optionsText = optionsText.substr(optionsText.indexOf("$") + 1); On 2018/03/15 12:14:21, Manish ...
1 year, 8 months ago (2018-03-15 13:13:34 UTC) #28
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode829 lib/filterClasses.js:829: if (/(;|^)\s*report-(to|uri)\b/.test(csp)) Since we allow single spaces in the ...
1 year, 8 months ago (2018-03-15 13:24:45 UTC) #29
kzar
https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode829 lib/filterClasses.js:829: if (/(;|^)\s*report-(to|uri)\b/.test(csp)) On 2018/03/15 13:24:44, Manish Jethani wrote: > ...
1 year, 8 months ago (2018-03-15 13:44:46 UTC) #30
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29722910/lib/filterClasses.js#newcode204 lib/filterClasses.js:204: optionsText = optionsText.substr(optionsText.indexOf("$") + 1); On 2018/03/15 13:13:33, kzar ...
1 year, 8 months ago (2018-03-15 13:47:34 UTC) #31
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode829 lib/filterClasses.js:829: if (/(;|^)\s*report-(to|uri)\b/.test(csp)) On 2018/03/15 13:47:33, Manish Jethani wrote: > ...
1 year, 8 months ago (2018-03-15 13:50:48 UTC) #32
kzar
https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode829 lib/filterClasses.js:829: if (/(;|^)\s*report-(to|uri)\b/.test(csp)) On 2018/03/15 13:47:33, Manish Jethani wrote: > ...
1 year, 8 months ago (2018-03-15 14:13:38 UTC) #33
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode829 lib/filterClasses.js:829: if (/(;|^)\s*report-(to|uri)\b/.test(csp)) On 2018/03/15 14:13:38, kzar wrote: > On ...
1 year, 8 months ago (2018-03-15 14:28:32 UTC) #34
kzar
On 2018/03/15 14:28:32, Manish Jethani wrote: > Referrer-Policy doesn't really concern us since we're not ...
1 year, 8 months ago (2018-03-15 15:55:23 UTC) #35
kzar
Patch Set 10 : Prevent the referrer directive https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode828 lib/filterClasses.js:828: // ...
1 year, 8 months ago (2018-03-15 16:09:47 UTC) #36
Manish Jethani
Looks good so far provided we decide to blacklist the three directives rather than whitelist ...
1 year, 8 months ago (2018-03-15 16:55:34 UTC) #37
Sebastian Noack
https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29720596/lib/filterClasses.js#newcode190 lib/filterClasses.js:190: // For most regexp filters we strip all whitespace, ...
1 year, 8 months ago (2018-03-15 17:25:41 UTC) #38
Sebastian Noack
https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode829 lib/filterClasses.js:829: if (/(;|^)\s*report-(to|uri)\b/.test(csp)) On 2018/03/15 14:13:38, kzar wrote: > I ...
1 year, 8 months ago (2018-03-15 19:03:05 UTC) #39
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode202 lib/filterClasses.js:202: let optionsText = text; On 2018/03/15 11:13:25, Manish Jethani ...
1 year, 8 months ago (2018-03-16 06:47:10 UTC) #40
Manish Jethani
The tests look good to me. https://codereview.adblockplus.org/29680689/diff/29723635/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723635/test/filterClasses.js#newcode317 test/filterClasses.js:317: compareFilter(test, "bla$csp=report-uri", ["type=invalid", ...
1 year, 8 months ago (2018-03-16 08:54:41 UTC) #41
kzar
Patch Set 11 : Added test case for case insensitive forbidden directives Patch Set 12 ...
1 year, 8 months ago (2018-03-16 11:29:06 UTC) #42
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode202 lib/filterClasses.js:202: let optionsText = text; On 2018/03/16 11:29:05, kzar wrote: ...
1 year, 8 months ago (2018-03-16 12:05:06 UTC) #43
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode829 lib/filterClasses.js:829: if (/(;|^)\s*report-(to|uri)\b/.test(csp)) On 2018/03/15 19:03:05, Sebastian Noack wrote: > ...
1 year, 8 months ago (2018-03-16 12:25:06 UTC) #44
Manish Jethani
LGTM
1 year, 8 months ago (2018-03-16 12:53:15 UTC) #45
Manish Jethani
On 2018/03/16 12:53:15, Manish Jethani wrote: > LGTM Sorry, I found another one to blacklist: ...
1 year, 8 months ago (2018-03-16 15:14:10 UTC) #46
Manish Jethani
On 2018/03/16 15:14:10, Manish Jethani wrote: > On 2018/03/16 12:53:15, Manish Jethani wrote: > > ...
1 year, 8 months ago (2018-03-16 15:28:04 UTC) #47
sergei
https://codereview.adblockplus.org/29680689/diff/29724561/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29724561/lib/filterClasses.js#newcode176 lib/filterClasses.js:176: text = text.replace(/[^\S ]+/g, ""); This change (including the ...
1 year, 8 months ago (2018-03-16 16:49:26 UTC) #48
kzar
Patch Set 13 : Added more tests, blacklisted directives and added a tiny optimisation > ...
1 year, 8 months ago (2018-03-17 14:35:08 UTC) #49
sergei
https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode202 lib/filterClasses.js:202: let optionsText = text; On 2018/03/17 14:35:06, kzar wrote: ...
1 year, 8 months ago (2018-03-19 15:47:32 UTC) #50
kzar
Patch Set 14 : Go with Manish's do ... while ... suggestion https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js ...
1 year, 8 months ago (2018-03-19 16:38:59 UTC) #51
Manish Jethani
https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29723593/lib/filterClasses.js#newcode202 lib/filterClasses.js:202: let optionsText = text; On 2018/03/17 14:35:06, kzar wrote: ...
1 year, 8 months ago (2018-03-19 18:03:40 UTC) #52
kzar
Patch Set 15 : Removed duplicate base-uri directive from blacklist https://codereview.adblockplus.org/29680689/diff/29727600/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29727600/lib/filterClasses.js#newcode107 ...
1 year, 8 months ago (2018-03-19 18:16:22 UTC) #53
Manish Jethani
LGTM
1 year, 8 months ago (2018-03-19 18:20:34 UTC) #54
Sebastian Noack
https://codereview.adblockplus.org/29680689/diff/29727620/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29727620/lib/filterClasses.js#newcode108 lib/filterClasses.js:108: ]); I don't get why we create this Set ...
1 year, 8 months ago (2018-03-21 16:23:14 UTC) #55
kzar
Patch Set 16 : Addressed Sebastian's feedback https://codereview.adblockplus.org/29680689/diff/29727620/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29680689/diff/29727620/lib/filterClasses.js#newcode108 lib/filterClasses.js:108: ]); On ...
1 year, 8 months ago (2018-03-21 16:58:12 UTC) #56
Sebastian Noack
LGTM
1 year, 8 months ago (2018-03-21 18:07:25 UTC) #57
Manish Jethani
1 year, 8 months ago (2018-03-22 08:05:03 UTC) #58
LGTM
Sign in to reply to this message.

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