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

Issue 29680696: [$csp4 adblockpluschrome] Issue 5241 - Add support for Content Security Policy filters (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 3 weeks ago by kzar
Modified:
2 months, 3 weeks ago
Reviewers:
Sebastian Noack
CC:
Manish Jethani, Thomas Greiner
Visibility:
Public.

Description

Issue 5241 - Add support for Content Security Policy filters Depends on: - https://codereview.adblockplus.org/29680684/ - https://codereview.adblockplus.org/29680689/ - https://codereview.adblockplus.org/29680693/

Patch Set 1 #

Total comments: 3

Patch Set 2 : Rebased and tidied a few things up #

Total comments: 19

Patch Set 3 : Get devtools logging working #

Total comments: 9

Patch Set 4 : Filed issue re normalisation of initiator URL #

Patch Set 5 : Rebased, ignored requests cached by ServiceWorkers and addressed Sebastian's feedback #

Total comments: 19

Patch Set 6 : Make use of stringifyUrl and remove unused WhitelistFilter #

Total comments: 4

Patch Set 7 : Get the parent frame's hostname where possible #

Total comments: 2

Patch Set 8 : Use extractHostFromFrame and attempt to match $csp exceptions non-recursively #

Total comments: 8

Patch Set 9 : Addressed Sebastian's feedback #

Total comments: 6

Patch Set 10 : Addressed some final nits #

Patch Set 11 : Rebased and updated adblockpluscore dependency #

Patch Set 12 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -49 lines) Patch
M _locales/en_US/messages.json View 1 1 chunk +3 lines, -0 lines 0 comments Download
M dependencies View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M lib/csp.js View 1 2 3 4 5 6 7 8 9 1 chunk +54 lines, -45 lines 0 comments Download
M lib/devtools.js View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M lib/requestBlocker.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 27
kzar
Patch Set 1 https://codereview.adblockplus.org/29680696/diff/29680697/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29680697/lib/csp.js#newcode31 lib/csp.js:31: // https://bugs.chromium.org/p/chromium/issues/detail?id=805649 This bug is also ...
4 months, 3 weeks ago (2018-01-26 18:12:37 UTC) #1
kzar
https://codereview.adblockplus.org/29680696/diff/29680697/_locales/en_US/messages.json File _locales/en_US/messages.json (right): https://codereview.adblockplus.org/29680696/diff/29680697/_locales/en_US/messages.json#newcode69 _locales/en_US/messages.json:69: "message": "Invalid Content Security Policy" This message is not ...
4 months, 3 weeks ago (2018-01-26 18:13:15 UTC) #2
kzar
Patch Set 2 : Rebased and tidied a few things up https://codereview.adblockplus.org/29680696/diff/29680697/_locales/en_US/messages.json File _locales/en_US/messages.json (right): ...
3 months, 1 week ago (2018-03-06 15:45:03 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js File lib/csp.js (left): https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#oldcode60 lib/csp.js:60: } Nit: Don't we omit braces anymore if there ...
3 months, 1 week ago (2018-03-06 19:43:25 UTC) #4
kzar
Patch Set 3 : Get devtools logging working (Adding Thomas to Cc since I mentioned ...
3 months, 1 week ago (2018-03-07 15:07:02 UTC) #5
Sebastian Noack
https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode32 lib/csp.js:32: // the URL matches the initiator's URL (once normalised). ...
3 months, 1 week ago (2018-03-07 17:27:07 UTC) #6
kzar
Patch Set 4 : Filed issue re normalisation of initiator URL https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js File lib/csp.js (right): ...
3 months, 1 week ago (2018-03-12 16:33:42 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js#newcode32 lib/csp.js:32: // the URL matches the initiator's URL (once normalised). ...
3 months, 1 week ago (2018-03-13 03:58:13 UTC) #8
kzar
Patch Set 5 : Rebased, ignored requests cached by ServiceWorkers and addressed Sebastian's feedback https://codereview.adblockplus.org/29680696/diff/29715749/lib/csp.js ...
3 months ago (2018-03-13 18:11:02 UTC) #9
Sebastian Noack
https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js#newcode35 lib/csp.js:35: let cspMatch = defaultMatcher.matchesAny(details.url, typeMap.CSP, hostname, It seems, we ...
3 months ago (2018-03-13 22:28:31 UTC) #10
kzar
Patch Set 6 : Make use of stringifyUrl and remove unused WhitelistFilter https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js File lib/csp.js ...
3 months ago (2018-03-14 10:46:43 UTC) #11
Sebastian Noack
https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js#newcode35 lib/csp.js:35: let cspMatch = defaultMatcher.matchesAny(details.url, typeMap.CSP, hostname, On 2018/03/14 10:46:43, ...
3 months ago (2018-03-14 15:57:28 UTC) #12
kzar
https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js#newcode35 lib/csp.js:35: let cspMatch = defaultMatcher.matchesAny(details.url, typeMap.CSP, hostname, On 2018/03/14 15:57:27, ...
3 months ago (2018-03-14 17:16:07 UTC) #13
Sebastian Noack
https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js#newcode41 lib/csp.js:41: if (checkWhitelisted(page, frame, typeMap.DOCUMENT | typeMap.CSP)) On 2018/03/14 17:16:07, ...
3 months ago (2018-03-14 20:19:43 UTC) #14
kzar
https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js#newcode41 lib/csp.js:41: if (checkWhitelisted(page, frame, typeMap.DOCUMENT | typeMap.CSP)) Sebastian Noack wrote: ...
3 months ago (2018-03-15 13:10:13 UTC) #15
Sebastian Noack
https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js#newcode41 lib/csp.js:41: if (checkWhitelisted(page, frame, typeMap.DOCUMENT | typeMap.CSP)) On 2018/03/15 13:10:13, ...
3 months ago (2018-03-15 17:14:25 UTC) #16
kzar
Patch Set 7 : Get the parent frame's hostname where possible https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js File lib/csp.js (right): ...
3 months ago (2018-03-15 18:18:44 UTC) #17
Sebastian Noack
https://codereview.adblockplus.org/29680696/diff/29723655/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29723655/lib/csp.js#newcode40 lib/csp.js:40: hostname = getDecodedHostname(parentFrame.url); Please use extractHostFromFrame().
3 months ago (2018-03-15 19:50:53 UTC) #18
kzar
Patch Set 8 : Use extractHostFromFrame and attempt to match $csp exceptions non-recursively https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js File ...
3 months ago (2018-03-16 12:54:59 UTC) #19
Sebastian Noack
https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js#newcode41 lib/csp.js:41: if (checkWhitelisted(page, frame, typeMap.DOCUMENT | typeMap.CSP)) On 2018/03/16 12:54:59, ...
3 months ago (2018-03-16 23:52:39 UTC) #20
kzar
Patch Set 9 : Addressed Sebastian's feedback https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js File lib/csp.js (right): https://codereview.adblockplus.org/29680696/diff/29721736/lib/csp.js#newcode41 lib/csp.js:41: if (checkWhitelisted(page, ...
3 months ago (2018-03-19 14:41:31 UTC) #21
Sebastian Noack
https://codereview.adblockplus.org/29680696/diff/29721736/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29680696/diff/29721736/metadata.chrome#newcode115 metadata.chrome:115: desktop-options.js = adblockplusui/js/desktop-options.js On 2018/03/14 10:46:43, kzar wrote: > ...
3 months ago (2018-03-19 22:25:12 UTC) #22
kzar
Patch Set 10 : Addressed some final nits https://codereview.adblockplus.org/29680696/diff/29721736/metadata.chrome File metadata.chrome (right): https://codereview.adblockplus.org/29680696/diff/29721736/metadata.chrome#newcode115 metadata.chrome:115: desktop-options.js ...
3 months ago (2018-03-20 09:02:31 UTC) #23
Sebastian Noack
Looks good to me, once rebased on top of the adblockplusui dependency update, and the ...
2 months, 4 weeks ago (2018-03-20 23:23:21 UTC) #24
kzar
Patch Set 11 : Rebased and updated adblockpluscore dependency (Still waiting for the adblockplusui dependency ...
2 months, 3 weeks ago (2018-03-22 16:50:38 UTC) #25
kzar
Rebased
2 months, 3 weeks ago (2018-03-26 18:23:47 UTC) #26
Sebastian Noack
2 months, 3 weeks ago (2018-03-27 00:15:00 UTC) #27
LGTM
Sign in to reply to this message.

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